linux-kernel - Re: [PATCH V4 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 for Android: free password hash cracker in your pocket

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

Message-ID: <060e3cf6-8fcb-6b43-01ef-63ef0a5ff64a@intel.com>
Date:   Mon, 18 May 2020 09:06:00 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     tglx@...utronix.de, fenghua.yu@...el.com, bp@...en8.de,
        tony.luck@...el.com, kuo-lang.tseng@...el.com,
        ravi.v.shankar@...el.com, mingo@...hat.com, babu.moger@....com,
        hpa@...or.com, x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 4/4] x86/resctrl: Use appropriate API for strings
 terminated by newline

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