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

[Florian Westphal <fw@strlen.de>]

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.

> +			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, .. ?

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

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.