* [PATCH v2 0/2] pegasus: correct buffer sizes
@ 2016-04-27 7:35 Petko Manolov
2016-04-27 7:35 ` [PATCH v2 1/2] pegasus: fixes URB buffer allocation size; Petko Manolov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Petko Manolov @ 2016-04-27 7:35 UTC (permalink / raw)
To: netdev; +Cc: davem, a1291762, Petko Manolov
As noticed by Lincoln Ramsay <a1291762@gmail.com> some old (usb 1.1) Pegasus
based devices may actually return more bytes than the specified in the datasheet
amount. That would not be a problem if the allocated space for the SKB was
equal to the parameter passed to usb_fill_bulk_urb(). Some poor bugger (i
really hope it was not me, but 'git blame' is useless in this case, so anyway)
decided to add '+ 8' to the buffer length parameter. Sometimes the usb transfer
overflows and corrupts the socket structure, leading to kernel panic.
The above doesn't seem to happen for newer (Pegasus2 based) devices which did
help this bug to hide for so long.
Nearly all Pegasus devices may append the RX status to the end of the received
packet. It is the default setup for the driver. The actual ethernet packet is
4 bytes shorter. Why and when 'pkt_len -= 4' became 'pkt_len -= 8' is again
hidden in the mists of time. There might have been a good reason to do so, but
multiple reads of the datasheet did not point me to any.
The patch is against v4.6-rc5 and was tested on ADM8515 device by transferring
multiple gigabytes of data over a couple of days without any complaints from the
kernel.
Changes since v1:
- split the patch in two parts;
- corrected the subject lines;
Petko Manolov (2):
pegasus: fixes URB buffer allocation size;
pegasus: fixes reported packet length
drivers/net/usb/pegasus.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--
2.8.0.rc3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] pegasus: fixes URB buffer allocation size;
2016-04-27 7:35 [PATCH v2 0/2] pegasus: correct buffer sizes Petko Manolov
@ 2016-04-27 7:35 ` Petko Manolov
2016-04-27 7:35 ` [PATCH v2 2/2] pegasus: fixes reported packet length Petko Manolov
2016-04-27 7:46 ` [PATCH v2 0/2] pegasus: correct buffer sizes Johannes Berg
2 siblings, 0 replies; 7+ messages in thread
From: Petko Manolov @ 2016-04-27 7:35 UTC (permalink / raw)
To: netdev; +Cc: davem, a1291762, Petko Manolov
usb_fill_bulk_urb() receives buffer length parameter 8 bytes larger
than what's allocated by alloc_skb(); This seems to be a problem with
older (pegasus usb-1.1) devices, which may silently return more data
than the maximal packet length.
Reported-by: Lincoln Ramsay <a1291762@gmail.com>
Signed-off-by: Petko Manolov <petkan@mip-labs.com>
---
drivers/net/usb/pegasus.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f840802..f919e20 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -528,7 +528,7 @@ static void read_bulk_callback(struct urb *urb)
goon:
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
read_bulk_callback, pegasus);
rx_status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
if (rx_status == -ENODEV)
@@ -569,7 +569,7 @@ static void rx_fixup(unsigned long data)
}
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
read_bulk_callback, pegasus);
try_again:
status = usb_submit_urb(pegasus->rx_urb, GFP_ATOMIC);
@@ -823,7 +823,7 @@ static int pegasus_open(struct net_device *net)
usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
usb_rcvbulkpipe(pegasus->usb, 1),
- pegasus->rx_skb->data, PEGASUS_MTU + 8,
+ pegasus->rx_skb->data, PEGASUS_MTU,
read_bulk_callback, pegasus);
if ((res = usb_submit_urb(pegasus->rx_urb, GFP_KERNEL))) {
if (res == -ENODEV)
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] pegasus: fixes reported packet length
2016-04-27 7:35 [PATCH v2 0/2] pegasus: correct buffer sizes Petko Manolov
2016-04-27 7:35 ` [PATCH v2 1/2] pegasus: fixes URB buffer allocation size; Petko Manolov
@ 2016-04-27 7:35 ` Petko Manolov
2016-04-27 7:46 ` [PATCH v2 0/2] pegasus: correct buffer sizes Johannes Berg
2 siblings, 0 replies; 7+ messages in thread
From: Petko Manolov @ 2016-04-27 7:35 UTC (permalink / raw)
To: netdev; +Cc: davem, a1291762, Petko Manolov
According to ADM851x docs the ethernet packet is appended with 4
bytes, not 8. Fixing this to report (hopefully) the right amount
of data.
Signed-off-by: Petko Manolov <petkan@mip-labs.com>
---
drivers/net/usb/pegasus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f919e20..780c217 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -497,7 +497,7 @@ static void read_bulk_callback(struct urb *urb)
pkt_len = buf[count - 3] << 8;
pkt_len += buf[count - 4];
pkt_len &= 0xfff;
- pkt_len -= 8;
+ pkt_len -= 4;
}
/*
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] pegasus: correct buffer sizes
2016-04-27 7:35 [PATCH v2 0/2] pegasus: correct buffer sizes Petko Manolov
2016-04-27 7:35 ` [PATCH v2 1/2] pegasus: fixes URB buffer allocation size; Petko Manolov
2016-04-27 7:35 ` [PATCH v2 2/2] pegasus: fixes reported packet length Petko Manolov
@ 2016-04-27 7:46 ` Johannes Berg
[not found] ` <20160427093358.GA30444@p310>
2 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2016-04-27 7:46 UTC (permalink / raw)
To: Petko Manolov, netdev; +Cc: davem, a1291762
On Wed, 2016-04-27 at 10:35 +0300, Petko Manolov wrote:
>
> Nearly all Pegasus devices may append the RX status to the end of the
> received packet. It is the default setup for the driver. The
> actual ethernet packet is 4 bytes shorter. Why and when 'pkt_len -=
> 4' became 'pkt_len -= 8' is again hidden in the mists of time. There
> might have been a good reason to do so, but multiple reads of the
> datasheet did not point me to any.
>
Wild guess: (some) devices pass the FCS, perhaps depending on the
configuration?
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] pegasus: correct buffer sizes
[not found] ` <20160427093358.GA30444@p310>
@ 2016-04-27 9:44 ` Johannes Berg
2016-04-27 16:26 ` David Laight
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2016-04-27 9:44 UTC (permalink / raw)
To: Petko Manolov; +Cc: netdev, davem, a1291762
On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
>
> Your guess turned out to be not so wild. ;) All Pegasus devices are
> configured (by the driver) to append CRC at the end of each RX
> packet. However, the driver reports packet length that does not
> include it.
Interesting, then my guess was wrong though, since the length is
reported without it, or am I misunderstanding this?
> I doubt the appended CRC is being silently verified by the upper
> layer, bit i might be wrong of course.
It's even "outside" the skb as you describe it, so it can't even be
touched, no?
> Perhaps it is best if instruct the device to not include the CRC as
> it seems ignored anyway.
Yeah, there's no point in passing it over the bus.
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2 0/2] pegasus: correct buffer sizes
2016-04-27 9:44 ` Johannes Berg
@ 2016-04-27 16:26 ` David Laight
2016-04-28 6:49 ` Johannes Berg
0 siblings, 1 reply; 7+ messages in thread
From: David Laight @ 2016-04-27 16:26 UTC (permalink / raw)
To: 'Johannes Berg', Petko Manolov; +Cc: netdev, davem, a1291762
From: Johannes Berg
> Sent: 27 April 2016 10:44
> On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
> >
> > Your guess turned out to be not so wild.;)All Pegasus devices are
> > configured (by the driver) to append CRC at the end of each RX
> > packet.However, the driver reports packet length that does not
> > include it.
>
> Interesting, then my guess was wrong though, since the length is
> reported without it, or am I misunderstanding this?
...
It is even possible that the crc is written into the rx buffer even
though the length the hardware reports excludes it.
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] pegasus: correct buffer sizes
2016-04-27 16:26 ` David Laight
@ 2016-04-28 6:49 ` Johannes Berg
0 siblings, 0 replies; 7+ messages in thread
From: Johannes Berg @ 2016-04-28 6:49 UTC (permalink / raw)
To: David Laight, Petko Manolov; +Cc: netdev, davem, a1291762
On Wed, 2016-04-27 at 16:26 +0000, David Laight wrote:
> From: Johannes Berg
> >
> > Sent: 27 April 2016 10:44
> > On Wed, 2016-04-27 at 12:33 +0300, Petko Manolov wrote:
> > >
> > >
> > > Your guess turned out to be not so wild.;)All Pegasus devices are
> > > configured (by the driver) to append CRC at the end of each RX
> > > packet.However, the driver reports packet length that does not
> > > include it.
> > Interesting, then my guess was wrong though, since the length is
> > reported without it, or am I misunderstanding this?
> ...
>
> It is even possible that the crc is written into the rx buffer even
> though the length the hardware reports excludes it.
>
Right. I think a proper test would be to construct some kind of
ethernet-only frames, and check that they come out properly on the
other side. IP always has its own length field, so may be happy with
trailing garbage reported to the network stack, but doing raw ethernet
will let you check that the frame length is correct.
johannes
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-28 6:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27 7:35 [PATCH v2 0/2] pegasus: correct buffer sizes Petko Manolov
2016-04-27 7:35 ` [PATCH v2 1/2] pegasus: fixes URB buffer allocation size; Petko Manolov
2016-04-27 7:35 ` [PATCH v2 2/2] pegasus: fixes reported packet length Petko Manolov
2016-04-27 7:46 ` [PATCH v2 0/2] pegasus: correct buffer sizes Johannes Berg
[not found] ` <20160427093358.GA30444@p310>
2016-04-27 9:44 ` Johannes Berg
2016-04-27 16:26 ` David Laight
2016-04-28 6:49 ` Johannes Berg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.