| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d49bdfad-df5e-77e1-4834-266dcd1b9055@arm.com>
Date: Wed, 30 Mar 2022 17:43:45 +0100
From: James Morse <james.morse@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, x86@...nel.org,
linux-kernel@...r.kernel.org
Cc: Fenghua Yu <fenghua.yu@...el.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
H Peter Anvin <hpa@...or.com>,
Babu Moger <Babu.Moger@....com>,
shameerali.kolothum.thodi@...wei.com,
Jamie Iles <jamie@...iainc.com>,
D Scott Phillips OS <scott@...amperecomputing.com>,
lcherian@...vell.com, bobo.shaobowang@...wei.com,
tan.shaopeng@...itsu.com
Subject: Re: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the
rdt_domain
Hi Reinette,
On 16/03/2022 21:50, Reinette Chatre wrote:
> I tried out this work and encountered a null pointer de-reference that
> seems related to this patch. After digging into that it is not
> clear to me how this is expected to work.
>
> I encounter the issue just by attempting to mount with "-o mba_MBps" which is
> the way to enable the mba_sc and exactly what this patch aims to address.
>
> More below ...
>
> On 2/17/2022 10:20 AM, James Morse wrote:
>> To support resctrl's MBA software controller, the architecture must provide
>> a second configuration array to hold the mbps_val[] from user-space.
>>
>> This complicates the interface between the architecture specific code and
>> the filesystem portions of resctrl that will move to /fs/, to allow
>> multiple architectures to support resctrl.
>>
>> Make the filesystem parts of resctrl create an array for the mba_sc
>> values when is_mba_sc() is set to true. The software controller
>> can be changed to use this, allowing the architecture code to only
>> consider the values configured in hardware.
...
>> @@ -3309,6 +3344,12 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>> if (err)
>> return err;
>>
>> + err = mba_sc_domain_allocate(r, d);
>> + if (err) {
>> + domain_destroy_mon_state(d);
>> + return err;
>> + }
>> +
>
> Before the above snippet there is a check if the resource is capable of monitoring:
>
> resctrl_online_domain()
> {
> ...
> if (!r->mon_capable)
> return 0;
>
> ...
> err = mba_sc_domain_allocate(r, d);
> ...
> }
>
> Thus, the rdt_domain->mbps_val array will only exist in those resources that
> support monitoring.
>
> Taking a look at where mon_capable is set we see it is done in
> get_rdt_mon_resources() and as you can see it is only done for RDT_RESOURCE_L3.
>
> get_rdt_mon_resources()
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>
> ...
>
> return !rdt_get_mon_l3_config(r); /* mon_capable is set within */
> }
>
> Based on the above the rdt_domain->mbps_val array can only exist for those
> domains that belong to resource RDT_RESOURCE_L3 (if it is capable of monitoring).
>
> Now, looking at set_mba_sc() changed here, it only interacts with RDT_RESOURCE_MBA:
>
> set_mba_sc()
> {
> struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>
> ...
>
> list_for_each_entry(d, &r->domains, list) {
> for (i = 0; i < num_closid; i++)
> d->mbps_val[i] = MBA_MAX_MBPS;
> }
> }
>
> Considering that no domain belonging to RDT_RESOURCE_MBA will have this array this
> always ends up being a null pointer de-reference.
Ugh. I'm not sure how I managed to miss that. Thanks for debugging it!
That loop was added to reset the array when the filesystem is mounted, as it may hold
stale values from a previous mount of the filesystem. Its currently done by
reset_all_ctrls(), but that function should really belong to the architecture code.
Because mbm_handle_overflow() always passes a domain from the L3 to update_mba_bw(), I
think the cleanest thing to do is move the reset to a helper that always operates on the
L3 array. (and leave some breadcrumbs in the comments).
Thanks!
James
-----------------%<-----------------
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 36edae7dbc6a..3b52f079a5b3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1913,6 +1913,23 @@ static void mba_sc_domain_destroy(struct rdt_resource *r,
d->mbps_val = NULL;
}
+static void mba_sc_reset(void)
+{
+ /*
+ * mbm_handle_overflow() only passes domains of the L3 resource to
+ * update_mba_bw(), so mba_sc only supports monitoring on the L3.
+ */
+ struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+ u32 num_closid = resctrl_arch_get_num_closid(r);
+ struct rdt_domain *d;
+ int i;
+
+ list_for_each_entry(d, &r->domains, list) {
+ for (i = 0; i < num_closid; i++)
+ d->mbps_val[i] = MBA_MAX_MBPS;
+ }
+}
+
/*
* Enable or disable the MBA software controller
* which helps user specify bandwidth in MBps.
@@ -1922,20 +1939,13 @@ static void mba_sc_domain_destroy(struct rdt_resource *r,
static int set_mba_sc(bool mba_sc)
{
struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
- u32 num_closid = resctrl_arch_get_num_closid(r);
- struct rdt_domain *d;
- int i;
if (!is_mbm_enabled() || !is_mba_linear() ||
mba_sc == is_mba_sc(r))
return -EINVAL;
r->membw.mba_sc = mba_sc;
-
- list_for_each_entry(d, &r->domains, list) {
- for (i = 0; i < num_closid; i++)
- d->mbps_val[i] = MBA_MAX_MBPS;
- }
+ mba_sc_reset();
return 0;
}
-----------------%<-----------------
Powered by blists - more mailing lists