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

[Martin KaFai Lau]

On Tue, Apr 05, 2016 at 07:56:54PM -0400, David Miller wrote:
> From: Cong Wang 
> Date: Mon, 4 Apr 2016 13:45:02 -0700
>
> > On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau  wrote:
> >> One thing to note is that this patch uses the addresses from the sk
> >> instead of iph when updating sk->sk_dst_cache.  It is basically the
> >> same logic that the __ip6_datagram_connect() is doing, so some
> >> refactoring works in the first two patches.
> >>
> >> AFAIK, a UDP socket can become connected after sending out some
> >> datagrams in un-connected state.  or It can be connected
> >> multiple times to different destinations.  I did some quick
> >> tests but I could be wrong.
> >>
> >> I am thinking if there could be a chance that the skb->data, which
> >> has the original outgoing iph, is not related to the current
> >> connected address.  If it is possible, we have to specifically
> >> use the addresses in the sk instead of skb->data (i.e. iph) when
> >> updating the sk->sk_dst_cache.
> >>
> >> If we need to use the sk addresses (and other info) to find out a
> >> new dst for a connected udp socket, it is better not doing it while
> >> the userland is connecting to somewhere else.
> >>
> >> If the above case is impossible, we can keep using the info from iph to
> >> do the dst update for a connected-udp sk without taking the lock.
> >
> > 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 think we are steadily getting closer to a version of this fix that
> we have some agreement on, right?
>
> Martin can you address Cong's feedback and spin another version of this
> series?
Dave, I will spin v2 shortly with some minor changes.

Cong, I believe the loose ends have been tied up after the last email thread
since last Thursday?

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

[Martin KaFai Lau]

On Thu, Apr 07, 2016 at 11:37:10AM -0700, Cong Wang wrote:
> You are lost in discussion
Indeed. :(

>
> 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?
When the pmtu-update is doing route lookup and another connect is
happening, what sk->sk_v6_daddr will this route lookup use?
the old one, new one or neither of them?

Is it acceptable that getsockopt() is returning something that it
is not currently connected to? and potentially somewhere that it
is never connected to?

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

[Cong Wang]

On Wed, Apr 6, 2016 at 11:49 AM, Martin KaFai Lau  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  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?

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

[Martin KaFai Lau]

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  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.

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.

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

[Cong Wang]

On Tue, Apr 5, 2016 at 5:11 PM, Martin KaFai Lau  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. 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.

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

[Martin KaFai Lau]

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?

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

[David Miller]

From: Cong Wang 
Date: Mon, 4 Apr 2016 13:45:02 -0700

> On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau  wrote:
>> One thing to note is that this patch uses the addresses from the sk
>> instead of iph when updating sk->sk_dst_cache.  It is basically the
>> same logic that the __ip6_datagram_connect() is doing, so some
>> refactoring works in the first two patches.
>>
>> AFAIK, a UDP socket can become connected after sending out some
>> datagrams in un-connected state.  or It can be connected
>> multiple times to different destinations.  I did some quick
>> tests but I could be wrong.
>>
>> I am thinking if there could be a chance that the skb->data, which
>> has the original outgoing iph, is not related to the current
>> connected address.  If it is possible, we have to specifically
>> use the addresses in the sk instead of skb->data (i.e. iph) when
>> updating the sk->sk_dst_cache.
>>
>> If we need to use the sk addresses (and other info) to find out a
>> new dst for a connected udp socket, it is better not doing it while
>> the userland is connecting to somewhere else.
>>
>> If the above case is impossible, we can keep using the info from iph to
>> do the dst update for a connected-udp sk without taking the lock.
> 
> 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 think we are steadily getting closer to a version of this fix that
we have some agreement on, right?

Martin can you address Cong's feedback and spin another version of this
series?

Thanks.

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

[Cong Wang]

On Sat, Apr 2, 2016 at 7:33 PM, Martin KaFai Lau  wrote:
> One thing to note is that this patch uses the addresses from the sk
> instead of iph when updating sk->sk_dst_cache.  It is basically the
> same logic that the __ip6_datagram_connect() is doing, so some
> refactoring works in the first two patches.
>
> AFAIK, a UDP socket can become connected after sending out some
> datagrams in un-connected state.  or It can be connected
> multiple times to different destinations.  I did some quick
> tests but I could be wrong.
>
> I am thinking if there could be a chance that the skb->data, which
> has the original outgoing iph, is not related to the current
> connected address.  If it is possible, we have to specifically
> use the addresses in the sk instead of skb->data (i.e. iph) when
> updating the sk->sk_dst_cache.
>
> If we need to use the sk addresses (and other info) to find out a
> new dst for a connected udp socket, it is better not doing it while
> the userland is connecting to somewhere else.
>
> If the above case is impossible, we can keep using the info from iph to
> do the dst update for a connected-udp sk without taking the lock.

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?

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

[Martin KaFai Lau]

On Fri, Apr 01, 2016 at 04:13:41PM -0700, Cong Wang wrote:
> On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau  wrote:
> > +   bh_lock_sock(sk);
> > +   if (!sock_owned_by_user(sk))
> > +   ip6_datagram_dst_update(sk, false);
> > +   bh_unlock_sock(sk);
>
>
> My discussion with Eric shows that we probably don't need to hold
> this sock lock here, and you are Cc'ed in that thread, so
>
> 1) why do you still take the lock here?
> 2) why didn't you involve in our discussion if you disagree?
It is because I agree with the last thread discussion that updating
sk->sk_dst_cache does not need a sk lock.  I also don't see
a lock is need for other operations in that thread.

I am thinking another case that needs a lock, so I start
another RFC thread.  A quick recall for this commit message:
>> It is done under '!sock_owned_by_user(sk)' condition because
>> the user may make another ip6_datagram_connect() while
>> dst lookup and update are happening.
If that could not happen, then the lock is not needed.

One thing to note is that this patch uses the addresses from the sk
instead of iph when updating sk->sk_dst_cache.  It is basically the
same logic that the __ip6_datagram_connect() is doing, so some
refactoring works in the first two patches.

AFAIK, a UDP socket can become connected after sending out some
datagrams in un-connected state.  or It can be connected
multiple times to different destinations.  I did some quick
tests but I could be wrong.

I am thinking if there could be a chance that the skb->data, which
has the original outgoing iph, is not related to the current
connected address.  If it is possible, we have to specifically
use the addresses in the sk instead of skb->data (i.e. iph) when
updating the sk->sk_dst_cache.

If we need to use the sk addresses (and other info) to find out a
new dst for a connected udp socket, it is better not doing it while
the userland is connecting to somewhere else.

If the above case is impossible, we can keep using the info from iph to
do the dst update for a connected-udp sk without taking the lock.

>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index ed44663..f7e6a6d 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -1417,8 +1417,19 @@ EXPORT_SYMBOL_GPL(ip6_update_pmtu);
>>
>>  void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
>>  {
>> +struct dst_entry *dst;
>> +
>>  ip6_update_pmtu(skb, sock_net(sk), mtu,
>>  sk->sk_bound_dev_if, sk->sk_mark);
iph's addresses are used to update the pmtu.  It is fine
because it does not update the sk->sk_dst_cache.

>>> +
>> +dst = __sk_dst_get(sk);
>> +if (!dst || dst->ops->check(dst, inet6_sk(sk)->dst_cookie))
>> +return;
>> +
>> +bh_lock_sock(sk);
>> +if (!sock_owned_by_user(sk))
sk is not connecting to another address.  Find a new dst
for the connected address.
>> +ip6_datagram_dst_update(sk, false);
>> +bh_unlock_sock(sk);
>>  }

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

[Cong Wang]

On Fri, Apr 1, 2016 at 4:13 PM, Cong Wang  wrote:
> On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau  wrote:
>> +   bh_lock_sock(sk);
>> +   if (!sock_owned_by_user(sk))
>> +   ip6_datagram_dst_update(sk, false);
>> +   bh_unlock_sock(sk);
>
>
> My discussion with Eric shows that we probably don't need to hold
> this sock lock here, and you are Cc'ed in that thread, so
>
> 1) why do you still take the lock here?
> 2) why didn't you involve in our discussion if you disagree?

Here is the original thread:
http://comments.gmane.org/gmane.linux.network/405404

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

[Cong Wang]

On Fri, Apr 1, 2016 at 3:56 PM, Martin KaFai Lau  wrote:
> +   bh_lock_sock(sk);
> +   if (!sock_owned_by_user(sk))
> +   ip6_datagram_dst_update(sk, false);
> +   bh_unlock_sock(sk);


My discussion with Eric shows that we probably don't need to hold
this sock lock here, and you are Cc'ed in that thread, so

1) why do you still take the lock here?
2) why didn't you involve in our discussion if you disagree?