Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1491366 > unrolled thread

[PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function  implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:40 +0200
    [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:40 +0200
      Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions Paul Bolle <pebolle@tiscali.nl> - 2016-09-28 13:40 +0200
        Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-28 18:50 +0200
          Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions Paul Bolle <pebolle@tiscali.nl> - 2016-09-28 19:50 +0200
    [PATCH 2/5] ISDN-Gigaset: Improve another size determination in  gigaset_initcs() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:50 +0200
    [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:50 +0200
      Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs()  after an allocation failure Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 23:20 +0200
        Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 07:30 +0200
          Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure Paul Bolle <pebolle@tiscali.nl> - 2016-09-27 10:50 +0200
            Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 11:40 +0200
              Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure Paul Bolle <pebolle@tiscali.nl> - 2016-09-27 11:50 +0200
              Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure Tilman Schmidt <tilman@imap.cc> - 2016-09-27 12:30 +0200
                Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 13:40 +0200
                  Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure Tilman Schmidt <tilman@imap.cc> - 2016-09-27 14:10 +0200
                  Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure isdn@linux-pingi.de - 2016-09-27 14:20 +0200
                    Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 15:00 +0200
                      Re: [Gigaset307x-common] ISDN-Gigaset: Release memory in  gigaset_initcs() after an allocation failure Tilman Schmidt <tilman@imap.cc> - 2016-09-27 16:40 +0200
        Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs()  after an allocation failure Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-27 09:40 +0200
        Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs()  after an allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 17:20 +0200
      Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs()  after an allocation failure Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-27 09:20 +0200
        Re: ISDN-Gigaset: Release memory in gigaset_initcs() after an  allocation failure SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 09:30 +0200
    [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed memory  allocation SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:50 +0200
      Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed  memory allocation Tilman Schmidt <tilman@imap.cc> - 2016-09-27 13:00 +0200
        Re: [PATCH 3/5] ISDN-Gigaset: Delete an error message for a failed  memory allocation Paul Bolle <pebolle@tiscali.nl> - 2016-09-28 13:50 +0200
    [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 17:50 +0200
      RE: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses David Laight <David.Laight@ACULAB.COM> - 2016-09-26 18:10 +0200
        Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses Joe Perches <joe@perches.com> - 2016-09-26 18:30 +0200
          Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 18:50 +0200
            Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses Julia Lawall <julia.lawall@lip6.fr> - 2016-09-26 19:00 +0200
          [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 19:50 +0200
            Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 20:40 +0200
              Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 20:50 +0200
            Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses David Miller <davem@davemloft.net> - 2016-09-27 02:20 +0200
              Re: [PATCH v2 5/5] ISDN-Gigaset: Enclose two expressions for the  sizeof operator by parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 07:40 +0200
      Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-09-26 19:40 +0200
        Re: ISDN-Gigaset: Enclose two expressions for the sizeof operator by  parentheses SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 20:10 +0200
          Re: ISDN-Gigaset: Enclose two expressions for the sizeof operator  by parentheses Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 20:40 +0200
        Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-27 09:10 +0200
          Re: [PATCH 5/5] ISDN-Gigaset: Enclose two expressions for the sizeof  operator by parentheses Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-27 09:30 +0200
    Re: [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function  implementations Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 22:40 +0200
      Re: ISDN-Gigaset: Fine-tuning for three function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-27 07:20 +0200
    Re: [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function  implementations Paul Bolle <pebolle@tiscali.nl> - 2016-09-28 14:00 +0200
      Re: ISDN-Gigaset: Fine-tuning for three function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-28 19:00 +0200
        Re: ISDN-Gigaset: Fine-tuning for three function implementations Paul Bolle <pebolle@tiscali.nl> - 2016-09-28 20:00 +0200

Page 1 of 3  [1] 2 3  Next page →

#1491366 — [PATCH 0/5] ISDN-Gigaset: Fine-tuning for three function implementations

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 26 Sep 2016 17:27:17 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kmalloc_array() in two functions
  Improve another size determination in gigaset_initcs()
  Delete an error message for a failed memory allocation
  Release memory in gigaset_initcs() after an allocation failure
  Enclose two expressions for the sizeof operator by parentheses

 drivers/isdn/gigaset/common.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

-- 
2.10.0

[toc] | [next] | [standalone]

#1491371 — [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 25 Sep 2016 22:22:04 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 7c78144..cecbb6a 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -709,8 +709,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 
 	cs->mode = M_UNKNOWN;
 	cs->mstate = MS_UNINITIALIZED;
-
-	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
+	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
 	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
 		pr_err("out of memory\n");
@@ -1089,8 +1088,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	drv->ops = ops;
 	drv->owner = owner;
 	INIT_LIST_HEAD(&drv->list);
-
-	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
+	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
 	if (!drv->cs)
 		goto error;
 
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

#1492538 — Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

On Mon, 2016-09-26 at 17:38 +0200, SF Markus Elfring wrote:
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kmalloc_array".

Was the current code incorrect? What makes kmalloc_array() better? None
of this is obvious to me.

I'm not going to change code just because some checker suggests to do
so.

>   This issue was detected by using the Coccinelle software.

So? And which coccinelle script was actually used? I couldn't spot a
coccinelle script doing that in the current tree.

> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.

I'm not happy with you mixing this with the above, less trivial,
change.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -709,8 +709,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  
>  	cs->mode = M_UNKNOWN;
>  	cs->mstate = MS_UNINITIALIZED;
> -

Unrelated whitespace change.

> -	cs->bcs = kmalloc(channels * sizeof(struct bc_state), GFP_KERNEL);
> +	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);

For the record: "channels" is basically hardcoded in the three gigaset
hardware drivers.

>  	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
>  	if (!cs->bcs || !cs->inbuf) {
>  		pr_err("out of memory\n");
> @@ -1089,8 +1088,7 @@ struct gigaset_driver
> *gigaset_initdriver(unsigned minor, unsigned minors,
>  	drv->ops = ops;
>  	drv->owner = owner;
>  	INIT_LIST_HEAD(&drv->list);
> -

Again unrelated whitespace change.

> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);

For "minors" the same holds as for "channels", above.

And you snuck in a parentheses change. That should have probably been
merged with 5/5.

>  	if (!drv->cs)
>  		goto error;


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1492742 — Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

>> * Multiplications for the size determination of memory allocations
>>   indicated that array data structures should be processed.
>>   Thus use the corresponding function "kmalloc_array".
> 
> Was the current code incorrect?

I suggest to use a safer interface for array allocations.


> What makes kmalloc_array() better?

1. How do you think about the safety checks that this function provides?

2. Will you be also affected by further software evolution here?
   2016-07-26
   mm: faster kmalloc_array(), kcalloc()
   https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=91c6a05f72a996bee5133e76374ab3ad7d3b9b72


> I'm not going to change code just because some checker suggests to do so.

The script "checkpatch.pl" can point information out like the following.

WARNING: Prefer kmalloc_array over kmalloc with multiply


>>   This issue was detected by using the Coccinelle software.
> 
> So? And which coccinelle script was actually used?

How do you think about to look into related information sources?
https://github.com/coccinelle/coccinelle/issues/81

Would you like to experiment any further with an excerpt?


@replacement1@
expression count, target;
type T;
@@
 target =
-         kmalloc(sizeof(T) * (count)
+         kmalloc_array(count, sizeof(T)
                        , ...);

@replacement2@
expression count, pointer, target;
@@
 target =
-         kmalloc(sizeof(*pointer) * (count)
+         kmalloc_array(count, sizeof(*pointer)
                        , ...);


> I couldn't spot a coccinelle script doing that in the current tree.

This is true for such a software update opportunity.


>> * Replace the specification of a data structure by a pointer dereference
>>   to make the corresponding size determination a bit safer according to
>>   the Linux coding style convention.
> 
> I'm not happy with you mixing this with the above, less trivial, change.

I find that it is a useful combination. - A parameter is adjusted together
with a special function name.


>> -	drv->cs = kmalloc(minors * sizeof *drv->cs, GFP_KERNEL);
>> +	drv->cs = kmalloc_array(minors, sizeof(*drv->cs), GFP_KERNEL);
> 
> For "minors" the same holds as for "channels", above.
> 
> And you snuck in a parentheses change. That should have probably been
> merged with 5/5.

Would you prefer to add them in another update step?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1492795 — Re: [PATCH 1/5] ISDN-Gigaset: Use kmalloc_array() in two functions

On Wed, 2016-09-28 at 18:38 +0200, SF Markus Elfring wrote:
> > I'm not going to change code just because some checker suggests to
> > do so.
> 
> The script "checkpatch.pl" can point information out like the
> following.
> 
> WARNING: Prefer kmalloc_array over kmalloc with multiply

Am I being trolled?


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1491376 — [PATCH 2/5] ISDN-Gigaset: Improve another size determination in gigaset_initcs()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 25 Sep 2016 22:32:14 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/isdn/gigaset/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index cecbb6a..f505b42 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c
@@ -710,7 +710,7 @@ 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);
-	cs->inbuf = kmalloc(sizeof(struct inbuf_t), GFP_KERNEL);
+	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
 		pr_err("out of memory\n");
 		goto error;
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

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

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[toc] | [prev] | [next] | [standalone]

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

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 <elfring@users.sourceforge.net>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

You're in Eliza mode again.

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


Paul Bolle

[toc] | [prev] | [next] | [standalone]

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

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
tilman@imap.cc

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
tilman@imap.cc

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
tilman@imap.cc

[toc] | [prev] | [next] | [standalone]

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

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

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web