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

On Thu, Aug 06, 2026 at 03:54:48PM +0200, Marc Kleine-Budde wrote:
>On 06.08.2026 13:51:22, [email protected] wrote:
>> 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?
>
>Sounds like a valid point. How is this handled with other diag data?

Hi Marc, thank you for the review.
I understand that with the new netlink guidelines, passing binaries
structures through the netlink message is discouraged. 
In the next version I will change that and I will send the single 
datafields using netlink attributes.

kind regards,
Filippo