[PATCH net v4 0/3] ipv6: udp6: set dst cache for a connected sk if current not valid

[PATCH net v4 0/3] ipv6: udp6: 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 two patches prepare the code to store dst cache
with ip6_sk_dst_lookup_flow():

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

  * the second patch adds 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 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().

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 (3):
  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: udp6: set dst cache for a connected sk if current not valid

 include/net/ipv6.h    |  6 ++++--
 net/ipv6/datagram.c   |  9 +--------
 net/ipv6/ip6_output.c | 32 +++++++++++++++++++++++++++++---
 net/ipv6/ping.c       |  2 +-
 net/ipv6/udp.c        | 21 ++-------------------
 5 files changed, 37 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[PATCH net v4 1/3] 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_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/ipv6.h    |  3 ++-
 net/ipv6/datagram.c   |  9 +--------
 net/ipv6/ip6_output.c | 17 +++++++++++++++++
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 8606c91..1dd31ca 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -971,7 +971,8 @@ static inline struct sk_buff *ip6_finish_skb(struct sock *sk)
 }
 
 unsigned int ip6_dst_mtu_forward(const struct dst_entry *dst);
-
+void ip6_dst_store_flow(struct sock *sk, struct dst_entry *dst,
+			const struct flowi6 *fl6);
 int ip6_dst_lookup(struct net *net, struct sock *sk, struct dst_entry **dst,
 		   struct flowi6 *fl6);
 struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a9f7eca..8b4fa0c 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_dst_store_flow(sk, dst, &fl6);
 
 out:
 	fl6_sock_release(flowlabel);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a8a9195..ed87ce5 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -943,6 +943,23 @@ static struct dst_entry *ip6_sk_dst_check(struct sock *sk,
 	return dst;
 }
 
+void ip6_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);
+}
+
 static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
 			       struct dst_entry **dst, struct flowi6 *fl6)
 {
-- 
1.8.3.1

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

[Alexey Kodanev]

Add 'connected' argument 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 1dd31ca..769550a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -978,7 +978,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,
+					 int 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 ed87ce5..cdcdf37 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1122,23 +1122,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,
+					 int 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_dst_store_flow(sk, dst_clone(dst), fl6);
 
 	return dst;
 }
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index d12c55d..546f4a6 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, 0);
 	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..e49dac4 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, 0);
 	if (IS_ERR(dst)) {
 		err = PTR_ERR(dst);
 		dst = NULL;
-- 
1.8.3.1

[PATCH net v4 3/3] ipv6: udp6: 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 e49dac4..da13c90 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, 0);
+	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 v4 0/3] ipv6: udp6: set dst cache for a connected sk if current not valid

[Martin KaFai Lau]

On Fri, Mar 30, 2018 at 08:53: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 two patches prepare the code to store dst cache
> with ip6_sk_dst_lookup_flow():
> 
>   * the first patch adds ip6_dst_store_flow() function with
>     commonly used source and destiantion addresses checks using
>     the flow information.
> 
>   * the second patch adds 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 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().
> 
> 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
Thanks for the patches!

Instead of ip6_output.c, would net/ipv6/route.c be a better
place for the new ip6_sk_dst_store_flow()?

Others LGTM.  

>     * 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 (3):
>   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: udp6: set dst cache for a connected sk if current not valid
> 
>  include/net/ipv6.h    |  6 ++++--
>  net/ipv6/datagram.c   |  9 +--------
>  net/ipv6/ip6_output.c | 32 +++++++++++++++++++++++++++++---
>  net/ipv6/ping.c       |  2 +-
>  net/ipv6/udp.c        | 21 ++-------------------
>  5 files changed, 37 insertions(+), 33 deletions(-)
> 
> -- 
> 1.8.3.1
>