can: bcm: disable bh when updating filter and timer values

KCSAN detected a simultaneous access to timer values that can be overwritten in bcm_rx_setup when updating timer and filter content. This caused no functional issues in the past as the new values might show up at any time without losing its intended functionality.

Btw. the KCSAN report can be easily resolved by protecting the 'lockless' data updates with local_bh_[dis|en]able().

---
 net/can/bcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 7eba8ae01a5b..5fde4d4db893 100644
@@ -1145,23 +1145,27 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		 * therefore (complexity / locking) it is not supported.
 		 */
 		if (msg_head->nframes > op->nframes)
 			return -E2BIG;
 
+		local_bh_disable();
 		if (msg_head->nframes) {
 			/* update CAN frames content */
 			err = memcpy_from_msg(op->frames, msg,
 					      msg_head->nframes * op->cfsiz);
-			if (err < 0)
+			if (err < 0) {
+				local_bh_enable();
 				return err;
+			}
 
 			/* clear last_frames to indicate 'nothing received' */
 			memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
 		}
 
 		op->nframes = msg_head->nframes;
 		op->flags = msg_head->flags;
+		local_bh_enable();
 
 		/* Only an update -> do not call can_rx_register() */
 		do_rx_register = 0;
 
 	} else {

@@ -1254,24 +1258,26 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 	} else {
 		if (op->flags & SETTIMER) {
 
 			/* set timer value */
+			local_bh_disable();
 			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;
+			local_bh_enable();
 
 			/* disable an active timer due to zero value? */
 			if (!op->kt_ival1)
 				hrtimer_cancel(&op->timer);
 
 			/*
 			 * In any case cancel the throttle timer, flush
 			 * potentially blocked msgs and reset throttle handling
 			 */
-			op->kt_lastmsg = 0;
 			hrtimer_cancel(&op->thrtimer);
 			bcm_rx_thr_flush(op);
 		}
 
 		if ((op->flags & STARTTIMER) && op->kt_ival1)

2.47.3

What's the exact problem we have here?

Simultaneous access or modification (which one?) of op->frames, op->last_frames, op->nframes, op->flags from the timer and bcm_rx_setup()?

How does disabling the bh on the local CPU only solve the problem? What happens if the timer is currently running?

Also see patch c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates") and the related discussion.

regards,
Marc

--

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 |

In opposite to the bcm op locking, where two sides are writing data to the variables, we can overwrite existing content in bcm_rx_setup().

The softirq side is only reading the filters and timer settings when a CAN frame is received. There is no concurrent write.

There are two potential timers (timeout of cyclic messages and throttle timer) that can be rearmed in the softirq. There is no issue IMO and it was no problem in the past.

KCSAN only remarked that writing the updated values and reading these values happened at the same time.

And I think this can exactly be solved with local_bh_disable().
Other code in the kernel using local_bh_disable() use the same pattern and asking the KI about the use case of local_bh_disable() fits to this fix.

Best regards,
Oliver