[PATCH net v6 0/4] ipv6: udp: set dst cache for a connected sk if current not valid

[PATCH net v6 0/4] ipv6: udp: set dst cache for a connected sk if current not valid

[Alexey Kodanev]

A new RTF_CACHE route can be created with the socket's dst cache
update between the below calls in udpv6_sendmsg(), when datagram
sending results to ICMPV6_PKT_TOOBIG error:

   dst = ip6_sk_dst_lookup_flow(...)
   ...
release_dst:
    if (dst) {
        if (connected) {
            ip6_dst_store(sk, dst)

Therefore, the new socket's dst cache reset to the old one on
"release_dst:".

The first three patches prepare the code to store dst cache
with ip6_sk_dst_lookup_flow():

  * the first patch adds ip6_sk_dst_store_flow() function with
    commonly used source and destiantion addresses checks using
    the flow information.

  * the second patch adds a new argument to ip6_sk_dst_lookup_flow()
    and ability to store dst in the socket's cache. Also, the two
    users of the function are updated without enabling the new
    behavior: pingv6_sendmsg() and udpv6_sendmsg().

  * the third patch makes 'connected' variable in udpv6_sendmsg()
    to be consistent with ip6_sk_dst_store_flow(), changes its type
    from int to bool.

The last patch contains the actual fix that removes sk dst cache
update in the end of udpv6_sendmsg(), and allows to do it in
ip6_sk_dst_lookup_flow().

v6: * use bool type for a new parameter in ip_sk_dst_lookup_flow()
    * add one more patch to convert 'connected' variable in
      udpv6_sendmsg() from int to bool type. If it shouldn't be
      here I will resend it when the net-next is opened.

v5: * relocate ip6_sk_dst_store_flow() to net/ipv6/route.c and
      rename ip6_dst_store_flow() to ip6_sk_dst_store_flow() as
      suggested by Martin

v4: * fix the error in the build of ip_dst_store_flow() reported by
      kbuild test robot due to missing checks for CONFIG_IPV6: add
      new function to ip6_output.c instead of ip6_route.h
    * add 'const' to struct flowi6 in ip6_dst_store_flow()
    * minor commit messages fixes

v3: * instead of moving ip6_dst_store() above udp_v6_send_skb(),
      update socket's dst cache inside ip6_sk_dst_lookup_flow()
      if the current one is invalid
    * the issue not reproduced in 4.1, but starting from 4.2. Add
      one more 'Fixes:' commit that creates new RTF_CACHE route.
      Though, it is also mentioned in the first one

Alexey Kodanev (4):
  ipv6: add a wrapper for ip6_dst_store() with flowi6 checks
  ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()
  ipv6: udp: convert 'connected' to bool type in udpv6_sendmsg()
  ipv6: udp: set dst cache for a connected sk if current not valid

 include/net/ip6_route.h |  3 +++
 include/net/ipv6.h      |  3 ++-
 net/ipv6/datagram.c     |  9 +--------
 net/ipv6/ip6_output.c   | 15 ++++++++++++---
 net/ipv6/ping.c         |  2 +-
 net/ipv6/route.c        | 17 +++++++++++++++++
 net/ipv6/udp.c          | 31 +++++++------------------------
 7 files changed, 43 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[PATCH net v6 1/4] ipv6: add a wrapper for ip6_dst_store() with flowi6 checks

[Alexey Kodanev]

Move commonly used pattern of ip6_dst_store() usage to a separate
function - ip6_sk_dst_store_flow(), which will check the addresses
for equality using the flow information, before saving them.

There is no functional changes in this patch. In addition, it will
be used in the next patch, in ip6_sk_dst_lookup_flow().

Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 include/net/ip6_route.h |  3 +++
 net/ipv6/datagram.c     |  9 +--------
 net/ipv6/route.c        | 17 +++++++++++++++++
 3 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index ac0866b..abec280 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -210,6 +210,9 @@ static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,
 #endif
 }
 
+void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
+			   const struct flowi6 *fl6);
+
 static inline bool ipv6_unicast_destination(const struct sk_buff *skb)
 {
 	struct rt6_info *rt = (struct rt6_info *) skb_dst(skb);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a9f7eca..8f6a391 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -106,14 +106,7 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)
 		}
 	}
 
-	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);
+	ip6_sk_dst_store_flow(sk, dst, &fl6);
 
 out:
 	fl6_sock_release(flowlabel);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b0d5c64..b14008e 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2153,6 +2153,23 @@ void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
 }
 EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
 
+void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
+			   const struct flowi6 *fl6)
+{
+#ifdef CONFIG_IPV6_SUBTREES
+	struct ipv6_pinfo *np = inet6_sk(sk);
+#endif
+
+	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);
+}
+
 /* Handle redirects */
 struct ip6rd_flowi {
 	struct flowi6 fl6;
-- 
1.8.3.1

[PATCH net v6 2/4] ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()

[Alexey Kodanev]

Add 'connected' parameter to ip6_sk_dst_lookup_flow() and update
the cache only if ip6_sk_dst_check() returns NULL and a socket
is connected.

The function is used as before, the new behavior for UDP sockets
in udpv6_sendmsg() will be enabled in the next patch.

Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 include/net/ipv6.h    |  3 ++-
 net/ipv6/ip6_output.c | 15 ++++++++++++---
 net/ipv6/ping.c       |  2 +-
 net/ipv6/udp.c        |  2 +-
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 8606c91..1d416f2 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -977,7 +977,8 @@ int ip6_dst_lookup(struct net *net, struct sock *sk, struct dst_entry **dst,
 struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
 				      const struct in6_addr *final_dst);
 struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
-					 const struct in6_addr *final_dst);
+					 const struct in6_addr *final_dst,
+					 bool connected);
 struct dst_entry *ip6_blackhole_route(struct net *net,
 				      struct dst_entry *orig_dst);
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a8a9195..46ea7b6 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1105,23 +1105,32 @@ struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
  *	@sk: socket which provides the dst cache and route info
  *	@fl6: flow to lookup
  *	@final_dst: final destination address for ipsec lookup
+ *	@connected: whether @sk is connected or not
  *
  *	This function performs a route lookup on the given flow with the
  *	possibility of using the cached route in the socket if it is valid.
  *	It will take the socket dst lock when operating on the dst cache.
  *	As a result, this function can only be used in process context.
  *
+ *	In addition, for a connected socket, cache the dst in the socket
+ *	if the current cache is not valid.
+ *
  *	It returns a valid dst pointer on success, or a pointer encoded
  *	error code.
  */
 struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
-					 const struct in6_addr *final_dst)
+					 const struct in6_addr *final_dst,
+					 bool connected)
 {
 	struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
 
 	dst = ip6_sk_dst_check(sk, dst, fl6);
-	if (!dst)
-		dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
+	if (dst)
+		return dst;
+
+	dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
+	if (connected && !IS_ERR(dst))
+		ip6_sk_dst_store_flow(sk, dst_clone(dst), fl6);
 
 	return dst;
 }
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index d12c55d..746eeae 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -121,7 +121,7 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	ipc6.tclass = np->tclass;
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
-	dst = ip6_sk_dst_lookup_flow(sk, &fl6,  daddr);
+	dst = ip6_sk_dst_lookup_flow(sk, &fl6, daddr, false);
 	if (IS_ERR(dst))
 		return PTR_ERR(dst);
 	rt = (struct rt6_info *) dst;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 52e3ea0..5bc102b2 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1289,7 +1289,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
-	dst = ip6_sk_dst_lookup_flow(sk, &fl6, final_p);
+	dst = ip6_sk_dst_lookup_flow(sk, &fl6, final_p, false);
 	if (IS_ERR(dst)) {
 		err = PTR_ERR(dst);
 		dst = NULL;
-- 
1.8.3.1

[PATCH net v6 3/4] ipv6: udp: convert 'connected' to bool type in udpv6_sendmsg()

[Alexey Kodanev]

This should make it consistent with ip6_sk_dst_lookup_flow()
that is accepting the new 'connected' parameter of type bool.

Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 net/ipv6/udp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 5bc102b2..4aa50ea 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1097,10 +1097,10 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	struct dst_entry *dst;
 	struct ipcm6_cookie ipc6;
 	int addr_len = msg->msg_namelen;
+	bool connected = false;
 	int ulen = len;
 	int corkreq = up->corkflag || msg->msg_flags&MSG_MORE;
 	int err;
-	int connected = 0;
 	int is_udplite = IS_UDPLITE(sk);
 	int (*getfrag)(void *, char *, int, int, int, struct sk_buff *);
 	struct sockcm_cookie sockc;
@@ -1222,7 +1222,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		fl6.fl6_dport = inet->inet_dport;
 		daddr = &sk->sk_v6_daddr;
 		fl6.flowlabel = np->flow_label;
-		connected = 1;
+		connected = true;
 	}
 
 	if (!fl6.flowi6_oif)
@@ -1252,7 +1252,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		}
 		if (!(opt->opt_nflen|opt->opt_flen))
 			opt = NULL;
-		connected = 0;
+		connected = false;
 	}
 	if (!opt) {
 		opt = txopt_get(np);
@@ -1274,11 +1274,11 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	final_p = fl6_update_dst(&fl6, opt, &final);
 	if (final_p)
-		connected = 0;
+		connected = false;
 
 	if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) {
 		fl6.flowi6_oif = np->mcast_oif;
-		connected = 0;
+		connected = false;
 	} else if (!fl6.flowi6_oif)
 		fl6.flowi6_oif = np->ucast_oif;
 
-- 
1.8.3.1

[PATCH net v6 4/4] ipv6: udp: set dst cache for a connected sk if current not valid

[Alexey Kodanev]

A new RTF_CACHE route can be created between ip6_sk_dst_lookup_flow()
and ip6_dst_store() calls in udpv6_sendmsg(), when datagram sending
results to ICMPV6_PKT_TOOBIG error:

    udp_v6_send_skb(), for example with vti6 tunnel:
        vti6_xmit(), get ICMPV6_PKT_TOOBIG error
            skb_dst_update_pmtu(), can create a RTF_CACHE clone
            icmpv6_send()
    ...
    udpv6_err()
        ip6_sk_update_pmtu()
           ip6_update_pmtu(), can create a RTF_CACHE clone
           ...
           ip6_datagram_dst_update()
                ip6_dst_store()

And after commit 33c162a980fe ("ipv6: datagram: Update dst cache of
a connected datagram sk during pmtu update"), the UDPv6 error handler
can update socket's dst cache, but it can happen before the update in
the end of udpv6_sendmsg(), preventing getting the new dst cache on
the next udpv6_sendmsg() calls.

In order to fix it, save dst in a connected socket only if the current
socket's dst cache is invalid.

The previous patch prepared ip6_sk_dst_lookup_flow() to do that with
the new argument, and this patch enables it in udpv6_sendmsg().

Fixes: 33c162a980fe ("ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update")
Fixes: 45e4fd26683c ("ipv6: Only create RTF_CACHE routes after encountering pmtu exception")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 net/ipv6/udp.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 4aa50ea..9b74092 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1289,7 +1289,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 	fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
-	dst = ip6_sk_dst_lookup_flow(sk, &fl6, final_p, false);
+	dst = ip6_sk_dst_lookup_flow(sk, &fl6, final_p, connected);
 	if (IS_ERR(dst)) {
 		err = PTR_ERR(dst);
 		dst = NULL;
@@ -1314,7 +1314,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		err = PTR_ERR(skb);
 		if (!IS_ERR_OR_NULL(skb))
 			err = udp_v6_send_skb(skb, &fl6);
-		goto release_dst;
+		goto out;
 	}
 
 	lock_sock(sk);
@@ -1348,23 +1348,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 		err = np->recverr ? net_xmit_errno(err) : 0;
 	release_sock(sk);
 
-release_dst:
-	if (dst) {
-		if (connected) {
-			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);
-		} else {
-			dst_release(dst);
-		}
-		dst = NULL;
-	}
-
 out:
 	dst_release(dst);
 	fl6_sock_release(flowlabel);
-- 
1.8.3.1

Re: [PATCH net v6 0/4] ipv6: udp: set dst cache for a connected sk if current not valid

[Martin KaFai Lau]

On Tue, Apr 03, 2018 at 03:00:06PM +0300, Alexey Kodanev wrote:
> A new RTF_CACHE route can be created with the socket's dst cache
> update between the below calls in udpv6_sendmsg(), when datagram
> sending results to ICMPV6_PKT_TOOBIG error:
> 
>    dst = ip6_sk_dst_lookup_flow(...)
>    ...
> release_dst:
>     if (dst) {
>         if (connected) {
>             ip6_dst_store(sk, dst)
> 
> Therefore, the new socket's dst cache reset to the old one on
> "release_dst:".
> 
> The first three patches prepare the code to store dst cache
> with ip6_sk_dst_lookup_flow():
> 
>   * the first patch adds ip6_sk_dst_store_flow() function with
>     commonly used source and destiantion addresses checks using
>     the flow information.
> 
>   * the second patch adds a new argument to ip6_sk_dst_lookup_flow()
>     and ability to store dst in the socket's cache. Also, the two
>     users of the function are updated without enabling the new
>     behavior: pingv6_sendmsg() and udpv6_sendmsg().
> 
>   * the third patch makes 'connected' variable in udpv6_sendmsg()
>     to be consistent with ip6_sk_dst_store_flow(), changes its type
>     from int to bool.
> 
> The last patch contains the actual fix that removes sk dst cache
> update in the end of udpv6_sendmsg(), and allows to do it in
> ip6_sk_dst_lookup_flow().
Thanks for the patches!

Acked-by: Martin KaFai Lau <kafai@fb.com>

> 
> v6: * use bool type for a new parameter in ip_sk_dst_lookup_flow()
>     * add one more patch to convert 'connected' variable in
>       udpv6_sendmsg() from int to bool type. If it shouldn't be
>       here I will resend it when the net-next is opened.
> 
> v5: * relocate ip6_sk_dst_store_flow() to net/ipv6/route.c and
>       rename ip6_dst_store_flow() to ip6_sk_dst_store_flow() as
>       suggested by Martin
> 
> v4: * fix the error in the build of ip_dst_store_flow() reported by
>       kbuild test robot due to missing checks for CONFIG_IPV6: add
>       new function to ip6_output.c instead of ip6_route.h
>     * add 'const' to struct flowi6 in ip6_dst_store_flow()
>     * minor commit messages fixes
> 
> v3: * instead of moving ip6_dst_store() above udp_v6_send_skb(),
>       update socket's dst cache inside ip6_sk_dst_lookup_flow()
>       if the current one is invalid
>     * the issue not reproduced in 4.1, but starting from 4.2. Add
>       one more 'Fixes:' commit that creates new RTF_CACHE route.
>       Though, it is also mentioned in the first one
> 
> Alexey Kodanev (4):
>   ipv6: add a wrapper for ip6_dst_store() with flowi6 checks
>   ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()
>   ipv6: udp: convert 'connected' to bool type in udpv6_sendmsg()
>   ipv6: udp: set dst cache for a connected sk if current not valid
> 
>  include/net/ip6_route.h |  3 +++
>  include/net/ipv6.h      |  3 ++-
>  net/ipv6/datagram.c     |  9 +--------
>  net/ipv6/ip6_output.c   | 15 ++++++++++++---
>  net/ipv6/ping.c         |  2 +-
>  net/ipv6/route.c        | 17 +++++++++++++++++
>  net/ipv6/udp.c          | 31 +++++++------------------------
>  7 files changed, 43 insertions(+), 37 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH net v6 0/4] ipv6: udp: set dst cache for a connected sk if current not valid

[David Miller]

From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Tue,  3 Apr 2018 15:00:06 +0300

> A new RTF_CACHE route can be created with the socket's dst cache
> update between the below calls in udpv6_sendmsg(), when datagram
> sending results to ICMPV6_PKT_TOOBIG error:
> 
>    dst = ip6_sk_dst_lookup_flow(...)
>    ...
> release_dst:
>     if (dst) {
>         if (connected) {
>             ip6_dst_store(sk, dst)
> 
> Therefore, the new socket's dst cache reset to the old one on
> "release_dst:".
> 
> The first three patches prepare the code to store dst cache
> with ip6_sk_dst_lookup_flow():
 ...
> The last patch contains the actual fix that removes sk dst cache
> update in the end of udpv6_sendmsg(), and allows to do it in
> ip6_sk_dst_lookup_flow().

Looks good, applied and queued up for -stable.

Thanks!

Re: [PATCH net v6 4/4] ipv6: udp: set dst cache for a connected sk if current not valid

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 33c162a980fe ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update.

The bot has tested the following trees: v4.16, v4.15.15, v4.14.32, v4.9.92.

v4.16: Failed to apply! Possible dependencies:
    96818159c3c0 ("ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()")

v4.15.15: Failed to apply! Possible dependencies:
    96818159c3c0 ("ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()")

v4.14.32: Failed to apply! Possible dependencies:
    96818159c3c0 ("ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()")

v4.9.92: Failed to apply! Possible dependencies:
    96818159c3c0 ("ipv6: allow to cache dst for a connected sk in ip6_sk_dst_lookup_flow()")