linux-kernel - Re: [PATCH 2/2] lib/test_xarray.c: fix error assumptions on check_xa_multi_store_adv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dodldgslvisjrqfnkki2ypt3qejn6gb6l25eewv2euywho4unh@jlbkllb6l3ct>
Date: Wed, 24 Apr 2024 11:16:01 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: akpm@...ux-foundation.org, willy@...radead.org,
        linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, djwong@...nel.org, david@...morbit.com,
        gost.dev@...sung.com, p.raghav@...sung.com, da.gomez@...sung.com
Subject: Re: [PATCH 2/2] lib/test_xarray.c: fix error assumptions on
 check_xa_multi_store_adv_add()

* Luis Chamberlain <mcgrof@...nel.org> [240423 14:05]:
> While testing lib/test_xarray in userspace I've noticed we can fail with:
> 
> make -C tools/testing/radix-tree
> ./tools/testing/radix-tree/xarray
> 
> BUG at check_xa_multi_store_adv_add:749
> xarray: 0x55905fb21a00x head 0x55905fa1d8e0x flags 0 marks 0 0 0
> 0: 0x55905fa1d8e0x
> xarray: ../../../lib/test_xarray.c:749: check_xa_multi_store_adv_add: Assertion `0' failed.
> Aborted
> 
> We get a failure with a BUG_ON(), and that is because we actually can
> fail due to -ENOMEM, the check in xas_nomem() will fix this for us so
> it makes no sense to expect no failure inside the loop. So modify the
> check and since this is also useful for instructional purposes clarify
> the situation.

The default behaviour in the testing framework is to test the error
path, which is what you are seeing with the less likely return of
-ENOMEM.

> 
> The check for XA_BUG_ON(xa, xa_load(xa, index) != p) is already done
> at the end of the loop so just remove the bogus on inside the loop.
> 
> With this we now pass the test in both kernel and userspace:
> 
> In userspace:
> 
> ./tools/testing/radix-tree/xarray
> XArray: 149092856 of 149092856 tests passed
> 
> In kernel space:
> 
> XArray: 148257077 of 148257077 tests passed
> 
> Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>

Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>

> ---
>  lib/test_xarray.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/test_xarray.c b/lib/test_xarray.c
> index ebe2af2e072d..5ab35190aae3 100644
> --- a/lib/test_xarray.c
> +++ b/lib/test_xarray.c
> @@ -744,15 +744,20 @@ static noinline void check_xa_multi_store_adv_add(struct xarray *xa,
>  
>  	do {
>  		xas_lock_irq(&xas);
> -
>  		xas_store(&xas, p);
> -		XA_BUG_ON(xa, xas_error(&xas));
> -		XA_BUG_ON(xa, xa_load(xa, index) != p);
> -
>  		xas_unlock_irq(&xas);
> +		/*
> +		 * In our selftest case the only failure we can expect is for
> +		 * there not to be enough memory as we're not mimicking the
> +		 * entire page cache, so verify that's the only error we can run
> +		 * into here. The xas_nomem() which follows will ensure to fix
> +		 * that condition for us so to chug on on the loop.
> +		 */
> +		XA_BUG_ON(xa, xas_error(&xas) && xas_error(&xas) != -ENOMEM);
>  	} while (xas_nomem(&xas, GFP_KERNEL));
>  
>  	XA_BUG_ON(xa, xas_error(&xas));
> +	XA_BUG_ON(xa, xa_load(xa, index) != p);
>  }
>  
>  /* mimics page_cache_delete() */
> -- 
> 2.43.0
>