Re: [PATCH net-next 11/11] can: protect the receive lists with a per-device lock

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 20.10.26 19:11, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] raw_bind() no longer holds RTNL, so the socket can miss a
>    NETDEV_DOWN that arrives while it is binding.
> - [Low] The commit message says "the callers pass the namespace that has
>    been used for can_rx_register()", and the new kernel-doc of…
> - [Low] The new comment on netns_can.rcvlists_lock in
>    include/net/netns/can.h says it "protects rcv_lists_stats".
> 

Thanks for the review.

[Medium] NETDEV_DOWN during raw_bind():

The window exists, but it only affects a down transition in the middle
of raw_bind(). Missing the asynchronous ENETDOWN there is harmless and
not worth an additional barrier pair in raw_bind() and raw_notify():

- Every transmission on a CAN_RAW socket goes through can_send(), which
   checks IFF_UP and returns -ENETDOWN to sendmsg(). So the state is
   reported reliably on every send attempt.

- The missed notification does not affect the binding: the CAN_RAW
   socket stays bound and its filters stay registered across down/up,
   and frames are received again once the interface is up.

- Applications can not rely on this notification anyway: a CAN_RAW
   socket bound to all CAN interfaces (ifindex 0) never gets ENETDOWN.
   Applications that need the link state have to track it via
   rtnetlink, not via the CAN socket.

[Low] rcvlists_lock comment / j1939 @net: These are documentation
details; the j1939 statistics behaviour is unchanged by this patch, as
noted in the patch description.

Best regards,
Oliver


> --
> 
> can: protect the receive lists with a per-device lock
> 
> This commit adds a spinlock to struct can_dev_rcv_lists. The receive lists
> of a CAN device are now protected per device instead of by the per-netns
> rcvlists_lock, so can_rx_unregister() still finds subscriptions after the
> device has moved to another namespace. It also drops RTNL from raw_bind(),
> raw_setsockopt(), raw_release(), isotp_bind() and isotp_release(), and
> relies on lock_sock() plus a device pointer published inside the RCU
> lookup to serialize with the netdev notifiers.
> 
> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> Link: https://patch.msgid.link/20261019152741.54854-6-socketcan@hartkopp.net
> 
>> diff --git a/include/net/netns/can.h b/include/net/netns/can.h
>> index 48b79f7e6236..ef7aefdf7b23 100644
>> --- a/include/net/netns/can.h
>> +++ b/include/net/netns/can.h
>> @@ -29,7 +29,7 @@ struct netns_can {
>>   
>>   	/* receive filters subscribed for 'all' CAN devices */
>>   	struct can_dev_rcv_lists *rx_alldev_list;
>> -	spinlock_t rcvlists_lock;
>> +	spinlock_t rcvlists_lock; /* protects rcv_lists_stats */
> 
> [Severity: Low]
> This isn't a bug, but is this comment accurate? The lock only serializes
> the rcv_entries and rcv_entries_max updates in can_rx_register() and
> can_rx_unregister().
> 
> can_init_stats() in net/can/proc.c updates other fields of the same
> structure without taking rcvlists_lock. It runs from the stats timer, or
> from the reset_stats proc read when the timer is disabled:
> 
> net/can/proc.c:can_init_stats() {
>      ...
> 	rcv_lists_stats->stats_reset++;
> 
> 	if (user_reset) {
> 		user_reset = 0;
> 		rcv_lists_stats->user_reset++;
> 	}
> }
> 
> can_stats_proc_show() also reads these counters without the lock.
> 
> Could the comment name rcv_entries and rcv_entries_max specifically?
> 
> [ ... ]
> 
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index d97f85fc3232..bf612c4b983d 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
> 
> [ ... ]
> 
>> @@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>    *
>>    * Description:
>>    *  Removes subscription entry depending on given (subscription) values.
>> + *  The receive lists of a CAN device are protected by a per-device lock,
>> + *  so the subscription is found even when the device has been moved to
>> + *  another network namespace after can_rx_register(). The caller has to
>> + *  ensure that @dev stays valid, e.g. by holding a reference to @dev or
>> + *  RTNL. @net is the namespace that has been used for can_rx_register().
>> + *  It selects the 'any' CAN device lists for @dev == NULL and is used for
>> + *  the receive list statistics.
>>    */
> 
> [Severity: Low]
> The new kernel-doc says @net is the namespace that was used for
> can_rx_register(). The commit message says the same: "the callers pass
> the namespace that has been used for can_rx_register()".
> 
> Does j1939 follow this? It reads dev_net(ndev) again at unregister time:
> 
> net/can/j1939/main.c:j1939_can_rx_register() {
>      ...
> 	ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> 			      j1939_can_recv, priv, 0, NULL);
>      ...
> }
> 
> net/can/j1939/main.c:j1939_can_rx_unregister() {
>      ...
> 	can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> 			  j1939_can_recv, priv);
>      ...
> }
> 
> A j1939 registration can outlive a namespace move. j1939_sk_bind() does
> not hold RTNL. It calls j1939_netdev_start(), which registers with the old
> namespace, before it calls j1939_jsk_add().
> 
> Suppose the NETDEV_UNREGISTER from __dev_change_net_namespace() runs
> j1939_sk_netdev_event_unregister() in that window. The socket is not
> J1939_SOCK_BOUND yet, so it is skipped and the registration survives the
> move. The final release then passes the new namespace:
> 
> j1939_netdev_stop()
>    __j1939_rx_release()
>      j1939_can_rx_unregister()
>        can_rx_unregister(dev_net(ndev), ...)
> 
> The receiver is still found through the per-device lists. However,
> rcv_entries is decremented in the new namespace's rcv_lists_stats
> (clamped at 0) and stays elevated in the old one. This accounting matches
> the behaviour before the patch.
> 
> Should j1939_priv store the registration namespace (with a netns
> reference)? Or should the kernel-doc and commit message describe a looser
> rule?
> 
> [ ... ]
> 
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index dfea48768b23..0d8edfa772fb 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
> 
> [ ... ]
> 
>> @@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>>   		       struct net_device *dev)
>>   {
>>   	struct sock *sk = &ro->sk;
>> +	struct net_device *bind_dev;
>>   
>>   	if (!net_eq(dev_net(dev), sock_net(sk)))
>>   		return;
>>   
>> -	if (ro->dev != dev)
>> +	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
>> +	 * Read it before ro->dev, pairs with smp_store_release() in
>> +	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
>> +	 */
>> +	bind_dev = smp_load_acquire(&ro->bind_dev);
>> +	if (READ_ONCE(ro->dev) != dev &&
>> +	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
>>   		return;
> 
> [Severity: Medium]
> Can a NETDEV_DOWN be lost while raw_bind() is running? The bind_dev hint
> is only checked for NETDEV_UNREGISTER, and raw_bind() no longer holds
> RTNL.
> 
> raw_bind() now runs these steps in order:
> 
>      smp_store_release(&ro->bind_dev, dev);
>      ...
>      if (!(READ_ONCE(dev->flags) & IFF_UP))   /* device still up here */
>          notify_enetdown = 1;
>      ...
>      err = raw_enable_allfilters(sock_net(sk), dev, sk);  /* cond_resched() */
>      ...
>      WRITE_ONCE(ro->dev, dev);
> 
> Suppose dev_close() runs after the IFF_UP check but before ro->dev is
> published. raw_notifier()->raw_notify() then sees ro->dev != dev with
> msg == NETDEV_DOWN, so it returns at the check above and never reaches
> case NETDEV_DOWN. raw_bind() finishes with notify_enetdown == 0.
> 
> The socket ends up bound to a down interface, and sk_err = ENETDOWN /
> sk_error_report() is never raised. Before this patch, rtnl_lock() in
> raw_bind() kept dev_close() out of this window.
> 
> isotp_bind() publishes so->dev before it reads IFF_UP, so it does not seem
> to have this problem.
> 
> Could raw_notify() also check bind_dev for NETDEV_DOWN? Alternatively,
> raw_bind() could publish ro->dev before the IFF_UP check.
> 
> [ ... ]
>