Re: [PATCH RFC 15/20] attestation: Prepare provider and instance core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] attestation: Mixing guard() and goto in attestation_registry_insert
- [Low] attestation: ID space exhaustion due to non-recycling monotonic index
--

commit f05c5cea6b4219fc6f6c64ba092550bdd10f870b
Author: Jiri Pirko <jiri@resnulli.us>

attestation: Prepare provider and instance core

Add provider validation, instance registration and lifecycle management for
the attestation family.

> diff --git a/drivers/attestation/core/core.c b/drivers/attestation/core/core.c
> new file mode 100644
> index 0000000000000..e112d3a0ff2b3
> --- /dev/null
> +++ b/drivers/attestation/core/core.c

[ ... ]

> +static int attestation_registry_insert(struct attestation *att)
> +{
> +	unsigned long index;
> +	void *old;
> +	int ret;
> +
> +	guard(mutex)
> +		(&attestation_registry_lock);
> +	if (attestation_next_index > UINT_MAX)
> +		return -ENOSPC;

[Severity: Low]
Because attestation_next_index is a monotonically increasing counter that is
never recycled upon instance unregistration, will this eventually exhaust the
32-bit ID space after enough hotplug or module reload cycles, preventing any
new providers from registering until reboot?

Could we use ida_alloc() or xa_alloc() to safely allocate and recycle
identifiers instead?

> +	index = attestation_next_index;
> +	ret = xa_reserve(&attestation_instances, index, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +
> +	xa_lock(&attestation_instances);
> +	if (attestation_parent_exists(ctlv_device_parent(att->ctlvdev))) {
> +		__xa_erase(&attestation_instances, index);
> +		ret = -EEXIST;
> +		goto out_unlock;

[Severity: Medium]
Does this manual goto mix incorrectly with the scope-based guard(mutex)
declared at the start of this function?

According to the cleanup guidelines, using goto-based error handling in the
same function as scope-based cleanup creates confusing ownership semantics and
increases the risk of bugs.

Could the xa_lock() be converted to use scope-based cleanup (e.g.,
guard(spinlock)), or the goto be eliminated to align with the guidelines?

> +	}
> +	old = __xa_store(&attestation_instances, index, att, GFP_NOWAIT);
> +	ret = xa_err(old);
> +	if (!ret) {
> +		att->index = index;
> +		attestation_next_index++;
> +	}
> +out_unlock:
> +	xa_unlock(&attestation_instances);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015145053.3664509-1-jiri@resnulli.us?part=15