[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

[PATCH v3] 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>
---
v3:
- Move the comment out of the declarations and reword it (Damien).
  No code change.

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
- v2: https://lore.kernel.org/r/20261019125811.21666-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, later versions differ only in comments).
 block/blk-ioc.c        |  5 +++--
 include/linux/ioprio.h | 11 +++++++++--
 2 files changed, 12 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..be999cbe7c91 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -56,9 +56,16 @@ 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;
+	struct io_context *ioc;
 	int prio;
 
+	/*
+	 * Only the current task itself drops its io_context, in
+	 * 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)
 		return IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
 					 task_nice_ioprio(p));
@@ -66,7 +73,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 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=feeb9b5352b9ff89618a
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

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

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

[Nguyen Ngoc Thang]

Hi Damien,

Title: agreed, I'll change it (v4 below).

On the pairing: a release store pairs with any load that is ordered after
the pointer load, and here the ordering comes from the address dependency,
not from an acquire.  The only thing the reader does with the loaded
pointer is dereference it (ioc->ioprio), and the kernel memory model orders
a dependent load after the READ_ONCE() that produced its address.  This is
the same pairing as rcu_assign_pointer()/rcu_dereference() (see "ADDRESS
DEPENDENCY" in Documentation/memory-barriers.txt); READ_ONCE() also carries
the extra barrier Alpha needs (arch/alpha/include/asm/rwonce.h).
smp_load_acquire() would only be needed if the reader looked at data not
dependent on the pointer, and would add a barrier on arm64/powerpc for
nothing.

Why not plain WRITE_ONCE(): it would silence KCSAN, but it does not order
the initialization of the new io_context (alloc_io_context() runs before
task_lock(), and spin_lock() is only an acquire, so those stores may be
reordered after the pointer store on weakly ordered CPUs). A reader could
then see the pointer without seeing the initialized ioc. The release is
what makes the publication safe.

If you'd rather keep this minimal, I can drop the release and use
WRITE_ONCE(), but I think the release is the correct thing to have.

v4 follows with the new title and a comment/commit-message paragraph
explaining the above.

Thanks,
Nguyen Ngoc Thang

[PATCH v4] block: use READ_ONCE() to access the task io_context ioprio

[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.

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.

Read the pointer and ->ioprio with READ_ONCE() and write ->ioprio with
WRITE_ONCE().  Publish a new io_context with smp_store_release() so that
its initialization is visible before the pointer.  The reader only
dereferences the pointer it loaded, so the address dependency of
READ_ONCE() is enough to pair with the release, as with
rcu_assign_pointer() and rcu_dereference(); an acquire is not needed.

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>
---
v4:
- Retitle: "annotate" was too vague (Damien).
- Explain in the commit message and in a comment why smp_store_release()
  pairs with READ_ONCE() and not smp_load_acquire(): the reader only uses
  the loaded pointer, so the address dependency orders the access, as in
  rcu_assign_pointer()/rcu_dereference().  No code change.

v3: https://lore.kernel.org/r/20261020125233.9820-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, later versions differ only in comments).
 block/blk-ioc.c        |  9 +++++++--
 include/linux/ioprio.h | 11 +++++++++--
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index d15918d7fabb..fac62f2858ec 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -277,9 +277,14 @@ 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;
+			/*
+			 * Make the ioc initialization visible before the
+			 * pointer.  Readers use READ_ONCE(), whose address
+			 * dependency is enough, like rcu_dereference().
+			 */
+			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..be999cbe7c91 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -56,9 +56,16 @@ 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;
+	struct io_context *ioc;
 	int prio;
 
+	/*
+	 * Only the current task itself drops its io_context, in
+	 * 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)
 		return IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
 					 task_nice_ioprio(p));
@@ -66,7 +73,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