Patchwork : tcp: Fix queue traversal in tcp_use_frto().

login
register
mail settings
Submitter David Miller
Date Sept. 23, 2008, 9:55 a.m.
Message ID <20080923.025542.193703353.davem@davemloft.net>
Download mbox | patch
Permalink /patch/1045/
State Accepted
Delegated to: David Miller
Headers show

Comments

David Miller - Sept. 23, 2008, 9:55 a.m.
One more skb_queue_next() BUG trigger.  I double audited the
remaining tcp_write_queue_next() cases and they should all
be good.

tcp: Fix queue traversal in tcp_use_frto().

We must check tcp_skb_is_last() before doing a tcp_write_queue_next().

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp_input.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Ilpo Järvinen - Sept. 23, 2008, 7:18 p.m.
On Tue, 23 Sep 2008, David Miller wrote:

> 
> One more skb_queue_next() BUG trigger.  I double audited the
> remaining tcp_write_queue_next() cases and they should all
> be good.
> 
> tcp: Fix queue traversal in tcp_use_frto().
> 
> We must check tcp_skb_is_last() before doing a tcp_write_queue_next().
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/ipv4/tcp_input.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index cbfe13d..3b76bce 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1746,6 +1746,8 @@ int tcp_use_frto(struct sock *sk)
>  		return 0;
>  
>  	skb = tcp_write_queue_head(sk);
> +	if (tcp_skb_is_last(sk, skb))
> +		return 1;
>  	skb = tcp_write_queue_next(sk, skb);	/* Skips head */
>  	tcp_for_write_queue_from(skb, sk) {
>  		if (skb == tcp_send_head(sk))
> 

For the record (as you probably know it already), this wasn't actually
a real bug though it hits your BUG_ON since tcp_for_write_queue_from exits
immediately, no garbage gets dereferenced.
David Miller - Sept. 23, 2008, 10 p.m.
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 23 Sep 2008 22:18:33 +0300 (EEST)

> For the record (as you probably know it already), this wasn't actually
> a real bug though it hits your BUG_ON since tcp_for_write_queue_from exits
> immediately, no garbage gets dereferenced.

Yes, that is correct.

It is actually disappointing for me, I thought maybe it would catch
a real bug somewhere :-)
--
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
Ilpo Järvinen - Sept. 24, 2008, 10:08 a.m.
On Tue, 23 Sep 2008, David Miller wrote:

> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Tue, 23 Sep 2008 22:18:33 +0300 (EEST)
> 
> > For the record (as you probably know it already), this wasn't actually
> > a real bug though it hits your BUG_ON since tcp_for_write_queue_from exits
> > immediately, no garbage gets dereferenced.
> 
> Yes, that is correct.
>
> It is actually disappointing for me, I thought maybe it would catch
> a real bug somewhere :-)

Rest assured, it will definately do that once some rewrite or so 
happens... :-)

Patch

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index cbfe13d..3b76bce 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1746,6 +1746,8 @@  int tcp_use_frto(struct sock *sk)
 		return 0;
 
 	skb = tcp_write_queue_head(sk);
+	if (tcp_skb_is_last(sk, skb))
+		return 1;
 	skb = tcp_write_queue_next(sk, skb);	/* Skips head */
 	tcp_for_write_queue_from(skb, sk) {
 		if (skb == tcp_send_head(sk))