Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> writes:
> 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().
Good point, and that really cleans things up.
> 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.
At this point I don't see anything that suggests we need to get rid of
the rtnl_lock. We just don't want to take it twice!
With the nf_hook_drop change all of the logic except for the
compatibility code is rtnl_lock free, and ought to stay that way. So I
think we are good.
If it comes up later that taking the rtnl_lock is a problem we can
convert everything to use nf_register_net_hook and
nf_unregister_net_hook. With the structure allocation performed in
those two functions the change is mostly code motion.
Eric
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
