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 05:12:03PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > Or is this guaranteed by UNREGISTER event already?
> > > If so, please add a comment.
> > 
> > Are packets relevant here? The question should be whether another CPU
> > traverses hook->ops_list at the same time, no?
> 
> Yes, but also if anyone else can look at the structure in parallel.

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.

> > Looking at
> > nft_flowtable_find_dev() mentioned in your other mail, there seems to be
> > a case which doesn't synchronize on commit_mutex. So same rules apply to
> > ops_list as for hook_list and thus I need to add an rcu_head to
> > nf_hook_ops as well?
> 
> I will need to apply your series locally first to get the full picture,
> sorry.

No sorry, thanks for your review so far!

> > > > +			if (ops) {
> > > > +				memcpy(ops, &basechain->ops, sizeof(*ops));
> > > > +				ops->dev = dev;
> > > > +			}
> > > > +			if (ops &&
> > > > +			    (ctx->chain->table->flags & NFT_TABLE_F_DORMANT ||
> > > > +			     !nf_register_net_hook(dev_net(dev), ops))) {
> > > > +				list_add_tail(&ops->list, &hook->ops_list);
> > > > +				break;
> > > > +			}
> > > > +			printk(KERN_ERR "chain %s: Can't hook into device %s\n",
> > > > +			       ctx->chain->name, dev->name);
> > > 
> > > I think its better to -ENOMEM and veto the netdevice register request in this case.
> > 
> > Ah, I wasn't aware we may influence netdev registration from a notifier.
> > So I'll change the callbacks to return NOTIFY_BAD in error case.
> > 
> > > I also think this needs extra handling for NETDEV_CHANGENAME rather than
> > > the 'treat as UNREG+REG' trick.
> > > 
> > > Else we may unregister and then fail to re-register which leaves the
> > > device without the registered hook op.
> > 
> > So search for another flowtable/chain with a hook matching the new name
> > first, then unregister, try to register in the new spot and undo on
> > failure? Sounds doable. :)
> 
> If possible i'd register new, then unreg old.
> But, do you need to do anything on CHANGENAME at all?
> 
> 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. Also, consider netdevs being
renamed by udev: Users may have a flowtable which matches the initial
name by accident. If it doesn't unbind them upon being renamed, they all
remain in there and may block the right flowtable from binding to them.

Cheers, Phil