Re: [PATCH net] netfilter: nf_qeueue: Drop queue entries on nf_unregister_hook

[ebiederm@xmission.com (Eric W. Biederman)]

Patrick McHardy <kaber@trash.net> writes:

> On 20.06, Pablo Neira Ayuso wrote:
>> On Fri, Jun 19, 2015 at 02:03:39PM -0500, Eric W. Biederman wrote:
>> > 
>> > Add code to nf_unregister_hook to flush the nf_queue when a hook is
>> > unregistered.  This guarantees that the pointer that the nf_queue code
>> > retains into the nf_hook list will remain valid while a packet is
>> > queued.
>> 
>> I think the real problem is that struct nf_queue_entry holds a pointer
>> to struct nf_hook_ops, which will be gone after removal. So you
>> uncovered a long standing problem that will amplify by when pernet
>> hooks are in place.
>> 
>> Regarding the pointer to nf_hook_list, now that new netdevice variant
>> doesn't support nf_queue yet, so that nf_hook_list will be always
>> valid since it will point to the global nf_hooks in the core.
>
> I think Eric's patch is the right thing to do. I'm not sure I get
> your netdev comment, but we certainly do want to drop packets once
> a hook is gone.
>
>> > +{
>> > +	const struct nf_queue_handler *qh;
>> > +	struct net *net;
>> > +
>> > +	rtnl_lock();
>> 
>> Why rtnl_lock() here?
>
> for_each_net(). Would actually be nice to have a variant that doesn't
> need the rtnl since it makes locking order analysis a lot harder.

Someone added a for_each_net_rcu.  But right now I am not at all certain
I trust an rcu variant not to miss something, in a weird corner case.
When missing something translates to an unprivileged user triggerable
kernel oops I am not ready to play games.

As for the lock analysis.  Except for nf_tables nf_unregister_hook is
called by module removal routines where rtnl_lock() is safe.

With nftables we seem to do everything under some version of the
nfnl_lock.  Does the nfnl_lock have any problems with taking the
rtnl_lock to nest underneath it?

I tested this path and I did not have any practical problems, but I
don't think I had lockdep enabled at the time.

Eric

>> > +	rcu_read_lock();
>> > +	qh = rcu_dereference(queue_handler);
>> > +	if (qh) {
>> > +		for_each_net(net) {
>> > +			qh->nf_hook_drop(net, ops);
>> > +		}
>> > +	}
>> > +	rcu_read_unlock();
>> > +	rtnl_unlock();
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in