linux-kernel - Re: [PATCH] test_xarray: fix soft lockup for advanced-api tests

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 for Android: free password hash cracker in your pocket

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

Date: Mon, 19 Feb 2024 18:28:08 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: willy@...radead.org, linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, gost.dev@...sung.com, p.raghav@...sung.com,
 da.gomez@...sung.com, kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] test_xarray: fix soft lockup for advanced-api tests

On Fri, 16 Feb 2024 11:43:29 -0800 Luis Chamberlain <mcgrof@...nel.org> wrote:

> The new adanced API tests

So this is a fix against the mm-unstable series "test_xarray: advanced
API multi-index tests", v2.

> want to vet the xarray API is doing what it
> promises by manually iterating over a set of possible indexes on its
> own, and using a query operation which holds the RCU lock and then
> releases it. So it is not using the helper loop options which xarray
> provides on purpose. Any loop which iterates over 1 million entries
> (which is possible with order 20, so emulating say a 4 GiB block size)
> to just to rcu lock and unlock will eventually end up triggering a soft
> lockup on systems which don't preempt, and have lock provin and RCU
> prooving enabled.
> 
> xarray users already use XA_CHECK_SCHED for loops which may take a long
> time, in our case we don't want to RCU unlock and lock as the caller
> does that already, but rather just force a schedule every XA_CHECK_SCHED
> iterations since the test is trying to not trust and rather test that
> xarray is doing the right thing.
> 
> [0] https://lkml.kernel.org/r/202402071613.70f28243-lkp@intel.com
> 
> Reported-by: kernel test robot <oliver.sang@...el.com>

As the above links shows, this should be

Reported-by: kernel test robot <oliver.sang@...el.com>
Closes: https://lore.kernel.org/oe-lkp/202402071613.70f28243-lkp@intel.com

> --- a/lib/test_xarray.c
> +++ b/lib/test_xarray.c
> @@ -781,6 +781,7 @@ static noinline void *test_get_entry(struct xarray *xa, unsigned long index)
>  {
>  	XA_STATE(xas, xa, index);
>  	void *p;
> +	static unsigned int i = 0;

I don't think this needs static storage.

PetPeeve: it is unexpected that `i' has unsigned type.  Can a more
communicative identifier be used?


I shall queue your patch as a fixup patch against
test_xarray-add-tests-for-advanced-multi-index-use and shall add the
below on top.  Pleae check.

--- a/lib/test_xarray.c~test_xarray-fix-soft-lockup-for-advanced-api-tests-fix
+++ a/lib/test_xarray.c
@@ -728,7 +728,7 @@ static noinline void *test_get_entry(str
 {
 	XA_STATE(xas, xa, index);
 	void *p;
-	static unsigned int i = 0;
+	unsigned int loops = 0;
 
 	rcu_read_lock();
 repeat:
@@ -746,7 +746,7 @@ repeat:
 	 * APIs won't be stupid, proper page cache APIs loop over the proper
 	 * order so when using a larger order we skip shared entries.
 	 */
-	if (++i % XA_CHECK_SCHED == 0)
+	if (++loops % XA_CHECK_SCHED == 0)
 		schedule();
 
 	return p;
_