On Thu, Jul 03, 2025 at 12:21:17PM +0200, Phil Sutter wrote:
> Hi Florian,
>
> On Thu, Jul 03, 2025 at 12:39:32AM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@xxxxxx> 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.
Downside is that this flag adds more complexity, since there will be
two paths to test (flag on/off).
> > > - 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.
Another concern is having a lot of devices, this is now interating
linearly performing strcmp() to find matches from the control plane
(ie. maybe this slow down time to load ruleset?), IIRC you mentioned
this should not be an issue.
> > > - 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.
Mistyped name is another scenario this flag could help.
> > > 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.
If this flag is added, I won't allow for updates until such
possibility is carefully review, having all possible tricky scenarios
in mind.
I think it boils down to the extra complexity that this flag adds is
worth having or documenting the new behaviour is sufficient, assuming
the two issues that have been mentioned are not problematic.
