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@xxxxxxxxxxxx wrote:
> On Mon, Dec 23, 2019 at 06:38:16PM +0500, Nikolai Merinov wrote:
>> From: Aleksandr Yashkin <a.yashkin@xxxxxxxxxxxxxxxxxx>
>> 
>> 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@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Nikolay Merinov <n.merinov@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Ariel Gilman <a.gilman@xxxxxxxxxxxxxxxxxx>
>> 
>> 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