Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On Thu, Oct 27, 2016 at 05:52:51PM +0200, Petr Mladek wrote:
> +static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
> {
> - const char *buf = s->buffer + start;
> + unsigned char *c, *end;
> + bool header;
> +
> + c = start;
> + end = start + len;
> + header = true;
> +
> + /* Print line by line. */
> + while (c < end) {
> + if (*c == '\n') {
> + printk_nmi_flush_line(start, c - start + 1);
> + start = ++c;
> + header = true;
> + continue;
> + }
>
> - printk_nmi_flush_line(buf, (end - start) + 1);
> + /* Handle continuous lines or missing new line. */
> + if ((c + 1 < end) && printk_get_level(c)) {
> + if (header) {
> + c += 2;
> + continue;
> + }
> +
> + printk_nmi_flush_line(start, c - start);
> + start = c++;
> + header = true;
> + continue;
> + }
> +
> + header = false;
> + c++;
> + }
> +
> + /* Check if there was a partial line. Ignore pure header. */
> + if (start < end && !header) {
> + printk_nmi_flush_line(start, end - start);
> + printk_nmi_flush_line("\n", strlen("\n"));
Not introduced by this patch as it was in the original code and the
compiler is smart enough to replace strlen("\n") with 1, but still it
looks strange.
Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On Thu, Oct 27, 2016 at 05:52:51PM +0200, Petr Mladek wrote:
> +static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
> {
> - const char *buf = s->buffer + start;
> + unsigned char *c, *end;
> + bool header;
> +
> + c = start;
> + end = start + len;
> + header = true;
> +
> + /* Print line by line. */
> + while (c < end) {
> + if (*c == '\n') {
> + printk_nmi_flush_line(start, c - start + 1);
> + start = ++c;
> + header = true;
> + continue;
> + }
>
> - printk_nmi_flush_line(buf, (end - start) + 1);
> + /* Handle continuous lines or missing new line. */
> + if ((c + 1 < end) && printk_get_level(c)) {
> + if (header) {
> + c += 2;
> + continue;
> + }
> +
> + printk_nmi_flush_line(start, c - start);
> + start = c++;
> + header = true;
> + continue;
> + }
> +
> + header = false;
> + c++;
> + }
> +
> + /* Check if there was a partial line. Ignore pure header. */
> + if (start < end && !header) {
> + printk_nmi_flush_line(start, end - start);
> + printk_nmi_flush_line("\n", strlen("\n"));
Not introduced by this patch as it was in the original code and the
compiler is smart enough to replace strlen("\n") with 1, but still it
looks strange.
Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On (10/27/16 09:35), Joe Perches wrote:
[..]
> > - printk_nmi_flush_line(buf, (end - start) + 1);
> > + /* Handle continuous lines or missing new line. */
> > + if ((c + 1 < end) && printk_get_level(c)) {
> > + if (header) {
> > + c += 2;
>
> printk_skip_level
agree, printk_skip_level() probably would look better here.
other than that, looks good to me. nice that you found it, Petr!
Reviewed-by: Sergey Senozhatsky
-ss
Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On (10/27/16 09:35), Joe Perches wrote:
[..]
> > - printk_nmi_flush_line(buf, (end - start) + 1);
> > + /* Handle continuous lines or missing new line. */
> > + if ((c + 1 < end) && printk_get_level(c)) {
> > + if (header) {
> > + c += 2;
>
> printk_skip_level
agree, printk_skip_level() probably would look better here.
other than that, looks good to me. nice that you found it, Petr!
Reviewed-by: Sergey Senozhatsky
-ss
Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On Thu, 2016-10-27 at 17:52 +0200, Petr Mladek wrote:
> The commit 4bcc595ccd80decb4245 ("printk: reinstate KERN_CONT for printing
> continuation lines") added back KERN_CONT message header. As a result
> it might appear in the middle of the line when the parts are squashed
> via the temporary NMI buffer.
>
> A reasonable solution seems to be to split the text in the NNI temporary
> not only by newlines but also by the message headers.
>
> Another solution would be to filter out KERN_CONT when writing to
> the temporary buffer. But this would complicate the lockless handling.
> Also it would not solve problems with a missing newline that was there
> even before the KERN_CONT stuff.
I believe the proper solution there is add the missing EOL/newlines
where appropriate. There aren't many treewide. Maybe a couple dozen
vs the 250,000 messages with newlines.
> This patch moves the temporary buffer handling into separate function.
> I played with it and it seems that using the char pointers make the
> code easier to read.
>
> Also it moves handling of the s->len overflow into the paranoid check.
> And allows to recover from the disaster.
>
> Signed-off-by: Petr Mladek
> ---
> kernel/printk/nmi.c | 78
> +
> 1 file changed, 49 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index 16bab471c7e2..740c90efc65d 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -113,16 +113,49 @@ static void printk_nmi_flush_line(const char *text, int
> len)
>
> }
>
> -/*
> - * printk one line from the temporary buffer from @start index until
> - * and including the @end index.
> - */
> -static void printk_nmi_flush_seq_line(struct nmi_seq_buf *s,
> - int start, int end)
> +/* printk part of the temporary buffer line by line */
> +static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
const unsigned char *?
> {
> - const char *buf = s->buffer + start;
> + unsigned char *c, *end;
> + bool header;
> +
> + c = start;
> + end = start + len;
> + header = true;
> +
> + /* Print line by line. */
> + while (c < end) {
> + if (*c == '\n') {
> + printk_nmi_flush_line(start, c - start + 1);
> + start = ++c;
> + header = true;
> + continue;
> + }
>
> - printk_nmi_flush_line(buf, (end - start) + 1);
> + /* Handle continuous lines or missing new line. */
> + if ((c + 1 < end) && printk_get_level(c)) {
> + if (header) {
> + c += 2;
printk_skip_level
> + continue;
> + }
> +
> + printk_nmi_flush_line(start, c - start);
> + start = c++;
> + header = true;
> + continue;
> + }
> +
> + header = false;
> + c++;
> + }
> +
> + /* Check if there was a partial line. Ignore pure header. */
> + if (start < end && !header) {
> + printk_nmi_flush_line(start, end - start);
> + printk_nmi_flush_line("\n", strlen("\n"));
> + }
> +
> + return len;
> }
>
> /*
> @@ -135,8 +168,8 @@ static void __printk_nmi_flush(struct irq_work *work)
> __RAW_SPIN_LOCK_INITIALIZER(read_lock);
> struct nmi_seq_buf *s = container_of(work, struct nmi_seq_buf, work);
> unsigned long flags;
> - size_t len, size;
> - int i, last_i;
> + size_t len;
> + int i;
>
> /*
>* The lock has two functions. First, one reader has to flush all
> @@ -154,35 +187,22 @@ static void __printk_nmi_flush(struct irq_work *work)
> /*
>* This is just a paranoid check that nobody has manipulated
>* the buffer an unexpected way. If we printed something then
> - * @len must only increase.
> + * @len must only increase. Also it should never overflow the
> + * buffer size.
>*/
> - if (i && i >= len) {
> + if ((i && i >= len) || len > sizeof(s->buffer)) {
> const char *msg = "printk_nmi_flush: internal error\n";
>
> printk_nmi_flush_line(msg, strlen(msg));
> + len = 0;
> }
>
> if (!len)
> goto out; /* Someone else has already flushed the buffer. */
>
> - /* Make sure that data has been written up to the @len */
> + /* Make sure the data has been written up to the @len */
> smp_rmb();
> -
> - size = min(len, sizeof(s->buffer));
> - last_i = i;
> -
> - /* Print line by line. */
> - for (; i < size; i++) {
> - if (s->buffer[i] == '\n') {
> - printk_nmi_flush_seq_line(s, last_i, i);
> - last_i = i + 1;
> - }
> - }
> - /* Check
Re: [PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
On Thu, 2016-10-27 at 17:52 +0200, Petr Mladek wrote:
> The commit 4bcc595ccd80decb4245 ("printk: reinstate KERN_CONT for printing
> continuation lines") added back KERN_CONT message header. As a result
> it might appear in the middle of the line when the parts are squashed
> via the temporary NMI buffer.
>
> A reasonable solution seems to be to split the text in the NNI temporary
> not only by newlines but also by the message headers.
>
> Another solution would be to filter out KERN_CONT when writing to
> the temporary buffer. But this would complicate the lockless handling.
> Also it would not solve problems with a missing newline that was there
> even before the KERN_CONT stuff.
I believe the proper solution there is add the missing EOL/newlines
where appropriate. There aren't many treewide. Maybe a couple dozen
vs the 250,000 messages with newlines.
> This patch moves the temporary buffer handling into separate function.
> I played with it and it seems that using the char pointers make the
> code easier to read.
>
> Also it moves handling of the s->len overflow into the paranoid check.
> And allows to recover from the disaster.
>
> Signed-off-by: Petr Mladek
> ---
> kernel/printk/nmi.c | 78
> +
> 1 file changed, 49 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index 16bab471c7e2..740c90efc65d 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -113,16 +113,49 @@ static void printk_nmi_flush_line(const char *text, int
> len)
>
> }
>
> -/*
> - * printk one line from the temporary buffer from @start index until
> - * and including the @end index.
> - */
> -static void printk_nmi_flush_seq_line(struct nmi_seq_buf *s,
> - int start, int end)
> +/* printk part of the temporary buffer line by line */
> +static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
const unsigned char *?
> {
> - const char *buf = s->buffer + start;
> + unsigned char *c, *end;
> + bool header;
> +
> + c = start;
> + end = start + len;
> + header = true;
> +
> + /* Print line by line. */
> + while (c < end) {
> + if (*c == '\n') {
> + printk_nmi_flush_line(start, c - start + 1);
> + start = ++c;
> + header = true;
> + continue;
> + }
>
> - printk_nmi_flush_line(buf, (end - start) + 1);
> + /* Handle continuous lines or missing new line. */
> + if ((c + 1 < end) && printk_get_level(c)) {
> + if (header) {
> + c += 2;
printk_skip_level
> + continue;
> + }
> +
> + printk_nmi_flush_line(start, c - start);
> + start = c++;
> + header = true;
> + continue;
> + }
> +
> + header = false;
> + c++;
> + }
> +
> + /* Check if there was a partial line. Ignore pure header. */
> + if (start < end && !header) {
> + printk_nmi_flush_line(start, end - start);
> + printk_nmi_flush_line("\n", strlen("\n"));
> + }
> +
> + return len;
> }
>
> /*
> @@ -135,8 +168,8 @@ static void __printk_nmi_flush(struct irq_work *work)
> __RAW_SPIN_LOCK_INITIALIZER(read_lock);
> struct nmi_seq_buf *s = container_of(work, struct nmi_seq_buf, work);
> unsigned long flags;
> - size_t len, size;
> - int i, last_i;
> + size_t len;
> + int i;
>
> /*
>* The lock has two functions. First, one reader has to flush all
> @@ -154,35 +187,22 @@ static void __printk_nmi_flush(struct irq_work *work)
> /*
>* This is just a paranoid check that nobody has manipulated
>* the buffer an unexpected way. If we printed something then
> - * @len must only increase.
> + * @len must only increase. Also it should never overflow the
> + * buffer size.
>*/
> - if (i && i >= len) {
> + if ((i && i >= len) || len > sizeof(s->buffer)) {
> const char *msg = "printk_nmi_flush: internal error\n";
>
> printk_nmi_flush_line(msg, strlen(msg));
> + len = 0;
> }
>
> if (!len)
> goto out; /* Someone else has already flushed the buffer. */
>
> - /* Make sure that data has been written up to the @len */
> + /* Make sure the data has been written up to the @len */
> smp_rmb();
> -
> - size = min(len, sizeof(s->buffer));
> - last_i = i;
> -
> - /* Print line by line. */
> - for (; i < size; i++) {
> - if (s->buffer[i] == '\n') {
> - printk_nmi_flush_seq_line(s, last_i, i);
> - last_i = i + 1;
> - }
> - }
> - /* Check if there was a
[PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
The commit 4bcc595ccd80decb4245 ("printk: reinstate KERN_CONT for printing
continuation lines") added back KERN_CONT message header. As a result
it might appear in the middle of the line when the parts are squashed
via the temporary NMI buffer.
A reasonable solution seems to be to split the text in the NNI temporary
not only by newlines but also by the message headers.
Another solution would be to filter out KERN_CONT when writing to
the temporary buffer. But this would complicate the lockless handling.
Also it would not solve problems with a missing newline that was there
even before the KERN_CONT stuff.
This patch moves the temporary buffer handling into separate function.
I played with it and it seems that using the char pointers make the
code easier to read.
Also it moves handling of the s->len overflow into the paranoid check.
And allows to recover from the disaster.
Signed-off-by: Petr Mladek
---
kernel/printk/nmi.c | 78 +
1 file changed, 49 insertions(+), 29 deletions(-)
diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
index 16bab471c7e2..740c90efc65d 100644
--- a/kernel/printk/nmi.c
+++ b/kernel/printk/nmi.c
@@ -113,16 +113,49 @@ static void printk_nmi_flush_line(const char *text, int
len)
}
-/*
- * printk one line from the temporary buffer from @start index until
- * and including the @end index.
- */
-static void printk_nmi_flush_seq_line(struct nmi_seq_buf *s,
- int start, int end)
+/* printk part of the temporary buffer line by line */
+static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
{
- const char *buf = s->buffer + start;
+ unsigned char *c, *end;
+ bool header;
+
+ c = start;
+ end = start + len;
+ header = true;
+
+ /* Print line by line. */
+ while (c < end) {
+ if (*c == '\n') {
+ printk_nmi_flush_line(start, c - start + 1);
+ start = ++c;
+ header = true;
+ continue;
+ }
- printk_nmi_flush_line(buf, (end - start) + 1);
+ /* Handle continuous lines or missing new line. */
+ if ((c + 1 < end) && printk_get_level(c)) {
+ if (header) {
+ c += 2;
+ continue;
+ }
+
+ printk_nmi_flush_line(start, c - start);
+ start = c++;
+ header = true;
+ continue;
+ }
+
+ header = false;
+ c++;
+ }
+
+ /* Check if there was a partial line. Ignore pure header. */
+ if (start < end && !header) {
+ printk_nmi_flush_line(start, end - start);
+ printk_nmi_flush_line("\n", strlen("\n"));
+ }
+
+ return len;
}
/*
@@ -135,8 +168,8 @@ static void __printk_nmi_flush(struct irq_work *work)
__RAW_SPIN_LOCK_INITIALIZER(read_lock);
struct nmi_seq_buf *s = container_of(work, struct nmi_seq_buf, work);
unsigned long flags;
- size_t len, size;
- int i, last_i;
+ size_t len;
+ int i;
/*
* The lock has two functions. First, one reader has to flush all
@@ -154,35 +187,22 @@ static void __printk_nmi_flush(struct irq_work *work)
/*
* This is just a paranoid check that nobody has manipulated
* the buffer an unexpected way. If we printed something then
-* @len must only increase.
+* @len must only increase. Also it should never overflow the
+* buffer size.
*/
- if (i && i >= len) {
+ if ((i && i >= len) || len > sizeof(s->buffer)) {
const char *msg = "printk_nmi_flush: internal error\n";
printk_nmi_flush_line(msg, strlen(msg));
+ len = 0;
}
if (!len)
goto out; /* Someone else has already flushed the buffer. */
- /* Make sure that data has been written up to the @len */
+ /* Make sure the data has been written up to the @len */
smp_rmb();
-
- size = min(len, sizeof(s->buffer));
- last_i = i;
-
- /* Print line by line. */
- for (; i < size; i++) {
- if (s->buffer[i] == '\n') {
- printk_nmi_flush_seq_line(s, last_i, i);
- last_i = i + 1;
- }
- }
- /* Check if there was a partial line. */
- if (last_i < size) {
- printk_nmi_flush_seq_line(s, last_i, size - 1);
- printk_nmi_flush_line("\n", strlen("\n"));
- }
+ i += printk_nmi_flush_buffer(s->buffer + i, len - i);
/*
* Check that nothing has got added in the meantime and truncate
--
1.8.5.6
[PATCH 1/4] printk/NMI: Handle continuous lines and missing newline
The commit 4bcc595ccd80decb4245 ("printk: reinstate KERN_CONT for printing
continuation lines") added back KERN_CONT message header. As a result
it might appear in the middle of the line when the parts are squashed
via the temporary NMI buffer.
A reasonable solution seems to be to split the text in the NNI temporary
not only by newlines but also by the message headers.
Another solution would be to filter out KERN_CONT when writing to
the temporary buffer. But this would complicate the lockless handling.
Also it would not solve problems with a missing newline that was there
even before the KERN_CONT stuff.
This patch moves the temporary buffer handling into separate function.
I played with it and it seems that using the char pointers make the
code easier to read.
Also it moves handling of the s->len overflow into the paranoid check.
And allows to recover from the disaster.
Signed-off-by: Petr Mladek
---
kernel/printk/nmi.c | 78 +
1 file changed, 49 insertions(+), 29 deletions(-)
diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
index 16bab471c7e2..740c90efc65d 100644
--- a/kernel/printk/nmi.c
+++ b/kernel/printk/nmi.c
@@ -113,16 +113,49 @@ static void printk_nmi_flush_line(const char *text, int
len)
}
-/*
- * printk one line from the temporary buffer from @start index until
- * and including the @end index.
- */
-static void printk_nmi_flush_seq_line(struct nmi_seq_buf *s,
- int start, int end)
+/* printk part of the temporary buffer line by line */
+static int printk_nmi_flush_buffer(unsigned char *start, size_t len)
{
- const char *buf = s->buffer + start;
+ unsigned char *c, *end;
+ bool header;
+
+ c = start;
+ end = start + len;
+ header = true;
+
+ /* Print line by line. */
+ while (c < end) {
+ if (*c == '\n') {
+ printk_nmi_flush_line(start, c - start + 1);
+ start = ++c;
+ header = true;
+ continue;
+ }
- printk_nmi_flush_line(buf, (end - start) + 1);
+ /* Handle continuous lines or missing new line. */
+ if ((c + 1 < end) && printk_get_level(c)) {
+ if (header) {
+ c += 2;
+ continue;
+ }
+
+ printk_nmi_flush_line(start, c - start);
+ start = c++;
+ header = true;
+ continue;
+ }
+
+ header = false;
+ c++;
+ }
+
+ /* Check if there was a partial line. Ignore pure header. */
+ if (start < end && !header) {
+ printk_nmi_flush_line(start, end - start);
+ printk_nmi_flush_line("\n", strlen("\n"));
+ }
+
+ return len;
}
/*
@@ -135,8 +168,8 @@ static void __printk_nmi_flush(struct irq_work *work)
__RAW_SPIN_LOCK_INITIALIZER(read_lock);
struct nmi_seq_buf *s = container_of(work, struct nmi_seq_buf, work);
unsigned long flags;
- size_t len, size;
- int i, last_i;
+ size_t len;
+ int i;
/*
* The lock has two functions. First, one reader has to flush all
@@ -154,35 +187,22 @@ static void __printk_nmi_flush(struct irq_work *work)
/*
* This is just a paranoid check that nobody has manipulated
* the buffer an unexpected way. If we printed something then
-* @len must only increase.
+* @len must only increase. Also it should never overflow the
+* buffer size.
*/
- if (i && i >= len) {
+ if ((i && i >= len) || len > sizeof(s->buffer)) {
const char *msg = "printk_nmi_flush: internal error\n";
printk_nmi_flush_line(msg, strlen(msg));
+ len = 0;
}
if (!len)
goto out; /* Someone else has already flushed the buffer. */
- /* Make sure that data has been written up to the @len */
+ /* Make sure the data has been written up to the @len */
smp_rmb();
-
- size = min(len, sizeof(s->buffer));
- last_i = i;
-
- /* Print line by line. */
- for (; i < size; i++) {
- if (s->buffer[i] == '\n') {
- printk_nmi_flush_seq_line(s, last_i, i);
- last_i = i + 1;
- }
- }
- /* Check if there was a partial line. */
- if (last_i < size) {
- printk_nmi_flush_seq_line(s, last_i, size - 1);
- printk_nmi_flush_line("\n", strlen("\n"));
- }
+ i += printk_nmi_flush_buffer(s->buffer + i, len - i);
/*
* Check that nothing has got added in the meantime and truncate
--
1.8.5.6
