3 XArray questions (was: Re: [PATCH v3 1/2] hwspinlock: Convert to XArray)

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

Hi Matthew,

Sashiko found issues with the XArray conversion[1]. I tried to educate
myself enough to evaluate its findings. Yet, I'd be happy if you could
double-check my reasoning. Note: I left all issues with use-after-free
aside. Reason is that the hwspinlock subsystem has so many problems with
removing locks that I proposed to remove the removal[2].

[1] https://sashiko.dev/#/patchset/20260713124622.63181-1-wsa%2Brenesas%40sang-engineering.com
[2] https://lkml.iu.edu/2605.1/07475.html

Now for the issues:

> > +	xas_lock(&xas);
> > +	existing = xas_load(&xas);
> > +	if (existing) {
> > +		pr_err("hwspinlock id %d already exists!\n", id);
> > +		xas_set_err(&xas, -EBUSY);
> >  	}
> > +	xas_store(&xas, hwlock);
> 
> [Severity: High]
> Could xas_store() fail with -ENOMEM under mild memory fragmentation?
> 
> Because xas_lock() acquires a spinlock, XArray's internal node allocations
> are restricted to GFP_NOWAIT. XArray typically requires callers modifying the
> array under a lock to wrap the operation in a xas_nomem() retry loop,
> similar to:
> 
>     do {
>         xas_lock(&xas);
>         ...
>         xas_store(&xas, hwlock);
>         ...
>         xas_unlock(&xas);
>     } while (xas_nomem(&xas, GFP_KERNEL));
> 
> Without this loop, the GFP_NOWAIT allocation failure would be fatal. Could
> this cause hardware spinlock registration to fail spuriously?

Reading the documentation of xas_nomem(), I think Sashiko is right and
such a loop is needed, or?

> >  	/* mark this hwspinlock as available */
> > -	tmp = radix_tree_tag_set(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
> > +	xas_set_mark(&xas, HWSPINLOCK_UNUSED);
> > +	ret = xas_error(&xas);
> > +	xas_unlock(&xas);
> >  
> > -	/* self-sanity check which should never fail */
> > -	WARN_ON(tmp != hwlock);
> > -
> > -out:
> > -	mutex_lock(&hwspinlock_tree_lock);
> >  	return ret;
> >  }
> >  
> >  static struct hwspinlock *hwspin_lock_unregister_single(unsigned int id)
> >  {
> > +	XA_STATE(xas, &hwspinlocks, id);
> >  	struct hwspinlock *hwlock = NULL;
> > -	int ret;
> > +	bool unused;
> >  
> > -	mutex_lock(&hwspinlock_tree_lock);
> > +	xas_lock(&xas);
> >  
> > -	/* make sure the hwspinlock is not in use (tag is set) */
> > -	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
> > -	if (ret == 0) {
> > +	/* make sure the hwspinlock is not in use (mark is set) */
> > +	unused = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
> 
> [Severity: High]
> Does xas_get_mark() return the correct value here?
> 
> Since the XA_STATE is freshly initialized and xas_load() hasn't been called
> to walk the tree, xas->xa_node is NULL. In this state, xas_get_mark() queries
> the global root mark of the XArray.
> 
> If any lock in the XArray is unused, won't this return true, potentially
> allowing active locks to be erroneously deleted?

Despite I intend to remove this function in the future anyhow, the fix
for this issue is a simple 'xas_load(&xas);' right before
'xas_get_mark()'?

> > @@ -782,12 +753,12 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
> >  	}
> >  
> >  	dev = hwlock->bank->dev;
> > -	mutex_lock(&hwspinlock_tree_lock);
> > +	xas_lock(&xas);
> > +	xas_set(&xas, hwlock_to_id(hwlock));
> >  
> >  	/* make sure the hwspinlock is used */
> > -	ret = radix_tree_tag_get(&hwspinlock_tree, hwlock_to_id(hwlock),
> > -							HWSPINLOCK_UNUSED);
> > -	if (ret == 1) {
> > +	unused = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
> 
> [Severity: High]
> Will xas_get_mark() silently abort here?
> 
> Calling xas_set() initializes the state to XAS_RESTART, which makes
> xas_invalid() evaluate to true. Because of this, both xas_get_mark() and the
> subsequent xas_set_mark() further down will silently return or abort without
> performing their intended operations.
> 
> Could this prevent the lock from being marked as available again, effectively
> leaking it permanently?

Not sure if I parse this correctly, but I think xas_load() is also
needed after xas_set() and before xas_get_mark()?

Thanks and happy hacking,

   Wolfram