Re: [PATCH v12 05/11] can: bcm: add missing rcu list annotations and operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] List corruption when updating an existing RX operation after a device unregistration.
--

--- Patch [5]: [PATCH v12 05/11] can: bcm: add missing rcu list annotations and operations ---
commit d541f7be2b7c689e5da6382dd975710ba6b59607
Author: Oliver Hartkopp <[email protected]>

can: bcm: add missing rcu list annotations and operations

This commit adds missing list_add_rcu() calls in the BCM setup paths to ensure
proper initialization before traversing bcm_op structures under RCU. It also
fixes the removal side by explicitly unlinking ops with list_del_rcu() before
freeing them.

Closes: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 38c3f690c1cd0..940cd9df1cca3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1357,9 +1361,6 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		hrtimer_setup(&op->thrtimer, bcm_rx_thr_handler, CLOCK_MONOTONIC,
>  			      HRTIMER_MODE_REL_SOFT);
>  
> -		/* add this bcm_op to the list of the rx_ops */
> -		list_add(&op->list, &bo->rx_ops);
> -
>  		/* call can_rx_register() */
>  		do_rx_register = 1;
>  

[ ... ]

> @@ -1431,10 +1432,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  					      bcm_rx_handler, op, "bcm", sk);
>  		if (err) {
>  			/* this bcm rx op is broken -> remove it */
> -			list_del_rcu(&op->list);
>  			bcm_remove_op(op);
>  			return err;
>  		}
> +
> +		/* add this bcm_op to the list of the rx_ops */
> +		list_add_rcu(&op->list, &bo->rx_ops);

[Severity: High]
Can this unconditionally add an already-linked operation to the list?

If a CAN interface is unregistered, the NETDEV_UNREGISTER notifier clears
op->rx_reg_dev but leaves the operation linked in bo->rx_ops.

If bcm_rx_setup() is called again for the same CAN ID on this existing op,
it sees op->rx_reg_dev is NULL and sets do_rx_register to 1:

		do_rx_register = (ifindex && !op->rx_reg_dev) ? 1 : 0;

After successfully re-registering with the CAN core, this list_add_rcu()
executes. Since the op was never removed from bo->rx_ops, won't this
overwrite the list pointers and corrupt the RCU list?

Should this be guarded by the new_op flag, similar to how bcm_tx_setup()
guards its list_add_rcu() with add_op_to_list?

>  	}
>  
>  	return msg_head->nframes * op->cfsiz + MHSIZ;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5