linux-kernel - Re: [PATCH printk v3 6/6] printk: syslog: close window between wait and read

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: <YNXuSx5KaCSaotqC@alley>
Date:   Fri, 25 Jun 2021 16:55:07 +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
Subject: Re: [PATCH printk v3 6/6] printk: syslog: close window between wait
 and read

On Fri 2021-06-25 10:17:40, John Ogness wrote:
> On 2021-06-24, Petr Mladek <pmladek@...e.com> wrote:
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 90954cb5a0ab..4737804d6c6d 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -1542,8 +1570,13 @@ static int syslog_print(char __user *buf, int size)
> >>  		len += n;
> >>  		size -= n;
> >>  		buf += n;
> >> -	}
> >>  
> >> +		if (!size)
> >> +			break;
> >
> > This looks like an unrelated optimization. If I get it correctly, it
> > does not change the existing behavior.
> 
> It was a necessary change in order to preserve the existing logic but
> allow the lock to be held when enterring the loop. Before the patch we
> have:
> 
>         ...get seq to read...
> 
>         while (size > 0) {
>             mutex_lock(&syslog_lock);
>             ...read record...
>             mutex_unlock(&syslog_lock);
>             ...copy record...
>        }
> 
> After the patch we enter the loop with the lock already held. So this
> changes the code to:
> 
>         mutex_lock(&syslog_lock);
>         ...get seq to read...
> 
>         for (;;) {
>             ...read record...
>             mutex_unlock(&syslog_lock);
>             ...copy record...
>            
>             if (!size)
>                 break;
>             mutex_lock(&syslog_lock);               
>         }
> 
> Note that @size always starts with >0, so there is no need to check it
> at the beginning of the loop. And checking for !0 instead of >0 is also
> ok, since @size will never be less than zero.

Ah, I have missed that you replaced the while-cycle with a for-cycle.
It makes sense now.

Plese, just mention these changes in the commit message. I mean that
size is always >0 at the befinning and never <0 later.

> > The patch itself makes sense. It somehow fixes a long standing race.
> > Even though the result still might be racy. The lock is released
> > when each record is copied to the user-provided buffer.
> 
> I do not understand this conclusion. The existing race is
> real. SYSLOG_ACTION_READ could return with no data, not because there is
> no records available, but because the race was hit. With this patch that
> race is closed: SYSLOG_ACTION_READ will either return with data or with
> an error.
> 
> You claim the result is still racy, but I do not know what you are
> referring to. If you have multiple readers, they will get different
> records (and record pieces), but collectively no data would be lost and
> no data would be redundant. And no readers would return from
> SYSLOG_ACTION_READ without data.

I mean that each reader will still get random lines. The race is that
it is not guaranteed what reader would get what lines.

By other words, the improvement is that each reader will read
at least something. But it is still not guaranteed that it will
see everything.

My understanding is that it was designed for a single daemon reading
all messages. And dmesg might probably cause races when using
the syslog interface.

Best Regards,
Petr