* [PATCH] pstore/ram: fix for adding dumps to non-empty zone @ 2019-12-23 13:38 Nikolai Merinov 2019-12-30 19:02 ` Kees Cook 2019-12-30 20:37 ` Kees Cook 0 siblings, 2 replies; 5+ messages in thread From: Nikolai Merinov @ 2019-12-23 13:38 UTC (permalink / raw) To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, linux-kernel Cc: Aleksandr Yashkin, Nikolay Merinov, Ariel Gilman 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone 2019-12-23 13:38 [PATCH] pstore/ram: fix for adding dumps to non-empty zone Nikolai Merinov @ 2019-12-30 19:02 ` Kees Cook 2019-12-30 20:37 ` Kees Cook 1 sibling, 0 replies; 5+ messages in thread From: Kees Cook @ 2019-12-30 19:02 UTC (permalink / raw) To: Nikolai Merinov Cc: Anton Vorontsov, Colin Cross, Tony Luck, linux-kernel, Aleksandr Yashkin, Ariel Gilman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone 2019-12-23 13:38 [PATCH] pstore/ram: fix for adding dumps to non-empty zone Nikolai Merinov 2019-12-30 19:02 ` Kees Cook @ 2019-12-30 20:37 ` Kees Cook 2019-12-31 8:00 ` Nikolai Merinov 1 sibling, 1 reply; 5+ messages in thread From: Kees Cook @ 2019-12-30 20:37 UTC (permalink / raw) To: Nikolai Merinov Cc: Anton Vorontsov, Colin Cross, Tony Luck, linux-kernel, Aleksandr Yashkin, Ariel Gilman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone 2019-12-30 20:37 ` Kees Cook @ 2019-12-31 8:00 ` Nikolai Merinov 2020-01-02 22:05 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Nikolai Merinov @ 2019-12-31 8:00 UTC (permalink / raw) To: Kees Cook Cc: Anton Vorontsov, Colin Cross, Tony Luck, linux-kernel, Aleksandr Yashkin, Ariel Gilman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pstore/ram: fix for adding dumps to non-empty zone 2019-12-31 8:00 ` Nikolai Merinov @ 2020-01-02 22:05 ` Kees Cook 0 siblings, 0 replies; 5+ messages in thread From: Kees Cook @ 2020-01-02 22:05 UTC (permalink / raw) To: Nikolai Merinov Cc: Anton Vorontsov, Colin Cross, Tony Luck, linux-kernel, Aleksandr Yashkin, Ariel Gilman 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-02 22:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-23 13:38 [PATCH] pstore/ram: fix for adding dumps to non-empty zone Nikolai Merinov 2019-12-30 19:02 ` Kees Cook 2019-12-30 20:37 ` Kees Cook 2019-12-31 8:00 ` Nikolai Merinov 2020-01-02 22:05 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).