Thread (88 messages) 88 messages, 13 authors, 2017-10-23

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

From: Michal Suchánek <hidden>
Date: 2017-10-19 11:56:37
Also in: kernel-janitors, linux-integrity, lkml

Hello,


On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Markus Elfring <redacted>
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 <redacted>
---
 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.
quoted hunk ↗ jump to hunk
 
 	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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help