Skip to content

Commit

Permalink
usbnet: remove generic hard_header_len check
Browse files Browse the repository at this point in the history
This patch removes a generic hard_header_len check from the usbnet
module that is causing dropped packages under certain circumstances
for devices that send rx packets that cross urb boundaries.

One example is the AX88772B which occasionally send rx packets that
cross urb boundaries where the remaining partial packet is sent with
no hardware 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.

With AX88772B 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

This patch introduces the following changes:
- Removes the generic hard_header_len check in the rx_complete
  function in the usbnet module.
- Introduces a ETH_HLEN check for skbs that are not cloned from
  within a rx_fixup callback.
- For safety a hard_header_len check is added to each rx_fixup
  callback function that could be affected by this change.
  These extra checks could possibly be removed by someone
  who has the hardware to test.
- Removes a call to dev_kfree_skb_any() and instead utilizes the
  dev->done list to queue skbs for cleanup.

The changes place full responsibility on the rx_fixup callback
functions that clone skbs to only pass valid skbs to the
usbnet_skb_return function.

Signed-off-by: Emil Goode <emilgoode@gmail.com>
Reported-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Emil Goode authored and David S. Miller committed Feb 17, 2014
1 parent 08b4465 commit eb85569
Show file tree
Hide file tree
Showing 10 changed files with 45 additions and 21 deletions.
4 changes: 4 additions & 0 deletions drivers/net/usb/ax88179_178a.c
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,10 @@ static int ax88179_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
u16 hdr_off;
u32 *pkt_hdr;

/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

skb_trim(skb, skb->len - 4);
memcpy(&rx_hdr, skb_tail_pointer(skb), 4);
le32_to_cpus(&rx_hdr);
Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/gl620a.c
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ static int genelink_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
u32 size;
u32 count;

/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

header = (struct gl_header *) skb->data;

// get the packet count of the received skb
Expand Down
5 changes: 3 additions & 2 deletions drivers/net/usb/mcs7830.c
Original file line number Diff line number Diff line change
Expand Up @@ -526,8 +526,9 @@ static int mcs7830_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
u8 status;

if (skb->len == 0) {
dev_err(&dev->udev->dev, "unexpected empty rx frame\n");
/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len) {
dev_err(&dev->udev->dev, "unexpected tiny rx frame\n");
return 0;
}

Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/net1080.c
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,10 @@ static int net1080_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
struct nc_trailer *trailer;
u16 hdr_len, packet_len;

/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

if (!(skb->len & 0x01)) {
netdev_dbg(dev->net, "rx framesize %d range %d..%d mtu %d\n",
skb->len, dev->net->hard_header_len, dev->hard_mtu,
Expand Down
8 changes: 4 additions & 4 deletions drivers/net/usb/qmi_wwan.c
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
__be16 proto;

/* usbnet rx_complete guarantees that skb->len is at least
* hard_header_len, so we can inspect the dest address without
* checking skb->len
*/
/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

switch (skb->data[0] & 0xf0) {
case 0x40:
proto = htons(ETH_P_IP);
Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/rndis_host.c
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ EXPORT_SYMBOL_GPL(rndis_unbind);
*/
int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

/* peripheral may have batched packets to us... */
while (likely(skb->len)) {
struct rndis_data_hdr *hdr = (void *)skb->data;
Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/smsc75xx.c
Original file line number Diff line number Diff line change
Expand Up @@ -2106,6 +2106,10 @@ static void smsc75xx_rx_csum_offload(struct usbnet *dev, struct sk_buff *skb,

static int smsc75xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

while (skb->len > 0) {
u32 rx_cmd_a, rx_cmd_b, align_count, size;
struct sk_buff *ax_skb;
Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/smsc95xx.c
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,10 @@ static void smsc95xx_rx_csum_offload(struct sk_buff *skb)

static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

while (skb->len > 0) {
u32 header, align_count;
struct sk_buff *ax_skb;
Expand Down
4 changes: 4 additions & 0 deletions drivers/net/usb/sr9800.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ static int sr_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
{
int offset = 0;

/* This check is no longer done by usbnet */
if (skb->len < dev->net->hard_header_len)
return 0;

while (offset + sizeof(u32) < skb->len) {
struct sk_buff *sr_skb;
u16 size;
Expand Down
25 changes: 10 additions & 15 deletions drivers/net/usb/usbnet.c
Original file line number Diff line number Diff line change
Expand Up @@ -542,17 +542,19 @@ static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
}
// else network stack removes extra byte if we forced a short packet

if (skb->len) {
/* all data was already cloned from skb inside the driver */
if (dev->driver_info->flags & FLAG_MULTI_PACKET)
dev_kfree_skb_any(skb);
else
usbnet_skb_return(dev, skb);
/* all data was already cloned from skb inside the driver */
if (dev->driver_info->flags & FLAG_MULTI_PACKET)
goto done;

if (skb->len < ETH_HLEN) {
dev->net->stats.rx_errors++;
dev->net->stats.rx_length_errors++;
netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len);
} else {
usbnet_skb_return(dev, skb);
return;
}

netif_dbg(dev, rx_err, dev->net, "drop\n");
dev->net->stats.rx_errors++;
done:
skb_queue_tail(&dev->done, skb);
}
Expand All @@ -574,13 +576,6 @@ static void rx_complete (struct urb *urb)
switch (urb_status) {
/* success */
case 0:
if (skb->len < dev->net->hard_header_len) {
state = rx_cleanup;
dev->net->stats.rx_errors++;
dev->net->stats.rx_length_errors++;
netif_dbg(dev, rx_err, dev->net,
"rx length %d\n", skb->len);
}
break;

/* stalls need manual reset. this is rare ... except that
Expand Down

0 comments on commit eb85569

Please sign in to comment.