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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 02.04.26 20:13, Filippo Storniolo wrote:
> Hi Oliver,
> thank you for the review.
> 
> On Thu, Apr 02, 2026 at 03:07:53PM +0200, Oliver Hartkopp wrote:
>> Hello Filippo,
>>
>> many thanks for your patches.
>>
>> I'm not yet convinced we would need this kind of interface as many
>> features in ss(8) are only relevant for IP and not really for CAN.
>>
>> Btw. having an overview over PIDs and open sockets might be a nice
>> informational feature.
>>
>> Some remarks at first sight:
>>
>> On 02.04.26 12:54, Filippo Storniolo wrote:
>>> 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
>>
>> I would suggest <linux/can/diag.h> to match the other CAN netlayer
>> definitions and not bloat the include/linux directory once more.
>>
> 
> I created the file can_diag.h because I saw that every other
> diag header (e.g. inet, packet, vm_sock, unix, etc.) are all
> located under /include/uapi/linux.
> However, on the other hand, most of these do not have a directory
> under /include/uapi/linux as can already has it.

Thanks

> 
>>> netlink request and response structs.  The request queries open
>>> can sockets and the response contains socket information fields
>>> including the interface index for bound sockets, inode number,
>>> transport protocol etc.
>>>
>>> Support can be added later by extending can_diag_dump().
>>>
>>> Suggested-by: Davide Caratti <dcaratti@redhat.com>
>>> Signed-off-by: Filippo Storniolo <fstornio@redhat.com>
>>> ---
>>>  MAINTAINERS                   |   1 +
>>>  include/uapi/linux/can_diag.h |  43 ++++++++++++
>>
>> include/uapi/linux/can/diag.h
>>
> 
> If needed, this will be changed accordingly.
> 
>>>  net/can/Kconfig               |  10 +++
>>>  net/can/Makefile              |   2 +
>>>  net/can/can-diag.c            | 153 ++++++++++++++++++++++++++++++++ 
>>> ++++++++++
>>
>> net/can/diag.c
>>
>> for the same reason.
>>
> 
> Agree, I will change the file from can_diag.c to diag.c

Thanks!

>>>  5 files changed, 209 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 
>>> 7a2ffd9d37d57c0db59e602eeffc2e2f09b613d9..f338ef2380a634a671d06f27bb4dac6f45f4d2a4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -5730,6 +5730,7 @@ F:      include/linux/can/skb.h
>>>  F:  include/net/can.h
>>>  F:  include/net/netns/can.h
>>>  F:  include/uapi/linux/can.h
>>> +F:   include/uapi/linux/can_diag.h
>>>  F:  include/uapi/linux/can/bcm.h
>>>  F:  include/uapi/linux/can/gw.h
>>>  F:  include/uapi/linux/can/isotp.h
>>> diff --git a/include/uapi/linux/can_diag.h b/include/uapi/linux/ 
>>> can_diag.h
>>> new file mode 100644
>>> index 
>>> 0000000000000000000000000000000000000000..e63d79f1ab3803a5778407e07d485732a112745a
>>> --- /dev/null
>>> +++ b/include/uapi/linux/can_diag.h
>>> @@ -0,0 +1,43 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +
>>> +#ifndef _UAPI__CAN_DIAG_H__
>>
>> Why not using _UAPI_CAN_DIAG_H_ here?
>>
> 
> No particular reason, we can change it to _UAPI_CAN_DIAG_H_
> 

Thanks!

>>> +#define _UAPI__CAN_DIAG_H__
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/can.h>
>>> +
>>> +/* Request */
>>> +struct can_diag_req {
>>> +     __u8    sdiag_family;   /* must be AF_CAN */
>>> +     __u8    sdiag_protocol; /* for future filtering of transport 
>>> protocols */
>>> +     __u16   pad;
>>> +     __u32   cdiag_states;
>>> +     __u32   cdiag_ino;
>>> +     __u32   cdiag_show;
>>> +     __u32   cdiag_cookie[2];
>>> +};
>>> +
>>> +enum {
>>> +     CAN_DIAG_UNSPEC,
>>> +     CAN_DIAG_UID,
>>> +
>>> +     __CAN_DIAG_MAX,
>>> +};
>>> +
>>> +#define CAN_DIAG_MAX (__CAN_DIAG_MAX - 1)
>>> +
>>> +/* Response */
>>> +struct can_diag_msg {
>>> +     __u8    cdiag_family;   /* AF_CAN */
>>> +     __u8    cdiag_state;
>>> +     __u16   cdiag_protocol;
>>> +     __u16   cdiag_type;
>>> +     __u16   pad16;
>>> +     __u32   cdiag_ino;
>>> +     canid_t cdiag_tx_id; /* meaningful only for ISO-TP */
>>> +     canid_t cdiag_rx_id; /* meaningful only for ISO-TP */
>>
>> What about the J1939 addressing here which is part of the struct
>> sockaddr_can too?
>>
> 
> Right, I missed this during implementation. I guess we can either
> extend the structure so that it contains also the addressing
> information of J1939 or add a new attribyte in the netlink message,
> similar to what it has been done for the uid.
> 
> IMHO, I think the second option is better, especially when
> we dump information that are valid only for specific protocols.
> Otherwise, we would have a bigger netlink message for every socket,
> even if they do not need some fields.
> 
> However, we can evaluate pros and cons and refine it.

In sockaddr_can the ISO-TP and J1939 address information is placed in an 
union as they can not be valid at the same time and the cdiag_protocol 
also provides the information what kind of content we would have read, 
right?

Best regards,
Oliver