Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

usbnet: remove generic hard_header_len check

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>

authored by

Emil Goode and committed by
David S. Miller
eb85569f 08b44656

+45 -21
+4
drivers/net/usb/ax88179_178a.c
··· 1118 1118 u16 hdr_off; 1119 1119 u32 *pkt_hdr; 1120 1120 1121 + /* This check is no longer done by usbnet */ 1122 + if (skb->len < dev->net->hard_header_len) 1123 + return 0; 1124 + 1121 1125 skb_trim(skb, skb->len - 4); 1122 1126 memcpy(&rx_hdr, skb_tail_pointer(skb), 4); 1123 1127 le32_to_cpus(&rx_hdr);
+4
drivers/net/usb/gl620a.c
··· 84 84 u32 size; 85 85 u32 count; 86 86 87 + /* This check is no longer done by usbnet */ 88 + if (skb->len < dev->net->hard_header_len) 89 + return 0; 90 + 87 91 header = (struct gl_header *) skb->data; 88 92 89 93 // get the packet count of the received skb
+3 -2
drivers/net/usb/mcs7830.c
··· 526 526 { 527 527 u8 status; 528 528 529 - if (skb->len == 0) { 530 - dev_err(&dev->udev->dev, "unexpected empty rx frame\n"); 529 + /* This check is no longer done by usbnet */ 530 + if (skb->len < dev->net->hard_header_len) { 531 + dev_err(&dev->udev->dev, "unexpected tiny rx frame\n"); 531 532 return 0; 532 533 } 533 534
+4
drivers/net/usb/net1080.c
··· 364 364 struct nc_trailer *trailer; 365 365 u16 hdr_len, packet_len; 366 366 367 + /* This check is no longer done by usbnet */ 368 + if (skb->len < dev->net->hard_header_len) 369 + return 0; 370 + 367 371 if (!(skb->len & 0x01)) { 368 372 netdev_dbg(dev->net, "rx framesize %d range %d..%d mtu %d\n", 369 373 skb->len, dev->net->hard_header_len, dev->hard_mtu,
+4 -4
drivers/net/usb/qmi_wwan.c
··· 80 80 { 81 81 __be16 proto; 82 82 83 - /* usbnet rx_complete guarantees that skb->len is at least 84 - * hard_header_len, so we can inspect the dest address without 85 - * checking skb->len 86 - */ 83 + /* This check is no longer done by usbnet */ 84 + if (skb->len < dev->net->hard_header_len) 85 + return 0; 86 + 87 87 switch (skb->data[0] & 0xf0) { 88 88 case 0x40: 89 89 proto = htons(ETH_P_IP);
+4
drivers/net/usb/rndis_host.c
··· 492 492 */ 493 493 int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb) 494 494 { 495 + /* This check is no longer done by usbnet */ 496 + if (skb->len < dev->net->hard_header_len) 497 + return 0; 498 + 495 499 /* peripheral may have batched packets to us... */ 496 500 while (likely(skb->len)) { 497 501 struct rndis_data_hdr *hdr = (void *)skb->data;
+4
drivers/net/usb/smsc75xx.c
··· 2106 2106 2107 2107 static int smsc75xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb) 2108 2108 { 2109 + /* This check is no longer done by usbnet */ 2110 + if (skb->len < dev->net->hard_header_len) 2111 + return 0; 2112 + 2109 2113 while (skb->len > 0) { 2110 2114 u32 rx_cmd_a, rx_cmd_b, align_count, size; 2111 2115 struct sk_buff *ax_skb;
+4
drivers/net/usb/smsc95xx.c
··· 1723 1723 1724 1724 static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb) 1725 1725 { 1726 + /* This check is no longer done by usbnet */ 1727 + if (skb->len < dev->net->hard_header_len) 1728 + return 0; 1729 + 1726 1730 while (skb->len > 0) { 1727 1731 u32 header, align_count; 1728 1732 struct sk_buff *ax_skb;
+4
drivers/net/usb/sr9800.c
··· 63 63 { 64 64 int offset = 0; 65 65 66 + /* This check is no longer done by usbnet */ 67 + if (skb->len < dev->net->hard_header_len) 68 + return 0; 69 + 66 70 while (offset + sizeof(u32) < skb->len) { 67 71 struct sk_buff *sr_skb; 68 72 u16 size;
+10 -15
drivers/net/usb/usbnet.c
··· 542 542 } 543 543 // else network stack removes extra byte if we forced a short packet 544 544 545 - if (skb->len) { 546 - /* all data was already cloned from skb inside the driver */ 547 - if (dev->driver_info->flags & FLAG_MULTI_PACKET) 548 - dev_kfree_skb_any(skb); 549 - else 550 - usbnet_skb_return(dev, skb); 545 + /* all data was already cloned from skb inside the driver */ 546 + if (dev->driver_info->flags & FLAG_MULTI_PACKET) 547 + goto done; 548 + 549 + if (skb->len < ETH_HLEN) { 550 + dev->net->stats.rx_errors++; 551 + dev->net->stats.rx_length_errors++; 552 + netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len); 553 + } else { 554 + usbnet_skb_return(dev, skb); 551 555 return; 552 556 } 553 557 554 - netif_dbg(dev, rx_err, dev->net, "drop\n"); 555 - dev->net->stats.rx_errors++; 556 558 done: 557 559 skb_queue_tail(&dev->done, skb); 558 560 } ··· 576 574 switch (urb_status) { 577 575 /* success */ 578 576 case 0: 579 - if (skb->len < dev->net->hard_header_len) { 580 - state = rx_cleanup; 581 - dev->net->stats.rx_errors++; 582 - dev->net->stats.rx_length_errors++; 583 - netif_dbg(dev, rx_err, dev->net, 584 - "rx length %d\n", skb->len); 585 - } 586 577 break; 587 578 588 579 /* stalls need manual reset. this is rare ... except that