Re: [PATCH nf-next] netfilter: nf_queue: fix deadlock in nf_queue_nf_hook_drop()

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Jul 22, 2015 at 03:06:12PM -0500, Eric W. Biederman wrote:
[...]
> This  code can be simplifed to:
> 
> diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
> index 4ab256824f5f..d002284e443b 100644
> --- a/net/netfilter/nf_queue.c
> +++ b/net/netfilter/nf_queue.c
> @@ -113,8 +113,7 @@ void nf_queue_nf_hook_drop(struct nf_hook_ops *ops)
>  	rcu_read_lock();
>  	qh = rcu_dereference(queue_handler);
>  	if (qh) {
> 		for_each_net_rcu(net)
>  			qh->nf_hook_drop(net, ops);
>  	}
>  	rcu_read_unlock();
> 
> In this particular case for_each_net_rcu is safe because:
> 
> - We don't care about new network namespaces that are added to the list
>   as it is walked as the nf_hook_ops are no longer registered, so 
>   they will not accumulate.
> 
> - We don't care about about network namespaces leaving the list as it
>   is walked as they ultimately call instance_destroy_rcu and clean
>   up their queues even if the drop hook is not called.
> 
> This matters as nf_unregister_net_hook can call nf_queue_nf_hook_drop
> without the rtnl_lock held when it is called from say nftables directly
> instead of from nf_unregister_hook.

Just noticed, nf_unregister_net_hook() should only destroy the queue
for this netns, not for every netns. I'm going to send a v2 to fix
nf_queue_nf_hook_drop().

BTW, on a different front, I don't see at this moment an easy way to
get rid of the rtnl_lock dependency through for_each_net_rcu() from
nf_register_hook(). We may skip new netns instances that are just
being added as the list is walked. Unless you have any better idea, we
will have to go back to the patches that propagate the complexity to
hook clients at some point if we need to skip the rtnl_lock
dependency.