[PATCH] locking/osq_lock: annotate a data race in osq_wait_next

[PATCH] locking/osq_lock: annotate a data race in osq_wait_next

[Jiayi Li]

KCSAN reported a data race in osq_unlock():

  BUG: KCSAN: data-race in osq_unlock+0xa6/0x120

  race at unknown origin, with read to 0xffff8e2c799b7600 of 8 bytes
  by task 6598 on cpu 11:
    osq_unlock+0xa6/0x120
    generic_osq_unlock+0xe/0x20
    rwsem_down_write_slowpath+0x78a/0xd50
    down_write+0xef/0x100
    vma_link_file+0x4c/0xb0
    mmap_region+0x865/0x12a0
    do_mmap+0x5b5/0x8d0

  value changed: 0x0000000000000000 -> 0xffff8e2c796b7600
  Reported by Kernel Concurrency Sanitizer on:
  CPU: 11 PID: 6598 Comm: modprobe Not tainted 6.6.103+ #588

static inline struct optimistic_spin_node *
osq_wait_next(struct optimistic_spin_queue *lock,
              struct optimistic_spin_node *node,
              struct optimistic_spin_node *prev)
{
        struct optimistic_spin_node *next = NULL;
        int curr = encode_cpu(smp_processor_id());
        int old;

        old = prev ? prev->cpu : OSQ_UNLOCKED_VAL;

        for (;;) {

                ......

                if (node->next) {      <--------------------------
                        next = xchg(&node->next, NULL);
                        if (next)
                                break;
                }

                cpu_relax();
        }

        return next;
}

The read in osq_wait_next() only tests if node->next is non-NULL before
using xchg() to fetch and clear the actual successor. Even if the load
observes a transient value, the code is still working correctly. Thus,
mark it as an intentional data race using the data_race() macro.

Signed-off-by: Jiayi Li <lijiayi@kylinos.cn>
---
 kernel/locking/osq_lock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index b4233dc2c2b0..ab87d06405f9 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -78,7 +78,7 @@ osq_wait_next(struct optimistic_spin_queue *lock,
 		 * wait for either @lock to point to us, through its Step-B, or
 		 * wait for a new @node->next from its Step-C.
 		 */
-		if (node->next) {
+		if (data_race(node->next)) { /* xchg() validates the value. */
 			struct optimistic_spin_node *next;
 
 			next = xchg(&node->next, NULL);
-- 
2.34.1

Re: [PATCH] locking/osq_lock: annotate a data race in osq_wait_next

[Waiman Long]

On 6/10/26 10:08 PM, Jiayi Li wrote:
> KCSAN reported a data race in osq_unlock():
>
>    BUG: KCSAN: data-race in osq_unlock+0xa6/0x120
>
>    race at unknown origin, with read to 0xffff8e2c799b7600 of 8 bytes
>    by task 6598 on cpu 11:
>      osq_unlock+0xa6/0x120
>      generic_osq_unlock+0xe/0x20
>      rwsem_down_write_slowpath+0x78a/0xd50
>      down_write+0xef/0x100
>      vma_link_file+0x4c/0xb0
>      mmap_region+0x865/0x12a0
>      do_mmap+0x5b5/0x8d0
>
>    value changed: 0x0000000000000000 -> 0xffff8e2c796b7600
>    Reported by Kernel Concurrency Sanitizer on:
>    CPU: 11 PID: 6598 Comm: modprobe Not tainted 6.6.103+ #588
>
> static inline struct optimistic_spin_node *
> osq_wait_next(struct optimistic_spin_queue *lock,
>                struct optimistic_spin_node *node,
>                struct optimistic_spin_node *prev)
> {
>          struct optimistic_spin_node *next = NULL;
>          int curr = encode_cpu(smp_processor_id());
>          int old;
>
>          old = prev ? prev->cpu : OSQ_UNLOCKED_VAL;
>
>          for (;;) {
>
>                  ......
>
>                  if (node->next) {      <--------------------------
>                          next = xchg(&node->next, NULL);
>                          if (next)
>                                  break;
>                  }
>
>                  cpu_relax();
>          }
>
>          return next;
> }
>
> The read in osq_wait_next() only tests if node->next is non-NULL before
> using xchg() to fetch and clear the actual successor. Even if the load
> observes a transient value, the code is still working correctly. Thus,
> mark it as an intentional data race using the data_race() macro.
>
> Signed-off-by: Jiayi Li <lijiayi@kylinos.cn>
> ---
>   kernel/locking/osq_lock.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> index b4233dc2c2b0..ab87d06405f9 100644
> --- a/kernel/locking/osq_lock.c
> +++ b/kernel/locking/osq_lock.c
> @@ -78,7 +78,7 @@ osq_wait_next(struct optimistic_spin_queue *lock,
>   		 * wait for either @lock to point to us, through its Step-B, or
>   		 * wait for a new @node->next from its Step-C.
>   		 */
> -		if (node->next) {
> +		if (data_race(node->next)) { /* xchg() validates the value. */
>   			struct optimistic_spin_node *next;
>   
>   			next = xchg(&node->next, NULL);
Acked-by: Waiman Long <longman@redhat.com>