On Wed 2020-02-05 16:48:32, John Ogness wrote:
> On 2020-02-05, Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx> wrote:
> > 3BUG: KASAN: wild-memory-access in copy_data+0x129/0x220>
> > 3Write of size 4 at addr 5a5a5a5a5a5a5a5a by task cat/474>
>
> The problem was due to an uninitialized pointer.
>
> Very recently the ringbuffer API was expanded so that it could
> optionally count lines in a record. This made it possible for me to
> implement record_print_text_inline(), which can do all the kmsg_dump
> multi-line madness without requiring a temporary buffer. Rather than
> passing an extra argument around for the optional line count, I added
> the text_line_count pointer to the printk_record struct. And since line
> counting is rarely needed, it is only performed if text_line_count is
> non-NULL.
>
> I oversaw that devkmsg_open() setup a printk_record and so I did not see
> to add the extra NULL initialization of text_line_count. There should be
> be an initializer function/macro to avoid this danger.
>
> John Ogness
>
> The quick fixup:
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index d0d24ee1d1f4..5ad67ff60cd9 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -883,6 +883,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> user->record.text_buf_size = sizeof(user->text_buf);
> user->record.dict_buf = &user->dict_buf[0];
> user->record.dict_buf_size = sizeof(user->dict_buf);
> + user->record.text_line_count = NULL;
The NULL pointer hidden in the structure also complicates the code
reading. It is less obvious when the same function is called
only to get the size/count and when real data.
I played with it and created extra function to get this information.
In addition, I had problems to follow the code in
record_print_text_inline(). So I tried to reuse the new function
and the existing record_printk_text() there.
Please, find below a patch that I ended with. I booted a system
with this patch. But I guess that I actually did not use the
record_print_text_inline(). So, it might be buggy.
Anyway, I wonder what you think about it:
>From 383e608f41a2f44898e4cd0751c5ccc18c82f71e Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@xxxxxxxx>
Date: Fri, 14 Feb 2020 16:14:18 +0100
Subject: [PATCH] printk: Alternative approach for inline dumping
line_count in struct printk_record looks a bit error prone. It causes
a system crash when people forget to initialize it. It seems better
to read this information via a separate API, for example,
prg_read_valid_info().
record_print_text_inline() is really complicated[*]. It is yet
another variant of the tricky logic used in record_print_text().
It would be great to actually reuse the existing function.
[*] I know that you created it on my request.
Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
---
kernel/printk/printk.c | 134 +++++++++++++-------------------------
kernel/printk/printk_ringbuffer.c | 55 +++++++++-------
kernel/printk/printk_ringbuffer.h | 7 +-
3 files changed, 84 insertions(+), 112 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5ad67ff60cd9..6b7d6716b178 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -883,7 +883,6 @@ static int devkmsg_open(struct inode *inode, struct file *file)
user->record.text_buf_size = sizeof(user->text_buf);
user->record.dict_buf = &user->dict_buf[0];
user->record.dict_buf_size = sizeof(user->dict_buf);
- user->record.text_line_count = NULL;
logbuf_lock_irq();
user->seq = prb_first_seq(prb);
@@ -1283,87 +1282,50 @@ static size_t record_print_text(const struct printk_record *r, bool syslog,
return len;
}
-static size_t record_print_text_inline(struct printk_record *r, bool syslog,
- bool time)
+static size_t
+get_record_text_size(struct printk_info *info, unsigned int line_count,
+ bool syslog, bool time)
{
- size_t text_len = r->info->text_len;
- size_t buf_size = r->text_buf_size;
- char *text = r->text_buf;
- char prefix[PREFIX_MAX];
- bool truncated = false;
size_t prefix_len;
- size_t len = 0;
- prefix_len = info_print_prefix(r->info, syslog, time, prefix);
-
- if (!text) {
- /* SYSLOG_ACTION_* buffer size only calculation */
- unsigned int line_count = 1;
-
- if (r->text_line_count)
- line_count = *(r->text_line_count);
- /*
- * Each line will be preceded with a prefix. The intermediate
- * newlines are already within the text, but a final trailing
- * newline will be added.
- */
- return ((prefix_len * line_count) + r->info->text_len + 1);
- }
+ prefix_len = info_print_prefix(info, syslog, time, NULL);
/*
- * Add the prefix for each line by shifting the rest of the text to
- * make room for the prefix. If the buffer is not large enough for all
- * the prefixes, then drop the trailing text and report the largest
- * length that includes full lines with their prefixes.
+ * Each line will be preceded with a prefix. The intermediate
+ * newlines are already within the text, but a final trailing
+ * newline will be added.
*/
- while (text_len) {
- size_t line_len;
- char *next;
-
- next = memchr(text, '\n', text_len);
- if (next) {
- line_len = next - text;
- } else {
- /*
- * If the text has been truncated, assume this line
- * was truncated and do not include this text.
- */
- if (truncated)
- break;
- line_len = text_len;
- }
+ return ((prefix_len * line_count) + info->text_len + 1);
+}
- /*
- * Is there enough buffer available to shift this line
- * (and add a newline at the end)?
- */
- if (len + prefix_len + line_len >= buf_size)
- break;
+static size_t record_print_text_inline(struct printk_record *r, bool syslog,
+ bool time)
+{
+ size_t text_len = r->info->text_len;
+ size_t text_buf_size = r->text_buf_size;
+ struct printk_info *info = r->info;
+ size_t record_len;
+ char *text = r->text_buf;
+ char *text_moved;
+ unsigned int line_count;
+ size_t len = 0;
- /*
- * Is there enough buffer available to shift all remaining
- * text (and add a newline at the end)?
- */
- if (len + prefix_len + text_len >= buf_size) {
- text_len = (buf_size - len) - prefix_len;
- truncated = true;
- }
+ if (!text)
+ return 0;
- memmove(text + prefix_len, text, text_len);
- memcpy(text, prefix, prefix_len);
+ line_count = prb_count_lines(text, text_len);
+ record_len = get_record_text_size(info, line_count, syslog, time);
- text += prefix_len + line_len;
- text_len -= line_len;
+ if (text_buf_size < record_len)
+ return 0;
- if (text_len) {
- text_len--;
- text++;
- } else {
- *text = '\n';
- }
+ /* Make space for timestamps */
+ text_moved = text + (record_len - text_len);
+ memmove(text_moved, text, text_len);
- len += prefix_len + line_len + 1;
- }
+ r->text_buf = text_moved;
+ len = record_print_text(r, syslog, time, text, text_buf_size);
+ r->text_buf = text;
return len;
}
@@ -3167,13 +3129,15 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
goto out;
/* Count text lines instead of reading text? */
- if (!line)
- r.text_line_count = &line_count;
-
- if (!prb_read_valid(prb, dumper->cur_seq, &r))
- goto out;
-
- l = record_print_text_inline(&r, syslog, printk_time);
+ if (!line) {
+ if (!prb_read_valid_info(prb, dumper->cur_seq, &info, &line_count))
+ goto out;
+ l = get_record_text_size(&info, line_count, syslog, printk_time);
+ } else {
+ if (!prb_read_valid(prb, dumper->cur_seq, &r))
+ goto out;
+ l = record_print_text_inline(&r, syslog, printk_time);
+ }
dumper->cur_seq = r.info->seq + 1;
ret = true;
@@ -3241,7 +3205,8 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
/* initially, only count text lines */
struct printk_record r = {
.info = &info,
- .text_line_count = &line_count,
+ .text_buf = buf,
+ .text_buf_size = size,
};
unsigned long flags;
u64 seq;
@@ -3267,30 +3232,25 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
/* calculate length of entire buffer */
seq = dumper->cur_seq;
- while (prb_read_valid(prb, seq, &r)) {
+ while (prb_read_valid_info(prb, seq, &info, &line_count)) {
if (r.info->seq >= dumper->next_seq)
break;
- l += record_print_text_inline(&r, true, time);
+ l += get_record_text_size(&info, line_count, true, time);
seq = r.info->seq + 1;
}
/* move first record forward until length fits into the buffer */
seq = dumper->cur_seq;
- while (l >= size && prb_read_valid(prb, seq, &r)) {
+ while (l >= size && prb_read_valid_info(prb, seq, &info, &line_count)) {
if (r.info->seq >= dumper->next_seq)
break;
- l -= record_print_text_inline(&r, true, time);
+ l -= get_record_text_size(&info, line_count, true, time);
seq = r.info->seq + 1;
}
/* last message in next interation */
next_seq = seq;
- /* actually read data into the buffer now */
- r.text_buf = buf;
- r.text_buf_size = size;
- r.text_line_count = NULL;
-
l = 0;
while (prb_read_valid(prb, seq, &r)) {
if (r.info->seq >= dumper->next_seq)
diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 796257f226ee..69976a49f828 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -893,7 +893,6 @@ bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
r->dict_buf_size = 0;
r->info = &d->info;
- r->text_line_count = NULL;
/* Set default values for the sizes. */
d->info.text_len = r->text_buf_size;
@@ -1002,6 +1001,21 @@ static char *get_data(struct prb_data_ring *data_ring,
return &db->data[0];
}
+unsigned long prb_count_lines(char *text, unsigned int text_size)
+{
+ unsigned int line_count;
+ char *next;
+
+ line_count = 1;
+ while ((next = memchr(text, '\n', text_size)) != NULL) {
+ text_size -= (next - text);
+ text = next;
+ line_count++;
+ }
+
+ return line_count;
+}
+
/*
* Given @blk_lpos, copy an expected @len of data into the provided buffer.
* If @line_count is provided, count the number of lines in the data.
@@ -1034,21 +1048,8 @@ static bool copy_data(struct prb_data_ring *data_ring,
}
/* Caller interested in the line count? */
- if (line_count) {
- unsigned long next_size = data_size;
- char *next = data;
-
- *line_count = 0;
-
- while (next_size) {
- (*line_count)++;
- next = memchr(next, '\n', next_size);
- if (!next)
- break;
- next++;
- next_size = data_size - (next - data);
- }
- }
+ if (line_count)
+ *line_count = prb_count_lines(data, data_size);
/* Caller interested in the data content? */
if (!buf || !buf_size)
@@ -1094,7 +1095,7 @@ static int desc_read_committed(struct prb_desc_ring *desc_ring,
* See desc_read_committed() for error return values.
*/
static int prb_read(struct printk_ringbuffer *rb, u64 seq,
- struct printk_record *r)
+ struct printk_record *r, unsigned int *line_count)
{
struct prb_desc_ring *desc_ring = &rb->desc_ring;
struct prb_desc *rdesc = to_desc(desc_ring, seq);
@@ -1121,7 +1122,7 @@ static int prb_read(struct printk_ringbuffer *rb, u64 seq,
/* Copy text data. If it fails, this is a data-less descriptor. */
if (!copy_data(&rb->text_data_ring, &desc.text_blk_lpos,
desc.info.text_len, r->text_buf, r->text_buf_size,
- r->text_line_count)) {
+ line_count)) {
return -ENOENT;
}
@@ -1212,12 +1213,12 @@ EXPORT_SYMBOL(prb_first_seq);
* See the description of prb_read_valid() for details.
*/
bool _prb_read_valid(struct printk_ringbuffer *rb, u64 *seq,
- struct printk_record *r)
+ struct printk_record *r, unsigned int *line_count)
{
u64 tail_seq;
int err;
- while ((err = prb_read(rb, *seq, r))) {
+ while ((err = prb_read(rb, *seq, r, line_count))) {
tail_seq = prb_first_seq(rb);
if (*seq < tail_seq) {
@@ -1264,10 +1265,20 @@ bool _prb_read_valid(struct printk_ringbuffer *rb, u64 *seq,
bool prb_read_valid(struct printk_ringbuffer *rb, u64 seq,
struct printk_record *r)
{
- return _prb_read_valid(rb, &seq, r);
+ return _prb_read_valid(rb, &seq, r, NULL);
}
EXPORT_SYMBOL(prb_read_valid);
+bool prb_read_valid_info(struct printk_ringbuffer *rb, u64 seq,
+ struct printk_info *info, unsigned int *line_count)
+{
+ struct printk_record r = {
+ .info = info,
+ };
+
+ return _prb_read_valid(rb, &seq, &r, line_count);
+}
+
/**
* prb_next_seq() - Get the sequence number after the last available record.
*
@@ -1287,7 +1298,7 @@ u64 prb_next_seq(struct printk_ringbuffer *rb)
do {
/* Search forward from the oldest descriptor. */
- if (!_prb_read_valid(rb, &seq, NULL))
+ if (!_prb_read_valid(rb, &seq, NULL, NULL))
return seq;
seq++;
} while (seq);
diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
index 4dc428427e7f..005b000fdb5b 100644
--- a/kernel/printk/printk_ringbuffer.h
+++ b/kernel/printk/printk_ringbuffer.h
@@ -28,8 +28,6 @@ struct printk_info {
* the reader provides the @info, @text_buf, @dict_buf buffers. On success,
* the struct pointed to by @info will be filled and the char arrays pointed
* to by @text_buf and @dict_buf will be filled with text and dict data.
- * If @text_line_count is provided, the number of lines in @text_buf will
- * be counted.
*/
struct printk_record {
struct printk_info *info;
@@ -37,7 +35,6 @@ struct printk_record {
char *dict_buf;
unsigned int text_buf_size;
unsigned int dict_buf_size;
- unsigned int *text_line_count;
};
/* Specifies the position/span of a data block. */
@@ -288,6 +285,8 @@ struct printk_record name = { \
.dict_buf_size = buf_size, \
}
+unsigned long prb_count_lines(char *text, unsigned int text_size);
+
/* Writer Interface */
bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb,
@@ -304,6 +303,8 @@ unsigned int prb_record_text_space(struct prb_reserved_entry *e);
bool prb_read_valid(struct printk_ringbuffer *rb, u64 seq,
struct printk_record *r);
+bool prb_read_valid_info(struct printk_ringbuffer *rb, u64 seq,
+ struct printk_info *info, unsigned int *line_count);
u64 prb_first_seq(struct printk_ringbuffer *rb);
u64 prb_next_seq(struct printk_ringbuffer *rb);
--
2.16.4
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
