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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: unconditional cleanup destroys pre-existing interfaces
--

commit ffa7302bd0c4a8a610bf89cdbd13b8881574eec4
Author: Quchaosheng <quchaosheng000406@163.com>
    
selftests: can: add isotp frame-type tests
    
This commit adds a suite of tests for the isotp socket frame-type
handling, ensuring that Classic CAN and CAN FD channels correctly separate
frames by channel characteristics without incorrectly identifying CAN XL
frames.

> diff --git a/tools/testing/selftests/net/can/test_isotp_frame_type.sh b/tools/testing/selftests/net/can/test_isotp_frame_type.sh
> new file mode 100755
> index 0000000000000..1ee28ff3108ba
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_isotp_frame_type.sh
[ ... ]
> +setup()
> +{
> +	if [[ $CANIF == vcan* ]]; then
> +		ip link add name $CANIF type vcan || exit $ksft_skip

[Severity: Medium]
If the vcan interface already exists, ip link add will fail, causing the
script to exit. Since the trap for the cleanup function is registered before
setup is called, won't this trigger the cleanup function and tear down the
user's pre-existing interface?

> +		# a vcan device defaults to CANXL_MTU, so it carries every frame
> +		# type this test needs without further configuration
> +		ip link set dev $CANIF mtu 2060 2>/dev/null
> +	else
> +		echo "test_isotp_frame_type requires a vcan device" >&2
> +		exit $ksft_skip

[Severity: Medium]
If a user runs this test with a non-vcan interface, the setup function
will exit and skip. Does this also trigger the unconditional cleanup on
the user's provided interface?

> +	fi
> +	ip link set dev $CANIF up
> +}
> +
> +cleanup()
> +{
> +	ip link set dev $CANIF down
> +	if [[ $CANIF == vcan* ]]; then
> +		ip link delete $CANIF
> +	fi
> +}

[Severity: Medium]
Can this unconditionally destroy host network interfaces?

Because setup can fail or skip after the exit trap is registered, this
cleanup function executes unconditionally. It looks like this will bring down
and potentially delete active host interfaces that the test itself never
created.

Should the script track whether it actually created the interface before
attempting to modify its state or delete it?

[ ... ]
> +trap cleanup EXIT
> +setup
> +
> +tests_run
> +
> +exit $EXIT_STATUS

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