Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 2 Aug 2018 09:41:33 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     mikelley@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, sthemmin@microsoft.com,
        kys@microsoft.com
Subject: Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory
 free path
Message-ID: <20180802064133.jm5wo4e5ypn6p3xq@mwanda>
References: <1533163513-10764-1-git-send-email-mikelley@microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1533163513-10764-1-git-send-email-mikelley@microsoft.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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