Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp87347pxj; Thu, 10 Jun 2021 15:39:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxLGMEu6dPJl232rKkcRyr55gVjBIGArK2RsaIof31jnsHjG3w3B+LUcs4flyWRxa7hQtGZ X-Received: by 2002:aa7:db90:: with SMTP id u16mr720653edt.106.1623364796380; Thu, 10 Jun 2021 15:39:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1623364796; cv=none; d=google.com; s=arc-20160816; b=Rc2zAuAadafW0uhB/UF1LaPLntWRU8NhqLKHtsjcixgZTqE4GhxUG6CEImlq0aIHpr iQ2GpGCNK9I9ri/FsOIqWuQE04jQUVdNjU2NoL/IUdshfGYtw/zAzBE0rZ+3DQ1Frd/v jh6Kke4PV6hgVh9YqLZTS+cPcbWm/L+CVTG8MenflykXHjyyIH+fc2vnvpYU/3S++k2e PjDWS1DbdWvq5dhtSK/tZMD9wu0EHtsdOSppCo4CHvtxwKvb4mkNHMdl2/IlxjtA4jYJ xB9EDeO/kUtqiA1tEH0A+eKHi5JYpgOFQCPyPtXiPyFYjRerTFybdGx14Bxyo0XBo9x4 Gb4A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=pwa9iNCMYzaOFfi5zFCAhrxikXICuCtA5BrImYbauiw=; b=Zu34o40G4C9UCd6XcPth3YXX66RaT6DFCi6sGa6w59bywyZ6POmfFoPXZIweJoClWX R9viixAKv19eAr3uwQK3deaLCauUgl30ZGpJMv6OpIixqW5oYUYevkeRuYJkObfRB/br bIdxdqAJRW4W/F8gmGCzNvWhV/MzrmvuxlwTSTwk9bmY3vuZtGP0W7P3hEChlEy/h3E4 vBZ5YBBHEGOjM6PB3jFQpv8UDYQ3Ur9zG3G8CCXyGRdGmq1WieVh2z0c26WQmw/Csvat 0lSdyWTRw0A6l2l8lwLwPUtb3i7cmFdWA4hC4SLtCTs83+WkEsy2j5xU6USldDBLkFXM hKKw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@amazon.co.jp header.s=amazon201209 header.b=mQJ5+nEY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amazon.co.jp Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id jz1si3035175ejb.461.2021.06.10.15.39.30; Thu, 10 Jun 2021 15:39:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@amazon.co.jp header.s=amazon201209 header.b=mQJ5+nEY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=amazon.co.jp Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231171AbhFJWjh (ORCPT + 99 others); Thu, 10 Jun 2021 18:39:37 -0400 Received: from smtp-fw-80006.amazon.com ([99.78.197.217]:58478 "EHLO smtp-fw-80006.amazon.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230001AbhFJWjg (ORCPT ); Thu, 10 Jun 2021 18:39:36 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.co.jp; i=@amazon.co.jp; q=dns/txt; s=amazon201209; t=1623364660; x=1654900660; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=pwa9iNCMYzaOFfi5zFCAhrxikXICuCtA5BrImYbauiw=; b=mQJ5+nEYuwgF2tKQrqkQLLLT+31SZWd3ljtJt4kuZKbJGHbOcSBqqmF0 BkmOu1oObgiRVSHcR5Z7kpeIkEhoD9xunvvujUkPwmc8c53Vt7Nmm1X0r fqZqv/8VrnMTSdRRFShN5ppZEg4eS6PQqUfNyaoEVlJ6Wf4U4hfZz0c01 k=; X-IronPort-AV: E=Sophos;i="5.83,264,1616457600"; d="scan'208";a="6075798" Received: from pdx4-co-svc-p1-lb2-vlan3.amazon.com (HELO email-inbound-relay-2b-baacba05.us-west-2.amazon.com) ([10.25.36.214]) by smtp-border-fw-80006.pdx80.corp.amazon.com with ESMTP; 10 Jun 2021 22:37:40 +0000 Received: from EX13MTAUWB001.ant.amazon.com (pdx1-ws-svc-p6-lb9-vlan2.pdx.amazon.com [10.236.137.194]) by email-inbound-relay-2b-baacba05.us-west-2.amazon.com (Postfix) with ESMTPS id 2F913A05BF; Thu, 10 Jun 2021 22:37:39 +0000 (UTC) Received: from EX13D04ANC001.ant.amazon.com (10.43.157.89) by EX13MTAUWB001.ant.amazon.com (10.43.161.249) with Microsoft SMTP Server (TLS) id 15.0.1497.18; Thu, 10 Jun 2021 22:37:38 +0000 Received: from 88665a182662.ant.amazon.com (10.43.160.17) by EX13D04ANC001.ant.amazon.com (10.43.157.89) with Microsoft SMTP Server (TLS) id 15.0.1497.18; Thu, 10 Jun 2021 22:37:33 +0000 From: Kuniyuki Iwashima To: CC: , , , , , , , , , , , , Subject: Re: [PATCH v7 bpf-next 03/11] tcp: Keep TCP_CLOSE sockets in the reuseport group. Date: Fri, 11 Jun 2021 07:37:30 +0900 Message-ID: <20210610223730.97716-1-kuniyu@amazon.co.jp> X-Mailer: git-send-email 2.30.2 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.43.160.17] X-ClientProxiedBy: EX13D46UWC004.ant.amazon.com (10.43.162.173) To EX13D04ANC001.ant.amazon.com (10.43.157.89) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Eric Dumazet Date: Thu, 10 Jun 2021 19:59:15 +0200 > On 5/21/21 8:20 PM, Kuniyuki Iwashima wrote: > > When we close a listening socket, to migrate its connections to another > > listener in the same reuseport group, we have to handle two kinds of child > > sockets. One is that a listening socket has a reference to, and the other > > is not. > > > > The former is the TCP_ESTABLISHED/TCP_SYN_RECV sockets, and they are in the > > accept queue of their listening socket. So we can pop them out and push > > them into another listener's queue at close() or shutdown() syscalls. On > > the other hand, the latter, the TCP_NEW_SYN_RECV socket is during the > > three-way handshake and not in the accept queue. Thus, we cannot access > > such sockets at close() or shutdown() syscalls. Accordingly, we have to > > migrate immature sockets after their listening socket has been closed. > > > > Currently, if their listening socket has been closed, TCP_NEW_SYN_RECV > > sockets are freed at receiving the final ACK or retransmitting SYN+ACKs. At > > that time, if we could select a new listener from the same reuseport group, > > no connection would be aborted. However, we cannot do that because > > reuseport_detach_sock() sets NULL to sk_reuseport_cb and forbids access to > > the reuseport group from closed sockets. > > > > This patch allows TCP_CLOSE sockets to remain in the reuseport group and > > access it while any child socket references them. The point is that > > reuseport_detach_sock() was called twice from inet_unhash() and > > sk_destruct(). This patch replaces the first reuseport_detach_sock() with > > reuseport_stop_listen_sock(), which checks if the reuseport group is > > capable of migration. If capable, it decrements num_socks, moves the socket > > backwards in socks[] and increments num_closed_socks. When all connections > > are migrated, sk_destruct() calls reuseport_detach_sock() to remove the > > socket from socks[], decrement num_closed_socks, and set NULL to > > sk_reuseport_cb. > > > > By this change, closed or shutdowned sockets can keep sk_reuseport_cb. > > Consequently, calling listen() after shutdown() can cause EADDRINUSE or > > EBUSY in inet_csk_bind_conflict() or reuseport_add_sock() which expects > > such sockets not to have the reuseport group. Therefore, this patch also > > loosens such validation rules so that a socket can listen again if it has a > > reuseport group with num_closed_socks more than 0. > > > > When such sockets listen again, we handle them in reuseport_resurrect(). If > > there is an existing reuseport group (reuseport_add_sock() path), we move > > the socket from the old group to the new one and free the old one if > > necessary. If there is no existing group (reuseport_alloc() path), we > > allocate a new reuseport group, detach sk from the old one, and free it if > > necessary, not to break the current shutdown behaviour: > > > > - we cannot carry over the eBPF prog of shutdowned sockets > > - we cannot attach/detach an eBPF prog to/from listening sockets via > > shutdowned sockets > > > > Note that when the number of sockets gets over U16_MAX, we try to detach a > > closed socket randomly to make room for the new listening socket in > > reuseport_grow(). > > > > Signed-off-by: Kuniyuki Iwashima > > Signed-off-by: Martin KaFai Lau > > --- > > include/net/sock_reuseport.h | 1 + > > net/core/sock_reuseport.c | 184 ++++++++++++++++++++++++++++++-- > > net/ipv4/inet_connection_sock.c | 12 ++- > > net/ipv4/inet_hashtables.c | 2 +- > > 4 files changed, 188 insertions(+), 11 deletions(-) > > > > diff --git a/include/net/sock_reuseport.h b/include/net/sock_reuseport.h > > index 0e558ca7afbf..1333d0cddfbc 100644 > > --- a/include/net/sock_reuseport.h > > +++ b/include/net/sock_reuseport.h > > @@ -32,6 +32,7 @@ extern int reuseport_alloc(struct sock *sk, bool bind_inany); > > extern int reuseport_add_sock(struct sock *sk, struct sock *sk2, > > bool bind_inany); > > extern void reuseport_detach_sock(struct sock *sk); > > +void reuseport_stop_listen_sock(struct sock *sk); > > extern struct sock *reuseport_select_sock(struct sock *sk, > > u32 hash, > > struct sk_buff *skb, > > diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c > > index 079bd1aca0e7..ea0e900d3e97 100644 > > --- a/net/core/sock_reuseport.c > > +++ b/net/core/sock_reuseport.c > > @@ -17,6 +17,8 @@ > > DEFINE_SPINLOCK(reuseport_lock); > > > > static DEFINE_IDA(reuseport_ida); > > +static int reuseport_resurrect(struct sock *sk, struct sock_reuseport *old_reuse, > > + struct sock_reuseport *reuse, bool bind_inany); > > > > static int reuseport_sock_index(struct sock *sk, > > struct sock_reuseport *reuse, > > @@ -61,6 +63,29 @@ static bool __reuseport_detach_sock(struct sock *sk, > > return true; > > } > > > > +static void __reuseport_add_closed_sock(struct sock *sk, > > + struct sock_reuseport *reuse) > > +{ > > + reuse->socks[reuse->max_socks - reuse->num_closed_socks - 1] = sk; > > + /* paired with READ_ONCE() in inet_csk_bind_conflict() */ > > + WRITE_ONCE(reuse->num_closed_socks, reuse->num_closed_socks + 1); > > +} > > + > > +static bool __reuseport_detach_closed_sock(struct sock *sk, > > + struct sock_reuseport *reuse) > > +{ > > + int i = reuseport_sock_index(sk, reuse, true); > > + > > + if (i == -1) > > + return false; > > + > > + reuse->socks[i] = reuse->socks[reuse->max_socks - reuse->num_closed_socks]; > > + /* paired with READ_ONCE() in inet_csk_bind_conflict() */ > > + WRITE_ONCE(reuse->num_closed_socks, reuse->num_closed_socks - 1); > > + > > + return true; > > +} > > + > > static struct sock_reuseport *__reuseport_alloc(unsigned int max_socks) > > { > > unsigned int size = sizeof(struct sock_reuseport) + > > @@ -92,6 +117,14 @@ int reuseport_alloc(struct sock *sk, bool bind_inany) > > reuse = rcu_dereference_protected(sk->sk_reuseport_cb, > > lockdep_is_held(&reuseport_lock)); > > if (reuse) { > > + if (reuse->num_closed_socks) { > > + /* sk was shutdown()ed before */ > > + int err = reuseport_resurrect(sk, reuse, NULL, bind_inany); > > + > > + spin_unlock_bh(&reuseport_lock); > > + return err; > > It seems coding style in this function would rather do > ret = reuseport_resurrect(sk, reuse, NULL, bind_inany); > goto out; > > Overall, changes in this commit are a bit scarry. I will change the style with ret and goto. Thank you.