On Thu, Sep 12, 2024 at 04:40:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> wrote:
> > Hook into new devices if their name matches the hook spec.
> >
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > 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
