| Message ID | 20240117090018.152031-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/xen: Add some null pointer checking to smp.c | expand |
On 2024/1/17 18:40, Markus Elfring wrote: >> kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. Ensure the allocation was successful >> by checking the pointer validity. > … >> +++ b/arch/x86/xen/smp.c >> @@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu) >> >> int xen_smp_intr_init(unsigned int cpu) >> { >> - int rc; >> + int rc = 0; > > I find the indication of a successful function execution sufficient by > the statement “return 0;” at the end. > How do you think about to omit such an extra variable initialisation? Thanks, it's no need now. I'll remove it in v3. > > >> char *resched_name, *callfunc_name, *debug_name; >> >> resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu); >> + if (!resched_name) { >> + rc = -ENOMEM; >> + goto fail; >> + } >> per_cpu(xen_resched_irq, cpu).name = resched_name; >> rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR, >> cpu, > > You propose to apply the same error code in four if branches. > I suggest to avoid the specification of duplicate assignment statements > for this purpose. > How do you think about to use another label like “e_nomem”? I'll add a new label to simply the code. > > Regards, > Markus
On Fri, Jan 19, 2024 at 05:22:25PM +0800, Kunwu Chan wrote: > On 2024/1/17 18:40, Markus Elfring wrote: > > > kasprintf() returns a pointer to dynamically allocated memory > > > which can be NULL upon failure. Ensure the allocation was successful > > > by checking the pointer validity. > > … > > > +++ b/arch/x86/xen/smp.c > > > @@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu) > > > > > > int xen_smp_intr_init(unsigned int cpu) > > > { > > > - int rc; > > > + int rc = 0; > > > > I find the indication of a successful function execution sufficient by > > the statement “return 0;” at the end. > > How do you think about to omit such an extra variable initialisation? > Thanks, it's no need now. I'll remove it in v3. This advice is good. Don't do unnecessary assignments. > > > > > > > char *resched_name, *callfunc_name, *debug_name; > > > > > > resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu); > > > + if (!resched_name) { > > > + rc = -ENOMEM; > > > + goto fail; > > > + } > > > per_cpu(xen_resched_irq, cpu).name = resched_name; > > > rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR, > > > cpu, > > > > You propose to apply the same error code in four if branches. > > I suggest to avoid the specification of duplicate assignment statements > > for this purpose. > > How do you think about to use another label like “e_nomem”? > I'll add a new label to simply the code. I'm not a Xen maintainer so I can't really comment on their style choices. However, as one of the kernel-janitors list people, I would say that not everyone agrees with Markus's style preferences. Markus was banned from the list for a while, but we unbanned everyone when we transitioned to the new list infrastructure. Do a search on lore to find out more. https://lore.kernel.org/all/?q=Elfring Perhaps wait for feedback from the maintainers for how to proceed? regards, dan carpenter
On 2024/1/22 16:30, Dan Carpenter wrote: > On Fri, Jan 19, 2024 at 05:22:25PM +0800, Kunwu Chan wrote: >> On 2024/1/17 18:40, Markus Elfring wrote: >>>> kasprintf() returns a pointer to dynamically allocated memory >>>> which can be NULL upon failure. Ensure the allocation was successful >>>> by checking the pointer validity. >>> … >>>> +++ b/arch/x86/xen/smp.c >>>> @@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu) >>>> >>>> int xen_smp_intr_init(unsigned int cpu) >>>> { >>>> - int rc; >>>> + int rc = 0; >>> >>> I find the indication of a successful function execution sufficient by >>> the statement “return 0;” at the end. >>> How do you think about to omit such an extra variable initialisation? >> Thanks, it's no need now. I'll remove it in v3. > > This advice is good. Don't do unnecessary assignments. Thanks for your suggestions, I'll keep it in mind. > >>> >>> >>>> char *resched_name, *callfunc_name, *debug_name; >>>> >>>> resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu); >>>> + if (!resched_name) { >>>> + rc = -ENOMEM; >>>> + goto fail; >>>> + } >>>> per_cpu(xen_resched_irq, cpu).name = resched_name; >>>> rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR, >>>> cpu, >>> >>> You propose to apply the same error code in four if branches. >>> I suggest to avoid the specification of duplicate assignment statements >>> for this purpose. >>> How do you think about to use another label like “e_nomem”? >> I'll add a new label to simply the code. > > I'm not a Xen maintainer so I can't really comment on their style > choices. However, as one of the kernel-janitors list people, I would > say that not everyone agrees with Markus's style preferences. Markus > was banned from the list for a while, but we unbanned everyone when we > transitioned to the new list infrastructure. Do a search on lore to > find out more. https://lore.kernel.org/all/?q=Elfring > > Perhaps wait for feedback from the maintainers for how to proceed? OK, I'll wait for the feedback. > > regards, > dan carpenter >
>>> How do you think about to use another label like “e_nomem”? >> I'll add a new label to simply the code. > > I'm not a Xen maintainer so I can't really comment on their style choices. Linux contributors can discuss various implementation details. > However, as one of the kernel-janitors list people, I would > say that not everyone agrees with Markus's style preferences. Can a corresponding document be improved accordingly? Centralized exiting of functions https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8-rc1#n526 Do you find a related information source helpful? https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources Regards, Markus
diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c index 4b0d6fff88de..0ea4f1b2ab21 100644 --- a/arch/x86/xen/smp.c +++ b/arch/x86/xen/smp.c @@ -61,10 +61,14 @@ void xen_smp_intr_free(unsigned int cpu) int xen_smp_intr_init(unsigned int cpu) { - int rc; + int rc = 0; char *resched_name, *callfunc_name, *debug_name; resched_name = kasprintf(GFP_KERNEL, "resched%d", cpu); + if (!resched_name) { + rc = -ENOMEM; + goto fail; + } per_cpu(xen_resched_irq, cpu).name = resched_name; rc = bind_ipi_to_irqhandler(XEN_RESCHEDULE_VECTOR, cpu, @@ -77,6 +81,10 @@ int xen_smp_intr_init(unsigned int cpu) per_cpu(xen_resched_irq, cpu).irq = rc; callfunc_name = kasprintf(GFP_KERNEL, "callfunc%d", cpu); + if (!callfunc_name) { + rc = -ENOMEM; + goto fail; + } per_cpu(xen_callfunc_irq, cpu).name = callfunc_name; rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_VECTOR, cpu, @@ -90,6 +98,10 @@ int xen_smp_intr_init(unsigned int cpu) if (!xen_fifo_events) { debug_name = kasprintf(GFP_KERNEL, "debug%d", cpu); + if (!debug_name) { + rc = -ENOMEM; + goto fail; + } per_cpu(xen_debug_irq, cpu).name = debug_name; rc = bind_virq_to_irqhandler(VIRQ_DEBUG, cpu, xen_debug_interrupt, @@ -101,6 +113,10 @@ int xen_smp_intr_init(unsigned int cpu) } callfunc_name = kasprintf(GFP_KERNEL, "callfuncsingle%d", cpu); + if (!callfunc_name) { + rc = -ENOMEM; + goto fail; + } per_cpu(xen_callfuncsingle_irq, cpu).name = callfunc_name; rc = bind_ipi_to_irqhandler(XEN_CALL_FUNCTION_SINGLE_VECTOR, cpu,
kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Ensure the allocation was successful by checking the pointer validity. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202401161119.iof6BQsf-lkp@intel.com/ --- v2: Initial rc and return errno in error paths --- arch/x86/xen/smp.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)