| Message ID | 20201030022839.438135-6-xie.he.0141@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 90 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Thu, Oct 29, 2020 at 10:32 PM Xie He <xie.he.0141@gmail.com> wrote: > > Change the fr_rx function to make this driver support any Ethertype > when receiving skbs on normal (non-Ethernet-emulating) PVC devices. > (This driver is already able to handle any Ethertype when sending.) > > Originally in the fr_rx function, the code that parses the long (10-byte) > header only recognizes a few Ethertype values and drops frames with other > Ethertype values. This patch replaces this code to make fr_rx support > any Ethertype. This patch also creates a new function fr_snap_parse as > part of the new code. > > Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Cc: Krzysztof Halasa <khc@pm.waw.pl> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- > drivers/net/wan/hdlc_fr.c | 75 +++++++++++++++++++++++++-------------- > 1 file changed, 49 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > index 9a37575686b9..e95efc14bc97 100644 > --- a/drivers/net/wan/hdlc_fr.c > +++ b/drivers/net/wan/hdlc_fr.c > @@ -871,6 +871,45 @@ static int fr_lmi_recv(struct net_device *dev, struct sk_buff *skb) > return 0; > } > > static int fr_rx(struct sk_buff *skb) > { > @@ -945,35 +984,19 @@ static int fr_rx(struct sk_buff *skb) > skb->protocol = htons(ETH_P_IPV6); > skb_reset_mac_header(skb); > > - } else if (skb->len > 10 && data[3] == FR_PAD && > - data[4] == NLPID_SNAP && data[5] == FR_PAD) { > - u16 oui = ntohs(*(__be16*)(data + 6)); > - u16 pid = ntohs(*(__be16*)(data + 8)); > - skb_pull(skb, 10); > - > - switch ((((u32)oui) << 16) | pid) { > - case ETH_P_ARP: /* routed frame with SNAP */ > - case ETH_P_IPX: > - case ETH_P_IP: /* a long variant */ > - case ETH_P_IPV6: > - if (!pvc->main) > - goto rx_drop; > - skb->dev = pvc->main; > - skb->protocol = htons(pid); > - skb_reset_mac_header(skb); > - break; > - > - case 0x80C20007: /* bridged Ethernet frame */ > - if (!pvc->ether) > + } else if (data[3] == FR_PAD) { > + if (skb->len < 5) > + goto rx_error; > + if (data[4] == NLPID_SNAP) { /* A SNAP header follows */ Should this still check data[5] == FR_PAD? > + skb_pull(skb, 5); > + if (skb->len < 5) /* Incomplete SNAP header */ > + goto rx_error; > + if (fr_snap_parse(skb, pvc)) > goto rx_drop; > - skb->protocol = eth_type_trans(skb, pvc->ether); > - break; > - > - default: > - netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n", > - oui, pid); > + } else { > goto rx_drop; > } > + > } else { > netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n", > data[3], skb->len); > -- > 2.27.0 >
On Fri, Oct 30, 2020 at 9:33 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Should this still check data[5] == FR_PAD? No, the 6th byte (data[5]) is not a padding field. It is the first byte of the SNAP header. The original code is misleading. That is part of the reasons why I want to fix it with this patch. The frame format is specified in RFC 2427 (https://tools.ietf.org/html/rfc2427). We can see in Section 4.1 and 4.2 that the 6th byte is the first byte of the SNAP header.
On Fri, Oct 30, 2020 at 3:29 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Fri, Oct 30, 2020 at 9:33 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > Should this still check data[5] == FR_PAD? > > No, the 6th byte (data[5]) is not a padding field. It is the first > byte of the SNAP header. The original code is misleading. That is part > of the reasons why I want to fix it with this patch. Oh, good point. In that case Acked-by: Willem de Bruijn <willemb@google.com> > The frame format is specified in RFC 2427 > (https://tools.ietf.org/html/rfc2427). We can see in Section 4.1 and > 4.2 that the 6th byte is the first byte of the SNAP header.
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c index 9a37575686b9..e95efc14bc97 100644 --- a/drivers/net/wan/hdlc_fr.c +++ b/drivers/net/wan/hdlc_fr.c @@ -871,6 +871,45 @@ static int fr_lmi_recv(struct net_device *dev, struct sk_buff *skb) return 0; } +static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc) +{ + /* OUI 00-00-00 indicates an Ethertype follows */ + if (skb->data[0] == 0x00 && + skb->data[1] == 0x00 && + skb->data[2] == 0x00) { + if (!pvc->main) + return -1; + skb->dev = pvc->main; + skb->protocol = *(__be16 *)(skb->data + 3); /* Ethertype */ + skb_pull(skb, 5); + skb_reset_mac_header(skb); + return 0; + + /* OUI 00-80-C2 stands for the 802.1 organization */ + } else if (skb->data[0] == 0x00 && + skb->data[1] == 0x80 && + skb->data[2] == 0xC2) { + /* PID 00-07 stands for Ethernet frames without FCS */ + if (skb->data[3] == 0x00 && + skb->data[4] == 0x07) { + if (!pvc->ether) + return -1; + skb_pull(skb, 5); + if (skb->len < ETH_HLEN) + return -1; + skb->protocol = eth_type_trans(skb, pvc->ether); + return 0; + + /* PID unsupported */ + } else { + return -1; + } + + /* OUI unsupported */ + } else { + return -1; + } +} static int fr_rx(struct sk_buff *skb) { @@ -945,35 +984,19 @@ static int fr_rx(struct sk_buff *skb) skb->protocol = htons(ETH_P_IPV6); skb_reset_mac_header(skb); - } else if (skb->len > 10 && data[3] == FR_PAD && - data[4] == NLPID_SNAP && data[5] == FR_PAD) { - u16 oui = ntohs(*(__be16*)(data + 6)); - u16 pid = ntohs(*(__be16*)(data + 8)); - skb_pull(skb, 10); - - switch ((((u32)oui) << 16) | pid) { - case ETH_P_ARP: /* routed frame with SNAP */ - case ETH_P_IPX: - case ETH_P_IP: /* a long variant */ - case ETH_P_IPV6: - if (!pvc->main) - goto rx_drop; - skb->dev = pvc->main; - skb->protocol = htons(pid); - skb_reset_mac_header(skb); - break; - - case 0x80C20007: /* bridged Ethernet frame */ - if (!pvc->ether) + } else if (data[3] == FR_PAD) { + if (skb->len < 5) + goto rx_error; + if (data[4] == NLPID_SNAP) { /* A SNAP header follows */ + skb_pull(skb, 5); + if (skb->len < 5) /* Incomplete SNAP header */ + goto rx_error; + if (fr_snap_parse(skb, pvc)) goto rx_drop; - skb->protocol = eth_type_trans(skb, pvc->ether); - break; - - default: - netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n", - oui, pid); + } else { goto rx_drop; } + } else { netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n", data[3], skb->len);
Change the fr_rx function to make this driver support any Ethertype when receiving skbs on normal (non-Ethernet-emulating) PVC devices. (This driver is already able to handle any Ethertype when sending.) Originally in the fr_rx function, the code that parses the long (10-byte) header only recognizes a few Ethertype values and drops frames with other Ethertype values. This patch replaces this code to make fr_rx support any Ethertype. This patch also creates a new function fr_snap_parse as part of the new code. Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Cc: Krzysztof Halasa <khc@pm.waw.pl> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/hdlc_fr.c | 75 +++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 26 deletions(-)