The user triggers the creation of a pseudo-locked region when writing
the requested schemata to the schemata resctrl file. The pseudo-locking
of a region is required to be done on a CPU that is associated with the
cache on which the pseudo-locked region will reside. In order to run the
locking code on a specific CPU the needed CPU has to be selected and
ensured to remain online during the entire locking sequence.
At this time the cpu_hotplug_lock is not taken during the pseudo-lock
region creation and it is thus possible for a CPU to be selected to run
the pseudo-locking code and then that CPU to go offline before the
thread is able to run on it.
Fix this by ensuring that the cpu_hotplug_lock is taken while the CPU on
which code has to run needs to be controlled. Since the cpu_hotplug_lock
is always taken before rdtgroup_mutex the lock order is maintained.
Fixes: e0bdfe8e36f3 ("x86/intel_rdt: Support creation/removal of pseudo-locked region")
Signed-off-by: Reinette Chatre <[email protected]>
Cc: [email protected]
---
V2:
- Rebase against tip/x86/urgent
- Modify subject from x86/resctrl to x86/intel_rdt to match subject used
before the code reorganization.
arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
index 27937458c231..efa4a519f5e5 100644
--- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
@@ -23,6 +23,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/cpu.h>
#include <linux/kernfs.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
@@ -310,9 +311,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
return -EINVAL;
buf[nbytes - 1] = '\0';
+ cpus_read_lock();
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
rdtgroup_kn_unlock(of->kn);
+ cpus_read_unlock();
return -ENOENT;
}
rdt_last_cmd_clear();
@@ -367,6 +370,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
out:
rdtgroup_kn_unlock(of->kn);
+ cpus_read_unlock();
return ret ?: nbytes;
}
--
2.17.0
On Mon, Dec 10, 2018 at 01:21:54PM -0800, Reinette Chatre wrote:
> The user triggers the creation of a pseudo-locked region when writing
> the requested schemata to the schemata resctrl file. The pseudo-locking
> of a region is required to be done on a CPU that is associated with the
> cache on which the pseudo-locked region will reside. In order to run the
> locking code on a specific CPU the needed CPU has to be selected and
> ensured to remain online during the entire locking sequence.
>
> At this time the cpu_hotplug_lock is not taken during the pseudo-lock
> region creation and it is thus possible for a CPU to be selected to run
> the pseudo-locking code and then that CPU to go offline before the
> thread is able to run on it.
>
> Fix this by ensuring that the cpu_hotplug_lock is taken while the CPU on
> which code has to run needs to be controlled. Since the cpu_hotplug_lock
> is always taken before rdtgroup_mutex the lock order is maintained.
>
> Fixes: e0bdfe8e36f3 ("x86/intel_rdt: Support creation/removal of pseudo-locked region")
> Signed-off-by: Reinette Chatre <[email protected]>
> Cc: [email protected]
> ---
> V2:
> - Rebase against tip/x86/urgent
> - Modify subject from x86/resctrl to x86/intel_rdt to match subject used
> before the code reorganization.
>
> arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 4 ++++
> 1 file changed, 4 insertions(+)
I took it but changed the subject to the more straight-forward:
"x86/intel_rdt: Disable CPU hotplug while modifying schemata"
Now, your second patch:
Subject: [PATCH V2] x86/resctrl: Fix rdt_find_domain() return value and checks
Message-Id: <b88cd4ff6a75995bf8db9b0ea546908fe50f69f3.1544479852.git.reinette.chatre@intel.com>
has the new file paths, has Fixes: tags but no CC:stable.
I'm guessing it needs to go in the next merge window with the rest of
the new stuff and not now, with the urgent pile?
I'm thinking that because it is not really a bug now, as the negative ID
happens to work.
Right?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Boris,
On 12/11/2018 4:34 AM, Borislav Petkov wrote:
> On Mon, Dec 10, 2018 at 01:21:54PM -0800, Reinette Chatre wrote:
>> The user triggers the creation of a pseudo-locked region when writing
>> the requested schemata to the schemata resctrl file. The pseudo-locking
>> of a region is required to be done on a CPU that is associated with the
>> cache on which the pseudo-locked region will reside. In order to run the
>> locking code on a specific CPU the needed CPU has to be selected and
>> ensured to remain online during the entire locking sequence.
>>
>> At this time the cpu_hotplug_lock is not taken during the pseudo-lock
>> region creation and it is thus possible for a CPU to be selected to run
>> the pseudo-locking code and then that CPU to go offline before the
>> thread is able to run on it.
>>
>> Fix this by ensuring that the cpu_hotplug_lock is taken while the CPU on
>> which code has to run needs to be controlled. Since the cpu_hotplug_lock
>> is always taken before rdtgroup_mutex the lock order is maintained.
>>
>> Fixes: e0bdfe8e36f3 ("x86/intel_rdt: Support creation/removal of pseudo-locked region")
>> Signed-off-by: Reinette Chatre <[email protected]>
>> Cc: [email protected]
>> ---
>> V2:
>> - Rebase against tip/x86/urgent
>> - Modify subject from x86/resctrl to x86/intel_rdt to match subject used
>> before the code reorganization.
>>
>> arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>
> I took it but changed the subject to the more straight-forward:
Thank you very much.
>
> "x86/intel_rdt: Disable CPU hotplug while modifying schemata"
I am not sure that this is an issue when updating a schemata in the
general case. In the case when just CAT schemata (without
pseudo-locking) is updated then the cpu mask associated with the cache
instance is indeed used to determine which CPUs should have their
registers changed but only the current CPU is not checked for being
online, for the other CPUs smp_call_function_many() is used that
includes an online check.
> Now, your second patch:
>
> Subject: [PATCH V2] x86/resctrl: Fix rdt_find_domain() return value and checks
> Message-Id: <b88cd4ff6a75995bf8db9b0ea546908fe50f69f3.1544479852.git.reinette.chatre@intel.com>
>
> has the new file paths, has Fixes: tags but no CC:stable.
>
> I'm guessing it needs to go in the next merge window with the rest of
> the new stuff and not now, with the urgent pile?
>
> I'm thinking that because it is not really a bug now, as the negative ID
> happens to work.
I had the same question in V1's notes to the maintainer :)
My initial concern was the lack of IS_ERR checking. Understanding the
flow better now it seems to me that this is indeed not a bug now. The
reasoning is that an ERR_PTR is only returned when a negative id is
provided in the parameters to rdt_find_domain(). There are currently
only two places where a negative id could be provided to
rdt_find_domain(), domain_add_cpu() and domain_remove_cpu(), and both
locations test the return value using IS_ERR.
Reinette
On Tue, Dec 11, 2018 at 10:33:59AM -0800, Reinette Chatre wrote:
> I am not sure that this is an issue when updating a schemata in the
> general case. In the case when just CAT schemata (without
> pseudo-locking) is updated then the cpu mask associated with the cache
> instance is indeed used to determine which CPUs should have their
> registers changed but only the current CPU is not checked for being
> online, for the other CPUs smp_call_function_many() is used that
> includes an online check.
Well, in your fix rdtgroup_schemata_write() disables hotplug for its
whole duration and doesn't look at what schemata update is being done,
right?
> I had the same question in V1's notes to the maintainer :)
Whoops, and I read that... Sorry. :-\
> My initial concern was the lack of IS_ERR checking. Understanding the
> flow better now it seems to me that this is indeed not a bug now. The
> reasoning is that an ERR_PTR is only returned when a negative id is
> provided in the parameters to rdt_find_domain(). There are currently
> only two places where a negative id could be provided to
> rdt_find_domain(), domain_add_cpu() and domain_remove_cpu(), and both
> locations test the return value using IS_ERR.
Right. I'll queue it for the normal merge window.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Boris,
On 12/11/2018 10:50 AM, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 10:33:59AM -0800, Reinette Chatre wrote:
>> I am not sure that this is an issue when updating a schemata in the
>> general case. In the case when just CAT schemata (without
>> pseudo-locking) is updated then the cpu mask associated with the cache
>> instance is indeed used to determine which CPUs should have their
>> registers changed but only the current CPU is not checked for being
>> online, for the other CPUs smp_call_function_many() is used that
>> includes an online check.
>
> Well, in your fix rdtgroup_schemata_write() disables hotplug for its
> whole duration and doesn't look at what schemata update is being done,
> right?
Correct.
I just wanted to emphasize that it is not the schemata writing that
needs to be protected, but instead the pseudo-locking code that runs
after schemata programming that needs to run on a particular CPU. The
new patch subject could be interpreted to mean the former ... but that
is starting to sound like nitpicking by me.
>> I had the same question in V1's notes to the maintainer :)
>
> Whoops, and I read that... Sorry. :-\
Not a problem at all :)
>> My initial concern was the lack of IS_ERR checking. Understanding the
>> flow better now it seems to me that this is indeed not a bug now. The
>> reasoning is that an ERR_PTR is only returned when a negative id is
>> provided in the parameters to rdt_find_domain(). There are currently
>> only two places where a negative id could be provided to
>> rdt_find_domain(), domain_add_cpu() and domain_remove_cpu(), and both
>> locations test the return value using IS_ERR.
>
> Right. I'll queue it for the normal merge window.
Thank you very much.
Reinette
On Tue, Dec 11, 2018 at 11:02:08AM -0800, Reinette Chatre wrote:
> I just wanted to emphasize that it is not the schemata writing that
> needs to be protected, but instead the pseudo-locking code that runs
> after schemata programming that needs to run on a particular CPU. The
> new patch subject could be interpreted to mean the former ... but that
> is starting to sound like nitpicking by me.
Nah, that's not nitpicking - it is important that we sort out stuff
fully before committing.
Now, I'm trying to understand what you're telling me and I believe you
mean what update_domains() does, yes?
And I guess a more fitting subject in that case could be:
x86/intel_rdt: Ensure a CPU remains online for the region's pseudo-locking sequence
or so, and then the commit message explains in more detail what that
title actually means. :)
Hmmm.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Hi Boris,
On 12/11/2018 12:43 PM, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 11:02:08AM -0800, Reinette Chatre wrote:
>> I just wanted to emphasize that it is not the schemata writing that
>> needs to be protected, but instead the pseudo-locking code that runs
>> after schemata programming that needs to run on a particular CPU. The
>> new patch subject could be interpreted to mean the former ... but that
>> is starting to sound like nitpicking by me.
>
> Nah, that's not nitpicking - it is important that we sort out stuff
> fully before committing.
I really appreciate your patience.
>
> Now, I'm trying to understand what you're telling me and I believe you
> mean what update_domains() does, yes?
Yes.
>
> And I guess a more fitting subject in that case could be:
>
> x86/intel_rdt: Ensure a CPU remains online for the region's pseudo-locking sequence
>
> or so, and then the commit message explains in more detail what that
> title actually means. :)
>
> Hmmm.
That subject line matches exactly the goal of this patch. Thank you very
much. Would you prefer that I send a new version with this subject line
or would you rather make the change yourself while applying this patch?
Reinette
On Tue, Dec 11, 2018 at 12:51:12PM -0800, Reinette Chatre wrote:
> I really appreciate your patience.
That goes both ways. Thx. :-)
> That subject line matches exactly the goal of this patch. Thank you very
> much. Would you prefer that I send a new version with this subject line
> or would you rather make the change yourself while applying this patch?
Nah, no need. I'll fix them up.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Commit-ID: 80b71c340f17705ec145911b9a193ea781811b16
Gitweb: https://git.kernel.org/tip/80b71c340f17705ec145911b9a193ea781811b16
Author: Reinette Chatre <[email protected]>
AuthorDate: Mon, 10 Dec 2018 13:21:54 -0800
Committer: Borislav Petkov <[email protected]>
CommitDate: Tue, 11 Dec 2018 21:59:01 +0100
x86/intel_rdt: Ensure a CPU remains online for the region's pseudo-locking sequence
The user triggers the creation of a pseudo-locked region when writing
the requested schemata to the schemata resctrl file. The pseudo-locking
of a region is required to be done on a CPU that is associated with the
cache on which the pseudo-locked region will reside. In order to run the
locking code on a specific CPU, the needed CPU has to be selected and
ensured to remain online during the entire locking sequence.
At this time, the cpu_hotplug_lock is not taken during the pseudo-lock
region creation and it is thus possible for a CPU to be selected to run
the pseudo-locking code and then that CPU to go offline before the
thread is able to run on it.
Fix this by ensuring that the cpu_hotplug_lock is taken while the CPU on
which code has to run needs to be controlled. Since the cpu_hotplug_lock
is always taken before rdtgroup_mutex the lock order is maintained.
Fixes: e0bdfe8e36f3 ("x86/intel_rdt: Support creation/removal of pseudo-locked region")
Signed-off-by: Reinette Chatre <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: stable <[email protected]>
Cc: x86-ml <[email protected]>
Link: https://lkml.kernel.org/r/b7b17432a80f95a1fa21a1698ba643014f58ad31.1544476425.git.reinette.chatre@intel.com
---
arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
index 27937458c231..efa4a519f5e5 100644
--- a/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_ctrlmondata.c
@@ -23,6 +23,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/cpu.h>
#include <linux/kernfs.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
@@ -310,9 +311,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
return -EINVAL;
buf[nbytes - 1] = '\0';
+ cpus_read_lock();
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
rdtgroup_kn_unlock(of->kn);
+ cpus_read_unlock();
return -ENOENT;
}
rdt_last_cmd_clear();
@@ -367,6 +370,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
out:
rdtgroup_kn_unlock(of->kn);
+ cpus_read_unlock();
return ret ?: nbytes;
}