[nf-next PATCH v3 00/16] Dynamic hook interface binding

[nf-next PATCH v3 00/16] Dynamic hook interface binding

[Phil Sutter]

Changes since v2:
- Practically complete rewrite with wildcard interface spec support

The first two patches of this series are fixes to existing code but
cause conflicts if not applied in order. They may go into nf tree as
well, though only the first one is a real bug and seems to be of low
impact.

The next three patches introduce external storing of the user-supplied
interface name in nft_hook structs to decouple code from values in
->ops.dev or ->ops value in general.

Patch 6 eliminates a quirk in netdev-family chain netdev event handler,
aligns behaviour with flowtables and paves the way for following
changes.

Patches 7-10 prepare for and implement nf_hook_ops lists in nft_hook
objects. This is crucial for wildcard interface specs and convenient
with dynamic netdev hook registration upon NETDEV_REGISTER events.

Patches 11-13 leverage the new infrastructure to correctly handle
NETDEV_REGISTER and NETDEV_CHANGENAME events.

Patch 14 prepares the code for non-NUL-terminated interface names passed
by user space which resemble prefixes to match on. As a side-effect,
hook allocation code becomes tolerant to non-matching interface specs.

The final two patches implement netlink notifications for netdev
add/remove events and add a kselftest.

Phil Sutter (16):
  netfilter: nf_tables: Keep deleted flowtable hooks until after RCU
  netfilter: nf_tables: Flowtable hook's pf value never varies
  netfilter: nf_tables: Store user-defined hook ifname
  netfilter: nf_tables: Use stored ifname in netdev hook dumps
  netfilter: nf_tables: Compare netdev hooks based on stored name
  netfilter: nf_tables: Tolerate chains with no remaining hooks
  netfilter: nf_tables: Introduce functions freeing nft_hook objects
  netfilter: nf_tables: Introduce nft_hook_find_ops()
  netfilter: nf_tables: Introduce nft_register_flowtable_ops()
  netfilter: nf_tables: Have a list of nf_hook_ops in nft_hook
  netfilter: nf_tables: chain: Respect NETDEV_REGISTER events
  netfilter: nf_tables: flowtable: Respect NETDEV_REGISTER events
  netfilter: nf_tables: Handle NETDEV_CHANGENAME events
  netfilter: nf_tables: Support wildcard netdev hook specs
  netfilter: nf_tables: Add notications for hook changes
  selftests: netfilter: Torture nftables netdev hooks

 include/linux/netfilter.h                     |   2 +
 include/net/netfilter/nf_tables.h             |  11 +-
 include/uapi/linux/netfilter/nf_tables.h      |   5 +
 net/netfilter/nf_tables_api.c                 | 386 +++++++++++++-----
 net/netfilter/nf_tables_offload.c             |  51 ++-
 net/netfilter/nft_chain_filter.c              |  64 +--
 net/netfilter/nft_flow_offload.c              |   2 +-
 .../testing/selftests/net/netfilter/Makefile  |   1 +
 .../net/netfilter/nft_interface_stress.sh     | 149 +++++++
 9 files changed, 508 insertions(+), 163 deletions(-)
 create mode 100755 tools/testing/selftests/net/netfilter/nft_interface_stress.sh

-- 
2.43.0

[nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Phil Sutter]

Documentation of list_del_rcu() warns callers to not immediately free
the deleted list item. While it seems not necessary to use the
RCU-variant of list_del() here in the first place, doing so seems to
require calling kfree_rcu() on the deleted item as well.

Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b6547fe22bd8..2982f49b6d55 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
 		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
 					    FLOW_BLOCK_UNBIND);
 		list_del_rcu(&hook->list);
-		kfree(hook);
+		kfree_rcu(hook, rcu);
 	}
 	kfree(flowtable->name);
 	module_put(flowtable->data.type->owner);
-- 
2.43.0

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Documentation of list_del_rcu() warns callers to not immediately free
> the deleted list item. While it seems not necessary to use the
> RCU-variant of list_del() here in the first place, doing so seems to
> require calling kfree_rcu() on the deleted item as well.
> 
> Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b6547fe22bd8..2982f49b6d55 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
>  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
>  					    FLOW_BLOCK_UNBIND);
>  		list_del_rcu(&hook->list);
> -		kfree(hook);
> +		kfree_rcu(hook, rcu);
>  	}
>  	kfree(flowtable->name);
>  	module_put(flowtable->data.type->owner);

AFAICS its safe to use list_del() everywhere, I can't find a single
instance where the hooks are iterated without mutex serialization.

nf_tables_flowtable_destroy() is called after the hook has been
unregisted (detached from nf_hook list) and rcu grace period elapsed.

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Phil Sutter]

On Thu, Sep 12, 2024 at 03:32:55PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Documentation of list_del_rcu() warns callers to not immediately free
> > the deleted list item. While it seems not necessary to use the
> > RCU-variant of list_del() here in the first place, doing so seems to
> > require calling kfree_rcu() on the deleted item as well.
> > 
> > Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nf_tables_api.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index b6547fe22bd8..2982f49b6d55 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
> >  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
> >  					    FLOW_BLOCK_UNBIND);
> >  		list_del_rcu(&hook->list);
> > -		kfree(hook);
> > +		kfree_rcu(hook, rcu);
> >  	}
> >  	kfree(flowtable->name);
> >  	module_put(flowtable->data.type->owner);
> 
> AFAICS its safe to use list_del() everywhere, I can't find a single
> instance where the hooks are iterated without mutex serialization.
> 
> nf_tables_flowtable_destroy() is called after the hook has been
> unregisted (detached from nf_hook list) and rcu grace period elapsed.

Yes, I didn't find a caller which didn't synchronize_rcu() before
calling it. Same applies to chain hooks, right? I'd drop all the _rcu()
calls and give it a try, but the resulting race conditions may be hard
to trigger.

Cheers, Phil

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > nf_tables_flowtable_destroy() is called after the hook has been
> > unregisted (detached from nf_hook list) and rcu grace period elapsed.
> 
> Yes, I didn't find a caller which didn't synchronize_rcu() before
> calling it. Same applies to chain hooks, right?

Sigh, there is nft_flowtable_find_dev() which iterates the nft_hook
list from packet path.

So the syncrhonize_rcu is irrelevant as long as the entry
is linked up and this patch is correct as-is.

 list_del_rcu(&hook->list);
 kfree(hook);

is illegal, and I think this should add a helper that unlinks
and then frees the entry via kfree_rcu and converts all instances
of this pattern.

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Pablo Neira Ayuso]

On Thu, Sep 12, 2024 at 03:32:55PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Documentation of list_del_rcu() warns callers to not immediately free
> > the deleted list item. While it seems not necessary to use the
> > RCU-variant of list_del() here in the first place, doing so seems to
> > require calling kfree_rcu() on the deleted item as well.
> > 
> > Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nf_tables_api.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index b6547fe22bd8..2982f49b6d55 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
> >  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
> >  					    FLOW_BLOCK_UNBIND);
> >  		list_del_rcu(&hook->list);
> > -		kfree(hook);
> > +		kfree_rcu(hook, rcu);

This looks correct to me.

> >  	}
> >  	kfree(flowtable->name);
> >  	module_put(flowtable->data.type->owner);
> 
> AFAICS its safe to use list_del() everywhere, I can't find a single
> instance where the hooks are iterated without mutex serialization.

Netlink dump path is lockless.

nft_dump_basechain_hook() is missing list_for_each_entry_rcu() for
list iteration, that needs a fix.

nf_tables_fill_flowtable_info() does use list_for_each_entry_rcu().

> nf_tables_flowtable_destroy() is called after the hook has been
> unregisted (detached from nf_hook list) and rcu grace period elapsed.

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Pablo Neira Ayuso]

On Mon, Sep 16, 2024 at 02:00:59AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 12, 2024 at 03:32:55PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > Documentation of list_del_rcu() warns callers to not immediately free
> > > the deleted list item. While it seems not necessary to use the
> > > RCU-variant of list_del() here in the first place, doing so seems to
> > > require calling kfree_rcu() on the deleted item as well.
> > > 
> > > Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  net/netfilter/nf_tables_api.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index b6547fe22bd8..2982f49b6d55 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
> > >  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
> > >  					    FLOW_BLOCK_UNBIND);
> > >  		list_del_rcu(&hook->list);
> > > -		kfree(hook);
> > > +		kfree_rcu(hook, rcu);
> 
> This looks correct to me.
> 
> > >  	}
> > >  	kfree(flowtable->name);
> > >  	module_put(flowtable->data.type->owner);
> > 
> > AFAICS its safe to use list_del() everywhere, I can't find a single
> > instance where the hooks are iterated without mutex serialization.
> 
> Netlink dump path is lockless.
> 
> nft_dump_basechain_hook() is missing list_for_each_entry_rcu() for
> list iteration, that needs a fix.
> 
> nf_tables_fill_flowtable_info() does use list_for_each_entry_rcu().

I'd suggest to start by sending fixes for nf.git to address these two
issues.

> > nf_tables_flowtable_destroy() is called after the hook has been
> > unregisted (detached from nf_hook list) and rcu grace period elapsed.

Re: [nf-next PATCH v3 01/16] netfilter: nf_tables: Keep deleted flowtable hooks until after RCU

[Pablo Neira Ayuso]

On Thu, Sep 12, 2024 at 02:21:33PM +0200, Phil Sutter wrote:
> Documentation of list_del_rcu() warns callers to not immediately free
> the deleted list item. While it seems not necessary to use the
> RCU-variant of list_del() here in the first place, doing so seems to
> require calling kfree_rcu() on the deleted item as well.

LGTM, I plan to take this into nf.git

Thanks.

> Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b6547fe22bd8..2982f49b6d55 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -9180,7 +9180,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
>  		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
>  					    FLOW_BLOCK_UNBIND);
>  		list_del_rcu(&hook->list);
> -		kfree(hook);
> +		kfree_rcu(hook, rcu);
>  	}
>  	kfree(flowtable->name);
>  	module_put(flowtable->data.type->owner);
> -- 
> 2.43.0
> 

[nf-next PATCH v3 02/16] netfilter: nf_tables: Flowtable hook's pf value never varies

[Phil Sutter]

When checking for duplicate hooks in nft_register_flowtable_net_hooks(),
comparing ops.pf value is pointless as it is always NFPROTO_NETDEV with
flowtable hooks.

Dropping the check leaves the search identical to the one in
nft_hook_list_find() so call that function instead of open coding.

Fixes: 3f0465a9ef02 ("netfilter: nf_tables: dynamically allocate hooks per net_device in flowtables")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2982f49b6d55..3ffb728309af 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8544,7 +8544,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 					    struct list_head *hook_list,
 					    struct nft_flowtable *flowtable)
 {
-	struct nft_hook *hook, *hook2, *next;
+	struct nft_hook *hook, *next;
 	struct nft_flowtable *ft;
 	int err, i = 0;
 
@@ -8553,12 +8553,9 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			if (!nft_is_active_next(net, ft))
 				continue;
 
-			list_for_each_entry(hook2, &ft->hook_list, list) {
-				if (hook->ops.dev == hook2->ops.dev &&
-				    hook->ops.pf == hook2->ops.pf) {
-					err = -EEXIST;
-					goto err_unregister_net_hooks;
-				}
+			if (nft_hook_list_find(&ft->hook_list, hook)) {
+				err = -EEXIST;
+				goto err_unregister_net_hooks;
 			}
 		}
 
-- 
2.43.0

[nf-next PATCH v3 03/16] netfilter: nf_tables: Store user-defined hook ifname

[Phil Sutter]

Prepare for hooks with NULL ops.dev pointer (due to non-existent device)
and store the interface name and length as specified by the user upon
creation. No functional change intended.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h | 2 ++
 net/netfilter/nf_tables_api.c     | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index c302b396e1a7..efd6b55b4914 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1191,6 +1191,8 @@ struct nft_hook {
 	struct list_head	list;
 	struct nf_hook_ops	ops;
 	struct rcu_head		rcu;
+	char			ifname[IFNAMSIZ];
+	u8			ifnamelen;
 };
 
 /**
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3ffb728309af..f1710aab5188 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2173,7 +2173,6 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 					      const struct nlattr *attr)
 {
 	struct net_device *dev;
-	char ifname[IFNAMSIZ];
 	struct nft_hook *hook;
 	int err;
 
@@ -2183,12 +2182,13 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 		goto err_hook_alloc;
 	}
 
-	nla_strscpy(ifname, attr, IFNAMSIZ);
+	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
+	hook->ifnamelen = nla_len(attr);
 	/* nf_tables_netdev_event() is called under rtnl_mutex, this is
 	 * indirectly serializing all the other holders of the commit_mutex with
 	 * the rtnl_mutex.
 	 */
-	dev = __dev_get_by_name(net, ifname);
+	dev = __dev_get_by_name(net, hook->ifname);
 	if (!dev) {
 		err = -ENOENT;
 		goto err_hook_dev;
-- 
2.43.0

Re: [nf-next PATCH v3 03/16] netfilter: nf_tables: Store user-defined hook ifname

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Prepare for hooks with NULL ops.dev pointer (due to non-existent device)
> and store the interface name and length as specified by the user upon
> creation. No functional change intended.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/net/netfilter/nf_tables.h | 2 ++
>  net/netfilter/nf_tables_api.c     | 6 +++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index c302b396e1a7..efd6b55b4914 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -1191,6 +1191,8 @@ struct nft_hook {
>  	struct list_head	list;
>  	struct nf_hook_ops	ops;
>  	struct rcu_head		rcu;
> +	char			ifname[IFNAMSIZ];
> +	u8			ifnamelen;
>  };
>  
>  /**
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 3ffb728309af..f1710aab5188 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2173,7 +2173,6 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
>  					      const struct nlattr *attr)
>  {
>  	struct net_device *dev;
> -	char ifname[IFNAMSIZ];
>  	struct nft_hook *hook;
>  	int err;
>  
> @@ -2183,12 +2182,13 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
>  		goto err_hook_alloc;
>  	}
>  
> -	nla_strscpy(ifname, attr, IFNAMSIZ);
> +	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
> +	hook->ifnamelen = nla_len(attr);


Hmm. nft_netdev_hook_alloc has no netlink attribute policy validation
:-/

Can you add another patch that fixes this up?


I'd suggest to move the if (nla_type(tmp) != NFTA_DEVICE_NAME)
test from nf_tables_parse_netdev_hooks() into nft_netdev_hook_alloc
so nft_chain_parse_netdev() also has this test.

Then,
> -     nla_strscpy(ifname, attr, IFNAMSIZ);

Into:

	err = nla_strscpy(ifname, attr, IFNAMSIZ)
	if (err < 0)
		goto err_hook_dev;

so we validate that
a) attr is NFTA_DEVICE_NAME
b) length doesn't exceed IFNAMSIZ.

ATM this check is implicit because nla_strscpy() always
null-terminates and the next line will check that the
device with this name actually exists.

But that check is removed later.
This patch can then set

 hook->ifnamelen = err

without risk that nla_len() returns 0xfff0 or some other bogus
value.

Re: [nf-next PATCH v3 03/16] netfilter: nf_tables: Store user-defined hook ifname

[Phil Sutter]

On Thu, Sep 12, 2024 at 02:56:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Prepare for hooks with NULL ops.dev pointer (due to non-existent device)
> > and store the interface name and length as specified by the user upon
> > creation. No functional change intended.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/net/netfilter/nf_tables.h | 2 ++
> >  net/netfilter/nf_tables_api.c     | 6 +++---
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index c302b396e1a7..efd6b55b4914 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -1191,6 +1191,8 @@ struct nft_hook {
> >  	struct list_head	list;
> >  	struct nf_hook_ops	ops;
> >  	struct rcu_head		rcu;
> > +	char			ifname[IFNAMSIZ];
> > +	u8			ifnamelen;
> >  };
> >  
> >  /**
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 3ffb728309af..f1710aab5188 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -2173,7 +2173,6 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
> >  					      const struct nlattr *attr)
> >  {
> >  	struct net_device *dev;
> > -	char ifname[IFNAMSIZ];
> >  	struct nft_hook *hook;
> >  	int err;
> >  
> > @@ -2183,12 +2182,13 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
> >  		goto err_hook_alloc;
> >  	}
> >  
> > -	nla_strscpy(ifname, attr, IFNAMSIZ);
> > +	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
> > +	hook->ifnamelen = nla_len(attr);
> 
> 
> Hmm. nft_netdev_hook_alloc has no netlink attribute policy validation
> :-/
> 
> Can you add another patch that fixes this up?
> 
> 
> I'd suggest to move the if (nla_type(tmp) != NFTA_DEVICE_NAME)
> test from nf_tables_parse_netdev_hooks() into nft_netdev_hook_alloc
> so nft_chain_parse_netdev() also has this test.

I fear this won't work, because nft_chain_parse_netdev() may call
nft_netdev_hook_alloc() directly, passing tb[NFTA_HOOK_DEV]. For
NFTA_HOOK_DEVS though, it calls nf_tables_parse_netdev_hooks() and thus
the nested attribute type check in there applies.
> 
> Then,
> > -     nla_strscpy(ifname, attr, IFNAMSIZ);
> 
> Into:
> 
> 	err = nla_strscpy(ifname, attr, IFNAMSIZ)
> 	if (err < 0)
> 		goto err_hook_dev;
> 
> so we validate that
> a) attr is NFTA_DEVICE_NAME

As said above, it may be NFTA_HOOK_DEV as well.

> b) length doesn't exceed IFNAMSIZ.

ACK. I think a) is already being asserted in spots where NFTA_HOOK_DEV
is not expected (which in turn has a policy).

> ATM this check is implicit because nla_strscpy() always
> null-terminates and the next line will check that the
> device with this name actually exists.
> 
> But that check is removed later.
> This patch can then set
> 
>  hook->ifnamelen = err
> 
> without risk that nla_len() returns 0xfff0 or some other bogus
> value.

Ah, nice!

From my perspective, all it takes is the nla_strscpy() return value
check in this patch to cover everything. Right?

Thanks, Phil

Re: [nf-next PATCH v3 03/16] netfilter: nf_tables: Store user-defined hook ifname

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > I'd suggest to move the if (nla_type(tmp) != NFTA_DEVICE_NAME)
> > test from nf_tables_parse_netdev_hooks() into nft_netdev_hook_alloc
> > so nft_chain_parse_netdev() also has this test.
> 
> I fear this won't work, because nft_chain_parse_netdev() may call
> nft_netdev_hook_alloc() directly, passing tb[NFTA_HOOK_DEV]. For
> NFTA_HOOK_DEVS though, it calls nf_tables_parse_netdev_hooks() and thus
> the nested attribute type check in there applies.

Ouch, you are right, it can be called with either attribute :-(

> > Then,
> > > -     nla_strscpy(ifname, attr, IFNAMSIZ);
> > 
> > Into:
> > 
> > 	err = nla_strscpy(ifname, attr, IFNAMSIZ)
> > 	if (err < 0)
> > 		goto err_hook_dev;
> > 
> > so we validate that
> > a) attr is NFTA_DEVICE_NAME
> 
> As said above, it may be NFTA_HOOK_DEV as well.

Yes, my bad.

> > b) length doesn't exceed IFNAMSIZ.
> 
> ACK. I think a) is already being asserted in spots where NFTA_HOOK_DEV
> is not expected (which in turn has a policy).

Yes, I did not see that this is using different attributes.

> > ATM this check is implicit because nla_strscpy() always
> > null-terminates and the next line will check that the
> > device with this name actually exists.
> > 
> > But that check is removed later.
> > This patch can then set
> > 
> >  hook->ifnamelen = err
> > 
> > without risk that nla_len() returns 0xfff0 or some other bogus
> > value.
> 
> Ah, nice!
> 
> From my perspective, all it takes is the nla_strscpy() return value
> check in this patch to cover everything. Right?

Yes, right.  So no extra patch is needd.

[nf-next PATCH v3 04/16] netfilter: nf_tables: Use stored ifname in netdev hook dumps

[Phil Sutter]

The stored ifname and ops.dev->name may deviate after creation due to
interface name changes. Prefer the more deterministic stored name in
dumps which also helps avoiding inadvertent changes to stored ruleset
dumps.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f1710aab5188..4fb230e4afe3 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1853,15 +1853,16 @@ static int nft_dump_basechain_hook(struct sk_buff *skb, int family,
 			if (!first)
 				first = hook;
 
-			if (nla_put_string(skb, NFTA_DEVICE_NAME,
-					   hook->ops.dev->name))
+			if (nla_put(skb, NFTA_DEVICE_NAME,
+				    hook->ifnamelen, hook->ifname))
 				goto nla_put_failure;
 			n++;
 		}
 		nla_nest_end(skb, nest_devs);
 
 		if (n == 1 &&
-		    nla_put_string(skb, NFTA_HOOK_DEV, first->ops.dev->name))
+		    nla_put(skb, NFTA_HOOK_DEV,
+			    first->ifnamelen, first->ifname))
 			goto nla_put_failure;
 	}
 	nla_nest_end(skb, nest);
@@ -8968,7 +8969,8 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 		hook_list = &flowtable->hook_list;
 
 	list_for_each_entry_rcu(hook, hook_list, list) {
-		if (nla_put_string(skb, NFTA_DEVICE_NAME, hook->ops.dev->name))
+		if (nla_put(skb, NFTA_DEVICE_NAME,
+			    hook->ifnamelen, hook->ifname))
 			goto nla_put_failure;
 	}
 	nla_nest_end(skb, nest_devs);
-- 
2.43.0

[nf-next PATCH v3 05/16] netfilter: nf_tables: Compare netdev hooks based on stored name

[Phil Sutter]

The 1:1 relationship between nft_hook and nf_hook_ops is about to break,
so choose the stored ifname to uniquely identify hooks.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4fb230e4afe3..457696f55003 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2210,7 +2210,7 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, hook_list, list) {
-		if (this->ops.dev == hook->ops.dev)
+		if (!strcmp(hook->ifname, this->ifname))
 			return hook;
 	}
 
-- 
2.43.0

[nf-next PATCH v3 06/16] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Phil Sutter]

Do not drop a netdev-family chain if the last interface it is registered
for vanishes. Users dumping and storing the ruleset upon shutdown for
restore upon next boot may otherwise lose the chain and all contained
rules. They will still lose the list of devices, a later patch will fix
that. For now, this aligns the event handler's behaviour with that for
flowtables.
The controversal situation at netns exit should be no problem here:
event handler will unregister the hooks, core nftables cleanup code will
drop the chain itself.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h |  2 --
 net/netfilter/nf_tables_api.c     | 21 ---------------------
 net/netfilter/nft_chain_filter.c  | 29 +++++++----------------------
 3 files changed, 7 insertions(+), 45 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index efd6b55b4914..16daffcee0e1 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1228,8 +1228,6 @@ static inline bool nft_is_base_chain(const struct nft_chain *chain)
 	return chain->flags & NFT_CHAIN_BASE;
 }
 
-int __nft_release_basechain(struct nft_ctx *ctx);
-
 unsigned int nft_do_chain(struct nft_pktinfo *pkt, void *priv);
 
 static inline bool nft_use_inc(u32 *use)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 457696f55003..46d4e9056bf1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -11386,27 +11386,6 @@ int nft_data_dump(struct sk_buff *skb, int attr, const struct nft_data *data,
 }
 EXPORT_SYMBOL_GPL(nft_data_dump);
 
-int __nft_release_basechain(struct nft_ctx *ctx)
-{
-	struct nft_rule *rule, *nr;
-
-	if (WARN_ON(!nft_is_base_chain(ctx->chain)))
-		return 0;
-
-	nf_tables_unregister_hook(ctx->net, ctx->chain->table, ctx->chain);
-	list_for_each_entry_safe(rule, nr, &ctx->chain->rules, list) {
-		list_del(&rule->list);
-		nft_use_dec(&ctx->chain->use);
-		nf_tables_rule_release(ctx, rule);
-	}
-	nft_chain_del(ctx->chain);
-	nft_use_dec(&ctx->table->use);
-	nf_tables_chain_destroy(ctx->chain);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(__nft_release_basechain);
-
 static void __nft_release_hook(struct net *net, struct nft_table *table)
 {
 	struct nft_flowtable *flowtable;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 7010541fcca6..543f258b7c6b 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -322,34 +322,19 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 			     struct nft_ctx *ctx)
 {
 	struct nft_base_chain *basechain = nft_base_chain(ctx->chain);
-	struct nft_hook *hook, *found = NULL;
-	int n = 0;
+	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.dev == dev)
-			found = hook;
-
-		n++;
-	}
-	if (!found)
-		return;
+		if (hook->ops.dev != dev)
+			continue;
 
-	if (n > 1) {
 		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
-			nf_unregister_net_hook(ctx->net, &found->ops);
+			nf_unregister_net_hook(ctx->net, &hook->ops);
 
-		list_del_rcu(&found->list);
-		kfree_rcu(found, rcu);
-		return;
+		list_del_rcu(&hook->list);
+		kfree_rcu(hook, rcu);
+		break;
 	}
-
-	/* UNREGISTER events are also happening on netns exit.
-	 *
-	 * Although nf_tables core releases all tables/chains, only this event
-	 * handler provides guarantee that hook->ops.dev is still accessible,
-	 * so we cannot skip exiting net namespaces.
-	 */
-	__nft_release_basechain(ctx);
 }
 
 static int nf_tables_netdev_event(struct notifier_block *this,
-- 
2.43.0

Re: [nf-next PATCH v3 06/16] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Do not drop a netdev-family chain if the last interface it is registered
> for vanishes. Users dumping and storing the ruleset upon shutdown for
> restore upon next boot may otherwise lose the chain and all contained
> rules. They will still lose the list of devices, a later patch will fix
> that. For now, this aligns the event handler's behaviour with that for
> flowtables.
> The controversal situation at netns exit should be no problem here:
> event handler will unregister the hooks, core nftables cleanup code will
> drop the chain itself.

This "breaks" 
W: [DUMP FAIL]  1/2 tests/shell/testcases/json/netdev
W: [DUMP FAIL]  2/2 tests/shell/testcases/chains/netdev_chain_0

any suggestions on how to handle this?

We can't fix the dump because old kernel will axe the empty basechain.
Should the dump files be removed?

Re: [nf-next PATCH v3 06/16] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Phil Sutter]

On Thu, Oct 31, 2024 at 03:01:04PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Do not drop a netdev-family chain if the last interface it is registered
> > for vanishes. Users dumping and storing the ruleset upon shutdown for
> > restore upon next boot may otherwise lose the chain and all contained
> > rules. They will still lose the list of devices, a later patch will fix
> > that. For now, this aligns the event handler's behaviour with that for
> > flowtables.
> > The controversal situation at netns exit should be no problem here:
> > event handler will unregister the hooks, core nftables cleanup code will
> > drop the chain itself.
> 
> This "breaks" 
> W: [DUMP FAIL]  1/2 tests/shell/testcases/json/netdev
> W: [DUMP FAIL]  2/2 tests/shell/testcases/chains/netdev_chain_0
> 
> any suggestions on how to handle this?
> 
> We can't fix the dump because old kernel will axe the empty basechain.

AFAIR, we did just that in the past with such cases. I agree, it pretty
much breaks any efforts at making the testsuite usable with stable
kernels.

> Should the dump files be removed?

Maybe "feature flag" it and introduce a mechanism for test cases to
revert to a different dump file?

Or we convince Pablo to axe his efforts at fixing chain deletion in
stable kernels and instead backport my "zombie chain" feature. ;)

Cheers, Phil

Re: [nf-next PATCH v3 06/16] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> AFAIR, we did just that in the past with such cases. I agree, it pretty
> much breaks any efforts at making the testsuite usable with stable
> kernels.
> 
> > Should the dump files be removed?
> 
> Maybe "feature flag" it and introduce a mechanism for test cases to
> revert to a different dump file?

What could be good enough:
1. fix dump files
2. add feature flag, if feature not present -> exit 77/skip to elide
dump compare.

Would you explore this?
Thanks!

Re: [nf-next PATCH v3 06/16] netfilter: nf_tables: Tolerate chains with no remaining hooks

[Phil Sutter]

On Thu, Oct 31, 2024 at 03:37:36PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > AFAIR, we did just that in the past with such cases. I agree, it pretty
> > much breaks any efforts at making the testsuite usable with stable
> > kernels.
> > 
> > > Should the dump files be removed?
> > 
> > Maybe "feature flag" it and introduce a mechanism for test cases to
> > revert to a different dump file?
> 
> What could be good enough:
> 1. fix dump files
> 2. add feature flag, if feature not present -> exit 77/skip to elide
> dump compare.

Reasonably simple to provide backwards compat (and still keeping the
actual test active)!

> Would you explore this?

Will do!

Thanks, Phil

[nf-next PATCH v3 07/16] netfilter: nf_tables: Introduce functions freeing nft_hook objects

[Phil Sutter]

Pointless wrappers around kfree() for now, prep work for an embedded
list of nf_hook_ops.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 46d4e9056bf1..dedf50ba266c 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2134,6 +2134,16 @@ static void nf_tables_chain_free_chain_rules(struct nft_chain *chain)
 	kvfree(chain->blob_next);
 }
 
+static void nft_netdev_hook_free(struct nft_hook *hook)
+{
+	kfree(hook);
+}
+
+static void nft_netdev_hook_free_rcu(struct nft_hook *hook)
+{
+	kfree_rcu(hook, rcu);
+}
+
 void nf_tables_chain_destroy(struct nft_chain *chain)
 {
 	const struct nft_table *table = chain->table;
@@ -2152,7 +2162,7 @@ void nf_tables_chain_destroy(struct nft_chain *chain)
 			list_for_each_entry_safe(hook, next,
 						 &basechain->hook_list, list) {
 				list_del_rcu(&hook->list);
-				kfree_rcu(hook, rcu);
+				nft_netdev_hook_free_rcu(hook);
 			}
 		}
 		module_put(basechain->type->owner);
@@ -2240,7 +2250,7 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
 		}
 		if (nft_hook_list_find(hook_list, hook)) {
 			NL_SET_BAD_ATTR(extack, tmp);
-			kfree(hook);
+			nft_netdev_hook_free(hook);
 			err = -EEXIST;
 			goto err_hook;
 		}
@@ -2258,7 +2268,7 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
 err_hook:
 	list_for_each_entry_safe(hook, next, hook_list, list) {
 		list_del(&hook->list);
-		kfree(hook);
+		nft_netdev_hook_free(hook);
 	}
 	return err;
 }
@@ -2401,7 +2411,7 @@ static void nft_chain_release_hook(struct nft_chain_hook *hook)
 
 	list_for_each_entry_safe(h, next, &hook->list, list) {
 		list_del(&h->list);
-		kfree(h);
+		nft_netdev_hook_free(h);
 	}
 	module_put(hook->type->owner);
 }
@@ -2691,7 +2701,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 
 				if (nft_hook_list_find(&basechain->hook_list, h)) {
 					list_del(&h->list);
-					kfree(h);
+					nft_netdev_hook_free(h);
 				}
 			}
 		} else {
@@ -2812,7 +2822,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 			if (unregister)
 				nf_unregister_net_hook(ctx->net, &h->ops);
 			list_del(&h->list);
-			kfree_rcu(h, rcu);
+			nft_netdev_hook_free_rcu(h);
 		}
 		module_put(hook.type->owner);
 	}
@@ -8586,7 +8596,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 
 		nft_unregister_flowtable_hook(net, flowtable, hook);
 		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		nft_netdev_hook_free_rcu(hook);
 	}
 
 	return err;
@@ -8598,7 +8608,7 @@ static void nft_hooks_destroy(struct list_head *hook_list)
 
 	list_for_each_entry_safe(hook, next, hook_list, list) {
 		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		nft_netdev_hook_free_rcu(hook);
 	}
 }
 
@@ -8622,7 +8632,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
 		if (nft_hook_list_find(&flowtable->hook_list, hook)) {
 			list_del(&hook->list);
-			kfree(hook);
+			nft_netdev_hook_free(hook);
 		}
 	}
 
@@ -8669,7 +8679,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 		if (unregister)
 			nft_unregister_flowtable_hook(ctx->net, flowtable, hook);
 		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		nft_netdev_hook_free_rcu(hook);
 	}
 
 	return err;
@@ -8815,7 +8825,7 @@ static void nft_flowtable_hook_release(struct nft_flowtable_hook *flowtable_hook
 
 	list_for_each_entry_safe(this, next, &flowtable_hook->list, list) {
 		list_del(&this->list);
-		kfree(this);
+		nft_netdev_hook_free(this);
 	}
 }
 
@@ -9179,7 +9189,7 @@ static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
 		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
 					    FLOW_BLOCK_UNBIND);
 		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		nft_netdev_hook_free_rcu(hook);
 	}
 	kfree(flowtable->name);
 	module_put(flowtable->data.type->owner);
-- 
2.43.0

[nf-next PATCH v3 08/16] netfilter: nf_tables: Introduce nft_hook_find_ops()

[Phil Sutter]

Also a pretty dull wrapper around the hook->ops.dev comparison for now.
Will search the embedded nf_hook_ops list in future. The ugly cast to
eliminate the const qualifier will vanish then, too.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h |  3 +++
 net/netfilter/nf_tables_api.c     | 14 +++++++++++++-
 net/netfilter/nf_tables_offload.c |  2 +-
 net/netfilter/nft_chain_filter.c  |  6 ++++--
 net/netfilter/nft_flow_offload.c  |  2 +-
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 16daffcee0e1..be11518646a3 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1195,6 +1195,9 @@ struct nft_hook {
 	u8			ifnamelen;
 };
 
+struct nf_hook_ops *nft_hook_find_ops(const struct nft_hook *hook,
+				      const struct net_device *dev);
+
 /**
  *	struct nft_base_chain - nf_tables base chain
  *
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index dedf50ba266c..65db4c54cfae 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9222,13 +9222,25 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net,
 	return -EMSGSIZE;
 }
 
+struct nf_hook_ops *nft_hook_find_ops(const struct nft_hook *hook,
+				      const struct net_device *dev)
+{
+	if (hook->ops.dev == dev)
+		return (struct nf_hook_ops *)&hook->ops;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(nft_hook_find_ops);
+
 static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 				struct nft_flowtable *flowtable)
 {
+	struct nf_hook_ops *ops;
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &flowtable->hook_list, list) {
-		if (hook->ops.dev != dev)
+		ops = nft_hook_find_ops(hook, dev);
+		if (!ops)
 			continue;
 
 		/* flow_offload_netdev_event() cleans up entries for us. */
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 64675f1c7f29..75b756f0b9f0 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -638,7 +638,7 @@ static struct nft_chain *__nft_offload_get_chain(const struct nftables_pernet *n
 			found = NULL;
 			basechain = nft_base_chain(chain);
 			list_for_each_entry(hook, &basechain->hook_list, list) {
-				if (hook->ops.dev != dev)
+				if (!nft_hook_find_ops(hook, dev))
 					continue;
 
 				found = hook;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 543f258b7c6b..d34c6fe7ba72 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -322,14 +322,16 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 			     struct nft_ctx *ctx)
 {
 	struct nft_base_chain *basechain = nft_base_chain(ctx->chain);
+	struct nf_hook_ops *ops;
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.dev != dev)
+		ops = nft_hook_find_ops(hook, dev);
+		if (!ops)
 			continue;
 
 		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
-			nf_unregister_net_hook(ctx->net, &hook->ops);
+			nf_unregister_net_hook(ctx->net, ops);
 
 		list_del_rcu(&hook->list);
 		kfree_rcu(hook, rcu);
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 9dcd1548df9d..646192321265 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -174,7 +174,7 @@ static bool nft_flowtable_find_dev(const struct net_device *dev,
 	bool found = false;
 
 	list_for_each_entry_rcu(hook, &ft->hook_list, list) {
-		if (hook->ops.dev != dev)
+		if (!nft_hook_find_ops(hook, dev))
 			continue;
 
 		found = true;
-- 
2.43.0

[nf-next PATCH v3 09/16] netfilter: nf_tables: Introduce nft_register_flowtable_ops()

[Phil Sutter]

Facilitate binding and registering of a flowtable hook via a single
function call.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 65db4c54cfae..dc30d2be09fb 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8550,6 +8550,26 @@ static void nft_unregister_flowtable_net_hooks(struct net *net,
 	__nft_unregister_flowtable_net_hooks(net, hook_list, false);
 }
 
+static int nft_register_flowtable_ops(struct net *net,
+				      struct nft_flowtable *flowtable,
+				      struct nf_hook_ops *ops)
+{
+	int err;
+
+	err = flowtable->data.type->setup(&flowtable->data,
+					  ops->dev, FLOW_BLOCK_BIND);
+	if (err < 0)
+		return err;
+
+	err = nf_register_net_hook(net, ops);
+	if (!err)
+		return 0;
+
+	flowtable->data.type->setup(&flowtable->data,
+				    ops->dev, FLOW_BLOCK_UNBIND);
+	return err;
+}
+
 static int nft_register_flowtable_net_hooks(struct net *net,
 					    struct nft_table *table,
 					    struct list_head *hook_list,
@@ -8570,20 +8590,10 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			}
 		}
 
-		err = flowtable->data.type->setup(&flowtable->data,
-						  hook->ops.dev,
-						  FLOW_BLOCK_BIND);
+		err = nft_register_flowtable_ops(net, flowtable, &hook->ops);
 		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++;
 	}
 
-- 
2.43.0

[nf-next PATCH v3 10/16] netfilter: nf_tables: Have a list of nf_hook_ops in nft_hook

[Phil Sutter]

Supporting a 1:n relationship between nft_hook and nf_hook_ops is
convenient since a chain's or flowtable's nft_hooks may remain in place
despite matching interfaces disappearing. This stabilizes ruleset dumps
in that regard and opens the possibility to claim newly added interfaces
which match the spec. Also it prepares for wildcard interface specs
since these will potentially match multiple interfaces.

All spots dealing with hook registration are updated to potentially
handle a list of multiple nf_hook_ops, but nft_netdev_hook_alloc()
only adds a single item for now to retain the old behaviour. The only
expected functional change here is how vanishing interfaces are handled:
Instead of dropping the respective nft_hook, only the matching
nf_hook_ops are dropped.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/linux/netfilter.h         |   2 +
 include/net/netfilter/nf_tables.h |   2 +-
 net/netfilter/nf_tables_api.c     | 155 +++++++++++++++++++++---------
 net/netfilter/nf_tables_offload.c |  49 ++++++----
 net/netfilter/nft_chain_filter.c  |   6 +-
 5 files changed, 143 insertions(+), 71 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 2683b2b77612..1318f18784ab 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -95,6 +95,8 @@ enum nf_hook_ops_type {
 };
 
 struct nf_hook_ops {
+	struct list_head	list;
+
 	/* User fills in from here down. */
 	nf_hookfn		*hook;
 	struct net_device	*dev;
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index be11518646a3..991b8d5e52f1 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1189,7 +1189,7 @@ struct nft_stats {
 
 struct nft_hook {
 	struct list_head	list;
-	struct nf_hook_ops	ops;
+	struct list_head	ops_list;
 	struct rcu_head		rcu;
 	char			ifname[IFNAMSIZ];
 	u8			ifnamelen;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index dc30d2be09fb..64f8305189f1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -299,25 +299,30 @@ void nf_tables_unbind_chain(const struct nft_ctx *ctx, struct nft_chain *chain)
 static int nft_netdev_register_hooks(struct net *net,
 				     struct list_head *hook_list)
 {
+	struct nf_hook_ops *ops;
 	struct nft_hook *hook;
 	int err, j;
 
 	j = 0;
 	list_for_each_entry(hook, hook_list, list) {
-		err = nf_register_net_hook(net, &hook->ops);
-		if (err < 0)
-			goto err_register;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			err = nf_register_net_hook(net, ops);
+			if (err < 0)
+				goto err_register;
 
-		j++;
+			j++;
+		}
 	}
 	return 0;
 
 err_register:
 	list_for_each_entry(hook, hook_list, list) {
-		if (j-- <= 0)
-			break;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			if (j-- <= 0)
+				break;
 
-		nf_unregister_net_hook(net, &hook->ops);
+			nf_unregister_net_hook(net, ops);
+		}
 	}
 	return err;
 }
@@ -326,10 +331,17 @@ static void nft_netdev_unregister_hooks(struct net *net,
 					struct list_head *hook_list,
 					bool release_netdev)
 {
+	struct nf_hook_ops *ops, *nextops;
 	struct nft_hook *hook, *next;
 
 	list_for_each_entry_safe(hook, next, hook_list, list) {
-		nf_unregister_net_hook(net, &hook->ops);
+		list_for_each_entry_safe(ops, nextops, &hook->ops_list, list) {
+			nf_unregister_net_hook(net, ops);
+			if (release_netdev) {
+				list_del(&ops->list);
+				kfree(ops);
+			}
+		}
 		if (release_netdev) {
 			list_del(&hook->list);
 			kfree_rcu(hook, rcu);
@@ -2134,13 +2146,25 @@ static void nf_tables_chain_free_chain_rules(struct nft_chain *chain)
 	kvfree(chain->blob_next);
 }
 
+static void nft_netdev_hook_free_ops(struct nft_hook *hook)
+{
+	struct nf_hook_ops *ops, *next;
+
+	list_for_each_entry_safe(ops, next, &hook->ops_list, list) {
+		list_del(&ops->list);
+		kfree(ops);
+	}
+}
+
 static void nft_netdev_hook_free(struct nft_hook *hook)
 {
+	nft_netdev_hook_free_ops(hook);
 	kfree(hook);
 }
 
 static void nft_netdev_hook_free_rcu(struct nft_hook *hook)
 {
+	nft_netdev_hook_free_ops(hook);
 	kfree_rcu(hook, rcu);
 }
 
@@ -2183,6 +2207,7 @@ void nf_tables_chain_destroy(struct nft_chain *chain)
 static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 					      const struct nlattr *attr)
 {
+	struct nf_hook_ops *ops;
 	struct net_device *dev;
 	struct nft_hook *hook;
 	int err;
@@ -2192,6 +2217,7 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 		err = -ENOMEM;
 		goto err_hook_alloc;
 	}
+	INIT_LIST_HEAD(&hook->ops_list);
 
 	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
 	hook->ifnamelen = nla_len(attr);
@@ -2204,7 +2230,14 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 		err = -ENOENT;
 		goto err_hook_dev;
 	}
-	hook->ops.dev = dev;
+
+	ops = kzalloc(sizeof(struct nf_hook_ops), GFP_KERNEL_ACCOUNT);
+	if (!ops) {
+		err = -ENOMEM;
+		goto err_hook_dev;
+	}
+	ops->dev = dev;
+	list_add_tail(&ops->list, &hook->ops_list);
 
 	return hook;
 
@@ -2464,6 +2497,7 @@ static int nft_basechain_init(struct nft_base_chain *basechain, u8 family,
 			      struct nft_chain_hook *hook, u32 flags)
 {
 	struct nft_chain *chain;
+	struct nf_hook_ops *ops;
 	struct nft_hook *h;
 
 	basechain->type = hook->type;
@@ -2472,8 +2506,10 @@ static int nft_basechain_init(struct nft_base_chain *basechain, u8 family,
 
 	if (nft_base_chain_netdev(family, hook->num)) {
 		list_splice_init(&hook->list, &basechain->hook_list);
-		list_for_each_entry(h, &basechain->hook_list, list)
-			nft_basechain_hook_init(&h->ops, family, hook, chain);
+		list_for_each_entry(h, &basechain->hook_list, list) {
+			list_for_each_entry(ops, &h->ops_list, list)
+				nft_basechain_hook_init(ops, family, hook, chain);
+		}
 	}
 	nft_basechain_hook_init(&basechain->ops, family, hook, chain);
 
@@ -2693,11 +2729,13 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 
 		if (nft_base_chain_netdev(ctx->family, basechain->ops.hooknum)) {
 			list_for_each_entry_safe(h, next, &hook.list, list) {
-				h->ops.pf	= basechain->ops.pf;
-				h->ops.hooknum	= basechain->ops.hooknum;
-				h->ops.priority	= basechain->ops.priority;
-				h->ops.priv	= basechain->ops.priv;
-				h->ops.hook	= basechain->ops.hook;
+				list_for_each_entry(ops, &h->ops_list, list) {
+					ops->pf		= basechain->ops.pf;
+					ops->hooknum	= basechain->ops.hooknum;
+					ops->priority	= basechain->ops.priority;
+					ops->priv	= basechain->ops.priv;
+					ops->hook	= basechain->ops.hook;
+				}
 
 				if (nft_hook_list_find(&basechain->hook_list, h)) {
 					list_del(&h->list);
@@ -2819,8 +2857,10 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 err_hooks:
 	if (nla[NFTA_CHAIN_HOOK]) {
 		list_for_each_entry_safe(h, next, &hook.list, list) {
-			if (unregister)
-				nf_unregister_net_hook(ctx->net, &h->ops);
+			if (unregister) {
+				list_for_each_entry(ops, &h->ops_list, list)
+					nf_unregister_net_hook(ctx->net, ops);
+			}
 			list_del(&h->list);
 			nft_netdev_hook_free_rcu(h);
 		}
@@ -8420,6 +8460,7 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
 				    struct netlink_ext_ack *extack, bool add)
 {
 	struct nlattr *tb[NFTA_FLOWTABLE_HOOK_MAX + 1];
+	struct nf_hook_ops *ops;
 	struct nft_hook *hook;
 	int hooknum, priority;
 	int err;
@@ -8474,11 +8515,13 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
 	}
 
 	list_for_each_entry(hook, &flowtable_hook->list, list) {
-		hook->ops.pf		= NFPROTO_NETDEV;
-		hook->ops.hooknum	= flowtable_hook->num;
-		hook->ops.priority	= flowtable_hook->priority;
-		hook->ops.priv		= &flowtable->data;
-		hook->ops.hook		= flowtable->data.type->hook;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			ops->pf		= NFPROTO_NETDEV;
+			ops->hooknum	= flowtable_hook->num;
+			ops->priority	= flowtable_hook->priority;
+			ops->priv	= &flowtable->data;
+			ops->hook	= flowtable->data.type->hook;
+		}
 	}
 
 	return err;
@@ -8520,12 +8563,12 @@ nft_flowtable_type_get(struct net *net, u8 family)
 }
 
 /* Only called from error and netdev event paths. */
-static void nft_unregister_flowtable_hook(struct net *net,
-					  struct nft_flowtable *flowtable,
-					  struct nft_hook *hook)
+static void nft_unregister_flowtable_ops(struct net *net,
+					 struct nft_flowtable *flowtable,
+					 struct nf_hook_ops *ops)
 {
-	nf_unregister_net_hook(net, &hook->ops);
-	flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
+	nf_unregister_net_hook(net, ops);
+	flowtable->data.type->setup(&flowtable->data, ops->dev,
 				    FLOW_BLOCK_UNBIND);
 }
 
@@ -8533,10 +8576,17 @@ static void __nft_unregister_flowtable_net_hooks(struct net *net,
 						 struct list_head *hook_list,
 					         bool release_netdev)
 {
+	struct nf_hook_ops *ops, *nextops;
 	struct nft_hook *hook, *next;
 
 	list_for_each_entry_safe(hook, next, hook_list, list) {
-		nf_unregister_net_hook(net, &hook->ops);
+		list_for_each_entry_safe(ops, nextops, &hook->ops_list, list) {
+			nf_unregister_net_hook(net, ops);
+			if (release_netdev) {
+				list_del(&ops->list);
+				kfree(ops);
+			}
+		}
 		if (release_netdev) {
 			list_del(&hook->list);
 			kfree_rcu(hook, rcu);
@@ -8577,6 +8627,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 {
 	struct nft_hook *hook, *next;
 	struct nft_flowtable *ft;
+	struct nf_hook_ops *ops;
 	int err, i = 0;
 
 	list_for_each_entry(hook, hook_list, list) {
@@ -8590,21 +8641,25 @@ static int nft_register_flowtable_net_hooks(struct net *net,
 			}
 		}
 
-		err = nft_register_flowtable_ops(net, flowtable, &hook->ops);
-		if (err < 0)
-			goto err_unregister_net_hooks;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			err = nft_register_flowtable_ops(net, flowtable, ops);
+			if (err < 0)
+				goto err_unregister_net_hooks;
 
-		i++;
+			i++;
+		}
 	}
 
 	return 0;
 
 err_unregister_net_hooks:
 	list_for_each_entry_safe(hook, next, hook_list, list) {
-		if (i-- <= 0)
-			break;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			if (i-- <= 0)
+				break;
 
-		nft_unregister_flowtable_hook(net, flowtable, hook);
+			nft_unregister_flowtable_ops(net, flowtable, ops);
+		}
 		list_del_rcu(&hook->list);
 		nft_netdev_hook_free_rcu(hook);
 	}
@@ -8629,6 +8684,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 	const struct nlattr * const *nla = ctx->nla;
 	struct nft_flowtable_hook flowtable_hook;
 	struct nft_hook *hook, *next;
+	struct nf_hook_ops *ops;
 	struct nft_trans *trans;
 	bool unregister = false;
 	u32 flags;
@@ -8686,8 +8742,11 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 
 err_flowtable_update_hook:
 	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
-		if (unregister)
-			nft_unregister_flowtable_hook(ctx->net, flowtable, hook);
+		if (unregister) {
+			list_for_each_entry(ops, &hook->ops_list, list)
+				nft_unregister_flowtable_ops(ctx->net,
+							     flowtable, ops);
+		}
 		list_del_rcu(&hook->list);
 		nft_netdev_hook_free_rcu(hook);
 	}
@@ -9193,11 +9252,14 @@ static void nf_tables_flowtable_notify(struct nft_ctx *ctx,
 static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
 {
 	struct nft_hook *hook, *next;
+	struct nf_hook_ops *ops;
 
 	flowtable->data.type->free(&flowtable->data);
 	list_for_each_entry_safe(hook, next, &flowtable->hook_list, list) {
-		flowtable->data.type->setup(&flowtable->data, hook->ops.dev,
-					    FLOW_BLOCK_UNBIND);
+		list_for_each_entry(ops, &hook->ops_list, list)
+			flowtable->data.type->setup(&flowtable->data,
+						    ops->dev,
+						    FLOW_BLOCK_UNBIND);
 		list_del_rcu(&hook->list);
 		nft_netdev_hook_free_rcu(hook);
 	}
@@ -9235,9 +9297,12 @@ static int nf_tables_fill_gen_info(struct sk_buff *skb, struct net *net,
 struct nf_hook_ops *nft_hook_find_ops(const struct nft_hook *hook,
 				      const struct net_device *dev)
 {
-	if (hook->ops.dev == dev)
-		return (struct nf_hook_ops *)&hook->ops;
+	struct nf_hook_ops *ops;
 
+	list_for_each_entry(ops, &hook->ops_list, list) {
+		if (ops->dev == dev)
+			return ops;
+	}
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(nft_hook_find_ops);
@@ -9254,9 +9319,9 @@ static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 			continue;
 
 		/* flow_offload_netdev_event() cleans up entries for us. */
-		nft_unregister_flowtable_hook(dev_net(dev), flowtable, hook);
-		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
+		nft_unregister_flowtable_ops(dev_net(dev), flowtable, ops);
+		list_del(&ops->list);
+		kfree(ops);
 		break;
 	}
 }
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 75b756f0b9f0..fd30e205de84 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -220,6 +220,7 @@ static int nft_chain_offload_priority(const struct nft_base_chain *basechain)
 
 bool nft_chain_offload_support(const struct nft_base_chain *basechain)
 {
+	struct nf_hook_ops *ops;
 	struct net_device *dev;
 	struct nft_hook *hook;
 
@@ -227,13 +228,16 @@ bool nft_chain_offload_support(const struct nft_base_chain *basechain)
 		return false;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (hook->ops.pf != NFPROTO_NETDEV ||
-		    hook->ops.hooknum != NF_NETDEV_INGRESS)
-			return false;
-
-		dev = hook->ops.dev;
-		if (!dev->netdev_ops->ndo_setup_tc && !flow_indr_dev_exists())
-			return false;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			if (ops->pf != NFPROTO_NETDEV ||
+			    ops->hooknum != NF_NETDEV_INGRESS)
+				return false;
+
+			dev = ops->dev;
+			if (!dev->netdev_ops->ndo_setup_tc &&
+			    !flow_indr_dev_exists())
+				return false;
+		}
 	}
 
 	return true;
@@ -455,34 +459,37 @@ static int nft_flow_block_chain(struct nft_base_chain *basechain,
 				const struct net_device *this_dev,
 				enum flow_block_command cmd)
 {
-	struct net_device *dev;
+	struct nf_hook_ops *ops;
 	struct nft_hook *hook;
 	int err, i = 0;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		dev = hook->ops.dev;
-		if (this_dev && this_dev != dev)
-			continue;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			if (this_dev && this_dev != ops->dev)
+				continue;
 
-		err = nft_chain_offload_cmd(basechain, dev, cmd);
-		if (err < 0 && cmd == FLOW_BLOCK_BIND) {
-			if (!this_dev)
-				goto err_flow_block;
+			err = nft_chain_offload_cmd(basechain, ops->dev, cmd);
+			if (err < 0 && cmd == FLOW_BLOCK_BIND) {
+				if (!this_dev)
+					goto err_flow_block;
 
-			return err;
+				return err;
+			}
+			i++;
 		}
-		i++;
 	}
 
 	return 0;
 
 err_flow_block:
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		if (i-- <= 0)
-			break;
+		list_for_each_entry(ops, &hook->ops_list, list) {
+			if (i-- <= 0)
+				break;
 
-		dev = hook->ops.dev;
-		nft_chain_offload_cmd(basechain, dev, FLOW_BLOCK_UNBIND);
+			nft_chain_offload_cmd(basechain, ops->dev,
+					      FLOW_BLOCK_UNBIND);
+		}
 	}
 	return err;
 }
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index d34c6fe7ba72..2507e3beac5c 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -332,10 +332,8 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 
 		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
 			nf_unregister_net_hook(ctx->net, ops);
-
-		list_del_rcu(&hook->list);
-		kfree_rcu(hook, rcu);
-		break;
+		list_del(&ops->list);
+		kfree(ops);
 	}
 }
 
-- 
2.43.0

[nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter]

Hook into new devices if their name matches the hook spec.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nft_chain_filter.c | 40 +++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 2507e3beac5c..ec44c27a9d91 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -326,14 +326,37 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &basechain->hook_list, list) {
-		ops = nft_hook_find_ops(hook, dev);
-		if (!ops)
-			continue;
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			ops = nft_hook_find_ops(hook, dev);
+			if (!ops)
+				continue;
 
-		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
-			nf_unregister_net_hook(ctx->net, ops);
-		list_del(&ops->list);
-		kfree(ops);
+			if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
+				nf_unregister_net_hook(ctx->net, ops);
+			list_del(&ops->list);
+			kfree(ops);
+			break;
+		case NETDEV_REGISTER:
+			if (strcmp(hook->ifname, dev->name))
+				continue;
+			ops = kzalloc(sizeof(struct nf_hook_ops),
+				      GFP_KERNEL_ACCOUNT);
+			if (ops) {
+				memcpy(ops, &basechain->ops, sizeof(*ops));
+				ops->dev = dev;
+			}
+			if (ops &&
+			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
+			     !nf_register_net_hook(dev_net(dev), ops))) {
+				list_add_tail(&ops->list, &hook->ops_list);
+				break;
+			}
+			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
+			       ctx->chain->name, dev->name);
+			kfree(ops);
+			continue;
+		}
 	}
 }
 
@@ -349,7 +372,8 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 		.net	= dev_net(dev),
 	};
 
-	if (event != NETDEV_UNREGISTER)
+	if (event != NETDEV_REGISTER &&
+	    event != NETDEV_UNREGISTER)
 		return NOTIFY_DONE;
 
 	nft_net = nft_pernet(ctx.net);
-- 
2.43.0

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Hook into new devices if their name matches the hook spec.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/nft_chain_filter.c | 40 +++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
> index 2507e3beac5c..ec44c27a9d91 100644
> --- a/net/netfilter/nft_chain_filter.c
> +++ b/net/netfilter/nft_chain_filter.c
> @@ -326,14 +326,37 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
>  	struct nft_hook *hook;
>  
>  	list_for_each_entry(hook, &basechain->hook_list, list) {
> -		ops = nft_hook_find_ops(hook, dev);
> -		if (!ops)
> -			continue;
> +		switch (event) {
> +		case NETDEV_UNREGISTER:
> +			ops = nft_hook_find_ops(hook, dev);
> +			if (!ops)
> +				continue;
>  
> -		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> -			nf_unregister_net_hook(ctx->net, ops);
> -		list_del(&ops->list);
> -		kfree(ops);
> +			if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> +				nf_unregister_net_hook(ctx->net, ops);
> +			list_del(&ops->list);
> +			kfree(ops);

This needs to use kfree_rcu() + list_del_rcu, nf_unregister_net_hook
only stops new packets from executing for dev, it doesn't stop new
packets.

Or is this guaranteed by UNREGISTER event already?
If so, please add a comment.

> +			break;
> +		case NETDEV_REGISTER:
> +			if (strcmp(hook->ifname, dev->name))
> +				continue;
> +			ops = kzalloc(sizeof(struct nf_hook_ops),
> +				      GFP_KERNEL_ACCOUNT);

ops = kmemdup(&basechain->ops, .. ?

> +			if (ops) {
> +				memcpy(ops, &basechain->ops, sizeof(*ops));
> +				ops->dev = dev;
> +			}
> +			if (ops &&
> +			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
> +			     !nf_register_net_hook(dev_net(dev), ops))) {
> +				list_add_tail(&ops->list, &hook->ops_list);
> +				break;
> +			}
> +			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
> +			       ctx->chain->name, dev->name);

I think its better to -ENOMEM and veto the netdevice register request in this case.

I also think this needs extra handling for NETDEV_CHANGENAME rather than
the 'treat as UNREG+REG' trick.

Else we may unregister and then fail to re-register which leaves the
device without the registered hook op.

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter]

On Thu, Sep 12, 2024 at 04:40:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Hook into new devices if their name matches the hook spec.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nft_chain_filter.c | 40 +++++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
> > index 2507e3beac5c..ec44c27a9d91 100644
> > --- a/net/netfilter/nft_chain_filter.c
> > +++ b/net/netfilter/nft_chain_filter.c
> > @@ -326,14 +326,37 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
> >  	struct nft_hook *hook;
> >  
> >  	list_for_each_entry(hook, &basechain->hook_list, list) {
> > -		ops = nft_hook_find_ops(hook, dev);
> > -		if (!ops)
> > -			continue;
> > +		switch (event) {
> > +		case NETDEV_UNREGISTER:
> > +			ops = nft_hook_find_ops(hook, dev);
> > +			if (!ops)
> > +				continue;
> >  
> > -		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> > -			nf_unregister_net_hook(ctx->net, ops);
> > -		list_del(&ops->list);
> > -		kfree(ops);
> > +			if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> > +				nf_unregister_net_hook(ctx->net, ops);
> > +			list_del(&ops->list);
> > +			kfree(ops);
> 
> This needs to use kfree_rcu() + list_del_rcu, nf_unregister_net_hook
> only stops new packets from executing for dev, it doesn't stop new
> packets.
> 
> Or is this guaranteed by UNREGISTER event already?
> If so, please add a comment.

Are packets relevant here? The question should be whether another CPU
traverses hook->ops_list at the same time, no? Looking at
nft_flowtable_find_dev() mentioned in your other mail, there seems to be
a case which doesn't synchronize on commit_mutex. So same rules apply to
ops_list as for hook_list and thus I need to add an rcu_head to
nf_hook_ops as well?

> > +			break;
> > +		case NETDEV_REGISTER:
> > +			if (strcmp(hook->ifname, dev->name))
> > +				continue;
> > +			ops = kzalloc(sizeof(struct nf_hook_ops),
> > +				      GFP_KERNEL_ACCOUNT);
> 
> ops = kmemdup(&basechain->ops, .. ?

Oh, sure!

> > +			if (ops) {
> > +				memcpy(ops, &basechain->ops, sizeof(*ops));
> > +				ops->dev = dev;
> > +			}
> > +			if (ops &&
> > +			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
> > +			     !nf_register_net_hook(dev_net(dev), ops))) {
> > +				list_add_tail(&ops->list, &hook->ops_list);
> > +				break;
> > +			}
> > +			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
> > +			       ctx->chain->name, dev->name);
> 
> I think its better to -ENOMEM and veto the netdevice register request in this case.

Ah, I wasn't aware we may influence netdev registration from a notifier.
So I'll change the callbacks to return NOTIFY_BAD in error case.

> I also think this needs extra handling for NETDEV_CHANGENAME rather than
> the 'treat as UNREG+REG' trick.
> 
> Else we may unregister and then fail to re-register which leaves the
> device without the registered hook op.

So search for another flowtable/chain with a hook matching the new name
first, then unregister, try to register in the new spot and undo on
failure? Sounds doable. :)

Thanks, Phil

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > Or is this guaranteed by UNREGISTER event already?
> > If so, please add a comment.
> 
> Are packets relevant here? The question should be whether another CPU
> traverses hook->ops_list at the same time, no?

Yes, but also if anyone else can look at the structure in parallel.

> Looking at
> nft_flowtable_find_dev() mentioned in your other mail, there seems to be
> a case which doesn't synchronize on commit_mutex. So same rules apply to
> ops_list as for hook_list and thus I need to add an rcu_head to
> nf_hook_ops as well?

I will need to apply your series locally first to get the full picture,
sorry.

> > > +			if (ops) {
> > > +				memcpy(ops, &basechain->ops, sizeof(*ops));
> > > +				ops->dev = dev;
> > > +			}
> > > +			if (ops &&
> > > +			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
> > > +			     !nf_register_net_hook(dev_net(dev), ops))) {
> > > +				list_add_tail(&ops->list, &hook->ops_list);
> > > +				break;
> > > +			}
> > > +			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
> > > +			       ctx->chain->name, dev->name);
> > 
> > I think its better to -ENOMEM and veto the netdevice register request in this case.
> 
> Ah, I wasn't aware we may influence netdev registration from a notifier.
> So I'll change the callbacks to return NOTIFY_BAD in error case.
> 
> > I also think this needs extra handling for NETDEV_CHANGENAME rather than
> > the 'treat as UNREG+REG' trick.
> > 
> > Else we may unregister and then fail to re-register which leaves the
> > device without the registered hook op.
> 
> So search for another flowtable/chain with a hook matching the new name
> first, then unregister, try to register in the new spot and undo on
> failure? Sounds doable. :)

If possible i'd register new, then unreg old.
But, do you need to do anything on CHANGENAME at all?

Device is the same, so maybe its enough to update the name
in nft_hooks structure?

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter]

On Thu, Sep 12, 2024 at 05:12:03PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > Or is this guaranteed by UNREGISTER event already?
> > > If so, please add a comment.
> > 
> > Are packets relevant here? The question should be whether another CPU
> > traverses hook->ops_list at the same time, no?
> 
> Yes, but also if anyone else can look at the structure in parallel.

So it is possible that ops are still used somewhere after
nf_unregister_net_hook() returns? I don't quite get that code, with all
the RCU-bells and ONCE-whistles.

> > Looking at
> > nft_flowtable_find_dev() mentioned in your other mail, there seems to be
> > a case which doesn't synchronize on commit_mutex. So same rules apply to
> > ops_list as for hook_list and thus I need to add an rcu_head to
> > nf_hook_ops as well?
> 
> I will need to apply your series locally first to get the full picture,
> sorry.

No sorry, thanks for your review so far!

> > > > +			if (ops) {
> > > > +				memcpy(ops, &basechain->ops, sizeof(*ops));
> > > > +				ops->dev = dev;
> > > > +			}
> > > > +			if (ops &&
> > > > +			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
> > > > +			     !nf_register_net_hook(dev_net(dev), ops))) {
> > > > +				list_add_tail(&ops->list, &hook->ops_list);
> > > > +				break;
> > > > +			}
> > > > +			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
> > > > +			       ctx->chain->name, dev->name);
> > > 
> > > I think its better to -ENOMEM and veto the netdevice register request in this case.
> > 
> > Ah, I wasn't aware we may influence netdev registration from a notifier.
> > So I'll change the callbacks to return NOTIFY_BAD in error case.
> > 
> > > I also think this needs extra handling for NETDEV_CHANGENAME rather than
> > > the 'treat as UNREG+REG' trick.
> > > 
> > > Else we may unregister and then fail to re-register which leaves the
> > > device without the registered hook op.
> > 
> > So search for another flowtable/chain with a hook matching the new name
> > first, then unregister, try to register in the new spot and undo on
> > failure? Sounds doable. :)
> 
> If possible i'd register new, then unreg old.
> But, do you need to do anything on CHANGENAME at all?
> 
> Device is the same, so maybe its enough to update the name
> in nft_hooks structure?

You're putting the cart before the horse here: The user sets
hook->ifname and we bind to whatever device matches that.

Now with a device being renamed, there are two options:

A) Unbind if the name doesn't match hook->ifname anymore and search for
   another, matching hook. This is what I had (tried to) implement.

B) Just leave the interface in place as long as it exists. This is how
   the old code behaves.

For users, I find (A) more intuitive. Also, consider netdevs being
renamed by udev: Users may have a flowtable which matches the initial
name by accident. If it doesn't unbind them upon being renamed, they all
remain in there and may block the right flowtable from binding to them.

Cheers, Phil

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> So it is possible that ops are still used somewhere after
> nf_unregister_net_hook() returns? I don't quite get that code, with all
> the RCU-bells and ONCE-whistles.

Not ops, but other core could still run the hookfn that
is being unregistered, i.e.:

cpu 0
 unreg hookfn                      nf_hook_slow
                                      runs hookfn
if nf_hook() fetched the hook blob (struct nf_hook_entries *)
before unreg path called its rcu_assign_pointer with the updated
incarnation.

That said, this might be a special case as entire nf_hook_entries blob
is tied to device that is going down, so packet flow might have stopped
already.

From a brief glance the device is already disabled at NETDEV_UNREGISTER
time, no packets are flowing anymore.

> > Device is the same, so maybe its enough to update the name
> > in nft_hooks structure?
> 
> You're putting the cart before the horse here: The user sets
> hook->ifname and we bind to whatever device matches that.
> 
> Now with a device being renamed, there are two options:
> 
> A) Unbind if the name doesn't match hook->ifname anymore and search for
>    another, matching hook. This is what I had (tried to) implement.
> 
> B) Just leave the interface in place as long as it exists. This is how
>    the old code behaves.
> 
> For users, I find (A) more intuitive.

Yes, that makes sense to me.  But can't you defer the unbind until after
you've figured out if there is a matching hook or not?

I.e., if no matching new hook, just unreg, else register new/unregister
old.

Otherwise, we might unreg, then fail to re-register?

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter]

On Thu, Sep 12, 2024 at 06:06:39PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > So it is possible that ops are still used somewhere after
> > nf_unregister_net_hook() returns? I don't quite get that code, with all
> > the RCU-bells and ONCE-whistles.
> 
> Not ops, but other core could still run the hookfn that
> is being unregistered, i.e.:
> 
> cpu 0
>  unreg hookfn                      nf_hook_slow
>                                       runs hookfn
> if nf_hook() fetched the hook blob (struct nf_hook_entries *)
> before unreg path called its rcu_assign_pointer with the updated
> incarnation.
> 
> That said, this might be a special case as entire nf_hook_entries blob
> is tied to device that is going down, so packet flow might have stopped
> already.
> 
> From a brief glance the device is already disabled at NETDEV_UNREGISTER
> time, no packets are flowing anymore.

This seems like a relief, but doesn't apply to NETDEV_CHANGENAME case.
At least I can't handle it like UNREG && REG - or vice versa, have to
consider the above for UNREG.

> > > Device is the same, so maybe its enough to update the name
> > > in nft_hooks structure?
> > 
> > You're putting the cart before the horse here: The user sets
> > hook->ifname and we bind to whatever device matches that.
> > 
> > Now with a device being renamed, there are two options:
> > 
> > A) Unbind if the name doesn't match hook->ifname anymore and search for
> >    another, matching hook. This is what I had (tried to) implement.
> > 
> > B) Just leave the interface in place as long as it exists. This is how
> >    the old code behaves.
> > 
> > For users, I find (A) more intuitive.
> 
> Yes, that makes sense to me.  But can't you defer the unbind until after
> you've figured out if there is a matching hook or not?
> 
> I.e., if no matching new hook, just unreg, else register new/unregister
> old.

I can't bind a device to multiple flowtables of the same family, so I
can't bind first, then unbind.

> Otherwise, we might unreg, then fail to re-register?

Assuming I can't bind to the new flowtable while still bound to the old
one, I can only try to roll back. Which means trying to bind to the old
flowtable again in error case which might still fail. I could return
NOTIFY_BAD then, but it won't help - the transaction is broken.

Cheers, Phil

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > I.e., if no matching new hook, just unreg, else register new/unregister
> > old.
> 
> I can't bind a device to multiple flowtables of the same family, so I
> can't bind first, then unbind.

I'm dense, why does that not work?

Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter]

On Thu, Sep 12, 2024 at 10:43:57PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > I.e., if no matching new hook, just unreg, else register new/unregister
> > > old.
> > 
> > I can't bind a device to multiple flowtables of the same family, so I
> > can't bind first, then unbind.
> 
> I'm dense, why does that not work?

Well, nft_register_flowtable_net_hooks() searches for a same hook in
other flowtables of the same table ("same" as in hook->ops.dev and
hook->ops.pf values match) and returns -EEXIST if found.

Originally this check was added by Pablo:

| commit 32fc71875127498bf99cc648e96400ee0895edf7
| Author: Pablo Neira Ayuso <pablo@netfilter.org>
| Date:   Mon Feb 26 13:16:04 2018 +0100
| 
|     netfilter: nf_tables: return EBUSY if device already belongs to flowtable
|     
|     If the netdevice is already part of a flowtable, return EBUSY. I cannot
|     find a valid usecase for having two flowtables bound to the same
|     netdevice. We can still have two flowtable where the device set is
|     disjoint.
|     
|     Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

The comment luckily indicates there's no technical reason, so the reg
first approach may fly. Apart from that, I'll try getting rid of this
because it prevents things like ft1(eth0, eth1) && ft2(eth1, eth2) which
seems like a valid use-case to me.

Thanks for questioning the basics here! :)

Cheers, Phil

[nf-next PATCH v3 12/16] netfilter: nf_tables: flowtable: Respect NETDEV_REGISTER events

[Phil Sutter]

Hook into new devices if their name matches the hook spec.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c | 47 +++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 64f8305189f1..40cff8539c74 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9314,15 +9314,41 @@ static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, &flowtable->hook_list, list) {
-		ops = nft_hook_find_ops(hook, dev);
-		if (!ops)
-			continue;
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			ops = nft_hook_find_ops(hook, dev);
+			if (!ops)
+				continue;
 
-		/* flow_offload_netdev_event() cleans up entries for us. */
-		nft_unregister_flowtable_ops(dev_net(dev), flowtable, ops);
-		list_del(&ops->list);
-		kfree(ops);
-		break;
+			/* flow_offload_netdev_event() cleans up entries for us. */
+			nft_unregister_flowtable_ops(dev_net(dev),
+						     flowtable, ops);
+			list_del(&ops->list);
+			kfree(ops);
+			break;
+		case NETDEV_REGISTER:
+			if (strcmp(hook->ifname, dev->name))
+				continue;
+			ops = kzalloc(sizeof(struct nf_hook_ops),
+				      GFP_KERNEL_ACCOUNT);
+			if (ops) {
+				ops->pf		= NFPROTO_NETDEV;
+				ops->hooknum	= flowtable->hooknum;
+				ops->priority	= flowtable->data.priority;
+				ops->priv	= &flowtable->data;
+				ops->hook	= flowtable->data.type->hook;
+				ops->dev	= dev;
+			}
+			if (ops && !nft_register_flowtable_ops(dev_net(dev),
+							       flowtable, ops)) {
+				list_add_tail(&ops->list, &hook->ops_list);
+				break;
+			}
+			printk(KERN_ERR "flowtable %s: Can't hook into device %s\n",
+			       flowtable->name, dev->name);
+			kfree(ops);
+			continue;
+		}
 	}
 }
 
@@ -9335,8 +9361,9 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
 	struct nft_table *table;
 	struct net *net;
 
-	if (event != NETDEV_UNREGISTER)
-		return 0;
+	if (event != NETDEV_REGISTER &&
+	    event != NETDEV_UNREGISTER)
+		return NOTIFY_DONE;
 
 	net = dev_net(dev);
 	nft_net = nft_pernet(net);
-- 
2.43.0

[nf-next PATCH v3 13/16] netfilter: nf_tables: Handle NETDEV_CHANGENAME events

[Phil Sutter]

For the sake of simplicity, treat them like consecutive
NETDEV_UNREGISTER and NETDEV_REGISTER events.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c    | 4 ++++
 net/netfilter/nft_chain_filter.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 40cff8539c74..88528775732a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9361,6 +9361,10 @@ static int nf_tables_flowtable_event(struct notifier_block *this,
 	struct nft_table *table;
 	struct net *net;
 
+	if (event == NETDEV_CHANGENAME) {
+		nf_tables_flowtable_event(this, NETDEV_UNREGISTER, ptr);
+		event = NETDEV_REGISTER;
+	}
 	if (event != NETDEV_REGISTER &&
 	    event != NETDEV_UNREGISTER)
 		return NOTIFY_DONE;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index ec44c27a9d91..4f13591e5cd1 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -372,6 +372,10 @@ static int nf_tables_netdev_event(struct notifier_block *this,
 		.net	= dev_net(dev),
 	};
 
+	if (event == NETDEV_CHANGENAME) {
+		nf_tables_netdev_event(this, NETDEV_UNREGISTER, ptr);
+		event = NETDEV_REGISTER;
+	}
 	if (event != NETDEV_REGISTER &&
 	    event != NETDEV_UNREGISTER)
 		return NOTIFY_DONE;
-- 
2.43.0

[nf-next PATCH v3 14/16] netfilter: nf_tables: Support wildcard netdev hook specs

[Phil Sutter]

User space may pass non-nul-terminated NFTA_DEVICE_NAME attribute values
to indicate a suffix wildcard.
Expect for multiple devices to match the given prefix in
nft_netdev_hook_alloc() and populate 'ops_list' with them all.
When checking for duplicate hooks, compare the shortest prefix so a
device may never match more than a single hook spec.
Finally respect the stored prefix length when hooking into new devices
from event handlers.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_tables_api.c    | 31 +++++++++++++++----------------
 net/netfilter/nft_chain_filter.c |  2 +-
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 88528775732a..3632be26d73a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2225,24 +2225,22 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 	 * indirectly serializing all the other holders of the commit_mutex with
 	 * the rtnl_mutex.
 	 */
-	dev = __dev_get_by_name(net, hook->ifname);
-	if (!dev) {
-		err = -ENOENT;
-		goto err_hook_dev;
-	}
+	for_each_netdev(net, dev) {
+		if (strncmp(dev->name, hook->ifname, hook->ifnamelen))
+			continue;
 
-	ops = kzalloc(sizeof(struct nf_hook_ops), GFP_KERNEL_ACCOUNT);
-	if (!ops) {
-		err = -ENOMEM;
-		goto err_hook_dev;
+		ops = kzalloc(sizeof(struct nf_hook_ops), GFP_KERNEL_ACCOUNT);
+		if (!ops) {
+			err = -ENOMEM;
+			goto err_ops_alloc;
+		}
+		ops->dev = dev;
+		list_add_tail(&ops->list, &hook->ops_list);
 	}
-	ops->dev = dev;
-	list_add_tail(&ops->list, &hook->ops_list);
-
 	return hook;
 
-err_hook_dev:
-	kfree(hook);
+err_ops_alloc:
+	nft_netdev_hook_free(hook);
 err_hook_alloc:
 	return ERR_PTR(err);
 }
@@ -2253,7 +2251,8 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
 	struct nft_hook *hook;
 
 	list_for_each_entry(hook, hook_list, list) {
-		if (!strcmp(hook->ifname, this->ifname))
+		if (!strncmp(hook->ifname, this->ifname,
+			     min(hook->ifnamelen, this->ifnamelen)))
 			return hook;
 	}
 
@@ -9327,7 +9326,7 @@ static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 			kfree(ops);
 			break;
 		case NETDEV_REGISTER:
-			if (strcmp(hook->ifname, dev->name))
+			if (strncmp(hook->ifname, dev->name, hook->ifnamelen))
 				continue;
 			ops = kzalloc(sizeof(struct nf_hook_ops),
 				      GFP_KERNEL_ACCOUNT);
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 4f13591e5cd1..d691f8354049 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -338,7 +338,7 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 			kfree(ops);
 			break;
 		case NETDEV_REGISTER:
-			if (strcmp(hook->ifname, dev->name))
+			if (strncmp(hook->ifname, dev->name, hook->ifnamelen))
 				continue;
 			ops = kzalloc(sizeof(struct nf_hook_ops),
 				      GFP_KERNEL_ACCOUNT);
-- 
2.43.0

[nf-next PATCH v3 15/16] netfilter: nf_tables: Add notications for hook changes

[Phil Sutter]

Notify user space if netdev hooks are updated due to netdev add/remove
events. Send minimal notification messages by introducing
NFT_MSG_NEWDEV/DELDEV message types describing a single device only.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        |  2 +
 include/uapi/linux/netfilter/nf_tables.h |  5 +++
 net/netfilter/nf_tables_api.c            | 57 ++++++++++++++++++++++++
 net/netfilter/nft_chain_filter.c         |  1 +
 4 files changed, 65 insertions(+)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 991b8d5e52f1..3ebdb46b3993 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1131,6 +1131,8 @@ int nft_setelem_validate(const struct nft_ctx *ctx, struct nft_set *set,
 int nft_set_catchall_validate(const struct nft_ctx *ctx, struct nft_set *set);
 int nf_tables_bind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
 void nf_tables_unbind_chain(const struct nft_ctx *ctx, struct nft_chain *chain);
+void nf_tables_chain_device_notify(const struct nft_chain *chain,
+				   const struct net_device *dev, int event);
 
 enum nft_chain_types {
 	NFT_CHAIN_T_DEFAULT = 0,
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 639894ed1b97..0f350f2fc807 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -142,6 +142,8 @@ enum nf_tables_msg_types {
 	NFT_MSG_DESTROYOBJ,
 	NFT_MSG_DESTROYFLOWTABLE,
 	NFT_MSG_GETSETELEM_RESET,
+	NFT_MSG_NEWDEV,
+	NFT_MSG_DELDEV,
 	NFT_MSG_MAX,
 };
 
@@ -1772,6 +1774,9 @@ enum nft_synproxy_attributes {
 enum nft_devices_attributes {
 	NFTA_DEVICE_UNSPEC,
 	NFTA_DEVICE_NAME,
+	NFTA_DEVICE_TABLE,
+	NFTA_DEVICE_FLOWTABLE,
+	NFTA_DEVICE_CHAIN,
 	__NFTA_DEVICE_MAX
 };
 #define NFTA_DEVICE_MAX		(__NFTA_DEVICE_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3632be26d73a..cdca5dfbe0b5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9306,6 +9306,62 @@ struct nf_hook_ops *nft_hook_find_ops(const struct nft_hook *hook,
 }
 EXPORT_SYMBOL_GPL(nft_hook_find_ops);
 
+static void
+nf_tables_device_notify(const struct nft_table *table, int attr,
+			const char *name, const struct net_device *dev,
+			int event)
+{
+	struct net *net = dev_net(dev);
+	struct nlmsghdr *nlh;
+	struct sk_buff *skb;
+	u16 flags = 0;
+
+	if (!nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
+		return;
+
+	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!skb)
+		goto err;
+
+	event = event == NETDEV_REGISTER ? NFT_MSG_NEWDEV : NFT_MSG_DELDEV;
+	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
+	nlh = nfnl_msg_put(skb, 0, 0, event, flags, table->family,
+			   NFNETLINK_V0, nft_base_seq(net));
+	if (!nlh)
+		goto err;
+
+	if (nla_put_string(skb, NFTA_DEVICE_TABLE, table->name) ||
+	    nla_put_string(skb, attr, name) ||
+	    nla_put_string(skb, NFTA_DEVICE_NAME, dev->name))
+		goto err;
+
+	nlmsg_end(skb, nlh);
+	nfnetlink_send(skb, net, 0, NFNLGRP_NFTABLES,
+		       nlmsg_report(nlh), GFP_KERNEL);
+	return;
+err:
+	if (skb)
+		kfree_skb(skb);
+	nfnetlink_set_err(net, 0, NFNLGRP_NFTABLES, -ENOBUFS);
+}
+
+void
+nf_tables_chain_device_notify(const struct nft_chain *chain,
+			      const struct net_device *dev, int event)
+{
+	nf_tables_device_notify(chain->table, NFTA_DEVICE_CHAIN,
+				chain->name, dev, event);
+}
+
+static void
+nf_tables_flowtable_device_notify(const struct nft_flowtable *ft,
+				  const struct net_device *dev, int event)
+{
+	nf_tables_device_notify(ft->table, NFTA_DEVICE_FLOWTABLE,
+				ft->name, dev, event);
+}
+
+
 static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 				struct nft_flowtable *flowtable)
 {
@@ -9348,6 +9404,7 @@ static void nft_flowtable_event(unsigned long event, struct net_device *dev,
 			kfree(ops);
 			continue;
 		}
+		nf_tables_flowtable_device_notify(flowtable, dev, event);
 	}
 }
 
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index d691f8354049..c9de9b12f772 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -357,6 +357,7 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
 			kfree(ops);
 			continue;
 		}
+		nf_tables_chain_device_notify(ctx->chain, dev, event);
 	}
 }
 
-- 
2.43.0

[nf-next PATCH v3 16/16] selftests: netfilter: Torture nftables netdev hooks

[Phil Sutter]

Add a ruleset which binds to various interface names via netdev-family
chains and flowtables and massage the notifiers by frequently renaming
interfaces to match these names. While doing so:
- Keep an 'nft monitor' running in background to receive the notifications
- Loop over 'nft list ruleset' to exercise ruleset dump codepath
- Have iperf running so the involved chains/flowtables see traffic

If supported, also test interface wildcard support separately by
creating a flowtable with 'wild*' interface spec and quickly add/remove
matching dummy interfaces.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../testing/selftests/net/netfilter/Makefile  |   1 +
 .../net/netfilter/nft_interface_stress.sh     | 149 ++++++++++++++++++
 2 files changed, 150 insertions(+)
 create mode 100755 tools/testing/selftests/net/netfilter/nft_interface_stress.sh

diff --git a/tools/testing/selftests/net/netfilter/Makefile b/tools/testing/selftests/net/netfilter/Makefile
index d13fb5ea3e89..823e0acf7171 100644
--- a/tools/testing/selftests/net/netfilter/Makefile
+++ b/tools/testing/selftests/net/netfilter/Makefile
@@ -21,6 +21,7 @@ TEST_PROGS += nft_concat_range.sh
 TEST_PROGS += nft_conntrack_helper.sh
 TEST_PROGS += nft_fib.sh
 TEST_PROGS += nft_flowtable.sh
+TEST_PROGS += nft_interface_stress.sh
 TEST_PROGS += nft_meta.sh
 TEST_PROGS += nft_nat.sh
 TEST_PROGS += nft_nat_zones.sh
diff --git a/tools/testing/selftests/net/netfilter/nft_interface_stress.sh b/tools/testing/selftests/net/netfilter/nft_interface_stress.sh
new file mode 100755
index 000000000000..92ce1d35ec19
--- /dev/null
+++ b/tools/testing/selftests/net/netfilter/nft_interface_stress.sh
@@ -0,0 +1,149 @@
+#!/bin/bash -e
+#
+# SPDX-License-Identifier: GPL-2.0
+#
+# Torture nftables' netdevice notifier callbacks and related code by frequent
+# renaming of interfaces which netdev-family chains and flowtables hook into.
+
+source lib.sh
+
+checktool "nft --version" "run test without nft tool"
+checktool "iperf3 --version" "run test without iperf3 tool"
+
+# how many seconds to torture the kernel, default to 80% of max run time
+TEST_RUNTIME=$((${kselftest_timeout:-60} * 8 / 10))
+
+trap "cleanup_all_ns" EXIT
+
+setup_ns nsc nsr nss
+
+ip -net $nsc link add cr0 type veth peer name rc0 netns $nsr
+ip -net $nsc addr add 10.0.0.1/24 dev cr0
+ip -net $nsc link set cr0 up
+ip -net $nsc route add default via 10.0.0.2
+
+ip -net $nss link add sr0 type veth peer name rs0 netns $nsr
+ip -net $nss addr add 10.1.0.1/24 dev sr0
+ip -net $nss link set sr0 up
+ip -net $nss route add default via 10.1.0.2
+
+ip -net $nsr addr add 10.0.0.2/24 dev rc0
+ip -net $nsr link set rc0 up
+ip -net $nsr addr add 10.1.0.2/24 dev rs0
+ip -net $nsr link set rs0 up
+ip netns exec $nsr sysctl -q net.ipv4.ip_forward=1
+ip netns exec $nsr sysctl -q net.ipv4.conf.all.forwarding=1
+
+{
+	echo "table netdev t {"
+	for ((i = 0; i < 10; i++)); do
+		cat <<-EOF
+		chain chain_rc$i {
+			type filter hook ingress device rc$i priority 0
+			counter
+		}
+		chain chain_rs$i {
+			type filter hook ingress device rs$i priority 0
+			counter
+		}
+		EOF
+	done
+	echo "}"
+	echo "table ip t {"
+	for ((i = 0; i < 10; i++)); do
+		cat <<-EOF
+		flowtable ft_${i} {
+			hook ingress priority 0
+			devices = { rc$i, rs$i }
+		}
+		EOF
+	done
+	echo "chain c {"
+	echo "type filter hook forward priority 0"
+	for ((i = 0; i < 10; i++)); do
+		echo -n "iifname rc$i oifname rs$i "
+		echo    "ip protocol tcp counter flow add @ft_${i}"
+	done
+	echo "counter"
+	echo "}"
+	echo "}"
+} | ip netns exec $nsr nft -f - || {
+	echo "SKIP: Could not load nft ruleset"
+	exit $ksft_skip
+}
+
+for ((o=0, n=1; ; o=n, n++, n %= 10)); do
+	ip -net $nsr link set rc$o name rc$n
+	ip -net $nsr link set rs$o name rs$n
+done &
+rename_loop_pid=$!
+
+while true; do ip netns exec $nsr nft list ruleset >/dev/null 2>&1; done &
+nft_list_pid=$!
+
+ip netns exec $nsr nft monitor >/dev/null &
+nft_monitor_pid=$!
+
+ip netns exec $nss iperf3 --server --daemon -1
+summary_expr='s,^\[SUM\] .* \([0-9]\+\) Mbits/sec .* receiver,\1,p'
+rate=$(ip netns exec $nsc iperf3 \
+	--format m -c 10.1.0.1 --time $TEST_RUNTIME \
+	--length 56 --parallel 10 -i 0 | sed -n "$summary_expr")
+
+kill $nft_list_pid
+kill $nft_monitor_pid
+kill $rename_loop_pid
+wait
+
+ip netns exec $nsr nft -f - <<EOF
+table ip t {
+	flowtable ft_wild {
+		hook ingress priority 0
+		devices = { wild* }
+	}
+}
+EOF
+if [[ $? -ne 0 ]]; then
+	echo "SKIP wildcard tests: not supported by host's nft?"
+else
+	for ((i = 0; i < 100; i++)); do
+		ip -net $nsr link add wild$i type dummy &
+	done
+	wait
+	for ((i = 80; i < 100; i++)); do
+		ip -net $nsr link del wild$i &
+	done
+	for ((i = 0; i < 80; i++)); do
+		ip -net $nsr link del wild$i &
+	done
+	wait
+	for ((i = 0; i < 100; i += 10)); do
+		(
+		for ((j = 0; j < 10; j++)); do
+			ip -net $nsr link add wild$((i + j)) type dummy
+		done
+		for ((j = 0; j < 10; j++)); do
+			ip -net $nsr link del wild$((i + j))
+		done
+		) &
+	done
+	wait
+fi
+
+[[ $(</proc/sys/kernel/tainted) -eq 0 ]] || {
+	echo "FAIL: Kernel is tainted!"
+	exit $ksft_fail
+}
+
+[[ $rate -gt 0 ]] || {
+	echo "FAIL: Zero throughput in iperf3"
+	exit $ksft_fail
+}
+
+[[ -f /sys/kernel/debug/kmemleak && \
+   -n $(</sys/kernel/debug/kmemleak) ]] && {
+	echo "FAIL: non-empty kmemleak report"
+	exit $ksft_fail
+}
+
+exit $ksft_pass
-- 
2.43.0