linux-kernel - Re: [PATCH V6 09/20] rtla: Add osnoise tool

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:   Fri, 29 Oct 2021 18:03:30 +0200
From:   Daniel Bristot de Oliveira <bristot@...nel.org>
To:     Tao Zhou <tao.zhou@...ux.dev>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Clark Williams <williams@...hat.com>,
        John Kacur <jkacur@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-rt-users@...r.kernel.org, linux-trace-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 09/20] rtla: Add osnoise tool

On 10/27/21 18:30, Tao Zhou wrote:
> On Wed, Oct 27, 2021 at 12:06:20AM +0200, Daniel Bristot de Oliveira wrote:
> 
>> +/*
>> + * osnoise_set_cpus - configure osnoise to run on *cpus
>> + *
>> + * "osnoise/cpus" file is used to set the cpus in which osnoise/timerlat
>> + * will run. This function opens this file, saves the current value,
>> + * and set the cpus passed as argument.
>> + */
>> +int osnoise_set_cpus(struct osnoise_context *context, char *cpus)
>> +{
>> +	char *osnoise_cpus = tracefs_get_tracing_file("osnoise/cpus");
>> +	char curr_cpus[1024];
>> +	int retval;
>> +
>> +	context->cpus_fd = open(osnoise_cpus, O_RDWR);
>> +	if (!context->cpus_fd)
>> +		goto out_err;
> The above check should be "context->cpus_fd < 0".

Revisited all open/read/write!

>> +	retval = read(context->cpus_fd, &curr_cpus, sizeof(curr_cpus));
>> +	if (!retval)
>> +		goto out_close;
>> +	context->orig_cpus = strdup(curr_cpus);
>> +	if (!context->orig_cpus)
>> +		goto out_err;
> Need to close ->cpus_fd;
> 
>   if (!context->orig_cpus)
>     goto out_close;
> 
>> +	retval = write(context->cpus_fd, cpus, strlen(cpus) + 1);
>> +	if (!retval)
>> +		goto out_err;
> Same as above. Use "goto out_close" instead.

yep! fixed in the next version.

>> +	tracefs_put_tracing_file(osnoise_cpus);
>> +
>> +	return 0;
>> +
>> +out_close:
>> +	close(context->cpus_fd);
>> +	context->cpus_fd = -1;
>> +out_err:
>> +	tracefs_put_tracing_file(osnoise_cpus);
>> +	return 1;
>> +}
>> +
>> +/*
>> + * osnoise_restore_cpus - restore the original "osnoise/cpus"
>> + *
>> + * osnoise_set_cpus() saves the original data for the "osnoise/cpus"
>> + * file. This function restore the original config it was previously
>> + * modified.
>> + */
>> +void osnoise_restore_cpus(struct osnoise_context *context)
>> +{
>> +	int retval;
>> +
>> +	if (!context->orig_cpus)
>> +		return;
>> +
>> +	retval = write(context->cpus_fd, context->orig_cpus, strlen(context->orig_cpus));
> __osnoise_write_runtime() check "context->cpus_fd == -1".
> Is it possible here we need to check "context->cpus_fd == -1".
> 

So, yeah, this was inconsistent. In some parts I checked the fd, on other I
checked if the original value was load, so the file was opened. In the next
version I am checking if the original value was loaded, and then using it to
define if the fd is open.

Thanks for your review Tao!
-- Daniel