[TCP]: Fix truesize underflow

[TCP]: Fix truesize underflow

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Hi Dave:

You're absolutely right about there being a problem with the TSO packet
trimming code.  The cause of this lies in the tcp_fragment() function.

When we allocate a fragment for a completely non-linear packet the
truesize is calculated for a payload length of zero.  This means that
truesize could in fact be less than the real payload length.

When that happens the TSO packet trimming can cause truesize to become
negative.  This in turn can cause sk_forward_alloc to be -n * PAGE_SIZE
which would trigger the warning.

I've copied the code you used in tso_fragment which should work here.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Everyone who's having the sk_forward_alloc warning problem should give
this patch a go to see if it cures things.

Just in case this still doesn't fix it, could everyone please also verify
whether disabling SMP has any effect on reproducing this?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: tcp-fragment.patch --]
[-- Type: text/plain, Size: 484 bytes --]

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index b871db6..44df1db 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -551,7 +551,9 @@
 	buff = sk_stream_alloc_skb(sk, nsize, GFP_ATOMIC);
 	if (buff == NULL)
 		return -ENOMEM; /* We'll just try again later. */
-	sk_charge_skb(sk, buff);
+
+	buff->truesize = skb->len - len;
+	skb->truesize -= buff->truesize;
 
 	/* Correct the sequence numbers. */
 	TCP_SKB_CB(buff)->seq = TCP_SKB_CB(skb)->seq + len;

Re: [TCP]: Fix truesize underflow

[Ingo Oeser]

Hi Herbert,

Herbert Xu wrote:
> I've copied the code you used in tso_fragment which should work here.

I'm happy to see, that this got resolved and this is a nice minimalistic fix 
for -stable. 

But shouldn't we put this kind of hairy manipulation into some nice functions?
Driver writers were already confused by all that size, len and truesize stuff, 
as this bug showed.


Regards

Ingo Oeser

Re: [TCP]: Fix truesize underflow

[David S. Miller]

From: Ingo Oeser <netdev@axxeo.de>
Date: Tue, 18 Apr 2006 18:29:59 +0200

> But shouldn't we put this kind of hairy manipulation into some nice
> functions?  Driver writers were already confused by all that size,
> len and truesize stuff, as this bug showed.

It's 2 lines and frankly it's a bit context dependant.  I think
Herbert's fix is fine for now.

In the long term, yes, a lot of these weird manipulations need to
be consolidated in well defined functions to make maintainence
and auditing easier.

Re: [TCP]: Fix truesize underflow

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 18 Apr 2006 22:32:04 +1000

> You're absolutely right about there being a problem with the TSO packet
> trimming code.  The cause of this lies in the tcp_fragment() function.
> 
> When we allocate a fragment for a completely non-linear packet the
> truesize is calculated for a payload length of zero.  This means that
> truesize could in fact be less than the real payload length.
> 
> When that happens the TSO packet trimming can cause truesize to become
> negative.  This in turn can cause sk_forward_alloc to be -n * PAGE_SIZE
> which would trigger the warning.
> 
> I've copied the code you used in tso_fragment which should work here.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks for discovering this, very nice work Herbert.

So what we find out time and time again, is that the TSO splitting and
trimming code enforces that the skb->truesize of every TCP packet must
be accurate at all times.

I think it is deserving of some run time assertions, else these bugs
will elude us continually.  Luckily there are only a few places that
would need the run time assertion checks on skb->truesize, and I'll
try to spend a few cycles on implementing this soon.

Patch applied, thanks a lot!

Re: [TCP]: Fix truesize underflow

[Herbert Xu]

On Tue, Apr 18, 2006 at 01:22:56PM -0700, David S. Miller wrote:
> 
> I think it is deserving of some run time assertions, else these bugs
> will elude us continually.  Luckily there are only a few places that
> would need the run time assertion checks on skb->truesize, and I'll
> try to spend a few cycles on implementing this soon.

Yes indeed.  One place that comes to mind would be tcp_trim_head just
before we munge truesize.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [TCP]: Fix truesize underflow

[Boris B. Zhmurov]

Hello, Herbert Xu.

On 19.04.2006 03:27 you said the following:

> On Tue, Apr 18, 2006 at 01:22:56PM -0700, David S. Miller wrote:
> 
>>I think it is deserving of some run time assertions, else these bugs
>>will elude us continually.  Luckily there are only a few places that
>>would need the run time assertion checks on skb->truesize, and I'll
>>try to spend a few cycles on implementing this soon.
> 
> 
> Yes indeed.  One place that comes to mind would be tcp_trim_head just
> before we munge truesize.
> 
> Cheers,


I confirm, finally I don't see messages in dmesg about assertions. Nice 
work :)

-- 
Boris B. Zhmurov
mailto: bb@kernelpanic.ru
"wget http://kernelpanic.ru/bb_public_key.pgp -O - | gpg --import"

Re: [TCP]: Fix truesize underflow

[Herbert Xu]

On Wed, Apr 19, 2006 at 10:04:53AM +0400, Boris B. Zhmurov wrote:
> 
> I confirm, finally I don't see messages in dmesg about assertions. Nice 
> work :)

That's great.  Thanks a lot for your and everyone else's help in
tracking down.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [TCP]: Fix truesize underflow

[Jesse Brandeburg]

Boris B. Zhmurov wrote:
> Hello, Herbert Xu.
>
> On 19.04.2006 03:27 you said the following:
>
>> On Tue, Apr 18, 2006 at 01:22:56PM -0700, David S. Miller wrote:
>>
>>> I think it is deserving of some run time assertions, else these bugs
>>> will elude us continually.  Luckily there are only a few places that
>>> would need the run time assertion checks on skb->truesize, and I'll
>>> try to spend a few cycles on implementing this soon.
>>
>>
>> Yes indeed.  One place that comes to mind would be tcp_trim_head just
>> before we munge truesize.
>>
>> Cheers,
>
>
> I confirm, finally I don't see messages in dmesg about assertions. 
> Nice work :)
>
I can also confirm that both machines here had no problems after 
applying this patch. Good work!

Re: [TCP]: Fix truesize underflow

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 938 bytes --]



On Wed, 19 Apr 2006, Jesse Brandeburg wrote:

> Boris B. Zhmurov wrote:
>> Hello, Herbert Xu.
>> 
>> On 19.04.2006 03:27 you said the following:
>> 
>>> On Tue, Apr 18, 2006 at 01:22:56PM -0700, David S. Miller wrote:
>>> 
>>>> I think it is deserving of some run time assertions, else these bugs
>>>> will elude us continually.  Luckily there are only a few places that
>>>> would need the run time assertion checks on skb->truesize, and I'll
>>>> try to spend a few cycles on implementing this soon.
>>> 
>>> 
>>> Yes indeed.  One place that comes to mind would be tcp_trim_head just
>>> before we munge truesize.
>>> 
>>> Cheers,
>> 
>> 
>> I confirm, finally I don't see messages in dmesg about assertions. Nice 
>> work :)
>> 
> I can also confirm that both machines here had no problems after applying 
> this patch. Good work!

Me too, me too. Thank you! :)

Best regards,

 				Krzysztof Olędzki

Re: [TCP]: Fix truesize underflow

[Stephen Hemminger]

On Tue, 18 Apr 2006 22:32:04 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi Dave:
> 
> You're absolutely right about there being a problem with the TSO packet
> trimming code.  The cause of this lies in the tcp_fragment() function.
> 
> When we allocate a fragment for a completely non-linear packet the
> truesize is calculated for a payload length of zero.  This means that
> truesize could in fact be less than the real payload length.
> 
> When that happens the TSO packet trimming can cause truesize to become
> negative.  This in turn can cause sk_forward_alloc to be -n * PAGE_SIZE
> which would trigger the warning.
> 
> I've copied the code you used in tso_fragment which should work here.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Everyone who's having the sk_forward_alloc warning problem should give
> this patch a go to see if it cures things.
> 
> Just in case this still doesn't fix it, could everyone please also verify
> whether disabling SMP has any effect on reproducing this?
> 
> Thanks,

Please put this in the next -stable load...

Re: [TCP]: Fix truesize underflow

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Wed, 19 Apr 2006 09:53:48 -0700

> Please put this in the next -stable load...

I already sent it to -stable.