From 7b0378f517fa1a32b5c8384248d2f8bf79c7c2ae Mon Sep 17 00:00:00 2001 From: Dean Jenkins Date: Fri, 2 Oct 2015 14:29:04 +0100 Subject: asix: Rename remaining and size for clarity The Data header synchronisation is easier to understand if the variables "remaining" and "size" are renamed. Therefore, the lifetime of the "remaining" variable exists outside of asix_rx_fixup_internal() and is used to indicate any remaining pending bytes of the Ethernet frame that need to be obtained from the next socket buffer. This allows an Ethernet frame to span across multiple socket buffers. "size" is now local to asix_rx_fixup_internal() and contains the size read from the Data header 32-bit word. Add "copy_length" to hold the number of the Ethernet frame bytes (maybe a part of a full frame) that are to be copied out of the socket buffer. Signed-off-by: Dean Jenkins Signed-off-by: Mark Craske Signed-off-by: David S. Miller --- drivers/net/usb/asix.h | 2 +- drivers/net/usb/asix_common.c | 41 +++++++++++++++++++++-------------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h index 5d049d00c2d7..a2d3ea6efb20 100644 --- a/drivers/net/usb/asix.h +++ b/drivers/net/usb/asix.h @@ -168,7 +168,7 @@ struct asix_data { struct asix_rx_fixup_info { struct sk_buff *ax_skb; u32 header; - u16 size; + u16 remaining; bool split_head; }; diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 75d6f26729a3..2bd5bdda8c2e 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -54,12 +54,13 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, struct asix_rx_fixup_info *rx) { int offset = 0; + u16 size; while (offset + sizeof(u16) <= skb->len) { - u16 remaining = 0; + u16 copy_length; unsigned char *data; - if (!rx->size) { + if (!rx->remaining) { if ((skb->len - offset == sizeof(u16)) || rx->split_head) { if(!rx->split_head) { @@ -81,42 +82,42 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, offset += sizeof(u32); } - /* get the packet length */ - rx->size = (u16) (rx->header & 0x7ff); - if (rx->size != ((~rx->header >> 16) & 0x7ff)) { + /* take frame length from Data header 32-bit word */ + size = (u16)(rx->header & 0x7ff); + if (size != ((~rx->header >> 16) & 0x7ff)) { netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n", rx->header, offset); - rx->size = 0; return 0; } - rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, - rx->size); + rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size); if (!rx->ax_skb) return 0; + rx->remaining = size; } - if (rx->size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) { + if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) { netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n", - rx->size); + rx->remaining); kfree_skb(rx->ax_skb); rx->ax_skb = NULL; - rx->size = 0U; - + rx->remaining = 0; return 0; } - if (rx->size > skb->len - offset) { - remaining = rx->size - (skb->len - offset); - rx->size = skb->len - offset; + if (rx->remaining > skb->len - offset) { + copy_length = skb->len - offset; + rx->remaining -= copy_length; + } else { + copy_length = rx->remaining; + rx->remaining = 0; } - data = skb_put(rx->ax_skb, rx->size); - memcpy(data, skb->data + offset, rx->size); - if (!remaining) + data = skb_put(rx->ax_skb, copy_length); + memcpy(data, skb->data + offset, copy_length); + if (!rx->remaining) usbnet_skb_return(dev, rx->ax_skb); - offset += (rx->size + 1) & 0xfffe; - rx->size = remaining; + offset += (copy_length + 1) & 0xfffe; } if (skb->len != offset) { -- cgit v1.2.3-59-g8ed1b From 3bfc69abf802f56901ffd83bb66b7dd7644ddcc3 Mon Sep 17 00:00:00 2001 From: Dean Jenkins Date: Fri, 2 Oct 2015 14:29:05 +0100 Subject: asix: Tidy-up 32-bit header word synchronisation Tidy-up the Data header 32-bit word synchronisation logic in asix_rx_fixup_internal() by removing redundant logic tests. The code is looking at the following cases of the Data header 32-bit word that is present before each Ethernet frame: a) all 32 bits of the Data header word are in the URB socket buffer b) first 16 bits of the Data header word are at the end of the URB socket buffer c) last 16 bits of the Data header word are at the start of the URB socket buffer eg. split_head = true Note that the lifetime of rx->split_head exists outside of the function call and is accessed per processing of each URB. Therefore, split_head being true acts on the next URB to be processed. To check for b) the offset will be 16 bits (2 bytes) from the end of the buffer then indicate split_head is true. To check for c) split_head must be true because the first 16 bits have been found. To check for a) else c) Note that the || logic of the old code included the state (skb->len - offset == sizeof(u16) && rx->split_head) which is not possible because the split_head cannot be true whilst checking for b). This is because the split_head indicates that the first 16 bits have been found and that is not possible whilst checking for the first 16 bits. Therefore simplify the logic. Signed-off-by: Dean Jenkins Signed-off-by: Mark Craske Signed-off-by: David S. Miller --- drivers/net/usb/asix_common.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 2bd5bdda8c2e..89efd6ad9644 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -61,21 +61,19 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, unsigned char *data; if (!rx->remaining) { - if ((skb->len - offset == sizeof(u16)) || - rx->split_head) { - if(!rx->split_head) { - rx->header = get_unaligned_le16( - skb->data + offset); - rx->split_head = true; - offset += sizeof(u16); - break; - } else { - rx->header |= (get_unaligned_le16( - skb->data + offset) - << 16); - rx->split_head = false; - offset += sizeof(u16); - } + if (skb->len - offset == sizeof(u16)) { + rx->header = get_unaligned_le16( + skb->data + offset); + rx->split_head = true; + offset += sizeof(u16); + break; + } + + if (rx->split_head == true) { + rx->header |= (get_unaligned_le16( + skb->data + offset) << 16); + rx->split_head = false; + offset += sizeof(u16); } else { rx->header = get_unaligned_le32(skb->data + offset); -- cgit v1.2.3-59-g8ed1b From 9a5ccd8e039eef53336e45d01c7d8a1acbd36b47 Mon Sep 17 00:00:00 2001 From: Dean Jenkins Date: Fri, 2 Oct 2015 14:29:06 +0100 Subject: asix: Simplify asix_rx_fixup_internal() netdev alloc The code is checking that the Ethernet frame will fit into a netdev allocated socket buffer within the constraints of MTU size, Ethernet header length plus VLAN header length. The original code was checking rx->remaining each loop of the while loop that processes multiple Ethernet frames per URB and/or Ethernet frames that span across URBs. rx->remaining decreases per while loop so there is no point in potentially checking multiple times that the Ethernet frame (remaining part) will fit into the netdev socket buffer. The modification checks that the size of the Ethernet frame will fit the netdev socket buffer before allocating the netdev socket buffer. This avoids grabbing memory and then deciding that the Ethernet frame is too big and then freeing the memory. Signed-off-by: Dean Jenkins Signed-off-by: Mark Craske Signed-off-by: David S. Miller --- drivers/net/usb/asix_common.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 89efd6ad9644..6a8eddfea229 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -87,19 +87,17 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, rx->header, offset); return 0; } + if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) { + netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n", + size); + return 0; + } + rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size); if (!rx->ax_skb) return 0; - rx->remaining = size; - } - if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) { - netdev_err(dev->net, "asix_rx_fixup() Bad RX Length %d\n", - rx->remaining); - kfree_skb(rx->ax_skb); - rx->ax_skb = NULL; - rx->remaining = 0; - return 0; + rx->remaining = size; } if (rx->remaining > skb->len - offset) { -- cgit v1.2.3-59-g8ed1b From 3f30b158eba5c604b6e0870027eef5d19fc9271d Mon Sep 17 00:00:00 2001 From: Dean Jenkins Date: Fri, 2 Oct 2015 14:29:07 +0100 Subject: asix: On RX avoid creating bad Ethernet frames When RX Ethernet frames span multiple URB socket buffers, the data stream may suffer a discontinuity which will cause the current Ethernet frame in the netdev socket buffer to be incomplete. This frame needs to be discarded instead of appending unrelated data from the current URB socket buffer to the Ethernet frame in the netdev socket buffer. This avoids creating a corrupted Ethernet frame in the netdev socket buffer. A discontinuity can occur when the previous URB socket buffer held an incomplete Ethernet frame due to truncation or a URB socket buffer containing the end of the Ethernet frame was missing. Therefore, add a sanity test for when an Ethernet frame spans multiple URB socket buffers to check that the remaining bytes of the currently received Ethernet frame point to a good Data header 32-bit word of the next Ethernet frame. Upon error, reset the remaining bytes variable to zero and discard the current netdev socket buffer. Assume that the Data header is located at the start of the current socket buffer and attempt to process the next Ethernet frame from there. This avoids unnecessarily discarding a good URB socket buffer that contains a new Ethernet frame. Signed-off-by: Dean Jenkins Signed-off-by: Mark Craske Signed-off-by: David S. Miller --- drivers/net/usb/asix_common.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 6a8eddfea229..1e4768debcc9 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -56,6 +56,34 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, int offset = 0; u16 size; + /* When an Ethernet frame spans multiple URB socket buffers, + * do a sanity test for the Data header synchronisation. + * Attempt to detect the situation of the previous socket buffer having + * been truncated or a socket buffer was missing. These situations + * cause a discontinuity in the data stream and therefore need to avoid + * appending bad data to the end of the current netdev socket buffer. + * Also avoid unnecessarily discarding a good current netdev socket + * buffer. + */ + if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) { + offset = ((rx->remaining + 1) & 0xfffe) + sizeof(u32); + rx->header = get_unaligned_le32(skb->data + offset); + offset = 0; + + size = (u16)(rx->header & 0x7ff); + if (size != ((~rx->header >> 16) & 0x7ff)) { + netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n", + rx->remaining); + kfree_skb(rx->ax_skb); + rx->ax_skb = NULL; + /* Discard the incomplete netdev Ethernet frame and + * assume the Data header is at the start of the current + * URB socket buffer. + */ + rx->remaining = 0; + } + } + while (offset + sizeof(u16) <= skb->len) { u16 copy_length; unsigned char *data; -- cgit v1.2.3-59-g8ed1b From 6a570814cd430fa5ef4f278e8046dcf12ee63f13 Mon Sep 17 00:00:00 2001 From: Dean Jenkins Date: Fri, 2 Oct 2015 14:29:08 +0100 Subject: asix: Continue processing URB if no RX netdev buffer Avoid a loss of synchronisation of the Ethernet Data header 32-bit word due to a failure to get a netdev socket buffer. The ASIX RX handling algorithm returned 0 upon a failure to get an allocation of a netdev socket buffer. This causes the URB processing to stop which potentially causes a loss of synchronisation with the Ethernet Data header 32-bit word. Therefore, subsequent processing of URBs may be rejected due to a loss of synchronisation. This may cause additional good Ethernet frames to be discarded along with outputting of synchronisation error messages. Implement a solution which checks whether a netdev socket buffer has been allocated before trying to copy the Ethernet frame into the netdev socket buffer. But continue to process the URB so that synchronisation is maintained. Therefore, only a single Ethernet frame is discarded when no netdev socket buffer is available. Signed-off-by: Dean Jenkins Signed-off-by: Mark Craske Signed-off-by: David S. Miller --- drivers/net/usb/asix_common.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c index 1e4768debcc9..a186b0a12d50 100644 --- a/drivers/net/usb/asix_common.c +++ b/drivers/net/usb/asix_common.c @@ -74,12 +74,14 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, if (size != ((~rx->header >> 16) & 0x7ff)) { netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n", rx->remaining); - kfree_skb(rx->ax_skb); - rx->ax_skb = NULL; - /* Discard the incomplete netdev Ethernet frame and - * assume the Data header is at the start of the current - * URB socket buffer. - */ + if (rx->ax_skb) { + kfree_skb(rx->ax_skb); + rx->ax_skb = NULL; + /* Discard the incomplete netdev Ethernet frame + * and assume the Data header is at the start of + * the current URB socket buffer. + */ + } rx->remaining = 0; } } @@ -121,9 +123,12 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, return 0; } + /* Sometimes may fail to get a netdev socket buffer but + * continue to process the URB socket buffer so that + * synchronisation of the Ethernet frame Data header + * word is maintained. + */ rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size); - if (!rx->ax_skb) - return 0; rx->remaining = size; } @@ -136,10 +141,12 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb, rx->remaining = 0; } - data = skb_put(rx->ax_skb, copy_length); - memcpy(data, skb->data + offset, copy_length); - if (!rx->remaining) - usbnet_skb_return(dev, rx->ax_skb); + if (rx->ax_skb) { + data = skb_put(rx->ax_skb, copy_length); + memcpy(data, skb->data + offset, copy_length); + if (!rx->remaining) + usbnet_skb_return(dev, rx->ax_skb); + } offset += (copy_length + 1) & 0xfffe; } -- cgit v1.2.3-59-g8ed1b