Re: [PATCH] mpt3sas: Fix double free warnings

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, May 08, 2020 at 05:18:54AM -0400, Suganath Prabu S wrote:
> Fix below warnings from Smatch static analyser:
> drivers/scsi/mpt3sas/mpt3sas_base.c:5256 _base_allocate_memory_pools()
> warn: 'ioc->hpr_lookup' double freed
> 
> drivers/scsi/mpt3sas/mpt3sas_base.c:5256 _base_allocate_memory_pools()
> warn: 'ioc->internal_lookup' double freed
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Suganath Prabu <suganath-prabu.subramani@broadcom.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 7fa3bdb..a6dbc73 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -4898,8 +4898,14 @@ _base_release_memory_pools(struct MPT3SAS_ADAPTER *ioc)
>  		    ioc->config_page, ioc->config_page_dma);
>  	}
>  
> -	kfree(ioc->hpr_lookup);
> -	kfree(ioc->internal_lookup);
> +	if (ioc->hpr_lookup) {
> +		kfree(ioc->hpr_lookup);
> +		ioc->hpr_lookup = NULL;
> +	}
> +	if (ioc->internal_lookup) {
> +		kfree(ioc->internal_lookup);
> +		ioc->internal_lookup = NULL;
> +	}


We could remove the if statements because kfree()ing a NULL is a no-op.
I think the build bots will automatically send a patch suggesting to do
that when they see this patch.

Really there is no way that these massive free everything in every
situation functions will ever work 100%.  There is a simple and correct
method to do error handling which is the "Free the most recent
allocation" method.

1) Every function cleans up it's own allocations.
2) Every allocation function has a mirrored free function.
3) Always use clear label names like "goto free_variable_x;"
4) The goto should free the most recent allocation.

The results of rule number 4 are:
1) You never free things which haven't been allocated.
2) The error handling is in the reverse/mirror order from the allocation
3) The error handling is simple because you only need to remember the
   most recent allocation.  Easy to audit.  No leaks.  No bugs.

It looks like this in practice.

int my_func(void)
{
	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM:
		goto free_a;
	}

	c = alloc();
	if (!c) {
		ret = -ENOMEM;
		goto free_b;
	}

	return 0;

free_b:
	free(b);
free_a:
	free(a);
	return ret;
}

Then cut and paste and add a free(c); to create the free function.

void my_free_func(void)
{
	free(c);
	free(b);
	free(a);
}

This method removes all the if statements in the error handling path but
it uses more labels so the number of lines is basically the same.  There
is one more rule for unwinding from loops and this code has a lot of
looped allocations:  Release the partial iteration before breaking
from the loop.

	for (i = 0; i < count; i++) {
		array[i].a = alloc();
		if (!array[i].a) {
			ret = -ENOMEM;
			goto unwind;
		}

		array[i].b = alloc();
		if (!array[i].b) {
			free(array[i].a);
			ret = -ENOMEM;
			goto unwind;
		}
	}

	...

unwind:
	while (--i >= 0) {
		free(array[i].b);
		free(array[i].a);
	}

The style here with a big _base_release_memory_pools() function that has
to handle partially allocated resources will never work 100% correctly.
Fortunately the remaining bugs are minor leaks and not crashing bugs so
that's probably as good as we can hope for.

regards,
dan carpenter