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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Netlink UAPI attributes embed binary structures with implicit compiler padding
--

--- Patch [3]: [PATCH can-next v2 3/3] can: add can diag interface ---
commit 98771deaa2e2d4a790b170eb8967f6122234dec8
Author: Filippo Storniolo <[email protected]>

can: add can diag interface

This commit adds the can_diag interface for querying CAN sockets from
userspace using the ss tool. It defines the userspace ABI including netlink
request and response structures to expose socket properties.

> 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: High]
Does this structure introduce implicit compiler padding? With a 64-bit, 32-bit,
and 8-bit field, it appears there are 3 bytes of implicit padding at the end.
This can lead to architecture-dependent layout mismatches in the UAPI.

Additionally, modern Netlink design rules strongly discourage passing binary
structures directly inside attributes. Should these structure members be broken
out into individual Netlink attributes to ensure proper validation and future
extensibility?

> diff --git a/net/can/diag.c b/net/can/diag.c
> new file mode 100644
> index 0000000000000..e9132ba3f8cc5
> --- /dev/null
> +++ b/net/can/diag.c
[ ... ]
> +static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
[ ... ]
> +	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: High]
Does sending this binary structure directly as a raw attribute prevent proper
validation and extensibility? 

Once this ships in a release, the UAPI is frozen and this layout becomes
unfixable. Could this be changed to send the individual fields as separate,
strongly-typed attributes instead of embedding the C structure directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3