linux-kernel - Re: [RFC PATCH 2/2] tracing: add testable specifications for event_enable

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]

Message-ID: <20250702111245.1fa23138@batman.local.home>
Date: Wed, 2 Jul 2025 11:12:45 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Gabriele Paoloni <gpaoloni@...hat.com>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 acarmina@...hat.com, chuck.wolber@...ing.com
Subject: Re: [RFC PATCH 2/2] tracing: add testable specifications for
 event_enable_write/read

On Wed, 2 Jul 2025 16:59:29 +0200
Gabriele Paoloni <gpaoloni@...hat.com> wrote:

> Mmm got it. What about
> 
> * Function's expectations:
> * - This function shall lock the global event_mutex before performing any
> *   operation on the target event file and unlock it after all operations on
> *   the target event file have completed;

Since 99% of the time that a lock is taken in a function it is
released, I think that should be the default assumption here, and only
when a lock is taken and not release, that should be explicitly called
out.

And also we should remove "This function" we know that these
requirements are for this function.

  - The global event_mutex shall be taken before performing any
    operation on the target event.

Should be good enough.

If the lock can be released and taken again, that too should be
explicit in the requirements otherwise it is assumed it is taken once
and not released until the operation is completed.

> *
> * - This function shall format the string copied to userspace according to
> *   the status flags retrieved from the target event file:
> *    - The first character shall be set to "1" if the enabled flag is
> set AND the
> *      soft_disabled flag is not set, else it shall be set to "0";
> *    - The second character is optional and shall be set to "*" if either the
> *      soft_disabled flag or the soft_mode flag is set;
> *   - The string shall be terminated by a newline ("\n") and any remaining
> *      character shall be set to "0";

 - The string copied to user space shall be formatted according to the
   status flags from the target event file:

   - If the enable flag is set AND the soft_disable flag is not set then
     the first character shall be set to "1" ELSE it shall be set to "0"

   - If either the soft_disable fag or the soft_mode flag is set then the
     second character shall be set to "*" ELSE it is skipped.

I think the above is easier to read and is a bit more consolidated.
Stating the status then the effect is also easier to read.

-- Steve


> *
> * - This function shall invoke simple_read_from_buffer() to perform the copy
> *   of the kernel space string to ubuf.
> 
> (pls note that the check on cnt has been removed in v3 that is out already)