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

From: Markus Elfring <redacted>
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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
Date: Mon, 26 Sep 2016 15:35:47 +0200

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <redacted>
---
 drivers/isdn/gigaset/common.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index f505b42..c05a2a4 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c

@@ -712,7 +712,6 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
 	cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
 	cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
 	if (!cs->bcs || !cs->inbuf) {
-		pr_err("out of memory\n");
 		goto error;
 	}
 	++cs->cs_init;

-- 
2.10.0

From: Markus Elfring <redacted>
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 <redacted>
---
 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

From: Markus Elfring <redacted>
Date: Mon, 26 Sep 2016 17:03:56 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

WARNING: sizeof … should be sizeof(…)

Thus fix the affected source code places.

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

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 2e9382f..d901ed7 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);
 	unsigned char *out = outbuf;
 	size_t numin = len;
 

@@ -1079,7 +1079,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	unsigned long flags;
 	unsigned i;
 
-	drv = kmalloc(sizeof *drv, GFP_KERNEL);
+	drv = kmalloc(sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return NULL;
 

-- 
2.10.0

From: SF Markus Elfring

Sent: 26 September 2016 16:45

...

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

WARNING: sizeof … should be sizeof(…)

...

wrong ...
think that is 7 instead of 79.

	David

On Mon, 2016-09-26 at 16:00 +0000, David Laight wrote:

From: SF Markus Elfring Sent: 26 September 2016 16:45

quoted

The script "checkpatch.pl" can point information out like the following.
WARNING: sizeof … should be sizeof(…)

[]

quoted

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c

[]

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,

 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);

wrong ...
think that is 7 instead of 79.

Well, it's sizeof(pointer) so either 4 or 8, but still,
what Markus proposes here is _wrong_.

Markus, do you know the script probably has a lower
style conversion defect introduction rate than you do
when doing these mechanical things.

Please use it to verify what you are doing before
submitting more defective patches.

Excuses and apologies aren't good enough at this point.

quoted

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);

quoted

wrong ...
think that is 7 instead of 79.

Well, it's sizeof(pointer) so either 4 or 8, but still,
what Markus proposes here is _wrong_.

You are right at this place.

Markus, do you know the script probably has a lower
style conversion defect introduction rate than you do
when doing these mechanical things.

Would it be nice if the script "checkpatch.pl" could avoid to point
such a false positive out anyhow?

Please use it to verify what you are doing before
submitting more defective patches.

Do you expect a resend for this update step so that an other statement
in the function "gigaset_initdriver" would eventually be adjusted?

Regards,
Markus

On Mon, 26 Sep 2016, SF Markus Elfring wrote:

quoted

quoted

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);

quoted

wrong ...
think that is 7 instead of 79.

Well, it's sizeof(pointer) so either 4 or 8, but still,
what Markus proposes here is _wrong_.

You are right at this place.

quoted

Markus, do you know the script probably has a lower
style conversion defect introduction rate than you do
when doing these mechanical things.

Would it be nice if the script "checkpatch.pl" could avoid to point
such a false positive out anyhow?

checkpatch reports:

WARNING: sizeof outbuf should be sizeof(outbuf)
#56: FILE: drivers/isdn/gigaset/common.c:56:
+	size_t space = sizeof outbuf - 1;

There is nothing wrong with the report,

julia

quoted

Please use it to verify what you are doing before
submitting more defective patches.

Do you expect a resend for this update step so that an other statement
in the function "gigaset_initdriver" would eventually be adjusted?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello.

On 09/26/2016 06:44 PM, SF Markus Elfring wrote:

   What?! Does that compile?

[...]

MBR, Sergei

From: Markus Elfring <redacted>
Date: Mon, 26 Sep 2016 19:34:27 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

WARNING: sizeof … should be sizeof(…)

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <redacted>
---

v2: Position the desired closing parenthesis behind the variable name for
    a character buffer.

 drivers/isdn/gigaset/common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/gigaset/common.c b/drivers/isdn/gigaset/common.c
index 2e9382f..dba44e1 100644
--- a/drivers/isdn/gigaset/common.c
+++ b/drivers/isdn/gigaset/common.c

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
 	unsigned char outbuf[80];
 	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf) - 1;
 	unsigned char *out = outbuf;
 	size_t numin = len;
 

@@ -1079,7 +1079,7 @@ struct gigaset_driver *gigaset_initdriver(unsigned minor, unsigned minors,
 	unsigned long flags;
 	unsigned i;
 
-	drv = kmalloc(sizeof *drv, GFP_KERNEL);
+	drv = kmalloc(sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return NULL;
 

-- 
2.10.0

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,
 {
     unsigned char outbuf[80];
     unsigned char c;
-    size_t space = sizeof outbuf - 1;
+    size_t space = sizeof(outbuf - 1);

   What?! Does that compile?

Yes, of course. - The calculated value will be lower than intended.

Unfortunately, I stumbled on one of my own programming mistakes once again.


I sent a corrected update suggestion for further considerations a few minutes ago.

Regards,
Markus

On Mon, 2016-09-26 at 19:44 +0200, SF Markus Elfring wrote:

v2: Position the desired closing parenthesis behind the variable name for
    a character buffer.

Please wait a reasonable amount of time (say a week or two) to collect
all feedback on all patches of a series. If there's feedback you should
resend the entire series (minus those patches that have been NAK-ed, of
course) as an update (v2, v3, etc.).

Don't resubmit one single patch of a series (within hours!) as you did
here.

I hope to have a look at your series within a few days. Show some
patience.

Thanks,


Paul Bolle

Where did the [PATCH 5/5] part of the subject go? You didn't drop it,
did you? Because that's surprisingly annoying.


Paul Bolle

Don't resubmit one single patch of a series (within hours!)
as you did here.

I hope that I could reduce the confusion a bit which I introduced
with a change in the last step of my questionable update suggestion.

I hope to have a look at your series within a few days.
Show some patience.

Yes, of course.

Regards,
Markus

On Mon, 2016-09-26 at 17:37 +0200, SF Markus Elfring wrote:

From: Markus Elfring <redacted>
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

Which "static source code analysis" was used for that discovery?

  Enclose two expressions for the sizeof operator by parentheses

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

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

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

When you need to make changes to patches that are part of a series,
you must resubmit the entire series, not just the things that
are changes.

quoted

  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

Which "static source code analysis" was used for that discovery?

Are you eventually asking more for the development tools which were involved here?

* Coccinelle software

* Script "checkpatch.pl"

* My own eyes with help of a current text editor and its programming support

Regards,
Markus

quoted

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

When you need to make changes to patches that are part of a series,
you must resubmit the entire series,

I imagine that will happen when the patch review time passed by a bit
more as Paul Bolle requested it yesterday.

not just the things that are changes.

Thanks for your reminder.

Regards,
Markus

On Mon, Sep 26, 2016 at 08:38:14PM +0300, Sergei Shtylyov wrote:

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,

{
	unsigned char outbuf[80];
	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);

   What?! Does that compile?

[...]

It prints a Smatch warning.  Smatch ignores these if they happen inside
a macro where you pass a pointer and it takes the sizeof() the argument.

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

On Tue, Sep 27, 2016 at 10:08:37AM +0300, Dan Carpenter wrote:

On Mon, Sep 26, 2016 at 08:38:14PM +0300, Sergei Shtylyov wrote:

quoted

quoted

@@ -53,7 +53,7 @@ void gigaset_dbg_buffer(enum debuglevel level, const unsigned char *msg,

{
	unsigned char outbuf[80];
	unsigned char c;
-	size_t space = sizeof outbuf - 1;
+	size_t space = sizeof(outbuf - 1);

   What?! Does that compile?

[...]

It prints a Smatch warning.  Smatch ignores these if they happen inside
a macro where you pass a pointer and it takes the sizeof() the argument.

Reading that again, I realize it's not clear.  Smatch ignores these any
time they happen in a macro whether they're valid or not.  (Many times
they are valid).

regards,
dan carpenter

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

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

regards,
dan carpenter

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

quoted

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

You're in Eliza mode again.

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


Paul Bolle

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:

quoted

quoted

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

On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote:

From: Markus Elfring <redacted>
Date: Mon, 26 Sep 2016 15:35:47 +0200

Omit an extra message for a memory allocation failure in this function.

Link:
http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <redacted>

The patch is fine but the link in the commit message is irrelevant.
Please remove it.
(Yes, I read through the whole presentation to verify that. It was fun,
even.)

-- 
Tilman Schmidt
tilman@imap.cc

quoted

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

On Tue, Sep 27, 2016, at 13:32, SF Markus Elfring wrote:

quoted

quoted

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

Am 27.09.2016 um 13:32 schrieb SF Markus Elfring:

quoted

quoted

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

quoted

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

quoted

* 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

On Tue, Sep 27, 2016, at 14:52, SF Markus Elfring wrote:

quoted

quoted

* 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

quoted

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

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

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.

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

On Tue, 2016-09-27 at 12:57 +0200, Tilman Schmidt wrote:

On Mon, Sep 26, 2016, at 17:42, SF Markus Elfring wrote:

quoted

Omit an extra message for a memory allocation failure in this
function.

Link:
http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-R
efactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <redacted>

The patch is fine but the link in the commit message is irrelevant.
Please remove it.
(Yes, I read through the whole presentation to verify that. It was fun,
even.)

Agree.

Except this patch assumes the superfluous braces would be removed in
4/5. But it turns out that other patch must be dropped. It would have
been better to remove the braces in this patch.


Paul Bolle

On Mon, 2016-09-26 at 17:37 +0200, SF Markus Elfring wrote:

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

Two of the five patches introduced bugs. The rest of the series isn't
free of various nits either. Of course, I was in no mood to be lenient
when I looked at those three patches.

I won't take any of these patches, sorry.


Paul Bolle

quoted

* 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

quoted

  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.

quoted

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

quoted

-	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

Two of the five patches introduced bugs. The rest of the series isn't
free of various nits either. Of course, I was in no mood to be lenient
when I looked at those three patches.

I won't take any of these patches, sorry.

Would you like to look once more into an improved patch series for this
software module a bit later?

Regards,
Markus

On Wed, 2016-09-28 at 18:38 +0200, SF Markus Elfring wrote:

quoted

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

On Wed, 2016-09-28 at 18:50 +0200, SF Markus Elfring wrote:

Would you like to look once more into an improved patch series for this
software module a bit later?

I'm afraid I'm not looking forward to receiving an update of this
series, sorry.

Thanks,


Paul Bolle