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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150620163521.GC6915@acer.localdomain>
Date:	Sat, 20 Jun 2015 18:35:21 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
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

On 20.06, Eric W. Biederman wrote:
> 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?

No, its fine, we have almost none interactions except for network
namespaces and device lookups.

Main reason why I'd prefer a non-RTNL version is because your
callbacks introduce bigger chunks of code under the RTNL, so
it might complicate things in the future. But your reasoning is
sound and for now this is perfectly fine.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ