> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: Wednesday, August 1, 2018 11:42 PM
> To: Michael Kelley (EOSG) <Michael.H.Kelley@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx;
> marcelo.cerri@xxxxxxxxxxxxx; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>
> 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, mhkelley58@xxxxxxxxx wrote:
> > From: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> >
> > 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 <mikelley@xxxxxxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > ---
> > 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),
> 209 GFP_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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
