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

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

[mhkelley58]

From: Michael Kelley <mikelley@microsoft.com>

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@microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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)
 		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);
 	}
 
 	kfree(hv_context.hv_numa_map);
-- 
1.8.3.1

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

[Dan Carpenter]

On Wed, Aug 01, 2018 at 03:45:13PM -0700, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mikelley@microsoft.com>
> 
> 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@microsoft.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  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.

   211                          pr_err("Unable to allocate clock event device\n");
   212                          goto err;
   213                  }

regards,
dan carpenter

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

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, August 1, 2018 11:42 PM
> To: Michael Kelley (EOSG) <Michael.H.Kelley@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; jasowang@redhat.com;
> marcelo.cerri@canonical.com; Stephen Hemminger
> <sthemmin@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> 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@gmail.com wrote:
> > From: Michael Kelley <mikelley@microsoft.com>
> >
> > 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@microsoft.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  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