New: alignment of rx_skb in r8169 driver

Bugme-new Andrew Morton 2 participants 2 messages Nov 25, 2009 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author

2009-11-25 21:55 UTC #1 lore raw

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 34 lines · On Wed, 25 Nov 2009 15:01:16 GMT [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=14688
           Summary: alignment of rx_skb in r8169 driver
           Product: Drivers
           Version: 2.5
    Kernel Version: 2.6.29
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Network
        AssignedTo: [email protected]
        ReportedBy: [email protected]
        Regression: No
Created an attachment (id=23933)
--> (http://bugzilla.kernel.org/attachment.cgi?id=23933)
patch for rtl8169_alloc_rx_skb
Dear Maintainers,
I think there is a bug in the rtl8169_alloc_rx_skb function of the RealTek 8169/8168/8101 ethernet driver in kernel version 2.6.29.6, but also in current 2.6.31 versions.
For the case the align argument is different from 0, the skb_reserve call is expected to align the data field accordingly by padding. The number of bytes to reserve is calculated by the expression
            (align - 1) & (unsigned long)skb->data

Quoted 8 lines

which is wrong. I would suggest the following correction:
pad = align ? align - ((align - 1) & (unsigned long)skb->data) : NET_IP_ALIGN; if (pad == align) pad = 0; // optional
skb_reserve(skb, pad);
A patch for r8169.c is provided as attachment.

Thanks, but..

- please always submit patches via email, not via bugzilla. Documentation/SubmittingPatches goes into some detail.

- prepare patches as a unified diff, in `patch -p1' form. It should look like:

drivers/net/r8169.c renamed from drivers/net/r8169.c~a +2 -0

@@ -207,7 +207,9 @@ enum rtl_registers {
 	ChipCmd		= 0x37,
 	TxPoll		= 0x38,
 	IntrMask	= 0x3c,
+wibble
 	IntrStatus	= 0x3e,
+wobble
 	TxConfig	= 0x40,
 	RxConfig	= 0x44,
 	RxMissed	= 0x4c,

- Please cc at least the following on the patch:

Francois Romieu <[email protected]>
"David S. Miller" <[email protected]>
[email protected]

- Don't forget the changelog and a Singed-off-by:!

David Miller

maintainer

2009-11-25 23:45 UTC #2 lore raw

From: Andrew Morton <[email protected]>
Date: Wed, 25 Nov 2009 13:55:07 -0800

Quoted 18 lines

Quoted 10 lines
            (align - 1) & (unsigned long)skb->data
which is wrong. I would suggest the following correction:
pad = align ? align - ((align - 1) & (unsigned long)skb->data) : NET_IP_ALIGN; if (pad == align) pad = 0; // optional
skb_reserve(skb, pad);
A patch for r8169.c is provided as attachment.
Thanks, but..
- please always submit patches via email, not via bugzilla. Documentation/SubmittingPatches goes into some detail.
- prepare patches as a unified diff, in `patch -p1' form. It should look like:

...

Quoted 11 lines

- Please cc at least the following on the patch:
Francois Romieu <[email protected]>
"David S. Miller" <[email protected]>
[email protected]
- Don't forget the changelog and a Singed-off-by:!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Also a better, more canonical, expression to use to fix this would be:

	align - ((align - 1) & (unsigned long)skb->data)

or similar.