[PATCH net-next v4 0/5] net: hdlc_fr: Improve fr_rx and add support for any Ethertype

[PATCH net-next v4 0/5] net: hdlc_fr: Improve fr_rx and add support for any Ethertype

[Xie He]

The main purpose of this series is the last patch. The previous 4 patches
are just code clean-ups so that the last patch will not make the code too
messy. The patches must be applied in sequence.

The receiving code of this driver doesn't support arbitrary Ethertype
values. It only recognizes a few known Ethertypes when receiving and drops
skbs with other Ethertypes.

However, the standard document RFC 2427 allows Frame Relay to support any
Ethertype values. This series adds support for this.

Change from v3:
Split the 4th patch out of the last patch.
Improve the commit message of the 1st patch to explicitly state that the
stats.rx_dropped count is also increased after "goto rx_error".

Change from v2:
Small fix to the commit message of the 2nd and 3rd patch

Change from v1:
Small fix to the commit message of the 2nd patch

Xie He (5):
  net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames
  net: hdlc_fr: Change the use of "dev" in fr_rx to make the code
    cleaner
  net: hdlc_fr: Improve the initial checks when we receive an skb
  net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC
    devices
  net: hdlc_fr: Add support for any Ethertype

 drivers/net/wan/hdlc_fr.c | 119 +++++++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 46 deletions(-)

-- 
2.27.0

[PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

1.
When the fr_rx function drops a received frame (because the protocol type
is not supported, or because the PVC virtual device that corresponds to
the DLCI number and the protocol type doesn't exist), the function frees
the skb and returns.

The code for freeing the skb and returning is repeated several times, this
patch uses "goto rx_drop" to replace them so that the code looks cleaner.

2.
Add code to increase the stats.rx_dropped count whenever we drop a frame.
Increase the stats.rx_dropped count both after "goto rx_drop" and after
"goto rx_error" because I think we should increase this value whenever an
skb is dropped.

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 | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 409e5a7ad8e2..c774eff44534 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -904,8 +904,7 @@ static int fr_rx(struct sk_buff *skb)
 		netdev_info(frad, "No PVC for received frame's DLCI %d\n",
 			    dlci);
 #endif
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (pvc->state.fecn != fh->fecn) {
@@ -963,14 +962,12 @@ static int fr_rx(struct sk_buff *skb)
 		default:
 			netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
 				    oui, pid);
-			dev_kfree_skb_any(skb);
-			return NET_RX_DROP;
+			goto rx_drop;
 		}
 	} else {
 		netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
 			    data[3], skb->len);
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (dev) {
@@ -982,12 +979,13 @@ static int fr_rx(struct sk_buff *skb)
 		netif_rx(skb);
 		return NET_RX_SUCCESS;
 	} else {
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
- rx_error:
+rx_error:
 	frad->stats.rx_errors++; /* Mark error */
+rx_drop:
+	frad->stats.rx_dropped++;
 	dev_kfree_skb_any(skb);
 	return NET_RX_DROP;
 }
-- 
2.27.0

[PATCH net-next v4 2/5] net: hdlc_fr: Change the use of "dev" in fr_rx to make the code cleaner

[Xie He]

The eth_type_trans function is called when we receive frames carrying
Ethernet frames. This function expects a non-NULL pointer as an argument,
and assigns it directly to skb->dev.

However, the code handling other types of frames first assigns the pointer
to "dev", and then at the end checks whether the value is NULL, and if it
is not NULL, assigns it to skb->dev.

The two flows are different. Mixing them in this function makes the code
messy. It's better that we convert the second flow to align with how
eth_type_trans does things.

So this patch changes the code to: first make sure the pointer is not
NULL, then assign it directly to skb->dev. "dev" is no longer needed until
the end where we use it to update stats.

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 | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index c774eff44534..ac65f5c435ef 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -880,7 +880,7 @@ static int fr_rx(struct sk_buff *skb)
 	u8 *data = skb->data;
 	u16 dlci;
 	struct pvc_device *pvc;
-	struct net_device *dev = NULL;
+	struct net_device *dev;
 
 	if (skb->len <= 4 || fh->ea1 || data[2] != FR_UI)
 		goto rx_error;
@@ -930,13 +930,17 @@ static int fr_rx(struct sk_buff *skb)
 	}
 
 	if (data[3] == NLPID_IP) {
+		if (!pvc->main)
+			goto rx_drop;
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
-		dev = pvc->main;
+		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IP);
 
 	} else if (data[3] == NLPID_IPV6) {
+		if (!pvc->main)
+			goto rx_drop;
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
-		dev = pvc->main;
+		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IPV6);
 
 	} else if (skb->len > 10 && data[3] == FR_PAD &&
@@ -950,13 +954,16 @@ static int fr_rx(struct sk_buff *skb)
 		case ETH_P_IPX:
 		case ETH_P_IP:	/* a long variant */
 		case ETH_P_IPV6:
-			dev = pvc->main;
+			if (!pvc->main)
+				goto rx_drop;
+			skb->dev = pvc->main;
 			skb->protocol = htons(pid);
 			break;
 
 		case 0x80C20007: /* bridged Ethernet frame */
-			if ((dev = pvc->ether) != NULL)
-				skb->protocol = eth_type_trans(skb, dev);
+			if (!pvc->ether)
+				goto rx_drop;
+			skb->protocol = eth_type_trans(skb, pvc->ether);
 			break;
 
 		default:
@@ -970,17 +977,13 @@ static int fr_rx(struct sk_buff *skb)
 		goto rx_drop;
 	}
 
-	if (dev) {
-		dev->stats.rx_packets++; /* PVC traffic */
-		dev->stats.rx_bytes += skb->len;
-		if (pvc->state.becn)
-			dev->stats.rx_compressed++;
-		skb->dev = dev;
-		netif_rx(skb);
-		return NET_RX_SUCCESS;
-	} else {
-		goto rx_drop;
-	}
+	dev = skb->dev;
+	dev->stats.rx_packets++; /* PVC traffic */
+	dev->stats.rx_bytes += skb->len;
+	if (pvc->state.becn)
+		dev->stats.rx_compressed++;
+	netif_rx(skb);
+	return NET_RX_SUCCESS;
 
 rx_error:
 	frad->stats.rx_errors++; /* Mark error */
-- 
2.27.0

[PATCH net-next v4 3/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Xie He]

1.
Change the skb->len check from "<= 4" to "< 4".
At first we only need to ensure a 4-byte header is present. We indeed
normally need the 5th byte, too, but it'd be more logical and cleaner
to check its existence when we actually need it.

2.
Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
the second address byte is the final address byte. We only support the
case where the address length is 2 bytes.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index ac65f5c435ef..3639c2bfb141 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -882,7 +882,7 @@ static int fr_rx(struct sk_buff *skb)
 	struct pvc_device *pvc;
 	struct net_device *dev;
 
-	if (skb->len <= 4 || fh->ea1 || data[2] != FR_UI)
+	if (skb->len < 4 || fh->ea1 || !fh->ea2 || data[2] != FR_UI)
 		goto rx_error;
 
 	dlci = q922_to_dlci(skb->data);
-- 
2.27.0

[PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Xie He]

When an skb is received on a normal (non-Ethernet-emulating) PVC device,
call skb_reset_mac_header before we pass it to upper layers.

This is because normal PVC devices don't have header_ops, so any header we
have would not be visible to upper layer code when sending, so the header
shouldn't be visible to upper layer code when receiving, either.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 3639c2bfb141..9a37575686b9 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -935,6 +935,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,6 +943,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_IPV6);
+		skb_reset_mac_header(skb);
 
 	} else if (skb->len > 10 && data[3] == FR_PAD &&
 		   data[4] == NLPID_SNAP && data[5] == FR_PAD) {
@@ -958,6 +960,7 @@ static int fr_rx(struct sk_buff *skb)
 				goto rx_drop;
 			skb->dev = pvc->main;
 			skb->protocol = htons(pid);
+			skb_reset_mac_header(skb);
 			break;
 
 		case 0x80C20007: /* bridged Ethernet frame */
-- 
2.27.0

[PATCH net-next v4 5/5] net: hdlc_fr: Add support for any Ethertype

[Xie He]

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_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);
-- 
2.27.0

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Willem de Bruijn]

On Thu, Oct 29, 2020 at 10:33 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> When an skb is received on a normal (non-Ethernet-emulating) PVC device,
> call skb_reset_mac_header before we pass it to upper layers.
>
> This is because normal PVC devices don't have header_ops, so any header we
> have would not be visible to upper layer code when sending, so the header
> shouldn't be visible to upper layer code when receiving, either.
>
> 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>

Acked-by: Willem de Bruijn <willemb@google.com>

Should this go to net if a bugfix though?

Re: [PATCH net-next v4 3/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Willem de Bruijn]

On Thu, Oct 29, 2020 at 10:33 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1.
> Change the skb->len check from "<= 4" to "< 4".
> At first we only need to ensure a 4-byte header is present. We indeed
> normally need the 5th byte, too, but it'd be more logical and cleaner
> to check its existence when we actually need it.
>
> 2.
> Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> the second address byte is the final address byte. We only support the
> case where the address length is 2 bytes.

Can you elaborate a bit for readers not intimately familiar with the codebase?

Is there something in the following code that has this implicit
assumption on 2-byte address lengths?

Re: [PATCH net-next v4 5/5] net: hdlc_fr: Add support for any Ethertype

[Willem de Bruijn]

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
>

Re: [PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Willem de Bruijn]

On Thu, Oct 29, 2020 at 10:31 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1.
> When the fr_rx function drops a received frame (because the protocol type
> is not supported, or because the PVC virtual device that corresponds to
> the DLCI number and the protocol type doesn't exist), the function frees
> the skb and returns.
>
> The code for freeing the skb and returning is repeated several times, this
> patch uses "goto rx_drop" to replace them so that the code looks cleaner.
>
> 2.
> Add code to increase the stats.rx_dropped count whenever we drop a frame.
> Increase the stats.rx_dropped count both after "goto rx_drop" and after
> "goto rx_error" because I think we should increase this value whenever an
> skb is dropped.

In general we try to avoid changing counter behavior like that, as
existing users
may depend on current behavior, e.g., in dashboards or automated monitoring.

I don't know how realistic that is in this specific case, no strong
objections. Use
good judgment.

Re: [PATCH net-next v4 3/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Xie He]

On Fri, Oct 30, 2020 at 9:31 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> > Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> > the second address byte is the final address byte. We only support the
> > case where the address length is 2 bytes.
>
> Can you elaborate a bit for readers not intimately familiar with the codebase?
>
> Is there something in the following code that has this implicit
> assumption on 2-byte address lengths?

Yes, the address length must be 2 bytes, otherwise the 3rd and 4th
bytes would not be the control and protocol fields as we assumed in
the code.

The frame format is specified in RFC 2427
(https://tools.ietf.org/html/rfc2427). We can see the overall frame
format on Page 3. If the address length is longer than 2 bytes, all
the following fields will be shifted behind.

Re: [PATCH net-next v4 5/5] net: hdlc_fr: Add support for any Ethertype

[Xie He]

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.

Re: [PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

On Fri, Oct 30, 2020 at 9:35 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> In general we try to avoid changing counter behavior like that, as
> existing users
> may depend on current behavior, e.g., in dashboards or automated monitoring.
>
> I don't know how realistic that is in this specific case, no strong
> objections. Use
> good judgment.

Originally this function only increases stats.rx_dropped only when
there's a memory squeeze. I don't know the specification for the
meaning of stats.rx_dropped, but as I understand it indicates a frame
is dropped. This is why I wanted to increase it whenever we drop a
frame.

Originally this function drops a frame silently if the PVC virtual
device that corresponds to the DLCI number and the protocol type
doesn't exist. I think we may at least need some way to note this.
Originally this function drops a frame with a kernel info message
printed if the protocol type is not supported. I think this is a bad
way because if the other end continuously sends us a lot of frames
with unsupported protocol types, our kernel message log will be
overwhelmed.

I don't know how important it is to keep backwards compatibility. I
usually don't consider this too much. But I can drop this change if we
really want to keep the counter behavior unchanged. I think changing
it is better if we don't consider backwards compatibility.

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Xie He]

On Fri, Oct 30, 2020 at 9:30 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Acked-by: Willem de Bruijn <willemb@google.com>

Thanks for your ack!

> Should this go to net if a bugfix though?

Yes, this should theoretically be a bug fix. But I didn't think this
was an important fix, because af_packet.c already had workarounds for
this issue for all drivers that didn't have header_ops. We can
separate this patch to make it go to "net" though, but I'm afraid that
this will cause merging conflicts between "net" and "net-next".

Re: [PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 4:02 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 9:35 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > In general we try to avoid changing counter behavior like that, as
> > existing users
> > may depend on current behavior, e.g., in dashboards or automated monitoring.
> >
> > I don't know how realistic that is in this specific case, no strong
> > objections. Use
> > good judgment.
>
> Originally this function only increases stats.rx_dropped only when
> there's a memory squeeze. I don't know the specification for the
> meaning of stats.rx_dropped, but as I understand it indicates a frame
> is dropped. This is why I wanted to increase it whenever we drop a
> frame.

Jakub recently made stats behavior less ambiguous, in commit
0db0c34cfbc9 ("net: tighten the definition of interface statistics").

That said, it's not entirely clear whether rx_dropped would be allowed
to include rx_errors.

My hunch is that it shouldn't. A quick scan of devices did quickly
show at least one example where it does: macvlan. But I expect that to
be an outlier.

> Originally this function drops a frame silently if the PVC virtual
> device that corresponds to the DLCI number and the protocol type
> doesn't exist. I think we may at least need some way to note this.
> Originally this function drops a frame with a kernel info message
> printed if the protocol type is not supported. I think this is a bad
> way because if the other end continuously sends us a lot of frames
> with unsupported protocol types, our kernel message log will be
> overwhelmed.
>
> I don't know how important it is to keep backwards compatibility. I
> usually don't consider this too much. But I can drop this change if we
> really want to keep the counter behavior unchanged. I think changing
> it is better if we don't consider backwards compatibility.

Please do always consider backward compatibility. In this case, I
don't think that the behavioral change is needed for the core of the
patch (changing control flow).

Re: [PATCH net-next v4 3/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 3:21 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 9:31 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > > Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> > > the second address byte is the final address byte. We only support the
> > > case where the address length is 2 bytes.
> >
> > Can you elaborate a bit for readers not intimately familiar with the codebase?
> >
> > Is there something in the following code that has this implicit
> > assumption on 2-byte address lengths?
>
> Yes, the address length must be 2 bytes, otherwise the 3rd and 4th
> bytes would not be the control and protocol fields as we assumed in
> the code.
>
> The frame format is specified in RFC 2427
> (https://tools.ietf.org/html/rfc2427). We can see the overall frame
> format on Page 3. If the address length is longer than 2 bytes, all
> the following fields will be shifted behind.

Thanks for that context. If it's not captured in the code, it would be
great to include in the commit message.

From a quick scan, RFC 2427 does not appear to actually define the
Q.922 address. For that I ended up reading ITU-T doc "Q.922 : ISDN
data link layer specification for frame mode bearer services", section
3.2.

Re: [PATCH net-next v4 5/5] net: hdlc_fr: Add support for any Ethertype

[Willem de Bruijn]

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.

Re: [PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

[Xie He]

On Fri, Oct 30, 2020 at 2:12 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jakub recently made stats behavior less ambiguous, in commit
> 0db0c34cfbc9 ("net: tighten the definition of interface statistics").
>
> That said, it's not entirely clear whether rx_dropped would be allowed
> to include rx_errors.
>
> My hunch is that it shouldn't. A quick scan of devices did quickly
> show at least one example where it does: macvlan. But I expect that to
> be an outlier.

OK. Thanks for the information.

> Please do always consider backward compatibility. In this case, I
> don't think that the behavioral change is needed for the core of the
> patch (changing control flow).

OK. I'll drop the change about stats.rx_dropped. Thanks.

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 4:08 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 9:30 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Acked-by: Willem de Bruijn <willemb@google.com>
>
> Thanks for your ack!
>
> > Should this go to net if a bugfix though?
>
> Yes, this should theoretically be a bug fix. But I didn't think this
> was an important fix, because af_packet.c already had workarounds for
> this issue for all drivers that didn't have header_ops. We can
> separate this patch to make it go to "net" though, but I'm afraid that
> this will cause merging conflicts between "net" and "net-next".

Yes, it might require holding off the other patches until net is
merged into net-next.

Packet sockets are likely not the only way these packets are received?
It changes mac_len as computed in __netif_receive_skb_core.

If there is no real bug that is fixed, net-next is fine.

Re: [PATCH net-next v4 3/5] net: hdlc_fr: Improve the initial checks when we receive an skb

[Xie He]

On Fri, Oct 30, 2020 at 2:20 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Thanks for that context. If it's not captured in the code, it would be
> great to include in the commit message.

OK. I'll update the commit message.

> From a quick scan, RFC 2427 does not appear to actually define the
> Q.922 address. For that I ended up reading ITU-T doc "Q.922 : ISDN
> data link layer specification for frame mode bearer services", section
> 3.2.

Yeah. Thanks for posting the name of the document. It's good to see
ITU's documents are published in multiple languages because this feels
more international :)

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Xie He]

On Fri, Oct 30, 2020 at 2:28 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Yes, it might require holding off the other patches until net is
> merged into net-next.
>
> Packet sockets are likely not the only way these packets are received?
> It changes mac_len as computed in __netif_receive_skb_core.

I looked at __netif_receive_skb_core. I didn't see it computing mac_len?

I thought only AF_PACKET/RAW sockets would need this information
because other upper layers would not care about what happened in L2.

I see mac_len is computed in netif_receive_generic_xdp. I'm not clear
about the reason why it calculates it. But it seems that it considers
the L2 header as an Ethernet header, which is incorrect for this
driver.

> If there is no real bug that is fixed, net-next is fine.

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Willem de Bruijn]

On Fri, Oct 30, 2020 at 5:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 2:28 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Yes, it might require holding off the other patches until net is
> > merged into net-next.
> >
> > Packet sockets are likely not the only way these packets are received?
> > It changes mac_len as computed in __netif_receive_skb_core.
>
> I looked at __netif_receive_skb_core. I didn't see it computing mac_len?

It's indirect:

        skb_reset_network_header(skb);
        if (!skb_transport_header_was_set(skb))
                skb_reset_transport_header(skb);
        skb_reset_mac_len(skb);

> I thought only AF_PACKET/RAW sockets would need this information
> because other upper layers would not care about what happened in L2.

I think that's a reasonable assumption. I don't have a good
counterexample ready. Specific to this case, it seems to have been
working with no one complaining so far ;)

> I see mac_len is computed in netif_receive_generic_xdp. I'm not clear
> about the reason why it calculates it. But it seems that it considers
> the L2 header as an Ethernet header, which is incorrect for this
> driver.
>
> > If there is no real bug that is fixed, net-next is fine.

Re: [PATCH net-next v4 4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

[Xie He]

On Fri, Oct 30, 2020 at 3:22 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> It's indirect:
>
>         skb_reset_network_header(skb);
>         if (!skb_transport_header_was_set(skb))
>                 skb_reset_transport_header(skb);
>         skb_reset_mac_len(skb);

Oh. I see. skb_reset_mac_len would set skb->mac_len. Not sure where
skb->mac_len would be used though.

> > I thought only AF_PACKET/RAW sockets would need this information
> > because other upper layers would not care about what happened in L2.
>
> I think that's a reasonable assumption. I don't have a good
> counterexample ready. Specific to this case, it seems to have been
> working with no one complaining so far ;)

Yeah. It seems to me that a lot of drivers (without header_ops) have
this problem. The comment in af_packet.c before my commit b79a80bd6dd8
also indicated this problem was widespread. It seemed to not cause any
issues.