| Message ID | 1520218927-309-1-git-send-email-chuhu@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 05-03-18, 11:02, Chunyu Hu wrote: > Kmemleak reported the below leak. When cppc_cpufreq_init went into > failure path, the cpu mask is not freed. After fix, this report is > gone. And to avaoid potential NULL pointer reference, check the cpu > value first. > > unreferenced object 0xffff800fd5ea4880 (size 128): > comm "swapper/0", pid 1, jiffies 4294939510 (age 668.680s) > hex dump (first 32 bytes): > 00 00 00 00 20 00 00 00 00 00 00 00 00 00 00 00 .... ........... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<ffff0000082c4ae4>] __kmalloc_node+0x278/0x634 > [<ffff0000088f4a74>] alloc_cpumask_var_node+0x28/0x60 > [<ffff0000088f4af0>] zalloc_cpumask_var+0x14/0x1c > [<ffff000008d20254>] cppc_cpufreq_init+0xd0/0x19c > [<ffff000008083828>] do_one_initcall+0xec/0x15c > [<ffff000008cd1018>] kernel_init_freeable+0x1f4/0x2a4 > [<ffff0000089099b0>] kernel_init+0x18/0x10c > [<ffff000008084d50>] ret_from_fork+0x10/0x18 > [<ffffffffffffffff>] 0xffffffffffffffff > > CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> > CC: Viresh Kumar <viresh.kumar@linaro.org> > CC: linux-pm@vger.kernel.org > Signed-off-by: Chunyu Hu <chuhu@redhat.com> > --- > drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index a1c3025..34eb4e3 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -230,8 +230,13 @@ static int __init cppc_cpufreq_init(void) > return ret; > > out: > - for_each_possible_cpu(i) > - kfree(all_cpu_data[i]); > + for_each_possible_cpu(i) { > + cpu = all_cpu_data[i]; > + if (!cpu) > + break; > + free_cpumask_var(cpu->shared_cpu_map); > + kfree(cpu); > + } > > kfree(all_cpu_data); > return -ENODEV; > @@ -246,6 +251,8 @@ static void __exit cppc_cpufreq_exit(void) > > for_each_possible_cpu(i) { > cpu = all_cpu_data[i]; > + if (!cpu) > + break; Why is this required? I feel it is guaranteed that cpu can't be NULL here ? > free_cpumask_var(cpu->shared_cpu_map); > kfree(cpu); > } > -- > 1.8.3.1
----- Original Message ----- > From: "Viresh Kumar" <viresh.kumar@linaro.org> > To: "Chunyu Hu" <chuhu@redhat.com> > Cc: rjw@rjwysocki.net, linux-pm@vger.kernel.org > Sent: Monday, March 5, 2018 12:49:15 PM > Subject: Re: [PATCH 2/2] cpufreq: Fix kmemleak in cppc_cpufreq_init failure path > > On 05-03-18, 11:02, Chunyu Hu wrote: > > Kmemleak reported the below leak. When cppc_cpufreq_init went into > > failure path, the cpu mask is not freed. After fix, this report is > > gone. And to avaoid potential NULL pointer reference, check the cpu > > value first. > > > > unreferenced object 0xffff800fd5ea4880 (size 128): > > comm "swapper/0", pid 1, jiffies 4294939510 (age 668.680s) > > hex dump (first 32 bytes): > > 00 00 00 00 20 00 00 00 00 00 00 00 00 00 00 00 .... ........... > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace: > > [<ffff0000082c4ae4>] __kmalloc_node+0x278/0x634 > > [<ffff0000088f4a74>] alloc_cpumask_var_node+0x28/0x60 > > [<ffff0000088f4af0>] zalloc_cpumask_var+0x14/0x1c > > [<ffff000008d20254>] cppc_cpufreq_init+0xd0/0x19c > > [<ffff000008083828>] do_one_initcall+0xec/0x15c > > [<ffff000008cd1018>] kernel_init_freeable+0x1f4/0x2a4 > > [<ffff0000089099b0>] kernel_init+0x18/0x10c > > [<ffff000008084d50>] ret_from_fork+0x10/0x18 > > [<ffffffffffffffff>] 0xffffffffffffffff > > > > CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > CC: Viresh Kumar <viresh.kumar@linaro.org> > > CC: linux-pm@vger.kernel.org > > Signed-off-by: Chunyu Hu <chuhu@redhat.com> > > --- > > drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/cppc_cpufreq.c > > b/drivers/cpufreq/cppc_cpufreq.c > > index a1c3025..34eb4e3 100644 > > --- a/drivers/cpufreq/cppc_cpufreq.c > > +++ b/drivers/cpufreq/cppc_cpufreq.c > > @@ -230,8 +230,13 @@ static int __init cppc_cpufreq_init(void) > > return ret; > > > > out: > > - for_each_possible_cpu(i) > > - kfree(all_cpu_data[i]); > > + for_each_possible_cpu(i) { > > + cpu = all_cpu_data[i]; > > + if (!cpu) > > + break; > > + free_cpumask_var(cpu->shared_cpu_map); > > + kfree(cpu); > > + } > > > > kfree(all_cpu_data); > > return -ENODEV; > > @@ -246,6 +251,8 @@ static void __exit cppc_cpufreq_exit(void) > > > > for_each_possible_cpu(i) { > > cpu = all_cpu_data[i]; > > + if (!cpu) > > + break; > > Why is this required? I feel it is guaranteed that cpu can't be NULL here ? oops, it's indeed useless, when the module init code failed, this exit code won't be executed as the module would be unloaded just when init failed. I'll send a V2 for removing this if no other problems in this patch :-). > > > free_cpumask_var(cpu->shared_cpu_map); > > kfree(cpu); > > } > > -- > > 1.8.3.1 > > -- > viresh >
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index a1c3025..34eb4e3 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -230,8 +230,13 @@ static int __init cppc_cpufreq_init(void) return ret; out: - for_each_possible_cpu(i) - kfree(all_cpu_data[i]); + for_each_possible_cpu(i) { + cpu = all_cpu_data[i]; + if (!cpu) + break; + free_cpumask_var(cpu->shared_cpu_map); + kfree(cpu); + } kfree(all_cpu_data); return -ENODEV; @@ -246,6 +251,8 @@ static void __exit cppc_cpufreq_exit(void) for_each_possible_cpu(i) { cpu = all_cpu_data[i]; + if (!cpu) + break; free_cpumask_var(cpu->shared_cpu_map); kfree(cpu); }
Kmemleak reported the below leak. When cppc_cpufreq_init went into failure path, the cpu mask is not freed. After fix, this report is gone. And to avaoid potential NULL pointer reference, check the cpu value first. unreferenced object 0xffff800fd5ea4880 (size 128): comm "swapper/0", pid 1, jiffies 4294939510 (age 668.680s) hex dump (first 32 bytes): 00 00 00 00 20 00 00 00 00 00 00 00 00 00 00 00 .... ........... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffff0000082c4ae4>] __kmalloc_node+0x278/0x634 [<ffff0000088f4a74>] alloc_cpumask_var_node+0x28/0x60 [<ffff0000088f4af0>] zalloc_cpumask_var+0x14/0x1c [<ffff000008d20254>] cppc_cpufreq_init+0xd0/0x19c [<ffff000008083828>] do_one_initcall+0xec/0x15c [<ffff000008cd1018>] kernel_init_freeable+0x1f4/0x2a4 [<ffff0000089099b0>] kernel_init+0x18/0x10c [<ffff000008084d50>] ret_from_fork+0x10/0x18 [<ffffffffffffffff>] 0xffffffffffffffff CC: "Rafael J. Wysocki" <rjw@rjwysocki.net> CC: Viresh Kumar <viresh.kumar@linaro.org> CC: linux-pm@vger.kernel.org Signed-off-by: Chunyu Hu <chuhu@redhat.com> --- drivers/cpufreq/cppc_cpufreq.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)