| 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
| ||
|
Date: Tue, 26 Jun 2018 14:43:32 +0200
From: Petr Mladek <pmladek@...e.com>
To: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Namit Gupta <gupta.namit@...sung.com>,
linux-kernel@...r.kernel.org, pankaj.m@...sung.com,
a.sahrawat@...sung.com, himanshu.m@...sung.com
Subject: Re: [PATCH] printk: remove unnecessary kmalloc() from syslog during
clear
On Mon 2018-06-25 23:44:07, Sergey Senozhatsky wrote:
> On (06/25/18 10:37), Steven Rostedt wrote:
> >
> > Is IMHO rather ugly.
>
> Either way works for me. So I'll leave it to you and Petr to decide :)
>
> > And the original patch has one more advantage. If buf and clear are
> > both NULL/zero, we don't take any locks.
>
> But we never use syslog_print_all(buf = NULL, clear = false). It's either
> NULL/true [move forward clear idx, do not copy to user], or !NULL/use defined
> value [copy to user, move or don't move clear idx forward]
Yup, I suggest the following version as a compromise. It has the code
duplication but I agree that it is negligible. Otherwise, it looks
cleaner.
>From be6e161b3d75830e710c3ada56f5eeb615fae002 Mon Sep 17 00:00:00 2001
From: Namit Gupta <gupta.namit@...sung.com>
Date: Wed, 20 Jun 2018 19:26:19 +0530
Subject: [PATCH] printk: remove unnecessary kmalloc() from syslog during clear
When the request is only for clearing logs, there is no need for
allocation/deallocation. Only the indexes need to be reset and returned.
This patch implements this simple case separately. And syslog_print_all()
is always called with a valid buf and could use a more sane indentation.
Link: http://lkml.kernel.org/r/20180620135951epcas5p3bd2a8f25ec689ca333bce861b527dba2~54wyKcT0_3155531555epcas5p3y@epcas5p3.samsung.com
Cc: linux-kernel@...r.kernel.org
Cc: pankaj.m@...sung.com
Cc: a.sahrawat@...sung.com
Signed-off-by: Namit Gupta <gupta.namit@...sung.com>
Signed-off-by: Himanshu Maithani <himanshu.m@...sung.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Signed-off-by: Petr Mladek <pmladek@...e.com>
---
kernel/printk/printk.c | 111 ++++++++++++++++++++++++++-----------------------
1 file changed, 58 insertions(+), 53 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..588f23c9c14f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1352,71 +1352,68 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
{
char *text;
int len = 0;
+ u64 next_seq;
+ u64 seq;
+ u32 idx;
text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
if (!text)
return -ENOMEM;
logbuf_lock_irq();
- if (buf) {
- u64 next_seq;
- u64 seq;
- u32 idx;
+ /*
+ * Find first record that fits, including all following records,
+ * into the user-provided buffer for this dump.
+ */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- /*
- * Find first record that fits, including all following records,
- * into the user-provided buffer for this dump.
- */
- seq = clear_seq;
- idx = clear_idx;
- while (seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
-
- len += msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ len += msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- /* move first record forward until length fits into the buffer */
- seq = clear_seq;
- idx = clear_idx;
- while (len > size && seq < log_next_seq) {
- struct printk_log *msg = log_from_idx(idx);
+ /* move first record forward until length fits into the buffer */
+ seq = clear_seq;
+ idx = clear_idx;
+ while (len > size && seq < log_next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
- len -= msg_print_text(msg, true, NULL, 0);
- idx = log_next(idx);
- seq++;
- }
+ len -= msg_print_text(msg, true, NULL, 0);
+ idx = log_next(idx);
+ seq++;
+ }
- /* last message fitting into this dump */
- next_seq = log_next_seq;
+ /* last message fitting into this dump */
+ next_seq = log_next_seq;
- len = 0;
- while (len >= 0 && seq < next_seq) {
- struct printk_log *msg = log_from_idx(idx);
- int textlen;
+ len = 0;
+ while (len >= 0 && seq < next_seq) {
+ struct printk_log *msg = log_from_idx(idx);
+ int textlen;
- textlen = msg_print_text(msg, true, text,
- LOG_LINE_MAX + PREFIX_MAX);
- if (textlen < 0) {
- len = textlen;
- break;
- }
- idx = log_next(idx);
- seq++;
+ textlen = msg_print_text(msg, true, text,
+ LOG_LINE_MAX + PREFIX_MAX);
+ if (textlen < 0) {
+ len = textlen;
+ break;
+ }
+ idx = log_next(idx);
+ seq++;
- logbuf_unlock_irq();
- if (copy_to_user(buf + len, text, textlen))
- len = -EFAULT;
- else
- len += textlen;
- logbuf_lock_irq();
-
- if (seq < log_first_seq) {
- /* messages are gone, move to next one */
- seq = log_first_seq;
- idx = log_first_idx;
- }
+ logbuf_unlock_irq();
+ if (copy_to_user(buf + len, text, textlen))
+ len = -EFAULT;
+ else
+ len += textlen;
+ logbuf_lock_irq();
+
+ if (seq < log_first_seq) {
+ /* messages are gone, move to next one */
+ seq = log_first_seq;
+ idx = log_first_idx;
}
}
@@ -1430,6 +1427,14 @@ static int syslog_print_all(char __user *buf, int size, bool clear)
return len;
}
+static void syslog_clear(void)
+{
+ logbuf_lock_irq();
+ clear_seq = log_next_seq;
+ clear_idx = log_next_idx;
+ logbuf_unlock_irq();
+}
+
int do_syslog(int type, char __user *buf, int len, int source)
{
bool clear = false;
@@ -1474,7 +1479,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
break;
/* Clear ring buffer */
case SYSLOG_ACTION_CLEAR:
- syslog_print_all(NULL, 0, true);
+ syslog_clear();
break;
/* Disable logging to console */
case SYSLOG_ACTION_CONSOLE_OFF:
--
2.13.7
Powered by blists - more mailing lists