The user input to files in the resctrl filesystem are expected to be
terminated with a newline. Testing the user input includes a test for
the presence of a newline and then replacing the newline with NUL
byte followed by comparison using strcmp().
sysfs_streq() exists to test if strings are equal, treating both NUL and
newline-then-NUL as equivalent string terminations. Even more,
sysfs_match_string() exists to match a given string in an array using
sysfs_streq().
Replace existing strcmp() comparisons of strings that are terminated
with a newline with more appropriate sysfs_streq() via the
sysfs_match_string() API that can perform the match across the different
mode strings that are already maintained in an array.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 088a1536bccc..0e302d3c44e6 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1415,12 +1415,11 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
{
struct rdtgroup *rdtgrp;
enum rdtgrp_mode mode;
- int ret = 0;
+ int user_m;
+ int ret;
- /* Valid input requires a trailing newline */
- if (nbytes == 0 || buf[nbytes - 1] != '\n')
+ if (nbytes == 0)
return -EINVAL;
- buf[nbytes - 1] = '\0';
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
@@ -1432,11 +1431,17 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
mode = rdtgrp->mode;
- if ((!strcmp(buf, "shareable") && mode == RDT_MODE_SHAREABLE) ||
- (!strcmp(buf, "exclusive") && mode == RDT_MODE_EXCLUSIVE) ||
- (!strcmp(buf, "pseudo-locksetup") &&
- mode == RDT_MODE_PSEUDO_LOCKSETUP) ||
- (!strcmp(buf, "pseudo-locked") && mode == RDT_MODE_PSEUDO_LOCKED))
+ ret = sysfs_match_string(rdt_mode_str, buf);
+ if (ret < 0) {
+ rdt_last_cmd_puts("Unknown or unsupported mode\n");
+ goto out;
+ }
+
+ user_m = ret;
+ ret = 0;
+
+ /* Do nothing and return success if user asks for current mode */
+ if (user_m == mode)
goto out;
if (mode == RDT_MODE_PSEUDO_LOCKED) {
@@ -1445,14 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
goto out;
}
- if (!strcmp(buf, "shareable")) {
+ if (user_m == RDT_MODE_SHAREABLE) {
if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
ret = rdtgroup_locksetup_exit(rdtgrp);
if (ret)
goto out;
}
rdtgrp->mode = RDT_MODE_SHAREABLE;
- } else if (!strcmp(buf, "exclusive")) {
+ } else if (user_m == RDT_MODE_EXCLUSIVE) {
if (!rdtgroup_mode_test_exclusive(rdtgrp)) {
ret = -EINVAL;
goto out;
@@ -1463,14 +1468,11 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
goto out;
}
rdtgrp->mode = RDT_MODE_EXCLUSIVE;
- } else if (!strcmp(buf, "pseudo-locksetup")) {
+ } else if (user_m == RDT_MODE_PSEUDO_LOCKSETUP) {
ret = rdtgroup_locksetup_enter(rdtgrp);
if (ret)
goto out;
rdtgrp->mode = RDT_MODE_PSEUDO_LOCKSETUP;
- } else {
- rdt_last_cmd_puts("Unknown or unsupported mode\n");
- ret = -EINVAL;
}
out:
--
2.21.0
On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
> The user input to files in the resctrl filesystem are expected to be
> terminated with a newline. Testing the user input includes a test for
> the presence of a newline and then replacing the newline with NUL
> byte followed by comparison using strcmp().
>
> sysfs_streq() exists to test if strings are equal, treating both NUL and
> newline-then-NUL as equivalent string terminations. Even more,
> sysfs_match_string() exists to match a given string in an array using
> sysfs_streq().
>
> Replace existing strcmp() comparisons of strings that are terminated
> with a newline with more appropriate sysfs_streq() via the
> sysfs_match_string() API that can perform the match across the different
> mode strings that are already maintained in an array.
Sorry for late comment, but just have noticed...
> if (mode == RDT_MODE_PSEUDO_LOCKED) {
> @@ -1445,14 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
> goto out;
> }
>
> - if (!strcmp(buf, "shareable")) {
> + if (user_m == RDT_MODE_SHAREABLE) {
> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> ret = rdtgroup_locksetup_exit(rdtgrp);
> if (ret)
> goto out;
> }
> rdtgrp->mode = RDT_MODE_SHAREABLE;
...can we simple move this and similar (in other branches) to...
> - } else {
> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
> - ret = -EINVAL;
> }
...here as
rdtgrp->mode = user_m;
?
> out:
Note, I didn't check all branches carefully.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 5/18/2020 4:50 AM, Andy Shevchenko wrote:
> On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
>> The user input to files in the resctrl filesystem are expected to be
>> terminated with a newline. Testing the user input includes a test for
>> the presence of a newline and then replacing the newline with NUL
>> byte followed by comparison using strcmp().
>>
>> sysfs_streq() exists to test if strings are equal, treating both NUL and
>> newline-then-NUL as equivalent string terminations. Even more,
>> sysfs_match_string() exists to match a given string in an array using
>> sysfs_streq().
>>
>> Replace existing strcmp() comparisons of strings that are terminated
>> with a newline with more appropriate sysfs_streq() via the
>> sysfs_match_string() API that can perform the match across the different
>> mode strings that are already maintained in an array.
>
> Sorry for late comment, but just have noticed...
No problem. I do appreciate your feedback because it helps me to improve
the code.
>
>> if (mode == RDT_MODE_PSEUDO_LOCKED) {
>> @@ -1445,14 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
>> goto out;
>> }
>>
>> - if (!strcmp(buf, "shareable")) {
>> + if (user_m == RDT_MODE_SHAREABLE) {
>> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>> ret = rdtgroup_locksetup_exit(rdtgrp);
>> if (ret)
>> goto out;
>> }
>
>> rdtgrp->mode = RDT_MODE_SHAREABLE;
>
> ...can we simple move this and similar (in other branches) to...
>
>
>> - } else {
>> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> - ret = -EINVAL;
>> }
>
> ...here as
>
> rdtgrp->mode = user_m;
Will do.
It also looks like the only reason for the "mode" local variable was to
make those earlier "strcmp" lines shorter. With those long lines removed
in this patch this local variable is no longer needed and I will remove
it also.
Reinette
Hi Andy,
On 5/18/2020 9:06 AM, Reinette Chatre wrote:
> On 5/18/2020 4:50 AM, Andy Shevchenko wrote:
>> On Sat, May 16, 2020 at 11:28:41AM -0700, Reinette Chatre wrote:
>>> The user input to files in the resctrl filesystem are expected to be
>>> terminated with a newline. Testing the user input includes a test for
>>> the presence of a newline and then replacing the newline with NUL
>>> byte followed by comparison using strcmp().
>>>
>>> sysfs_streq() exists to test if strings are equal, treating both NUL and
>>> newline-then-NUL as equivalent string terminations. Even more,
>>> sysfs_match_string() exists to match a given string in an array using
>>> sysfs_streq().
>>>
>>> Replace existing strcmp() comparisons of strings that are terminated
>>> with a newline with more appropriate sysfs_streq() via the
>>> sysfs_match_string() API that can perform the match across the different
>>> mode strings that are already maintained in an array.
>>
>> Sorry for late comment, but just have noticed...
>
> No problem. I do appreciate your feedback because it helps me to improve
> the code.
>
>>
>>> if (mode == RDT_MODE_PSEUDO_LOCKED) {
>>> @@ -1445,14 +1450,14 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
>>> goto out;
>>> }
>>>
>>> - if (!strcmp(buf, "shareable")) {
>>> + if (user_m == RDT_MODE_SHAREABLE) {
>>> if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>> ret = rdtgroup_locksetup_exit(rdtgrp);
>>> if (ret)
>>> goto out;
>>> }
>>
>>> rdtgrp->mode = RDT_MODE_SHAREABLE;
>>
>> ...can we simple move this and similar (in other branches) to...
>>
>>
>>> - } else {
>>> - rdt_last_cmd_puts("Unknown or unsupported mode\n");
>>> - ret = -EINVAL;
>>> }
>>
>> ...here as
>>
>> rdtgrp->mode = user_m;
>
> Will do.
Actually ... now that I take a closer look, this would let an invalid
mode change (RDT_MODE_PSEUDO_LOCKED) slip through so I plan to keep this
code as is.
> It also looks like the only reason for the "mode" local variable was to
> make those earlier "strcmp" lines shorter. With those long lines removed
> in this patch this local variable is no longer needed and I will remove
> it also.
I will still do this.
Reinette