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

On 8/26/26 9:06 PM, Phil Sutter wrote:
> On Wed, Aug 26, 2026 at 06:47:57PM +0200, Fernando Fernandez Mancera wrote:
>> 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?
> 
> You're right! I tested my concern with a flowtable, which behaves as I
> expected (so the strict lookup suggested below works as it should). With
> chains however, there is no duplicate hook check as for flowtables in
> nft_register_flowtable_net_hooks.
> 
> I think the policy was to allow multiple netdev chains on the same
> interface but not multiple flowtables. And IIRC, it wasn't clear whether
> this restriction is really needed (e.g. for hw offloading) or kept "just
> in case".
> 
> Either way, the duplicate hook check for flowtables just coincidentally
> happens to do what we need, namely reject non-strict matching hooks in
> the same flowtable/chain. This deserves a distinct lookup loop for both
> flowtables and chains IMO.
> 

I agree. I am sending a v4 of this patch removing the unrelated change 
and let's address the other problem in a follow-up to nf-next. We should 
likely unify this behavior between flowtables and chains.

> I'll look into that tomorrow and once I have something I'll also submit
> the test cases I wrote.
> 
>> 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.
> 
> Sorry for putting obstacles in your way. :(

No need to apologize, you found interesting issues in the patch :)

Thanks,
Fernando.

> 
> Cheers, Phil
>