Re: [PATCH v3] block: annotate lockless task ioprio accesses

[Damien Le Moal]

On 2026/10/20 14:52, Nguyen Ngoc Thang wrote:
> __get_task_ioprio() reads task->io_context and ioc->ioprio without
> task_lock() when called on current, e.g. from init_sync_kiocb() on every
> write.  set_task_ioprio() can update both for another task at any time,
> including the first publication of a new io_context.  KCSAN reports it:
> 
>   BUG: KCSAN: data-race in __kernel_write_iter / set_task_ioprio
>   value changed: 0x0000000000000000 -> 0xffff888109813730
> 
> A stale or torn value is harmless, a task simply gets the old or new
> priority, but the compiler may tear or refetch the accesses and the
> io_context fields must be visible before the pointer is.
> 
> The io_context itself needs no extra protection: only the owning task
> drops it, in exit_io_context(), so it cannot be freed under a lockless
> reader running as current.
> 
> Publish the pointer with smp_store_release(), read it with READ_ONCE()
> (address dependency orders the ioprio load), and mark the ioprio
> accesses.
> 
> Reported-by: syzbot+feeb9b5352b9ff89618a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=feeb9b5352b9ff89618a
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>

The patch title still mentions "annotate", which is really vague. Please change
the title to be more descriptive. E.g. "use READ_ONCE() to access a task ioprio
context".

> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index d15918d7fabb..12ddc672f41d 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -277,9 +277,10 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
>  		if (task->io_context)
>  			kmem_cache_free(iocontext_cachep, ioc);
>  		else
> -			task->io_context = ioc;
> +			/* Pairs with READ_ONCE() in __get_task_ioprio() */
> +			smp_store_release(&task->io_context, ioc);

This is normally paired with smp_load_acquire(). The generic implementation of
that is simply a READ_ONCE() followed by an __smp_mb(), but some architectures
may have their own implementation. So why is it always correct to pair with
READ_ONCE()? Why not use WRITE_ONCE() here?

>  	}
> -	task->io_context->ioprio = ioprio;
> +	WRITE_ONCE(task->io_context->ioprio, ioprio);
>  out:
>  	task_unlock(task);
>  	return 0;
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h


-- 
Damien Le Moal
Western Digital Research