[PATCH 0/5] staging-COMEDI: Fine-tuning for three functions

[PATCH 0/5] staging-COMEDI: Fine-tuning for three functions

[SF Markus Elfring]

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

[PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs()

[SF Markus Elfring]

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

Re: [PATCH 1/5] staging: comedi: serial2002: Combine four kcalloc() calls into one in serial2002_setup_subdevs()

[Dan Carpenter]

The original code was simpler.

regards,
dan carpenter

[PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

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

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Dan Carpenter]

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

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Dan Carpenter]

Same bug.

regards,
dan carpenter

Re: [PATCH 2/5] staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[Ian Abbott]

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/  )=-

Re: staging: comedi: usbdux: Split a condition check in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

> 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

[PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()

[SF Markus Elfring]

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

Re: [PATCH 3/5] staging: comedi: usbdux: Move an assignment in usbdux_alloc_usb_buffers()

[Dan Carpenter]

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

[PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

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

Re: [PATCH 4/5] staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[Ian Abbott]

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/  )=-

Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

>> * Reduce memory allocation sizes for two function calls.

Is this implementation detail worth for further considerations?

Regards,
Markus

Re: staging: comedi: usbduxsigma: Split a condition check in usbduxsigma_alloc_usb_buffers()

[Ian Abbott]

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/  )=-

[PATCH 5/5] staging: comedi: usbduxsigma: Move an assignment in usbduxsigma_alloc_usb_buffers()

[SF Markus Elfring]

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

Re: [PATCH 0/5] staging-COMEDI: Fine-tuning for three functions

[Greg Kroah-Hartman]

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

Re: staging-COMEDI: Fine-tuning for three functions

[SF Markus Elfring]

> 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