Re: [nf-next PATCH v3 11/16] netfilter: nf_tables: chain: Respect NETDEV_REGISTER events

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 12, 2024 at 06:06:39PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > So it is possible that ops are still used somewhere after
> > nf_unregister_net_hook() returns? I don't quite get that code, with all
> > the RCU-bells and ONCE-whistles.
> 
> Not ops, but other core could still run the hookfn that
> is being unregistered, i.e.:
> 
> cpu 0
>  unreg hookfn                      nf_hook_slow
>                                       runs hookfn
> if nf_hook() fetched the hook blob (struct nf_hook_entries *)
> before unreg path called its rcu_assign_pointer with the updated
> incarnation.
> 
> That said, this might be a special case as entire nf_hook_entries blob
> is tied to device that is going down, so packet flow might have stopped
> already.
> 
> From a brief glance the device is already disabled at NETDEV_UNREGISTER
> time, no packets are flowing anymore.

This seems like a relief, but doesn't apply to NETDEV_CHANGENAME case.
At least I can't handle it like UNREG && REG - or vice versa, have to
consider the above for UNREG.

> > > Device is the same, so maybe its enough to update the name
> > > in nft_hooks structure?
> > 
> > You're putting the cart before the horse here: The user sets
> > hook->ifname and we bind to whatever device matches that.
> > 
> > Now with a device being renamed, there are two options:
> > 
> > A) Unbind if the name doesn't match hook->ifname anymore and search for
> >    another, matching hook. This is what I had (tried to) implement.
> > 
> > B) Just leave the interface in place as long as it exists. This is how
> >    the old code behaves.
> > 
> > For users, I find (A) more intuitive.
> 
> Yes, that makes sense to me.  But can't you defer the unbind until after
> you've figured out if there is a matching hook or not?
> 
> I.e., if no matching new hook, just unreg, else register new/unregister
> old.

I can't bind a device to multiple flowtables of the same family, so I
can't bind first, then unbind.

> Otherwise, we might unreg, then fail to re-register?

Assuming I can't bind to the new flowtable while still bound to the old
one, I can only try to roll back. Which means trying to bind to the old
flowtable again in error case which might still fail. I could return
NOTIFY_BAD then, but it won't help - the transaction is broken.

Cheers, Phil