Re: [PATCH can-next v2 4/9] selftests: can: add J1939 socket option and metadata tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new selftest treats a kernel setsockopt contract violation as
  expected behaviour, so the test can never catch it.

--

selftests: can: add J1939 socket option and metadata tests

This adds a kselftest for J1939 socket options and send metadata. It
covers the SO_J1939_FILTER whitelist, SO_J1939_SEND_PRIO as seen on the
wire via a CAN_RAW peer, unconnected sendto(), a wrong-family bind,
SO_J1939_ERRQUEUE TX completion, and set/getsockopt round-trips of
several options.

> diff --git a/tools/testing/selftests/net/can/j1939_tests.h b/tools/testing/selftests/net/can/j1939_tests.h
> index 78eb35f5b6b06..639e000ac5e86 100644
> --- a/tools/testing/selftests/net/can/j1939_tests.h
> +++ b/tools/testing/selftests/net/can/j1939_tests.h

[ ... ]

> @@ -671,6 +673,71 @@ static inline bool recv_cm_cmd_ok(int raw, uint8_t want_pf, uint8_t want_da,
>  	return frame->data[J1939_CM_CMD_OFF] == want_cmd;
>  }
>  
> +/*
> + * Enable the error queue of s with SOF_TIMESTAMPING_TX_ACK only: a finished
> + * send then queues one TX-completion ACK, or an abort entry if the session
> + * fails (__j1939_sk_errqueue() in net/can/j1939/socket.c). The kernel reads
> + * these flags when the session ends, so this may come after the send.
> + */
> +static inline bool errqueue_enable_ack_ok(int s)
> +{
> +	int tstamp_flags = SOF_TIMESTAMPING_TX_ACK;
> +	int enable = 1;
> +
> +	/* setsockopt(SO_J1939_ERRQUEUE) returns the flag value, not 0 */
> +	if (setsockopt(s, SOL_CAN_J1939, SO_J1939_ERRQUEUE, &enable,
> +		       sizeof(enable)) < 0)
> +		return false;

[Severity: Low]
Should the test accept this as expected behaviour, or is this a kernel bug
the test should catch?

The comment matches what j1939_sk_setsockopt_flag() in
net/can/j1939/socket.c does today:

net/can/j1939/socket.c:j1939_sk_setsockopt_flag() {
    ...
	lock_sock(&jsk->sk);
	if (tmp)
		jsk->state |= flag;
	else
		jsk->state &= ~flag;
	release_sock(&jsk->sk);
	return tmp;
}

It returns the int that userspace passed in, not 0. For SO_J1939_PROMISC
and SO_J1939_ERRQUEUE that value goes back to userspace unchanged, so a
successful setsockopt() returns 1 instead of 0. A caller that checks
if (setsockopt(...)) will treat that success as a failure.

A negative optval such as -1 also sets the flag and then returns -1. The
caller then sees errno EPERM even though the state has changed.

This kernel behaviour dates back to 2018 (9d71dd0c700999), so this patch
did not introduce it. However, the check above only tests for < 0, and the
comment presents the positive return value as normal.

sockopt_roundtrip() in test_j1939_opts.c does the same:

	if (setsockopt(s, SOL_CAN_J1939, optname, &val, sizeof(val)) < 0)
		return -1;

As a result, getsockopt_promisc and getsockopt_errqueue cannot detect the
wrong return value on the set side. The file header still says they cover
"set/getsockopt round-trips" of these options.

The last patch in the series, "selftests: can: check SO_J1939_PROMISC
receive effect", repeats the pattern in promisc_receive. It uses
ASSERT_GE(setsockopt(...), 0) with a comment saying SO_J1939_PROMISC
returns the flag value (1) on success. No patch in the series changes
net/.

Could j1939_sk_setsockopt_flag() return 0 instead, with the tests checking
for == 0? If not, could the commit message at least say this is a known
deviation from the setsockopt() contract?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-j1939-selftests-v2-0-cf797c83317b%40pengutronix.de