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 | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index fbee891a7d6e..623e33c0a290 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1412,11 +1412,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;
/* Valid input requires a trailing newline */
if (nbytes == 0 || buf[nbytes - 1] != '\n')
return -EINVAL;
- buf[nbytes - 1] = '\0';
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (!rdtgrp) {
@@ -1428,11 +1428,15 @@ 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))
+ user_m = sysfs_match_string(rdt_mode_str, buf);
+ if (user_m < 0) {
+ rdt_last_cmd_puts("Unknown or unsupported mode\n");
+ ret = user_m;
+ goto out;
+ }
+
+ /* Do nothing and return success if user asks for current mode */
+ if (user_m == mode)
goto out;
if (mode == RDT_MODE_PSEUDO_LOCKED) {
@@ -1441,14 +1445,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;
@@ -1459,14 +1463,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 Wed, Apr 01, 2020 at 11:30:48AM -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.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Reinette Chatre <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index fbee891a7d6e..623e33c0a290 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1412,11 +1412,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;
>
> /* Valid input requires a trailing newline */
> if (nbytes == 0 || buf[nbytes - 1] != '\n')
> return -EINVAL;
> - buf[nbytes - 1] = '\0';
The above test is not needed and comment now is misleading.
WRT nbytes I believe that kernel fs code checks for that.
> rdtgrp = rdtgroup_kn_lock_live(of->kn);
> if (!rdtgrp) {
> @@ -1428,11 +1428,15 @@ 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))
> + user_m = sysfs_match_string(rdt_mode_str, buf);
> + if (user_m < 0) {
> + rdt_last_cmd_puts("Unknown or unsupported mode\n");
> + ret = user_m;
> + goto out;
> + }
You can do it the way
ret = sysfs_match_string(...);
if (ret < 0) {
...
}
user_m = ret;
> + /* Do nothing and return success if user asks for current mode */
> + if (user_m == mode)
> goto out;
>
> if (mode == RDT_MODE_PSEUDO_LOCKED) {
> @@ -1441,14 +1445,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;
> @@ -1459,14 +1463,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
>
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 02, 2020 at 04:06:25PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 01, 2020 at 11:30:48AM -0700, Reinette Chatre wrote:
...
> > int ret = 0;
> > + int user_m;
...and forgot to mention this...
int user_m;
int ret;
> > /* Valid input requires a trailing newline */
> > if (nbytes == 0 || buf[nbytes - 1] != '\n')
> > return -EINVAL;
> > - buf[nbytes - 1] = '\0';
>
> The above test is not needed and comment now is misleading.
> WRT nbytes I believe that kernel fs code checks for that.
>
> > rdtgrp = rdtgroup_kn_lock_live(of->kn);
> > if (!rdtgrp) {
> > @@ -1428,11 +1428,15 @@ 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))
> > + user_m = sysfs_match_string(rdt_mode_str, buf);
> > + if (user_m < 0) {
> > + rdt_last_cmd_puts("Unknown or unsupported mode\n");
> > + ret = user_m;
> > + goto out;
> > + }
>
> You can do it the way
>
> ret = sysfs_match_string(...);
> if (ret < 0) {
> ...
> }
> user_m = ret;
...and this changes
ret = 0;
--
With Best Regards,
Andy Shevchenko
Hi Andy,
(Your two responses have been merged)
On 4/2/2020 6:06 AM, Andy Shevchenko wrote:
> On Wed, Apr 01, 2020 at 11:30:48AM -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.
>>
>> Suggested-by: Andy Shevchenko <[email protected]>
>> Signed-off-by: Reinette Chatre <[email protected]>
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index fbee891a7d6e..623e33c0a290 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1412,11 +1412,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;
>>
>
> ...and forgot to mention this...
>
> int user_m;
> int ret;
>
>
>> /* Valid input requires a trailing newline */
>> if (nbytes == 0 || buf[nbytes - 1] != '\n')
>> return -EINVAL;
>> - buf[nbytes - 1] = '\0';
>
> The above test is not needed and comment now is misleading.
> WRT nbytes I believe that kernel fs code checks for that.
If nbytes is 0 it is still passed to this function. You are correct that
those tests are not needed though (if nbytes is 0 then
sysfs_match_string() will not find a match and return EINVAL via that path).
Thank you for catching this. I'll remove those unnecessary checks.
>
>> rdtgrp = rdtgroup_kn_lock_live(of->kn);
>> if (!rdtgrp) {
>> @@ -1428,11 +1428,15 @@ 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))
>> + user_m = sysfs_match_string(rdt_mode_str, buf);
>> + if (user_m < 0) {
>> + rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> + ret = user_m;
>> + goto out;
>> + }
>
> You can do it the way
>
> ret = sysfs_match_string(...);
> if (ret < 0) {
> ...
> }
> user_m = ret;
>
> ...and this changes
>
> ret = 0;
>
>
ok, I'll do it this way in the next version.
Thank you
Reinette
On Fri, Apr 3, 2020 at 12:54 AM Reinette Chatre
<[email protected]> wrote:
> On 4/2/2020 6:06 AM, Andy Shevchenko wrote:
> > On Wed, Apr 01, 2020 at 11:30:48AM -0700, Reinette Chatre wrote:
...
> >> @@ -1412,11 +1412,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;
> >>
>
> >
> > ...and forgot to mention this...
> >
> > int user_m;
> > int ret;
> >
>
> >
> >> /* Valid input requires a trailing newline */
> >> if (nbytes == 0 || buf[nbytes - 1] != '\n')
> >> return -EINVAL;
> >> - buf[nbytes - 1] = '\0';
> >
> > The above test is not needed and comment now is misleading.
> > WRT nbytes I believe that kernel fs code checks for that.
This module provides it's own kernfs_ops...
> If nbytes is 0 it is still passed to this function. You are correct that
> those tests are not needed though (if nbytes is 0 then
> sysfs_match_string() will not find a match and return EINVAL via that path).
>
> Thank you for catching this. I'll remove those unnecessary checks.
...which means that nbytes == 0 is a valid check. Please keep it
there. It will protect from unnecessary locking and loading CPU for
nothing.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 4/3/2020 12:27 AM, Andy Shevchenko wrote:
> On Fri, Apr 3, 2020 at 12:54 AM Reinette Chatre
> <[email protected]> wrote:
>> On 4/2/2020 6:06 AM, Andy Shevchenko wrote:
>>> On Wed, Apr 01, 2020 at 11:30:48AM -0700, Reinette Chatre wrote:
...
>>>> /* Valid input requires a trailing newline */
>>>> if (nbytes == 0 || buf[nbytes - 1] != '\n')
>>>> return -EINVAL;
>>>> - buf[nbytes - 1] = '\0';
>>>
>>> The above test is not needed and comment now is misleading.
>>> WRT nbytes I believe that kernel fs code checks for that.
>
> This module provides it's own kernfs_ops...
>
>> If nbytes is 0 it is still passed to this function. You are correct that
>> those tests are not needed though (if nbytes is 0 then
>> sysfs_match_string() will not find a match and return EINVAL via that path).
>>
>> Thank you for catching this. I'll remove those unnecessary checks.
>
> ...which means that nbytes == 0 is a valid check. Please keep it
> there. It will protect from unnecessary locking and loading CPU for
> nothing.
>
Will do. Thank you.
Reinette