Re: [PATCH] net: do not allow changing SO_REUSEADDR/SO_REUSEPORT on bound sockets

From: Marc Dionne <marc.c.dionne@gmail.com>
To: "Maciej Żenczykowski" <zenczykowski@gmail.com>
Cc: "Maciej Żenczykowski" <maze@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	netdev <netdev@vger.kernel.org>,
	"Jeffrey Altman" <jaltman@auristor.com>,
	"Marc Dionne" <marc@auristor.com>
Subject: Re: [PATCH] net: do not allow changing SO_REUSEADDR/SO_REUSEPORT on bound sockets
Date: Mon, 11 Jun 2018 18:25:02 -0300	[thread overview]
Message-ID: <CAB9dFdueWF6iXdqC+uN5pmfs5Z6hdb9b-Zq0g+dUtX20cMuWSg@mail.gmail.com> (raw)
In-Reply-To: <20180603174705.51802-1-zenczykowski@gmail.com>

On Sun, Jun 3, 2018 at 2:47 PM, Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> It is not safe to do so because such sockets are already in the
> hash tables and changing these options can result in invalidating
> the tb->fastreuse(port) caching.
>
> This can have later far reaching consequences wrt. bind conflict checks
> which rely on these caches (for optimization purposes).
>
> Not to mention that you can currently end up with two identical
> non-reuseport listening sockets bound to the same local ip:port
> by clearing reuseport on them after they've already both been bound.
>
> There is unfortunately no EISBOUND error or anything similar,
> and EISCONN seems to be misleading for a bound-but-not-connected
> socket, so use EUCLEAN 'Structure needs cleaning' which AFAICT
> is the closest you can get to meaning 'socket in bad state'.
> (although perhaps EINVAL wouldn't be a bad choice either?)
>
> This does unfortunately run the risk of breaking buggy
> userspace programs...

This change is a potential performance regression for us.  Our
networking code sets up multiple sockets used by multiple threads to
listen on the same udp port.  For the first socket, the bind is done
before setting SO_REUSEPORT so that we can get an error and detect
that the port is currently in use by a different process, possibly a
previous incarnation of the same application.

With this change, the servers end up with a single listener socket and
thread, which can have a large impact on performance for a busy
server.

While we can adjust for the new behaviour going forward, this could be
an unpleasant surprise for our customers who get this update when
moving to a new kernel version or through a backport to stable
kernels, if this is targeted for stable.

Marc