| Message ID | 20250217194421.601813-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/console: Fix truncation of panic() messages | expand |
On 2/17/25 8:44 PM, Andrew Cooper wrote: > The panic() function uses a static buffer to format its arguments into, simply > to emit the result via printk("%s", buf). This buffer is not large enough for > some existing users in Xen. e.g.: > > (XEN) **************************************** > (XEN) Panic on CPU 0: > (XEN) Invalid device tree blob at physical address 0x46a00000. > (XEN) The DTB must be 8-byte aligned and must not exceed 2 MB in size. > (XEN) > (XEN) Plea**************************************** > > The remainder of this particular message is 'e check your bootloader.', but > has been inherited by RISC-V from ARM. > > It is also pointless double buffering. Implement vprintk() beside printk(), > and use it directly rather than rendering into a local buffer, removing it as > one source of message limitation. > > This marginally simplifies panic(), and drops a global used-once buffer. > > Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich<jbeulich@suse.com> Release-Acked-By: Oleksii Kurochko<oleksii.kurochko@gmail.com> Thanks. ~ Oleksii > --- > CC: Jan Beulich<JBeulich@suse.com> > CC: Roger Pau Monné<roger.pau@citrix.com> > CC: Stefano Stabellini<sstabellini@kernel.org> > CC: Julien Grall<julien@xen.org> > CC: Volodymyr Babchuk<Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis<bertrand.marquis@arm.com> > CC: Michal Orzel<michal.orzel@amd.com> > CC: Ayan Kumar Halder<ayan.kumar.halder@amd.com> > CC: Oleksii Kurochko<oleksii.kurochko@gmail.com> > > This taken from a security series (hence partially reviewed already), and > thought it only applied to a forthcoming selftest, but then Ayan posted a > truncated example to Matrix. > > Therefore this needs backporting, and might be wanted for 4.20 at this point. > It's low risk and easy to test. > --- > xen/drivers/char/console.c | 21 +++++++++++++-------- > xen/include/xen/lib.h | 2 ++ > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 7da8c5296f3b..2926c97df9a4 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -962,11 +962,17 @@ static void vprintk_common(const char *prefix, const char *fmt, va_list args) > local_irq_restore(flags); > } > > +void vprintk(const char *fmt, va_list args) > +{ > + vprintk_common("(XEN) ", fmt, args); > +} > + > void printk(const char *fmt, ...) > { > va_list args; > + > va_start(args, fmt); > - vprintk_common("(XEN)", fmt, args); + vprintk(fmt, args); va_end(args); } @@ -1268,23 > +1274,22 @@ void panic(const char *fmt, ...) va_list args; unsigned > long flags; static DEFINE_SPINLOCK(lock); - static char buf[128]; > spin_debug_disable(); spinlock_profile_printall('\0'); > debugtrace_dump(); - /* Protects buf[] and ensure multi-line message > prints atomically. */ + /* Ensure multi-line message prints > atomically. */ spin_lock_irqsave(&lock, flags); - va_start(args, fmt); > - (void)vsnprintf(buf, sizeof(buf), fmt, args); - va_end(args); - > console_start_sync(); printk("\n****************************************\n"); > printk("Panic on CPU %d:\n", smp_processor_id()); > - printk("%s", buf); > + > + va_start(args, fmt); > + vprintk(fmt, args); > + va_end(args); > + > printk("****************************************\n\n"); > if ( opt_noreboot ) > printk("Manual reset required ('noreboot' specified)\n"); > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h > index 81b722ea3e80..130f96791f75 100644 > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -60,6 +60,8 @@ debugtrace_printk(const char *fmt, ...) {} > > extern void printk(const char *fmt, ...) > __attribute__ ((format (printf, 1, 2), cold)); > +void vprintk(const char *fmt, va_list args) > + __attribute__ ((format (printf, 1, 0), cold)); > > #define printk_once(fmt, args...) \ > ({ \
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 7da8c5296f3b..2926c97df9a4 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -962,11 +962,17 @@ static void vprintk_common(const char *prefix, const char *fmt, va_list args) local_irq_restore(flags); } +void vprintk(const char *fmt, va_list args) +{ + vprintk_common("(XEN) ", fmt, args); +} + void printk(const char *fmt, ...) { va_list args; + va_start(args, fmt); - vprintk_common("(XEN) ", fmt, args); + vprintk(fmt, args); va_end(args); } @@ -1268,23 +1274,22 @@ void panic(const char *fmt, ...) va_list args; unsigned long flags; static DEFINE_SPINLOCK(lock); - static char buf[128]; spin_debug_disable(); spinlock_profile_printall('\0'); debugtrace_dump(); - /* Protects buf[] and ensure multi-line message prints atomically. */ + /* Ensure multi-line message prints atomically. */ spin_lock_irqsave(&lock, flags); - va_start(args, fmt); - (void)vsnprintf(buf, sizeof(buf), fmt, args); - va_end(args); - console_start_sync(); printk("\n****************************************\n"); printk("Panic on CPU %d:\n", smp_processor_id()); - printk("%s", buf); + + va_start(args, fmt); + vprintk(fmt, args); + va_end(args); + printk("****************************************\n\n"); if ( opt_noreboot ) printk("Manual reset required ('noreboot' specified)\n"); diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 81b722ea3e80..130f96791f75 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -60,6 +60,8 @@ debugtrace_printk(const char *fmt, ...) {} extern void printk(const char *fmt, ...) __attribute__ ((format (printf, 1, 2), cold)); +void vprintk(const char *fmt, va_list args) + __attribute__ ((format (printf, 1, 0), cold)); #define printk_once(fmt, args...) \ ({ \
The panic() function uses a static buffer to format its arguments into, simply to emit the result via printk("%s", buf). This buffer is not large enough for some existing users in Xen. e.g.: (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) Invalid device tree blob at physical address 0x46a00000. (XEN) The DTB must be 8-byte aligned and must not exceed 2 MB in size. (XEN) (XEN) Plea**************************************** The remainder of this particular message is 'e check your bootloader.', but has been inherited by RISC-V from ARM. It is also pointless double buffering. Implement vprintk() beside printk(), and use it directly rather than rendering into a local buffer, removing it as one source of message limitation. This marginally simplifies panic(), and drops a global used-once buffer. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Ayan Kumar Halder <ayan.kumar.halder@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> This taken from a security series (hence partially reviewed already), and thought it only applied to a forthcoming selftest, but then Ayan posted a truncated example to Matrix. Therefore this needs backporting, and might be wanted for 4.20 at this point. It's low risk and easy to test. --- xen/drivers/char/console.c | 21 +++++++++++++-------- xen/include/xen/lib.h | 2 ++ 2 files changed, 15 insertions(+), 8 deletions(-)