Re: [PATCH v5 01/24] x86/resctrl: Track the closid with the rmid

[James Morse <james.morse@arm.com>]

Hi Reinette,

On 09/08/2023 23:32, Reinette Chatre wrote:
> On 7/28/2023 9:42 AM, James Morse wrote:
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index ded1fc7cb7cb..fa66029de41c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -470,7 +480,8 @@ void mon_event_count(void *info)
>>  
>>  	if (rdtgrp->type == RDTCTRL_GROUP) {
>>  		list_for_each_entry(entry, head, mon.crdtgrp_list) {
>> -			if (__mon_event_count(entry->mon.rmid, rr) == 0)
>> +			if (__mon_event_count(rdtgrp->closid, entry->mon.rmid,
>> +					      rr) == 0)
>>  				ret = 0;
>>  		}
>>  	}

> I understand that the parent and child resource groups should have the same
> closid, but that makes me wonder why you use the parent closid in this change,
> but later in the change to mbm_handle_overflow() where the monitor groups are
> traversed you use the closid from the child resource group?

I'd intended to always use the values from the same struct, as that is the least
surprising thing to do. This is the odd one out, I'll fix it.


>> @@ -732,10 +744,11 @@ static int dom_data_init(struct rdt_resource *r)
>>  	}
>>  
>>  	/*
>> -	 * RMID 0 is special and is always allocated. It's used for all
>> -	 * tasks that are not monitored.
>> +	 * CLOSID 0 and RMID 0 are special and are always allocated. These are
>> +	 * used for rdtgroup_default control group, which will be setup later.
>> +	 * See rdtgroup_setup_root().
>>  	 */
>> -	entry = __rmid_entry(0);
>> +	entry = __rmid_entry(0, 0);
> 
> There seems to be an ordering issue here with the hardcoded values for 
> RESCTRL_RESERVED_CLOSID and RESCTRL_RESERVED_RMID used before those defines
> are introduced in the next patch. That may be ok since this code changes in
> the next patch ... but the comment is left referring to the constant. Maybe
> it would just be clearer if the defines are moved to this patch?

Sure,


>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 725344048f85..f7fda4fc2c9e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -3252,7 +3252,7 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>>  	return 0;
>>  
>>  out_idfree:
>> -	free_rmid(rdtgrp->mon.rmid);
>> +	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>  out_destroy:
>>  	kernfs_put(rdtgrp->kn);
>>  	kernfs_remove(rdtgrp->kn);
> 
> This does not look right ... as you note in later patches closid_alloc() is called
> _after_ mkdir_rdt_prepare(). Adding rdtgrp->closid to free_rmid() at this point would
> thus use an uninitialized value. I know this code is being moved in subsequent
> patches so it seems the patches may need to be reordered?
> 
>> @@ -3266,7 +3266,7 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>>  static void mkdir_rdt_prepare_clean(struct rdtgroup *rgrp)
>>  {
>>  	kernfs_remove(rgrp->kn);
>> -	free_rmid(rgrp->mon.rmid);
>> +	free_rmid(rgrp->closid, rgrp->mon.rmid);
>>  	rdtgroup_remove(rgrp);
>>  }
>>  
> 
> Related issue to above. Looking at how mkdir_rdt_prepare_clean() is called, right
> after closid is freed, this seems to be use-after-free?  Another motivation to
> re-order the patches?

It all washes out in the end, and nothing depends on this value until the MPAM support is
merged.

I'll take a look at how invasive it is to re-order the series.


Thanks,

James