* Fwd: Regarding to your linux kernel CL [not found] <AANLkTikdcL0JQgkR6u0qtmDu-phMZ6-Juq91B1N5GfiY@mail.gmail.com> @ 2010-10-05 16:42 ` Chung-Yih Wang (王崇懿) 2010-10-06 7:02 ` Timo Teräs 1 sibling, 0 replies; 6+ messages in thread From: Chung-Yih Wang (王崇懿) @ 2010-10-05 16:42 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding to your linux kernel CL [not found] <AANLkTikdcL0JQgkR6u0qtmDu-phMZ6-Juq91B1N5GfiY@mail.gmail.com> 2010-10-05 16:42 ` Fwd: Regarding to your linux kernel CL Chung-Yih Wang (王崇懿) @ 2010-10-06 7:02 ` Timo Teräs 2010-10-06 7:59 ` Herbert Xu 1 sibling, 1 reply; 6+ messages in thread From: Timo Teräs @ 2010-10-06 7:02 UTC (permalink / raw) To: "Chung-Yih Wang (王崇懿)" Cc: herbert, davem, netdev 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 ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding to your linux kernel CL 2010-10-06 7:02 ` Timo Teräs @ 2010-10-06 7:59 ` Herbert Xu 2010-10-06 8:04 ` Chung-Yih Wang (王崇懿) 2010-10-06 8:14 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Herbert Xu @ 2010-10-06 7:59 UTC (permalink / raw) To: Timo Teräs Cc: "Chung-Yih Wang (王崇懿)", davem, netdev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding to your linux kernel CL 2010-10-06 7:59 ` Herbert Xu @ 2010-10-06 8:04 ` Chung-Yih Wang (王崇懿) 2010-10-06 8:14 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: Chung-Yih Wang (王崇懿) @ 2010-10-06 8:04 UTC (permalink / raw) To: Herbert Xu; +Cc: Timo Teräs, davem, netdev 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding to your linux kernel CL 2010-10-06 7:59 ` Herbert Xu 2010-10-06 8:04 ` Chung-Yih Wang (王崇懿) @ 2010-10-06 8:14 ` David Miller 2010-10-06 21:23 ` Chung-Yih Wang (王崇懿) 1 sibling, 1 reply; 6+ messages in thread From: David Miller @ 2010-10-06 8:14 UTC (permalink / raw) To: herbert; +Cc: timo.teras, cywang, netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regarding to your linux kernel CL 2010-10-06 8:14 ` David Miller @ 2010-10-06 21:23 ` Chung-Yih Wang (王崇懿) 0 siblings, 0 replies; 6+ messages in thread From: Chung-Yih Wang (王崇懿) @ 2010-10-06 21:23 UTC (permalink / raw) To: David Miller; +Cc: herbert, timo.teras, netdev 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-06 21:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <AANLkTikdcL0JQgkR6u0qtmDu-phMZ6-Juq91B1N5GfiY@mail.gmail.com>
2010-10-05 16:42 ` Fwd: Regarding to your linux kernel CL Chung-Yih Wang (王崇懿)
2010-10-06 7:02 ` Timo Teräs
2010-10-06 7:59 ` Herbert Xu
2010-10-06 8:04 ` Chung-Yih Wang (王崇懿)
2010-10-06 8:14 ` David Miller
2010-10-06 21:23 ` Chung-Yih Wang (王崇懿)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).