[RFC] can: isotp TX-path cleanups

[RFC] can: isotp TX-path cleanups

[Marc Kleine-Budde]

Hello

we've noticed that "candump -x" on normal CAN ISO-TP traffic shows:

| (2021-02-18 14:52:51.643904)  ca0  TX B E  713   [8]  20 0D 0E 0F AA AA AA AA

Note here the "B" and "E" flags are set. Another possibility is to use
skb_put_zero() instead of skb_put(), but with a bigger overhead. A 3.
option is to only memset() the non-data part of the struct canfd_frame.

regards,
Marc

[net-rfc 1/2] can: isotp: isotp_setsockopt(): only allow to set low level TX flags for CAN-FD

[Marc Kleine-Budde]

CAN-FD frames have struct canfd_frame::flags, while classic CAN frames
don't.

This patch refuses to set TX flags (struct
can_isotp_ll_options::tx_flags) on non CAN-FD isotp sockets.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/isotp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 3ef7f78e553b..e32d446c121e 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1228,7 +1228,8 @@ static int isotp_setsockopt(struct socket *sock, int level, int optname,
 			if (ll.mtu != CAN_MTU && ll.mtu != CANFD_MTU)
 				return -EINVAL;
 
-			if (ll.mtu == CAN_MTU && ll.tx_dl > CAN_MAX_DLEN)
+			if (ll.mtu == CAN_MTU &&
+			    (ll.tx_dl > CAN_MAX_DLEN || ll.tx_flags != 0))
 				return -EINVAL;
 
 			memcpy(&so->ll, &ll, sizeof(ll));

base-commit: 3af409ca278d4a8d50e91f9f7c4c33b175645cf3
-- 
2.30.0

[net-rfc 2/2] can: isotp: TX-path: ensure that CAN frame flags are initialized

[Marc Kleine-Budde]

The previous patch ensures that the TX flags (struct
can_isotp_ll_options::tx_flags) are 0 for classic CAN frames or a user
configured value for CAN-FD frames.

This patch sets the CAN frames flags unconditionally to the ISO-TP TX
flags, so that they are initialized to a proper value. Otherwise when
running "candump -x" on a classical CAN ISO-TP stream shows wrongly
set "B" and "E" flags.

| $ candump any,0:0,#FFFFFFFF -extA
| [...]
| can0  TX B E  713   [8]  2B 0A 0B 0C 0D 0E 0F 00
| can0  TX B E  713   [8]  2C 01 02 03 04 05 06 07
| can0  TX B E  713   [8]  2D 08 09 0A 0B 0C 0D 0E
| can0  TX B E  713   [8]  2E 0F 00 01 02 03 04 05

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/isotp.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index e32d446c121e..430976485d95 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -215,8 +215,7 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
 	if (ae)
 		ncf->data[0] = so->opt.ext_address;
 
-	if (so->ll.mtu == CANFD_MTU)
-		ncf->flags = so->ll.tx_flags;
+	ncf->flags = so->ll.tx_flags;
 
 	can_send_ret = can_send(nskb, 1);
 	if (can_send_ret)
@@ -790,8 +789,7 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
 		so->tx.sn %= 16;
 		so->tx.bs++;
 
-		if (so->ll.mtu == CANFD_MTU)
-			cf->flags = so->ll.tx_flags;
+		cf->flags = so->ll.tx_flags;
 
 		skb->dev = dev;
 		can_skb_set_owner(skb, sk);
@@ -939,8 +937,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	}
 
 	/* send the first or only CAN frame */
-	if (so->ll.mtu == CANFD_MTU)
-		cf->flags = so->ll.tx_flags;
+	cf->flags = so->ll.tx_flags;
 
 	skb->dev = dev;
 	skb->sk = sk;
-- 
2.30.0

Re: [RFC] can: isotp TX-path cleanups

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On 18.02.2021 22:54:32, Marc Kleine-Budde wrote:
> Hello
> 
> we've noticed that "candump -x" on normal CAN ISO-TP traffic shows:
> 
> | (2021-02-18 14:52:51.643904)  ca0  TX B E  713   [8]  20 0D 0E 0F AA AA AA AA
> 
> Note here the "B" and "E" flags are set. Another possibility is to use
> skb_put_zero() instead of skb_put(), but with a bigger overhead. A 3.
> option is to only memset() the non-data part of the struct canfd_frame.

Oliver, any opinion this?

I think I'll include this in my next pull request to net/master.

regards,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]