[PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[Alexey Kodanev]

After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
connected datagram sk during pmtu update"), when the error occurs on
sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
error handler can trigger the following path and call ip6_dst_store():

    udpv6_err()
        ip6_sk_update_pmtu()
            ip6_datagram_dst_update()
                ip6_dst_lookup_flow(), can create a RTF_CACHE clone
                ...
                ip6_dst_store()

It can happen before a connected UDP socket invokes ip6_dst_store()
in the end of udpv6_sendmsg(), on destination release, as a result,
the last one changes dst to the old one, preventing getting updated
dst cache on the next udpv6_sendmsg() call.

This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().

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

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 52e3ea0..0d413c6 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1299,6 +1299,16 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	if (ipc6.hlimit < 0)
 		ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, 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);
+
 	if (msg->msg_flags&MSG_CONFIRM)
 		goto do_confirm;
 back_from_confirm:
@@ -1350,18 +1360,8 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 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 {
+		if (!connected)
 			dst_release(dst);
-		}
 		dst = NULL;
 	}
 
-- 
1.8.3.1

Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[Eric Dumazet]

On 03/23/2018 07:39 AM, Alexey Kodanev wrote:
> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
> connected datagram sk during pmtu update"), when the error occurs on
> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
> error handler can trigger the following path and call ip6_dst_store():
> 
>     udpv6_err()
>         ip6_sk_update_pmtu()
>             ip6_datagram_dst_update()
>                 ip6_dst_lookup_flow(), can create a RTF_CACHE clone
>                 ...
>                 ip6_dst_store()
> 
> It can happen before a connected UDP socket invokes ip6_dst_store()
> in the end of udpv6_sendmsg(), on destination release, as a result,
> the last one changes dst to the old one, preventing getting updated
> dst cache on the next udpv6_sendmsg() call.
> 
> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
> 


A Fixes: tag would be nice, for automatic tools (and humans as well)

> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
>  net/ipv6/udp.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 52e3ea0..0d413c6 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1299,6 +1299,16 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	if (ipc6.hlimit < 0)
>  		ipc6.hlimit = ip6_sk_dst_hoplimit(np, &fl6, 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);
> +

What about the MSG_CONFIRM stuff ?

>  	if (msg->msg_flags&MSG_CONFIRM)
>  		goto do_confirm;
>  back_from_confirm:

Should not you move the above code here instead ?

Also ip6_dst_store() does not increment dst refcount.

I fear that as soon as dst is visible to other cpus, it might be stolen.

> @@ -1350,18 +1360,8 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  
>  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 {
> +		if (!connected)
>  			dst_release(dst);
> -		}
>  		dst = NULL;
>  	}
>  
> 

Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[Alexey Kodanev]

On 03/23/2018 06:50 PM, Eric Dumazet wrote:
> 
> 
> On 03/23/2018 07:39 AM, Alexey Kodanev wrote:
>> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
>> connected datagram sk during pmtu update"), when the error occurs on
>> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
>> error handler can trigger the following path and call ip6_dst_store():
>>
>>     udpv6_err()
>>         ip6_sk_update_pmtu()
>>             ip6_datagram_dst_update()
>>                 ip6_dst_lookup_flow(), can create a RTF_CACHE clone
>>                 ...
>>                 ip6_dst_store()
>>
>> It can happen before a connected UDP socket invokes ip6_dst_store()
>> in the end of udpv6_sendmsg(), on destination release, as a result,
>> the last one changes dst to the old one, preventing getting updated
>> dst cache on the next udpv6_sendmsg() call.
>>
>> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
>> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
>>
> 
> 
> A Fixes: tag would be nice, for automatic tools (and humans as well)

Hi Eric,

I see, will add the commit 33c162a980fe.

...
>>  
>> +	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);
>> +
> 
> What about the MSG_CONFIRM stuff ?
> 
>>  	if (msg->msg_flags&MSG_CONFIRM)
>>  		goto do_confirm;
>>  back_from_confirm:
> 
> Should not you move the above code here instead ?

Ah, you are right, it can release that dst if it go to "do_confirm".

> > Also ip6_dst_store() does not increment dst refcount.
> 
> I fear that as soon as dst is visible to other cpus, it might be stolen.
> 

So we should pass dst_clone(dst) to ip6_dst_store() instead of dst,
because udpv6_err() can release it if it's set the new one.

Then, I guess, we could left udpv6_sendmsg()/ip6_dst_store() where it
is now in the patch and remove the check for "connected" before dst_relase(),
similar to udp_sendmsg(), right?

Thanks,
Alexey

Re: [PATCH net] udp6: set dst cache for a connected sk before udp_v6_send_skb

[Alexey Kodanev]

On 03/23/2018 08:13 PM, Alexey Kodanev wrote:
> On 03/23/2018 06:50 PM, Eric Dumazet wrote:
...
>>> +	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);
>>> +
>>
>> What about the MSG_CONFIRM stuff ?
>>
>>>  	if (msg->msg_flags&MSG_CONFIRM)
>>>  		goto do_confirm;
>>>  back_from_confirm:
>>
>> Should not you move the above code here instead ?
> 
> Ah, you are right, it can release that dst if it go to "do_confirm".
> 
>>> Also ip6_dst_store() does not increment dst refcount.
>>
>> I fear that as soon as dst is visible to other cpus, it might be stolen.
>>
> 
> So we should pass dst_clone(dst) to ip6_dst_store() instead of dst,
> because udpv6_err() can release it if it's set the new one.
> 
> Then, I guess, we could left udpv6_sendmsg()/ip6_dst_store() where it
> is now in the patch and remove the check for "connected" before dst_relase(),
> similar to udp_sendmsg(), right?

And the section "release_dst:" looks redundant after that too.

Thanks,
Alexey