Re: [PATCH v12 09/11] can: bcm: fix stale rx/tx ops after device removal

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bcm_rx_setup() partially updates the state of an existing rx_op before returning an error if re-registration fails.
--

--- Patch [9]: [PATCH v12 09/11] can: bcm: fix stale rx/tx ops after device removal ---
commit 284aaee60a085295a0065bd19eaa02386e2787bd
Author: Oliver Hartkopp <[email protected]>

can: bcm: fix stale rx/tx ops after device removal

This commit fixes issues with stale rx/tx ops after device removal by
re-registering RX ops when an update occurs after NETDEV_UNREGISTER,
and by cancelling cyclic TX timers upon device removal.

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

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 5660165cdf0eb..fbf1cbb7c99fe 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1483,18 +1495,27 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  				err = -ENODEV;
>  			}
>  
> -		} else
> +		} else {
>  			err = can_rx_register(sock_net(sk), NULL, op->can_id,
>  					      REGMASK(op->can_id),
>  					      bcm_rx_handler, op, "bcm", sk);
> +		}
> +
>  		if (err) {
> -			/* this bcm rx op is broken -> remove it */
> -			bcm_remove_op(op);
> +			/* newly created bcm rx op is broken -> remove it */
> +			if (new_op)
> +				bcm_remove_op(op);
> +
> +			/* an existing op just stays unregistered until the
> +			 * next successful RX_SETUP - it is already part of
> +			 * bo->rx_ops and must not be freed here
> +			 */
>  			return err;

[Severity: Medium]
If re-registration fails for an existing op, an error is returned to userspace
here. However, does this leave the internal state of the op partially updated?

Before this code block, bcm_rx_setup() already modifies the frame data, frame
counts, and timer configurations, and may have started timers:

net/can/bcm.c:bcm_rx_setup() {
    ...
		if ((op->flags & STARTTIMER) && op->kt_ival1)
			hrtimer_start(&op->timer, op->kt_ival1,
				      HRTIMER_MODE_REL_SOFT);
    ...
}

Does returning an error without rolling back these state changes or cancelling
the timers leave the operation in an inconsistent state? It appears sendmsg
will return an error to userspace, but the operation might unexpectedly trigger
timeouts based on the partially applied setup.

>  		}
>  
> -		/* add this bcm_op to the list of the rx_ops */
> -		list_add_rcu(&op->list, &bo->rx_ops);
> +		/* add a new bcm_op to the list of the rx_ops */
> +		if (new_op)
> +			list_add_rcu(&op->list, &bo->rx_ops);
>  	}

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