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
