* [PATCH 0/5] staging-COMEDI: Fine-tuning for three functions
@ 2016-12-08 11:30 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:30 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 11:37:37 +0100
Some update suggestions were taken into account
from static source code analysis.
Markus Elfring (5):
Combine four kcalloc() calls into one in serial2002_setup_subdevs()
Split a condition check in usbdux_alloc_usb_buffers()
Move an assignment in usbdux_alloc_usb_buffers()
Split a condition check in usbduxsigma_alloc_usb_buffers()
Move an assignment in usbduxsigma_alloc_usb_buffers()
drivers/staging/comedi/drivers/serial2002.c | 22 +++++-----
drivers/staging/comedi/drivers/usbdux.c | 56 ++++++++++++++++++-----
drivers/staging/comedi/drivers/usbduxsigma.c | 66 ++++++++++++++++++++++------
3 files changed, 108 insertions(+), 36 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_s
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 11:33 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:33 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 07:37:29 +0100
The function "kcalloc" was called in three cases by the function
"serial2002_setup_subdevs" without checking immediately if it failed.
This issue was detected by using the Coccinelle software.
* Perform the desired memory allocation (and release at the end) by a single
function call instead.
* Adjust a jump target so that a redundant check is avoided.
Fixes: 623a73926c7012e3bb132e225621890207f5c611 ("staging: comedi: serial2002: split up serial_2002_open()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/serial2002.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
index 0d33e520f635..9542f4f8afe0 100644
--- a/drivers/staging/comedi/drivers/serial2002.c
+++ b/drivers/staging/comedi/drivers/serial2002.c
@@ -392,6 +392,7 @@ static int serial2002_setup_subdevice(struct comedi_subdevice *s,
static int serial2002_setup_subdevs(struct comedi_device *dev)
{
struct serial2002_private *devpriv = dev->private;
+ struct config_t *array;
struct config_t *di_cfg;
struct config_t *do_cfg;
struct config_t *ai_cfg;
@@ -402,15 +403,17 @@ static int serial2002_setup_subdevs(struct comedi_device *dev)
int i;
/* Allocate the temporary structs to hold the configuration data */
- di_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- do_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- ai_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- ao_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- if (!di_cfg || !do_cfg || !ai_cfg || !ao_cfg) {
+ array = kcalloc(4 * 32, sizeof(*cfg), GFP_KERNEL);
+ if (!array) {
result = -ENOMEM;
- goto err_alloc_configs;
+ goto check_tty;
}
+ di_cfg = array;
+ do_cfg = array + 1 * 32;
+ ai_cfg = array + 2 * 32;
+ ao_cfg = array + 3 * 32;
+
/* Read the configuration from the connected device */
serial2002_tty_setspeed(devpriv->tty, devpriv->speed);
serial2002_poll_channel(devpriv->tty, 31);
@@ -534,13 +537,10 @@ static int serial2002_setup_subdevs(struct comedi_device *dev)
}
}
-err_alloc_configs:
- kfree(di_cfg);
- kfree(do_cfg);
- kfree(ai_cfg);
- kfree(ao_cfg);
+ kfree(array);
if (result) {
+check_tty:
if (devpriv->tty) {
filp_close(devpriv->tty, NULL);
devpriv->tty = NULL;
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs()
@ 2016-12-08 11:33 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:33 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 07:37:29 +0100
The function "kcalloc" was called in three cases by the function
"serial2002_setup_subdevs" without checking immediately if it failed.
This issue was detected by using the Coccinelle software.
* Perform the desired memory allocation (and release at the end) by a single
function call instead.
* Adjust a jump target so that a redundant check is avoided.
Fixes: 623a73926c7012e3bb132e225621890207f5c611 ("staging: comedi: serial2002: split up serial_2002_open()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/serial2002.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
index 0d33e520f635..9542f4f8afe0 100644
--- a/drivers/staging/comedi/drivers/serial2002.c
+++ b/drivers/staging/comedi/drivers/serial2002.c
@@ -392,6 +392,7 @@ static int serial2002_setup_subdevice(struct comedi_subdevice *s,
static int serial2002_setup_subdevs(struct comedi_device *dev)
{
struct serial2002_private *devpriv = dev->private;
+ struct config_t *array;
struct config_t *di_cfg;
struct config_t *do_cfg;
struct config_t *ai_cfg;
@@ -402,15 +403,17 @@ static int serial2002_setup_subdevs(struct comedi_device *dev)
int i;
/* Allocate the temporary structs to hold the configuration data */
- di_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- do_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- ai_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- ao_cfg = kcalloc(32, sizeof(*cfg), GFP_KERNEL);
- if (!di_cfg || !do_cfg || !ai_cfg || !ao_cfg) {
+ array = kcalloc(4 * 32, sizeof(*cfg), GFP_KERNEL);
+ if (!array) {
result = -ENOMEM;
- goto err_alloc_configs;
+ goto check_tty;
}
+ di_cfg = array;
+ do_cfg = array + 1 * 32;
+ ai_cfg = array + 2 * 32;
+ ao_cfg = array + 3 * 32;
+
/* Read the configuration from the connected device */
serial2002_tty_setspeed(devpriv->tty, devpriv->speed);
serial2002_poll_channel(devpriv->tty, 31);
@@ -534,13 +537,10 @@ static int serial2002_setup_subdevs(struct comedi_device *dev)
}
}
-err_alloc_configs:
- kfree(di_cfg);
- kfree(do_cfg);
- kfree(ai_cfg);
- kfree(ao_cfg);
+ kfree(array);
if (result) {
+check_tty:
if (devpriv->tty) {
filp_close(devpriv->tty, NULL);
devpriv->tty = NULL;
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_set
2016-12-08 11:33 ` [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs() SF Markus Elfring
@ 2016-12-08 12:22 ` Dan Carpenter
-1 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:22 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, kernel-janitors, LKML
The original code was simpler.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 11:34 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:34 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 10:01:54 +0100
The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbdux_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.
Allocated memory was also not released if one of these function
calls failed.
* Split a condition check for memory allocation failures.
* Add more exception handling.
Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..d7d683bd669c 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
- int i;
+ int i, x;
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+ if (!devpriv->dux_commands)
+ return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+ if (!devpriv->in_buf)
+ goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
+ if (!devpriv->insn_buf)
+ goto free_in_buf;
+
devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
GFP_KERNEL);
+ if (!devpriv->ai_urbs)
+ goto free_insn_buf;
+
devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
GFP_KERNEL);
- if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
- !devpriv->ai_urbs || !devpriv->ao_urbs)
- return -ENOMEM;
+ if (!devpriv->ao_urbs)
+ goto free_ai_urbs;
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
urb->dev = usb;
@@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ai_urbs;
urb->complete = usbduxsub_ai_isoc_irq;
urb->number_of_packets = 1;
@@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
urb->dev = usb;
@@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ao_urbs;
urb->complete = usbduxsub_ao_isoc_irq;
urb->number_of_packets = 1;
@@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
/* max bulk ep size in high speed */
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_pwm_urb;
}
return 0;
+free_pwm_urb:
+ usb_free_urb(urb);
+free_n_ao_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ao_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ao_urbs[x]);
+ }
+free_n_ai_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ai_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ai_urbs[x]);
+ }
+ kfree(devpriv->ao_urbs);
+free_ai_urbs:
+ kfree(devpriv->ai_urbs);
+free_insn_buf:
+ kfree(devpriv->insn_buf);
+free_in_buf:
+ kfree(devpriv->in_buf);
+free_commands:
+ kfree(devpriv->dux_commands);
+ return -ENOMEM;
}
static void usbdux_free_usb_buffers(struct comedi_device *dev)
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
@ 2016-12-08 11:34 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:34 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 10:01:54 +0100
The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbdux_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.
Allocated memory was also not released if one of these function
calls failed.
* Split a condition check for memory allocation failures.
* Add more exception handling.
Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
index f4f05d29d30d..d7d683bd669c 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
- int i;
+ int i, x;
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+ if (!devpriv->dux_commands)
+ return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+ if (!devpriv->in_buf)
+ goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
+ if (!devpriv->insn_buf)
+ goto free_in_buf;
+
devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
GFP_KERNEL);
+ if (!devpriv->ai_urbs)
+ goto free_insn_buf;
+
devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
GFP_KERNEL);
- if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
- !devpriv->ai_urbs || !devpriv->ao_urbs)
- return -ENOMEM;
+ if (!devpriv->ao_urbs)
+ goto free_ai_urbs;
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
urb->dev = usb;
@@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ai_urbs;
urb->complete = usbduxsub_ai_isoc_irq;
urb->number_of_packets = 1;
@@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
urb->dev = usb;
@@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ao_urbs;
urb->complete = usbduxsub_ao_isoc_irq;
urb->number_of_packets = 1;
@@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
/* max bulk ep size in high speed */
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_pwm_urb;
}
return 0;
+free_pwm_urb:
+ usb_free_urb(urb);
+free_n_ao_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ao_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ao_urbs[x]);
+ }
+free_n_ai_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ai_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ai_urbs[x]);
+ }
+ kfree(devpriv->ao_urbs);
+free_ai_urbs:
+ kfree(devpriv->ai_urbs);
+free_insn_buf:
+ kfree(devpriv->insn_buf);
+free_in_buf:
+ kfree(devpriv->in_buf);
+free_commands:
+ kfree(devpriv->dux_commands);
+ return -ENOMEM;
}
static void usbdux_free_usb_buffers(struct comedi_device *dev)
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
2016-12-08 11:34 ` SF Markus Elfring
@ 2016-12-08 12:35 ` Dan Carpenter
-1 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:35 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, LKML, kernel-janitors
On Thu, Dec 08, 2016 at 12:34:27PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 10:01:54 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbdux_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index f4f05d29d30d..d7d683bd669c 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> struct usb_device *usb = comedi_to_usb_dev(dev);
> struct usbdux_private *devpriv = dev->private;
> struct urb *urb;
> - int i;
> + int i, x;
>
> devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
> + if (!devpriv->dux_commands)
> + return -ENOMEM;
> +
> devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
> + if (!devpriv->in_buf)
> + goto free_commands;
> +
> devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
> + if (!devpriv->insn_buf)
> + goto free_in_buf;
> +
> devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
> GFP_KERNEL);
> + if (!devpriv->ai_urbs)
> + goto free_insn_buf;
> +
> devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
> GFP_KERNEL);
> - if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
> - !devpriv->ai_urbs || !devpriv->ao_urbs)
> - return -ENOMEM;
> + if (!devpriv->ao_urbs)
> + goto free_ai_urbs;
>
> for (i = 0; i < devpriv->n_ai_urbs; i++) {
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
> devpriv->ai_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>
> urb->complete = usbduxsub_ai_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->ao_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>
> urb->complete = usbduxsub_ao_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> if (devpriv->pwm_buf_sz) {
> urb = usb_alloc_urb(0, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->pwm_urb = urb;
>
> /* max bulk ep size in high speed */
> urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
> GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_pwm_urb;
> }
>
> return 0;
> +free_pwm_urb:
> + usb_free_urb(urb);
> +free_n_ao_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ao_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ao_urbs[x]);
> + }
> +free_n_ai_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ai_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ai_urbs[x]);
> + }
This is buggy. We re-use i for two loops so if it fails part way
through allocating ->ao_urbs[] then we don't free all the ->ai_urbs[].
Also the use of "x" as a generic iterator name is not idiomatic.
Better to change the first loop to use n_ai and the second to use n_ao
as iterators. Then use i as an iterator to free them.
regards,
dan carpenter
> + kfree(devpriv->ao_urbs);
> +free_ai_urbs:
> + kfree(devpriv->ai_urbs);
> +free_insn_buf:
> + kfree(devpriv->insn_buf);
> +free_in_buf:
> + kfree(devpriv->in_buf);
> +free_commands:
> + kfree(devpriv->dux_commands);
> + return -ENOMEM;
> }
>
> static void usbdux_free_usb_buffers(struct comedi_device *dev)
> --
> 2.11.0
>
> --
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
@ 2016-12-08 12:35 ` Dan Carpenter
0 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:35 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, LKML, kernel-janitors
On Thu, Dec 08, 2016 at 12:34:27PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 10:01:54 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbdux_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index f4f05d29d30d..d7d683bd669c 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> struct usb_device *usb = comedi_to_usb_dev(dev);
> struct usbdux_private *devpriv = dev->private;
> struct urb *urb;
> - int i;
> + int i, x;
>
> devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
> + if (!devpriv->dux_commands)
> + return -ENOMEM;
> +
> devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
> + if (!devpriv->in_buf)
> + goto free_commands;
> +
> devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
> + if (!devpriv->insn_buf)
> + goto free_in_buf;
> +
> devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
> GFP_KERNEL);
> + if (!devpriv->ai_urbs)
> + goto free_insn_buf;
> +
> devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
> GFP_KERNEL);
> - if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
> - !devpriv->ai_urbs || !devpriv->ao_urbs)
> - return -ENOMEM;
> + if (!devpriv->ao_urbs)
> + goto free_ai_urbs;
>
> for (i = 0; i < devpriv->n_ai_urbs; i++) {
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
> devpriv->ai_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>
> urb->complete = usbduxsub_ai_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->ao_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>
> urb->complete = usbduxsub_ao_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> if (devpriv->pwm_buf_sz) {
> urb = usb_alloc_urb(0, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->pwm_urb = urb;
>
> /* max bulk ep size in high speed */
> urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
> GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_pwm_urb;
> }
>
> return 0;
> +free_pwm_urb:
> + usb_free_urb(urb);
> +free_n_ao_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ao_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ao_urbs[x]);
> + }
> +free_n_ai_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ai_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ai_urbs[x]);
> + }
This is buggy. We re-use i for two loops so if it fails part way
through allocating ->ao_urbs[] then we don't free all the ->ai_urbs[].
Also the use of "x" as a generic iterator name is not idiomatic.
Better to change the first loop to use n_ai and the second to use n_ao
as iterators. Then use i as an iterator to free them.
regards,
dan carpenter
> + kfree(devpriv->ao_urbs);
> +free_ai_urbs:
> + kfree(devpriv->ai_urbs);
> +free_insn_buf:
> + kfree(devpriv->insn_buf);
> +free_in_buf:
> + kfree(devpriv->in_buf);
> +free_commands:
> + kfree(devpriv->dux_commands);
> + return -ENOMEM;
> }
>
> static void usbdux_free_usb_buffers(struct comedi_device *dev)
> --
> 2.11.0
>
> --
> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
2016-12-08 11:34 ` SF Markus Elfring
@ 2016-12-08 12:37 ` Dan Carpenter
-1 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:37 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, LKML, kernel-janitors
Same bug.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
2016-12-08 11:34 ` SF Markus Elfring
@ 2016-12-08 12:44 ` Ian Abbott
-1 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 12:44 UTC (permalink / raw)
To: SF Markus Elfring, devel, Chris Cesare, Greg Kroah-Hartman,
H Hartley Sweeten
Cc: LKML, kernel-janitors
On 08/12/16 11:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 10:01:54 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbdux_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index f4f05d29d30d..d7d683bd669c 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> struct usb_device *usb = comedi_to_usb_dev(dev);
> struct usbdux_private *devpriv = dev->private;
> struct urb *urb;
> - int i;
> + int i, x;
>
> devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
> + if (!devpriv->dux_commands)
> + return -ENOMEM;
> +
> devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
> + if (!devpriv->in_buf)
> + goto free_commands;
> +
> devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
> + if (!devpriv->insn_buf)
> + goto free_in_buf;
> +
> devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
> GFP_KERNEL);
> + if (!devpriv->ai_urbs)
> + goto free_insn_buf;
> +
> devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
> GFP_KERNEL);
> - if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
> - !devpriv->ai_urbs || !devpriv->ao_urbs)
> - return -ENOMEM;
> + if (!devpriv->ao_urbs)
> + goto free_ai_urbs;
>
> for (i = 0; i < devpriv->n_ai_urbs; i++) {
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
> devpriv->ai_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>
> urb->complete = usbduxsub_ai_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->ao_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>
> urb->complete = usbduxsub_ao_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> if (devpriv->pwm_buf_sz) {
> urb = usb_alloc_urb(0, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->pwm_urb = urb;
>
> /* max bulk ep size in high speed */
> urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
> GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_pwm_urb;
> }
>
> return 0;
> +free_pwm_urb:
> + usb_free_urb(urb);
> +free_n_ao_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ao_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ao_urbs[x]);
> + }
> +free_n_ai_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ai_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ai_urbs[x]);
> + }
> + kfree(devpriv->ao_urbs);
> +free_ai_urbs:
> + kfree(devpriv->ai_urbs);
> +free_insn_buf:
> + kfree(devpriv->insn_buf);
> +free_in_buf:
> + kfree(devpriv->in_buf);
> +free_commands:
> + kfree(devpriv->dux_commands);
> + return -ENOMEM;
> }
>
> static void usbdux_free_usb_buffers(struct comedi_device *dev)
>
Actually, the original code worked fine, and these changes will result
in an Oops if the allocations fail. I'll explain why, since it isn't
obvious without some knowledge of the clean-up strategy used by comedi
drivers:
1. usbdux_alloc_usb_buffers() is called from usbdux_auto_attach().
2. usbdux_auto_attach() will return an error if
usbdux_alloc_usb_buffers() fails.
3. If usbdux_auto_attach() returns an error to the core comedi module,
the core comedi module will call usbdux_detach().
4. usbdux_detach() calls usbdux_free_usb_buffers().
5. usbdux_free_usb_buffers() frees any of the buffers that were
successfully allocated by usbdux_alloc_usb_buffers().
The net result is that devpriv->dux_commands and the others will be
passed to kfree() twice, leading to the oops. That could be prevented
by settting devpriv->dux_commands and the others to NULL in the error
handling of usbdux_alloc_usb_buffers(), but it really isn't necessary as
the existing code works, and all the other comedi drivers follow the
same strategy of leaving clean-up to their comedi 'detach' handler.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
@ 2016-12-08 12:44 ` Ian Abbott
0 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 12:44 UTC (permalink / raw)
To: SF Markus Elfring, devel, Chris Cesare, Greg Kroah-Hartman,
H Hartley Sweeten
Cc: LKML, kernel-janitors
On 08/12/16 11:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 10:01:54 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbdux_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: ef1e3c4a3b383c6da3979670fcb5c6e9c7de4741 ("staging: comedi: usbdux: tidy up usbdux_alloc_usb_buffers()")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbdux.c | 53 ++++++++++++++++++++++++++-------
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
> index f4f05d29d30d..d7d683bd669c 100644
> --- a/drivers/staging/comedi/drivers/usbdux.c
> +++ b/drivers/staging/comedi/drivers/usbdux.c
> @@ -1449,24 +1449,35 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> struct usb_device *usb = comedi_to_usb_dev(dev);
> struct usbdux_private *devpriv = dev->private;
> struct urb *urb;
> - int i;
> + int i, x;
>
> devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
> + if (!devpriv->dux_commands)
> + return -ENOMEM;
> +
> devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
> + if (!devpriv->in_buf)
> + goto free_commands;
> +
> devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
> + if (!devpriv->insn_buf)
> + goto free_in_buf;
> +
> devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(void *),
> GFP_KERNEL);
> + if (!devpriv->ai_urbs)
> + goto free_insn_buf;
> +
> devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(void *),
> GFP_KERNEL);
> - if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
> - !devpriv->ai_urbs || !devpriv->ao_urbs)
> - return -ENOMEM;
> + if (!devpriv->ao_urbs)
> + goto free_ai_urbs;
>
> for (i = 0; i < devpriv->n_ai_urbs; i++) {
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
> devpriv->ai_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1475,7 +1486,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ai_urbs;
>
> urb->complete = usbduxsub_ai_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1488,7 +1499,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> /* one frame: 1ms */
> urb = usb_alloc_urb(1, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->ao_urbs[i] = urb;
>
> urb->dev = usb;
> @@ -1497,7 +1508,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
>
> urb->complete = usbduxsub_ao_isoc_irq;
> urb->number_of_packets = 1;
> @@ -1514,17 +1525,39 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
> if (devpriv->pwm_buf_sz) {
> urb = usb_alloc_urb(0, GFP_KERNEL);
> if (!urb)
> - return -ENOMEM;
> + goto free_n_ao_urbs;
> devpriv->pwm_urb = urb;
>
> /* max bulk ep size in high speed */
> urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
> GFP_KERNEL);
> if (!urb->transfer_buffer)
> - return -ENOMEM;
> + goto free_pwm_urb;
> }
>
> return 0;
> +free_pwm_urb:
> + usb_free_urb(urb);
> +free_n_ao_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ao_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ao_urbs[x]);
> + }
> +free_n_ai_urbs:
> + for (x = 0; x < i; ++x) {
> + kfree(devpriv->ai_urbs[x]->transfer_buffer);
> + usb_free_urb(devpriv->ai_urbs[x]);
> + }
> + kfree(devpriv->ao_urbs);
> +free_ai_urbs:
> + kfree(devpriv->ai_urbs);
> +free_insn_buf:
> + kfree(devpriv->insn_buf);
> +free_in_buf:
> + kfree(devpriv->in_buf);
> +free_commands:
> + kfree(devpriv->dux_commands);
> + return -ENOMEM;
> }
>
> static void usbdux_free_usb_buffers(struct comedi_device *dev)
>
Actually, the original code worked fine, and these changes will result
in an Oops if the allocations fail. I'll explain why, since it isn't
obvious without some knowledge of the clean-up strategy used by comedi
drivers:
1. usbdux_alloc_usb_buffers() is called from usbdux_auto_attach().
2. usbdux_auto_attach() will return an error if
usbdux_alloc_usb_buffers() fails.
3. If usbdux_auto_attach() returns an error to the core comedi module,
the core comedi module will call usbdux_detach().
4. usbdux_detach() calls usbdux_free_usb_buffers().
5. usbdux_free_usb_buffers() frees any of the buffers that were
successfully allocated by usbdux_alloc_usb_buffers().
The net result is that devpriv->dux_commands and the others will be
passed to kfree() twice, leading to the oops. That could be prevented
by settting devpriv->dux_commands and the others to NULL in the error
handling of usbdux_alloc_usb_buffers(), but it really isn't necessary as
the existing code works, and all the other comedi drivers follow the
same strategy of leaving clean-up to their comedi 'detach' handler.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
2016-12-08 12:44 ` Ian Abbott
@ 2016-12-08 15:43 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:43 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
> Actually, the original code worked fine,
I got my doubts when some memory allocations are attempted without checking
the desired success immediately.
> and these changes will result in an Oops if the allocations fail. I'll explain why,
> since it isn't obvious without some knowledge of the clean-up strategy used by comedi drivers:
Thanks for your explanation.
> …, and all the other comedi drivers follow the same strategy of leaving clean-up
> to their comedi 'detach' handler.
Are there other source code parts worth for further considerations?
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()
@ 2016-12-08 15:43 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:43 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
> Actually, the original code worked fine,
I got my doubts when some memory allocations are attempted without checking
the desired success immediately.
> and these changes will result in an Oops if the allocations fail. I'll explain why,
> since it isn't obvious without some knowledge of the clean-up strategy used by comedi drivers:
Thanks for your explanation.
> …, and all the other comedi drivers follow the same strategy of leaving clean-up
> to their comedi 'detach' handler.
Are there other source code parts worth for further considerations?
Regards,
Markus
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 11:35 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:35 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 10:13:56 +0100
Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbdux.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
index d7d683bd669c..7efeac71161b 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1446,7 +1446,7 @@ static int usbdux_firmware_upload(struct comedi_device *dev,
static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
{
- struct usb_device *usb = comedi_to_usb_dev(dev);
+ struct usb_device *usb;
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1473,6 +1473,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
+ usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()
@ 2016-12-08 11:35 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:35 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 10:13:56 +0100
Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbdux.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbdux.c b/drivers/staging/comedi/drivers/usbdux.c
index d7d683bd669c..7efeac71161b 100644
--- a/drivers/staging/comedi/drivers/usbdux.c
+++ b/drivers/staging/comedi/drivers/usbdux.c
@@ -1446,7 +1446,7 @@ static int usbdux_firmware_upload(struct comedi_device *dev,
static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
{
- struct usb_device *usb = comedi_to_usb_dev(dev);
+ struct usb_device *usb;
struct usbdux_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1473,6 +1473,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
+ usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()
2016-12-08 11:35 ` SF Markus Elfring
@ 2016-12-08 12:40 ` Dan Carpenter
-1 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:40 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, LKML, kernel-janitors
This one is pointless. It's just a style issue that you invented. Only
Joe Perches is allowed to invent new style guidelines.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()
@ 2016-12-08 12:40 ` Dan Carpenter
0 siblings, 0 replies; 34+ messages in thread
From: Dan Carpenter @ 2016-12-08 12:40 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott, LKML, kernel-janitors
This one is pointless. It's just a style issue that you invented. Only
Joe Perches is allowed to invent new style guidelines.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 11:37 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:37 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 11:15:40 +0100
The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbduxsigma_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.
Allocated memory was also not released if one of these function
calls failed.
* Reduce memory allocation sizes for two function calls.
* Split a condition check for memory allocation failures.
* Add more exception handling.
Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: factor usb buffer allocation out of (*probe)")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++++++++++++++++++++++------
1 file changed, 49 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 456e9f13becb..8c04aa5339f3 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1341,22 +1341,37 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
- int i;
+ int i, x;
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+ if (!devpriv->dux_commands)
+ return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+ if (!devpriv->in_buf)
+ goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
- devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(urb), GFP_KERNEL);
- devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(urb), GFP_KERNEL);
- if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
- !devpriv->ai_urbs || !devpriv->ao_urbs)
- return -ENOMEM;
+ if (!devpriv->insn_buf)
+ goto free_in_buf;
+
+ devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs,
+ sizeof(*devpriv->ai_urbs),
+ GFP_KERNEL);
+ if (!devpriv->ai_urbs)
+ goto free_insn_buf;
+
+ devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs,
+ sizeof(*devpriv->ao_urbs),
+ GFP_KERNEL);
+ if (!devpriv->ao_urbs)
+ goto free_ai_urbs;
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1366,7 +1381,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ai_urbs;
urb->complete = usbduxsigma_ai_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEINBUF;
@@ -1378,7 +1393,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1388,7 +1403,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ao_urbs;
urb->complete = usbduxsigma_ao_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEOUTBUF;
@@ -1400,16 +1415,38 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_pwm_urb;
}
return 0;
+free_pwm_urb:
+ usb_free_urb(urb);
+free_n_ao_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ao_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ao_urbs[x]);
+ }
+free_n_ai_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ai_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ai_urbs[x]);
+ }
+ kfree(devpriv->ao_urbs);
+free_ai_urbs:
+ kfree(devpriv->ai_urbs);
+free_insn_buf:
+ kfree(devpriv->insn_buf);
+free_in_buf:
+ kfree(devpriv->in_buf);
+free_commands:
+ kfree(devpriv->dux_commands);
+ return -ENOMEM;
}
static void usbduxsigma_free_usb_buffers(struct comedi_device *dev)
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
@ 2016-12-08 11:37 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:37 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 11:15:40 +0100
The functions "kcalloc" and "kzalloc" were called in four cases by the
function "usbduxsigma_alloc_usb_buffers" without checking immediately
if they succeded.
This issue was detected by using the Coccinelle software.
Allocated memory was also not released if one of these function
calls failed.
* Reduce memory allocation sizes for two function calls.
* Split a condition check for memory allocation failures.
* Add more exception handling.
Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: factor usb buffer allocation out of (*probe)")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++++++++++++++++++++++------
1 file changed, 49 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 456e9f13becb..8c04aa5339f3 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1341,22 +1341,37 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
struct usb_device *usb = comedi_to_usb_dev(dev);
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
- int i;
+ int i, x;
devpriv->dux_commands = kzalloc(SIZEOFDUXBUFFER, GFP_KERNEL);
+ if (!devpriv->dux_commands)
+ return -ENOMEM;
+
devpriv->in_buf = kzalloc(SIZEINBUF, GFP_KERNEL);
+ if (!devpriv->in_buf)
+ goto free_commands;
+
devpriv->insn_buf = kzalloc(SIZEINSNBUF, GFP_KERNEL);
- devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs, sizeof(urb), GFP_KERNEL);
- devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs, sizeof(urb), GFP_KERNEL);
- if (!devpriv->dux_commands || !devpriv->in_buf || !devpriv->insn_buf ||
- !devpriv->ai_urbs || !devpriv->ao_urbs)
- return -ENOMEM;
+ if (!devpriv->insn_buf)
+ goto free_in_buf;
+
+ devpriv->ai_urbs = kcalloc(devpriv->n_ai_urbs,
+ sizeof(*devpriv->ai_urbs),
+ GFP_KERNEL);
+ if (!devpriv->ai_urbs)
+ goto free_insn_buf;
+
+ devpriv->ao_urbs = kcalloc(devpriv->n_ao_urbs,
+ sizeof(*devpriv->ao_urbs),
+ GFP_KERNEL);
+ if (!devpriv->ao_urbs)
+ goto free_ai_urbs;
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ai_urbs;
devpriv->ai_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1366,7 +1381,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ai_urbs;
urb->complete = usbduxsigma_ai_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEINBUF;
@@ -1378,7 +1393,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->ao_urbs[i] = urb;
urb->dev = usb;
/* will be filled later with a pointer to the comedi-device */
@@ -1388,7 +1403,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
urb->transfer_flags = URB_ISO_ASAP;
urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_n_ao_urbs;
urb->complete = usbduxsigma_ao_urb_complete;
urb->number_of_packets = 1;
urb->transfer_buffer_length = SIZEOUTBUF;
@@ -1400,16 +1415,38 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
if (devpriv->pwm_buf_sz) {
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
- return -ENOMEM;
+ goto free_n_ao_urbs;
devpriv->pwm_urb = urb;
urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
GFP_KERNEL);
if (!urb->transfer_buffer)
- return -ENOMEM;
+ goto free_pwm_urb;
}
return 0;
+free_pwm_urb:
+ usb_free_urb(urb);
+free_n_ao_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ao_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ao_urbs[x]);
+ }
+free_n_ai_urbs:
+ for (x = 0; x < i; ++x) {
+ kfree(devpriv->ai_urbs[x]->transfer_buffer);
+ usb_free_urb(devpriv->ai_urbs[x]);
+ }
+ kfree(devpriv->ao_urbs);
+free_ai_urbs:
+ kfree(devpriv->ai_urbs);
+free_insn_buf:
+ kfree(devpriv->insn_buf);
+free_in_buf:
+ kfree(devpriv->in_buf);
+free_commands:
+ kfree(devpriv->dux_commands);
+ return -ENOMEM;
}
static void usbduxsigma_free_usb_buffers(struct comedi_device *dev)
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffe
2016-12-08 11:37 ` SF Markus Elfring
@ 2016-12-08 12:51 ` Ian Abbott
-1 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 12:51 UTC (permalink / raw)
To: SF Markus Elfring, devel, Chris Cesare, Greg Kroah-Hartman,
H Hartley Sweeten
Cc: LKML, kernel-janitors
On 08/12/16 11:37, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 11:15:40 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbduxsigma_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Reduce memory allocation sizes for two function calls.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: factor usb buffer allocation out of (*probe)")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++++++++++++++++++++++------
> 1 file changed, 49 insertions(+), 12 deletions(-)
My comments on PATCH 2/5 about how comedi drivers handle clean-up apply
to this patch too. So this patch will cause an Oops in the unlikely
event of running out of memory during buffer allocation.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
@ 2016-12-08 12:51 ` Ian Abbott
0 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 12:51 UTC (permalink / raw)
To: SF Markus Elfring, devel, Chris Cesare, Greg Kroah-Hartman,
H Hartley Sweeten
Cc: LKML, kernel-janitors
On 08/12/16 11:37, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 11:15:40 +0100
>
> The functions "kcalloc" and "kzalloc" were called in four cases by the
> function "usbduxsigma_alloc_usb_buffers" without checking immediately
> if they succeded.
> This issue was detected by using the Coccinelle software.
>
> Allocated memory was also not released if one of these function
> calls failed.
>
> * Reduce memory allocation sizes for two function calls.
>
> * Split a condition check for memory allocation failures.
>
> * Add more exception handling.
>
> Fixes: 65989c030bbca96be45ed137f6384dbd46030d10 ("staging: comedi: usbduxsigma: factor usb buffer allocation out of (*probe)")
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbduxsigma.c | 61 ++++++++++++++++++++++------
> 1 file changed, 49 insertions(+), 12 deletions(-)
My comments on PATCH 2/5 about how comedi drivers handle clean-up apply
to this patch too. So this patch will cause an Oops in the unlikely
event of running out of memory during buffer allocation.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
2016-12-08 12:51 ` [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers() Ian Abbott
@ 2016-12-08 15:46 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:46 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
>> * Reduce memory allocation sizes for two function calls.
Is this implementation detail worth for further considerations?
Regards,
Markus
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
@ 2016-12-08 15:46 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:46 UTC (permalink / raw)
To: Ian Abbott
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
>> * Reduce memory allocation sizes for two function calls.
Is this implementation detail worth for further considerations?
Regards,
Markus
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
2016-12-08 15:46 ` SF Markus Elfring
@ 2016-12-08 18:12 ` Ian Abbott
-1 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 18:12 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
On 08/12/16 15:46, SF Markus Elfring wrote:
>>> * Reduce memory allocation sizes for two function calls.
>
> Is this implementation detail worth for further considerations?
I assume you are referring to the allocation of devpriv->ai_urbs and
devpriv->ao_urbs? Your patch does not reduce their sizes; `urb` and
`*devpriv->ai_urbs` have the same type `struct urb *`. Having said
that, `sizeof(*devpriv->ai_urbs)` is cleaner code than `sizeof(urb)` in
this case.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()
@ 2016-12-08 18:12 ` Ian Abbott
0 siblings, 0 replies; 34+ messages in thread
From: Ian Abbott @ 2016-12-08 18:12 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten, LKML,
kernel-janitors
On 08/12/16 15:46, SF Markus Elfring wrote:
>>> * Reduce memory allocation sizes for two function calls.
>
> Is this implementation detail worth for further considerations?
I assume you are referring to the allocation of devpriv->ai_urbs and
devpriv->ao_urbs? Your patch does not reduce their sizes; `urb` and
`*devpriv->ai_urbs` have the same type `struct urb *`. Having said
that, `sizeof(*devpriv->ai_urbs)` is cleaner code than `sizeof(urb)` in
this case.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 5/5] staging: comedi: usbduxsigma: Move an assignment in usbduxsigma_alloc_usb_buffers()
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 11:38 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:38 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 11:20:38 +0100
Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbduxsigma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 8c04aa5339f3..7c1f9198447a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1338,7 +1338,7 @@ static int usbduxsigma_firmware_upload(struct comedi_device *dev,
static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
{
- struct usb_device *usb = comedi_to_usb_dev(dev);
+ struct usb_device *usb;
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1367,6 +1367,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
+ usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 5/5] staging: comedi: usbduxsigma: Move an assignment in usbduxsigma_alloc_usb_buffers()
@ 2016-12-08 11:38 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 11:38 UTC (permalink / raw)
To: devel, Chris Cesare, Greg Kroah-Hartman, H Hartley Sweeten,
Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 8 Dec 2016 11:20:38 +0100
Move one assignment for the local variable "usb" so that its setting
will only be performed after some memory allocations succeeded
by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbduxsigma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/usbduxsigma.c b/drivers/staging/comedi/drivers/usbduxsigma.c
index 8c04aa5339f3..7c1f9198447a 100644
--- a/drivers/staging/comedi/drivers/usbduxsigma.c
+++ b/drivers/staging/comedi/drivers/usbduxsigma.c
@@ -1338,7 +1338,7 @@ static int usbduxsigma_firmware_upload(struct comedi_device *dev,
static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
{
- struct usb_device *usb = comedi_to_usb_dev(dev);
+ struct usb_device *usb;
struct usbduxsigma_private *devpriv = dev->private;
struct urb *urb;
int i, x;
@@ -1367,6 +1367,7 @@ static int usbduxsigma_alloc_usb_buffers(struct comedi_device *dev)
if (!devpriv->ao_urbs)
goto free_ai_urbs;
+ usb = comedi_to_usb_dev(dev);
for (i = 0; i < devpriv->n_ai_urbs; i++) {
/* one frame: 1ms */
urb = usb_alloc_urb(1, GFP_KERNEL);
--
2.11.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] staging-COMEDI: Fine-tuning for three functions
2016-12-08 11:30 ` SF Markus Elfring
@ 2016-12-08 13:30 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 34+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-08 13:30 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, H Hartley Sweeten, Ian Abbott, LKML,
kernel-janitors
On Thu, Dec 08, 2016 at 12:30:20PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 11:37:37 +0100
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5):
> Combine four kcalloc() calls into one in serial2002_setup_subdevs()
> Split a condition check in usbdux_alloc_usb_buffers()
> Move an assignment in usbdux_alloc_usb_buffers()
> Split a condition check in usbduxsigma_alloc_usb_buffers()
> Move an assignment in usbduxsigma_alloc_usb_buffers()
>
> drivers/staging/comedi/drivers/serial2002.c | 22 +++++-----
> drivers/staging/comedi/drivers/usbdux.c | 56 ++++++++++++++++++-----
> drivers/staging/comedi/drivers/usbduxsigma.c | 66 ++++++++++++++++++++++------
> 3 files changed, 108 insertions(+), 36 deletions(-)
You do realize that I no longer take patches from you for any of the
subsystems I maintain, right? This patch series is one reason why...
I suggest working on other projects to learn C better first.
best of luck,
greg k-h
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] staging-COMEDI: Fine-tuning for three functions
@ 2016-12-08 13:30 ` Greg Kroah-Hartman
0 siblings, 0 replies; 34+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-08 13:30 UTC (permalink / raw)
To: SF Markus Elfring
Cc: devel, Chris Cesare, H Hartley Sweeten, Ian Abbott, LKML,
kernel-janitors
On Thu, Dec 08, 2016 at 12:30:20PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 8 Dec 2016 11:37:37 +0100
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5):
> Combine four kcalloc() calls into one in serial2002_setup_subdevs()
> Split a condition check in usbdux_alloc_usb_buffers()
> Move an assignment in usbdux_alloc_usb_buffers()
> Split a condition check in usbduxsigma_alloc_usb_buffers()
> Move an assignment in usbduxsigma_alloc_usb_buffers()
>
> drivers/staging/comedi/drivers/serial2002.c | 22 +++++-----
> drivers/staging/comedi/drivers/usbdux.c | 56 ++++++++++++++++++-----
> drivers/staging/comedi/drivers/usbduxsigma.c | 66 ++++++++++++++++++++++------
> 3 files changed, 108 insertions(+), 36 deletions(-)
You do realize that I no longer take patches from you for any of the
subsystems I maintain, right? This patch series is one reason why...
I suggest working on other projects to learn C better first.
best of luck,
greg k-h
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging-COMEDI: Fine-tuning for three functions
2016-12-08 13:30 ` Greg Kroah-Hartman
@ 2016-12-08 15:26 ` SF Markus Elfring
-1 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Chris Cesare, H Hartley Sweeten, Ian Abbott, LKML,
kernel-janitors
> You do realize that I no longer take patches from you for any of the
> subsystems I maintain, right?
Not so far.
It seems that you would like to present new information for our
challenging collaboration.
> This patch series is one reason why...
I hope that corresponding disagreements around shown change possibilities
can still be clarified somehow.
Would you like to check further improvements for the affected
source code here?
Regards,
Markus
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: staging-COMEDI: Fine-tuning for three functions
@ 2016-12-08 15:26 ` SF Markus Elfring
0 siblings, 0 replies; 34+ messages in thread
From: SF Markus Elfring @ 2016-12-08 15:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Chris Cesare, H Hartley Sweeten, Ian Abbott, LKML,
kernel-janitors
> You do realize that I no longer take patches from you for any of the
> subsystems I maintain, right?
Not so far.
It seems that you would like to present new information for our
challenging collaboration.
> This patch series is one reason why...
I hope that corresponding disagreements around shown change possibilities
can still be clarified somehow.
Would you like to check further improvements for the affected
source code here?
Regards,
Markus
^ permalink raw reply [flat|nested] 34+ messages in thread