Phil Sutter <phil@xxxxxx> 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.
> 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.
> > > + 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?
