This series includes some minor fixes to RDT parsing of the schemata.
Patches are based on tip x86/cpu/
[PATCH 1/3] x86/intel_rdt: Fix padding when resource is enabled via
mount
[PATCH 2/3] x86/intel_rdt: Trim whitespace while parsing schemata
[PATCH 3/3] x86/intel_rdt: Return error for incorrect resource names
When schemata parses the resource names, currently it may not return
error for incorrect resource names and fail quietly without updating the
control values.
This is because for_each_enabled_rdt_resource(r) leaves "r" pointing
beyond the end of the rdt_resources_all[] array, and we check for
!r->name which results in an out of bounds access and also may not be
NULL, hence we may not detect that we did not find the name user
supplied.
Update this to check (r == (rdt_resources_all + RDT_NUM_RESOURCES))
instead.
Reported-by: Prakhya, Sai Praneeth <[email protected]>
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt_schemata.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 3cfa1ca..bb99bd8 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -228,7 +228,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
break;
}
}
- if (!r->name) {
+ if (r == (rdt_resources_all + RDT_NUM_RESOURCES)) {
ret = -EINVAL;
goto out;
}
--
1.9.1
Schemata is displayed in tabular format which introduces some whitespace
to show data in a tabular format. If user wants to input the same data
that is displayed, the parsing fails. Trim the leading and trailing
whitespace to help parse such data.
Reported-by: Prakhya, Sai Praneeth <[email protected]>
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt_schemata.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 9467a00..3cfa1ca 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -143,7 +143,7 @@ static int parse_line(char *line, struct rdt_resource *r)
return -EINVAL;
list_for_each_entry(d, &r->domains, list) {
if (d->id == dom_id) {
- if (r->parse_ctrlval(dom, r, d))
+ if (r->parse_ctrlval(strim(dom), r, d))
return -EINVAL;
goto next;
}
@@ -220,7 +220,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
goto out;
}
for_each_enabled_rdt_resource(r) {
- if (!strcmp(resname, r->name) &&
+ if (!strcmp(strim(resname), r->name) &&
closid < r->num_closid) {
ret = parse_line(tok, r);
if (ret)
--
1.9.1
Currently max width of 'resource name' and 'resource data' is being
initialized based on 'enabled resources' during boot. But the mount can
enable different capable resources at a later time which upsets the
tabular format of schemata. Fix this to be based on 'all capable'
resources.
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 731f70a..5b36646 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -492,7 +492,7 @@ static __init void rdt_init_padding(void)
struct rdt_resource *r;
int cl;
- for_each_enabled_rdt_resource(r) {
+ for_each_capable_rdt_resource(r) {
cl = strlen(r->name);
if (cl > max_name_width)
max_name_width = cl;
--
1.9.1
On Wed, 19 Apr 2017, Vikas Shivappa wrote:
> When schemata parses the resource names, currently it may not return
> error for incorrect resource names and fail quietly without updating the
> control values.
>
> This is because for_each_enabled_rdt_resource(r) leaves "r" pointing
> beyond the end of the rdt_resources_all[] array, and we check for
> !r->name which results in an out of bounds access and also may not be
> NULL, hence we may not detect that we did not find the name user
> supplied.
>
> Update this to check (r == (rdt_resources_all + RDT_NUM_RESOURCES))
> instead.
>
> Reported-by: Prakhya, Sai Praneeth <[email protected]>
> Signed-off-by: Vikas Shivappa <[email protected]>
> Tested-by: Prakhya, Sai Praneeth <[email protected]>
> ---
> arch/x86/kernel/cpu/intel_rdt_schemata.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> index 3cfa1ca..bb99bd8 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -228,7 +228,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> break;
> }
> }
> - if (!r->name) {
> + if (r == (rdt_resources_all + RDT_NUM_RESOURCES)) {
> ret = -EINVAL;
> goto out;
> }
The resulting loop is just horrible to read. We can do better than
that. Patch below.
Thanks,
tglx
8<-------------
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -187,6 +187,17 @@ static int update_domains(struct rdt_res
return 0;
}
+static int rdtgroup_parse_resource(char *resname, char *tok, int closid)
+{
+ struct rdt_resource *r;
+
+ for_each_enabled_rdt_resource(r) {
+ if (!strcmp(resname, r->name) && closid < r->num_closid)
+ return parse_line(tok, r);
+ }
+ return -EINVAL;
+}
+
ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off)
{
@@ -209,9 +220,10 @@ ssize_t rdtgroup_schemata_write(struct k
closid = rdtgrp->closid;
- for_each_enabled_rdt_resource(r)
+ for_each_enabled_rdt_resource(r) {
list_for_each_entry(dom, &r->domains, list)
dom->have_new_ctrl = false;
+ }
while ((tok = strsep(&buf, "\n")) != NULL) {
resname = strsep(&tok, ":");
@@ -219,19 +231,9 @@ ssize_t rdtgroup_schemata_write(struct k
ret = -EINVAL;
goto out;
}
- for_each_enabled_rdt_resource(r) {
- if (!strcmp(strim(resname), r->name) &&
- closid < r->num_closid) {
- ret = parse_line(tok, r);
- if (ret)
- goto out;
- break;
- }
- }
- if (!r->name) {
- ret = -EINVAL;
+ ret = rdtgroup_parse_resource(strim(resname), tok, closid);
+ if (ret)
goto out;
- }
}
for_each_enabled_rdt_resource(r) {
On Wed, 19 Apr 2017, Vikas Shivappa wrote:
> Schemata is displayed in tabular format which introduces some whitespace
> to show data in a tabular format. If user wants to input the same data
> that is displayed, the parsing fails. Trim the leading and trailing
> whitespace to help parse such data.
>
> Reported-by: Prakhya, Sai Praneeth <[email protected]>
> Signed-off-by: Vikas Shivappa <[email protected]>
> Tested-by: Prakhya, Sai Praneeth <[email protected]>
> ---
> arch/x86/kernel/cpu/intel_rdt_schemata.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> index 9467a00..3cfa1ca 100644
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -143,7 +143,7 @@ static int parse_line(char *line, struct rdt_resource *r)
> return -EINVAL;
> list_for_each_entry(d, &r->domains, list) {
> if (d->id == dom_id) {
> - if (r->parse_ctrlval(dom, r, d))
> + if (r->parse_ctrlval(strim(dom), r, d))
> return -EINVAL;
> goto next;
> }
> @@ -220,7 +220,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> goto out;
> }
> for_each_enabled_rdt_resource(r) {
> - if (!strcmp(resname, r->name) &&
> + if (!strcmp(strim(resname), r->name) &&
Both strims() invocations are just silly. They can be done before the loop
once instead of doing them over and over again.
Thanks,
tglx
Commit-ID: adcbdd70309dba5a12a9d8158deb6a62a6d5fc98
Gitweb: http://git.kernel.org/tip/adcbdd70309dba5a12a9d8158deb6a62a6d5fc98
Author: Vikas Shivappa <[email protected]>
AuthorDate: Wed, 19 Apr 2017 16:50:02 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 20 Apr 2017 15:57:59 +0200
x86/intel_rdt: Fix padding when resource is enabled via mount
Currently max width of 'resource name' and 'resource data' is being
initialized based on 'enabled resources' during boot. But the mount can
enable different capable resources at a later time which upsets the
tabular format of schemata. Fix this to be based on 'all capable'
resources.
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
index 731f70a..5b36646 100644
--- a/arch/x86/kernel/cpu/intel_rdt.c
+++ b/arch/x86/kernel/cpu/intel_rdt.c
@@ -492,7 +492,7 @@ static __init void rdt_init_padding(void)
struct rdt_resource *r;
int cl;
- for_each_enabled_rdt_resource(r) {
+ for_each_capable_rdt_resource(r) {
cl = strlen(r->name);
if (cl > max_name_width)
max_name_width = cl;
Commit-ID: 634b0e0491d6f6e882b922eb41c278d01a743bab
Gitweb: http://git.kernel.org/tip/634b0e0491d6f6e882b922eb41c278d01a743bab
Author: Vikas Shivappa <[email protected]>
AuthorDate: Wed, 19 Apr 2017 16:50:03 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 20 Apr 2017 15:57:59 +0200
x86/intel_rdt: Trim whitespace while parsing schemata input
Schemata is displayed in tabular format which introduces some whitespace
to show data in a tabular format.
Writing back the same data fails as the parser does not handle the
whitespace.
Trim the leading and trailing whitespace before parsing.
Reported-by: Prakhya, Sai Praneeth <[email protected]>
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt_schemata.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index 9467a00..e64b2cf 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -141,6 +141,7 @@ next:
id = strsep(&dom, "=");
if (!dom || kstrtoul(id, 10, &dom_id))
return -EINVAL;
+ dom = strim(dom);
list_for_each_entry(d, &r->domains, list) {
if (d->id == dom_id) {
if (r->parse_ctrlval(dom, r, d))
@@ -214,7 +215,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
dom->have_new_ctrl = false;
while ((tok = strsep(&buf, "\n")) != NULL) {
- resname = strsep(&tok, ":");
+ resname = strim(strsep(&tok, ":"));
if (!tok) {
ret = -EINVAL;
goto out;
Commit-ID: 4797b7dfdfcf457075c36743d71e2b0feeaaa20f
Gitweb: http://git.kernel.org/tip/4797b7dfdfcf457075c36743d71e2b0feeaaa20f
Author: Vikas Shivappa <[email protected]>
AuthorDate: Wed, 19 Apr 2017 16:50:04 -0700
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 20 Apr 2017 15:57:59 +0200
x86/intel_rdt: Return error for incorrect resource names in schemata
When schemata parses the resource names it does not return an error if it
detects incorrect resource names and fails quietly.
This happens because for_each_enabled_rdt_resource(r) leaves "r" pointing
beyond the end of the rdt_resources_all[] array, and the check for !r->name
results in an out of bounds access.
Split the resource parsing part into a helper function to avoid the issue.
[ tglx: Made it readable by splitting the parser loop out into a function ]
Reported-by: Prakhya, Sai Praneeth <[email protected]>
Signed-off-by: Vikas Shivappa <[email protected]>
Tested-by: Prakhya, Sai Praneeth <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/cpu/intel_rdt_schemata.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
index e64b2cf..406d7a6 100644
--- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
+++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
@@ -188,6 +188,17 @@ done:
return 0;
}
+static int rdtgroup_parse_resource(char *resname, char *tok, int closid)
+{
+ struct rdt_resource *r;
+
+ for_each_enabled_rdt_resource(r) {
+ if (!strcmp(resname, r->name) && closid < r->num_closid)
+ return parse_line(tok, r);
+ }
+ return -EINVAL;
+}
+
ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off)
{
@@ -210,9 +221,10 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
closid = rdtgrp->closid;
- for_each_enabled_rdt_resource(r)
+ for_each_enabled_rdt_resource(r) {
list_for_each_entry(dom, &r->domains, list)
dom->have_new_ctrl = false;
+ }
while ((tok = strsep(&buf, "\n")) != NULL) {
resname = strim(strsep(&tok, ":"));
@@ -220,19 +232,9 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
ret = -EINVAL;
goto out;
}
- for_each_enabled_rdt_resource(r) {
- if (!strcmp(resname, r->name) &&
- closid < r->num_closid) {
- ret = parse_line(tok, r);
- if (ret)
- goto out;
- break;
- }
- }
- if (!r->name) {
- ret = -EINVAL;
+ ret = rdtgroup_parse_resource(resname, tok, closid);
+ if (ret)
goto out;
- }
}
for_each_enabled_rdt_resource(r) {
On Thu, 20 Apr 2017, Thomas Gleixner wrote:
> On Wed, 19 Apr 2017, Vikas Shivappa wrote:
>
>> Schemata is displayed in tabular format which introduces some whitespace
>> to show data in a tabular format. If user wants to input the same data
>> that is displayed, the parsing fails. Trim the leading and trailing
>> whitespace to help parse such data.
>>
>> Reported-by: Prakhya, Sai Praneeth <[email protected]>
>> Signed-off-by: Vikas Shivappa <[email protected]>
>> Tested-by: Prakhya, Sai Praneeth <[email protected]>
>> ---
>> arch/x86/kernel/cpu/intel_rdt_schemata.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel_rdt_schemata.c b/arch/x86/kernel/cpu/intel_rdt_schemata.c
>> index 9467a00..3cfa1ca 100644
>> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
>> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
>> @@ -143,7 +143,7 @@ static int parse_line(char *line, struct rdt_resource *r)
>> return -EINVAL;
>> list_for_each_entry(d, &r->domains, list) {
>> if (d->id == dom_id) {
>> - if (r->parse_ctrlval(dom, r, d))
>> + if (r->parse_ctrlval(strim(dom), r, d))
>> return -EINVAL;
>> goto next;
>> }
>> @@ -220,7 +220,7 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>> goto out;
>> }
>> for_each_enabled_rdt_resource(r) {
>> - if (!strcmp(resname, r->name) &&
>> + if (!strcmp(strim(resname), r->name) &&
>
> Both strims() invocations are just silly. They can be done before the loop
> once instead of doing them over and over again.
Will fix. I went overboard not wanting to add a line
Thanks,
Vikas
>
> Thanks,
>
> tglx
>
On Thu, 20 Apr 2017, Shivappa Vikas wrote:
> Will fix. I went overboard not wanting to add a line
Finish reading the thread before you start :)
On Thu, 20 Apr 2017, Thomas Gleixner wrote:
> On Wed, 19 Apr 2017, Vikas Shivappa wrote:
>
>> }
>
> The resulting loop is just horrible to read. We can do better than
> that. Patch below.
>
> Thanks,
>
> tglx
> 8<-------------
>
> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c
> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c
> @@ -187,6 +187,17 @@ static int update_domains(struct rdt_res
> return 0;
> }
>
> +static int rdtgroup_parse_resource(char *resname, char *tok, int closid)
> +{
> + struct rdt_resource *r;
> +
> + for_each_enabled_rdt_resource(r) {
> + if (!strcmp(resname, r->name) && closid < r->num_closid)
> + return parse_line(tok, r);
> + }
> + return -EINVAL;
> +}
> +
> ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
> char *buf, size_t nbytes, loff_t off)
> {
> @@ -209,9 +220,10 @@ ssize_t rdtgroup_schemata_write(struct k
>
> closid = rdtgrp->closid;
>
> - for_each_enabled_rdt_resource(r)
> + for_each_enabled_rdt_resource(r) {
> list_for_each_entry(dom, &r->domains, list)
> dom->have_new_ctrl = false;
> + }
>
> while ((tok = strsep(&buf, "\n")) != NULL) {
> resname = strsep(&tok, ":");
> @@ -219,19 +231,9 @@ ssize_t rdtgroup_schemata_write(struct k
> ret = -EINVAL;
> goto out;
> }
> - for_each_enabled_rdt_resource(r) {
> - if (!strcmp(strim(resname), r->name) &&
> - closid < r->num_closid) {
> - ret = parse_line(tok, r);
> - if (ret)
> - goto out;
> - break;
> - }
> - }
> - if (!r->name) {
> - ret = -EINVAL;
> + ret = rdtgroup_parse_resource(strim(resname), tok, closid);
> + if (ret)
> goto out;
> - }
> }
Ok agree its better way. I was trying to add a NULL which Tony already said was
Nacked, did not really want to use a pointer which may be out of bounds
although we dont check the contents.
Thanks,
Vikas
On Thu, 20 Apr 2017, Thomas Gleixner wrote:
> On Thu, 20 Apr 2017, Shivappa Vikas wrote:
>> Will fix. I went overboard not wanting to add a line
>
> Finish reading the thread before you start :)
Got it :) Had not seen the tip bot emails on my other email..
>