From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrei Vagin Subject: Re: net: do not allow changing SO_REUSEADDR/SO_REUSEPORT on bound sockets Date: Mon, 11 Jun 2018 11:57:49 -0700 Message-ID: <20180611185749.GB2729@outlook.office365.com> References: <20180603174705.51802-1-zenczykowski@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Maciej =?utf-8?Q?=C5=BBenczykowski?= , "David S . Miller" , Eric Dumazet , netdev@vger.kernel.org To: Maciej =?utf-8?Q?=C5=BBenczykowski?= Return-path: Received: from mail-eopbgr60130.outbound.protection.outlook.com ([40.107.6.130]:53343 "EHLO EUR04-DB3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S934485AbeFKS6H (ORCPT ); Mon, 11 Jun 2018 14:58:07 -0400 Content-Disposition: inline In-Reply-To: <20180603174705.51802-1-zenczykowski@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Jun 03, 2018 at 10:47:05AM -0700, Maciej Żenczykowski wrote: > From: Maciej Żenczykowski > > 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... > > Signed-off-by: Maciej Żenczykowski > Cc: Eric Dumazet > > Change-Id: I77c2b3429b2fdf42671eee0fa7a8ba721c94963b > Reviewed-by: Eric Dumazet > --- > net/core/sock.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 435a0ba85e52..feca4c98f8a0 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -728,9 +728,22 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > sock_valbool_flag(sk, SOCK_DBG, valbool); > break; > case SO_REUSEADDR: > - sk->sk_reuse = (valbool ? SK_CAN_REUSE : SK_NO_REUSE); > + val = (valbool ? SK_CAN_REUSE : SK_NO_REUSE); > + if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) && > + inet_sk(sk)->inet_num && > + (sk->sk_reuse != val)) { > + ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN; There are a few more states like TCP_LAST_ACK, TCP_CLOSE_WAIT which means that a socket is connected. Actually, I don't see any reasons to return two different values here. > + break; > + } > + sk->sk_reuse = val; > break; > case SO_REUSEPORT: > + if ((sk->sk_family == PF_INET || sk->sk_family == PF_INET6) && > + inet_sk(sk)->inet_num && > + (sk->sk_reuseport != valbool)) { > + ret = (sk->sk_state == TCP_ESTABLISHED) ? -EISCONN : -EUCLEAN; > + break; > + } > sk->sk_reuseport = valbool; > break; > case SO_TYPE: