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

["Chen, Yu C" <yu.c.chen@intel.com>]

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