Messages in this thread | | | Date | Mon, 7 Oct 2019 16:37:10 +0100 | From | Will Deacon <> | Subject | Re: [PATCH v2] arm64: armv8_deprecated: Checking return value for memory allocation |
| |
On Mon, Oct 07, 2019 at 06:06:35PM +0800, Yunfeng Ye wrote: > There are no return value checking when using kzalloc() and kcalloc() for > memory allocation. so add it. > > Signed-off-by: Yunfeng Ye <yeyunfeng@huawei.com> > --- > v1 -> v2: > - return error code when memory allocation failure > > arch/arm64/kernel/armv8_deprecated.c | 57 +++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c > index 2ec09de..2284fcb 100644 > --- a/arch/arm64/kernel/armv8_deprecated.c > +++ b/arch/arm64/kernel/armv8_deprecated.c > @@ -168,12 +168,15 @@ static int update_insn_emulation_mode(struct insn_emulation *insn, > return ret; > } > > -static void __init register_insn_emulation(struct insn_emulation_ops *ops) > +static int __init register_insn_emulation(struct insn_emulation_ops *ops) > { > unsigned long flags; > struct insn_emulation *insn; > > insn = kzalloc(sizeof(*insn), GFP_KERNEL); > + if (!insn) > + return -ENOMEM; > + > insn->ops = ops; > insn->min = INSN_UNDEF; > > @@ -197,6 +200,7 @@ static void __init register_insn_emulation(struct insn_emulation_ops *ops) > > /* Register any handlers if required */ > update_insn_emulation_mode(insn, INSN_UNDEF); > + return 0; > } > > static int emulation_proc_handler(struct ctl_table *table, int write, > @@ -224,7 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write, > return ret; > } > > -static void __init register_insn_emulation_sysctl(void) > +static int __init register_insn_emulation_sysctl(void) > { > unsigned long flags; > int i = 0; > @@ -233,6 +237,8 @@ static void __init register_insn_emulation_sysctl(void) > > insns_sysctl = kcalloc(nr_insn_emulated + 1, sizeof(*sysctl), > GFP_KERNEL); > + if (!insns_sysctl) > + return -ENOMEM; > > raw_spin_lock_irqsave(&insn_emulation_lock, flags); > list_for_each_entry(insn, &insn_emulation, node) { > @@ -251,6 +257,7 @@ static void __init register_insn_emulation_sysctl(void) > raw_spin_unlock_irqrestore(&insn_emulation_lock, flags); > > register_sysctl("abi", insns_sysctl); > + return 0; > } > > /* > @@ -617,25 +624,47 @@ static int t16_setend_handler(struct pt_regs *regs, u32 instr) > */ > static int __init armv8_deprecated_init(void) > { > - if (IS_ENABLED(CONFIG_SWP_EMULATION)) > - register_insn_emulation(&swp_ops); > + int ret = 0; > + int err = 0; > + > + if (IS_ENABLED(CONFIG_SWP_EMULATION)) { > + ret = register_insn_emulation(&swp_ops); > + if (ret) { > + pr_err("register insn emulation swp: fail\n"); > + err = ret; > + } > + }
Is there much point in continuing here? May as well just return ret, I think. I also don't think you need to print anything, since kmalloc should already have shouted.
> - if (IS_ENABLED(CONFIG_CP15_BARRIER_EMULATION)) > - register_insn_emulation(&cp15_barrier_ops); > + if (IS_ENABLED(CONFIG_CP15_BARRIER_EMULATION)) { > + ret = register_insn_emulation(&cp15_barrier_ops); > + if (ret) { > + pr_err("register insn emulation cpu15_barrier: fail\n"); > + err = ret; > + } > + } > > if (IS_ENABLED(CONFIG_SETEND_EMULATION)) { > - if(system_supports_mixed_endian_el0()) > - register_insn_emulation(&setend_ops); > - else > + if (system_supports_mixed_endian_el0()) { > + ret = register_insn_emulation(&setend_ops); > + if (ret) { > + pr_err("register insn emulation setend: fail\n"); > + err = ret; > + } > + } else { > pr_info("setend instruction emulation is not supported on this system\n"); > + } > } > > - cpuhp_setup_state_nocalls(CPUHP_AP_ARM64_ISNDEP_STARTING, > - "arm64/isndep:starting", > - run_all_insn_set_hw_mode, NULL); > - register_insn_emulation_sysctl(); > + if (nr_insn_emulated) { > + cpuhp_setup_state_nocalls(CPUHP_AP_ARM64_ISNDEP_STARTING, > + "arm64/isndep:starting", > + run_all_insn_set_hw_mode, NULL); > + ret = register_insn_emulation_sysctl(); > + if (ret) > + err = ret; > + }
I'm dubious about leaving the cpuhp notifier registered if we fail here. Can we simply reorder the logic so that the notifier is registered after successfully calling register_insn_emulation_sysctl()?
Will
| |