Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
From: Paul Bolle <hidden>
Date: 2016-09-26 21:14:01
Also in:
kernel-janitors, lkml
Markus, On Mon, 2016-09-26 at 17:43 +0200, SF Markus Elfring wrote:
Memory was not released (as it would be expected) when one call of further resource reservations failed.
This was the only thing in this series that triggered more than a, very uninspired, "meh" on first read.
* Split a condition check for memory allocation failures so that each pointer from these function calls will be checked immediately. See also background information: Topic "CWE-754: Improper check for unusual or exceptional conditions" Link: https://cwe.mitre.org/data/definitions/754.html
A quick scan of that link suggests we can do without the above "background information" in the commit explanation.
* Adjust jump targets according to the Linux coding style convention.
Another "meh".
quoted hunk ↗ jump to hunk
Signed-off-by: Markus Elfring <redacted> --- drivers/isdn/gigaset/common.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)diff --git a/drivers/isdn/gigaset/common.cb/drivers/isdn/gigaset/common.c index c05a2a4..2e9382f 100644--- a/drivers/isdn/gigaset/common.c +++ b/drivers/isdn/gigaset/common.c@@ -710,10 +710,13 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,cs->mode = M_UNKNOWN; cs->mstate = MS_UNINITIALIZED; cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL); + if (!cs->bcs) + goto report_failure; + cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL); - if (!cs->bcs || !cs->inbuf) { - goto error; - } + if (!cs->inbuf) + goto free_bcs; + ++cs->cs_init; gig_dbg(DEBUG_INIT, "setting up at_state");@@ -737,14 +740,14 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,gig_dbg(DEBUG_INIT, "setting up iif"); if (gigaset_isdn_regdev(cs, modulename) < 0) { pr_err("error registering ISDN device\n"); - goto error; + goto free_bcs; } make_valid(cs, VALID_ID); ++cs->cs_init; gig_dbg(DEBUG_INIT, "setting up hw"); if (cs->ops->initcshw(cs) < 0) - goto error; + goto free_bcs; ++cs->cs_init;@@ -759,7 +762,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,gig_dbg(DEBUG_INIT, "setting up bcs[%d]", i); if (gigaset_initbcs(cs->bcs + i, cs, i) < 0) { pr_err("could not allocate channel %d data\n", i); - goto error; + goto free_bcs; } }@@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,gig_dbg(DEBUG_INIT, "cs initialized"); return cs; - -error: +free_bcs: + kfree(cs->bcs); +report_failure: gig_dbg(DEBUG_INIT, "failed"); gigaset_freecs(cs);
gigaset_freecs() is not a function I look at for the fun of it. But still, in it we find: case 0: /* error in basic setup */ [...] kfree(cs->inbuf); kfree(cs->bcs); As far as I can tell we will call those two kfree()'s if we jump to "error". So, contrary to your analysis, I don't think we leak cs->bcs.
return NULL;
Paul Bolle