RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory free path

[KY Srinivasan]

> -----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