netdev - Re: [PATCH net] netfilter: nf_qeueue: Drop queue entries on nf_unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87616i8nyv.fsf@x220.int.ebiederm.org>
Date:	Sat, 20 Jun 2015 09:16:08 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Patrick McHardy <kaber@...sh.net>
Cc:	Pablo Neira Ayuso <pablo@...filter.org>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net] netfilter: nf_qeueue: Drop queue entries on nf_unregister_hook

Patrick McHardy <kaber@...sh.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 netdev" in