can: raw: fix CAN FD frame transmissions over CAN XL devices

PATCH Superseded Oliver Hartkopp 2 participants 3 messages Jan 31, 2023

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2023-01-31 09:19 UTC #0 lore raw

PATCH v2 1/1 can: raw: fix CAN FD frame transmissions over CAN XL devices

A CAN XL device is always capable to process CAN FD frames. The former check when sending CAN FD frames relied on the existence of a CAN FD device and did not check for a CAN XL device that would be correct too.

With this patch the CAN FD feature is enabled automatically when CAN XL is switched on - and CAN FD cannot be switch off while CAN XL is enabled.

This precondition also leads to a clean up and reduction of checks in the hot path in raw_rcv() and raw_sendmsg().

Fixes 626332696d75 ("can: raw: add CAN XL support")

Signed-off-by Oliver Hartkopp <[email protected]>

---

V2: fixed typo: devive -> device

net/can/raw.c | 45 ++++++++++++++++++++++++++++++---------------

 1 file changed, 30 insertions(+), 15 deletions(-)

net/can/raw.c +30 ~~-15~~

index 81071cdb0301..c505d315a7a9 100644
@@ -130,11 +130,11 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
 	/* make sure to not pass oversized frames to the socket */
-	if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
+	if ((can_is_canfd_skb(oskb) && !ro->fd_frames) ||
 	    (can_is_canxl_skb(oskb) && !ro->xl_frames))
 		return;
 
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&

@@ -668,19 +668,27 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
 			return -EFAULT;
 
+		/* Enabling CAN XL includes CAN FD */
+		if (ro->xl_frames && !ro->fd_frames) {
+			ro->fd_frames = ro->xl_frames;
+			return -EINVAL;
+		}
 		break;
 
 	case CAN_RAW_XL_FRAMES:
 		if (optlen != sizeof(ro->xl_frames))
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->xl_frames, optval, optlen))
 			return -EFAULT;
 
+		/* Enabling CAN XL includes CAN FD */
+		if (ro->xl_frames)
+			ro->fd_frames = ro->xl_frames;
 		break;
 
 	case CAN_RAW_JOIN_FILTERS:
 		if (optlen != sizeof(ro->join_filters))
 			return -EINVAL;

@@ -784,10 +792,29 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 	if (copy_to_user(optval, val, len))
 		return -EFAULT;
 	return 0;
 }
 
+static bool raw_bad_txframe(struct raw_sock *ro, struct sk_buff *skb, int mtu)
+{
+	/* Classical CAN -> no checks for flags and device capabilities */
+	if (can_is_can_skb(skb))
+		return false;
+
+	/* CAN FD -> needs to be enabled and a CAN FD or CAN XL device */
+	if (can_is_canfd_skb(skb) && ro->fd_frames &&
+	    (mtu == CANFD_MTU || can_is_canxl_dev_mtu(mtu)))
+		return false;
+
+	/* CAN XL -> needs to be enabled and a CAN XL device */
+	if (can_is_canxl_skb(skb) && ro->xl_frames &&
+	    can_is_canxl_dev_mtu(mtu))
+		return false;
+
+	return true;
+}
+
 static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 {
 	struct sock *sk = sock->sk;
 	struct raw_sock *ro = raw_sk(sk);
 	struct sockcm_cookie sockc;

@@ -831,24 +858,12 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	err = memcpy_from_msg(skb_put(skb, size), msg, size);
 	if (err < 0)
 		goto free_skb;
 
 	err = -EINVAL;
-	if (ro->xl_frames && can_is_canxl_dev_mtu(dev->mtu)) {
-		/* CAN XL, CAN FD and Classical CAN */
-		if (!can_is_canxl_skb(skb) && !can_is_canfd_skb(skb) &&
-		    !can_is_can_skb(skb))
-			goto free_skb;
-	} else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
-		/* CAN FD and Classical CAN */
-		if (!can_is_canfd_skb(skb) && !can_is_can_skb(skb))
-			goto free_skb;
-	} else {
-		/* Classical CAN */
-		if (!can_is_can_skb(skb))
-			goto free_skb;
-	}
+	if (raw_bad_txframe(ro, skb, dev->mtu))
+		goto free_skb;
 
 	sockcm_init(&sockc, sk);
 	if (msg->msg_controllen) {
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (unlikely(err))

Signature · 1 line

2.39.0

Marc Kleine-Budde

maintainer

2023-01-31 11:06 UTC #1 lore raw

Quoted 37 lines · Oliver Hartkopp

A CAN XL device is always capable to process CAN FD frames. The former check when sending CAN FD frames relied on the existence of a CAN FD device and did not check for a CAN XL device that would be correct too.
With this patch the CAN FD feature is enabled automatically when CAN XL is switched on - and CAN FD cannot be switch off while CAN XL is enabled.
This precondition also leads to a clean up and reduction of checks in the hot path in raw_rcv() and raw_sendmsg().
Fixes 626332696d75 ("can: raw: add CAN XL support")
Signed-off-by Oliver Hartkopp <[email protected]>
---
V2: fixed typo: devive -> device
net/can/raw.c | 45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)
net/can/raw.c +1 -1
index 81071cdb0301..c505d315a7a9 100644
@@ -130,11 +130,11 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
 	/* make sure to not pass oversized frames to the socket */
-	if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
+	if ((can_is_canfd_skb(oskb) && !ro->fd_frames) ||
 	    (can_is_canxl_skb(oskb) && !ro->xl_frames))
 		return;
 
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&

net/can/raw.c static int raw_setsockopt(struct socket *sock, int level, int optname, · line 677

Quoted 10 lines

net/can/raw.c +5 -0

@@ -668,19 +668,27 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
 			return -EFAULT;
 
+		/* Enabling CAN XL includes CAN FD */
+		if (ro->xl_frames && !ro->fd_frames) {
+			ro->fd_frames = ro->xl_frames;
+			return -EINVAL;
+		}

This looks a bit weird. Maybe copy to a local variable first, then check, then assign ->fd_frames or exit with error (instead of rolling back)?

Marc

Signature · 4 lines

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 |

Oliver Hartkopp

author maintainer

2023-01-31 12:00 UTC #2 lore raw

Quoted 43 lines · Marc Kleine-Budde

Quoted 42 lines · Oliver Hartkopp
A CAN XL device is always capable to process CAN FD frames. The former check when sending CAN FD frames relied on the existence of a CAN FD device and did not check for a CAN XL device that would be correct too.
With this patch the CAN FD feature is enabled automatically when CAN XL is switched on - and CAN FD cannot be switch off while CAN XL is enabled.
This precondition also leads to a clean up and reduction of checks in the hot path in raw_rcv() and raw_sendmsg().
Fixes 626332696d75 ("can: raw: add CAN XL support")
Signed-off-by Oliver Hartkopp <[email protected]>
---
V2: fixed typo: devive -> device
net/can/raw.c | 45 ++++++++++++++++++++++++++++++---------------
  1 file changed, 30 insertions(+), 15 deletions(-)
net/can/raw.c +1 -1
index 81071cdb0301..c505d315a7a9 100644
@@ -130,11 +130,11 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
  	/* check the received tx sock reference */
  	if (!ro->recv_own_msgs && oskb->sk == sk)
  		return;
 
  	/* make sure to not pass oversized frames to the socket */
-	if ((can_is_canfd_skb(oskb) && !ro->fd_frames && !ro->xl_frames) ||
+	if ((can_is_canfd_skb(oskb) && !ro->fd_frames) ||
  	    (can_is_canxl_skb(oskb) && !ro->xl_frames))
  		return;
 
  	/* eliminate multiple filter matches for the same skb */
  	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
@@ -668,19 +668,27 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
  			return -EINVAL;
 
  		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
  			return -EFAULT;
 

Quoted 10 lines

net/can/raw.c static int raw_setsockopt(struct socket *sock, int level, int optname, · line 677
Quoted 5 lines
net/can/raw.c +5 -0
+		/* Enabling CAN XL includes CAN FD */
+		if (ro->xl_frames && !ro->fd_frames) {
+			ro->fd_frames = ro->xl_frames;
+			return -EINVAL;
+		}
This looks a bit weird. Maybe copy to a local variable first, then check, then assign ->fd_frames or exit with error (instead of rolling back)?

Changed in V4.

Thanks & BR,
Oliver