Re: [PATCH v3 1/6] x86/resctrl: Parse ACPI ERDT table and map RMDD domains by L3 cache ID

[Chen, Yu C]

Hi Thomas,

On 6/8/2026 4:59 PM, Thomas Gleixner wrote:
> On Sat, Jun 06 2026 at 10:32, Chen Yu wrote:
> > +
> > +static inline struct acpi_subtbl_hdr_16 *rmdd_subtbl(struct acpi_erdt_rmdd *rmdd)
> > +{
> > +	return (void *)rmdd + sizeof(*rmdd);
> > +}
> > +
> > +static inline struct acpi_subtbl_hdr_16 *next_subtbl(struct acpi_subtbl_hdr_16 *subtbl)
> > +{
> > +	return (void *)subtbl + subtbl->length;
> > +}
>
> Second thoughts on this. Hit send too fast.
>
> > +static inline bool subtbl_valid(struct acpi_erdt_rmdd *rmdd, struct acpi_subtbl_hdr_16 *subtbl)
> > +{
> > +	void *rmdd_end = (void *)rmdd + rmdd->header.length;
> > +
> > +	if (subtbl->length < sizeof(*subtbl))
> > +		return false;
> > +
> > +	if ((void *)subtbl + sizeof(*subtbl) > rmdd_end)
> > +		return false;
> > +
> > +	if ((void *)subtbl + subtbl->length > rmdd_end)
> > +		return false;
>
> These conditions are confusing. This basically allows subtbl->length to
> be larger than sizeof(tbl) as long as it's within the limits.
>
> If that's intentional then the second condition is pointless because
> according to the first condition
>
>            length >= sizeof(tbl)
>
> so
>
>            tbl + length <= end
>
> is catching both. No?

You are right, subtbl + sizeof(*subtbl) > rmdd_end
is redundant because condition 3 has protected against
any invalid out-of-boundary access. Will remove condition 2.

> > +	return true;
>
> > +static __init int enumerate_erdt_table(struct acpi_table_header *table_hdr)
> > +{
> > +	struct acpi_table_erdt *erdt = (struct acpi_table_erdt *)table_hdr;
> > +	struct acpi_subtbl_hdr_16 *subtbl;
> > +	void *table_end;
> > +
> > +	if (erdt->header.revision != ERDT_VALID_VERSION) {
> > +		pr_info("Unknown ERDT table revision %d\n", erdt->header.revision);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (erdt->header.length < sizeof(*erdt)) {
> > +		pr_info(FW_BUG "ERDT: Invalid table length %u\n", erdt->header.length);
> > +		return -EINVAL;
> > +	}
> > +
> > +	subtbl = (void *)erdt + sizeof(struct acpi_table_erdt);
> > +	table_end = (void *)erdt + erdt->header.length;
> > +
> > +	while ((void *)subtbl + sizeof(*subtbl) <= table_end) {
> > +		if (subtbl->length < sizeof(*subtbl) ||
> > +		    (void *)subtbl + subtbl->length > table_end) {
> > +			pr_info("ERDT: Invalid subtable length\n");
> > +			goto cleanup;
> > +		}
>
> So this is yet another version of the above, just slighty different with
> the same strange conditions.
>
> If you make subtbl_valid():
>
> static inline bool subtbl_valid(void *end, struct acpi_subtbl_hdr_16 *subtbl)
>     
> and calculate the end at the call sites you can reuse that function.
>
> > +
> > +		if (subtbl->type == ACPI_ERDT_TYPE_RMDD)
> > +			if (!parse_rmdd_entry(subtbl))
> > +				goto cleanup;
> > +
> > +		subtbl = (void *)subtbl + subtbl->length;
>
>   open coded variant of next_subtbl()

OK, will reuse next_subtbl() and subtbl_valid(). Thanks!


thanks,
Chenyu