netdev - Re: [RFT Patch net 1/2] ipv6: invalidate the socket cached route on pmtu events if possible

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:	Fri, 25 Mar 2016 10:17:38 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Wei Wang <weiwan@...gle.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Martin KaFai Lau <kafai@...com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Julian Anastasov <ja@....bg>
Subject: Re: [RFT Patch net 1/2] ipv6: invalidate the socket cached route on
 pmtu events if possible

On Thu, Mar 24, 2016 at 6:51 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Thu, 2016-03-24 at 17:15 -0700, Cong Wang wrote:
>
>> My understanding is that bh_lock_sock() prevents concurrent
>> access to sock struct. Since this is in softirq context, multiple
>> CPU's could call this function concurrently, the whole pmtu
>> update needs to be done atomically.
>>
>> UDP, on the other hand, doesn't do this logic, it just looks up
>> for dst and save it in sk_dst_cache.
>
> Two ICMP messages processed on two different cpus can still get two
> different sockets pointing to the same dst.
>

Yeah, and dst is refcounted so a shared dst will not be freed by both.

> I do not see how dst pmtu update could be protected by a lock on each
> socket. It would require a dst lock , or some simple memory writes that
> do not require special synchro.

I am lost here because you removed the dst lock in ipv4_sk_update_pmtu():

commit 7f502361531e9eecb396cf99bdc9e9a59f7ebd7f
Author: Eric Dumazet <edumazet@...gle.com>
Date:   Mon Jun 30 01:26:23 2014 -0700

    ipv4: irq safe sk_dst_[re]set() and ipv4_sk_update_pmtu() fix

    We have two different ways to handle changes to sk->sk_dst

    First way (used by TCP) assumes socket lock is owned by caller, and use
    no extra lock : __sk_dst_set() & __sk_dst_reset()

    Another way (used by UDP) uses sk_dst_lock because socket lock is not
    always taken. Note that sk_dst_lock is not softirq safe.

    These ways are not inter changeable for a given socket type.

    ipv4_sk_update_pmtu(), added in linux-3.8, added a race, as it used
    the socket lock as synchronization, but users might be UDP sockets.

    Instead of converting sk_dst_lock to a softirq safe version, use xchg()
    as we did for sk_rx_dst in commit e47eb5dfb296b ("udp: ipv4: do not use
    sk_dst_lock from softirq context")

    In a follow up patch, we probably can remove sk_dst_lock, as it is
    only used in IPv6.

My understand is that:

1) sock lock protects the whole update: the whole check, update, recheck,
set logic, to make sure another CPU will not do the same to the same socket
at the same time.

2) the dst itself is safe, because it is always refcounted, and we use xchg()
to change the pointer in sk_dst_cache.

Or am I still missing anything here?

Thanks.