From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuniyuki Iwashima Date: Thu, 17 Nov 2022 00:20:10 +0000 Subject: Re: [PATCH v2 net 1/4] dccp/tcp: Reset saddr on failure after inet6?_hash_connect(). Message-Id: <20221117002010.72675-1-kuniyu@amazon.com> List-Id: References: <20221116222805.64734-2-kuniyu@amazon.com> In-Reply-To: <20221116222805.64734-2-kuniyu@amazon.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: dccp@vger.kernel.org From: Joanne Koong Date: Wed, 16 Nov 2022 16:11:21 -0800 > On Wed, Nov 16, 2022 at 2:28 PM Kuniyuki Iwashima wrote: > > > > When connect() is called on a socket bound to the wildcard address, > > we change the socket's saddr to a local address. If the socket > > fails to connect() to the destination, we have to reset the saddr. > > > > However, when an error occurs after inet_hash6?_connect() in > > (dccp|tcp)_v[46]_conect(), we forget to reset saddr and leave > > the socket bound to the address. > > > > From the user's point of view, whether saddr is reset or not varies > > with errno. Let's fix this inconsistent behaviour. > > > > Note that after this patch, the repro [0] will trigger the WARN_ON() > > in inet_csk_get_port() again, but this patch is not buggy and rather > > fixes a bug papering over the bhash2's bug for which we need another > > fix. > > > > For the record, the repro causes -EADDRNOTAVAIL in inet_hash6_connect() > > by this sequence: > > > > s1 = socket() > > s1.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1) > > s1.bind(('127.0.0.1', 10000)) > > s1.sendto(b'hello', MSG_FASTOPEN, (('127.0.0.1', 10000))) > > # or s1.connect(('127.0.0.1', 10000)) > > > > s2 = socket() > > s2.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1) > > s2.bind(('0.0.0.0', 10000)) > > s2.connect(('127.0.0.1', 10000)) # -EADDRNOTAVAIL > > > > s2.listen(32) # WARN_ON(inet_csk(sk)->icsk_bind2_hash != tb2); > > > > [0]: https://syzkaller.appspot.com/bug?extid5d756bbd1f8b5c8f09 > > > > Fixes: 3df80d9320bc ("[DCCP]: Introduce DCCPv6") > > Fixes: 7c657876b63c ("[DCCP]: Initial implementation") > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Kuniyuki Iwashima > > LGTM. Btw, the 4th patch in this series overwrites these changes by > moving this logic into the new inet_bhash2_reset_saddr() function you > added, so we could also drop this patch from the series. OTOH, this > commit message in this patch has some good background context. So I > don't have a preference either way :) > > Acked-by: Joanne Koong Thanks for reviewing! Yes, these changes are overwritten later, but only this patch can be backported to other stable versions, so I kept this separated. > > --- > > net/dccp/ipv4.c | 2 ++ > > net/dccp/ipv6.c | 2 ++ > > net/ipv4/tcp_ipv4.c | 2 ++ > > net/ipv6/tcp_ipv6.c | 2 ++ > > 4 files changed, 8 insertions(+) > > > > diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c > > index 713b7b8dad7e..40640c26680e 100644 > > --- a/net/dccp/ipv4.c > > +++ b/net/dccp/ipv4.c > > @@ -157,6 +157,8 @@ int dccp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > * This unhashes the socket and releases the local port, if necessary. > > */ > > dccp_set_state(sk, DCCP_CLOSED); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > ip_rt_put(rt); > > sk->sk_route_caps = 0; > > inet->inet_dport = 0; > > diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c > > index e57b43006074..626166cb6d7e 100644 > > --- a/net/dccp/ipv6.c > > +++ b/net/dccp/ipv6.c > > @@ -985,6 +985,8 @@ static int dccp_v6_connect(struct sock *sk, struct sockaddr *uaddr, > > > > late_failure: > > dccp_set_state(sk, DCCP_CLOSED); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > __sk_dst_reset(sk); > > failure: > > inet->inet_dport = 0; > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > index 87d440f47a70..6a3a732b584d 100644 > > --- a/net/ipv4/tcp_ipv4.c > > +++ b/net/ipv4/tcp_ipv4.c > > @@ -343,6 +343,8 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > * if necessary. > > */ > > tcp_set_state(sk, TCP_CLOSE); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > ip_rt_put(rt); > > sk->sk_route_caps = 0; > > inet->inet_dport = 0; > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c > > index 2a3f9296df1e..81b396e5cf79 100644 > > --- a/net/ipv6/tcp_ipv6.c > > +++ b/net/ipv6/tcp_ipv6.c > > @@ -359,6 +359,8 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr, > > > > late_failure: > > tcp_set_state(sk, TCP_CLOSE); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > failure: > > inet->inet_dport = 0; > > sk->sk_route_caps = 0; > > -- > > 2.30.2 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 66A9EC433FE for ; Thu, 17 Nov 2022 00:20:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233325AbiKQAUa (ORCPT ); Wed, 16 Nov 2022 19:20:30 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54114 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234039AbiKQAU3 (ORCPT ); Wed, 16 Nov 2022 19:20:29 -0500 Received: from smtp-fw-9103.amazon.com (smtp-fw-9103.amazon.com [207.171.188.200]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C69A645EF1; Wed, 16 Nov 2022 16:20:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1668644427; x=1700180427; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=OwavocW5NJuPb92cb+UmVHArM9k2XXgrJ2q8fSHGR3M=; b=MraeKwuqFN7oJpWW9NgUruy+yYO6URBSBo21gMH5NCkPXZB/XU22DLiB shAC5tBeutarJGHyumRF8nouQfyBuA7MgMSV/WT9bWtBIVSxDxW0+WFpT EoA9J3THRm8bASSydkXuKV28GV0s3UM687sGJM0KiVXY6in5IS9LQFWWv Q=; X-IronPort-AV: E=Sophos;i="5.96,169,1665446400"; d="scan'208";a="1074475348" Received: from pdx4-co-svc-p1-lb2-vlan3.amazon.com (HELO email-inbound-relay-pdx-2b-m6i4x-32fb4f1a.us-west-2.amazon.com) ([10.25.36.214]) by smtp-border-fw-9103.sea19.amazon.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Nov 2022 00:20:22 +0000 Received: from EX13MTAUWB002.ant.amazon.com (pdx1-ws-svc-p6-lb9-vlan2.pdx.amazon.com [10.236.137.194]) by email-inbound-relay-pdx-2b-m6i4x-32fb4f1a.us-west-2.amazon.com (Postfix) with ESMTPS id A87AFC333F; Thu, 17 Nov 2022 00:20:21 +0000 (UTC) Received: from EX19D004ANA001.ant.amazon.com (10.37.240.138) by EX13MTAUWB002.ant.amazon.com (10.43.161.202) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Thu, 17 Nov 2022 00:20:21 +0000 Received: from 88665a182662.ant.amazon.com (10.43.162.178) by EX19D004ANA001.ant.amazon.com (10.37.240.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA) id 15.2.1118.20; Thu, 17 Nov 2022 00:20:18 +0000 From: Kuniyuki Iwashima To: CC: , , , , , , , , , , , , , , , Subject: Re: [PATCH v2 net 1/4] dccp/tcp: Reset saddr on failure after inet6?_hash_connect(). Date: Wed, 16 Nov 2022 16:20:10 -0800 Message-ID: <20221117002010.72675-1-kuniyu@amazon.com> 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.162.178] X-ClientProxiedBy: EX13D34UWA001.ant.amazon.com (10.43.160.173) To EX19D004ANA001.ant.amazon.com (10.37.240.138) Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Joanne Koong Date: Wed, 16 Nov 2022 16:11:21 -0800 > On Wed, Nov 16, 2022 at 2:28 PM Kuniyuki Iwashima wrote: > > > > When connect() is called on a socket bound to the wildcard address, > > we change the socket's saddr to a local address. If the socket > > fails to connect() to the destination, we have to reset the saddr. > > > > However, when an error occurs after inet_hash6?_connect() in > > (dccp|tcp)_v[46]_conect(), we forget to reset saddr and leave > > the socket bound to the address. > > > > From the user's point of view, whether saddr is reset or not varies > > with errno. Let's fix this inconsistent behaviour. > > > > Note that after this patch, the repro [0] will trigger the WARN_ON() > > in inet_csk_get_port() again, but this patch is not buggy and rather > > fixes a bug papering over the bhash2's bug for which we need another > > fix. > > > > For the record, the repro causes -EADDRNOTAVAIL in inet_hash6_connect() > > by this sequence: > > > > s1 = socket() > > s1.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1) > > s1.bind(('127.0.0.1', 10000)) > > s1.sendto(b'hello', MSG_FASTOPEN, (('127.0.0.1', 10000))) > > # or s1.connect(('127.0.0.1', 10000)) > > > > s2 = socket() > > s2.setsockopt(SOL_SOCKET, SO_REUSEADDR, 1) > > s2.bind(('0.0.0.0', 10000)) > > s2.connect(('127.0.0.1', 10000)) # -EADDRNOTAVAIL > > > > s2.listen(32) # WARN_ON(inet_csk(sk)->icsk_bind2_hash != tb2); > > > > [0]: https://syzkaller.appspot.com/bug?extid=015d756bbd1f8b5c8f09 > > > > Fixes: 3df80d9320bc ("[DCCP]: Introduce DCCPv6") > > Fixes: 7c657876b63c ("[DCCP]: Initial implementation") > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Kuniyuki Iwashima > > LGTM. Btw, the 4th patch in this series overwrites these changes by > moving this logic into the new inet_bhash2_reset_saddr() function you > added, so we could also drop this patch from the series. OTOH, this > commit message in this patch has some good background context. So I > don't have a preference either way :) > > Acked-by: Joanne Koong Thanks for reviewing! Yes, these changes are overwritten later, but only this patch can be backported to other stable versions, so I kept this separated. > > --- > > net/dccp/ipv4.c | 2 ++ > > net/dccp/ipv6.c | 2 ++ > > net/ipv4/tcp_ipv4.c | 2 ++ > > net/ipv6/tcp_ipv6.c | 2 ++ > > 4 files changed, 8 insertions(+) > > > > diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c > > index 713b7b8dad7e..40640c26680e 100644 > > --- a/net/dccp/ipv4.c > > +++ b/net/dccp/ipv4.c > > @@ -157,6 +157,8 @@ int dccp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > * This unhashes the socket and releases the local port, if necessary. > > */ > > dccp_set_state(sk, DCCP_CLOSED); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > ip_rt_put(rt); > > sk->sk_route_caps = 0; > > inet->inet_dport = 0; > > diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c > > index e57b43006074..626166cb6d7e 100644 > > --- a/net/dccp/ipv6.c > > +++ b/net/dccp/ipv6.c > > @@ -985,6 +985,8 @@ static int dccp_v6_connect(struct sock *sk, struct sockaddr *uaddr, > > > > late_failure: > > dccp_set_state(sk, DCCP_CLOSED); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > __sk_dst_reset(sk); > > failure: > > inet->inet_dport = 0; > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > index 87d440f47a70..6a3a732b584d 100644 > > --- a/net/ipv4/tcp_ipv4.c > > +++ b/net/ipv4/tcp_ipv4.c > > @@ -343,6 +343,8 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len) > > * if necessary. > > */ > > tcp_set_state(sk, TCP_CLOSE); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > ip_rt_put(rt); > > sk->sk_route_caps = 0; > > inet->inet_dport = 0; > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c > > index 2a3f9296df1e..81b396e5cf79 100644 > > --- a/net/ipv6/tcp_ipv6.c > > +++ b/net/ipv6/tcp_ipv6.c > > @@ -359,6 +359,8 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr, > > > > late_failure: > > tcp_set_state(sk, TCP_CLOSE); > > + if (!(sk->sk_userlocks & SOCK_BINDADDR_LOCK)) > > + inet_reset_saddr(sk); > > failure: > > inet->inet_dport = 0; > > sk->sk_route_caps = 0; > > -- > > 2.30.2