Re: [PATCH v3 2/3] can: bcm: add locking when updating filter and timer values

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2928 bytes --]

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a

commit 6ea1c1c7064ff2b5c2a0434ec61d62fd3463fc12
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

This commit addresses a KCSAN-detected data race in the CAN BCM protocol
by adding locking to protect simultaneous access to timer and filter values
during updates in bcm_rx_setup(). The race was benign functionally but is
resolved by introducing spin_lock_bh() protection.

Closes: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fd9fa072881e..0a3dc5500e14 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -125,6 +125,7 @@ struct bcm_op {
>  	struct sock *sk;
>  	struct net_device *rx_reg_dev;
>  	spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
> +	spinlock_t bcm_rx_update_lock; /* protect update of filter data */
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The comment says this lock protects "update of filter data" but the lock
is also used to protect timer data updates (ival1, ival2, kt_ival1,
kt_ival2, kt_lastmsg) in the SETTIMER block later in bcm_rx_setup().

Should the comment be more accurate and say "protect update of filter and
timer data"?

Additionally, this lock differs from bcm_tx_lock in that it's only taken on
the writer side (bcm_rx_setup) while the reader-side softirq handlers
(bcm_rx_handler, bcm_rx_timeout_handler, bcm_rx_thr_handler) never acquire
this lock. The protection is asymmetric -- it provides KCSAN suppression
and local-CPU softirq exclusion through spin_lock_bh(), but doesn't
synchronize against readers on other CPUs.

Is this asymmetric locking strategy intentional, or should the comment
clarify the actual protection model?

>  };
>
>  struct bcm_sock {

[ ... ]

> @@ -1263,10 +1286,13 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		if (op->flags & SETTIMER) {
>
>  			/* set timer value */
> +			spin_lock_bh(&op->bcm_rx_update_lock);
>  			op->ival1 = msg_head->ival1;
>  			op->ival2 = msg_head->ival2;
>  			op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
>  			op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
> +			op->kt_lastmsg = 0;
> +			spin_unlock_bh(&op->bcm_rx_update_lock);

This is where the lock also protects timer data, which confirms the comment
at the struct field declaration should mention timer data protection as well.

[ ... ]

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]