[PATCH] net: asix: fix bad header length bug

[PATCH] net: asix: fix bad header length bug

[Emil Goode]

The AX88772B occasionally send rx packets that cross urb boundaries
and the remaining partial packet is sent with no header.
When the buffer with a partial packet is of less number of octets
than the value of hard_header_len the buffer is discarded by the
usbnet module. This is causing dropped packages and error messages
in dmesg.

This can be reproduced by using ping with a packet size
between 1965-1976.

The bug has been reported here:

https://bugzilla.kernel.org/show_bug.cgi?id=29082

Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
 drivers/net/usb/asix_devices.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 9765a7d..120bb29 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -455,6 +455,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->net->ethtool_ops = &ax88772_ethtool_ops;
 	dev->net->needed_headroom = 4; /* cf asix_tx_fixup() */
 	dev->net->needed_tailroom = 4; /* cf asix_tx_fixup() */
+	dev->net->hard_header_len = 0; /* Partial packets have no header */
 
 	embd_phy = ((dev->mii.phy_id & 0x1f) == 0x10 ? 1 : 0);
 
-- 
1.7.10.4

Re: [PATCH] net: asix: fix bad header length bug

[Igor Gnatenko]

On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote: 
> The AX88772B occasionally send rx packets that cross urb boundaries
> and the remaining partial packet is sent with no header.
> When the buffer with a partial packet is of less number of octets
> than the value of hard_header_len the buffer is discarded by the
> usbnet module. This is causing dropped packages and error messages
> in dmesg.
> 
> This can be reproduced by using ping with a packet size
> between 1965-1976.
> 
> The bug has been reported here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=29082
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>
Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com> 
> ---
>  drivers/net/usb/asix_devices.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 9765a7d..120bb29 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -455,6 +455,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->net->ethtool_ops = &ax88772_ethtool_ops;
>  	dev->net->needed_headroom = 4; /* cf asix_tx_fixup() */
>  	dev->net->needed_tailroom = 4; /* cf asix_tx_fixup() */
> +	dev->net->hard_header_len = 0; /* Partial packets have no header */
>  
>  	embd_phy = ((dev->mii.phy_id & 0x1f) == 0x10 ? 1 : 0);
>  


-- 
-Igor Gnatenko

RE: [PATCH] net: asix: fix bad header length bug

[David Laight]

From: Igor Gnatenko
> On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote:
> > The AX88772B occasionally send rx packets that cross urb boundaries
> > and the remaining partial packet is sent with no header.
> > When the buffer with a partial packet is of less number of octets
> > than the value of hard_header_len the buffer is discarded by the
> > usbnet module. This is causing dropped packages and error messages
> > in dmesg.
> >
> > This can be reproduced by using ping with a packet size
> > between 1965-1976.
> >
> > The bug has been reported here:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=29082
> >
> > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> > ---
> >  drivers/net/usb/asix_devices.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > index 9765a7d..120bb29 100644
> > --- a/drivers/net/usb/asix_devices.c
> > +++ b/drivers/net/usb/asix_devices.c
> > @@ -455,6 +455,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> >  	dev->net->ethtool_ops = &ax88772_ethtool_ops;
> >  	dev->net->needed_headroom = 4; /* cf asix_tx_fixup() */
> >  	dev->net->needed_tailroom = 4; /* cf asix_tx_fixup() */
> > +	dev->net->hard_header_len = 0; /* Partial packets have no header */

That is the wrong place for the fix.

It should only be done when rx_urb_size is set to a multiple of the usb
packet size.
That is only done for some of the supported devices.

In fact, if the rx_urb_size is a multiple of the usb frame size (or 1k)
then maybe the usbnet code should assume that the driver is capable
of processing ethernet frames that cross usb packet boundaries and
not delete short packets at all - regardless of the hard_header_len.

	David

Re: [PATCH] net: asix: fix bad header length bug

[Emil Goode]

On Thu, Feb 06, 2014 at 03:28:13PM +0000, David Laight wrote:
> From: Igor Gnatenko
> > On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote:
> > > The AX88772B occasionally send rx packets that cross urb boundaries
> > > and the remaining partial packet is sent with no header.
> > > When the buffer with a partial packet is of less number of octets
> > > than the value of hard_header_len the buffer is discarded by the
> > > usbnet module. This is causing dropped packages and error messages
> > > in dmesg.
> > >
> > > This can be reproduced by using ping with a packet size
> > > between 1965-1976.
> > >
> > > The bug has been reported here:
> > >
> > > https://bugzilla.kernel.org/show_bug.cgi?id=29082
> > >
> > > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > Reported-and-tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> > > ---
> > >  drivers/net/usb/asix_devices.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > > index 9765a7d..120bb29 100644
> > > --- a/drivers/net/usb/asix_devices.c
> > > +++ b/drivers/net/usb/asix_devices.c
> > > @@ -455,6 +455,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
> > >  	dev->net->ethtool_ops = &ax88772_ethtool_ops;
> > >  	dev->net->needed_headroom = 4; /* cf asix_tx_fixup() */
> > >  	dev->net->needed_tailroom = 4; /* cf asix_tx_fixup() */
> > > +	dev->net->hard_header_len = 0; /* Partial packets have no header */
> 
> That is the wrong place for the fix.
> 
> It should only be done when rx_urb_size is set to a multiple of the usb
> packet size.
> That is only done for some of the supported devices.
> 
> In fact, if the rx_urb_size is a multiple of the usb frame size (or 1k)
> then maybe the usbnet code should assume that the driver is capable
> of processing ethernet frames that cross usb packet boundaries and
> not delete short packets at all - regardless of the hard_header_len.
> 
> 	David
>

I will do some more digging in the code, but the test of skb->len
against hard_header_len is done already in the completion callback
function passed to usb_fill_bulk_urb so it seems that buffers of less
than hard_header_len number of octets will be dropped regardless.

Best regards,

Emil Goode

Re: [PATCH] net: asix: fix bad header length bug

[Bjørn Mork]

Emil Goode <emilgoode@gmail.com> writes:
> On Thu, Feb 06, 2014 at 03:28:13PM +0000, David Laight wrote:
>> From: Igor Gnatenko
>> > On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote:
>> > > The AX88772B occasionally send rx packets that cross urb boundaries
>> > > and the remaining partial packet is sent with no header.
>> > > When the buffer with a partial packet is of less number of octets
>> > > than the value of hard_header_len the buffer is discarded by the
>> > > usbnet module. This is causing dropped packages and error messages
>> > > in dmesg.

> I will do some more digging in the code, but the test of skb->len
> against hard_header_len is done already in the completion callback
> function passed to usb_fill_bulk_urb so it seems that buffers of less
> than hard_header_len number of octets will be dropped regardless.

I am pretty sure you are right about this bug. And the exact same
solution is already used by the cx82310_eth minidriver, so I don't see
the problem.  Your fix is fine IMHO.  But you should apply it to all the
devices using asix_rx_fixup_common(), not just the ax88772 ones.

You could maybe make this a usbnet flag instead and create a generic
solution in usbnet, but frankly I believe the number of flags and their
meaning have exceeded drivers authors capabilities a long time ago.  At
least mine, which are quite limited ;-)

An example of that problem is another bloody obvious bug I noticed while
looking at this driver: The 'struct driver_info ax88178_info' points to
asix_rx_fixup_common without setting the FLAG_MULTI_PACKET.  This will
result in usbnet rx_process() calling usbnet_skb_return() on skbs which
are already consumed by the minidriver.  Not a big problem, but will
give some odd results.  But if you allow skbs shorter than ETH_HLEN to
slip through then it might go boom, so you should probably fix that as
well.


Bjørn

Re: [PATCH] net: asix: fix bad header length bug

[Emil Goode]

On Fri, Feb 07, 2014 at 10:38:04AM +0100, Bjørn Mork wrote:
> Emil Goode <emilgoode@gmail.com> writes:
> > On Thu, Feb 06, 2014 at 03:28:13PM +0000, David Laight wrote:
> >> From: Igor Gnatenko
> >> > On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote:
> >> > > The AX88772B occasionally send rx packets that cross urb boundaries
> >> > > and the remaining partial packet is sent with no header.
> >> > > When the buffer with a partial packet is of less number of octets
> >> > > than the value of hard_header_len the buffer is discarded by the
> >> > > usbnet module. This is causing dropped packages and error messages
> >> > > in dmesg.
> 
> > I will do some more digging in the code, but the test of skb->len
> > against hard_header_len is done already in the completion callback
> > function passed to usb_fill_bulk_urb so it seems that buffers of less
> > than hard_header_len number of octets will be dropped regardless.
> 
> I am pretty sure you are right about this bug. And the exact same
> solution is already used by the cx82310_eth minidriver, so I don't see
> the problem.  Your fix is fine IMHO.  But you should apply it to all the
> devices using asix_rx_fixup_common(), not just the ax88772 ones.
> 
> You could maybe make this a usbnet flag instead and create a generic
> solution in usbnet, but frankly I believe the number of flags and their
> meaning have exceeded drivers authors capabilities a long time ago.  At
> least mine, which are quite limited ;-)
> 
> An example of that problem is another bloody obvious bug I noticed while
> looking at this driver: The 'struct driver_info ax88178_info' points to
> asix_rx_fixup_common without setting the FLAG_MULTI_PACKET.  This will
> result in usbnet rx_process() calling usbnet_skb_return() on skbs which
> are already consumed by the minidriver.  Not a big problem, but will
> give some odd results.  But if you allow skbs shorter than ETH_HLEN to
> slip through then it might go boom, so you should probably fix that as
> well.
> 
> 
> Bjørn

Yes I believe the patch is necessary, but maybe it would be nice with
a prettier solution rather than setting hard_header_len to 0 for all
devices with this behaviour. Perhaps it would be better to let each
driver that uses the usbnet module decide what skbs to discard?

What David describes seems to be another bug, but I don't think it is
related to this patch as I'm able to reproduce the bug without the patch
beeing applied by setting the mtu to pretty much any value other than 
1500 and using ping with a larger packet size than that mtu value.

Best regards,

Emil Goode

RE: [PATCH] net: asix: fix bad header length bug

[David Laight]

From: Emil Goode 
> On Fri, Feb 07, 2014 at 10:38:04AM +0100, Bjørn Mork wrote:
> > Emil Goode <emilgoode@gmail.com> writes:
> > > On Thu, Feb 06, 2014 at 03:28:13PM +0000, David Laight wrote:
> > >> From: Igor Gnatenko
> > >> > On Thu, 2014-02-06 at 13:56 +0100, Emil Goode wrote:
> > >> > > The AX88772B occasionally send rx packets that cross urb boundaries
> > >> > > and the remaining partial packet is sent with no header.
> > >> > > When the buffer with a partial packet is of less number of octets
> > >> > > than the value of hard_header_len the buffer is discarded by the
> > >> > > usbnet module. This is causing dropped packages and error messages
> > >> > > in dmesg.
> >
> > > I will do some more digging in the code, but the test of skb->len
> > > against hard_header_len is done already in the completion callback
> > > function passed to usb_fill_bulk_urb so it seems that buffers of less
> > > than hard_header_len number of octets will be dropped regardless.
> >
> > I am pretty sure you are right about this bug. And the exact same
> > solution is already used by the cx82310_eth minidriver, so I don't see
> > the problem.  Your fix is fine IMHO.  But you should apply it to all the
> > devices using asix_rx_fixup_common(), not just the ax88772 ones.
> >
> > You could maybe make this a usbnet flag instead and create a generic
> > solution in usbnet, but frankly I believe the number of flags and their
> > meaning have exceeded drivers authors capabilities a long time ago.  At
> > least mine, which are quite limited ;-)
> >
> > An example of that problem is another bloody obvious bug I noticed while
> > looking at this driver: The 'struct driver_info ax88178_info' points to
> > asix_rx_fixup_common without setting the FLAG_MULTI_PACKET.  This will
> > result in usbnet rx_process() calling usbnet_skb_return() on skbs which
> > are already consumed by the minidriver.  Not a big problem, but will
> > give some odd results.  But if you allow skbs shorter than ETH_HLEN to
> > slip through then it might go boom, so you should probably fix that as
> > well.
> >
> >
> > Bjørn
> 
> Yes I believe the patch is necessary, but maybe it would be nice with
> a prettier solution rather than setting hard_header_len to 0 for all
> devices with this behaviour. Perhaps it would be better to let each
> driver that uses the usbnet module decide what skbs to discard?
> 
> What David describes seems to be another bug, but I don't think it is
> related to this patch as I'm able to reproduce the bug without the patch
> beeing applied by setting the mtu to pretty much any value other than
> 1500 and using ping with a larger packet size than that mtu value.

Yes - plenty of bugs if you just look for them!

I did a quick scan through the sub-drivers and although the usbnet code
seems to treat the 'hard_header_len' as a constant to add to the mtu when
allocating rx urb (when the driver doesn't set rx_urb_len), some of the
sub-drivers seem to have three length, the rx header, tx header and hard_header,
and set them separately (I've not just rechecked) - which may not exactly
match what the usbnet code does is the lengths are different.

The ax88772b driver seems to support several different bits of silicon.
Only some put multiple ethernet frames in a single urb, and only for these
does the driver set the rx_urb_length to 2048.
For the other silicon it relies on usbnet setting the rx urb size - so
the hard_header_len better not be set to zero.

Someone with some time to spare needs to modify usbnet to support page
aligned rx buffers (probably 4k urb) and then build correctly formatted
skb from them.

At the moment the ax179_178a driver allocates 20kB urb which end up
with an 0x40 byte offset into the page (so are probably 24k) and
then cause alignment issues in the xhci driver which currently doesn't
correctly handle non-aligned 64k address boundaries when the cross the
ring end.

	David

RE: [PATCH] net: asix: fix bad header length bug

[David Laight]

From: Emil Goode
> The AX88772B occasionally send rx packets that cross urb boundaries
> and the remaining partial packet is sent with no header.
> When the buffer with a partial packet is of less number of octets
> than the value of hard_header_len the buffer is discarded by the
> usbnet module. This is causing dropped packages and error messages
> in dmesg.
> 
> This can be reproduced by using ping with a packet size
> between 1965-1976.

I think this can affect other USB ethernet drivers.
Probably most of the ones that explicitly set rx_urb_len.

The ax88179_178a driver sets massive 20k receive urb.
I've seen over 10k of data in a single urb, dunno if it
can actually generate more than 20k - possibly if the usb3 link
is loaded with other traffic.
It would be much more efficient for it to use an aligned 4k urb
and then merge the fragment into skbs.

Once you've set:
+	dev->net->hard_header_len = 0; /* Partial packets have no header */
try setting the mtu to a multiple of 1k.
There is a very odd check in usbnet_change_mtu() that tries to stop the
receive urb_length being a multiple of the usb packet size.

This code looks as though it is hoping that the usb controller will discard
any full length bulk messages after finding a short buffer.
I suspect that might be just wishful thinking!

	David

Re: [PATCH] net: asix: fix bad header length bug

[Emil Goode]

Hello David,

Thank's for the review.

On Thu, Feb 06, 2014 at 01:37:12PM +0000, David Laight wrote:
> From: Emil Goode
> > The AX88772B occasionally send rx packets that cross urb boundaries
> > and the remaining partial packet is sent with no header.
> > When the buffer with a partial packet is of less number of octets
> > than the value of hard_header_len the buffer is discarded by the
> > usbnet module. This is causing dropped packages and error messages
> > in dmesg.
> > 
> > This can be reproduced by using ping with a packet size
> > between 1965-1976.
> 
> I think this can affect other USB ethernet drivers.
> Probably most of the ones that explicitly set rx_urb_len.
> 
> The ax88179_178a driver sets massive 20k receive urb.

The ax88179_178a has it's own bind function, so I believe it's
not affected by this change.

> I've seen over 10k of data in a single urb, dunno if it
> can actually generate more than 20k - possibly if the usb3 link
> is loaded with other traffic.
> It would be much more efficient for it to use an aligned 4k urb
> and then merge the fragment into skbs.
> 
> Once you've set:
> +	dev->net->hard_header_len = 0; /* Partial packets have no header */
> try setting the mtu to a multiple of 1k.

I tried setting the mtu to 2000 and when using ping with a large enough
packet size to fill the urb to rx_urb_size all packages are dropped.

> There is a very odd check in usbnet_change_mtu() that tries to stop the
> receive urb_length being a multiple of the usb packet size.

This is very odd indeed!

>
> This code looks as though it is hoping that the usb controller will discard
> any full length bulk messages after finding a short buffer.
> I suspect that might be just wishful thinking!
> 
> 	David
> 
> 
>