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