Fwd: Regarding to your linux kernel CL

Fwd: Regarding to your linux kernel CL

[Chung-Yih Wang (王崇懿)]

Hi,

We have an issue with the CL
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d11a4dc18bf41719c9f0d7ed494d295dd2973b92.
Please see the original mail below. Any comment?

Thanks,
Chung-yih


---------- Forwarded message ----------
From: Chung-Yih Wang (王崇懿) <cywang@google.com>
Date: Mon, Oct 4, 2010 at 6:23 PM
Subject: Regarding to your linux kernel CL
To: timo.teras@iki.fi
Cc: herbert@gondor.apana.org.au, davem@davemloft.net


Hi Timo,

   I encountered an issue with your CL
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d11a4dc18bf41719c9f0d7ed494d295dd2973b92.
The cause is that we use a connected UDP socket for building the
l2tp/ipsec vpn connection. However, when the ipsec tunnel is built,
your CL made the sk_dst_check useless(since it always return the
'freed' dst_entry and can not reset the dst entry for the socket).
What is your comment to conquer this issue?

Solution 1. We could add a CL to change it to (dst && dst->obsolete &&
(dst->obsolete>0  || dst->ops->check(...)==NULL) in sk_dst_check()) ?

Solution 2. Revert the change?

Any comment?

Thanks,
Chung-yih

Re: Regarding to your linux kernel CL

[Timo Teräs]

On 10/05/2010 04:23 AM, Chung-Yih Wang (王崇懿) wrote:
>     I encountered an issue with your CL
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d11a4dc18bf41719c9f0d7ed494d295dd2973b92.
> The cause is that we use a connected UDP socket for building the
> l2tp/ipsec vpn connection. However, when the ipsec tunnel is built,
> your CL made the sk_dst_check useless(since it always return the
> 'freed' dst_entry and can not reset the dst entry for the socket).
> What is your comment to conquer this issue?
> 
> Solution 1. We could add a CL to change it to (dst && dst->obsolete &&
> (dst->obsolete>0  || dst->ops->check(...)==NULL) in sk_dst_check()) ?
> 
> Solution 2. Revert the change?
> 
> Any comment?

What's the problem here? sk_dst_check not honoring if dst->obsolete>0 ?
Sounds like the sk_dst_check was buggy in the first place.

Looks like there's still some code around that does not do what the
obsolete field has been used for a long time.
  obsolete =  0, dst entry is ok
  obsolete = -1, you need to call ops->check for this entry
  obsolete >  0, this entry is invalid

So net/core/sock.c needs fixing. Just if we should change dst_check()
too, I'm not sure.

Should we fix sk_dst_check to use dst_check(), and dst_check() to check
for dst->obsolete>0 ?

Re: Regarding to your linux kernel CL

[Herbert Xu]

On Wed, Oct 06, 2010 at 10:02:56AM +0300, Timo Teräs wrote:
>
> What's the problem here? sk_dst_check not honoring if dst->obsolete>0 ?
> Sounds like the sk_dst_check was buggy in the first place.

Well the problem is that before we changed ip4_dst_check, everything
worked properly.  With IPv6, whenever a route is released, the serial
number is always updated accordingly.  This means that ip6_dst_check
will always return NULL when obsolete > 1.

The old ip4_dst_check also satisfied this requirement since it always
returns NULL.

> Looks like there's still some code around that does not do what the
> obsolete field has been used for a long time.
>   obsolete =  0, dst entry is ok
>   obsolete = -1, you need to call ops->check for this entry
>   obsolete >  0, this entry is invalid
> 
> So net/core/sock.c needs fixing. Just if we should change dst_check()
> too, I'm not sure.
> 
> Should we fix sk_dst_check to use dst_check(), and dst_check() to check
> for dst->obsolete>0 ?

Yes this should work too.  However, I was never totally happy with
this new dst->obsolete logic which means that we're doing an
indirect call for every single packet which almost always does
nothing.

Perhaps we should move the genid/cookie logic into the dst so that
we can eliminate the dst->check call or at least make it a lot less
frequent.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Regarding to your linux kernel CL

[Chung-Yih Wang (王崇懿)]

I have submitted a patch([PATCH] net: Fix sk_dst_check() to reset the
obsolete dst_entry of a socket) for this, please reply to that thread
then.

Thanks,
Chung-yih

On Wed, Oct 6, 2010 at 12:59 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Oct 06, 2010 at 10:02:56AM +0300, Timo Teräs wrote:
>>
>> What's the problem here? sk_dst_check not honoring if dst->obsolete>0 ?
>> Sounds like the sk_dst_check was buggy in the first place.
>
> Well the problem is that before we changed ip4_dst_check, everything
> worked properly.  With IPv6, whenever a route is released, the serial
> number is always updated accordingly.  This means that ip6_dst_check
> will always return NULL when obsolete > 1.
>
> The old ip4_dst_check also satisfied this requirement since it always
> returns NULL.
>
>> Looks like there's still some code around that does not do what the
>> obsolete field has been used for a long time.
>>   obsolete =  0, dst entry is ok
>>   obsolete = -1, you need to call ops->check for this entry
>>   obsolete >  0, this entry is invalid
>>
>> So net/core/sock.c needs fixing. Just if we should change dst_check()
>> too, I'm not sure.
>>
>> Should we fix sk_dst_check to use dst_check(), and dst_check() to check
>> for dst->obsolete>0 ?
>
> Yes this should work too.  However, I was never totally happy with
> this new dst->obsolete logic which means that we're doing an
> indirect call for every single packet which almost always does
> nothing.
>
> Perhaps we should move the genid/cookie logic into the dst so that
> we can eliminate the dst->check call or at least make it a lot less
> frequent.
>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>

Re: Regarding to your linux kernel CL

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 6 Oct 2010 15:59:46 +0800

> Perhaps we should move the genid/cookie logic into the dst so that
> we can eliminate the dst->check call or at least make it a lot less
> frequent.

That sounds like a good idea.

Re: Regarding to your linux kernel CL

[Chung-Yih Wang (王崇懿)]

Do you mean if we should move the genid/cookie logic into sk_dst_check
and remove the dst->ops->check()? If so, I dont think it will be a
good idea to remove 'dst->ops->check' since it is per dst entry's
check not just for ipv4 entry. At least, the fact of obsolete=2
reveals that the entry is freed and is ready for being released. That
will be weird if we still use the obsolete entry for routing the
socket's traffic(for sure, that's why the packets of the socket goes
nowhere but dropped).

On Wed, Oct 6, 2010 at 1:14 AM, David Miller <davem@davemloft.net> wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 6 Oct 2010 15:59:46 +0800
>
>> Perhaps we should move the genid/cookie logic into the dst so that
>> we can eliminate the dst->check call or at least make it a lot less
>> frequent.
>
> That sounds like a good idea.
>