[PATCH v4 0/4] can: add support for ETAS ES58X CAN USB

[PATCH v4 0/4] can: add support for ETAS ES58X CAN USB

[Vincent Mailhol]

The purpose of this patch series is to introduce a new CAN USB
driver to support ETAS USB interfaces (ES58X series).

During development, issues in drivers/net/can/dev.c were discovered,
the fix for those issues are included in this patch series.

We also propose to add one helper functions in include/linux/can/dev.h
which we think can benefit other drivers: get_can_len().

*Side notes*: scripts/checkpatch.pl returns 4 'checks' findings in
[PATCH 4/4]. All those findings are of type: "Macro argument reuse 'x'
possible side-effects?".  Those arguments reuse are actually made by
calling either __stringify() or sizeof_field() which are both
pre-processor constant. Furthermore, those macro are never called with
arguments sensible to side-effects. So no actual side effect would
occur.

Changes in v4:
  - Remove from the series the patches with have already been merged
  into net-next.
Reference: https://lkml.org/lkml/2020/10/4/78
Reference: https://lkml.org/lkml/2020/10/5/355
  - Modify [PATCH 4/4] according to comments from Marc.
Reference: https://lkml.org/lkml/2020/10/4/80)

Changes in v3:
  - Added one additional patch: [PATCH v3 2/7] can: dev: fix type of
 get_can_dlc() and get_canfd_dlc() macros.
  - Make get_can_len() return u8 and make the skb const in PATCH 3/7.
  - Remove all the calls to likely() and unlikely() in PATCH 6/7.

Changes in v2:
  - Fixed -W1 warnings in PATCH 6/7 (v1 was tested with GCC -WExtra
  but not with -W1).
  - Added lsusb -v information in PATCH 7/7 and rephrased the comment.
  - Take care to put everyone in CC of each of the patch of the series
  (sorry for the mess in v1...)

Vincent Mailhol (4):
  can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ
    context
  can: dev: add a helper function to get the correct length of Classical
    frames
  can: dev: __can_get_echo_skb(): fix the return length
  can: usb: etas_es58X: add support for ETAS ES58X CAN USB interfaces

 drivers/net/can/dev.c                       |   13 +-
 drivers/net/can/usb/Kconfig                 |    9 +
 drivers/net/can/usb/Makefile                |    1 +
 drivers/net/can/usb/etas_es58x/Makefile     |    3 +
 drivers/net/can/usb/etas_es58x/es581_4.c    |  556 ++++
 drivers/net/can/usb/etas_es58x/es581_4.h    |  209 ++
 drivers/net/can/usb/etas_es58x/es58x_core.c | 2639 +++++++++++++++++++
 drivers/net/can/usb/etas_es58x/es58x_core.h |  704 +++++
 drivers/net/can/usb/etas_es58x/es58x_fd.c   |  657 +++++
 drivers/net/can/usb/etas_es58x/es58x_fd.h   |  241 ++
 include/linux/can/dev.h                     |   23 +
 11 files changed, 5048 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/can/usb/etas_es58x/Makefile
 create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.c
 create mode 100644 drivers/net/can/usb/etas_es58x/es581_4.h
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.c
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_core.h
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.c
 create mode 100644 drivers/net/can/usb/etas_es58x/es58x_fd.h

-- 
2.26.2

[PATCH v4 1/4] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context

[Vincent Mailhol]

If a driver calls can_get_echo_skb() during a hardware IRQ (which is
often, but not always, the case), the 'WARN_ON(in_irq)' in
net/core/skbuff.c#skb_release_head_state() might be triggered, under
network congestion circumstances, together with the potential risk of
a NULL pointer dereference.

The root cause of this issue is the call to kfree_skb() instead of
dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().

This patch prevents the skb to be freed within the call to netif_rx()
by incrementing its reference count with skb_get(). The skb is finally
freed by one of the in-irq-context safe functions:
dev_consume_skb_any() or dev_kfree_skb_any().  The "any" version is
used because some drivers might call can_get_echo_skb() in a normal
context.

The reason for this issue to occur is that initially, in the core
network stack, loopback skb were not supposed to be received in
hardware IRQ context. The CAN stack is an exeption.

This bug was previously reported back in 2017 in [1] but the proposed
patch never got accepted.

While [1] directly modifies net/core/dev.c, we try to propose here a
smoother modification local to CAN network stack (the assumption
behind is that only CAN devices are affected by this issue).

[1] https://patchwork.ozlabs.org/patch/835236/

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v3 and v4: None

Changes in v2:
 - Minor changes of link format in the changelog.
---
 drivers/net/can/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b70ded3760f2..73cfcd7e9517 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 	if (!skb)
 		return 0;
 
-	netif_rx(skb);
+	skb_get(skb);
+	if (netif_rx(skb) == NET_RX_SUCCESS)
+		dev_consume_skb_any(skb);
+	else
+		dev_kfree_skb_any(skb);
 
 	return len;
 }
-- 
2.26.2

[PATCH v4 2/4] can: dev: add a helper function to get the correct length of Classical frames

[Vincent Mailhol]

In classical CAN, the length of the data (i.e. CAN payload) is not
always equal to the DLC! If the frame is a Remote Transmission Request
(RTR), data length is always zero regardless of DLC value and else, if
the DLC is greater than 8, the length is 8. Contrary to common belief,
ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow DLCs greater than 8
for Classical Frames and specifies that those DLCs shall indicate that
the data field is 8 bytes long.

Above facts are widely unknown and so many developpers uses the "len"
field of "struct canfd_frame" to get the length of classical CAN
frames: this is incorrect!

This patch introduces function get_can_len() which can be used in
remediation. The function takes the SKB as an input in order to be
able to determine if the frame is classical or FD.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v4: None

Changes in v3:
  - Make get_can_len() return u8.
  - Make the skb const.
Reference: https://lkml.org/lkml/2020/9/30/883

Changes in v2: None
---
 include/linux/can/dev.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 41ff31795320..d90890172d2a 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -192,6 +192,29 @@ u8 can_dlc2len(u8 can_dlc);
 /* map the sanitized data length to an appropriate data length code */
 u8 can_len2dlc(u8 len);
 
+/*
+ * get_can_len(skb) - get the length of the CAN payload.
+ *
+ * In classical CAN, the length of the data (i.e. CAN payload) is not
+ * always equal to the DLC! If the frame is a Remote Transmission
+ * Request (RTR), data length is always zero regardless of DLC value
+ * and else, if the DLC is greater than 8, the length is 8. Contrary
+ * to common belief, ISO 11898-1 Chapter 8.4.2.3 (DLC field) do allow
+ * DLCs greater than 8 for Classical Frames and specifies that those
+ * DLCs shall indicate that the data field is 8 bytes long.
+ */
+static inline u8 get_can_len(const struct sk_buff *skb)
+{
+	const struct canfd_frame *cf = (const struct canfd_frame *)skb->data;
+
+	if (can_is_canfd_skb(skb))
+		return min_t(u8, cf->len, CANFD_MAX_DLEN);
+	else if (cf->can_id & CAN_RTR_FLAG)
+		return 0;
+	else
+		return min_t(u8, cf->len, CAN_MAX_DLEN);
+}
+
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 				    unsigned int txqs, unsigned int rxqs);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-- 
2.26.2

[PATCH v4 3/4] can: dev: __can_get_echo_skb(): fix the return length

[Vincent Mailhol]

The length of Remote Transmission Request (RTR) frames is always 0
bytes. The DLC represents the requested length, not the actual length
of the RTR. But __can_get_echo_skb() returns the DLC value regardless.

Apply get_can_len() function to retrieve the correct length.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---

Changes in v2 to v4: None
---
 drivers/net/can/dev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 73cfcd7e9517..8f91d23c1ca7 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -507,14 +507,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
 	}
 
 	if (priv->echo_skb[idx]) {
-		/* Using "struct canfd_frame::len" for the frame
-		 * length is supported on both CAN and CANFD frames.
-		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
-		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
-		u8 len = cf->len;
 
-		*len_ptr = len;
+		*len_ptr = get_can_len(skb);
 		priv->echo_skb[idx] = NULL;
 
 		return skb;
-- 
2.26.2

Re: [PATCH v4 0/4] can: add support for ETAS ES58X CAN USB

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]

On 10/16/20 7:13 PM, Vincent Mailhol wrote:
> The purpose of this patch series is to introduce a new CAN USB
> driver to support ETAS USB interfaces (ES58X series).
> 
> During development, issues in drivers/net/can/dev.c were discovered,
> the fix for those issues are included in this patch series.
> 
> We also propose to add one helper functions in include/linux/can/dev.h
> which we think can benefit other drivers: get_can_len().
I applied patches 1-3 to linux-can, I've changed get_can_len() -> can_get_len()
to use a common can_ prefix for all CAN related functions.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]