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 04:40:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Hook into new devices if their name matches the hook spec.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  net/netfilter/nft_chain_filter.c | 40 +++++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
> > index 2507e3beac5c..ec44c27a9d91 100644
> > --- a/net/netfilter/nft_chain_filter.c
> > +++ b/net/netfilter/nft_chain_filter.c
> > @@ -326,14 +326,37 @@ static void nft_netdev_event(unsigned long event, struct net_device *dev,
> >  	struct nft_hook *hook;
> >  
> >  	list_for_each_entry(hook, &basechain->hook_list, list) {
> > -		ops = nft_hook_find_ops(hook, dev);
> > -		if (!ops)
> > -			continue;
> > +		switch (event) {
> > +		case NETDEV_UNREGISTER:
> > +			ops = nft_hook_find_ops(hook, dev);
> > +			if (!ops)
> > +				continue;
> >  
> > -		if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> > -			nf_unregister_net_hook(ctx->net, ops);
> > -		list_del(&ops->list);
> > -		kfree(ops);
> > +			if (!(ctx->chain->table->flags & NFT_TABLE_F_DORMANT))
> > +				nf_unregister_net_hook(ctx->net, ops);
> > +			list_del(&ops->list);
> > +			kfree(ops);
> 
> This needs to use kfree_rcu() + list_del_rcu, nf_unregister_net_hook
> only stops new packets from executing for dev, it doesn't stop new
> packets.
> 
> 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? 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?

> > +			break;
> > +		case NETDEV_REGISTER:
> > +			if (strcmp(hook->ifname, dev->name))
> > +				continue;
> > +			ops = kzalloc(sizeof(struct nf_hook_ops),
> > +				      GFP_KERNEL_ACCOUNT);
> 
> ops = kmemdup(&basechain->ops, .. ?

Oh, sure!

> > +			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. :)

Thanks, Phil