Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

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

You are right.

Thanks that you pointed this source code place out again.

I imagined that the exception handling implementation could be more direct
somehow for a while. But this function takes extra care for data synchronisation
by a mutex.

Now I recognise also that this proposed update step "4" was inappropriate.
I'm sorry for the confusion I introduced here.

Regards,
Markus

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

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

You are right.

Thanks that you pointed this source code place out again.

I imagined that the exception handling implementation could be more direct
somehow for a while. But this function takes extra care for data synchronisation
by a mutex.

Now I recognise also that this proposed update step "4" was inappropriate.
I'm sorry for the confusion I introduced here.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote:
> >> * Is it still correct nowadays that the function "gigaset_initcs" did not
> >>   call the function "kfree" after a later function call failed?
> > 
> > Yes, if it is handled in another place, Paul already did show you the place.
> 
> To which source code place do you refer here?

Obviously the one Paul pointed out to you in detail in his mail dated
Mon, 26 Sep 2016 23:13:54 +0200.

HTH,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote:
> >> * Is it still correct nowadays that the function "gigaset_initcs" did not
> >>   call the function "kfree" after a later function call failed?
> > 
> > Yes, if it is handled in another place, Paul already did show you the place.
> 
> To which source code place do you refer here?

Obviously the one Paul pointed out to you in detail in his mail dated
Mon, 26 Sep 2016 23:13:54 +0200.

HTH,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> * Is it still correct nowadays that the function "gigaset_initcs" did not
>>   call the function "kfree" after a later function call failed?
> 
> Yes, if it is handled in another place, Paul already did show you the place.

To which source code place do you refer here?


>> * Do you expect that allocated memory will be automatically reclaimed
>>   after it would return a null pointer?
>>
> Of course not

Thanks for this acknowledgement.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> * Is it still correct nowadays that the function "gigaset_initcs" did not
>>   call the function "kfree" after a later function call failed?
> 
> Yes, if it is handled in another place, Paul already did show you the place.

To which source code place do you refer here?


>> * Do you expect that allocated memory will be automatically reclaimed
>>   after it would return a null pointer?
>>
> Of course not

Thanks for this acknowledgement.

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[isdn]

Am 27.09.2016 um 13:32 schrieb SF Markus Elfring:
>>> I got the impression that the exception handling  was incomplete in the
>>> implementation of the function "gigaset_initcs".
>>
>> That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?
> 

Yes, if it is handled in another place, Paul already did show you the place.


> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?
> 
Of course not

Best regards
Karsten

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[isdn]

Am 27.09.2016 um 13:32 schrieb SF Markus Elfring:
>>> I got the impression that the exception handling  was incomplete in the
>>> implementation of the function "gigaset_initcs".
>>
>> That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?
> 

Yes, if it is handled in another place, Paul already did show you the place.


> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?
> 
Of course not

Best regards
Karsten

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote:
> >> I got the impression that the exception handling  was incomplete in the
> >> implementation of the function "gigaset_initcs".
> > 
> > That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?

Wrong premise. That statement was never correct.

> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?

No. Should I? Do you?

Regards,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote:
> >> I got the impression that the exception handling  was incomplete in the
> >> implementation of the function "gigaset_initcs".
> > 
> > That impression is wrong. Careful reading of the code will confirm that.
> 
> * Is it still correct nowadays that the function "gigaset_initcs" did not
>   call the function "kfree" after a later function call failed?

Wrong premise. That statement was never correct.

> * Do you expect that allocated memory will be automatically reclaimed
>   after it would return a null pointer?

No. Should I? Do you?

Regards,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> I got the impression that the exception handling  was incomplete in the
>> implementation of the function "gigaset_initcs".
> 
> That impression is wrong. Careful reading of the code will confirm that.

* Is it still correct nowadays that the function "gigaset_initcs" did not
  call the function "kfree" after a later function call failed?

* Do you expect that allocated memory will be automatically reclaimed
  after it would return a null pointer?

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> I got the impression that the exception handling  was incomplete in the
>> implementation of the function "gigaset_initcs".
> 
> That impression is wrong. Careful reading of the code will confirm that.

* Is it still correct nowadays that the function "gigaset_initcs" did not
  call the function "kfree" after a later function call failed?

* Do you expect that allocated memory will be automatically reclaimed
  after it would return a null pointer?

Regards,
Markus

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

Hi,

as longtime maintainer of the code in question I feel compelled to chime
in at this point.

On Tue, Sep 27, 2016, at 11:34, SF Markus Elfring wrote:
> >> Will it matter here if the function "kfree" will be called for the
> >> data structure members "bcs" and "inbuf" after a later function call
> >> failed within the implementation of "gigaset_initcs"?
> > 
> > My translation of this question is: could you please hold my hand while
> > I read the code of a driver I do not use - a driver for hardware that I
> > don't even have, and therefor cannot really test - after I submitted a
> > patch that appears to be broken?
> 
> I got the impression that the exception handling  was incomplete in the
> implementation of the function "gigaset_initcs".

That impression is wrong. Careful reading of the code will confirm that.

> Does anybody (besides me) care for improving the software situation
> there?

There's no urgent need for improvement. The code is stable and there's
no demonstrated bug to be fixed.
You could improve the coding style, but that is of secondary importance,
and if you want to do that, as a minimum you have to make sure that you
don't introduce new bugs.

Thanks,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Tilman Schmidt]

Hi,

as longtime maintainer of the code in question I feel compelled to chime
in at this point.

On Tue, Sep 27, 2016, at 11:34, SF Markus Elfring wrote:
> >> Will it matter here if the function "kfree" will be called for the
> >> data structure members "bcs" and "inbuf" after a later function call
> >> failed within the implementation of "gigaset_initcs"?
> > 
> > My translation of this question is: could you please hold my hand while
> > I read the code of a driver I do not use - a driver for hardware that I
> > don't even have, and therefor cannot really test - after I submitted a
> > patch that appears to be broken?
> 
> I got the impression that the exception handling  was incomplete in the
> implementation of the function "gigaset_initcs".

That impression is wrong. Careful reading of the code will confirm that.

> Does anybody (besides me) care for improving the software situation
> there?

There's no urgent need for improvement. The code is stable and there's
no demonstrated bug to be fixed.
You could improve the coding style, but that is of secondary importance,
and if you want to do that, as a minimum you have to make sure that you
don't introduce new bugs.

Thanks,
Tilman

-- 
Tilman Schmidt
til...@imap.cc

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

You're in Eliza mode again.

(Hat tip to Björn Mork, https://lkml.org/lkml/2016/1/4/259).


Paul Bolle

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

You're in Eliza mode again.

(Hat tip to Björn Mork, https://lkml.org/lkml/2016/1/4/259).


Paul Bolle

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> Will it matter here if the function "kfree" will be called for the
>> data structure members "bcs" and "inbuf" after a later function call
>> failed within the implementation of "gigaset_initcs"?
> 
> My translation of this question is: could you please hold my hand while
> I read the code of a driver I do not use - a driver for hardware that I
> don't even have, and therefor cannot really test - after I submitted a
> patch that appears to be broken?

I got the impression that the exception handling  was incomplete in the
implementation of the function "gigaset_initcs".
Does anybody (besides me) care for improving the software situation there?


> My answer to that question is: no, sorry, I won't do that.

I find that the process has just started once more to clarify in which
directions this software module could evolve.

Do we discuss a potential memory leak under special conditions here?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

>> Will it matter here if the function "kfree" will be called for the
>> data structure members "bcs" and "inbuf" after a later function call
>> failed within the implementation of "gigaset_initcs"?
> 
> My translation of this question is: could you please hold my hand while
> I read the code of a driver I do not use - a driver for hardware that I
> don't even have, and therefor cannot really test - after I submitted a
> patch that appears to be broken?

I got the impression that the exception handling  was incomplete in the
implementation of the function "gigaset_initcs".
Does anybody (besides me) care for improving the software situation there?


> My answer to that question is: no, sorry, I won't do that.

I find that the process has just started once more to clarify in which
directions this software module could evolve.

Do we discuss a potential memory leak under special conditions here?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

On Tue, 2016-09-27 at 07:20 +0200, SF Markus Elfring wrote:
> Will it matter here if the function "kfree" will be called for the
> data structure members "bcs" and "inbuf" after a later function call
> failed within the implementation of "gigaset_initcs"?

My translation of this question is: could you please hold my hand while
I read the code of a driver I do not use - a driver for hardware that I
don't even have, and therefor cannot really test - after I submitted a
patch that appears to be broken?

My answer to that question is: no, sorry, I won't do that.


Paul Bolle

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

On Tue, 2016-09-27 at 07:20 +0200, SF Markus Elfring wrote:
> Will it matter here if the function "kfree" will be called for the
> data structure members "bcs" and "inbuf" after a later function call
> failed within the implementation of "gigaset_initcs"?

My translation of this question is: could you please hold my hand while
I read the code of a driver I do not use - a driver for hardware that I
don't even have, and therefor cannot really test - after I submitted a
patch that appears to be broken?

My answer to that question is: no, sorry, I won't do that.


Paul Bolle

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

Ah well...  Someone else discovered the double free bug first and gave
it away.  Reassuring, I guess.

regards,
dan carpenter

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

Ah well...  Someone else discovered the double free bug first and gave
it away.  Reassuring, I guess.

regards,
dan carpenter

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

> This patch creates new bugs.

Thanks for your information.


> I have a policy of not telling Markus where the bug is,

I find this kind of response strange.


> because otherwise he'll just resend the patch

This can also happen when the other contributors request it.


> and I have told him many times to stop sending these cleanup patches

Software "cleanups" seem to stress the review process to some degree.


> that just introduce bugs and waste maintainer time.

I guess that the situation is mixed depending on the subsystem
or concrete software module, isn't it?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

> This patch creates new bugs.

Thanks for your information.


> I have a policy of not telling Markus where the bug is,

I find this kind of response strange.


> because otherwise he'll just resend the patch

This can also happen when the other contributors request it.


> and I have told him many times to stop sending these cleanup patches

Software "cleanups" seem to stress the review process to some degree.


> that just introduce bugs and waste maintainer time.

I guess that the situation is mixed depending on the subsystem
or concrete software module, isn't it?

Regards,
Markus

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

This patch creates new bugs.

I have a policy of not telling Markus where the bug is, because
otherwise he'll just resend the patch and I have told him many times to
stop sending these cleanup patches that just introduce bugs and waste
maintainer time.

regards,
dan carpenter

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Dan Carpenter]

This patch creates new bugs.

I have a policy of not telling Markus where the bug is, because
otherwise he'll just resend the patch and I have told him many times to
stop sending these cleanup patches that just introduce bugs and waste
maintainer time.

regards,
dan carpenter

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

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

Will it matter here if the function "kfree" will be called for the
data structure members "bcs" and "inbuf" after a later function call
failed within the implementation of "gigaset_initcs"?

Regards,
Markus

Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

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

Will it matter here if the function "kfree" will be called for the
data structure members "bcs" and "inbuf" after a later function call
failed within the implementation of "gigaset_initcs"?

Regards,
Markus

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

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

> Signed-off-by: Markus Elfring 
> ---
>  drivers/isdn/gigaset/common.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/common.c
> b/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

Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[Paul Bolle]

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

> Signed-off-by: Markus Elfring 
> ---
>  drivers/isdn/gigaset/common.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/common.c
> b/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

[PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 26 Sep 2016 16:30:50 +0200

Memory was not released (as it would be expected) when one call
of further resource reservations failed.

* 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

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring 
---
 drivers/isdn/gigaset/common.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/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);
return NULL;
-- 
2.10.0

[PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

[SF Markus Elfring]

From: Markus Elfring 
Date: Mon, 26 Sep 2016 16:30:50 +0200

Memory was not released (as it would be expected) when one call
of further resource reservations failed.

* 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

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring 
---
 drivers/isdn/gigaset/common.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/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);
return NULL;
-- 
2.10.0