* [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops
@ 2021-09-17 10:24 Walter Stoll
2021-09-17 11:36 ` John Ogness
0 siblings, 1 reply; 3+ messages in thread
From: Walter Stoll @ 2021-09-17 10:24 UTC (permalink / raw)
To: linux-rt-users
Effect observed
---------------
We want to store kernel oops logs on a NAND flash partition by means of the
MTD Oops/Panic console logger/driver. However, no log is generated.
We observed the effect with kernel version v5.4.138-rt62. However, we think
that the most recent kernel exhibits the same behavior because the structure of
the sources in question (see below) did not change.
Root cause
----------
Further investigations revealed, than the issue is due to the realtime adaption
of the kmsg_dump() function.
Non realtime version of kmsg_dump():
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/printk/printk.c?h=linux-5.4.y#n3149
Realtime version of kmsg_dump():
https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/kernel/printk/printk.c?h=v5.4-rt#n2904
In our case, the the mtd_panic_write() function returns with error when the
kernel panics. As a result, no log is created. Call chain:
... panic() -> kmsg_dump() -> mtdoops_do_dump() -> mtdoops_write()
mtd_panic_write()
Note that within the kmsg_dump() function, a locally create kmsg_dumper object
is passed to the dumper_local.dump(&dumper_local, reason) call. However the
callee mtdoops_do_dump() function expects this object being embedded in a
mtdoops_context object, see
https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/drivers/mtd/mtdoops.c?h=v5.4-rt#n272
This is obviously not the case, therefore the error. Note that this could even
led to a security issue.
Bug fix proposal
----------------
We fixed the bug locally by applying the patch below. Note that we did not
modify the kmsg_dump() function. Instead we modified the mtdoops_do_dump()
function which seems for us the cleaner way.
diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 774970bfcf85..317ecf47fce8 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -269,8 +269,7 @@ static void find_next_position(struct mtdoops_context *cxt)
static void mtdoops_do_dump(struct kmsg_dumper *dumper,
enum kmsg_dump_reason reason)
{
- struct mtdoops_context *cxt = container_of(dumper,
- struct mtdoops_context, dump);
+ struct mtdoops_context *cxt = &oops_cxt;
/* Only dump oopses if dump_oops is set */
if (reason == KMSG_DUMP_OOPS && !dump_oops)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops
2021-09-17 10:24 [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops Walter Stoll
@ 2021-09-17 11:36 ` John Ogness
2021-09-17 12:03 ` AW: " Walter Stoll
0 siblings, 1 reply; 3+ messages in thread
From: John Ogness @ 2021-09-17 11:36 UTC (permalink / raw)
To: Walter Stoll, linux-rt-users; +Cc: linux-kernel
Hello Walter,
On 2021-09-17, Walter Stoll <Walter.Stoll@duagon.com> wrote:
> Effect observed
> ---------------
>
> We want to store kernel oops logs on a NAND flash partition by means of the
> MTD Oops/Panic console logger/driver. However, no log is generated.
>
> We observed the effect with kernel version v5.4.138-rt62. However, we think
> that the most recent kernel exhibits the same behavior because the structure of
> the sources in question (see below) did not change.
This problem does not exist in v5.10-rt. The kmsg_dump API was changed
(later in mainline as well) to avoid this issue. For v5.10-rt the change
was buried in the all-in-one commit:
https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/commit/?h=v5.10-rt&id=9835363e31e75d3287a81c6fc1b22c5c2b43d277
And later. the API change for mainline (5.12):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9f3f02db98bbe678a8e57fe9432b196174744a3
> Root cause
> ----------
>
> Further investigations revealed, than the issue is due to the realtime adaption
> of the kmsg_dump() function.
>
> Non realtime version of kmsg_dump():
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/printk/printk.c?h=linux-5.4.y#n3149
>
> Realtime version of kmsg_dump():
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/kernel/printk/printk.c?h=v5.4-rt#n2904
>
> In our case, the the mtd_panic_write() function returns with error when the
> kernel panics. As a result, no log is created. Call chain:
> ... panic() -> kmsg_dump() -> mtdoops_do_dump() -> mtdoops_write()
> mtd_panic_write()
>
> Note that within the kmsg_dump() function, a locally create kmsg_dumper object
> is passed to the dumper_local.dump(&dumper_local, reason) call. However the
> callee mtdoops_do_dump() function expects this object being embedded in a
> mtdoops_context object, see
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/drivers/mtd/mtdoops.c?h=v5.4-rt#n272
>
> This is obviously not the case, therefore the error. Note that this could even
> led to a security issue.
>
>
> Bug fix proposal
> ----------------
>
> We fixed the bug locally by applying the patch below. Note that we did not
> modify the kmsg_dump() function. Instead we modified the mtdoops_do_dump()
> function which seems for us the cleaner way.
I agree that this is a better fix, rather than backporting the newer
API. For mtdoops, the only function registering a dumper is
mtdoops_notify_add() and it also directly references the global context
with:
struct mtdoops_context *cxt = &oops_cxt;
Reviewed-by: John Ogness <john.ogness@linutronix.de>
> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index 774970bfcf85..317ecf47fce8 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -269,8 +269,7 @@ static void find_next_position(struct mtdoops_context *cxt)
> static void mtdoops_do_dump(struct kmsg_dumper *dumper,
> enum kmsg_dump_reason reason)
> {
> - struct mtdoops_context *cxt = container_of(dumper,
> - struct mtdoops_context, dump);
> + struct mtdoops_context *cxt = &oops_cxt;
>
> /* Only dump oopses if dump_oops is set */
> if (reason == KMSG_DUMP_OOPS && !dump_oops)
Thanks for your work on this!
John Ogness
^ permalink raw reply [flat|nested] 3+ messages in thread
* AW: [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops
2021-09-17 11:36 ` John Ogness
@ 2021-09-17 12:03 ` Walter Stoll
0 siblings, 0 replies; 3+ messages in thread
From: Walter Stoll @ 2021-09-17 12:03 UTC (permalink / raw)
To: John Ogness, linux-rt-users; +Cc: linux-kernel
> -----Ursprüngliche Nachricht-----
> Von: John Ogness <john.ogness@linutronix.de>
> Gesendet: Freitag, 17. September 2021 13:36
> An: Walter Stoll <Walter.Stoll@duagon.com>; linux-rt-users@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Betreff: Re: [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops
>
> Hello Walter,
>
> On 2021-09-17, Walter Stoll <Walter.Stoll@duagon.com> wrote:
> > Effect observed
> > ---------------
> >
> > We want to store kernel oops logs on a NAND flash partition by means of the
> > MTD Oops/Panic console logger/driver. However, no log is generated.
> >
> > We observed the effect with kernel version v5.4.138-rt62. However, we think
> > that the most recent kernel exhibits the same behavior because the structure of
> > the sources in question (see below) did not change.
>
> This problem does not exist in v5.10-rt. The kmsg_dump API was changed
> (later in mainline as well) to avoid this issue. For v5.10-rt the change
> was buried in the all-in-one commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/commit/?h=v5.10-
> rt&id=9835363e31e75d3287a81c6fc1b22c5c2b43d277
>
> And later. the API change for mainline (5.12):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9f3f02db98b
> be678a8e57fe9432b196174744a3
>
Ok, thanks a lot for your response. I was not aware of this.
> > Root cause
> > ----------
> >
> > Further investigations revealed, than the issue is due to the realtime adaption
> > of the kmsg_dump() function.
> >
> > Non realtime version of kmsg_dump():
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/printk/printk
> .c?h=linux-5.4.y#n3149
> >
> > Realtime version of kmsg_dump():
> > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-
> rt.git/tree/kernel/printk/printk.c?h=v5.4-rt#n2904
> >
> > In our case, the the mtd_panic_write() function returns with error when the
> > kernel panics. As a result, no log is created. Call chain:
> > ... panic() -> kmsg_dump() -> mtdoops_do_dump() -> mtdoops_write()
> > mtd_panic_write()
> >
> > Note that within the kmsg_dump() function, a locally create kmsg_dumper object
> > is passed to the dumper_local.dump(&dumper_local, reason) call. However the
> > callee mtdoops_do_dump() function expects this object being embedded in a
> > mtdoops_context object, see
> > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-
> rt.git/tree/drivers/mtd/mtdoops.c?h=v5.4-rt#n272
> >
> > This is obviously not the case, therefore the error. Note that this could even
> > led to a security issue.
> >
> >
> > Bug fix proposal
> > ----------------
> >
> > We fixed the bug locally by applying the patch below. Note that we did not
> > modify the kmsg_dump() function. Instead we modified the mtdoops_do_dump()
> > function which seems for us the cleaner way.
>
> I agree that this is a better fix, rather than backporting the newer
> API. For mtdoops, the only function registering a dumper is
> mtdoops_notify_add() and it also directly references the global context
> with:
>
> struct mtdoops_context *cxt = &oops_cxt;
>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
>
> > diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> > index 774970bfcf85..317ecf47fce8 100644
> > --- a/drivers/mtd/mtdoops.c
> > +++ b/drivers/mtd/mtdoops.c
> > @@ -269,8 +269,7 @@ static void find_next_position(struct mtdoops_context *cxt)
> > static void mtdoops_do_dump(struct kmsg_dumper *dumper,
> > enum kmsg_dump_reason reason)
> > {
> > - struct mtdoops_context *cxt = container_of(dumper,
> > - struct mtdoops_context, dump);
> > + struct mtdoops_context *cxt = &oops_cxt;
> >
> > /* Only dump oopses if dump_oops is set */
> > if (reason == KMSG_DUMP_OOPS && !dump_oops)
>
> Thanks for your work on this!
>
> John Ogness
Regards
Walter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-17 12:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17 10:24 [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops Walter Stoll
2021-09-17 11:36 ` John Ogness
2021-09-17 12:03 ` AW: " Walter Stoll
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).