Re: KCSAN: data-race in desc_read / prb_reserve_in_last

Petr Mladek
2026-03-19T11:17:07Z

On Fri 2026-03-13 17:37:55, John Ogness wrote:
> Hi Petr,
>
> On 2026-03-12, Petr Mladek <pmladek@suse.com> wrote:
> > [rfc v2]:
> >
> > diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> > index 56c8e3d031f4..c080edf5636a 100644
> > --- a/kernel/printk/printk_ringbuffer.c
> > +++ b/kernel/printk/printk_ringbuffer.c
> > @@ -454,11 +454,12 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
> > struct printk_info *info = to_info(desc_ring, id);
> > struct prb_desc *desc = to_desc(desc_ring, id);
> > atomic_long_t *state_var = &desc->state_var;
> > + unsigned long state_val, state_val_check;
> > enum desc_state d_state;
> > - unsigned long state_val;
> >
> > /* Check the descriptor state. */
> > state_val = atomic_long_read(state_var); /* LMM(desc_read:A) */
> > +try_again:
> > d_state = get_desc_state(id, state_val);
> > if (d_state == desc_miss || d_state == desc_reserved) {
> > /*
> > @@ -466,7 +467,9 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
> > * @state_var so that the caller can see the details of
> > * the inconsistent state.
> > */
> > - goto out;
> > + if (desc_out)
> > + atomic_long_set(&desc_out->state_var, state_val);
> > + return d_state;
>
> I would keep the "goto out" so that the success scenario can also set
> the state_var of @desc_out.

You are right.

> > }
> >
> > /*
> > @@ -542,14 +545,19 @@ static enum desc_state desc_read(struct prb_desc_ring *desc_ring,
> > smp_rmb(); /* LMM(desc_read:D) */
> >
> > /*
> > - * The data has been copied. Return the current descriptor state,
> > - * which may have changed since the load above.
> > + * The data has been copied. Make sure that they match the given @id
> > + * and @d_state read at the beginning. Re-read them otherwise.
> > + *
> > + * Note that this simplifies the logic. Especially, the reader API
> > + * expects that all values are stable when the record is finalized.
> > + * But they are stable only when it stays finalized all the time.
> > */
> > - state_val = atomic_long_read(state_var); /* LMM(desc_read:E) */
> > - d_state = get_desc_state(id, state_val);
> > -out:
> > - if (desc_out)
> > - atomic_long_set(&desc_out->state_var, state_val);
> > + state_val_check = atomic_long_read(state_var); /* LMM(desc_read:E) */
> > + if (state_val_check != state_val) {
> > + state_val = state_val_check;
> > + goto try_again;
> > + }
> > +
>
> The out label and code could stay as it was. Or is there some reason you
> do not want to set the state_var in the success case?

It was a mistake. I was confused by the following code:

/*
* Copy the descriptor data. The data is not valid until the
* state has been re-checked. A memcpy() for all of @desc
* cannot be used because of the atomic_t @state_var field.
*/
if (desc_out) {
memcpy(&desc_out->text_blk_lpos, &desc->text_blk_lpos,
sizeof(desc_out->text_blk_lpos)); /* LMM(desc_read:C) */
}

I somehow assumed that the memcpy copied everything. And we will not
longer need to update state_var when state_val did not change.

But the code copies only text_blk_lpos.

/o\

I am going to send a proper patch where this will be fixed.

> out:
> if (desc_out)
> atomic_long_set(&desc_out->state_var, state_val);
>
> > return d_state;
> > }
>
> We should probably annotate the memcpy() with a wrapping
> data_race(). Sadly we were aware of and expected the data race, but did
> not realize that commit 4cfc7258f876 ("printk: ringbuffer: add
> finalization/extension support") made the data race a problem.

Yup. I'll add the data_race() as well.

Thanks a lot for review.

Best Regards,
Petr