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: <20150516.170819.1082459707799000658.davem@davemloft.net>
Date:	Sat, 16 May 2015 17:08:19 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	herbert@...dor.apana.org.au
Cc:	eric.dumazet@...il.com, tgraf@...g.ch, netdev@...r.kernel.org,
	ying.xue@...driver.com
Subject: Re: [net] netlink: Make autobind rover an atomic_t

From: Herbert Xu <herbert@...dor.apana.org.au>
Date: Sat, 16 May 2015 21:40:07 +0800

> The commit 21e4902aea80ef35afc00ee8d2abdea4f519b7f7 ("netlink:
> Lockless lookup with RCU grace period in socket release") removed
> the locks around the autobind rover without making the rover itself
> safe for use by multiple threads.
> 
> This patch converts rover to an atomic_t to make it at least
> somewhat safe to use locklessly.  The tricky bit is when the
> rover wraps around.  This patch simply deals with it by blindly
> doing an atomic_set.  So if many threads encounter the wraparound
> simultaneously then they'll all step on each other's toes and
> all try to bind to -4097.  But this should eventually sort itself
> out as they loop around and try the atomic_dec_return after the
> last thread does an atomic_set.
> 
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>

As far as I can tell, this ought to be fine as-is.

Everyone synchronizes on the netlink_insert().

And the rover is just a heuristic to find a free negative
portid quickly.  If the cpus walk on top of eachother, it
will sort itself out in the end.

There is one part of your patch we certainly do need, and
that's the correction of 'portid' when rover rolls over.

Something like the following.

What do you think?

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index dbe8859..bd26e69 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1305,7 +1305,7 @@ retry:
 		/* Bind collision, search negative portid values. */
 		portid = rover--;
 		if (rover > -4097)
-			rover = -4097;
+			portid = rover = -4097;
 		rcu_read_unlock();
 		goto retry;
 	}

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ