Re: [PATCH v2] selftests: can: add isotp frame-type tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: alignment and strict aliasing violation via byte array cast
- [Medium] selftests: tests fail unconditionally on kernels lacking CAN XL support
--

commit b7b667ec4c19dd41b74b7f4ee25cdee4ffaf4702
Author: Quchaosheng <quchaosheng000406@163.com>

selftests: can: add isotp frame-type tests

This commit adds an ISOTP frame-type test to verify that the CAN transport
channels are correctly separated. It ensures that CAN XL frames are not
mistaken for flow control frames on both Classic CAN and CAN FD channels.

> diff --git a/tools/testing/selftests/net/can/test_isotp_frame_type.c b/tools/testing/selftests/net/can/test_isotp_frame_type.c
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_isotp_frame_type.c
[ ... ]
> +static int iface_max_xl_len(const char *ifname)
> +{
> +	struct sockaddr_can addr = { .can_family = AF_CAN };
> +	unsigned char buf[CANXL_MTU];
> +	struct canxl_frame *cxl = (struct canxl_frame *)buf;

[Severity: Medium]
Is this cast safe on architectures that enforce strict alignment? Since buf
is an unsigned char array on the stack, it is only guaranteed to be 1-byte
aligned. Casting it to struct canxl_frame * and writing to 32-bit and
16-bit fields like cxl->prio and cxl->len could lead to unaligned memory
access crashes. Also, does this violate strict aliasing rules, which are
enabled by default in the tools/ directory?

> +	int s, one = 1, ret = -1;
[ ... ]
> +static int inject_xl(int tx, unsigned int frame_len)
> +{
> +	unsigned char buf[CANXL_MTU];
> +	struct canxl_frame *cxl = (struct canxl_frame *)buf;

[Severity: Medium]
As with iface_max_xl_len above, does this stack-allocated byte array cast
cause unaligned accesses and strict aliasing violations when writing to
cxl->prio and cxl->flags in inject_xl?

> +
> +	memset(buf, 0, sizeof(buf));
> +	cxl->prio = ID_RX;
> +	cxl->flags = CANXL_XLF | 0x7f;	/* read back as canfd_frame.len */
[ ... ]
> +static int open_observer(const char *ifname)
> +{
> +	struct sockaddr_can addr = { .can_family = AF_CAN };
> +	int s, one = 1;
> +
> +	s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> +	if (s < 0)
> +		return -1;
> +	addr.can_ifindex = ifindex_of(ifname);
> +	if (addr.can_ifindex < 0)
> +		goto err;
> +	/* the First Frame is a CAN FD frame: without this the observer is
> +	 * blind and the precondition check below reports a false alarm
> +	 */
> +	if (setsockopt(s, SOL_CAN_RAW, CAN_RAW_FD_FRAMES, &one, sizeof(one)) < 0)
> +		goto err;
> +	if (setsockopt(s, SOL_CAN_RAW, CAN_RAW_XL_FRAMES, &one, sizeof(one)) < 0)
> +		goto err;

[Severity: Medium]
The commit message states "a host without CAN XL support still gets flow
control and padding coverage", but this setsockopt causes open_observer
to return -1 if the kernel lacks CAN XL support.

Since the Classic CAN tests assert that open_observer succeeds via
ASSERT_GE(self->obs, 0), won't this cause the tests to fail entirely on
kernels without CAN XL support rather than skipping or running the non-XL
portions?

> +	if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0)
> +		goto err;
> +	return s;
> +err:
> +	close(s);
> +	return -1;
> +}
> +
> +static int open_injector(const char *ifname)
> +{
> +	struct sockaddr_can addr = { .can_family = AF_CAN };
> +	int s, one = 1;
> +
> +	s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> +	if (s < 0)
> +		return -1;
> +	addr.can_ifindex = ifindex_of(ifname);
> +	if (addr.can_ifindex < 0)
> +		goto err;
> +	if (setsockopt(s, SOL_CAN_RAW, CAN_RAW_XL_FRAMES, &one, sizeof(one)) < 0)
> +		goto err;

[Severity: Medium]
Similarly, does this unconditional setsockopt for CAN_RAW_XL_FRAMES in
open_injector prevent the non-XL tests from running successfully on a
kernel without CAN XL support?

> +	if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0)
> +		goto err;
> +	return s;
> +err:
> +	close(s);
> +	return -1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928114551.2148536-1-quchaosheng000406@163.com?part=1