[PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Jan Beulich]

The recent changes to the printk buffer management resulted in
SYSLOG_ACTION_READ to only return a single message, whereas previously
the buffer would get filled as much as possible. As, when too small to
fit everything, filling it to the last byte would be pretty ugly with
the new code, the patch arranges for as many messages as possible to
get returned in a single invocation. User space tools in at least all
SLES versions depend on the old behavior.

This at once addresses the issue attempted to get fixed with commit
b56a39ac263e5b8cafedd551a49c2105e68b98c2 ("printk: return -EINVAL if
the message len is bigger than the buf size"), and since that commit
widened the possibility for losing a message altogether, the patch
here assumes that this other commit would get reverted first
(otherwise the patch here won't apply).

Furthermore, this patch also addresses the problem dealt with in
commit 4a77a5a06ec66ed05199b301e7c25f42f979afdc ("printk: use mutex
lock to stop syslog_seq from going wild"), so I'd recommend reverting
that one too (albeit there's no direct collision between the two).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: Kay Sievers <kay@vrfy.org>
Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>

---
 kernel/printk.c |   51 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 14 deletions(-)

--- 3.5-rc3/kernel/printk.c
+++ 3.5-rc3-syslog-read/kernel/printk.c
@@ -860,26 +860,49 @@ static int syslog_print(char __user *buf
 {
 	char *text;
 	struct log *msg;
-	int len;
+	int len = 0;
 
 	text = kmalloc(LOG_LINE_MAX, GFP_KERNEL);
 	if (!text)
 		return -ENOMEM;
 
-	raw_spin_lock_irq(&logbuf_lock);
-	if (syslog_seq < log_first_seq) {
-		/* messages are gone, move to first one */
-		syslog_seq = log_first_seq;
-		syslog_idx = log_first_idx;
-	}
-	msg = log_from_idx(syslog_idx);
-	len = msg_print_text(msg, true, text, LOG_LINE_MAX);
-	syslog_idx = log_next(syslog_idx);
-	syslog_seq++;
-	raw_spin_unlock_irq(&logbuf_lock);
+	while (size > 0) {
+		size_t n;
+
+		raw_spin_lock_irq(&logbuf_lock);
+		if (syslog_seq < log_first_seq) {
+			/* messages are gone, move to first one */
+			syslog_seq = log_first_seq;
+			syslog_idx = log_first_idx;
+		}
+		if (syslog_seq == log_next_seq) {
+			raw_spin_unlock_irq(&logbuf_lock);
+			break;
+		}
+		msg = log_from_idx(syslog_idx);
+		n = msg_print_text(msg, true, text, LOG_LINE_MAX);
+		if (n <= size) {
+			syslog_idx = log_next(syslog_idx);
+			syslog_seq++;
+		} else
+			n = 0;
+		raw_spin_unlock_irq(&logbuf_lock);
+
+		if (!n)
+			break;
 
-	if (len > 0 && copy_to_user(buf, text, len))
-		len = -EFAULT;
+		len += n;
+		size -= n;
+		buf += n;
+		n = copy_to_user(buf - n, text, n);
+
+		if (n) {
+			len -= n;
+			if (!len)
+				len = -EFAULT;
+			break;
+		}
+	}
 
 	kfree(text);
 	return len;

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Kay Sievers]

On Fri, Jun 22, 2012 at 5:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> The recent changes to the printk buffer management resulted in
> SYSLOG_ACTION_READ to only return a single message, whereas previously
> the buffer would get filled as much as possible. As, when too small to
> fit everything, filling it to the last byte would be pretty ugly with
> the new code, the patch arranges for as many messages as possible to
> get returned in a single invocation. User space tools in at least all
> SLES versions depend on the old behavior.

What a weird assumption for using that interface. It does a simple
one-time read and assumes it will never block in that? If something
would clear the log, or it would not contain a message, that read will
block and the tool would just hang? :)

But sure, sounds needed to restore the old behaviour if such weird
users exist, and the patch looks fine from looking over it.

> This at once addresses the issue attempted to get fixed with commit
> b56a39ac263e5b8cafedd551a49c2105e68b98c2 ("printk: return -EINVAL if
> the message len is bigger than the buf size"), and since that commit
> widened the possibility for losing a message altogether, the patch
> here assumes that this other commit would get reverted first
> (otherwise the patch here won't apply).
>
> Furthermore, this patch also addresses the problem dealt with in
> commit 4a77a5a06ec66ed05199b301e7c25f42f979afdc ("printk: use mutex
> lock to stop syslog_seq from going wild"), so I'd recommend reverting
> that one too (albeit there's no direct collision between the two).

Are you sure that is covered? Doesn't the other thread would just
return 0 to the caller then, instead of continuing to stay in the
syscall when the first thread got the message?

Kay

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Jan Beulich]

>>> On 23.06.12 at 20:03, Kay Sievers <kay@vrfy.org> wrote:
>> Furthermore, this patch also addresses the problem dealt with in
>> commit 4a77a5a06ec66ed05199b301e7c25f42f979afdc ("printk: use mutex
>> lock to stop syslog_seq from going wild"), so I'd recommend reverting
>> that one too (albeit there's no direct collision between the two).
> 
> Are you sure that is covered? Doesn't the other thread would just
> return 0 to the caller then, instead of continuing to stay in the
> syscall when the first thread got the message?

The old code permitted returning zero in that case too, so I don't
see why the new code shouldn't be allowed to. But anyway, as
said this patch doesn't directly conflict, and hence it's up to the
maintainer(s) of the code to decide whether to keep it. The
conflicting one, however, imo ought to be reverted in any case.

Jan

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Greg KH]

On Mon, Jun 25, 2012 at 08:05:17AM +0100, Jan Beulich wrote:
> >>> On 23.06.12 at 20:03, Kay Sievers <kay@vrfy.org> wrote:
> >> Furthermore, this patch also addresses the problem dealt with in
> >> commit 4a77a5a06ec66ed05199b301e7c25f42f979afdc ("printk: use mutex
> >> lock to stop syslog_seq from going wild"), so I'd recommend reverting
> >> that one too (albeit there's no direct collision between the two).
> > 
> > Are you sure that is covered? Doesn't the other thread would just
> > return 0 to the caller then, instead of continuing to stay in the
> > syscall when the first thread got the message?
> 
> The old code permitted returning zero in that case too, so I don't
> see why the new code shouldn't be allowed to. But anyway, as
> said this patch doesn't directly conflict, and hence it's up to the
> maintainer(s) of the code to decide whether to keep it. The
> conflicting one, however, imo ought to be reverted in any case.

Ok, so I'm confused, you want me to apply this patch and then revert a
different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?

Kay, do you agree?

thanks,

greg k-h

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Kay Sievers]

On Mon, Jun 25, 2012 at 9:20 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

> Ok, so I'm confused, you want me to apply this patch and then revert a
> different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?

b56a39ac conflicts and is replaced with that patch.
4a77a5a0 should stay, as it fixes an actual problem, even with the old
code, I think.

> Kay, do you agree?

Yes, seems, we need to restore the old behaviour, even when it's worse
than the current one. The stuff SUSE does here really doesn't sound
right, but it's history done long ago, and we an not change that.

Thanks,
Kay

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Greg KH]

On Mon, Jun 25, 2012 at 09:35:03PM +0200, Kay Sievers wrote:
> On Mon, Jun 25, 2012 at 9:20 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > Ok, so I'm confused, you want me to apply this patch and then revert a
> > different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?
> 
> b56a39ac conflicts and is replaced with that patch.
> 4a77a5a0 should stay, as it fixes an actual problem, even with the old
> code, I think.

Ok, I'm even more confused now.  Maybe I just need more coffee.

Can you list what exactly I should do here?  Should I apply this patch?
Revert some other patch?  Which one(s)?

lost,

greg k-h

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Kay Sievers]

On Tue, Jun 26, 2012 at 2:01 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Jun 25, 2012 at 09:35:03PM +0200, Kay Sievers wrote:
>> On Mon, Jun 25, 2012 at 9:20 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> > Ok, so I'm confused, you want me to apply this patch and then revert a
>> > different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?
>>
>> b56a39ac conflicts and is replaced with that patch.
>> 4a77a5a0 should stay, as it fixes an actual problem, even with the old
>> code, I think.
>
> Ok, I'm even more confused now.  Maybe I just need more coffee.
>
> Can you list what exactly I should do here?  Should I apply this patch?
> Revert some other patch?  Which one(s)?

Revert b56a39ac (return -EINVAL) and apply the patch from Jan. The
problem b56a39ac solved , is replaced by other logic in Jan's patch.

Kay

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Greg KH]

On Tue, Jun 26, 2012 at 02:34:49AM +0200, Kay Sievers wrote:
> On Tue, Jun 26, 2012 at 2:01 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jun 25, 2012 at 09:35:03PM +0200, Kay Sievers wrote:
> >> On Mon, Jun 25, 2012 at 9:20 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> > Ok, so I'm confused, you want me to apply this patch and then revert a
> >> > different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?
> >>
> >> b56a39ac conflicts and is replaced with that patch.
> >> 4a77a5a0 should stay, as it fixes an actual problem, even with the old
> >> code, I think.
> >
> > Ok, I'm even more confused now.  Maybe I just need more coffee.
> >
> > Can you list what exactly I should do here?  Should I apply this patch?
> > Revert some other patch?  Which one(s)?
> 
> Revert b56a39ac (return -EINVAL) and apply the patch from Jan. The
> problem b56a39ac solved , is replaced by other logic in Jan's patch.

Ok, now done, there's nothing else needed here, right?

thanks,

greg k-h

Re: [PATCH] syslog: fill buffer with more than a single message for SYSLOG_ACTION_READ

[Jan Beulich]

>>> On 26.06.12 at 21:55, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jun 26, 2012 at 02:34:49AM +0200, Kay Sievers wrote:
>> On Tue, Jun 26, 2012 at 2:01 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Jun 25, 2012 at 09:35:03PM +0200, Kay Sievers wrote:
>> >> On Mon, Jun 25, 2012 at 9:20 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >>
>> >> > Ok, so I'm confused, you want me to apply this patch and then revert a
>> >> > different one?  Which one, 4a77a5a06ec66ed05199b301e7c25f42f979afdc?
>> >>
>> >> b56a39ac conflicts and is replaced with that patch.
>> >> 4a77a5a0 should stay, as it fixes an actual problem, even with the old
>> >> code, I think.
>> >
>> > Ok, I'm even more confused now.  Maybe I just need more coffee.
>> >
>> > Can you list what exactly I should do here?  Should I apply this patch?
>> > Revert some other patch?  Which one(s)?
>> 
>> Revert b56a39ac (return -EINVAL) and apply the patch from Jan. The
>> problem b56a39ac solved , is replaced by other logic in Jan's patch.
> 
> Ok, now done, there's nothing else needed here, right?

Thanks - not that I'm aware of.

Jan