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

On 14.07.26 19:04, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Unconditional `list_add_rcu()` on the success path in `bcm_rx_setup()` corrupts the list for existing operations.
> - [Critical] Removing `list_del_rcu()` from the registration error path in `bcm_rx_setup()` causes a Use-After-Free for existing operations.
> --
> 

Great!

This is a correct fix. Nothing to do here.

The other problems related to this review are fixed in patch 9.

Only the sashiko-bot review 7 queue is stuck but had not problems in V14.

=> no potential regressions

https://sashiko.dev/#/patchset/20260714-bcm_fixes-v15-0-562f7e3e42da%40hartkopp.net

Best regards,
Oliver

> --- Patch [5]: [PATCH v15 05/11] can: bcm: add missing rcu list annotations and operations ---
> commit 493e1bc1997871e18b09125c187a88444720b12c
> Author: Oliver Hartkopp <[email protected]>
> 
> can: bcm: add missing rcu list annotations and operations
> 
> This commit updates the broadcast manager (BCM) to use proper RCU list
> operations when adding and removing operations. It addresses missing
> list_add_rcu() calls in setup functions and ensures ops are unlinked
> before being handed to bcm_remove_op().
> 
> Closes: https://lore.kernel.org/linux-can/[email protected]/
> 
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 03c98e4cc677d..5c1e83eeb4ff3 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
> [ ... ]
>> @@ -1449,10 +1450,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;
>>   		}
> 
> [Severity: Critical]
> Does removing this list_del_rcu() cause a use-after-free for existing
> operations?
> 
> If a user updates an existing BCM RX operation for a specific device that
> was unregistered, the op remains in the list but op->rx_reg_dev is cleared.
> 
> If the update occurs and the interface is gone, dev_get_by_index() fails,
> and this error path is taken.
> 
> Since bcm_remove_op() frees the operation via call_rcu(), removing
> list_del_rcu() leaves a freed operation linked in bo->rx_ops. Any
> subsequent operation traversing the list, such as bcm_release() or procfs
> reads, would then dereference freed memory.
> 
>> +
>> +		/* add this bcm_op to the list of the rx_ops */
>> +		list_add_rcu(&op->list, &bo->rx_ops);
>>   	}
> 
> [Severity: Critical]
> Could this unconditional list_add_rcu() corrupt the list if the operation
> is already linked?
> 
> When updating an existing operation that needs to be re-registered (for
> example, if the interface was unregistered and a new device reuses the
> ifindex), do_rx_register will be 1 for an operation that is already linked
> in bo->rx_ops.
> 
> Calling list_add_rcu() on a node that is already part of the list corrupts
> the list pointers, which could lead to an infinite loop or panic during RCU
> traversal.
>