RE: [PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[Michael Kelley <mhklinux@outlook.com>]

From: Michael Kelley <mhklinux@outlook.com> Sent: Monday, December 29, 2025 7:04 PM
> 
> From: vdso@mailbox.org <vdso@mailbox.org> Sent: Sunday, December 28, 2025 8:12 PM
> >
> > > On 12/19/2025 8:08 AM  mhkelley58@gmail.com wrote:
> 
> [snip]
> 
> > > @@ -198,9 +199,9 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> > >  	 * be single-threaded.
> > >  	 */
> > >  	kmsg_dump_rewind(&iter);
> > > -	kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> > > -			     &bytes_written);
> > > -	if (!bytes_written)
> > > +	ret = kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> > > +				   &bytes_written);
> > > +	if (!ret || !bytes_written)
> > >  		return;
> > >  	/*
> > >  	 * P3 to contain the physical address of the panic page & P4 to
> >
> > The existing code
> >
> > 1. doesn't care about the return value from kmsg_dump_get_buffer.
> >    The return value wouldn't make the function return before, why does that
> >    need to change?
> 
> The existing code depends on the implementation of kmsg_dump_get_buffer()
> always setting bytes_written, even if it fails. That's atypical behavior, but it is
> what kmsg_dump_get_buffer() does -- except that if CONFIG_PRINTK=n, the
> stub kmsg_dump_get_buffer() does *not* do that. Testing the return value is
> the more typical pattern, and bytes_written should be used only if the return
> value indicates success. So that's why I proposed this change, instead of just
> initializing bytes_written to zero when it is defined. My proposed change
> makes the overall pattern more typical, and would work if the implementation
> of kmsg_dump_get_buffer() should ever change to not set bytes_written in
> some error case.
> 
> >
> > 2. returns early when there are no bytes written.
> >    I think it shouldn't as otherwise the crash control register isn't written to,
> >    and the panic isn't signalled to the host. Is there another path maybe that
> >    I'm not noticing?
> 
> You make an excellent point. I didn't even think about the possibility of the
> current logic being wrong. There is hyperv_report_panic(), but it is not called
> if hv_panic_page is allocated, in order to avoid duplicate reports. I agree that
> this code should go ahead and send the panic report even if there's no
> message data. And in that case the discussion about testing the return value
> from kmsg_dump_get_buffer() is moot.
> 
> I'll submit a new patch to change the behavior to send the panic report to
> the host even if the message length is zero. I did a quick test of that case,
> and it behaves like the case where HV_CRASH_CTL_CRASH_NOTIFY_MSG
> is not set, which is fine.
> 
> I'll submit a new version of the patch focused on submitting the panic
> report to the hypervisor even if the message size is zero. Avoiding the
> uninitialized bytes_written will fall out of that change.

FWIW, the plot thickens here a bit. In testing my new version of
the patch, it turns out that hv_kmsg_dump() is never called when
CONFIG_PRINTK is not set. In that case, kmsg_dump_register()
fails, and hv_kmsg_dump_register() sets hv_panic_page back
to NULL. So hyperv_report_panic() is used to report the panic
to Hyper-V with no guest message.

The original uninitialized variable report is actually bogus since
the non-stub kmsg_dump_get_buffer() always sets bytes_written,
even if it returns false. But it's still worth initializing bytes_written
to zero so the static checkers don't generate noise.

I'll take all this into account in my new version of the patch.

Michael

> 
> See a comment below in your suggested patch.
> 
> >
> > That said, would it make sense to you the patch be something similar to:
> >
> > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > index 0a3ab7efed46..20e4a9a13b32 100644
> > --- a/drivers/hv/hv_common.c
> > +++ b/drivers/hv/hv_common.c
> > @@ -188,6 +188,7 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> >  {
> >         struct kmsg_dump_iter iter;
> >         size_t bytes_written;
> > +       bool ret;
> >
> >         /* We are only interested in panics. */
> >         if (detail->reason != KMSG_DUMP_PANIC || !sysctl_record_panic_msg)
> > @@ -197,11 +198,16 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> >          * Write dump contents to the page. No need to synchronize; panic should
> >          * be single-threaded.
> >          */
> > +       bytes_written = 0;
> >         kmsg_dump_rewind(&iter);
> > -       kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> > +       ret = kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> >                              &bytes_written);
> 
> Ignoring the return value can be made explicit as:
> 
>  +       (void)kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
>                               &bytes_written);
> 
> Plus an appropriate comment. Then there's no need to introduce the "ret" local
> variable and the somewhat funky:
> 
> 	(void) ret;
> 
> Michael
> 
> > -       if (!bytes_written)
> > -               return;
> > +       /*
> > +        * Whether there is more data available or not, send what has been captured
> > +        * to the host. Ignore the return value.
> > +        */
> > +       (void) ret;
> > +
> >         /*
> >          * P3 to contain the physical address of the panic page & P4 to
> >          * contain the size of the panic data in that page. Rest of the
> > @@ -210,7 +216,7 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> >         hv_set_msr(HV_MSR_CRASH_P0, 0);
> >         hv_set_msr(HV_MSR_CRASH_P1, 0);
> >         hv_set_msr(HV_MSR_CRASH_P2, 0);
> > -       hv_set_msr(HV_MSR_CRASH_P3, virt_to_phys(hv_panic_page));
> > +       hv_set_msr(HV_MSR_CRASH_P3, bytes_written ? virt_to_phys(hv_panic_page) :
> NULL);
> >         hv_set_msr(HV_MSR_CRASH_P4, bytes_written);
> >
> >         /*
> >
> > --
> > Cheers,
> > Roman
> >
> > > --
> > > 2.25.1