[PATCH] pstore/ram: fix for adding dumps to non-empty zone

[PATCH] pstore/ram: fix for adding dumps to non-empty zone

[Nikolai Merinov]

From: Aleksandr Yashkin <a.yashkin@inango-systems.com>

The circle buffer in ramoops zones has a problem for adding a new
oops dump to already an existing one.

The solution to this problem is to reset the circle buffer state before
writing a new oops dump.

Signed-off-by: Aleksandr Yashkin <a.yashkin@inango-systems.com>
Signed-off-by: Nikolay Merinov <n.merinov@inango-systems.com>
Signed-off-by: Ariel Gilman <a.gilman@inango-systems.com>

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 8caff834f002..33fceadbf515 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -407,6 +407,13 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
 
 	prz = cxt->dprzs[cxt->dump_write_cnt];
 
+	/* Clean the buffer from old info.
+	 * `ramoops_read_kmsg_hdr' expects to find a header in the beginning of
+	 * buffer data, so we must to reset the buffer values, in order to
+	 * ensure that the header will be written to the beginning of the buffer
+	 */
+	persistent_ram_zap(prz);
+
 	/* Build header and append record contents. */
 	hlen = ramoops_write_kmsg_hdr(prz, record);
 	if (!hlen)
-- 
2.17.1

Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone

[Kees Cook]

On Mon, Dec 23, 2019 at 06:38:16PM +0500, Nikolai Merinov wrote:
> From: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> 
> The circle buffer in ramoops zones has a problem for adding a new
> oops dump to already an existing one.
> 
> The solution to this problem is to reset the circle buffer state before
> writing a new oops dump.

Can you describe the real-world problem you're fixing here? Dumps should
be appended as new records. Are you saying that once the circular list
is full, it blocks future dumps?

-Kees

> 
> Signed-off-by: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> Signed-off-by: Nikolay Merinov <n.merinov@inango-systems.com>
> Signed-off-by: Ariel Gilman <a.gilman@inango-systems.com>
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 8caff834f002..33fceadbf515 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -407,6 +407,13 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
>  
>  	prz = cxt->dprzs[cxt->dump_write_cnt];
>  
> +	/* Clean the buffer from old info.
> +	 * `ramoops_read_kmsg_hdr' expects to find a header in the beginning of
> +	 * buffer data, so we must to reset the buffer values, in order to
> +	 * ensure that the header will be written to the beginning of the buffer
> +	 */
> +	persistent_ram_zap(prz);
> +
>  	/* Build header and append record contents. */
>  	hlen = ramoops_write_kmsg_hdr(prz, record);
>  	if (!hlen)
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone

[Kees Cook]

On Mon, Dec 23, 2019 at 06:38:16PM +0500, Nikolai Merinov wrote:
> From: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> 
> The circle buffer in ramoops zones has a problem for adding a new
> oops dump to already an existing one.
> 
> The solution to this problem is to reset the circle buffer state before
> writing a new oops dump.

Ah, I see it now. When the crashes wrap around, the header is written at
the end of the (possibly incompletely filled) buffer, instead of at the
start, since it wasn't explicitly zapped.

Yes, this is important; thank you for tracking this down! This bug has
existed for a very long time. I'll try to find the right Fixes tag for
it...

-Kees

> Signed-off-by: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> Signed-off-by: Nikolay Merinov <n.merinov@inango-systems.com>
> Signed-off-by: Ariel Gilman <a.gilman@inango-systems.com>
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 8caff834f002..33fceadbf515 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -407,6 +407,13 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
>  
>  	prz = cxt->dprzs[cxt->dump_write_cnt];
>  
> +	/* Clean the buffer from old info.
> +	 * `ramoops_read_kmsg_hdr' expects to find a header in the beginning of
> +	 * buffer data, so we must to reset the buffer values, in order to
> +	 * ensure that the header will be written to the beginning of the buffer
> +	 */
> +	persistent_ram_zap(prz);
> +
>  	/* Build header and append record contents. */
>  	hlen = ramoops_write_kmsg_hdr(prz, record);
>  	if (!hlen)
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone

[Nikolai Merinov]

On Dec 31, 2019, at 1:37 AM, Kees Cook keescook@chromium.org wrote:
> On Mon, Dec 23, 2019 at 06:38:16PM +0500, Nikolai Merinov wrote:
>> From: Aleksandr Yashkin <a.yashkin@inango-systems.com>
>> 
>> The circle buffer in ramoops zones has a problem for adding a new
>> oops dump to already an existing one.
>> 
>> The solution to this problem is to reset the circle buffer state before
>> writing a new oops dump.
> 
> Ah, I see it now. When the crashes wrap around, the header is written at
> the end of the (possibly incompletely filled) buffer, instead of at the
> start, since it wasn't explicitly zapped.

Yes, you are right. We observed this issue when we got two consecutive
reboots with a kernel panic: After the first reboot the pstore was able
to parse the saved data, but after the second we got a part of the
previous compressed message and the new compressed message with the
ramoops message header at the middle of the file. 

According to our analysis the same issue can occur if we get more
than (rampoops.mem_size/ramoops.record_size) kernel oops during work.

> 
> Yes, this is important; thank you for tracking this down! This bug has
> existed for a very long time. I'll try to find the right Fixes tag for
> it...

We would like to see this changes in the LTS kernel releases. Could you
please point me the manual about propagation such fixes?

> 
> -Kees
> 
>> Signed-off-by: Aleksandr Yashkin <a.yashkin@inango-systems.com>
>> Signed-off-by: Nikolay Merinov <n.merinov@inango-systems.com>
>> Signed-off-by: Ariel Gilman <a.gilman@inango-systems.com>
>> 
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 8caff834f002..33fceadbf515 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -407,6 +407,13 @@ static int notrace ramoops_pstore_write(struct
>> pstore_record *record)
>>  
>>  	prz = cxt->dprzs[cxt->dump_write_cnt];
>>  
>> +	/* Clean the buffer from old info.
>> +	 * `ramoops_read_kmsg_hdr' expects to find a header in the beginning of
>> +	 * buffer data, so we must to reset the buffer values, in order to
>> +	 * ensure that the header will be written to the beginning of the buffer
>> +	 */
>> +	persistent_ram_zap(prz);
>> +
>>  	/* Build header and append record contents. */
>>  	hlen = ramoops_write_kmsg_hdr(prz, record);
>>  	if (!hlen)
>> --
>> 2.17.1
>> 
> 
> --
> Kees Cook
--
Nikolai Merinov

Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone

[Kees Cook]

On Tue, Dec 31, 2019 at 10:00:34AM +0200, Nikolai Merinov wrote:
> On Dec 31, 2019, at 1:37 AM, Kees Cook keescook@chromium.org wrote:
> > On Mon, Dec 23, 2019 at 06:38:16PM +0500, Nikolai Merinov wrote:
> >> From: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> >> 
> >> The circle buffer in ramoops zones has a problem for adding a new
> >> oops dump to already an existing one.
> >> 
> >> The solution to this problem is to reset the circle buffer state before
> >> writing a new oops dump.
> > 
> > Ah, I see it now. When the crashes wrap around, the header is written at
> > the end of the (possibly incompletely filled) buffer, instead of at the
> > start, since it wasn't explicitly zapped.
> 
> Yes, you are right. We observed this issue when we got two consecutive
> reboots with a kernel panic: After the first reboot the pstore was able
> to parse the saved data, but after the second we got a part of the
> previous compressed message and the new compressed message with the
> ramoops message header at the middle of the file. 
> 
> According to our analysis the same issue can occur if we get more
> than (rampoops.mem_size/ramoops.record_size) kernel oops during work.

Yup, perfect. Thanks for confirming! I've tested the patches with this
in mind and everything seems to be working correctly.

> 
> > 
> > Yes, this is important; thank you for tracking this down! This bug has
> > existed for a very long time. I'll try to find the right Fixes tag for
> > it...
> 
> We would like to see this changes in the LTS kernel releases. Could you
> please point me the manual about propagation such fixes?

You don't need to do anything. :) I have already marked it for -stable,
and it'll end up there as it makes its way through the development
process.

-Kees

> 
> > 
> > -Kees
> > 
> >> Signed-off-by: Aleksandr Yashkin <a.yashkin@inango-systems.com>
> >> Signed-off-by: Nikolay Merinov <n.merinov@inango-systems.com>
> >> Signed-off-by: Ariel Gilman <a.gilman@inango-systems.com>
> >> 
> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> >> index 8caff834f002..33fceadbf515 100644
> >> --- a/fs/pstore/ram.c
> >> +++ b/fs/pstore/ram.c
> >> @@ -407,6 +407,13 @@ static int notrace ramoops_pstore_write(struct
> >> pstore_record *record)
> >>  
> >>  	prz = cxt->dprzs[cxt->dump_write_cnt];
> >>  
> >> +	/* Clean the buffer from old info.
> >> +	 * `ramoops_read_kmsg_hdr' expects to find a header in the beginning of
> >> +	 * buffer data, so we must to reset the buffer values, in order to
> >> +	 * ensure that the header will be written to the beginning of the buffer
> >> +	 */
> >> +	persistent_ram_zap(prz);
> >> +
> >>  	/* Build header and append record contents. */
> >>  	hlen = ramoops_write_kmsg_hdr(prz, record);
> >>  	if (!hlen)
> >> --
> >> 2.17.1
> >> 
> > 
> > --
> > Kees Cook
> --
> Nikolai Merinov

-- 
Kees Cook