Fixes coverity (CID 1642026)
Cc: Aditya Gupta <adityag@linux.ibm.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
---
hw/ppc/spapr_fadump.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index fa3aeac94c..883a60cdcf 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
qemu_log_mask(LOG_GUEST_ERROR,
"FADump: Failed allocating memory (size: %zu) for copying"
" reserved memory regions\n", FADUMP_CHUNK_SIZE);
+ return false;
}
num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
--
2.51.0
On 28/10/25 09:05, Shivang Upadhyay wrote: > Fixes coverity (CID 1642026) > > Cc: Aditya Gupta <adityag@linux.ibm.com> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> > --- > hw/ppc/spapr_fadump.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c > index fa3aeac94c..883a60cdcf 100644 > --- a/hw/ppc/spapr_fadump.c > +++ b/hw/ppc/spapr_fadump.c > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) > qemu_log_mask(LOG_GUEST_ERROR, FWIW host heap exhaustion is not really a *guest* error, because the guest can not control it. > "FADump: Failed allocating memory (size: %zu) for copying" > " reserved memory regions\n", FADUMP_CHUNK_SIZE); > + return false; > } > > num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote: > On 28/10/25 09:05, Shivang Upadhyay wrote: > > Fixes coverity (CID 1642026) > > > > Cc: Aditya Gupta <adityag@linux.ibm.com> > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> > > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> > > --- > > hw/ppc/spapr_fadump.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c > > index fa3aeac94c..883a60cdcf 100644 > > --- a/hw/ppc/spapr_fadump.c > > +++ b/hw/ppc/spapr_fadump.c > > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) > > qemu_log_mask(LOG_GUEST_ERROR, > > FWIW host heap exhaustion is not really a *guest* error, because the > guest can not control it. Hi, Philippe Thanks for the review. There are following log level defined in log.h .... #define CPU_LOG_TB_OUT_ASM (1u << 0) #define CPU_LOG_TB_IN_ASM (1u << 1) #define CPU_LOG_TB_OP (1u << 2) #define CPU_LOG_TB_OP_OPT (1u << 3) #define CPU_LOG_INT (1u << 4) #define CPU_LOG_EXEC (1u << 5) #define CPU_LOG_PCALL (1u << 6) #define CPU_LOG_TB_CPU (1u << 8) #define CPU_LOG_RESET (1u << 9) #define LOG_UNIMP (1u << 10) #define LOG_GUEST_ERROR (1u << 11) #define CPU_LOG_MMU (1u << 12) #define CPU_LOG_TB_NOCHAIN (1u << 13) #define CPU_LOG_PAGE (1u << 14) /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ #define CPU_LOG_TB_OP_IND (1u << 16) #define CPU_LOG_TB_FPU (1u << 17) #define CPU_LOG_PLUGIN (1u << 18) /* LOG_STRACE is used for user-mode strace logging. */ #define LOG_STRACE (1u << 19) #define LOG_PER_THREAD (1u << 20) #define CPU_LOG_TB_VPU (1u << 21) #define LOG_TB_OP_PLUGIN (1u << 22) #define LOG_INVALID_MEM (1u << 23) .... Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`, if that's more appropriate. Thanks ~Shivang. > > > "FADump: Failed allocating memory (size: %zu) for copying" > > " reserved memory regions\n", FADUMP_CHUNK_SIZE); > > + return false; > > } > > num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE); >
On 10/28/25 15:54, Shivang Upadhyay wrote: > On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote: >> On 28/10/25 09:05, Shivang Upadhyay wrote: >>> Fixes coverity (CID 1642026) >>> >>> Cc: Aditya Gupta <adityag@linux.ibm.com> >>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> >>> Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> >>> --- >>> hw/ppc/spapr_fadump.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c >>> index fa3aeac94c..883a60cdcf 100644 >>> --- a/hw/ppc/spapr_fadump.c >>> +++ b/hw/ppc/spapr_fadump.c >>> @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) >>> qemu_log_mask(LOG_GUEST_ERROR, >> >> FWIW host heap exhaustion is not really a *guest* error, because the >> guest can not control it. > Hi, Philippe > > > Thanks for the review. There are following log level defined in log.h > > .... > > #define CPU_LOG_TB_OUT_ASM (1u << 0) > #define CPU_LOG_TB_IN_ASM (1u << 1) > #define CPU_LOG_TB_OP (1u << 2) > #define CPU_LOG_TB_OP_OPT (1u << 3) > #define CPU_LOG_INT (1u << 4) > #define CPU_LOG_EXEC (1u << 5) > #define CPU_LOG_PCALL (1u << 6) > #define CPU_LOG_TB_CPU (1u << 8) > #define CPU_LOG_RESET (1u << 9) > #define LOG_UNIMP (1u << 10) > #define LOG_GUEST_ERROR (1u << 11) > #define CPU_LOG_MMU (1u << 12) > #define CPU_LOG_TB_NOCHAIN (1u << 13) > #define CPU_LOG_PAGE (1u << 14) > /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ > #define CPU_LOG_TB_OP_IND (1u << 16) > #define CPU_LOG_TB_FPU (1u << 17) > #define CPU_LOG_PLUGIN (1u << 18) > /* LOG_STRACE is used for user-mode strace logging. */ > #define LOG_STRACE (1u << 19) > #define LOG_PER_THREAD (1u << 20) > #define CPU_LOG_TB_VPU (1u << 21) > #define LOG_TB_OP_PLUGIN (1u << 22) > #define LOG_INVALID_MEM (1u << 23) > > .... > > Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`, > if that's more appropriate. I think it would be better to have LOG_INSUFF_MEM for this case, but let's hear from Philippe and others for suggestions. Since it's unlreated to the coverity fix and can be taken separately, so: Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> > > Thanks > ~Shivang. >> >>> "FADump: Failed allocating memory (size: %zu) for copying" >>> " reserved memory regions\n", FADUMP_CHUNK_SIZE); >>> + return false; >>> } >>> num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE); >>
On Thu, 30 Oct 2025, Harsh Prateek Bora wrote: > On 10/28/25 15:54, Shivang Upadhyay wrote: >> On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote: >>> On 28/10/25 09:05, Shivang Upadhyay wrote: >>>> Fixes coverity (CID 1642026) >>>> >>>> Cc: Aditya Gupta <adityag@linux.ibm.com> >>>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> >>>> Link: >>>> https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ >>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >>>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> >>>> --- >>>> hw/ppc/spapr_fadump.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c >>>> index fa3aeac94c..883a60cdcf 100644 >>>> --- a/hw/ppc/spapr_fadump.c >>>> +++ b/hw/ppc/spapr_fadump.c >>>> @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) >>>> qemu_log_mask(LOG_GUEST_ERROR, >>> >>> FWIW host heap exhaustion is not really a *guest* error, because the >>> guest can not control it. >> Hi, Philippe >> >> >> Thanks for the review. There are following log level defined in log.h >> >> .... >> >> #define CPU_LOG_TB_OUT_ASM (1u << 0) >> #define CPU_LOG_TB_IN_ASM (1u << 1) >> #define CPU_LOG_TB_OP (1u << 2) >> #define CPU_LOG_TB_OP_OPT (1u << 3) >> #define CPU_LOG_INT (1u << 4) >> #define CPU_LOG_EXEC (1u << 5) >> #define CPU_LOG_PCALL (1u << 6) >> #define CPU_LOG_TB_CPU (1u << 8) >> #define CPU_LOG_RESET (1u << 9) >> #define LOG_UNIMP (1u << 10) >> #define LOG_GUEST_ERROR (1u << 11) >> #define CPU_LOG_MMU (1u << 12) >> #define CPU_LOG_TB_NOCHAIN (1u << 13) >> #define CPU_LOG_PAGE (1u << 14) >> /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ >> #define CPU_LOG_TB_OP_IND (1u << 16) >> #define CPU_LOG_TB_FPU (1u << 17) >> #define CPU_LOG_PLUGIN (1u << 18) >> /* LOG_STRACE is used for user-mode strace logging. */ >> #define LOG_STRACE (1u << 19) >> #define LOG_PER_THREAD (1u << 20) >> #define CPU_LOG_TB_VPU (1u << 21) >> #define LOG_TB_OP_PLUGIN (1u << 22) >> #define LOG_INVALID_MEM (1u << 23) >> >> .... >> >> Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`, >> if that's more appropriate. > > I think it would be better to have LOG_INSUFF_MEM for this case, but let's > hear from Philippe and others for suggestions. If it's not a guest error but an error in QEMU then maybe error_report (or warn_report if it's recoverable)? Regards, BALATON Zoltan > Since it's unlreated to the coverity fix and can be taken separately, so: > > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> > >> >> Thanks >> ~Shivang. >>> >>>> "FADump: Failed allocating memory (size: %zu) for copying" >>>> " reserved memory regions\n", FADUMP_CHUNK_SIZE); >>>> + return false; >>>> } >>>> num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE); >>> > >
On Thu, Oct 30, 2025 at 12:09:44PM +0100, BALATON Zoltan wrote: > On Thu, 30 Oct 2025, Harsh Prateek Bora wrote: > > On 10/28/25 15:54, Shivang Upadhyay wrote: > > > On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote: > > > > On 28/10/25 09:05, Shivang Upadhyay wrote: > > > > > Fixes coverity (CID 1642026) > > > > > > > > > > Cc: Aditya Gupta <adityag@linux.ibm.com> > > > > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> > > > > > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ > > > > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > > > > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> > > > > > --- > > > > > hw/ppc/spapr_fadump.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c > > > > > index fa3aeac94c..883a60cdcf 100644 > > > > > --- a/hw/ppc/spapr_fadump.c > > > > > +++ b/hw/ppc/spapr_fadump.c > > > > > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) > > > > > qemu_log_mask(LOG_GUEST_ERROR, > > > > > > > > FWIW host heap exhaustion is not really a *guest* error, because the > > > > guest can not control it. > > > Hi, Philippe > > > > > > > > > Thanks for the review. There are following log level defined in log.h > > > > > > .... > > > > > > #define CPU_LOG_TB_OUT_ASM (1u << 0) > > > #define CPU_LOG_TB_IN_ASM (1u << 1) > > > #define CPU_LOG_TB_OP (1u << 2) > > > #define CPU_LOG_TB_OP_OPT (1u << 3) > > > #define CPU_LOG_INT (1u << 4) > > > #define CPU_LOG_EXEC (1u << 5) > > > #define CPU_LOG_PCALL (1u << 6) > > > #define CPU_LOG_TB_CPU (1u << 8) > > > #define CPU_LOG_RESET (1u << 9) > > > #define LOG_UNIMP (1u << 10) > > > #define LOG_GUEST_ERROR (1u << 11) > > > #define CPU_LOG_MMU (1u << 12) > > > #define CPU_LOG_TB_NOCHAIN (1u << 13) > > > #define CPU_LOG_PAGE (1u << 14) > > > /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ > > > #define CPU_LOG_TB_OP_IND (1u << 16) > > > #define CPU_LOG_TB_FPU (1u << 17) > > > #define CPU_LOG_PLUGIN (1u << 18) > > > /* LOG_STRACE is used for user-mode strace logging. */ > > > #define LOG_STRACE (1u << 19) > > > #define LOG_PER_THREAD (1u << 20) > > > #define CPU_LOG_TB_VPU (1u << 21) > > > #define LOG_TB_OP_PLUGIN (1u << 22) > > > #define LOG_INVALID_MEM (1u << 23) > > > > > > .... > > > > > > Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`, > > > if that's more appropriate. > > > > I think it would be better to have LOG_INSUFF_MEM for this case, but > > let's hear from Philippe and others for suggestions. > > If it's not a guest error but an error in QEMU then maybe error_report (or > warn_report if it's recoverable)? > > Regards, > BALATON Zoltan Hi This allocation failure does not seem recoverable. Maybe Aditya can be more right about this. Also I noticed a pattern to use `g_malloc` for critical things instead of `g_try_malloc`. But it will kill the full application if failure happens. So maybe just `error_report` is fine here(?). ~Shivang.
On Thu, 30 Oct 2025 at 14:23, Shivang Upadhyay <shivangu@linux.ibm.com> wrote: > Also I noticed a pattern to use `g_malloc` for critical things instead > of `g_try_malloc`. But it will kill the full application if failure happens. > So maybe just `error_report` is fine here(?). docs/devel/style.rst has some notes on malloc choices, including this: # Care should be taken to avoid introducing places where the guest could # trigger an exit by causing a large allocation. For small allocations, # of the order of 4k, a failure to allocate is likely indicative of an # overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable # approach. However for larger allocations where we could realistically # fall-back to a smaller one if need be we should use functions like # ``g_try_new`` and check the result. For example this is valid approach # for a time/space trade-off like ``tlb_mmu_resize_locked`` in the # SoftMMU TLB code. Since we're trying to allocate 32MB at once and this is during the guest run rather than at startup, this is probably a reasonable place to use g_try_malloc(). There are other places in this code that use LOG_GUEST_ERROR for things that aren't exactly guest errors, so my suggestion is that we take this patch as-is to fix the logic error. We can consider whether we want to try to improve the error reporting of this group of functions as a separate patch. thanks -- PMM
On Thu, Oct 30, 2025 at 02:45:07PM +0000, Peter Maydell wrote: > docs/devel/style.rst has some notes on malloc choices, including this: > > # Care should be taken to avoid introducing places where the guest could > # trigger an exit by causing a large allocation. For small allocations, > # of the order of 4k, a failure to allocate is likely indicative of an > # overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable > # approach. However for larger allocations where we could realistically > # fall-back to a smaller one if need be we should use functions like > # ``g_try_new`` and check the result. For example this is valid approach > # for a time/space trade-off like ``tlb_mmu_resize_locked`` in the > # SoftMMU TLB code. Hi Peter, Thanks for clearing it up. > > Since we're trying to allocate 32MB at once and this is during > the guest run rather than at startup, this is probably a reasonable > place to use g_try_malloc(). > > There are other places in this code that use LOG_GUEST_ERROR > for things that aren't exactly guest errors, so my suggestion > is that we take this patch as-is to fix the logic error. > We can consider whether we want to try to improve the error > reporting of this group of functions as a separate patch. Sure. ~Shivang.
On Thu, Oct 30, 2025 at 01:35:11PM +0530, Harsh Prateek Bora wrote: > > > On 10/28/25 15:54, Shivang Upadhyay wrote: > > On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote: > > > On 28/10/25 09:05, Shivang Upadhyay wrote: > > > > Fixes coverity (CID 1642026) > > > > > > > > Cc: Aditya Gupta <adityag@linux.ibm.com> > > > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com> > > > > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/ > > > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > > > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com> > > > > --- > > > > hw/ppc/spapr_fadump.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c > > > > index fa3aeac94c..883a60cdcf 100644 > > > > --- a/hw/ppc/spapr_fadump.c > > > > +++ b/hw/ppc/spapr_fadump.c > > > > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region) > > > > qemu_log_mask(LOG_GUEST_ERROR, > > > > > > FWIW host heap exhaustion is not really a *guest* error, because the > > > guest can not control it. > > Hi, Philippe > > > > > > Thanks for the review. There are following log level defined in log.h > > > > .... > > > > #define CPU_LOG_TB_OUT_ASM (1u << 0) > > #define CPU_LOG_TB_IN_ASM (1u << 1) > > #define CPU_LOG_TB_OP (1u << 2) > > #define CPU_LOG_TB_OP_OPT (1u << 3) > > #define CPU_LOG_INT (1u << 4) > > #define CPU_LOG_EXEC (1u << 5) > > #define CPU_LOG_PCALL (1u << 6) > > #define CPU_LOG_TB_CPU (1u << 8) > > #define CPU_LOG_RESET (1u << 9) > > #define LOG_UNIMP (1u << 10) > > #define LOG_GUEST_ERROR (1u << 11) > > #define CPU_LOG_MMU (1u << 12) > > #define CPU_LOG_TB_NOCHAIN (1u << 13) > > #define CPU_LOG_PAGE (1u << 14) > > /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ > > #define CPU_LOG_TB_OP_IND (1u << 16) > > #define CPU_LOG_TB_FPU (1u << 17) > > #define CPU_LOG_PLUGIN (1u << 18) > > /* LOG_STRACE is used for user-mode strace logging. */ > > #define LOG_STRACE (1u << 19) > > #define LOG_PER_THREAD (1u << 20) > > #define CPU_LOG_TB_VPU (1u << 21) > > #define LOG_TB_OP_PLUGIN (1u << 22) > > #define LOG_INVALID_MEM (1u << 23) > > > > .... > > > > Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`, > > if that's more appropriate. > > I think it would be better to have LOG_INSUFF_MEM for this case, but let's > hear from Philippe and others for suggestions. > > Since it's unlreated to the coverity fix and can be taken separately, so: > > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> Thanks Harsh. ~Shivang.
© 2016 - 2025 Red Hat, Inc.