Re: [nf-next RFC] netfilter: nf_tables: Feature ifname-based hook registration

[Phil Sutter <phil@nwl.cc>]

Hi Florian,

On Thu, Jul 03, 2025 at 12:39:32AM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Require user space to set a flag upon flowtable or netdev-family chain
> > creation explicitly relaxing the hook registration when it comes to
> > non-existent interfaces. For the sake of simplicity, just restore error
> > condition if a given hook does not find an interface to bind to, leave
> > everyting else in place.
> 
> OK, but then this needs to go in via nf.git and:
> 
> Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> 
> tag.  We shouldn't introduce a "error" -> "no error" -> "error" semantic
> change sequence in kernel releases, i.e. this change is urgent; its now
> (before 6.16 release) or never.

Oh, right. So a decision whether this is feasible and if, how it should
behave in detail, is urgent.

> > - A wildcard interface spec is accepted as long as at least a single
> >   interface matches.
> 
> Is there a reason for this? Why are they handled differently?

I wasn't sure if it's "required" to prevent it as well or not. This
patch was motivated by Pablo reporting users would not notice mis-typed
interface names anymore and asking for whether introducing a feature
flag for it was possible. So I went ahead to have something for a
discussion.

Actually, wildcards are not handled differently: If user specifies
"eth123", kernel errors if no "eth123" exists and accepts otherwise. If
user specifies "eth*", kernel errors if no interface with that prefix
exists and accepts otherwise.

I don't know where to go with this. If the flag should turn interface
specs name-based, its absence should fully restore the old behaviour (as
you kindly summarized below). If it's just about the typo, this patch
might be fine.

> > - Dynamic unregistering and re-registering of vanishing/re-appearing
> >   interfaces is still happening.
> 
> You mean, without the flag? AFAIU old behaviour is:
> For netdev chains:
> - auto-removal AND free of device basechain -> no reappearance
> - -ENOENT error on chain add if device name doesn't exist
> For flowtable:
> - device is removed from the list (and list can become empty), flowtable
>   stays 100%, just the device name disappears from the devices list.
>   Doesn't reappear (auto re-added) either.
> - -ENOENT error on flowtable add if even one device doesn't exist
> 
> Neither netdev nor flowtable support "foo*" wildcards.
> 
> nf.git:
> - netdev basechain kept alive, no freeing, auto-reregister (becomes
>   active again if device with same name reappears).
>   No error if device name doesn't exists -> delayed auto-register
>   instead, including multi-reg for "foo*" case.
> - flowtable: same as old BUT device is auto-(re)added if same name
>   (re)appears.
> - No -ENOENT error on flowtable add, even if no single device existed
> 
> Full "foo*" support.
> 
> Now (this patch, without new flag):
> - netdev basechain: same as above.
>   But you do get an error if the device name did not exist.
>   Unless it was for "foo*", thats accepted even if no match is found.

No, this patch has the kernel error also if it doesn't find a match for
the wildcard. It merely asserts that the hook's ops_list is non-empty
after nft_netdev_hook_alloc() (which did the search for matching
interfaces) returns.

>   AFAICS its a userspace/nft change, ie. the new flag is actually
>   provided silently in the "foo*" case?
> - flowtable: same as old BUT device is auto-(re)added if same name
>   (re)appears.
> - -ENOENT error on flowtable add if even one device doesn't exist
>   Except "foo*" case, then its ok even if no match found.
> 
> Maybe add a table that explains the old/new/wanted (this patch) behaviours?
> And an explanation/rationale for the new flag?
> 
> Is there a concern that users depend on old behaviour?
> If so, why are we only concerned about the "add" behaviour but not the
> auto-reregistering?
> 
> Is it to protect users from typos going unnoticed?
> I could imagine "wlp0s20f1" getting misspelled occasionally...

Yes, that was the premise upon which I wrote the patch. I didn't intend
to make the flag toggle between the old interface hooks and the new
interface name hooks.

> > Note that this flag is persistent, i.e. included in ruleset dumps. This
> > effectively makes it "updatable": User space may create a "name-based"
> > flowtable for a non-existent interface, then update the flowtable to
> > drop the flag. What should happen then? Right now this is simply
> > accepted, even though the flowtable still does not bind to an interface.
> 
> AFAIU:
> If we accept off -> On, the flowtable should bind.
> If we accept on -> off, then it looks we should continue to drop devices
> from the list but just stop auto-readding?
> 
> If in doubt the flag should not be updateable (hard error), in
> that case we can refine/relax later.

My statement above was probably a bit confusing: With non-persistent, I
meant for the flag to be recognized upon chain/flowtable creation but
not added to chain->flags or flowtable->data.flags.

Cheers, Phil