| Message ID | 1384383646.28458.138.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Nov 13, 2013 at 3:00 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > We had some reports of crashes using TCP fastopen, and Dave Jones > gave a nice stack trace pointing to the error. > > Issue is that tcp_get_metrics() should not be called with a NULL dst > > Fixes: 1fe4c481ba637 ("net-tcp: Fast Open client - cookie cache") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dave Jones <davej@redhat.com> > Cc: Yuchung Cheng <ycheng@google.com> Acked-by: Yuchung Cheng <ycheng@google.com> > --- > net/ipv4/tcp_metrics.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c > index 2ab09cbae74d..d3ee2e0c28b6 100644 > --- a/net/ipv4/tcp_metrics.c > +++ b/net/ipv4/tcp_metrics.c > @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss, > void tcp_fastopen_cache_set(struct sock *sk, u16 mss, > struct tcp_fastopen_cookie *cookie, bool syn_lost) > { > + struct dst_entry *dst = __sk_dst_get(sk); > struct tcp_metrics_block *tm; > > + if (!dst) > + return; > rcu_read_lock(); > - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true); > + tm = tcp_get_metrics(sk, dst, true); > if (tm) { > struct tcp_fastopen_metrics *tfom = &tm->tcpm_fastopen; > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 13, 2013 at 03:00:46PM -0800, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > We had some reports of crashes using TCP fastopen, and Dave Jones > gave a nice stack trace pointing to the error. > > Issue is that tcp_get_metrics() should not be called with a NULL dst > > Fixes: 1fe4c481ba637 ("net-tcp: Fast Open client - cookie cache") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dave Jones <davej@redhat.com> > Cc: Yuchung Cheng <ycheng@google.com> I let this run overnight, looks good. Tested-by: Dave Jones <davej@fedoraproject.org> thanks Eric. Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-11-13 at 15:00 -0800, Eric Dumazet wrote: > --- a/net/ipv4/tcp_metrics.c > +++ b/net/ipv4/tcp_metrics.c > @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss, > void tcp_fastopen_cache_set(struct sock *sk, u16 mss, > struct tcp_fastopen_cookie *cookie, bool syn_lost) > { > + struct dst_entry *dst = __sk_dst_get(sk); > struct tcp_metrics_block *tm; > > + if (!dst) > + return; > rcu_read_lock(); > - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true); Doesn't that __sk_dst_get() have to go inside the rcu_read_lock()? Then again, I guess we hold the socket. Still looks a bit weird to be moving it out. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-11-14 at 20:13 +0100, Johannes Berg wrote: > On Wed, 2013-11-13 at 15:00 -0800, Eric Dumazet wrote: > > > --- a/net/ipv4/tcp_metrics.c > > +++ b/net/ipv4/tcp_metrics.c > > @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss, > > void tcp_fastopen_cache_set(struct sock *sk, u16 mss, > > struct tcp_fastopen_cookie *cookie, bool syn_lost) > > { > > + struct dst_entry *dst = __sk_dst_get(sk); > > struct tcp_metrics_block *tm; > > > > + if (!dst) > > + return; > > rcu_read_lock(); > > - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true); > > Doesn't that __sk_dst_get() have to go inside the rcu_read_lock()? > > Then again, I guess we hold the socket. Still looks a bit weird to be > moving it out. Yep, in fact this rcu_read_lock() is not needed. I'll send a v2. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-11-14 at 11:36 -0800, Eric Dumazet wrote: > On Thu, 2013-11-14 at 20:13 +0100, Johannes Berg wrote: > > On Wed, 2013-11-13 at 15:00 -0800, Eric Dumazet wrote: > > > > > --- a/net/ipv4/tcp_metrics.c > > > +++ b/net/ipv4/tcp_metrics.c > > > @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss, > > > void tcp_fastopen_cache_set(struct sock *sk, u16 mss, > > > struct tcp_fastopen_cookie *cookie, bool syn_lost) > > > { > > > + struct dst_entry *dst = __sk_dst_get(sk); > > > struct tcp_metrics_block *tm; > > > > > > + if (!dst) > > > + return; > > > rcu_read_lock(); > > > - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true); > > > > Doesn't that __sk_dst_get() have to go inside the rcu_read_lock()? > > > > Then again, I guess we hold the socket. Still looks a bit weird to be > > moving it out. > > Yep, in fact this rcu_read_lock() is not needed. I'll send a v2. I take it back. the rcu_read_lock() protects the tcp_get_metrics(), not the __sk_dst_get(sk) So the patch is correct, unless you disagree of course ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-11-14 at 11:38 -0800, Eric Dumazet wrote: > > > Doesn't that __sk_dst_get() have to go inside the rcu_read_lock()? > > > > > > Then again, I guess we hold the socket. Still looks a bit weird to be > > > moving it out. > > > > Yep, in fact this rcu_read_lock() is not needed. I'll send a v2. > > I take it back. > > the rcu_read_lock() protects the tcp_get_metrics(), not the > __sk_dst_get(sk) > > So the patch is correct, unless you disagree of course ;) Heh. I have no idea, it just seemed a little odd on first look given that __sk_dst_get() *can* actually use RCU protection. :) johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-11-14 at 21:53 +0100, Johannes Berg wrote: > Heh. I have no idea, it just seemed a little odd on first look given > that __sk_dst_get() *can* actually use RCU protection. :) Yep, the deal is that if you own socket lock, you do not need rcu_read_lock() Look for dst_negative_advice() as another example : We call __sk_dst_get(sk) without rcu_read_lock() protection. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 13 Nov 2013 15:00:46 -0800 > From: Eric Dumazet <edumazet@google.com> > > We had some reports of crashes using TCP fastopen, and Dave Jones > gave a nice stack trace pointing to the error. > > Issue is that tcp_get_metrics() should not be called with a NULL dst > > Fixes: 1fe4c481ba637 ("net-tcp: Fast Open client - cookie cache") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dave Jones <davej@redhat.com> Applied and queued up for -stable, thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c index 2ab09cbae74d..d3ee2e0c28b6 100644 --- a/net/ipv4/tcp_metrics.c +++ b/net/ipv4/tcp_metrics.c @@ -663,10 +663,13 @@ void tcp_fastopen_cache_get(struct sock *sk, u16 *mss, void tcp_fastopen_cache_set(struct sock *sk, u16 mss, struct tcp_fastopen_cookie *cookie, bool syn_lost) { + struct dst_entry *dst = __sk_dst_get(sk); struct tcp_metrics_block *tm; + if (!dst) + return; rcu_read_lock(); - tm = tcp_get_metrics(sk, __sk_dst_get(sk), true); + tm = tcp_get_metrics(sk, dst, true); if (tm) { struct tcp_fastopen_metrics *tfom = &tm->tcpm_fastopen;