diff mbox series

[net-next,v2,4/4] net: hdlc_fr: Add support for any Ethertype

Message ID 20201028131807.3371-5-xie.he.0141@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: hdlc_fr: Add support for any Ethertype | expand

Commit Message

Xie He Oct. 28, 2020, 1:18 p.m. UTC
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.

Also add skb_reset_mac_header before we pass an skb (received on normal
PVC devices) to upper layers. Because we don't use header_ops for normal
PVC devices, we should hide the header from upper layer code in this case.

Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 76 ++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 25 deletions(-)

Comments

Willem de Bruijn Oct. 29, 2020, 5:23 p.m. UTC | #1
On Wed, Oct 28, 2020 at 6:58 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.
>
> Also add skb_reset_mac_header before we pass an skb (received on normal
> PVC devices) to upper layers. Because we don't use header_ops for normal
> PVC devices, we should hide the header from upper layer code in this case.

This should probably be a separate commit

> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>
> ---
>  drivers/net/wan/hdlc_fr.c | 76 ++++++++++++++++++++++++++-------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index 3639c2bfb141..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 */

Does it make sense to define a struct snap_hdr instead of manually
casting all these bytes?

> +               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) {


And macros or constant integers to self document these kinds of fields.
Xie He Oct. 29, 2020, 11:53 p.m. UTC | #2
On Thu, Oct 29, 2020 at 10:24 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > Also add skb_reset_mac_header before we pass an skb (received on normal
> > PVC devices) to upper layers. Because we don't use header_ops for normal
> > PVC devices, we should hide the header from upper layer code in this case.
>
> This should probably be a separate commit

OK. I'll make it a separate commit in the next version of the series. Thanks!

> Does it make sense to define a struct snap_hdr instead of manually
> casting all these bytes?

> And macros or constant integers to self document these kinds of fields.

Yes, we can define a struct snap_hdr, like this:

struct snap_hdr {
        u8 oui[3];
        __be16 pid;
} __packed;

And then the fr_snap_parse function could be like this:

static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
{
       struct snap_hdr *hdr = (struct snap_hdr *)skb->data;

       if (hdr->oui[0] == OUI_ETHERTYPE_1 &&
           hdr->oui[1] == OUI_ETHERTYPE_2 &&
           hdr->oui[2] == OUI_ETHERTYPE_3) {
               if (!pvc->main)
                       return -1;
               skb->dev = pvc->main;
               skb->protocol = hdr->pid; /* Ethertype */
               skb_pull(skb, 5);
               skb_reset_mac_header(skb);
               return 0;

       } else if (hdr->oui[0] == OUI_802_1_1 &&
                  hdr->oui[1] == OUI_802_1_2 &&
                  hdr->oui[2] == OUI_802_1_3) {
               if (hdr->pid == htons(PID_ETHER_WO_FCS)) {

Would this look cleaner?
Xie He Oct. 30, 2020, 12:49 a.m. UTC | #3
On Thu, Oct 29, 2020 at 4:53 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> > Does it make sense to define a struct snap_hdr instead of manually
> > casting all these bytes?
>
> > And macros or constant integers to self document these kinds of fields.
>
> Yes, we can define a struct snap_hdr, like this:
>
> struct snap_hdr {
>         u8 oui[3];
>         __be16 pid;
> } __packed;
>
> And then the fr_snap_parse function could be like this:
>
> static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
> {
>        struct snap_hdr *hdr = (struct snap_hdr *)skb->data;
>
>        if (hdr->oui[0] == OUI_ETHERTYPE_1 &&
>            hdr->oui[1] == OUI_ETHERTYPE_2 &&
>            hdr->oui[2] == OUI_ETHERTYPE_3) {
>                if (!pvc->main)
>                        return -1;
>                skb->dev = pvc->main;
>                skb->protocol = hdr->pid; /* Ethertype */
>                skb_pull(skb, 5);
>                skb_reset_mac_header(skb);
>                return 0;
>
>        } else if (hdr->oui[0] == OUI_802_1_1 &&
>                   hdr->oui[1] == OUI_802_1_2 &&
>                   hdr->oui[2] == OUI_802_1_3) {
>                if (hdr->pid == htons(PID_ETHER_WO_FCS)) {
>
> Would this look cleaner?

Actually I don't think this is significantly cleaner than the previous
version of code. A reader of this code may still wonder what are the
values of all these macros, and he/she may still want to look up the
values of them. The comment in the previous version of code has made
the meaning of these values very clear, and the reader of the code
would not need to go to another place of this file to find the values.

The struct snap_hdr eliminates a cast, but only one cast. So it might
not be very necessary, either. Introducing this struct also makes the
reader need to go to another place of this file to look up the
definition of this struct. So it does not significantly improve the
readability (IMHO).
Willem de Bruijn Oct. 30, 2020, 12:50 a.m. UTC | #4
On Thu, Oct 29, 2020 at 7:53 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Thu, Oct 29, 2020 at 10:24 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > > Also add skb_reset_mac_header before we pass an skb (received on normal
> > > PVC devices) to upper layers. Because we don't use header_ops for normal
> > > PVC devices, we should hide the header from upper layer code in this case.
> >
> > This should probably be a separate commit
>
> OK. I'll make it a separate commit in the next version of the series. Thanks!
>
> > Does it make sense to define a struct snap_hdr instead of manually
> > casting all these bytes?
>
> > And macros or constant integers to self document these kinds of fields.
>
> Yes, we can define a struct snap_hdr, like this:
>
> struct snap_hdr {
>         u8 oui[3];
>         __be16 pid;
> } __packed;
>
> And then the fr_snap_parse function could be like this:
>
> static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
> {
>        struct snap_hdr *hdr = (struct snap_hdr *)skb->data;
>
>        if (hdr->oui[0] == OUI_ETHERTYPE_1 &&
>            hdr->oui[1] == OUI_ETHERTYPE_2 &&
>            hdr->oui[2] == OUI_ETHERTYPE_3) {
>                if (!pvc->main)
>                        return -1;
>                skb->dev = pvc->main;
>                skb->protocol = hdr->pid; /* Ethertype */
>                skb_pull(skb, 5);
>                skb_reset_mac_header(skb);
>                return 0;
>
>        } else if (hdr->oui[0] == OUI_802_1_1 &&
>                   hdr->oui[1] == OUI_802_1_2 &&
>                   hdr->oui[2] == OUI_802_1_3) {
>                if (hdr->pid == htons(PID_ETHER_WO_FCS)) {
>
> Would this look cleaner?

Oh, right. The OUI being 3 bytes introduces masking and endianness
issues if casting to a wider type. Similar to IPv6 flowlabel handling,
for instance.

There is a lot of OUI code, such as cea_db_is_hdmi_forum_vsdb. But no
standard way of going about this. Some does byte-by-byte, some memcmp,
some masks.

The existing is probably as concise and readable as anything, and we
don't really care about a few extra branches here. Never mind this
suggestion.
Willem de Bruijn Oct. 30, 2020, 12:53 a.m. UTC | #5
On Thu, Oct 29, 2020 at 8:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Thu, Oct 29, 2020 at 4:53 PM Xie He <xie.he.0141@gmail.com> wrote:
> >
> > > Does it make sense to define a struct snap_hdr instead of manually
> > > casting all these bytes?
> >
> > > And macros or constant integers to self document these kinds of fields.
> >
> > Yes, we can define a struct snap_hdr, like this:
> >
> > struct snap_hdr {
> >         u8 oui[3];
> >         __be16 pid;
> > } __packed;
> >
> > And then the fr_snap_parse function could be like this:
> >
> > static int fr_snap_parse(struct sk_buff *skb, struct pvc_device *pvc)
> > {
> >        struct snap_hdr *hdr = (struct snap_hdr *)skb->data;
> >
> >        if (hdr->oui[0] == OUI_ETHERTYPE_1 &&
> >            hdr->oui[1] == OUI_ETHERTYPE_2 &&
> >            hdr->oui[2] == OUI_ETHERTYPE_3) {
> >                if (!pvc->main)
> >                        return -1;
> >                skb->dev = pvc->main;
> >                skb->protocol = hdr->pid; /* Ethertype */
> >                skb_pull(skb, 5);
> >                skb_reset_mac_header(skb);
> >                return 0;
> >
> >        } else if (hdr->oui[0] == OUI_802_1_1 &&
> >                   hdr->oui[1] == OUI_802_1_2 &&
> >                   hdr->oui[2] == OUI_802_1_3) {
> >                if (hdr->pid == htons(PID_ETHER_WO_FCS)) {
> >
> > Would this look cleaner?
>
> Actually I don't think this is significantly cleaner than the previous
> version of code. A reader of this code may still wonder what are the
> values of all these macros, and he/she may still want to look up the
> values of them. The comment in the previous version of code has made
> the meaning of these values very clear, and the reader of the code
> would not need to go to another place of this file to find the values.
>
> The struct snap_hdr eliminates a cast, but only one cast. So it might
> not be very necessary, either. Introducing this struct also makes the
> reader need to go to another place of this file to look up the
> definition of this struct. So it does not significantly improve the
> readability (IMHO).

Thanks for coding up an example. Yes, seeing the alternative, I agree.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 3639c2bfb141..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)
 {
@@ -935,6 +974,7 @@  static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IP);
+		skb_reset_mac_header(skb);
 
 	} else if (data[3] == NLPID_IPV6) {
 		if (!pvc->main)
@@ -942,35 +982,21 @@  static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		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);
-			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);