| Message ID | 20200508091854.32748-1-suganath-prabu.subramani@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mpt3sas: Fix double free warnings | expand |
On 08/05/2020 11:19, Suganath Prabu S wrote: > - 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; > + } The check before kfree() isn't needed, you could simplify this to: kfree(ioc->hpr_lookup); ioc->hpr_lookup = NULL; kfree(ioc->internal_lookup); ioc->internal_lookup = NULL;
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
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; + } if (ioc->chain_lookup) { for (i = 0; i < ioc->scsiio_depth; i++) { for (j = ioc->chains_per_prp_buffer;
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(-)