linux-kernel - Re: [PATCH printk v2 04/38] printk: introduce console_is

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: <Y1JfFTAIbcFOrPtD@alley>
Date:   Fri, 21 Oct 2022 10:57:57 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v2 04/38] printk: introduce console_is_enabled()
 wrapper

On Wed 2022-10-19 17:01:26, John Ogness wrote:
> After switching to SRCU for console list iteration, some readers
> will begin accessing console->flags as a data race. This is safe
> because there is at most one CPU modifying console->flags and
> using rmw operations.
> 
> The primary reason for readers to access console->flags is to
> check if the console is enabled. Introduce console_is_enabled()
> to mark such access as a data race.
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> ---
>  include/linux/console.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index cff86cc615f8..60195cd086dc 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -172,6 +172,26 @@ extern void console_srcu_read_unlock(int cookie);
>  
>  extern struct hlist_head console_list;
>  
> +/**
> + * console_is_enabled - Check if the console is enabled
> + * @con:	struct console pointer of console to check
> + *
> + * This should be used instead of manually testing for the CON_ENABLED
> + * bit in the console->flags.
> + *
> + * Context: Any context.
> + */
> +static inline bool console_is_enabled(const struct console *con)
> +{
> +	/*
> +	 * If SRCU is used, reading of console->flags can be a data
> +	 * race. However, this is safe because there is at most one
> +	 * CPU modifying console->flags and it is using only
> +	 * read-modify-write operations to do so.

Hmm, I somehow do not understand the explanation. How does
read-modify-write operation make this safe, please?

We are interested into one bit. IMHO, it is not important
if the flags variable is modified atomically or byte by byte.
The important thing is if the reading is synchronized against
modifications.

This function does not do any synchronization on its own.
So, it depends on the caller.


I would personally do two variants. for example:

    console_is_enabled()
    console_is_enabled_safe()

The first variant would be called in situations where the race
does not matter and the other when it matters.



> +	 */
> +	return (data_race(READ_ONCE(con->flags)) & CON_ENABLED);
> +}
> +
>  /**
>   * for_each_console_srcu() - Iterator over registered consoles
>   * @con:	struct console pointer used as loop cursor

Best Regards,
Petr