Re: [PATCH nf v3] netfilter: nf_tables: fix device name and prefix match in hook lookup

[Phil Sutter <phil@nwl.cc>]

On Wed, Aug 26, 2026 at 03:32:08PM +0200, Fernando Fernandez Mancera wrote:
> Currently, a netdev chain or flowtable hooked to a device prefix can be
> unintentionally deleted by a control-plane request targeting an exact
> device name or even a shorter one due to the usage of min() to calculate
> the length to match.
> 
> Fix this by making sure an exact device match never matches a prefix and
> that both the target and the candidate have the same length during
> delete operation. The add and update paths retain the existing overlap
> matching to prevent a single device from matching multiple hooks.
> 
> Reported-by: Wei Fang <void0red@gmail.com>
> Closes: https://lore.kernel.org/netfilter-devel/CANE+tVrDeNCHQVmsqkV2ozeBqyE3GtRDMhZgsg1bhw10yGNTRQ@mail.gmail.com/
> Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/netfilter/nf_tables_api.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c112ecc4fca3..9ef12feec1ac 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1978,7 +1978,7 @@ static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
>  	return -ENOSPC;
>  }
>  
> -static bool hook_is_prefix(struct nft_hook *hook)
> +static bool hook_is_prefix(const struct nft_hook *hook)

This is an unrelated change now.

>  {
>  	return strlen(hook->ifname) >= hook->ifnamelen;
>  }
> @@ -2440,11 +2440,14 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
>  }
>  
>  static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
> -					   const struct nft_hook *this)
> +					   const struct nft_hook *this,
> +					   bool strict)
>  {
>  	struct nft_hook *hook;
>  
>  	list_for_each_entry(hook, hook_list, list) {
> +		if (strict && hook->ifnamelen != this->ifnamelen)
> +			continue;
>  		if (!strncmp(hook->ifname, this->ifname,
>  			     min(hook->ifnamelen, this->ifnamelen))) {
>  			if (hook->flags & NFT_HOOK_REMOVE)
> @@ -2486,7 +2489,7 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
>  			err = PTR_ERR(hook);
>  			goto err_hook;
>  		}
> -		if (nft_hook_list_find(hook_list, hook)) {
> +		if (nft_hook_list_find(hook_list, hook, false)) {
>  			NL_SET_BAD_ATTR(extack, tmp);
>  			nft_netdev_hook_free(hook);
>  			err = -EEXIST;
> @@ -2943,7 +2946,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
>  					ops->hook	= basechain->ops.hook;
>  				}
>  
> -				if (nft_hook_list_find(&basechain->hook_list, h)) {
> +				if (nft_hook_list_find(&basechain->hook_list, h, false)) {

I think this should be a strict match. It fixes a problem you didn't
intend to fix (I feel like an LLM-reviewer now), when updating a chain
with a partially matching wildcard:

| add table netdev t
| add chain netdev t c '{ type filter hook ingress priority 0; devices = { eth* }; }'
| add chain netdev t c '{ type filter hook ingress priority 0; devices = { et* }; }'

The last command should return EEXIST instead of being accepted and
treated as a NOP.

>  					list_del(&h->list);
>  					nft_netdev_hook_free(h);
>  					continue;
> @@ -2956,7 +2959,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
>  					    !nft_trans_chain_update(trans))
>  						continue;
>  
> -					if (nft_hook_list_find(&nft_trans_chain_hooks(trans), h)) {
> +					if (nft_hook_list_find(&nft_trans_chain_hooks(trans),
> +							       h, false)) {
>  						nft_chain_release_hook(&hook);
>  						return -EEXIST;
>  					}
> @@ -3257,7 +3261,7 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
>  		return err;
>  
>  	list_for_each_entry(this, &chain_hook.list, list) {
> -		hook = nft_hook_list_find(&basechain->hook_list, this);
> +		hook = nft_hook_list_find(&basechain->hook_list, this, true);
>  		if (!hook) {
>  			err = -ENOENT;
>  			goto err_chain_del_hook;
> @@ -9073,7 +9077,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
>  			if (!nft_is_active_next(net, ft))
>  				continue;
>  
> -			if (nft_hook_list_find(&ft->hook_list, hook)) {
> +			if (nft_hook_list_find(&ft->hook_list, hook, false)) {
>  				err = -EEXIST;
>  				goto err_unregister_net_hooks;
>  			}
> @@ -9150,7 +9154,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
>  		return err;
>  
>  	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
> -		if (nft_hook_list_find(&flowtable->hook_list, hook)) {
> +		if (nft_hook_list_find(&flowtable->hook_list, hook, false)) {

Same here.

I'll write test cases for nftables shell test suite to cover these.

Thanks, Phil

>  			list_del(&hook->list);
>  			nft_netdev_hook_free(hook);
>  			continue;
> @@ -9163,7 +9167,7 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
>  			    !nft_trans_flowtable_update(trans))
>  				continue;
>  
> -			if (nft_hook_list_find(&nft_trans_flowtable_hooks(trans), hook)) {
> +			if (nft_hook_list_find(&nft_trans_flowtable_hooks(trans), hook, false)) {
>  				err = -EEXIST;
>  				goto err_flowtable_update_hook;
>  			}
> @@ -9383,7 +9387,7 @@ static int nft_delflowtable_hook(struct nft_ctx *ctx,
>  		return err;
>  
>  	list_for_each_entry(this, &flowtable_hook.list, list) {
> -		hook = nft_hook_list_find(&flowtable->hook_list, this);
> +		hook = nft_hook_list_find(&flowtable->hook_list, this, true);
>  		if (!hook) {
>  			err = -ENOENT;
>  			goto err_flowtable_del_hook;
> -- 
> 2.55.0
> 
>