Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] Split critical sections in `bcm_rx_setup()` create a window of inconsistent state.
Pre-existing issues:
- [High] Lockless reads and writes of timer values and flags in TX path cause a data race and potential torn reads.
- [High] Direct, lockless modification of `op->frames` array in `bcm_tx_setup()` leads to data races and unrecoverable state corruption.
- [High] Use of `list_add()` instead of `list_add_rcu()` causes data races with lockless RCU readers.
--
--- Patch [3]: [PATCH v14 03/11] can: bcm: add locking when updating filter and timer values ---
commit b803c079b36024599ab905728a0b29f52d124d0c
Author: Oliver Hartkopp <[email protected]>
can: bcm: add locking when updating filter and timer values
This commit introduces bcm_rx_update_lock to protect timer and filter data
updates in bcm_rx_setup() and various RX handlers. It also adds read-side
locking in bcm_can_tx() and normalizes RTR frames in a staging buffer
before registering them.
Link: https://lore.kernel.org/linux-can/[email protected]/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..af4bb4182bc29 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -293,22 +294,26 @@ static int bcm_proc_show(struct seq_file *m, void *v)
> * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
> * of the given bcm tx op
> */
> -static void bcm_can_tx(struct bcm_op *op)
> +static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
> {
> struct sk_buff *skb;
> struct can_skb_ext *csx;
> struct net_device *dev;
> - struct canfd_frame *cf;
> + struct canfd_frame cframe;
> int err;
>
> /* no target device? => exit */
> if (!op->ifindex)
> return;
>
> - /* read currframe under lock protection */
> - spin_lock_bh(&op->bcm_tx_lock);
> - cf = op->frames + op->cfsiz * op->currframe;
> - spin_unlock_bh(&op->bcm_tx_lock);
> + if (!cf) {
> + /* read currframe under lock protection */
> + spin_lock_bh(&op->bcm_tx_lock);
> + memcpy(&cframe, op->frames + op->cfsiz * op->currframe,
> + op->cfsiz);
> + cf = &cframe;
> + spin_unlock_bh(&op->bcm_tx_lock);
> + }
[Severity: High]
This is a pre-existing issue, but does this read-side lock actually protect
against concurrent updates?
While bcm_can_tx() acquires bcm_tx_lock here, the writer in bcm_tx_setup()
directly modifies the op->frames array without holding the lock and without
using a staging buffer:
bcm_tx_setup() {
...
for (i = 0; i < msg_head->nframes; i++) {
cf = op->frames + op->cfsiz * i;
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
...
}
Because memcpy_from_msg() can sleep, it cannot be called under a spinlock.
Could this lead to bcm_can_tx() copying partially overwritten CAN frames, or
unrecoverable state corruption if memcpy_from_msg() fails midway?
[ ... ]
> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
> struct bcm_msg_head msg_head;
>
> if (op->kt_ival1 && (op->count > 0)) {
> - bcm_can_tx(op);
> + bcm_can_tx(op, NULL);
[Severity: High]
This is a pre-existing issue, but can this result in torn reads on 32-bit
platforms?
The bcm_tx_timeout_handler() reads kt_ival1, count, and flags locklessly
here. Concurrently, a user calling sendmsg() with TX_SETUP can trigger
bcm_tx_setup() to write new timer values locklessly:
bcm_tx_setup() {
...
if (op->flags & SETTIMER) {
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);
}
...
}
Does this data race risk unpredictable timer expiration or spurious CAN
transmissions if inconsistent combinations of flags and counts are observed?
[ ... ]
> @@ -1175,19 +1230,40 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> return -E2BIG;
>
> if (msg_head->nframes) {
> - /* update CAN frames content */
> - err = memcpy_from_msg(op->frames, msg,
> + /* get new CAN frames content before locking */
> + new_frames = kmalloc(msg_head->nframes * op->cfsiz,
> + GFP_KERNEL);
> + if (!new_frames)
> + return -ENOMEM;
> +
> + err = memcpy_from_msg(new_frames, msg,
> msg_head->nframes * op->cfsiz);
> - if (err < 0)
> + if (err < 0) {
> + kfree(new_frames);
> return err;
> + }
>
> - /* clear last_frames to indicate 'nothing received' */
> - memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
> + bcm_rx_setup_rtr_check(msg_head, op, new_frames);
> }
>
> + spin_lock_bh(&op->bcm_rx_update_lock);
> op->nframes = msg_head->nframes;
> op->flags = msg_head->flags;
>
> + if (msg_head->nframes) {
> + /* update CAN frames content */
> + memcpy(op->frames, new_frames,
> + msg_head->nframes * op->cfsiz);
> +
> + /* clear last_frames to indicate 'nothing received' */
> + memset(op->last_frames, 0,
> + msg_head->nframes * op->cfsiz);
> + }
> + spin_unlock_bh(&op->bcm_rx_update_lock);
[ ... ]
> @@ -1266,29 +1345,20 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> /* check flags */
>
> if (op->flags & RX_RTR_FRAME) {
> - struct canfd_frame *frame0 = op->frames;
> -
> /* no timers in RTR-mode */
> hrtimer_cancel(&op->thrtimer);
> hrtimer_cancel(&op->timer);
> -
> - /*
> - * funny feature in RX(!)_SETUP only for RTR-mode:
> - * copy can_id into frame BUT without RTR-flag to
> - * prevent a full-load-loopback-test ... ;-]
> - */
> - if ((op->flags & TX_CP_CAN_ID) ||
> - (frame0->can_id == op->can_id))
> - frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
> -
> } else {
> 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;
[Severity: Medium]
Could a concurrent bcm_rx_handler() process a frame during the gap between
these two critical sections?
In bcm_rx_setup(), the lock is acquired to update flags and frames, then
dropped, and re-acquired here to update the timer values. Does this create
a window where bcm_rx_handler() might see the new flags and frames alongside
stale kt_ival1 and kt_ival2 timer values, resulting in incorrect throttling
behavior?
[Severity: High]
This is a pre-existing issue, but does publishing operations without RCU
primitives cause data races with lockless readers?
In bcm_tx_setup() and bcm_rx_setup(), operations are published to the lists
using list_add() rather than list_add_rcu():
bcm_tx_setup() {
...
list_add(&op->list, &bo->tx_ops);
...
}
However, readers like bcm_proc_show() traverse the list locklessly:
bcm_proc_show() {
...
list_for_each_entry(op, &bo->tx_ops, list) {
...
}
Since list_add() lacks the smp_store_release() barrier provided by
list_add_rcu(), can lockless readers observe uninitialized memory if they
reach a newly linked node before its contents are globally visible?
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.