[RFC PATCH net 0/4] ip6: datagram: Update dst cache of a connected datagram sk during pmtu update

[RFC PATCH net 0/4] ip6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

There is a case in connected UDP socket such that
getsockopt(IPV6_MTU) will return a stale MTU value. The reproducible
sequence could be the following:
1. Create a connected UDP socket
2. Send some datagrams out
3. Receive a ICMPV6_PKT_TOOBIG
4. No new outgoing datagrams to trigger the sk_dst_check()
   logic to update the sk->sk_dst_cache.
5. getsockopt(IPV6_MTU) returns the mtu from the invalid
   sk->sk_dst_cache instead of the newly created RTF_CACHE clone.

Patch 1 and 2 are the prep work.
Patch 3 and 4 are the fixes.

[RFC PATCH net 1/4] ipv6: datagram: Refactor flowi6 init codes to a new function

[Martin KaFai Lau]

Move flowi6 init codes for connected datagram sk to a newly created
function ip6_datagram_flow_key_init().

It will be reused during pmtu update in the later patch.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 net/ipv6/datagram.c | 50 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 4281621..f07c1dd 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -40,6 +40,30 @@ static bool ipv6_mapped_addr_any(const struct in6_addr *a)
 	return ipv6_addr_v4mapped(a) && (a->s6_addr32[3] == 0);
 }
 
+static void ip6_datagram_flow_key_init(struct flowi6 *fl6, struct sock *sk)
+{
+	struct inet_sock *inet = inet_sk(sk);
+	struct ipv6_pinfo *np = inet6_sk(sk);
+
+	memset(fl6, 0, sizeof(*fl6));
+	fl6->flowi6_proto = sk->sk_protocol;
+	fl6->daddr = sk->sk_v6_daddr;
+	fl6->saddr = np->saddr;
+	fl6->flowi6_oif = sk->sk_bound_dev_if;
+	fl6->flowi6_mark = sk->sk_mark;
+	fl6->fl6_dport = inet->inet_dport;
+	fl6->fl6_sport = inet->inet_sport;
+	fl6->flowlabel = np->flow_label;
+
+	if (!fl6->flowi6_oif)
+		fl6->flowi6_oif = np->sticky_pktinfo.ipi6_ifindex;
+
+	if (!fl6->flowi6_oif && ipv6_addr_is_multicast(&fl6->daddr))
+		fl6->flowi6_oif = np->mcast_oif;
+
+	security_sk_classify_flow(sk, flowi6_to_flowi(fl6));
+}
+
 static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
 	struct sockaddr_in6	*usin = (struct sockaddr_in6 *) uaddr;
@@ -52,6 +76,7 @@ static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int a
 	struct ipv6_txoptions	*opt;
 	int			addr_type;
 	int			err;
+	__be32			fl6_flowlabel = 0;
 
 	if (usin->sin6_family == AF_INET) {
 		if (__ipv6_only_sock(sk))
@@ -66,11 +91,10 @@ static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int a
 	if (usin->sin6_family != AF_INET6)
 		return -EAFNOSUPPORT;
 
-	memset(&fl6, 0, sizeof(fl6));
 	if (np->sndflow) {
-		fl6.flowlabel = usin->sin6_flowinfo&IPV6_FLOWINFO_MASK;
-		if (fl6.flowlabel&IPV6_FLOWLABEL_MASK) {
-			flowlabel = fl6_sock_lookup(sk, fl6.flowlabel);
+		fl6_flowlabel = usin->sin6_flowinfo & IPV6_FLOWINFO_MASK;
+		if (fl6_flowlabel & IPV6_FLOWLABEL_MASK) {
+			flowlabel = fl6_sock_lookup(sk, fl6_flowlabel);
 			if (!flowlabel)
 				return -EINVAL;
 		}
@@ -145,7 +169,7 @@ ipv4_connected:
 	}
 
 	sk->sk_v6_daddr = *daddr;
-	np->flow_label = fl6.flowlabel;
+	np->flow_label = fl6_flowlabel;
 
 	inet->inet_dport = usin->sin6_port;
 
@@ -154,21 +178,7 @@ ipv4_connected:
 	 *	destination cache for it.
 	 */
 
-	fl6.flowi6_proto = sk->sk_protocol;
-	fl6.daddr = sk->sk_v6_daddr;
-	fl6.saddr = np->saddr;
-	fl6.flowi6_oif = sk->sk_bound_dev_if;
-	fl6.flowi6_mark = sk->sk_mark;
-	fl6.fl6_dport = inet->inet_dport;
-	fl6.fl6_sport = inet->inet_sport;
-
-	if (!fl6.flowi6_oif)
-		fl6.flowi6_oif = np->sticky_pktinfo.ipi6_ifindex;
-
-	if (!fl6.flowi6_oif && (addr_type&IPV6_ADDR_MULTICAST))
-		fl6.flowi6_oif = np->mcast_oif;
-
-	security_sk_classify_flow(sk, flowi6_to_flowi(&fl6));
+	ip6_datagram_flow_key_init(&fl6, sk);
 
 	rcu_read_lock();
 	opt = flowlabel ? flowlabel->opt : rcu_dereference(np->opt);
-- 
2.5.1

[RFC PATCH net 2/4] ipv6: datagram: Refactor dst lookup and update codes to a new function

[Martin KaFai Lau]

This patch moves the route lookup and update codes for connected
datagram sk to a newly created function ip6_datagram_dst_update()

It will be reused during the pmtu update in the later patch.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 net/ipv6/datagram.c | 103 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 46 deletions(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index f07c1dd..140665b 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -64,16 +64,65 @@ static void ip6_datagram_flow_key_init(struct flowi6 *fl6, struct sock *sk)
 	security_sk_classify_flow(sk, flowi6_to_flowi(fl6));
 }
 
+static int ip6_datagram_dst_update(struct sock *sk)
+{
+	struct ip6_flowlabel *flowlabel = NULL;
+	struct in6_addr *final_p, final;
+	struct ipv6_txoptions *opt;
+	struct dst_entry *dst;
+	struct inet_sock *inet = inet_sk(sk);
+	struct ipv6_pinfo *np = inet6_sk(sk);
+	struct flowi6 fl6;
+	int err = 0;
+
+	if (np->sndflow && np->flow_label) {
+		flowlabel = fl6_sock_lookup(sk, np->flow_label);
+		if (!flowlabel)
+			return -EINVAL;
+	}
+	ip6_datagram_flow_key_init(&fl6, sk);
+
+	rcu_read_lock();
+	opt = flowlabel ? flowlabel->opt : rcu_dereference(np->opt);
+	final_p = fl6_update_dst(&fl6, opt, &final);
+	rcu_read_unlock();
+
+	dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
+	if (IS_ERR(dst)) {
+		err = PTR_ERR(dst);
+		goto out;
+	}
+
+	if (ipv6_addr_any(&np->saddr))
+		np->saddr = fl6.saddr;
+
+	if (ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
+		sk->sk_v6_rcv_saddr = fl6.saddr;
+		inet->inet_rcv_saddr = LOOPBACK4_IPV6;
+		if (sk->sk_prot->rehash)
+			sk->sk_prot->rehash(sk);
+	}
+
+	ip6_dst_store(sk, dst,
+		      ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
+		      &sk->sk_v6_daddr : NULL,
+#ifdef CONFIG_IPV6_SUBTREES
+		      ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
+		      &np->saddr :
+#endif
+		      NULL);
+
+out:
+	fl6_sock_release(flowlabel);
+	return err;
+}
+
 static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
 	struct sockaddr_in6	*usin = (struct sockaddr_in6 *) uaddr;
 	struct inet_sock	*inet = inet_sk(sk);
 	struct ipv6_pinfo	*np = inet6_sk(sk);
-	struct in6_addr	*daddr, *final_p, final;
-	struct dst_entry	*dst;
-	struct flowi6		fl6;
-	struct ip6_flowlabel	*flowlabel = NULL;
-	struct ipv6_txoptions	*opt;
+	struct in6_addr		*daddr;
 	int			addr_type;
 	int			err;
 	__be32			fl6_flowlabel = 0;
@@ -91,14 +140,8 @@ static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int a
 	if (usin->sin6_family != AF_INET6)
 		return -EAFNOSUPPORT;
 
-	if (np->sndflow) {
+	if (np->sndflow)
 		fl6_flowlabel = usin->sin6_flowinfo & IPV6_FLOWINFO_MASK;
-		if (fl6_flowlabel & IPV6_FLOWLABEL_MASK) {
-			flowlabel = fl6_sock_lookup(sk, fl6_flowlabel);
-			if (!flowlabel)
-				return -EINVAL;
-		}
-	}
 
 	addr_type = ipv6_addr_type(&usin->sin6_addr);
 
@@ -178,45 +221,13 @@ ipv4_connected:
 	 *	destination cache for it.
 	 */
 
-	ip6_datagram_flow_key_init(&fl6, sk);
-
-	rcu_read_lock();
-	opt = flowlabel ? flowlabel->opt : rcu_dereference(np->opt);
-	final_p = fl6_update_dst(&fl6, opt, &final);
-	rcu_read_unlock();
-
-	dst = ip6_dst_lookup_flow(sk, &fl6, final_p);
-	err = 0;
-	if (IS_ERR(dst)) {
-		err = PTR_ERR(dst);
+	err = ip6_datagram_dst_update(sk);
+	if (err)
 		goto out;
-	}
-
-	/* source address lookup done in ip6_dst_lookup */
-
-	if (ipv6_addr_any(&np->saddr))
-		np->saddr = fl6.saddr;
-
-	if (ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
-		sk->sk_v6_rcv_saddr = fl6.saddr;
-		inet->inet_rcv_saddr = LOOPBACK4_IPV6;
-		if (sk->sk_prot->rehash)
-			sk->sk_prot->rehash(sk);
-	}
-
-	ip6_dst_store(sk, dst,
-		      ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
-		      &sk->sk_v6_daddr : NULL,
-#ifdef CONFIG_IPV6_SUBTREES
-		      ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
-		      &np->saddr :
-#endif
-		      NULL);
 
 	sk->sk_state = TCP_ESTABLISHED;
 	sk_set_txhash(sk);
 out:
-	fl6_sock_release(flowlabel);
 	return err;
 }
 
-- 
2.5.1

[RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

There is a case in connected UDP socket such that
getsockopt(IPV6_MTU) will return a stale MTU value. The reproducible
sequence could be the following:
1. Create a connected UDP socket
2. Send some datagrams out
3. Receive a ICMPV6_PKT_TOOBIG
4. No new outgoing datagrams to trigger the sk_dst_check()
   logic to update the sk->sk_dst_cache.
5. getsockopt(IPV6_MTU) returns the mtu from the invalid
   sk->sk_dst_cache instead of the newly created RTF_CACHE clone.

This patch updates the sk->sk_dst_cache for a connected datagram sk.

It is done under '!sock_owned_by_user(sk)' condition because
the user may make another ip6_datagram_connect() while
dst lookup and update are happening.

For the sock_owned_by_user(sk) == true case, the next patch will
introduce a release_cb() which will update the sk->sk_dst_cache.

Test:

Server (Connected UDP Socket):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Route Details:
[root@arch-fb-vm1 ~]# ip -6 r show | egrep '2fac'
2fac::/64 dev eth0  proto kernel  metric 256  pref medium
2fac:face::/64 via 2fac::face dev eth0  metric 1024  pref medium

A simple python code to create a connected UDP socket:

import socket
import errno

HOST = '2fac::1'
PORT = 8080

s = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
s.bind((HOST, PORT))
s.connect(('2fac:face::face', 53))
print("connected")
while True:
    try:
	data = s.recv(1024)
    except socket.error as se:
	if se.errno == errno.EMSGSIZE:
		pmtu = s.getsockopt(41, 24)
		print("PMTU:%d" % pmtu)
		break
s.close()

Python program output after getting a ICMPV6_PKT_TOOBIG:
[root@arch-fb-vm1 ~]# python2 ~/devshare/kernel/tasks/fib6/udp-connect-53-8080.py
connected
PMTU:1300

Cache routes after recieving TOOBIG:
[root@arch-fb-vm1 ~]# ip -6 r show table cache
2fac:face::face via 2fac::face dev eth0  metric 0
    cache  expires 463sec mtu 1300 pref medium

Client (Send the ICMPV6_PKT_TOOBIG):
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scapy is used to generate the TOOBIG message.  Here is the scapy script I have
used:

>>> p=Ether(src='da:75:4d:36:ac:32', dst='52:54:00:12:34:66', type=0x86dd)/IPv6(src='2fac::face', dst='2fac::1')/ICMPv6PacketTooBig(mtu=1300)/IPv6(src='2fac::
1',dst='2fac:face::face', nh='UDP')/UDP(sport=8080,dport=53)
>>> sendp(p, iface='qemubr0')

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Reported-by: Wei Wang <weiwan@google.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 include/net/ipv6.h  |  1 +
 net/ipv6/datagram.c | 20 +++++++++++---------
 net/ipv6/route.c    | 11 +++++++++++
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index d0aeb97..fd02e90 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -959,6 +959,7 @@ int compat_ipv6_getsockopt(struct sock *sk, int level, int optname,
 int ip6_datagram_connect(struct sock *sk, struct sockaddr *addr, int addr_len);
 int ip6_datagram_connect_v6_only(struct sock *sk, struct sockaddr *addr,
 				 int addr_len);
+int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr);
 
 int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len,
 		    int *addr_len);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 140665b..0b60f1e 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -64,7 +64,7 @@ static void ip6_datagram_flow_key_init(struct flowi6 *fl6, struct sock *sk)
 	security_sk_classify_flow(sk, flowi6_to_flowi(fl6));
 }
 
-static int ip6_datagram_dst_update(struct sock *sk)
+int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)
 {
 	struct ip6_flowlabel *flowlabel = NULL;
 	struct in6_addr *final_p, final;
@@ -93,14 +93,16 @@ static int ip6_datagram_dst_update(struct sock *sk)
 		goto out;
 	}
 
-	if (ipv6_addr_any(&np->saddr))
-		np->saddr = fl6.saddr;
+	if (fix_sk_saddr) {
+		if (ipv6_addr_any(&np->saddr))
+			np->saddr = fl6.saddr;
 
-	if (ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
-		sk->sk_v6_rcv_saddr = fl6.saddr;
-		inet->inet_rcv_saddr = LOOPBACK4_IPV6;
-		if (sk->sk_prot->rehash)
-			sk->sk_prot->rehash(sk);
+		if (ipv6_addr_any(&sk->sk_v6_rcv_saddr)) {
+			sk->sk_v6_rcv_saddr = fl6.saddr;
+			inet->inet_rcv_saddr = LOOPBACK4_IPV6;
+			if (sk->sk_prot->rehash)
+				sk->sk_prot->rehash(sk);
+		}
 	}
 
 	ip6_dst_store(sk, dst,
@@ -221,7 +223,7 @@ ipv4_connected:
 	 *	destination cache for it.
 	 */
 
-	err = ip6_datagram_dst_update(sk);
+	err = ip6_datagram_dst_update(sk, true);
 	if (err)
 		goto out;
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed44663..f7e6a6d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1417,8 +1417,19 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
 
 void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
 {
+	struct dst_entry *dst;
+
 	ip6_update_pmtu(skb, sock_net(sk), mtu,
 			sk->sk_bound_dev_if, sk->sk_mark);
+
+	dst = __sk_dst_get(sk);
+	if (!dst || dst->ops->check(dst, inet6_sk(sk)->dst_cookie))
+		return;
+
+	bh_lock_sock(sk);
+	if (!sock_owned_by_user(sk))
+		ip6_datagram_dst_update(sk, false);
+	bh_unlock_sock(sk);
 }
 EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
 
-- 
2.5.1

[RFC PATCH net 4/4] ipv6: udp: Do a route lookup and update during release_cb

[Martin KaFai Lau]

This patch adds a release_cb for UDPv6.  It does a route lookup
and updates sk->sk_dst_cache if it is needed.  It picks up the
left-over job from ip6_sk_update_pmtu() if the sk is owned
by user during the pmtu update.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Reported-by: Wei Wang <weiwan@google.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 include/net/ipv6.h  |  1 +
 net/ipv6/datagram.c | 12 ++++++++++++
 net/ipv6/udp.c      |  1 +
 3 files changed, 14 insertions(+)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index fd02e90..1be050a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -960,6 +960,7 @@ int ip6_datagram_connect(struct sock *sk, struct sockaddr *addr, int addr_len);
 int ip6_datagram_connect_v6_only(struct sock *sk, struct sockaddr *addr,
 				 int addr_len);
 int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr);
+void ip6_datagram_release_cb(struct sock *sk);
 
 int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len,
 		    int *addr_len);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 0b60f1e..a4f06bd 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -119,6 +119,18 @@ out:
 	return err;
 }
 
+void ip6_datagram_release_cb(struct sock *sk)
+{
+	struct dst_entry *dst;
+
+	dst = __sk_dst_get(sk);
+	if (!dst || dst->ops->check(dst, inet6_sk(sk)->dst_cookie))
+		return;
+
+	ip6_datagram_dst_update(sk, false);
+}
+EXPORT_SYMBOL_GPL(ip6_datagram_release_cb);
+
 static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 {
 	struct sockaddr_in6	*usin = (struct sockaddr_in6 *) uaddr;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 8125931..6bc5c66 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1539,6 +1539,7 @@ struct proto udpv6_prot = {
 	.sendmsg	   = udpv6_sendmsg,
 	.recvmsg	   = udpv6_recvmsg,
 	.backlog_rcv	   = __udpv6_queue_rcv_skb,
+	.release_cb	   = ip6_datagram_release_cb,
 	.hash		   = udp_lib_hash,
 	.unhash		   = udp_lib_unhash,
 	.rehash		   = udp_v6_rehash,
-- 
2.5.1

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Cong Wang]

On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> +       bh_lock_sock(sk);
> +       if (!sock_owned_by_user(sk))
> +               ip6_datagram_dst_update(sk, false);
> +       bh_unlock_sock(sk);


My discussion with Eric shows that we probably don't need to hold
this sock lock here, and you are Cc'ed in that thread, so

1) why do you still take the lock here?
2) why didn't you involve in our discussion if you disagree?

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Cong Wang]

On Fri, Apr 1, 2016 at 4:13 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau <kafai@fb.com> wrote:
>> +       bh_lock_sock(sk);
>> +       if (!sock_owned_by_user(sk))
>> +               ip6_datagram_dst_update(sk, false);
>> +       bh_unlock_sock(sk);
>
>
> My discussion with Eric shows that we probably don't need to hold
> this sock lock here, and you are Cc'ed in that thread, so
>
> 1) why do you still take the lock here?
> 2) why didn't you involve in our discussion if you disagree?

Here is the original thread:
http://comments.gmane.org/gmane.linux.network/405404

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

On Fri, Apr 01, 2016 at 04:13:41PM -0700, Cong Wang wrote:
> On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> > +       bh_lock_sock(sk);
> > +       if (!sock_owned_by_user(sk))
> > +               ip6_datagram_dst_update(sk, false);
> > +       bh_unlock_sock(sk);
>
>
> My discussion with Eric shows that we probably don't need to hold
> this sock lock here, and you are Cc'ed in that thread, so
>
> 1) why do you still take the lock here?
> 2) why didn't you involve in our discussion if you disagree?
It is because I agree with the last thread discussion that updating
sk->sk_dst_cache does not need a sk lock.  I also don't see
a lock is need for other operations in that thread.

I am thinking another case that needs a lock, so I start
another RFC thread.  A quick recall for this commit message:
>> It is done under '!sock_owned_by_user(sk)' condition because
>> the user may make another ip6_datagram_connect() while
>> dst lookup and update are happening.
If that could not happen, then the lock is not needed.

One thing to note is that this patch uses the addresses from the sk
instead of iph when updating sk->sk_dst_cache.  It is basically the
same logic that the __ip6_datagram_connect() is doing, so some
refactoring works in the first two patches.

AFAIK, a UDP socket can become connected after sending out some
datagrams in un-connected state.  or It can be connected
multiple times to different destinations.  I did some quick
tests but I could be wrong.

I am thinking if there could be a chance that the skb->data, which
has the original outgoing iph, is not related to the current
connected address.  If it is possible, we have to specifically
use the addresses in the sk instead of skb->data (i.e. iph) when
updating the sk->sk_dst_cache.

If we need to use the sk addresses (and other info) to find out a
new dst for a connected udp socket, it is better not doing it while
the userland is connecting to somewhere else.

If the above case is impossible, we can keep using the info from iph to
do the dst update for a connected-udp sk without taking the lock.

>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index ed44663..f7e6a6d 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -1417,8 +1417,19 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
>>
>>  void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
>>  {
>> +	struct dst_entry *dst;
>> +
>>  	ip6_update_pmtu(skb, sock_net(sk), mtu,
>>  			sk->sk_bound_dev_if, sk->sk_mark);
iph's addresses are used to update the pmtu.  It is fine
because it does not update the sk->sk_dst_cache.

>>> +
>> +	dst = __sk_dst_get(sk);
>> +	if (!dst || dst->ops->check(dst, inet6_sk(sk)->dst_cookie))
>> +		return;
>> +
>> +	bh_lock_sock(sk);
>> +	if (!sock_owned_by_user(sk))
sk is not connecting to another address.  Find a new dst
for the connected address.
>> +		ip6_datagram_dst_update(sk, false);
>> +	bh_unlock_sock(sk);
>>  }

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Cong Wang]

On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> One thing to note is that this patch uses the addresses from the sk
> instead of iph when updating sk->sk_dst_cache.  It is basically the
> same logic that the __ip6_datagram_connect() is doing, so some
> refactoring works in the first two patches.
>
> AFAIK, a UDP socket can become connected after sending out some
> datagrams in un-connected state.  or It can be connected
> multiple times to different destinations.  I did some quick
> tests but I could be wrong.
>
> I am thinking if there could be a chance that the skb->data, which
> has the original outgoing iph, is not related to the current
> connected address.  If it is possible, we have to specifically
> use the addresses in the sk instead of skb->data (i.e. iph) when
> updating the sk->sk_dst_cache.
>
> If we need to use the sk addresses (and other info) to find out a
> new dst for a connected udp socket, it is better not doing it while
> the userland is connecting to somewhere else.
>
> If the above case is impossible, we can keep using the info from iph to
> do the dst update for a connected-udp sk without taking the lock.

I see your point, but calling __ip6_datagram_connect() seems overkill
here, we don't need to update so many things in the pmtu update context,
at least IPv4 doesn't do that either. I don't think you have to do that.

So why just updating the dst cache (also some addr cache) here is not
enough?

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 4 Apr 2016 13:45:02 -0700

> On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau <kafai@fb.com> wrote:
>> One thing to note is that this patch uses the addresses from the sk
>> instead of iph when updating sk->sk_dst_cache.  It is basically the
>> same logic that the __ip6_datagram_connect() is doing, so some
>> refactoring works in the first two patches.
>>
>> AFAIK, a UDP socket can become connected after sending out some
>> datagrams in un-connected state.  or It can be connected
>> multiple times to different destinations.  I did some quick
>> tests but I could be wrong.
>>
>> I am thinking if there could be a chance that the skb->data, which
>> has the original outgoing iph, is not related to the current
>> connected address.  If it is possible, we have to specifically
>> use the addresses in the sk instead of skb->data (i.e. iph) when
>> updating the sk->sk_dst_cache.
>>
>> If we need to use the sk addresses (and other info) to find out a
>> new dst for a connected udp socket, it is better not doing it while
>> the userland is connecting to somewhere else.
>>
>> If the above case is impossible, we can keep using the info from iph to
>> do the dst update for a connected-udp sk without taking the lock.
> 
> I see your point, but calling __ip6_datagram_connect() seems overkill
> here, we don't need to update so many things in the pmtu update context,
> at least IPv4 doesn't do that either. I don't think you have to do that.
> 
> So why just updating the dst cache (also some addr cache) here is not
> enough?

I think we are steadily getting closer to a version of this fix that
we have some agreement on, right?

Martin can you address Cong's feedback and spin another version of this
series?

Thanks.

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

On Mon, Apr 04, 2016 at 01:45:02PM -0700, Cong Wang wrote:
> I see your point, but calling __ip6_datagram_connect() seems overkill
> here, we don't need to update so many things in the pmtu update context,
> at least IPv4 doesn't do that either. I don't think you have to do that.
>
> So why just updating the dst cache (also some addr cache) here is not
> enough?
I am not sure I understand.  I could be missing something.

This patch uses ip6_datagram_dst_update() to do the route lookup and
sk->sk_dst_cache update.  ip6_datagram_dst_update() is
created in the first two refactoring patches and is also used by
__ip6_datagram_connect().

Which operations in ip6_datagram_dst_update() could be saved
during the pmtu update?

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Cong Wang]

On Tue, Apr 5, 2016 at 5:11 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Mon, Apr 04, 2016 at 01:45:02PM -0700, Cong Wang wrote:
>> I see your point, but calling __ip6_datagram_connect() seems overkill
>> here, we don't need to update so many things in the pmtu update context,
>> at least IPv4 doesn't do that either. I don't think you have to do that.
>>
>> So why just updating the dst cache (also some addr cache) here is not
>> enough?
> I am not sure I understand.  I could be missing something.
>
> This patch uses ip6_datagram_dst_update() to do the route lookup and
> sk->sk_dst_cache update.  ip6_datagram_dst_update() is
> created in the first two refactoring patches and is also used by
> __ip6_datagram_connect().
>
> Which operations in ip6_datagram_dst_update() could be saved
> during the pmtu update?

I thought you call the same ip6_datagram_dst_update() for both
pmtu update and __ip6_datagram_connect(), but you actually skip
some sk operations for pmtu case, which means you don't need
to worry about parallel ip6_datagram_connect().

IPv6 UDP sendmsg() path stores the dst without sock lock anyway,
we don't cope with a concurrent connect() on another cpu. But
still, I don't see this is a problem here, because even if we store
an obsolete address in cache, it would be corrected later.

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

On Wed, Apr 06, 2016 at 10:58:23AM -0700, Cong Wang wrote:
> On Tue, Apr 5, 2016 at 5:11 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> > On Mon, Apr 04, 2016 at 01:45:02PM -0700, Cong Wang wrote:
> >> I see your point, but calling __ip6_datagram_connect() seems overkill
> >> here, we don't need to update so many things in the pmtu update context,
> >> at least IPv4 doesn't do that either. I don't think you have to do that.
> >>
> >> So why just updating the dst cache (also some addr cache) here is not
> >> enough?
> > I am not sure I understand.  I could be missing something.
> >
> > This patch uses ip6_datagram_dst_update() to do the route lookup and
> > sk->sk_dst_cache update.  ip6_datagram_dst_update() is
> > created in the first two refactoring patches and is also used by
> > __ip6_datagram_connect().
> >
> > Which operations in ip6_datagram_dst_update() could be saved
> > during the pmtu update?
>
> I thought you call the same ip6_datagram_dst_update() for both
> pmtu update and __ip6_datagram_connect(), but you actually skip
> some sk operations for pmtu case, which means you don't need
> to worry about parallel ip6_datagram_connect().
>
> IPv6 UDP sendmsg() path stores the dst without sock lock anyway,
> we don't cope with a concurrent connect() on another cpu.
A parallel sendmsg and connect could be an issue.  The user is connecting
to a new dest while another parallel sendmsg is sending to (could be the old
dest, new dest or somewhere between old and new dest?)

However, it is the userland making and it will be another patch if we want
to protect this case too.

In pmtu update, the kernel is doing the lookup and update without the
userland conscious.

> But still, I don't see this is a problem here, because even if we store
> an obsolete address in cache, it would be corrected later.
The sendmsg() path will correct it (relookup and update sk_dst_cache) but not
the getsockopt(IPV6_MTU) path which is what this patch is trying to fix: Update
a _valid_ dst to sk->sk_dst_cache.

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Cong Wang]

On Wed, Apr 6, 2016 at 11:49 AM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Wed, Apr 06, 2016 at 10:58:23AM -0700, Cong Wang wrote:
>> On Tue, Apr 5, 2016 at 5:11 PM, Martin KaFai Lau <kafai@fb.com> wrote:
>> > On Mon, Apr 04, 2016 at 01:45:02PM -0700, Cong Wang wrote:
>> >> I see your point, but calling __ip6_datagram_connect() seems overkill
>> >> here, we don't need to update so many things in the pmtu update context,
>> >> at least IPv4 doesn't do that either. I don't think you have to do that.
>> >>
>> >> So why just updating the dst cache (also some addr cache) here is not
>> >> enough?
>> > I am not sure I understand.  I could be missing something.
>> >
>> > This patch uses ip6_datagram_dst_update() to do the route lookup and
>> > sk->sk_dst_cache update.  ip6_datagram_dst_update() is
>> > created in the first two refactoring patches and is also used by
>> > __ip6_datagram_connect().
>> >
>> > Which operations in ip6_datagram_dst_update() could be saved
>> > during the pmtu update?
>>
>> I thought you call the same ip6_datagram_dst_update() for both
>> pmtu update and __ip6_datagram_connect(), but you actually skip
>> some sk operations for pmtu case, which means you don't need
>> to worry about parallel ip6_datagram_connect().
>>
>> IPv6 UDP sendmsg() path stores the dst without sock lock anyway,
>> we don't cope with a concurrent connect() on another cpu.
> A parallel sendmsg and connect could be an issue.  The user is connecting
> to a new dest while another parallel sendmsg is sending to (could be the old
> dest, new dest or somewhere between old and new dest?)
>
> However, it is the userland making and it will be another patch if we want
> to protect this case too.

Yeah, it is a different problem, but no one complains about it yet.

>
> In pmtu update, the kernel is doing the lookup and update without the
> userland conscious.
>
>> But still, I don't see this is a problem here, because even if we store
>> an obsolete address in cache, it would be corrected later.
> The sendmsg() path will correct it (relookup and update sk_dst_cache) but not
> the getsockopt(IPV6_MTU) path which is what this patch is trying to fix: Update
> a _valid_ dst to sk->sk_dst_cache.

You are lost in discussion, I never object to update sk_dst_cache, what
we disagree here is merely if we need to lock the sock in pmtu update
context.

I still think it is okay without the lock, because even if you take the lock,
the pmtu update could still happen after you release it, so there is no
essential difference here. The only reason I can think of for taking
the sock lock is protecting parallel pmtu update, but it looks safe for
this case too.

So which case do you want to protect by taking the sock lock?

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

On Thu, Apr 07, 2016 at 11:37:10AM -0700, Cong Wang wrote:
> You are lost in discussion
Indeed. :(

>
> I still think it is okay without the lock, because even if you take the lock,
> the pmtu update could still happen after you release it, so there is no
> essential difference here. The only reason I can think of for taking
> the sock lock is protecting parallel pmtu update, but it looks safe for
> this case too.
>
> So which case do you want to protect by taking the sock lock?
When the pmtu-update is doing route lookup and another connect is
happening, what sk->sk_v6_daddr will this route lookup use?
the old one, new one or neither of them?

Is it acceptable that getsockopt() is returning something that it
is not currently connected to? and potentially somewhere that it
is never connected to?

Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

[Martin KaFai Lau]

On Tue, Apr 05, 2016 at 07:56:54PM -0400, David Miller wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 4 Apr 2016 13:45:02 -0700
>
> > On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> >> One thing to note is that this patch uses the addresses from the sk
> >> instead of iph when updating sk->sk_dst_cache.  It is basically the
> >> same logic that the __ip6_datagram_connect() is doing, so some
> >> refactoring works in the first two patches.
> >>
> >> AFAIK, a UDP socket can become connected after sending out some
> >> datagrams in un-connected state.  or It can be connected
> >> multiple times to different destinations.  I did some quick
> >> tests but I could be wrong.
> >>
> >> I am thinking if there could be a chance that the skb->data, which
> >> has the original outgoing iph, is not related to the current
> >> connected address.  If it is possible, we have to specifically
> >> use the addresses in the sk instead of skb->data (i.e. iph) when
> >> updating the sk->sk_dst_cache.
> >>
> >> If we need to use the sk addresses (and other info) to find out a
> >> new dst for a connected udp socket, it is better not doing it while
> >> the userland is connecting to somewhere else.
> >>
> >> If the above case is impossible, we can keep using the info from iph to
> >> do the dst update for a connected-udp sk without taking the lock.
> >
> > I see your point, but calling __ip6_datagram_connect() seems overkill
> > here, we don't need to update so many things in the pmtu update context,
> > at least IPv4 doesn't do that either. I don't think you have to do that.
> >
> > So why just updating the dst cache (also some addr cache) here is not
> > enough?
>
> I think we are steadily getting closer to a version of this fix that
> we have some agreement on, right?
>
> Martin can you address Cong's feedback and spin another version of this
> series?
Dave, I will spin v2 shortly with some minor changes.

Cong, I believe the loose ends have been tied up after the last email thread
since last Thursday?