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

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?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSl+MghEFFAdY3pYJLMOmT6rpmt0gUCanSSJgAKCRDMOmT6rpmt
0h7VAP9Ih6E1S6WqXwO6uJvPL7CjjYhLMCHhwU/1GlFBQsGBTQEAtGyqGBdoWnM2
cNlaCUG7tRS0rI3eVPRzSbPjba6eaws=
=WNak
-----END PGP SIGNATURE-----