[PATCH 0/4] fix error return code

[PATCH 0/4] fix error return code

[Julia Lawall]

The complate semantic patch that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
// identify a function that returns a negative return value at least once.
@ok exists@
identifier f,ret,i;
expression e;
constant c;
@@

f(...) {
... when any
(
return -c@i;
|
ret = -c@i;
... when != ret = e
return ret;
|
if (ret < 0) { ... return ret; }
)
... when any
}

// identify a case where the return variable is set to a non-negative value
// and then returned in error-handling code
@r exists@
identifier ret,ok.f,fn;
expression e1,e2,e3,e4,e5,e6,x;
statement S,S1;
position p1,p2,p3;
@@

f(...) {
... when any
(
if@p1 (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret@p1 = 0
)
... when != \(ret = e1\|ret++\|ret--\|ret+á\|ret-á\)
    when != &ret
    when any
(
 if (<+... ret = e5 ...+>) S1
|
 if (<+... &ret ...+>) S1
|
if@p2(<+...x = fn(...)...+>)
 {
  ... when != ret = e6
      when forall
 return@p3 ret;
}
|
break;
|
x = fn(...)
... when != \(ret = e4\|ret++\|ret--\|ret+ä\|ret-ä\)
    when != &ret
(
 if (<+... ret = e3 ...+>) S
|
 if (<+... &ret ...+>) S
|
if@p2(<+...\(x != 0\|x < 0\|x = NULL\|IS_ERR(x)\)...+>)
 {
  ... when != ret = e2
      when forall
 return@p3 ret;
}
)
)
... when any
}

@printer depends on r@
position p;
identifier ok.f,pr;
constant char [] c;
@@

f(...) { <...pr@p(...,c,...)...> }

@bad0 exists@
identifier r.ret,ok.f,g != {ERR_PTR,IS_ERR};
position p != printer.p;
@@

f(...) { ... when any
g@p(...,ret,...)
... when any
 }

// ignore the above if there is some path where the variable is set to
// something else
@bad depends on !bad0 exists@
position r.p1,r.p2;
statement S1,S2;
identifier r.ret;
expression e1;
@@

(
if@p1 (\(ret < 0\|ret != 0\)) S1
|
ret@p1 = 0
)
... when any
 \(ret = e1\|ret++\|ret--\|ret+á\|ret-á\|&ret\)
... when any
if@p2(...) S2

@bad1 depends on !bad0 && !bad exists@
position r.p2;
statement S2;
identifier r.ret;
expression e1;
constant c;
@@

ret = -c
... when != \(ret = e1\|ret++\|ret--\|ret+á\|ret-á\)
    when != &ret
    when any
if@p2(...) S2

// likewise ignore it if there has been an intervening return
@bad2 depends on !bad0 && !bad && !bad1 exists@
position r.p1,r.p2;
identifier r.ret;
expression e1;
statement S2;
constant c;
@@

ret@p1 = 0
... when != if (...) { ... ret = e1 ... return ret; }
    when != if (...) { ... return -c; }
    when any
if@p2(...) S2


@script:python depends on !bad0 && !bad && !bad1 && !bad2@
p1 << r.p1;
p2 << r.p2;
p3 << r.p3;
@@

cocci.print_main("",p1)
cocci.print_secs("",p2)
cocci.print_secs("",p3)
// </smpl>

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

[PATCH 2/4] gdrom: fix error return code

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert a zero return value on error to a negative one, as returned
elsewhere in the function.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/cdrom/gdrom.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 584bc31..46ecd95 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -807,16 +807,20 @@ static int probe_gdrom(struct platform_device *devptr)
 	if (err)
 		goto probe_fail_cmdirq_register;
 	gd.gdrom_rq = blk_init_queue(gdrom_request, &gdrom_lock);
-	if (!gd.gdrom_rq)
+	if (!gd.gdrom_rq) {
+		err = -ENOMEM;
 		goto probe_fail_requestq;
+	}
 
 	err = probe_gdrom_setupqueue();
 	if (err)
 		goto probe_fail_toc;
 
 	gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
-	if (!gd.toc)
+	if (!gd.toc) {
+		err = -ENOMEM;
 		goto probe_fail_toc;
+	}
 	add_disk(gd.disk);
 	return 0;
 

[PATCH 1/4] umem: fix error return code

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert a zero return value on error to a negative one, as returned
elsewhere in the function.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/block/umem.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/block/umem.c b/drivers/block/umem.c
index 4cf81b5..371e819 100644
--- a/drivers/block/umem.c
+++ b/drivers/block/umem.c
@@ -881,6 +881,7 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (card->mm_pages[0].desc = NULL ||
 	    card->mm_pages[1].desc = NULL) {
 		dev_printk(KERN_ERR, &card->dev->dev, "alloc failed\n");
+		ret = -ENOMEM;
 		goto failed_alloc;
 	}
 	reset_page(&card->mm_pages[0]);
@@ -891,8 +892,10 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	card->biotail = &card->bio;
 
 	card->queue = blk_alloc_queue(GFP_KERNEL);
-	if (!card->queue)
+	if (!card->queue) {
+		ret = -ENOMEM;
 		goto failed_alloc;
+	}
 
 	blk_queue_make_request(card->queue, mm_make_request);
 	card->queue->queue_lock = &card->lock;

[PATCH 3/4] block: fix error return code

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert a zero return value on error to a negative one, as returned
elsewhere in the function.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/block/rsxx/core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index a8de2ee..fa8077a 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -942,6 +942,7 @@ static int rsxx_pci_probe(struct pci_dev *dev,
 	card->event_wq = create_singlethread_workqueue(DRIVER_NAME"_event");
 	if (!card->event_wq) {
 		dev_err(CARD_TO_DEV(card), "Failed card event setup.\n");
+		st = -ENOMEM;
 		goto failed_event_handler;
 	}
 

[PATCH 4/4] usb: gadget: fix error return code

[Julia Lawall]

From: Julia Lawall <Julia.Lawall@lip6.fr>

Convert a zero return value on error to a negative one, as returned
elsewhere in the function.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/usb/gadget/udc/fusb300_udc.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/fusb300_udc.c b/drivers/usb/gadget/udc/fusb300_udc.c
index d40255f..5c5d1ad 100644
--- a/drivers/usb/gadget/udc/fusb300_udc.c
+++ b/drivers/usb/gadget/udc/fusb300_udc.c
@@ -1398,13 +1398,17 @@ static int fusb300_probe(struct platform_device *pdev)
 
 	/* initialize udc */
 	fusb300 = kzalloc(sizeof(struct fusb300), GFP_KERNEL);
-	if (fusb300 = NULL)
+	if (fusb300 = NULL) {
+		ret = -ENOMEM;
 		goto clean_up;
+	}
 
 	for (i = 0; i < FUSB300_MAX_NUM_EP; i++) {
 		_ep[i] = kzalloc(sizeof(struct fusb300_ep), GFP_KERNEL);
-		if (_ep[i] = NULL)
+		if (_ep[i] = NULL) {
+			ret = -ENOMEM;
 			goto clean_up;
+		}
 		fusb300->ep[i] = _ep[i];
 	}
 

Re: [PATCH 1/4] umem: fix error return code

[Jeff Moyer]

Julia Lawall <Julia.Lawall@lip6.fr> writes:

> diff --git a/drivers/block/umem.c b/drivers/block/umem.c
> index 4cf81b5..371e819 100644
> --- a/drivers/block/umem.c
> +++ b/drivers/block/umem.c
> @@ -881,6 +881,7 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (card->mm_pages[0].desc = NULL ||
>  	    card->mm_pages[1].desc = NULL) {
>  		dev_printk(KERN_ERR, &card->dev->dev, "alloc failed\n");
> +		ret = -ENOMEM;
>  		goto failed_alloc;
>  	}
>  	reset_page(&card->mm_pages[0]);
> @@ -891,8 +892,10 @@ static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	card->biotail = &card->bio;
>  
>  	card->queue = blk_alloc_queue(GFP_KERNEL);
> -	if (!card->queue)
> +	if (!card->queue) {
> +		ret = -ENOMEM;
>  		goto failed_alloc;
> +	}

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 2/4] gdrom: fix error return code

[Jeff Moyer]

Julia Lawall <Julia.Lawall@lip6.fr> writes:

> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> index 584bc31..46ecd95 100644
> --- a/drivers/cdrom/gdrom.c
> +++ b/drivers/cdrom/gdrom.c
> @@ -807,16 +807,20 @@ static int probe_gdrom(struct platform_device *devptr)
>  	if (err)
>  		goto probe_fail_cmdirq_register;
>  	gd.gdrom_rq = blk_init_queue(gdrom_request, &gdrom_lock);
> -	if (!gd.gdrom_rq)
> +	if (!gd.gdrom_rq) {
> +		err = -ENOMEM;
>  		goto probe_fail_requestq;
> +	}
>  
>  	err = probe_gdrom_setupqueue();
>  	if (err)
>  		goto probe_fail_toc;
>  
>  	gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
> -	if (!gd.toc)
> +	if (!gd.toc) {
> +		err = -ENOMEM;
>  		goto probe_fail_toc;
> +	}

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 3/4] block: fix error return code

[Jeff Moyer]

Julia Lawall <Julia.Lawall@lip6.fr> writes:

> diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
> index a8de2ee..fa8077a 100644
> --- a/drivers/block/rsxx/core.c
> +++ b/drivers/block/rsxx/core.c
> @@ -942,6 +942,7 @@ static int rsxx_pci_probe(struct pci_dev *dev,
>  	card->event_wq = create_singlethread_workqueue(DRIVER_NAME"_event");
>  	if (!card->event_wq) {
>  		dev_err(CARD_TO_DEV(card), "Failed card event setup.\n");
> +		st = -ENOMEM;
>  		goto failed_event_handler;
>  	}

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

BTW, just above this there is questionable code:

        st = rsxx_get_num_targets(card, &card->n_targets);
        if (st)
                dev_info(CARD_TO_DEV(card),
                        "Failed reading the number of DMA targets\n");

        card->ctrl = kzalloc(card->n_targets * sizeof(*card->ctrl), GFP_KERNEL);
        if (!card->ctrl) {
                st = -ENOMEM;
                goto failed_dma_setup;
        }

From my reading of the kzalloc code, ZERO_SIZE_PTR (which is 16 cast to
a void *) would be returned from that kzalloc call if the
rsxx_get_num_targets call failed (since you'd be kzalloc-ing 0 bytes).
That would lead to the !card->ctrl check not working, right?

I'd suggest not continuing after rsxx_get_num_targets fails.

Cheers,
Jeff

Re: [PATCH 4/4] usb: gadget: fix error return code

[Jeff Moyer]

Julia Lawall <Julia.Lawall@lip6.fr> writes:

> diff --git a/drivers/usb/gadget/udc/fusb300_udc.c b/drivers/usb/gadget/udc/fusb300_udc.c
> index d40255f..5c5d1ad 100644
> --- a/drivers/usb/gadget/udc/fusb300_udc.c
> +++ b/drivers/usb/gadget/udc/fusb300_udc.c
> @@ -1398,13 +1398,17 @@ static int fusb300_probe(struct platform_device *pdev)
>  
>  	/* initialize udc */
>  	fusb300 = kzalloc(sizeof(struct fusb300), GFP_KERNEL);
> -	if (fusb300 = NULL)
> +	if (fusb300 = NULL) {
> +		ret = -ENOMEM;
>  		goto clean_up;
> +	}
>  
>  	for (i = 0; i < FUSB300_MAX_NUM_EP; i++) {
>  		_ep[i] = kzalloc(sizeof(struct fusb300_ep), GFP_KERNEL);
> -		if (_ep[i] = NULL)
> +		if (_ep[i] = NULL) {
> +			ret = -ENOMEM;
>  			goto clean_up;
> +		}
>  		fusb300->ep[i] = _ep[i];
>  	}

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 3/4] block: fix error return code

[Julia Lawall]

On Thu, 7 Aug 2014, Jeff Moyer wrote:

> Julia Lawall <Julia.Lawall@lip6.fr> writes:
> 
> > diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
> > index a8de2ee..fa8077a 100644
> > --- a/drivers/block/rsxx/core.c
> > +++ b/drivers/block/rsxx/core.c
> > @@ -942,6 +942,7 @@ static int rsxx_pci_probe(struct pci_dev *dev,
> >  	card->event_wq = create_singlethread_workqueue(DRIVER_NAME"_event");
> >  	if (!card->event_wq) {
> >  		dev_err(CARD_TO_DEV(card), "Failed card event setup.\n");
> > +		st = -ENOMEM;
> >  		goto failed_event_handler;
> >  	}
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> 
> BTW, just above this there is questionable code:
> 
>         st = rsxx_get_num_targets(card, &card->n_targets);
>         if (st)
>                 dev_info(CARD_TO_DEV(card),
>                         "Failed reading the number of DMA targets\n");
> 
>         card->ctrl = kzalloc(card->n_targets * sizeof(*card->ctrl), GFP_KERNEL);
>         if (!card->ctrl) {
>                 st = -ENOMEM;
>                 goto failed_dma_setup;
>         }
> 
> From my reading of the kzalloc code, ZERO_SIZE_PTR (which is 16 cast to
> a void *) would be returned from that kzalloc call if the
> rsxx_get_num_targets call failed (since you'd be kzalloc-ing 0 bytes).
> That would lead to the !card->ctrl check not working, right?
> 
> I'd suggest not continuing after rsxx_get_num_targets fails.

Good point.  I'll fix it up.

julia

Re: [PATCH 3/4] block: fix error return code

[Dan Carpenter]

On Thu, Aug 07, 2014 at 05:48:24PM -0400, Jeff Moyer wrote:
> BTW, just above this there is questionable code:
> 
>         st = rsxx_get_num_targets(card, &card->n_targets);
>         if (st)
>                 dev_info(CARD_TO_DEV(card),
>                         "Failed reading the number of DMA targets\n");
> 
>         card->ctrl = kzalloc(card->n_targets * sizeof(*card->ctrl), GFP_KERNEL);
>         if (!card->ctrl) {
>                 st = -ENOMEM;
>                 goto failed_dma_setup;
>         }
> 
> >From my reading of the kzalloc code, ZERO_SIZE_PTR (which is 16 cast to
> a void *) would be returned from that kzalloc call if the
> rsxx_get_num_targets call failed (since you'd be kzalloc-ing 0 bytes).
> That would lead to the !card->ctrl check not working, right?
>

ZERO_SIZE_PTR is a subtle thing.  The if (!card->ctrl) check correctly
tells you if you allocated enough space to hold zero elements.  Which is
yes so we can continue without a problem.

Of course, you'd have to look at the surrounding code to see if there is
a problem...  I think I have seen dereferencing ZERO_SIZE_PTR bugs in
the past, but they are rare.

> I'd suggest not continuing after rsxx_get_num_targets fails.

Introducing new failures is a bad thing unless you know the code very
well or you can test it.  Your instinct should always be to not do that.

regards,
dan carpenter