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

On 8/26/26 6:22 PM, Phil Sutter wrote:
> 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 <[email protected]>
>> 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 <[email protected]>
>> ---
>>   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.
> 

Oops, yes sorry.

>>   {
>>   	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.

Hm. I am not so sure about it. Wouldn't that allow for overlapping 
devices with prefixes?

Given this is not straightforward and we need to think a bit about it, 
Pablo leave this out of the next PR to netdev. I am going to do a bit 
more testing.

> 
>>   					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.
> 

Thank you Phil for these reviews!

> 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
>>
>>