Re: [PATCH v12 08/11] can: bcm: add missing device refcount for CAN filter removal

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Updating a BCM RX operation whose device was concurrently unregistered fails to re-register the CAN filter.
- [High] Lockless writes to op->rx_stamp and op->rx_ifindex in bcm_rx_handler can cause corrupted timestamps and mismatched metadata.
--

--- Patch [8]: [PATCH v12 08/11] can: bcm: add missing device refcount for CAN filter removal ---
commit 58881d5a5a65fccadb46fbccf91d34e84d09b49e
Author: Oliver Hartkopp <[email protected]>

can: bcm: add missing device refcount for CAN filter removal

This commit addresses a memory leak issue and concurrent device
unregistration by taking a tracked netdevice reference during filter
registration.

Link: https://sashiko.dev/#/patchset/[email protected]

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3cc3deb98a984..5660165cdf0eb 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -778,8 +778,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>  
>  	/* disable timeout */
>  	hrtimer_cancel(&op->timer);
>  
>  	/* save rx timestamp */
>  	op->rx_stamp = skb->tstamp;
>  	/* save originator for recvfrom() */
>  	op->rx_ifindex = skb->dev->ifindex;
>  
>  	/* op->flags/op->frames may be updated concurrently by bcm_rx_setup() */
>  	spin_lock_bh(&op->bcm_rx_update_lock);

[Severity: High]
This isn't a bug introduced by this patch, but can these lockless writes to
op->rx_stamp and op->rx_ifindex cause corrupted timestamps and mismatched
metadata?

If a CAN socket bound to all interfaces (ifindex == 0) receives packets
simultaneously from multiple interfaces in softirq context, could concurrent
execution of bcm_rx_handler() on different CPUs race to write these fields
outside of bcm_rx_update_lock?

On 32-bit systems, could this cause torn 64-bit ktime_t writes and wildly
corrupted timestamps?

Additionally, could one CPU overwrite the ifindex while another is passing it
to userspace, causing packets to be delivered with the wrong source interface
index?

[ ... ]

> @@ -1341,8 +1341,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  
>  		/* free temporary frames / kfree(NULL) is safe */
>  		kfree(new_frames);
>  
>  		/* Only an update -> do not call can_rx_register() */
>  		do_rx_register = 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditionally force
do_rx_register to 0 for an existing operation?

If a CAN interface is unregistered, triggering bcm_notify() which unregisters
the filter but leaves the bcm_op alive, and the application then issues an
RX_SETUP update on the same ifindex, won't skipping can_rx_register() here
leave the updated filter permanently detached from the CAN core?

Could this silently drop all CAN frames while returning a success return code to
the user application?

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