> -----Ursprüngliche Nachricht----- > Von: John Ogness <john.ogness@xxxxxxxxxxxxx> > Gesendet: Freitag, 17. September 2021 13:36 > An: Walter Stoll <Walter.Stoll@xxxxxxxxxx>; linux-rt-users@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > Betreff: Re: [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops > > Hello Walter, > > On 2021-09-17, Walter Stoll <Walter.Stoll@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > 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
