* Re: skb_padto and small fragmented transmits
[not found] <BD9B60A108C4D511AAA10002A50708F20BA2AAE3@orsmsx118.jf.intel.com>
@ 2003-02-06 19:22 ` Chris Leech
2003-02-06 22:43 ` David S. Miller
0 siblings, 1 reply; 8+ messages in thread
From: Chris Leech @ 2003-02-06 19:22 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel
On Thu, 2003-02-06 at 10:44, David S. Miller wrote:
> From: Chris Leech <christopher.leech@intel.com>
> Date: 06 Feb 2003 11:22:51 -0800
>
> I fail to see how the statement "skb->len + skb->data_len" has any
> usable meaning, or how it can be anything other than a bug.
>
> This equation is the standard way to find the full length
> on any skb. For linear skbs, data_len is always zero.
>
> I asked Alan to use this formula so that greps on the source
> tree would always show data_len being taken into account, and
> thus usage would be consistent.
OK, now I'm really getting confused. Every other example I can find in
the networking code, and every scatter-gather capable driver, uses
skb->len as the full length and skb->len - skb->data_len as the length
of the first or linear portion.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
2003-02-06 19:22 ` skb_padto and small fragmented transmits Chris Leech
@ 2003-02-06 22:43 ` David S. Miller
2003-02-07 13:33 ` Alan Cox
0 siblings, 1 reply; 8+ messages in thread
From: David S. Miller @ 2003-02-06 22:43 UTC (permalink / raw)
To: christopher.leech; +Cc: netdev, linux-kernel
From: Chris Leech <christopher.leech@intel.com>
Date: 06 Feb 2003 11:22:08 -0800
OK, now I'm really getting confused. Every other example I can find in
the networking code, and every scatter-gather capable driver, uses
skb->len as the full length and skb->len - skb->data_len as the length
of the first or linear portion.
Indeed, Alan you need to fix the skb_padto stuff to use
skb->len, ignore the skb->data_len as skb->len is the
full length.
Sorry for telling you to do the wrong thing Alan, my bad :)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
2003-02-06 22:43 ` David S. Miller
@ 2003-02-07 13:33 ` Alan Cox
2003-02-08 8:53 ` David S. Miller
0 siblings, 1 reply; 8+ messages in thread
From: Alan Cox @ 2003-02-07 13:33 UTC (permalink / raw)
To: David S. Miller; +Cc: christopher.leech, netdev, Linux Kernel Mailing List
On Thu, 2003-02-06 at 22:43, David S. Miller wrote:
> From: Chris Leech <christopher.leech@intel.com>
> Date: 06 Feb 2003 11:22:08 -0800
>
> OK, now I'm really getting confused. Every other example I can find in
> the networking code, and every scatter-gather capable driver, uses
> skb->len as the full length and skb->len - skb->data_len as the length
> of the first or linear portion.
>
> Indeed, Alan you need to fix the skb_padto stuff to use
> skb->len, ignore the skb->data_len as skb->len is the
> full length.
Dave just fix it next time you touch the code and push it to Marcelo. It
doesnt affect the 2.2 backport so that will be ok
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
2003-02-07 13:33 ` Alan Cox
@ 2003-02-08 8:53 ` David S. Miller
0 siblings, 0 replies; 8+ messages in thread
From: David S. Miller @ 2003-02-08 8:53 UTC (permalink / raw)
To: alan; +Cc: christopher.leech, netdev, linux-kernel
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: 07 Feb 2003 13:33:41 +0000
On Thu, 2003-02-06 at 22:43, David S. Miller wrote:
> Indeed, Alan you need to fix the skb_padto stuff to use
> skb->len, ignore the skb->data_len as skb->len is the
> full length.
Dave just fix it next time you touch the code and push it to Marcelo. It
doesnt affect the 2.2 backport so that will be ok
Ok, I will take care of this.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
[not found] <BD9B60A108C4D511AAA10002A50708F20BA2AAD1@orsmsx118.jf.intel.com>
@ 2003-02-06 19:22 ` Chris Leech
2003-02-06 18:44 ` David S. Miller
0 siblings, 1 reply; 8+ messages in thread
From: Chris Leech @ 2003-02-06 19:22 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel
On Thu, 2003-02-06 at 03:58, David S. Miller wrote:
> skb_padto() only works on linear skb.
The result is always a linear skb. Given that skb_padto() takes into
account data_len (incorrectly, but still) and skb_pad() contains a
comment about non-linear skb always having zero tailroom, it certainly
looks like these were written with the attempt to work for non-linear
buffers.
I fail to see how the statement "skb->len + skb->data_len" has any
usable meaning, or how it can be anything other than a bug.
The checksum issue I mentioned is not as clear. I haven't looked at all
the callers of skb_copy_expand() and copy_skb_header() to see what
effect copying ip_summed in one of those calls might have elsewhere.
> And if you look at all the drivers where it is used, they
> do not enable things like scatter-gather.
So because the problem is not currently exposed, it's acceptable for the
code to be incorrect?
-- Chris
diff -aur a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h 2003-01-13 12:45:20.000000000 -0800
+++ b/include/linux/skbuff.h 2003-02-05 12:25:38.000000000 -0800
@@ -1102,7 +1102,7 @@
static inline struct sk_buff *skb_padto(struct sk_buff *skb, unsigned int len)
{
- unsigned int size = skb->len + skb->data_len;
+ unsigned int size = skb->len;
if (likely(size >= len))
return skb;
return skb_pad(skb, len-size);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
2003-02-06 19:22 ` Chris Leech
@ 2003-02-06 18:44 ` David S. Miller
0 siblings, 0 replies; 8+ messages in thread
From: David S. Miller @ 2003-02-06 18:44 UTC (permalink / raw)
To: christopher.leech; +Cc: netdev, linux-kernel
From: Chris Leech <christopher.leech@intel.com>
Date: 06 Feb 2003 11:22:51 -0800
I fail to see how the statement "skb->len + skb->data_len" has any
usable meaning, or how it can be anything other than a bug.
This equation is the standard way to find the full length
on any skb. For linear skbs, data_len is always zero.
I asked Alan to use this formula so that greps on the source
tree would always show data_len being taken into account, and
thus usage would be consistent.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: skb_padto and small fragmented transmits
2003-02-05 21:39 Chris Leech
@ 2003-02-06 11:58 ` David S. Miller
0 siblings, 0 replies; 8+ messages in thread
From: David S. Miller @ 2003-02-06 11:58 UTC (permalink / raw)
To: Chris Leech; +Cc: netdev, linux-kernel
skb_padto() only works on linear skb.
And if you look at all the drivers where it is used, they
do not enable things like scatter-gather.
^ permalink raw reply [flat|nested] 8+ messages in thread
* skb_padto and small fragmented transmits
@ 2003-02-05 21:39 Chris Leech
2003-02-06 11:58 ` David S. Miller
0 siblings, 1 reply; 8+ messages in thread
From: Chris Leech @ 2003-02-05 21:39 UTC (permalink / raw)
To: netdev, linux-kernel
While looking at the new software padding routines, something caught my
eye in skb_padto. It seemed that the fragmented portion of a packet
would actually be counted twice when checking to see if padding is
needed, as skb->len already includes the count of skb->data_len.
> unsigned int size = skb->len + skb->data_len;
I tested this by modifying e1000 to use skb_padto, disabling TCP
timestamps, and writing a small app to transmit 4 bytes using sendfile.
The resulting packet had 54 bytes of headers, and 4 bytes of data in a
separate fragment. Calling skb_padto(skb,60) should have linearized the
skb, and zeroed out the first 2 bytes of tailroom. Instead the length
was incorrectly calculated as 62 bytes, and the buffer was returned as
is.
Changing skb_padto to simply use size = skb->len fixed the padding, but
then I started seeing incorrect TCP checksums going out. I found this
comment in skb_copy_expand that seemed to explain things.
> BUG ALERT: ip_summed is not copied. Why does this work? Is it used
> only by netfilter in the cases when checksum is recalculated? --ANK
So after calling skb_copy_expand the checksum is not recalculated in
software, but the checksum offload information is discarded.
--
Chris Leech <christopher.leech@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2003-02-08 8:57 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <BD9B60A108C4D511AAA10002A50708F20BA2AAE3@orsmsx118.jf.intel.com>
2003-02-06 19:22 ` skb_padto and small fragmented transmits Chris Leech
2003-02-06 22:43 ` David S. Miller
2003-02-07 13:33 ` Alan Cox
2003-02-08 8:53 ` David S. Miller
[not found] <BD9B60A108C4D511AAA10002A50708F20BA2AAD1@orsmsx118.jf.intel.com>
2003-02-06 19:22 ` Chris Leech
2003-02-06 18:44 ` David S. Miller
2003-02-05 21:39 Chris Leech
2003-02-06 11:58 ` David S. Miller
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).