Re: [PATCH can-next 3/3] can: add can diag interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `can_diag` module fails to enforce a dependency on the core `can` module, leading to a crash when locking an uninitialized mutex.
- [Medium] Binary structures are embedded directly inside Netlink attributes, violating Netlink design rules.
--

commit 058ea09f4025994f935c1bd793eb19a873974e20
Author: Filippo Storniolo <fstornio@redhat.com>

can: add can diag interface

Add the can_diag interface for querying sockets from userspace.
ss(8) tool can use this interface to list open sockets. The userspace ABI
is defined in <linux/can_diag.h> and includes netlink request and response
structs.

> diff --git a/include/uapi/linux/can/diag.h b/include/uapi/linux/can/diag.h
> new file mode 100644
> index 0000000000000..7f99f85c42425
> --- /dev/null
> +++ b/include/uapi/linux/can/diag.h
[ ... ]
> +struct can_diag_j1939_addr {
> +	__u64	name;
> +	__u32	pgn;
> +	__u8	addr;
> +};

[Severity: Medium]
Does this violate the Netlink subsystem design guidelines?

The Netlink documentation states to avoid binary structures inside attributes
and suggests breaking each member into its own attribute.

Also, does this structure have implicit trailing padding? Without explicit
padding after the 1-byte field, we might run into architecture-dependent size
differences, breaking 32-bit vs 64-bit compatibility.

[ ... ]
> diff --git a/net/can/diag.c b/net/can/diag.c
> new file mode 100644
> index 0000000000000..eb623ed118af5
> --- /dev/null
> +++ b/net/can/diag.c
[ ... ]
> +	case CAN_J1939:
> +	{
> +		struct can_diag_j1939_addr j1939_addr;
> +
> +		memset(&j1939_addr, 0, sizeof(j1939_addr));
> +
> +		j1939_addr.name = can_addr.can_addr.j1939.name;
> +		j1939_addr.pgn = can_addr.can_addr.j1939.pgn;
> +		j1939_addr.addr = can_addr.can_addr.j1939.addr;
> +
> +		err = nla_put(skb, CAN_DIAG_J1939_ADDR, sizeof(struct can_diag_j1939_addr),
> +			      &j1939_addr);

[Severity: Medium]
Does this directly write the binary struct into the Netlink message attribute?
The Netlink guidelines discourage putting binary structs into attributes for
new families.

[ ... ]
> +static int can_diag_dump(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	int num = 0, s_num = cb->args[0];
> +	struct can_diag_req *req;
> +	struct net *net;
> +	struct sock *sk;
> +
> +	net = sock_net(skb->sk);
> +	req = nlmsg_data(cb->nlh);
> +
> +	mutex_lock(&net->can.sklist_lock);

[Severity: High]
Can this attempt to lock an uninitialized mutex?

Since can_diag.ko does not call any exported symbols from can.ko, the kernel
module loader will not enforce a load dependency between them. If a userspace
tool queries CAN sockets, the kernel might autoload can_diag.ko without loading
can.ko.

Because can.ko is responsible for registering the pernet operations that
initialize net->can.sklist_lock, can_diag_dump() could end up calling
mutex_lock() on a zeroed, uninitialized mutex, leading to lockdep warnings
or crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-feat-can-diag-v1-0-021e3f1631a0@redhat.com?part=3