Re: [PATCH v7 07/23] firmware: arm_scmi: Add support to parse SHMTIs areas

[Cristian Marussi <cristian.marussi@arm.com>]

On Mon, Aug 03, 2026 at 11:53:45PM +0100, Fayssal Benmlih wrote:
> Hi Cristian,
> 

Hi,

> I found two UUID database issues that appear to be blockers.
> 
> > 	if (ti->info.num_uuids + SCMI_UUID_DB_THRESH >= ti->uuids_len) {
> > 		uuid_t **uuids, **old_uuids;
> >
> > 		uuids = kcalloc(ti->uuids_len * 2, sizeof(*uuids),
> > 				GFP_KERNEL);
> > 		if (!uuids)
> > 			return -ENOMEM;
> >
> > 		/* Copy/move old allocated UUIDs */
> > 		for (int i = 0; i < ti->info.num_uuids; i++)
> > 			uuid_copy(uuids[i], ti->info.uuids[i]);
> 
> uuids is a newly allocated and zeroed array of uuid_t pointers, so
> uuids[i] is NULL here. uuid_copy() therefore copies into a NULL
> destination when the database grows with existing entries.
> 
> The database stores pointers to UUIDs owned by telemetry_uuid objects, so
> should this instead copy the pointers themselves, for example:
> 
> 	uuids[i] = ti->info.uuids[i];
>

Exactly...my bad .. fixed in V8.

> or use an appropriately sized memcpy() of the pointer array?
> 
> > 	ti->uuids_len = ti->num_shmti * 2;
> > 	ti->info.uuids = kcalloc(ti->uuids_len,
> > 				 sizeof(*ti->info.uuids),
> > 				 GFP_KERNEL);
> 
> A valid implementation can have zero SHMTIs while exposing fast-channel or
> notification-only DEs. In that case uuids_len is zero.
> 
> Primary UUID creation then enters the resize path, doubles zero to zero,
> and eventually writes the primary UUID pointer through a zero-size
> allocation.
> 
> Please give the UUID database a nonzero minimum initial capacity and use
> checked growth so that zero cannot remain zero.
> 

Done in V8, since Primary is always present AND also we'd like to avoid
to immediately resize the Array so initial len is set to at least
SCMI_UUID_DB_THRESH + 1

> > static void scmi_telemetry_line_put(struct telemetry_line *line,
> > 				    void *blob)
> > {
> > 	if (refcount_dec_and_test(&line->users)) {
> > 		xa_erase(line->xa_lines,
> > 			 (unsigned long)line->payld);
> > 		kfree(blob);
> > 	}
> > }
> 
> Lookups and refcount increments are serialized using lines_mtx, but this
> final decrement, XArray erase and free are not performed under the same
> lock.
> 
> A concurrent get-or-create operation can load the entry while another
> thread decrements the refcount to zero and frees it. Please serialize the
> final put with lookup/creation, or use a lifetime scheme such as
> refcount_inc_not_zero() with appropriate XArray/RCU protection.
> 

I have reviewed/reworked all of the lines internal and external mutexing
in V8 due to also a ton of Sashiko reports...

Thanks,
Cristian