Re: [PATCH v4 3/3] hwspinlock: add low level debug capabilities

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

> > +static struct hwspinlock *hwspin_lock_ll_debug_get(u64 id)
> > +{
> > +	struct hwspinlock *hwlock;
> > +	unsigned long index = id;
> 
> [Severity: Low]
> Does casting the u64 id to an unsigned long truncate the value on
> 32-bit architectures, causing operations to mistakenly target a different
> lock if the provided ID exceeds 32 bits?

Addressed in v5.

> 
> > +
> > +	rcu_read_lock();
> > +	hwlock = xa_find(&hwspinlocks, &index, ULONG_MAX, XA_PRESENT);
> 
> [Severity: Medium]
> Since xa_find() searches for the first present entry at or above the index
> and updates the index variable, will this silently operate on an
> unrelated active lock if the exact requested id is not present, instead
> of using xa_load() for an exact-ID lookup?

Addressed in v5.

> [Severity: Low]
> hwspin_lock_request_specific() expects an unsigned int id. Does passing
> the u64 id here silently truncate it on 64-bit architectures as well?

Addressed in v5.

> > +static int fops_trylock_raw_set(void *data, u64 id)
> > +{
> > +	struct hwspinlock *hwlock = hwspin_lock_ll_debug_get(id);
> > +
> > +	return hwlock ? hwspin_trylock_raw(hwlock) : -ENOENT;
> > +}
> 
> [Severity: High]
> Do these raw debugfs accessors invoke memory-mapped I/O registers without
> ensuring the device's clocks and power domains are active?
> 
> Since this interface explicitly allows operating on unrequested locks
> (which are likely powered off), can accessing the registers trigger an
> asynchronous external abort or synchronous bus fault on typical SoCs?

True but it is still a great debugging help. I extended the comment to
explain this potential problem.

> > +		ret = hwspin_trylock(hwlock);
> > +		if (ret != -EBUSY) {
> > +			hwspin_lock_free(hwlock);
> > +			return -EACCES;
> > +		}
> 
> [Severity: High]
> If the second hwspin_trylock() unexpectedly succeeds, it enters this
> error path and returns -EACCES. Since the first hwspin_trylock()
> disabled preemption and took the hardware lock, does returning here
> without calling hwspin_unlock() leave preemption permanently disabled?

Addressed in v5.

The rest were pre-existing UAF issues to be fixed in the next series.