linux-kernel - Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg

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: <20190923113936.73lhmpxurynem62e@pathway.suse.cz>
Date:   Mon, 23 Sep 2019 13:39:36 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     He Zhe <zhe.he@...driver.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        sergey.senozhatsky@...il.com, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org,
        John Ogness <john.ogness@...utronix.de>
Subject: Re: [PATCH] printk: Fix unnecessary returning broken pipe error from
 devkmsg_read

On Mon 2019-09-23 18:45:18, He Zhe wrote:
> 
> 
> On 9/23/19 6:05 PM, Sergey Senozhatsky wrote:
> > On (09/18/19 21:31), zhe.he@...driver.com wrote:
> >> When users read the buffer from start, there is no need to return -EPIPE
> >> since the possible overflows will not affect the output.
> >>
> > [..]
> >> -	if (user->seq < log_first_seq) {
> >> +	if (user->seq == 0) {
> >> +		user->seq = log_first_seq;
> >> +	} else if (user->seq < log_first_seq) {
> >>  		/* our last seen message is gone, return error and reset */
> >>  		user->idx = log_first_idx;
> >>  		user->seq = log_first_seq;
> > A tough call.
> >
> > User-space wants to read messages which are gone: log_first_seq > user->seq.
> >
> > I think returning EPIPE is sort of appropriate; user-space, possibly,
> > can printf(stderr, "Some /dev/kmsg messages are gone\n"); or something
> > like that.
> 
> Yes, a tough call. I was looking at the similar part of RT kernel and thought
> that handling was better.
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/kernel/printk/printk.c?h=linux-5.2.y-rt#n706

Adding John into CC. He takes care of printk() in RT kernel.


> IMHO, the EPIPE is used to inform user-space when the buffer has overflown and
> there is a inconsistency/break between what has been read and what has not.

What is the motivation for the change, please?
Have you just noticed the inconsistency between normal and RT kernel?
Or was there any bug report?

We need to be careful to do not break user space. And this patch
modifies the existing behavior.

> When user-space just wants to read the buffer from sequence 0, there would not
> be the inconsistency.
> 
> I think it is NOT necessary to inform user-space, when it just wants to read
> from the beginning of the buffer, that the buffer has changed since the time
> point when it issues the action of reading.

Who would set sequence 0, please?

IMHO, the patch is wrong.

devkmsg_open() initializes user->seq to a valid sequence number.
We should return -EPIPE when user->seq == 0 during devkmsg_open()
and the first messages got lost before the first read.

> But if that IS what we want, the RT
> kernel needs to be changed so that mainline and RT kernel act in the same way.

I agree.

Best Regards,
Petr