linux-kernel - Re: [PATCH V5 4/4] x86/resctrl: Use appropriate API for strings terminated by newline

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 May 2020 08:50:22 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Borislav Petkov <bp@...en8.de>,
        Tony Luck <tony.luck@...el.com>, kuo-lang.tseng@...el.com,
        ravi.v.shankar@...el.com, Ingo Molnar <mingo@...hat.com>,
        Babu Moger <babu.moger@....com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH V5 4/4] x86/resctrl: Use appropriate API for strings
 terminated by newline

Hi Andy,

On a high level the changes from v4 to v5 aimed to address your feedback
and also ensure that original user interface behavior is maintained.

On 5/19/2020 1:28 AM, Andy Shevchenko wrote:
> On Tue, May 19, 2020 at 2:50 AM Reinette Chatre
> <reinette.chatre@...el.com> wrote:
> 
> ...
> 
>> +       ret = sysfs_match_string(rdt_mode_str, buf);
>> +       if (ret < 0) {
>> +               rdt_last_cmd_puts("Unknown or unsupported mode\n");
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }

>From your previous email ...

>> +       ret = sysfs_match_string(rdt_mode_str, buf);
>> +       if (ret < 0) {
>> +               rdt_last_cmd_puts("Unknown or unsupported mode\n");
>
>> +               ret = -EINVAL;
>
> This is redundant.

I understand that shadowing an error code is generally of concern. In
this case the error code is set to -EINVAL to ensure that it is the same
error code that was returned to user space originally and will continue
to be so no matter what changes may come to sysfs_match_string().

>> +
>> +       user_m = ret;
> 
>> +       } else if (user_m == RDT_MODE_PSEUDO_LOCKED) {
>>                 rdt_last_cmd_puts("Unknown or unsupported mode\n");
>>                 ret = -EINVAL;
>> +               goto out;
>>         }
> 
> Can't we unify latter with a former like
> 
>   if (ret == RDT_MODE_PSEUDO_LOCKED)
>     ret = -EINVAL;
>   if (ret < 0) {
>       rdt_last_cmd_puts("Unknown or unsupported mode\n");
>       goto out;
>   }
> 
> ?
> 
> Or closer to initial like
> 
>   if (ret < 0 || ret == RDT_MODE_PSEUDO_LOCKED) {
>     rdt_last_cmd_puts("Unknown or unsupported mode\n");
>     ret = -EINVAL;
>     goto out;
>   }
> 

This would have been ideal if done from the start but currently "0" is
returned if the current mode is pseudo-locked and user attempts to
change the mode to pseudo-locked. Thus, to maintain the current user
interface the check if user wants to set pseudo-locked mode is moved
after the check if new mode is same as existing mode and thus not
unified because that will result in an error returned always when user
requests pseudo-locked mode.

Reinette