Re: KCSAN: data-race in desc_read / prb_reserve_in_last

John Ogness
2026-03-13T16:31:58Z

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.

> }
>
> /*
> @@ -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?

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.

John