> -Original Message-
> From: Dan Carpenter
> Sent: Wednesday, August 1, 2018 11:42 PM
> To: Michael Kelley (EOSG)
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com;
> vkuzn...@redhat.com; jasow...@redhat.com;
> marcelo.ce...@canonical.com; Stephen Hemminger
> ; KY Srinivasan
> Subject: Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic
> memory free path
>
> On Wed, Aug 01, 2018 at 03:45:13PM -0700, mhkelle...@gmail.com wrote:
> > From: Michael Kelley
> >
> > clk_evt memory is not being freed when the synic is shutdown
> > or when there is an allocation error. Add the appropriate
> > kfree() call, along with a comment to clarify how the memory
> > gets freed after an allocation error. Make the free path
> > consistent by removing checks for NULL since kfree() and
> > free_page() already do the check.
> >
> > Signed-off-by: Michael Kelley
> > Reported-by: Dan Carpenter
> > ---
> > drivers/hv/hv.c | 14 --
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> > index 8d4fe0e..1fb9a6b 100644
> > --- a/drivers/hv/hv.c
> > +++ b/drivers/hv/hv.c
> > @@ -240,6 +240,10 @@ int hv_synic_alloc(void)
> >
> > return 0;
> > err:
> > + /*
> > +* Any memory allocations that succeeded will be freed when
> > +* the caller cleans up by calling hv_synic_free()
> > +*/
> > return -ENOMEM;
> > }
> >
> > @@ -252,12 +256,10 @@ void hv_synic_free(void)
> > for_each_present_cpu(cpu) {
> > struct hv_per_cpu_context *hv_cpu
> > = per_cpu_ptr(hv_context.cpu_context, cpu);
> >
> > - if (hv_cpu->synic_event_page)
> > - free_page((unsigned long)hv_cpu-
> >synic_event_page);
> > - if (hv_cpu->synic_message_page)
> > - free_page((unsigned long)hv_cpu-
> >synic_message_page);
> > - if (hv_cpu->post_msg_page)
> > - free_page((unsigned long)hv_cpu-
> >post_msg_page);
> > + kfree(hv_cpu->clk_evt);
> > + free_page((unsigned long)hv_cpu->synic_event_page);
> > + free_page((unsigned long)hv_cpu->synic_message_page);
> > + free_page((unsigned long)hv_cpu->post_msg_page);
>
> This looks buggy.
>
> We can pass NULLs to free_page() so that's fine. So the error handling
> assumes
> that hv_cpu->clk_evt is either NULL or allocated. Here is how it is
> allocated:
>
>189 int hv_synic_alloc(void)
>190 {
>191 int cpu;
>192
>193 hv_context.hv_numa_map = kcalloc(nr_node_ids, sizeof(struct
> cpumask),
>194 GFP_KERNEL);
>195 if (hv_context.hv_numa_map == NULL) {
>196 pr_err("Unable to allocate NUMA map\n");
>197 goto err;
>198 }
>199
>200 for_each_present_cpu(cpu) {
> ^^^
> We loop over each CPU.
>
>201 struct hv_per_cpu_context *hv_cpu
>202 = per_cpu_ptr(hv_context.cpu_context, cpu);
>203
>204 memset(hv_cpu, 0, sizeof(*hv_cpu));
> ^^
> We set this cpu memory to NULL.
>
>205 tasklet_init(&hv_cpu->msg_dpc,
>206 vmbus_on_msg_dpc, (unsigned long)
> hv_cpu);
>207
>208 hv_cpu->clk_evt = kzalloc(sizeof(struct
> clock_event_device),
>209GFP_KERNEL);
>210 if (hv_cpu->clk_evt == NULL) {
> ^^^
> Let's assume this fails on the first iteration through the loop. We
> haven't memset the next cpu to NULL or allocated it. But we loop over
> all the cpus in the error handling. Since we didn't set everything to NULL in
> hv_synic_free() then it seems like this could be a double free. It's
> possible I
> am misreading the code, but either it's buggy or the memset() can be
> removed.
>
> This is a very typical bug for this style of error handling where we free
> things which were never allocated.
Thanks Dan! We will fix this issue soon.
K. Y
>
>211 pr_err("Unable to allocate clock event
> device\n");
>212 goto err;
>213 }
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
