[4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

Commit Message

SF Markus Elfring Oct. 16, 2017, 5:34 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Mon, 16 Oct 2017 19:00:34 +0200

Two pointer checks could be repeated by the tpm_ibmvtpm_probe() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Return directly after a call of the function "kzalloc" failed
  at the beginning.

* Adjust jump targets so that extra checks can be omitted at the end.

Signed-off-by: Markus Elfring <[e-mail]>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Michal Suchánek Oct. 19, 2017, 11:56 a.m. UTC | #1

Hello,


On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring <[e-mail]> wrote:

> From: Markus Elfring <[e-mail]>
> Date: Mon, 16 Oct 2017 19:00:34 +0200
> 
> Two pointer checks could be repeated by the tpm_ibmvtpm_probe()
> function during error handling even if the relevant properties can be
> determined for the involved variables before by source code analysis.
> 
> * Return directly after a call of the function "kzalloc" failed
>   at the beginning.
> 
> * Adjust jump targets so that extra checks can be omitted at the end.
> 
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c index a4b462a77b99..b8dda7546f64
> 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, 
>  	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
>  	if (!ibmvtpm)
> -		goto cleanup;
> +		return -ENOMEM;

Just no.

I have seen many fixes that do inverse of this after a piece of code
allocating some more resources was added before code that returns
straight away because it is the first allocation in a function.

>  
>  	ibmvtpm->dev = dev;
>  	ibmvtpm->vdev = vio_dev;
> @@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, crq_q->crq_addr = (struct ibmvtpm_crq
> *)get_zeroed_page(GFP_KERNEL); if (!crq_q->crq_addr) {
>  		dev_err(dev, "Unable to allocate memory for
> crq_addr\n");
> -		goto cleanup;
> +		goto free_tpm;
>  	}
>  
>  	crq_q->num_entry = CRQ_RES_BUF_SIZE /
> sizeof(*crq_q->crq_addr); @@ -629,7 +629,7 @@ static int
> tpm_ibmvtpm_probe(struct vio_dev *vio_dev, 
>  	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
>  		dev_err(dev, "dma mapping failed\n");
> -		goto cleanup;
> +		goto free_page;
>  	}
>  
>  	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, reg_crq_cleanup:
>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> -cleanup:
> -	if (ibmvtpm) {
> -		if (crq_q->crq_addr)
> -			free_page((unsigned long)crq_q->crq_addr);
> -		kfree(ibmvtpm);
> -	}
> -

I think a single cleanup section is better than many labels that just
avoid a single null check.

As long as you can tell easily which resources were already allocated
and need to be freed it is saner to keep only one cleanup section.

If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten again.

Also just changing this just for the sake of code style does not seem
worth it whatever style you prefer.

Thanks

Michal

SF Markus Elfring Oct. 19, 2017, 12:36 p.m. UTC | #2

>> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>> reg_crq_cleanup:
>>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
>> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
>> -cleanup:
>> -	if (ibmvtpm) {
>> -		if (crq_q->crq_addr)
>> -			free_page((unsigned long)crq_q->crq_addr);
>> -		kfree(ibmvtpm);
>> -	}
>> -
> 
> I think a single cleanup section is better than many labels that just
> avoid a single null check.

I proposed to delete two unnecessary condition checks together with
an adjustment of jump targets.

Regards,
Markus

Michal Suchánek Oct. 19, 2017, 12:46 p.m. UTC | #3

On Thu, 19 Oct 2017 14:36:09 +0200
SF Markus Elfring <[e-mail]> wrote:

> >> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> >> *vio_dev, reg_crq_cleanup:
> >>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> >> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> >> -cleanup:
> >> -	if (ibmvtpm) {
> >> -		if (crq_q->crq_addr)
> >> -			free_page((unsigned long)crq_q->crq_addr);
> >> -		kfree(ibmvtpm);
> >> -	}
> >> -  
> > 
> > I think a single cleanup section is better than many labels that
> > just avoid a single null check.  
> 
> I proposed to delete two unnecessary condition checks together with
> an adjustment of jump targets.
> 

They are necessary to ensure the code works with single jump target.

The compiler is free to optimize them away and create the new jump
target implicitly. Do not do the optimization in place of the compiler.
It can do it automatically, in most cases better, and automatically
adapt it to code changes.

Thanks

Michal

Dan Carpenter Oct. 19, 2017, 1:36 p.m. UTC | #4

On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> 
> I think a single cleanup section is better than many labels that just
> avoid a single null check.
>

I am not a big advocate of churn, but one err style error handling is
really bug prone.

I'm dealing with static analysis so most of the bugs I see are error
handling bugs.  That's because error handling is hard to test but easy
for static analysis.  One err style error handling is the worst because
you get things like:

fail:
	kfree(foo->bar);
	kfree(foo);

Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
to free things that haven't been allocated so, for example, I see
refcounting bugs in error handling paths as well where we decrement
something that wasn't incremented.  Freeing everything is more
complicated than just freeing one specific thing the way standard kernel
error handling works.

> As long as you can tell easily which resources were already allocated
> and need to be freed it is saner to keep only one cleanup section.
> 

Sure, if the function is simple and short then the error handling is
normally simple and short.  This is true for any style of error
handling.

> If the code doing the allocation is changed in the future the single
> cleanup can stay whereas multiple labels have to be rewritten again.

No, they don't unless you choose bad label names.  Perhaps numbered
labels?  We don't get a lot of those in the kernel any more.  Label
name should be based on what the label does.  Often I see bad label
names like generic labels:

	foo = kmalloc();
	if (!foo)
		goto out;

What is out going to do?  Another common anti-pattern is come-from
labels:

	foo = kmalloc();
	if (!foo)
		goto kmalloc_failed;

Obviously, we can see from the if statement that the alloc failed and
you *just* know the next line is going to be is going to be:

	if (invalid)
		goto kmalloc_failed;

Which is wrong because kmalloc didn't fail...  But if the label name is
based on what it does then, when you add or a remove an allocation, you
just have to edit the one thing.  It's very simple:

+	foo = new_alloc();
+	if (!foo)
+		return -ENOMEM;
+
 	bar = old_alloc();
-	if (!bar)
-		return -ENOMEM;
+	if (!bar) {
+		ret -ENOMEM;
+		goto free_foo;

[ snip ]

 free_whatever:
 	free(whatever);
+free_foo:
+	free(foo);

 return ret;

> 
> Also just changing this just for the sake of code style does not seem
> worth it whatever style you prefer.

True.

regards,
dan carpenter

Michal Suchánek Oct. 19, 2017, 2:16 p.m. UTC | #5

On Thu, 19 Oct 2017 16:36:46 +0300
Dan Carpenter <[e-mail]> wrote:

> On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> > 
> > I think a single cleanup section is better than many labels that
> > just avoid a single null check.
> >  
> 
> I am not a big advocate of churn, but one err style error handling is
> really bug prone.
> 
> I'm dealing with static analysis so most of the bugs I see are error
> handling bugs.  

But it looks like you do not deal much with actual code development
because then you would know that some of the changes proposed by the
static analysis lead to errors later when the code dynamically changes.

> That's because error handling is hard to test but easy
> for static analysis.  One err style error handling is the worst
> because you get things like:
> 
> fail:
> 	kfree(foo->bar);
> 	kfree(foo);
> 
> Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
> to free things that haven't been allocated so, for example, I see
> refcounting bugs in error handling paths as well where we decrement
> something that wasn't incremented.  Freeing everything is more
> complicated than just freeing one specific thing the way standard
> kernel error handling works.

It depends on the function in question. If it only allocates memory
which is not reference-counted and kfree() checks for the null in most
cases it is easier to do just one big cleanup. 

If it is more complex more labels are preferable.

> 
> > As long as you can tell easily which resources were already
> > allocated and need to be freed it is saner to keep only one cleanup
> > section. 
> 
> Sure, if the function is simple and short then the error handling is
> normally simple and short.  This is true for any style of error
> handling.
> 
> > If the code doing the allocation is changed in the future the single
> > cleanup can stay whereas multiple labels have to be rewritten
> > again.  
> 
> No, they don't unless you choose bad label names.  

You obviously miss the fact that resource allocation is not always
added at the end of the function.

You may need to reorder the code and hence the order of allocation or
add allocation at the start. Both of these cases break multiple labels
and special cases but work with one big cleanup just fine.

Thanks

Michal

Dan Carpenter Oct. 19, 2017, 2:26 p.m. UTC | #6

tpm_ibmvtpm_probe() is an example of poor names.  It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:

   647          rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
   648                           tpm_ibmvtpm_driver_name, ibmvtpm);
   649          if (rc) {
   650                  dev_err(dev, "Error %d register irq 0x%x\n", rc, vio_dev->irq);
   651                  goto init_irq_cleanup;
   652          }
   653  
   654          rc = vio_enable_interrupts(vio_dev);
   655          if (rc) {
   656                  dev_err(dev, "Error %d enabling interrupts\n", rc);
   657                  goto init_irq_cleanup;
   658          }

Sadly, we never do call free_irq().

> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.

I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof".  It's not true.
The best error handling is methodical error handling:

1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
   does
5) That doesn't free anything which hasn't been allocated

regards,
dan carpenter

Dan Carpenter Oct. 19, 2017, 2:59 p.m. UTC | #7

On Thu, Oct 19, 2017 at 04:16:37PM +0200, Michal Suchánek wrote:
> On Thu, 19 Oct 2017 16:36:46 +0300
> Dan Carpenter <[e-mail]> wrote:
> 
> > On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> > > 
> > > I think a single cleanup section is better than many labels that
> > > just avoid a single null check.
> > >  
> > 
> > I am not a big advocate of churn, but one err style error handling is
> > really bug prone.
> > 
> > I'm dealing with static analysis so most of the bugs I see are error
> > handling bugs.  
> 
> But it looks like you do not deal much with actual code development
> because then you would know that some of the changes proposed by the
> static analysis lead to errors later when the code dynamically changes.
> 

This is silly...  Anyway, my record is there in git.  I mostly send
bugfixes, and not cleanups.  In terms of patches that are merged, I
probably have introduced one or two runtime bugs per year over the past
decade.

> > That's because error handling is hard to test but easy
> > for static analysis.  One err style error handling is the worst
> > because you get things like:
> > 
> > fail:
> > 	kfree(foo->bar);
> > 	kfree(foo);
> > 
> > Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
> > to free things that haven't been allocated so, for example, I see
> > refcounting bugs in error handling paths as well where we decrement
> > something that wasn't incremented.  Freeing everything is more
> > complicated than just freeing one specific thing the way standard
> > kernel error handling works.
> 
> It depends on the function in question. If it only allocates memory
> which is not reference-counted and kfree() checks for the null in most
> cases it is easier to do just one big cleanup.
> 

No.  Just always do it standard way.  If there is only one error
condition just free it directly:

	if (invalid) {
		free(foo);
		return -EINVAL;
	}

But once you add a second error condition then use gotos.  There is no
reason to deviate from the standard.

> If it is more complex more labels are preferable.
> 
> > 
> > > As long as you can tell easily which resources were already
> > > allocated and need to be freed it is saner to keep only one cleanup
> > > section. 
> > 
> > Sure, if the function is simple and short then the error handling is
> > normally simple and short.  This is true for any style of error
> > handling.
> > 
> > > If the code doing the allocation is changed in the future the single
> > > cleanup can stay whereas multiple labels have to be rewritten
> > > again.  
> > 
> > No, they don't unless you choose bad label names.  
> 
> You obviously miss the fact that resource allocation is not always
> added at the end of the function.
> 

No, in my example, the new allocation was added to the *start* not the
end.  It doesn't matter though, standard error handling works the same
either way.

> You may need to reorder the code and hence the order of allocation or
> add allocation at the start. Both of these cases break multiple labels
> and special cases but work with one big cleanup just fine.

No, You don't need to re-order the labels or change them.  The label
name says what is freed.  The order is the reverse order from how they
were allocated.  It's very simple.  You just have to keep track of the
most recently allocated thing:

	foo = alloc();
	if (!foo)
		return -ENOMEM;

	bar = alloc();
	if (!bar)
		goto free_foo;

	baz = alloc();
	if (!baz)
		goto free_bar;

You can tell this code doesn't leak just from looking at the label
names.

regards,
dan carpenter

SF Markus Elfring Oct. 19, 2017, 8:44 p.m. UTC | #8

>> If the code doing the allocation is changed in the future the single
>> cleanup can stay whereas multiple labels have to be rewritten again.
> 
> No, they don't unless you choose bad label names.  Perhaps numbered
> labels?  We don't get a lot of those in the kernel any more.  Label
> name should be based on what the label does.  Often I see bad label
> names like generic labels:
> 
> 	foo = kmalloc();
> 	if (!foo)
> 		goto out;
> 
> What is out going to do?  Another common anti-pattern is come-from
> labels:
> 
> 	foo = kmalloc();
> 	if (!foo)
> 		goto kmalloc_failed;
> 
> Obviously, we can see from the if statement that the alloc failed and
> you *just* know the next line is going to be is going to be:
> 
> 	if (invalid)
> 		goto kmalloc_failed;
> 
> Which is wrong because kmalloc didn't fail...  But if the label name is
> based on what it does then, when you add or a remove an allocation, you
> just have to edit the one thing.

Would you be interested in an update on a topic like “Source code review
around jump label usage”?
https://lkml.org/lkml/2015/12/11/378

Regards,
Markus

Patch

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index a4b462a77b99..b8dda7546f64 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -610,7 +610,7 @@  static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 
 	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
 	if (!ibmvtpm)
-		goto cleanup;
+		return -ENOMEM;
 
 	ibmvtpm->dev = dev;
 	ibmvtpm->vdev = vio_dev;
@@ -619,7 +619,7 @@  static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	crq_q->crq_addr = (struct ibmvtpm_crq *)get_zeroed_page(GFP_KERNEL);
 	if (!crq_q->crq_addr) {
 		dev_err(dev, "Unable to allocate memory for crq_addr\n");
-		goto cleanup;
+		goto free_tpm;
 	}
 
 	crq_q->num_entry = CRQ_RES_BUF_SIZE / sizeof(*crq_q->crq_addr);
@@ -629,7 +629,7 @@  static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 
 	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
 		dev_err(dev, "dma mapping failed\n");
-		goto cleanup;
+		goto free_page;
 	}
 
 	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
@@ -683,13 +683,10 @@  static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 reg_crq_cleanup:
 	dma_unmap_single(dev, ibmvtpm->crq_dma_handle, CRQ_RES_BUF_SIZE,
 			 DMA_BIDIRECTIONAL);
-cleanup:
-	if (ibmvtpm) {
-		if (crq_q->crq_addr)
-			free_page((unsigned long)crq_q->crq_addr);
-		kfree(ibmvtpm);
-	}
-
+free_page:
+	free_page((unsigned long)crq_q->crq_addr);
+free_tpm:
+	kfree(ibmvtpm);
 	return rc;
 }