[PATCH] Xarray: Fix race in xa_get_order()

[PATCH] Xarray: Fix race in xa_get_order()

[Han Xing Yi]

Hello! This is my first patch ever, so please bear with me if I make some rookie
mistakes. I've tried my best to follow the documentation :) 

KCSAN detects data-race in xa_get_order:1779 / xas_store:819. xas_store() uses
rcu_assign_pointer() for the slots pointer, but xa_get_order() does not
use rcu to access the same pointer, resulting in a value change from 
0x0000000000000000 -> 0xffffea0000488f00.

Use rcu_dereference() to access the rcu slots pointer in xa_get_order() to
avoid KCSAN warnings. 

Full bug report:
==================================================================
BUG: KCSAN: data-race in xa_get_order / xas_store

write (marked) to 0xffff88800b4cb150 of 8 bytes by task 239 on cpu 1:
 xas_store+0x6c8/0x11d0 lib/xarray.c:819
 __filemap_add_folio+0x61a/0xb30 mm/filemap.c:898
 filemap_add_folio+0x69/0x160 mm/filemap.c:937
 page_cache_ra_unbounded+0x134/0x3b0 mm/readahead.c:250
 do_page_cache_ra mm/readahead.c:299 [inline]
 page_cache_ra_order+0xc4/0xe0 mm/readahead.c:546
 do_sync_mmap_readahead mm/filemap.c:3150 [inline]
 filemap_fault+0xe45/0x1960 mm/filemap.c:3242
 __do_fault+0x8e/0x2c0 mm/memory.c:4266
 do_read_fault mm/memory.c:4629 [inline]
 do_fault mm/memory.c:4763 [inline]
 do_pte_missing mm/memory.c:3731 [inline]
 handle_pte_fault mm/memory.c:5039 [inline]
 __handle_mm_fault+0xd96/0x1df0 mm/memory.c:5180
 handle_mm_fault+0x227/0x6a0 mm/memory.c:5345
 do_user_addr_fault+0x2d5/0xf30 arch/x86/mm/fault.c:1364
 handle_page_fault arch/x86/mm/fault.c:1507 [inline]
 exc_page_fault+0xa9/0x330 arch/x86/mm/fault.c:1563
 asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:570

read to 0xffff88800b4cb150 of 8 bytes by task 235 on cpu 0:
 xa_get_order+0x1a2/0x400 lib/xarray.c:1779
 __filemap_add_folio+0x44c/0xb30 mm/filemap.c:870
 filemap_add_folio+0x69/0x160 mm/filemap.c:937
 page_cache_ra_unbounded+0x134/0x3b0 mm/readahead.c:250
 do_page_cache_ra mm/readahead.c:299 [inline]
 page_cache_ra_order+0xc4/0xe0 mm/readahead.c:546
 do_sync_mmap_readahead mm/filemap.c:3150 [inline]
 filemap_fault+0xe45/0x1960 mm/filemap.c:3242
 __do_fault+0x8e/0x2c0 mm/memory.c:4266
 do_read_fault mm/memory.c:4629 [inline]
 do_fault mm/memory.c:4763 [inline]
 do_pte_missing mm/memory.c:3731 [inline]
 handle_pte_fault mm/memory.c:5039 [inline]
 __handle_mm_fault+0xd96/0x1df0 mm/memory.c:5180
 handle_mm_fault+0x227/0x6a0 mm/memory.c:5345
 do_user_addr_fault+0x2d5/0xf30 arch/x86/mm/fault.c:1364
 handle_page_fault arch/x86/mm/fault.c:1507 [inline]
 exc_page_fault+0xa9/0x330 arch/x86/mm/fault.c:1563
 asm_exc_page_fault+0x26/0x30 arch/x86/include/asm/idtentry.h:570

value changed: 0x0000000000000000 -> 0xffffea0000488f00

Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 235 Comm: gcc Not tainted 6.7.0-g65c265174d11-dirty #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
==================================================================

Signed-off-by: Han Xing Yi <hxingyi104@gmail.com>
---
 lib/xarray.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/xarray.c b/lib/xarray.c
index 39f07bfc4dcc..7fc225f3cf4e 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1776,7 +1776,7 @@ int xa_get_order(struct xarray *xa, unsigned long index)
 
 		if (slot >= XA_CHUNK_SIZE)
 			break;
-		if (!xa_is_sibling(xas.xa_node->slots[slot]))
+		if (!xa_is_sibling(rcu_dereference(xas.xa_node->slots[slot])))
 			break;
 		order++;
 	}
-- 
2.34.1

Re: [PATCH] Xarray: Fix race in xa_get_order()

[Matthew Wilcox]

On Sat, Mar 02, 2024 at 10:13:28PM +0800, Han Xing Yi wrote:
> Hello! This is my first patch ever, so please bear with me if I make some rookie
> mistakes. I've tried my best to follow the documentation :) 

Thanks!  This is indeed a mistake.  Probably a harmless one, but worth
fixing to silence the warning.

Annoyingly, building with C=1 (sparse) finds the problem:

  CHECK   ../lib/xarray.c
../lib/xarray.c:1779:54: warning: incorrect type in argument 1 (different address spaces)
../lib/xarray.c:1779:54:    expected void const *entry
../lib/xarray.c:1779:54:    got void [noderef] __rcu *

so that means I got out of the habit of running sparse, and for some
reason none of the build bots notified me of the new warning (or I
missed that email).

> +++ b/lib/xarray.c
> @@ -1776,7 +1776,7 @@ int xa_get_order(struct xarray *xa, unsigned long index)
>  
>  		if (slot >= XA_CHUNK_SIZE)
>  			break;
> -		if (!xa_is_sibling(xas.xa_node->slots[slot]))
> +		if (!xa_is_sibling(rcu_dereference(xas.xa_node->slots[slot])))

This is such a common thing to do that I have a helper for it.
So what I'll actually commit is:

-               if (!xa_is_sibling(xas.xa_node->slots[slot]))
+               if (!xa_is_sibling(xa_entry(xas.xa, xas.xa_node, slot)))

but I'll leave your name on it since you did the actual work.

Re: [PATCH] Xarray: Fix race in xa_get_order()

[Han Xing Yi]

On Sun, Mar 03, 2024 at 12:11:05PM +0000, Matthew Wilcox wrote:
> On Sat, Mar 02, 2024 at 10:13:28PM +0800, Han Xing Yi wrote:
> > Hello! This is my first patch ever, so please bear with me if I make some rookie
> > mistakes. I've tried my best to follow the documentation :) 
> 
> Thanks!  This is indeed a mistake.  Probably a harmless one, but worth
> fixing to silence the warning.

Yeah, it is probably harmless since the RCU write means that either the
old or new value will be read, but not a temporary value. Thanks for
pointing this out!

> Annoyingly, building with C=1 (sparse) finds the problem:
> 
>   CHECK   ../lib/xarray.c
> ../lib/xarray.c:1779:54: warning: incorrect type in argument 1 (different address spaces)
> ../lib/xarray.c:1779:54:    expected void const *entry
> ../lib/xarray.c:1779:54:    got void [noderef] __rcu *
> 
> so that means I got out of the habit of running sparse, and for some
> reason none of the build bots notified me of the new warning (or I
> missed that email).

I'm so sorry for this mistake, I did not build the kernel with sparse
when testing the patch. I'll be sure to do that next time around. 

> This is such a common thing to do that I have a helper for it.
> So what I'll actually commit is:
> 
> -               if (!xa_is_sibling(xas.xa_node->slots[slot]))
> +               if (!xa_is_sibling(xa_entry(xas.xa, xas.xa_node, slot)))
> 
> but I'll leave your name on it since you did the actual work.

Thank you so much for fixing my mistakes and your quick response on
this, I really appreciate it :)