Re: KCSAN: data-race in desc_read / prb_reserve_in_last

[Petr Mladek]

On Fri 2026-03-13 17:37:55, John Ogness wrote:
> Hi Petr,
> 
> On 2026-03-12, Petr Mladek <pmladek@xxxxxxxx> 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