| Message ID | 1573816886-2743-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter: nf_tables: check the bind callback failed and unbind callback if hook register failed | expand |
On Fri, Nov 15, 2019 at 07:21:26PM +0800, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > Undo the callback binding before unregistering the existing hooks. It also > should check err of the bind setup call > > Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support") > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > This patch is based on: > http://patchwork.ozlabs.org/patch/1195539/ This is actually like this one: https://patchwork.ozlabs.org/patch/1194046/ right? > net/netfilter/nf_tables_api.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 0f8080e..149de13 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -6001,12 +6001,20 @@ static int nft_register_flowtable_net_hooks(struct net *net, > } > } > > - flowtable->data.type->setup(&flowtable->data, hook->ops.dev, > - FLOW_BLOCK_BIND); > - err = nf_register_net_hook(net, &hook->ops); > + err = flowtable->data.type->setup(&flowtable->data, > + hook->ops.dev, > + FLOW_BLOCK_BIND); I'd rather not check for the return value. ->setup returns 0 unless you use anything else than FLOW_BLOCK_BIND or _UNBIND. Probably better turn nf_flow_table_block_setup void and add WARN_ON_ONCE() there.
On Fri, Nov 15, 2019 at 12:25:01PM +0100, Pablo Neira Ayuso wrote: > On Fri, Nov 15, 2019 at 07:21:26PM +0800, wenxu@ucloud.cn wrote: > > From: wenxu <wenxu@ucloud.cn> > > > > Undo the callback binding before unregistering the existing hooks. It also > > should check err of the bind setup call > > > > Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support") > > Signed-off-by: wenxu <wenxu@ucloud.cn> > > --- > > This patch is based on: > > http://patchwork.ozlabs.org/patch/1195539/ > > This is actually like this one: > > https://patchwork.ozlabs.org/patch/1194046/ > > right? I'm attaching the one I made based on yours that I posted yesterday.
在 2019/11/15 19:25, Pablo Neira Ayuso 写道: > >> net/netfilter/nf_tables_api.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c >> index 0f8080e..149de13 100644 >> --- a/net/netfilter/nf_tables_api.c >> +++ b/net/netfilter/nf_tables_api.c >> @@ -6001,12 +6001,20 @@ static int nft_register_flowtable_net_hooks(struct net *net, >> } >> } >> >> - flowtable->data.type->setup(&flowtable->data, hook->ops.dev, >> - FLOW_BLOCK_BIND); >> - err = nf_register_net_hook(net, &hook->ops); >> + err = flowtable->data.type->setup(&flowtable->data, >> + hook->ops.dev, >> + FLOW_BLOCK_BIND); > I'd rather not check for the return value. ->setup returns 0 unless > you use anything else than FLOW_BLOCK_BIND or _UNBIND. Probably better > turn nf_flow_table_block_setup void and add WARN_ON_ONCE() there. If BIND failed. It means hw-offload failed. But the flowtable is set as hw-offload. Maybe it is not too make sense?
在 2019/11/15 19:27, Pablo Neira Ayuso 写道: > On Fri, Nov 15, 2019 at 12:25:01PM +0100, Pablo Neira Ayuso wrote: >> On Fri, Nov 15, 2019 at 07:21:26PM +0800, wenxu@ucloud.cn wrote: >>> From: wenxu <wenxu@ucloud.cn> >>> >>> Undo the callback binding before unregistering the existing hooks. It also >>> should check err of the bind setup call >>> >>> Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support") >>> Signed-off-by: wenxu <wenxu@ucloud.cn> >>> --- >>> This patch is based on: >>> http://patchwork.ozlabs.org/patch/1195539/ >> This is actually like this one: >> >> https://patchwork.ozlabs.org/patch/1194046/ >> >> right? > I'm attaching the one I made based on yours that I posted yesterday. yes it is include this one.
On Fri, Nov 15, 2019 at 07:37:53PM +0800, wenxu wrote: > > 在 2019/11/15 19:25, Pablo Neira Ayuso 写道: > > > >> net/netfilter/nf_tables_api.c | 14 +++++++++++--- > >> 1 file changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > >> index 0f8080e..149de13 100644 > >> --- a/net/netfilter/nf_tables_api.c > >> +++ b/net/netfilter/nf_tables_api.c > >> @@ -6001,12 +6001,20 @@ static int nft_register_flowtable_net_hooks(struct net *net, > >> } > >> } > >> > >> - flowtable->data.type->setup(&flowtable->data, hook->ops.dev, > >> - FLOW_BLOCK_BIND); > >> - err = nf_register_net_hook(net, &hook->ops); > >> + err = flowtable->data.type->setup(&flowtable->data, > >> + hook->ops.dev, > >> + FLOW_BLOCK_BIND); > > I'd rather not check for the return value. ->setup returns 0 unless > > you use anything else than FLOW_BLOCK_BIND or _UNBIND. Probably better > > turn nf_flow_table_block_setup void and add WARN_ON_ONCE() there. > > If BIND failed. It means hw-offload failed. But the flowtable is set as hw-offload. > > Maybe it is not too make sense? Oh right. Your patch: https://patchwork.ozlabs.org/patch/1195554/ is actually better. I'll take this and I'll drop mine. Thanks.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 0f8080e..149de13 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -6001,12 +6001,20 @@ static int nft_register_flowtable_net_hooks(struct net *net, } } - flowtable->data.type->setup(&flowtable->data, hook->ops.dev, - FLOW_BLOCK_BIND); - err = nf_register_net_hook(net, &hook->ops); + err = flowtable->data.type->setup(&flowtable->data, + hook->ops.dev, + FLOW_BLOCK_BIND); if (err < 0) goto err_unregister_net_hooks; + err = nf_register_net_hook(net, &hook->ops); + if (err < 0) { + flowtable->data.type->setup(&flowtable->data, + hook->ops.dev, + FLOW_BLOCK_UNBIND); + goto err_unregister_net_hooks; + } + i++; }