Re: KCSAN: data-race in desc_read / prb_reserve_in_last

[John Ogness <john.ogness@linutronix.de>]

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