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

[Filippo Storniolo <fstornio@redhat.com>]

On Fri, Sep 04, 2026 at 02:26:05PM +0000, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [Medium] Index-based resumption in can_diag_dump() causes skipped or repeated sockets without notifying userspace via NLM_F_DUMP_INTR.
>--
>
>commit 8ae9bc3a24d9d75317a64a778b0ca0dc88c6a32a
>Author: Filippo Storniolo <fstornio@redhat.com>
>
>can: add can diag interface
>
>Add the can_diag interface for querying sockets from userspace. The ss(8)
>tool can use this interface to list open sockets. The userspace ABI includes
>netlink request and response structs with socket information fields.
>
>> diff --git a/net/can/diag.c b/net/can/diag.c
>> new file mode 100644
>> index 0000000000000..647a94636a548
>> --- /dev/null
>> +++ b/net/can/diag.c
>
>[ ... ]
>
>> +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);
>> +
>> +	lock_can_diag_mutex(net);
>> +	sk_for_each(sk, &net->can.sklist) {
>> +		if (num < s_num)
>> +			goto next;
>> +
>> +		if (sk_diag_fill(sk, skb, req,
>
>[Severity: Medium]
>Will this simple index-based resumption (num < s_num) cause skipped or
>repeated sockets without notifying userspace if the list is modified between
>dump iterations?
>
>A netlink dump of CAN sockets can require multiple messages, forcing
>can_diag_dump() to return and resume later. If a concurrent thread creates
>or destroys a CAN socket, altering the layout of net->can.sklist while the
>mutex is dropped, iteration by index becomes inaccurate.
>
>According to the Netlink uAPI guidelines, if iteration during a dump may skip
>or repeat objects, NLM_F_DUMP_INTR must be set on the affected messages. This
>is normally implemented by maintaining a generation counter and recording it
>in cb->seq to detect list modifications.
>
>> +				 sk_user_ns(NETLINK_CB(cb->skb).sk),
>> +				 NETLINK_CB(cb->skb).portid,
>> +				 cb->nlh->nlmsg_seq, NLM_F_MULTI,
>> +				 sock_i_ino(sk)) < 0)
>> +			goto done;
>> +next:
>> +		num++;
>> +	}
>> +done:
>> +	unlock_can_diag_mutex(net);
>> +	cb->args[0] = num;
>> +
>> +	return skb->len;
>> +}
>
>-- 
>Sashiko AI review · https://sashiko.dev/#/patchset/20260904-feat-can-diag-v3-0-bdcebf07b373@redhat.com?part=3
>

I understand the concern, but as far as I can tell no other diag
handler implements this.  Does it make sense to add it only here rather
than as a follow-up done uniformly across all the diag handlers?

@Marc Kleine-Budde @Oliver Hartkopp what do you think about this?
If you think it could be worth adding this feature, I can do a respin.

Best regards,
Filippo