* [PATCH] X86: intel_ips, check for kzalloc properly @ 2010-06-21 15:02 Jiri Slaby 2010-06-21 15:25 ` Jesse Barnes 0 siblings, 1 reply; 6+ messages in thread From: Jiri Slaby @ 2010-06-21 15:02 UTC (permalink / raw) To: mjg; +Cc: platform-driver-x86, linux-kernel, jirislaby, Jesse Barnes Stanse found that there are two NULL checks missing in ips_monitor. So check their value too and bail out appropriately if the allocation failed. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Matthew Garrett <mjg@redhat.com> --- drivers/platform/x86/intel_ips.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c index cdaf40e..3c7ea9a 100644 --- a/drivers/platform/x86/intel_ips.c +++ b/drivers/platform/x86/intel_ips.c @@ -931,7 +931,8 @@ static int ips_monitor(void *data) mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || + !cpu_samples || !mchp_samples) { dev_err(&ips->dev->dev, "failed to allocate sample array, ips disabled\n"); kfree(mcp_samples); -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] X86: intel_ips, check for kzalloc properly 2010-06-21 15:02 [PATCH] X86: intel_ips, check for kzalloc properly Jiri Slaby @ 2010-06-21 15:25 ` Jesse Barnes 2010-06-21 15:33 ` Jiri Slaby 0 siblings, 1 reply; 6+ messages in thread From: Jesse Barnes @ 2010-06-21 15:25 UTC (permalink / raw) To: Jiri Slaby; +Cc: mjg, platform-driver-x86, linux-kernel, jirislaby On Mon, 21 Jun 2010 17:02:11 +0200 Jiri Slaby <jslaby@suse.cz> wrote: > Stanse found that there are two NULL checks missing in ips_monitor. So > check their value too and bail out appropriately if the allocation > failed. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Matthew Garrett <mjg@redhat.com> > --- > drivers/platform/x86/intel_ips.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c > index cdaf40e..3c7ea9a 100644 > --- a/drivers/platform/x86/intel_ips.c > +++ b/drivers/platform/x86/intel_ips.c > @@ -931,7 +931,8 @@ static int ips_monitor(void *data) > mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); > cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { > + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || > + !cpu_samples || !mchp_samples) { > dev_err(&ips->dev->dev, > "failed to allocate sample array, ips disabled\n"); > kfree(mcp_samples); Ah cool, am I also missing the appropriate kfree() calls for the last two? The context doesn't have it. Otherwise, Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org> -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] X86: intel_ips, check for kzalloc properly 2010-06-21 15:25 ` Jesse Barnes @ 2010-06-21 15:33 ` Jiri Slaby 2010-06-21 15:34 ` Jesse Barnes 0 siblings, 1 reply; 6+ messages in thread From: Jiri Slaby @ 2010-06-21 15:33 UTC (permalink / raw) To: Jesse Barnes; +Cc: Jiri Slaby, mjg, platform-driver-x86, linux-kernel On 06/21/2010 05:25 PM, Jesse Barnes wrote: > On Mon, 21 Jun 2010 17:02:11 +0200 > Jiri Slaby <jslaby@suse.cz> wrote: > >> Stanse found that there are two NULL checks missing in ips_monitor. So >> check their value too and bail out appropriately if the allocation >> failed. >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >> Cc: Matthew Garrett <mjg@redhat.com> >> --- >> drivers/platform/x86/intel_ips.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c >> index cdaf40e..3c7ea9a 100644 >> --- a/drivers/platform/x86/intel_ips.c >> +++ b/drivers/platform/x86/intel_ips.c >> @@ -931,7 +931,8 @@ static int ips_monitor(void *data) >> mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); >> cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); >> mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); >> - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { >> + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || >> + !cpu_samples || !mchp_samples) { >> dev_err(&ips->dev->dev, >> "failed to allocate sample array, ips disabled\n"); >> kfree(mcp_samples); > > Ah cool, am I also missing the appropriate kfree() calls for the last > two? The context doesn't have it. Otherwise, I checked when I was patching that and the last is not freed there. However I didn't add it since at least one must be non-NULL and in this case it can be only the last. (Until somebody add another allocation indeed.) So should I add even the last free there? thanks, -- js ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] X86: intel_ips, check for kzalloc properly 2010-06-21 15:33 ` Jiri Slaby @ 2010-06-21 15:34 ` Jesse Barnes 2010-06-21 15:40 ` Jiri Slaby 0 siblings, 1 reply; 6+ messages in thread From: Jesse Barnes @ 2010-06-21 15:34 UTC (permalink / raw) To: Jiri Slaby; +Cc: Jiri Slaby, mjg, platform-driver-x86, linux-kernel On Mon, 21 Jun 2010 17:33:48 +0200 Jiri Slaby <jirislaby@gmail.com> wrote: > On 06/21/2010 05:25 PM, Jesse Barnes wrote: > > On Mon, 21 Jun 2010 17:02:11 +0200 > > Jiri Slaby <jslaby@suse.cz> wrote: > > > >> Stanse found that there are two NULL checks missing in ips_monitor. So > >> check their value too and bail out appropriately if the allocation > >> failed. > >> > >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> > >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > >> Cc: Matthew Garrett <mjg@redhat.com> > >> --- > >> drivers/platform/x86/intel_ips.c | 3 ++- > >> 1 files changed, 2 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c > >> index cdaf40e..3c7ea9a 100644 > >> --- a/drivers/platform/x86/intel_ips.c > >> +++ b/drivers/platform/x86/intel_ips.c > >> @@ -931,7 +931,8 @@ static int ips_monitor(void *data) > >> mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); > >> cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > >> mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > >> - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { > >> + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || > >> + !cpu_samples || !mchp_samples) { > >> dev_err(&ips->dev->dev, > >> "failed to allocate sample array, ips disabled\n"); > >> kfree(mcp_samples); > > > > Ah cool, am I also missing the appropriate kfree() calls for the last > > two? The context doesn't have it. Otherwise, > > I checked when I was patching that and the last is not freed there. > However I didn't add it since at least one must be non-NULL and in this > case it can be only the last. (Until somebody add another allocation > indeed.) So should I add even the last free there? Up to Matthew. Not adding it will make people do a double take, but I agree it'll be safe. -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] X86: intel_ips, check for kzalloc properly 2010-06-21 15:34 ` Jesse Barnes @ 2010-06-21 15:40 ` Jiri Slaby 2010-06-21 16:58 ` Jesse Barnes 0 siblings, 1 reply; 6+ messages in thread From: Jiri Slaby @ 2010-06-21 15:40 UTC (permalink / raw) To: mjg; +Cc: platform-driver-x86, linux-kernel, jirislaby, Jesse Barnes Or what about this one? -- Stanse found that there are two NULL checks missing in ips_monitor. So check their value too and bail out appropriately if the allocation failed. While at it, add one more kfree to the fail path. It is not necessary now, but may be needed in the future when a new allocation is added. And for completeness. Also remove unneeded initialization of the variables. They are all set right after their declaration. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Matthew Garrett <mjg@redhat.com> --- drivers/platform/x86/intel_ips.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c index cdaf40e..0344822 100644 --- a/drivers/platform/x86/intel_ips.c +++ b/drivers/platform/x86/intel_ips.c @@ -920,9 +920,8 @@ static int ips_monitor(void *data) struct timer_list timer; unsigned long seqno_timestamp, expire, last_msecs, last_sample_period; int i; - u32 *cpu_samples = NULL, *mchp_samples = NULL, old_cpu_power; - u16 *mcp_samples = NULL, *ctv1_samples = NULL, *ctv2_samples = NULL, - *mch_samples = NULL; + u32 *cpu_samples, *mchp_samples, old_cpu_power; + u16 *mcp_samples, *ctv1_samples, *ctv2_samples, *mch_samples; u8 cur_seqno, last_seqno; mcp_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); @@ -931,7 +930,8 @@ static int ips_monitor(void *data) mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || + !cpu_samples || !mchp_samples) { dev_err(&ips->dev->dev, "failed to allocate sample array, ips disabled\n"); kfree(mcp_samples); @@ -939,6 +939,7 @@ static int ips_monitor(void *data) kfree(ctv2_samples); kfree(mch_samples); kfree(cpu_samples); + kfree(mchp_samples); kthread_stop(ips->adjust); return -ENOMEM; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] X86: intel_ips, check for kzalloc properly 2010-06-21 15:40 ` Jiri Slaby @ 2010-06-21 16:58 ` Jesse Barnes 0 siblings, 0 replies; 6+ messages in thread From: Jesse Barnes @ 2010-06-21 16:58 UTC (permalink / raw) To: Jiri Slaby; +Cc: mjg, platform-driver-x86, linux-kernel, jirislaby On Mon, 21 Jun 2010 17:40:15 +0200 Jiri Slaby <jslaby@suse.cz> wrote: > Or what about this one? > > -- > > Stanse found that there are two NULL checks missing in ips_monitor. So > check their value too and bail out appropriately if the allocation > failed. > > While at it, add one more kfree to the fail path. It is not necessary > now, but may be needed in the future when a new allocation is added. > And for completeness. > > Also remove unneeded initialization of the variables. They are all set > right after their declaration. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Matthew Garrett <mjg@redhat.com> > --- > drivers/platform/x86/intel_ips.c | 9 +++++---- > 1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c > index cdaf40e..0344822 100644 > --- a/drivers/platform/x86/intel_ips.c > +++ b/drivers/platform/x86/intel_ips.c > @@ -920,9 +920,8 @@ static int ips_monitor(void *data) > struct timer_list timer; > unsigned long seqno_timestamp, expire, last_msecs, last_sample_period; > int i; > - u32 *cpu_samples = NULL, *mchp_samples = NULL, old_cpu_power; > - u16 *mcp_samples = NULL, *ctv1_samples = NULL, *ctv2_samples = NULL, > - *mch_samples = NULL; > + u32 *cpu_samples, *mchp_samples, old_cpu_power; > + u16 *mcp_samples, *ctv1_samples, *ctv2_samples, *mch_samples; > u8 cur_seqno, last_seqno; > > mcp_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); > @@ -931,7 +930,8 @@ static int ips_monitor(void *data) > mch_samples = kzalloc(sizeof(u16) * IPS_SAMPLE_COUNT, GFP_KERNEL); > cpu_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > mchp_samples = kzalloc(sizeof(u32) * IPS_SAMPLE_COUNT, GFP_KERNEL); > - if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples) { > + if (!mcp_samples || !ctv1_samples || !ctv2_samples || !mch_samples || > + !cpu_samples || !mchp_samples) { > dev_err(&ips->dev->dev, > "failed to allocate sample array, ips disabled\n"); > kfree(mcp_samples); > @@ -939,6 +939,7 @@ static int ips_monitor(void *data) > kfree(ctv2_samples); > kfree(mch_samples); > kfree(cpu_samples); > + kfree(mchp_samples); > kthread_stop(ips->adjust); > return -ENOMEM; > } Even better. Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org> Thanks, -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-06-21 16:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-06-21 15:02 [PATCH] X86: intel_ips, check for kzalloc properly Jiri Slaby 2010-06-21 15:25 ` Jesse Barnes 2010-06-21 15:33 ` Jiri Slaby 2010-06-21 15:34 ` Jesse Barnes 2010-06-21 15:40 ` Jiri Slaby 2010-06-21 16:58 ` Jesse Barnes
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.