linux-kernel - Re: [PATCH printk-rework 08/14] printk: add syslog

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, 19 Feb 2021 14:30:41 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk-rework 08/14] printk: add syslog_lock

On Thu 2021-02-18 09:18:11, John Ogness wrote:
> The global variables @syslog_seq, @syslog_partial, @syslog_time
> and write access to @clear_seq are protected by @logbuf_lock.
> Once @logbuf_lock is removed, these variables will need their
> own synchronization method. Introduce @syslog_lock for this
> purpose.
> 
> @syslog_lock is a raw_spin_lock for now. This simplifies the
> transition to removing @logbuf_lock. Once @logbuf_lock and the
> safe buffers are removed, @syslog_lock can change to spin_lock.
> ---
>  kernel/printk/printk.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 20c21a25143d..401df370832b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> +/* Return a consistent copy of @syslog_seq. */
> +static u64 read_syslog_seq_irq(void)
> +{
> +	u64 seq;
> +
> +	raw_spin_lock_irq(&syslog_lock);
> +	seq = syslog_seq;
> +	raw_spin_unlock_irq(&syslog_lock);

Is there any particular reason to disable interrupts here?

It would make sense only when the lock could be taken in IRQ
context. Then we would need to always disable interrupts when
the lock is taken. And if it is taken in IRQ context, we would
need to safe flags.

I guess that you got confused because it is used in
wait_event_interruptible(). The name is misleading.
"interruptible" means wake_up_process() and not IRQ here.

IMHO, we should remove _irq suffix from the lock operations
and also from the function name.

> +
> +	return seq;
> +}
> +
>  int do_syslog(int type, char __user *buf, int len, int source)
>  {
>  	struct printk_info info;
> @@ -1664,8 +1688,9 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  			return 0;
>  		if (!access_ok(buf, len))
>  			return -EFAULT;
> +
>  		error = wait_event_interruptible(log_wait,
> -				prb_read_valid(prb, syslog_seq, NULL));
> +				prb_read_valid(prb, read_syslog_seq_irq(), NULL));
>  		if (error)
>  			return error;
>  		error = syslog_print(buf, len);

Otherwise, the patch looks good to me.

Best Regards,
Petr