netdev - Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update

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:	Thu, 7 Apr 2016 11:37:10 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Martin KaFai Lau <kafai@...com>
Cc:	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Wei Wang <weiwan@...gle.com>, Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH net 3/4] ipv6: datagram: Update dst cache of a
 connected datagram sk during pmtu update

On Wed, Apr 6, 2016 at 11:49 AM, Martin KaFai Lau <kafai@...com> wrote:
> On Wed, Apr 06, 2016 at 10:58:23AM -0700, Cong Wang wrote:
>> On Tue, Apr 5, 2016 at 5:11 PM, Martin KaFai Lau <kafai@...com> wrote:
>> > On Mon, Apr 04, 2016 at 01:45:02PM -0700, Cong Wang wrote:
>> >> I see your point, but calling __ip6_datagram_connect() seems overkill
>> >> here, we don't need to update so many things in the pmtu update context,
>> >> at least IPv4 doesn't do that either. I don't think you have to do that.
>> >>
>> >> So why just updating the dst cache (also some addr cache) here is not
>> >> enough?
>> > I am not sure I understand.  I could be missing something.
>> >
>> > This patch uses ip6_datagram_dst_update() to do the route lookup and
>> > sk->sk_dst_cache update.  ip6_datagram_dst_update() is
>> > created in the first two refactoring patches and is also used by
>> > __ip6_datagram_connect().
>> >
>> > Which operations in ip6_datagram_dst_update() could be saved
>> > during the pmtu update?
>>
>> I thought you call the same ip6_datagram_dst_update() for both
>> pmtu update and __ip6_datagram_connect(), but you actually skip
>> some sk operations for pmtu case, which means you don't need
>> to worry about parallel ip6_datagram_connect().
>>
>> IPv6 UDP sendmsg() path stores the dst without sock lock anyway,
>> we don't cope with a concurrent connect() on another cpu.
> A parallel sendmsg and connect could be an issue.  The user is connecting
> to a new dest while another parallel sendmsg is sending to (could be the old
> dest, new dest or somewhere between old and new dest?)
>
> However, it is the userland making and it will be another patch if we want
> to protect this case too.

Yeah, it is a different problem, but no one complains about it yet.

>
> In pmtu update, the kernel is doing the lookup and update without the
> userland conscious.
>
>> But still, I don't see this is a problem here, because even if we store
>> an obsolete address in cache, it would be corrected later.
> The sendmsg() path will correct it (relookup and update sk_dst_cache) but not
> the getsockopt(IPV6_MTU) path which is what this patch is trying to fix: Update
> a _valid_ dst to sk->sk_dst_cache.

You are lost in discussion, I never object to update sk_dst_cache, what
we disagree here is merely if we need to lock the sock in pmtu update
context.

I still think it is okay without the lock, because even if you take the lock,
the pmtu update could still happen after you release it, so there is no
essential difference here. The only reason I can think of for taking
the sock lock is protecting parallel pmtu update, but it looks safe for
this case too.

So which case do you want to protect by taking the sock lock?