netdev - Re: [PATCH net v2] udp6: set dst cache for a connected sk before udp_v6_send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Mar 2018 16:27:30 +0300
From:   Alexey Kodanev <alexey.kodanev@...cle.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net v2] udp6: set dst cache for a connected sk before
 udp_v6_send_skb

On 26.03.2018 20:02, Martin KaFai Lau wrote:
> On Mon, Mar 26, 2018 at 05:48:47PM +0300, 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
> Instead of ip6_dst_lookup_flow(),
> you meant the RTF_CACHE route created in ip6_update_pmtu()
> 

Right, or even earlier... I was using vti tunnel and it invokes
skb_dst_update_pmtu() on this error, then sends ICMPv6_PKT_TOOBIG.

>>                 ...
>>                 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().
> After this patch, the above udpv6_err() path could not happen after
> ip6_sk_dst_lookup_flow() and before the ip6_dst_store() in udpv6_sendmsg()?
>

May be we could minimize this if save it in ip6_sk_dst_lookup_flow()
for a connected UDP sockets only if we're not getting it from a cache
for some reason?

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a8a9195..0204f52 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1115,13 +1115,30 @@ struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
  *     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_dst_store(sk, dst_clone(dst), ...);

Thanks,
Alexey
 
>>
>> Also, increase refcnt for dst, when passing it to ip6_dst_store()
>> because after that the dst cache can be released by other calls
>> to ip6_dst_store() with the same socket.
>>
>> Fixes: 33c162a980fe ("ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update")
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@...cle.com>
>> ---
>>
>> v2: * remove 'release_dst:' label
>>
>>     * move ip6_dst_store() below MSG_CONFIRM check as
>>       suggested by Eric and add dst_clone()
>>
>>     * add 'Fixes' commit.
>>
>>
>>  net/ipv6/udp.c | 29 +++++++++++------------------
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 52e3ea0..4508e5a 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1303,6 +1303,16 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>  		goto do_confirm;
>>  back_from_confirm:
>>  
>> +	if (connected)>> +		ip6_dst_store(sk, dst_clone(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);
>> +
>>  	/* Lockless fast path for the non-corking case */
>>  	if (!corkreq) {
>>  		struct sk_buff *skb;
>> @@ -1314,7 +1324,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 +1358,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
>>