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

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/18 9:16, 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.
> 
> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=feeb9b5352b9ff89618a
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---
> Tested with a KCSAN kernel in QEMU. syzbot's coredump repro is too slow to hit
> the window, so a thread loops pwrite() on a tmpfs file while the main thread
> does the first ioprio_set() on its tid (~35k iterations per run). Same pair,
> same "0 -> pointer" value change: 26 reports per run before, 0 in two runs
> after. No Fixes: tag, the history I have is grafted.
>  block/blk-ioc.c        | 5 +++--
>  include/linux/ioprio.h | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> 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);
>  	}
> -	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
> index 5210e8371238..56371e9ea306 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -56,7 +56,8 @@ static inline int task_nice_ioclass(struct task_struct *task)
>   */
>  static inline int __get_task_ioprio(struct task_struct *p)
>  {
> -	struct io_context *ioc = p->io_context;
> +	/* Paired with smp_store_release() in set_task_ioprio() */
> +	struct io_context *ioc = READ_ONCE(p->io_context);

How does this help with the ioc dereference below? By the time we access
ioc->ioprio without task_lock() held, ioc could have been freed already, no?
So it feels like this should be using rcu.

>  	int prio;
>  
>  	if (!ioc)
> @@ -66,7 +67,7 @@ static inline int __get_task_ioprio(struct task_struct *p)
>  	if (p != current)
>  		lockdep_assert_held(&p->alloc_lock);
>  
> -	prio = ioc->ioprio;
> +	prio = READ_ONCE(ioc->ioprio);
>  	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
>  		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
>  					 task_nice_ioprio(p));


-- 
Damien Le Moal
Western Digital Research