Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: Remove the source address setting in connect() for UDP
@ 2019-09-06  2:54 Enke Chen
  2019-09-06  7:13 ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Enke Chen @ 2019-09-06  2:54 UTC (permalink / raw)
  To: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
  Cc: enkechen, linux-kernel, xe-linux-external

The connect() system call for a UDP socket is for setting the destination
address and port. But the current code mistakenly sets the source address
for the socket as well. Remove the source address setting in connect() for
UDP in this patch.

Implications of the bug:

  - Packet drop:

    On a multi-homed device, an address assigned to any interface may
    qualify as a source address when originating a packet. If needed, the
    IP_PKTINFO option can be used to explicitly specify the source address.
    But with the source address being mistakenly set for the socket in
    connect(), a return packet (for the socket) destined to an interface
    address different from that source address would be wrongly dropped
    due to address mismatch.

    This can be reproduced easily. The dropped packets are shown in the
    following output by "netstat -s" for UDP:

          xxx packets to unknown port received

  - Source address selection:

    The source address, if unspecified via "bind()" or IP_PKTINFO, should
    be determined by routing at the time of packet origination, and not at
    the time when the connect() call is made. The difference matters as
    routing can change, e.g., by interface down/up events, and using a
    source address of an "down" interface is known to be problematic.

There is no backward compatibility issue here as the source address setting
in connect() is not needed anyway.

  - No impact on the source address selection when the source address
    is explicitly specified by "bind()", or by the "IP_PKTINFO" option.

  - In the case that the source address is not explicitly specified,
    the selection of the source address would be more accurate and
    reliable based on the up-to-date routing table.

Signed-off-by: Enke Chen <enkechen@cisco.com>
---
 net/ipv4/datagram.c |  7 -------
 net/ipv6/datagram.c | 15 +--------------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
index f915abff1350..4065808ec6c1 100644
--- a/net/ipv4/datagram.c
+++ b/net/ipv4/datagram.c
@@ -64,13 +64,6 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
 		err = -EACCES;
 		goto out;
 	}
-	if (!inet->inet_saddr)
-		inet->inet_saddr = fl4->saddr;	/* Update source address */
-	if (!inet->inet_rcv_saddr) {
-		inet->inet_rcv_saddr = fl4->saddr;
-		if (sk->sk_prot->rehash)
-			sk->sk_prot->rehash(sk);
-	}
 	inet->inet_daddr = fl4->daddr;
 	inet->inet_dport = usin->sin_port;
 	sk->sk_state = TCP_ESTABLISHED;
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index ecf440a4f593..80388cd50dc3 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -197,19 +197,6 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 			goto out;
 
 		ipv6_addr_set_v4mapped(inet->inet_daddr, &sk->sk_v6_daddr);
-
-		if (ipv6_addr_any(&np->saddr) ||
-		    ipv6_mapped_addr_any(&np->saddr))
-			ipv6_addr_set_v4mapped(inet->inet_saddr, &np->saddr);
-
-		if (ipv6_addr_any(&sk->sk_v6_rcv_saddr) ||
-		    ipv6_mapped_addr_any(&sk->sk_v6_rcv_saddr)) {
-			ipv6_addr_set_v4mapped(inet->inet_rcv_saddr,
-					       &sk->sk_v6_rcv_saddr);
-			if (sk->sk_prot->rehash)
-				sk->sk_prot->rehash(sk);
-		}
-
 		goto out;
 	}
 
@@ -247,7 +234,7 @@ int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
 	 *	destination cache for it.
 	 */
 
-	err = ip6_datagram_dst_update(sk, true);
+	err = ip6_datagram_dst_update(sk, false);
 	if (err) {
 		/* Restore the socket peer info, to keep it consistent with
 		 * the old socket state
-- 
2.19.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: Remove the source address setting in connect() for UDP
  2019-09-06  2:54 [PATCH] net: Remove the source address setting in connect() for UDP Enke Chen
@ 2019-09-06  7:13 ` David Miller
  2019-09-06  7:23   ` Enke Chen (enkechen)
  0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2019-09-06  7:13 UTC (permalink / raw)
  To: enkechen; +Cc: kuznet, yoshfuji, netdev, linux-kernel, xe-linux-external

From: Enke Chen <enkechen@cisco.com>
Date: Thu,  5 Sep 2019 19:54:37 -0700

> The connect() system call for a UDP socket is for setting the destination
> address and port. But the current code mistakenly sets the source address
> for the socket as well. Remove the source address setting in connect() for
> UDP in this patch.

Do you have any idea how many decades of precedence this behavior has and
therefore how much you potentially will break userspace?

This boat has sailed a long time ago I'm afraid.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: Remove the source address setting in connect() for UDP
  2019-09-06  7:13 ` David Miller
@ 2019-09-06  7:23   ` Enke Chen (enkechen)
  2019-09-10 23:55     ` Enke Chen (enkechen)
  0 siblings, 1 reply; 5+ messages in thread
From: Enke Chen (enkechen) @ 2019-09-06  7:23 UTC (permalink / raw)
  To: David Miller
  Cc: kuznet, yoshfuji, netdev, linux-kernel,
	xe-linux-external(mailer list), Enke Chen (enkechen)

Hi, David:

Yes, I understand the code has been there for a long time.  But the issues are real, and it's really nasty when
You run into them.  As I described in the patch log, there is no backward compatibility Issue for fixing it.

---
There is no backward compatibility issue here as the source address setting
in connect() is not needed anyway.

  - No impact on the source address selection when the source address
    is explicitly specified by "bind()", or by the "IP_PKTINFO" option.

  - In the case that the source address is not explicitly specified,
    the selection of the source address would be more accurate and
    reliable based on the up-to-date routing table.
---

Thanks.  -- Enke

-----Original Message-----
From: <linux-kernel-owner@vger.kernel.org> on behalf of David Miller <davem@davemloft.net>
Date: Friday, September 6, 2019 at 12:14 AM
To: "Enke Chen (enkechen)" <enkechen@cisco.com>
Cc: "kuznet@ms2.inr.ac.ru" <kuznet@ms2.inr.ac.ru>, "yoshfuji@linux-ipv6.org" <yoshfuji@linux-ipv6.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "xe-linux-external(mailer list)" <xe-linux-external@cisco.com>
Subject: Re: [PATCH] net: Remove the source address setting in connect() for UDP

From: Enke Chen <enkechen@cisco.com>
Date: Thu,  5 Sep 2019 19:54:37 -0700

> The connect() system call for a UDP socket is for setting the destination
> address and port. But the current code mistakenly sets the source address
> for the socket as well. Remove the source address setting in connect() for
> UDP in this patch.

Do you have any idea how many decades of precedence this behavior has and
therefore how much you potentially will break userspace?

This boat has sailed a long time ago I'm afraid.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: Remove the source address setting in connect() for UDP
  2019-09-06  7:23   ` Enke Chen (enkechen)
@ 2019-09-10 23:55     ` Enke Chen (enkechen)
  2019-09-11  7:57       ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Enke Chen (enkechen) @ 2019-09-10 23:55 UTC (permalink / raw)
  To: David Miller
  Cc: kuznet, yoshfuji, netdev, linux-kernel,
	xe-linux-external(mailer list), Enke Chen (enkechen)

Hi, David:

Do you still have concerns about backward compatibility of the fix?

I really do not see how existing, working applications would be negatively impacted
by the fix.

Thanks.   -- Enke

-----Original Message-----
From: "Enke Chen (enkechen)" <enkechen@cisco.com>
Date: Friday, September 6, 2019 at 12:23 AM
To: David Miller <davem@davemloft.net>
Cc: "kuznet@ms2.inr.ac.ru" <kuznet@ms2.inr.ac.ru>, "yoshfuji@linux-ipv6.org" <yoshfuji@linux-ipv6.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "xe-linux-external(mailer list)" <xe-linux-external@cisco.com>, "Enke Chen (enkechen)" <enkechen@cisco.com>
Subject: Re: [PATCH] net: Remove the source address setting in connect() for UDP

Hi, David:

Yes, I understand the code has been there for a long time.  But the issues are real, and it's really nasty when
You run into them.  As I described in the patch log, there is no backward compatibility Issue for fixing it.

---
There is no backward compatibility issue here as the source address setting
in connect() is not needed anyway.

  - No impact on the source address selection when the source address
    is explicitly specified by "bind()", or by the "IP_PKTINFO" option.

  - In the case that the source address is not explicitly specified,
    the selection of the source address would be more accurate and
    reliable based on the up-to-date routing table.
---

Thanks.  -- Enke

-----Original Message-----
From: <linux-kernel-owner@vger.kernel.org> on behalf of David Miller <davem@davemloft.net>
Date: Friday, September 6, 2019 at 12:14 AM
To: "Enke Chen (enkechen)" <enkechen@cisco.com>
Cc: "kuznet@ms2.inr.ac.ru" <kuznet@ms2.inr.ac.ru>, "yoshfuji@linux-ipv6.org" <yoshfuji@linux-ipv6.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "xe-linux-external(mailer list)" <xe-linux-external@cisco.com>
Subject: Re: [PATCH] net: Remove the source address setting in connect() for UDP

From: Enke Chen <enkechen@cisco.com>
Date: Thu,  5 Sep 2019 19:54:37 -0700

> The connect() system call for a UDP socket is for setting the destination
> address and port. But the current code mistakenly sets the source address
> for the socket as well. Remove the source address setting in connect() for
> UDP in this patch.

Do you have any idea how many decades of precedence this behavior has and
therefore how much you potentially will break userspace?

This boat has sailed a long time ago I'm afraid.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] net: Remove the source address setting in connect() for UDP
  2019-09-10 23:55     ` Enke Chen (enkechen)
@ 2019-09-11  7:57       ` David Miller
  0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2019-09-11  7:57 UTC (permalink / raw)
  To: enkechen; +Cc: kuznet, yoshfuji, netdev, linux-kernel, xe-linux-external

From: "Enke Chen (enkechen)" <enkechen@cisco.com>
Date: Tue, 10 Sep 2019 23:55:59 +0000

> Do you still have concerns about backward compatibility of the fix?

I'm not convinced by your arguments and I am also completely swamped at
LPC2019 running the networking track this week.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06  2:54 [PATCH] net: Remove the source address setting in connect() for UDP Enke Chen
2019-09-06  7:13 ` David Miller
2019-09-06  7:23   ` Enke Chen (enkechen)
2019-09-10 23:55     ` Enke Chen (enkechen)
2019-09-11  7:57       ` David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox