[PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

When users read the buffer from start, there is no need to return -EPIPE
since the possible overflows will not affect the output.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 kernel/printk/printk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1888f6a..4a6a129 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -886,7 +886,9 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
 		logbuf_lock_irq();
 	}
 
-	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;
-- 
2.7.4

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[Sergey Senozhatsky]

On (09/18/19 21:31), zhe.he@windriver.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.

	-ss

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[He Zhe]

On 9/23/19 6:05 PM, Sergey Senozhatsky wrote:
> On (09/18/19 21:31), zhe.he@windriver.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

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.

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. 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.


Zhe

>
> 	-ss
>

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[Petr Mladek]

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@windriver.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

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[He Zhe]

On 9/23/19 7:39 PM, Petr Mladek wrote:
> 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@windriver.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?

I mean when messages that are going to be read get lost, there is an
inconsistency between what we want and what are left.

This is all due to an LTP case which passes on mainline kernel but fails
(line 425) on RT kernel.

https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/logging/kmsg/kmsg01.c#L425

But after comparing how mainling and RT kernel handle the case, I thought it was
better not to return an EPIPE.

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



>
> 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?

When log_first_seq is 0 in  devkmsg_open.

>
> 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.

If this is the case, I'm going to submit a patch for RT kernel.

Thanks,
Zhe

>
> Best Regards,
> Petr
>

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[John Ogness]

On 2019-09-23, He Zhe <zhe.he@windriver.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;
>>
>> IMHO, the patch is wrong.
>
> If this is the case, I'm going to submit a patch for RT kernel.

It should be enough just to remove the if-branch.

FYI, this bug only exists in the RFCv1 of my proposed printk patchset,
which is what RT 5.x is based on. The code currently being prepared for
mainline does not have this issue.

Thanks for reporting. Looking forward to your patch.

John Ogness

Re: [PATCH] printk: Fix unnecessary returning broken pipe error from devkmsg_read

[Sergey Senozhatsky]

On (09/23/19 18:45), He Zhe wrote:
> 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.

The point here is not to notify user space that the logbuf has changed,
but to notify user space that unseen messages are gone, we lost them,
user space will never read them.

	-ss