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

[netdev-bot+sashiko@kernel.org]

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".

--

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.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019170834.1727150-2-mkl%40pengutronix.de