linux-kernel - Re: [PATCH printk v3 07/40] console: 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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 11 Nov 2022 14:32:05 +0100
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 v3 07/40] console: introduce console_is_enabled()
 wrapper

On Thu 2022-11-10 16:11:43, John Ogness wrote:
> On 2022-11-08, Petr Mladek <pmladek@...e.com> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
> >>  {
> >>  	__pr_flush(console, 1000, true);
> >>  	console_lock();
> >> -	console->flags &= ~CON_ENABLED;
> >> +	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
> >
> > My first reaction is that using the atomic operation only for the
> > store side is suspicious. It is correct because the read is serialized
> > by console_lock(). But it is far from obvious why we need and can do
> > it this way.
> 
> The READ_ONCE()/WRITE_ONCE() usage is really about documenting data-race
> reads and the writes that they are racing with.
> 
> For WRITE_ONCE() the rule is:
> 
> - If console->flags is modified for a registered console, it is done
>   under the console_list_lock and using WRITE_ONCE().
> 
> If we use a wrapper for this rule, then we can also add the lockdep
> assertion that console_list_lock is held.
>
> For READ_ONCE() the rule is:
> 
> - If console->flags is read for a registered console, then either
>   console_list_lock must be held _or_ it must be read via READ_ONCE().
>
> If we use wrappers here, then we can use lockdep assertion on
> console_list_lock for the non-READ_ONCE wrapper, and scru_read_lock
> assertion for the READ_ONCE wrapper.

Exactly, all the assertions are one big advantage for hiding this into
an API.

> > It would deserve a comment. But there are several other writes.
> > Also it is not obvious why many other con->flags modifications
> > do not need this.
> >
> > I think about hiding this into an API. We could also add some
> > checks that it is used the right way. Also it might make sense
> > to avoid using the READ_ONCE()/WRITE_ONCE by using
> > set_bit()/test_bit().
> 
> I do not see any advantage of set_bit()/test_bit(). They have the
> disadvantage that they only work with 1 bit at a time. And there are
> multiple sites where more than 1 bit is set/tested. It is important that
> the multi-bit tests are simultaneous.

Fair enough.

> > I would prefer to use the proposed API because it should make all
> > accesses more clear and safe. And most importantly, it would help use
> > to catch bugs.
> >
> > But I do not resist on it. The patch looks correct and we could do
> > this later. I could live with it if we add some comments above the
> > WRITE_ONCE() calls.
> 
> I do not want to do a full API replacement for all console->flags access
> in this series or at this time. I am concerned that it is taking us too
> far away from our current goal. Also, with the upcoming atomic/threaded
> model, all consoles need to be modified that want to use it anyway. So
> that would be a more appropriate time to require the use of new API's.

Fair enough.

> For console_is_enabled() I will add the srcu_read_lock check. I suppose
> I should also name the function console_srcu_is_enabled().
> 
> For the WRITE_ONCE() calls, I will add a static inline wrapper in
> printk.c that includes the lockdep console_list_lock assertion. Perhaps
> called console_srcu_write_flags(struct console *con, short flags).
> 
> In console_srcu_write_flags() and console_srcu_is_enabled() I can
> document their relationship and when they are to be used. Both these
> functions are used rarely and should be considered the exception, not
> the rule.
> 
> For code that is reading registered console->flags under the
> console_list_lock, I will leave the "normal access" as is. Just as I am
> leaving the "normal access" for non-registered console-flags as is. We
> can convert those to a new generic API later if we think it is really
> necessary.

Sounds reasonable.

The main motivation for introducing the wrappers is that there are
currently 40+ locations where console->flags are touched. It is easy
to miss something especially when we are reworking the locking around
this code. Some of the callers are outside kernel/printk/* which
even increases the risk of a misuse. The lockdep checks
would help us to catch potential mistakes.

Anyway, I am fine with adding the wrappers for the synchronized reads
later. This patchset is already big enough.

Best Regards,
Petr