[PATCH] block: annotate lockless task ioprio accesses

[PATCH] block: annotate lockless task ioprio accesses

[Nguyen Ngoc Thang]

__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);
 	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));
-- 
2.43.0

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

[Damien Le Moal]

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

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

[Nguyen Ngoc Thang]

Hi Damien,

Thanks for looking. I don't think RCU is needed here, because the reader
can only be the owner of the io_context:

 - The only path that drops the task's reference is exit_io_context(),
   which the task runs on itself in do_exit() (and on the copy_process()
   error path, where the task never ran). It clears task->io_context
   under task_lock() and only then puts the ioc.
 - So when __get_task_ioprio() runs lockless, p == current, and current
   cannot be inside exit_io_context() at the same time. Once the pointer
   is non-NULL it stays valid for as long as current is executing; the
   ioc cannot be freed under it.
 - For p != current the function already requires p->alloc_lock
   (lockdep_assert_held()), and that is what serializes against
   exit_io_context().

The only thing another task can change under the lockless reader is the
NULL -> ioc transition and ->ioprio, both done by set_task_ioprio() under
task_lock(), and that is exactly what the KCSAN report shows
("value changed: 0x0 -> 0xffff..."). The patch only needs to make that
publication and the ->ioprio accesses well defined, not to extend the
lifetime of the ioc.

I can add a sentence to the commit message and a short comment in
__get_task_ioprio() stating this lifetime rule in a v2 if you think it
helps.

Thanks,
Nguyen Ngoc Thang

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

[Damien Le Moal]

On 2026/10/19 14:18, Nguyen Ngoc Thang wrote:
> Hi Damien,
> 
> Thanks for looking. I don't think RCU is needed here, because the reader
> can only be the owner of the io_context:
> 
>  - The only path that drops the task's reference is exit_io_context(),
>    which the task runs on itself in do_exit() (and on the copy_process()
>    error path, where the task never ran). It clears task->io_context
>    under task_lock() and only then puts the ioc.
>  - So when __get_task_ioprio() runs lockless, p == current, and current
>    cannot be inside exit_io_context() at the same time. Once the pointer
>    is non-NULL it stays valid for as long as current is executing; the
>    ioc cannot be freed under it.
>  - For p != current the function already requires p->alloc_lock
>    (lockdep_assert_held()), and that is what serializes against
>    exit_io_context().
> 
> The only thing another task can change under the lockless reader is the
> NULL -> ioc transition and ->ioprio, both done by set_task_ioprio() under
> task_lock(), and that is exactly what the KCSAN report shows
> ("value changed: 0x0 -> 0xffff..."). The patch only needs to make that
> publication and the ->ioprio accesses well defined, not to extend the
> lifetime of the ioc.
> 
> I can add a sentence to the commit message and a short comment in
> __get_task_ioprio() stating this lifetime rule in a v2 if you think it
> helps.

Yes, a comment would be nice.

-- 
Damien Le Moal
Western Digital Research

[PATCH v2] block: annotate lockless task ioprio accesses

[Nguyen Ngoc Thang]

__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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=feeb9b5352b9ff89618a
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v2:
- Document why no RCU is needed: only the owning task drops its io_context
  (Damien).  Comment in __get_task_ioprio() and a paragraph in the commit
  message.  No code change.
- v1: https://lore.kernel.org/r/20261018071636.79198-1-ngocthang2710.1999@gmail.com

Tested with a KCSAN kernel in QEMU: 26 data-race reports per run before, 0
in two runs after (v1 code, v2 differs only in comments).
 block/blk-ioc.c        | 5 +++--
 include/linux/ioprio.h | 9 +++++++--
 2 files changed, 10 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..d148548691c7 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -56,7 +56,12 @@ 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;
+	/*
+	 * Only the task itself drops its io_context (exit_io_context()), so
+	 * for p == current a non-NULL ioc stays valid.  Another task can
+	 * still install it or change ->ioprio, hence the annotations.
+	 */
+	struct io_context *ioc = READ_ONCE(p->io_context);
 	int prio;
 
 	if (!ioc)
@@ -66,7 +71,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));
-- 
2.43.0

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

[Damien Le Moal]

On 2026/10/19 14:58, 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=feeb9b5352b9ff89618a
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---
> v2:
> - Document why no RCU is needed: only the owning task drops its io_context
>   (Damien).  Comment in __get_task_ioprio() and a paragraph in the commit
>   message.  No code change.
> - v1: https://lore.kernel.org/r/20261018071636.79198-1-ngocthang2710.1999@gmail.com
> 
> Tested with a KCSAN kernel in QEMU: 26 data-race reports per run before, 0
> in two runs after (v1 code, v2 differs only in comments).
>  block/blk-ioc.c        | 5 +++--
>  include/linux/ioprio.h | 9 +++++++--
>  2 files changed, 10 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..d148548691c7 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -56,7 +56,12 @@ 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;
> +	/*
> +	 * Only the task itself drops its io_context (exit_io_context()), so
> +	 * for p == current a non-NULL ioc stays valid.  Another task can
> +	 * still install it or change ->ioprio, hence the annotations.
> +	 */
> +	struct io_context *ioc = READ_ONCE(p->io_context);

Personally, I do not find comment blocks in the middle of declarations pretty.
Can you maybe rewrite this like:

	struct io_context *ioc;
	int prio;

	/*
	 * Only the task itself drops its io_context (exit_io_context()), so
	 * for p == current a non-NULL ioc stays valid.  Another task can
	 * still install it or change ->ioprio, hence the annotations.
	 */
	ioc = READ_ONCE(p->io_context);
	if (!ioc)
		...

Also, the comment mentions "annotation" which I am not sure is the right naming
for READ_ONCE(). It is also a little confusing as that hints at something like
lockdep annotations, which is not what is doe here.

So maybe the comment should be something like:

	/*
	 * Only the current task itself drops its io_context on
	 * exit_io_context()), so a non-NULL ioc is always valid.
	 * However, another task may change ->io_context or ioc->ioprio,
	 * so make sure to get the latest value with READ_ONCE().
	 */
	ioc = READ_ONCE(p->io_context);
	if (!ioc)
		...>  	
>  
>  	if (!ioc)
> @@ -66,7 +71,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