usb: cdnsp: fix error handling in cdnsp_mem_init()

This is just an RFC patch because I couldn't figure out why we were calling halt and reset so I just deleted that.

	cdnsp_halt(pdev);
	v(pdev);

This function uses "One Function Cleans up Everything" style and that's basically impossible to do correctly. It's cleaner to write it with "clean up the most recent allocation". It's simple to review because you only have to remember the most recent successful allocation and verify that the goto matches. You never free anything that wasn't allocated so if avoids a lot of bugs. Also you can copy and paste the error handling from here, remove the labels, and add a call to cdnsp_free_priv_device(pdev) and it auto generates the cdnsp_mem_cleanup() function.

There are two problems that I see with the original code. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL dereference inside the cdnsp_free_priv_device() function. And if cdnsp_alloc_priv_device() fails that leads to a double free because we free pdev->out_ctx.bytes in several places.

---
drivers/usb/cdns3/cdnsp-mem.c | 36 ++++++++++++++++++++++-------------

1 file changed, 23 insertions(+), 13 deletions(-)

drivers/usb/cdns3/cdnsp-mem.c +23 -14

index 6a0a12e1f54c..6d3fe72e920e 100644
@@ -1229,7 +1229,7 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->dcbaa = dma_alloc_coherent(dev, sizeof(*pdev->dcbaa),
 					 &dma, GFP_KERNEL);
 	if (!pdev->dcbaa)
-		goto mem_init_fail;
+		return -ENOMEM;
 
 	memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa));
 	pdev->dcbaa->dma = dma;

@@ -1247,17 +1247,19 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->segment_pool = dma_pool_create("CDNSP ring segments", dev,
 					     TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
 					     page_size);
+	if (!pdev->segment_pool)
+		goto release_dcbaa;
 
 	pdev->device_pool = dma_pool_create("CDNSP input/output contexts", dev,
 					    CDNSP_CTX_SIZE, 64, page_size);
+	if (!pdev->device_pool)
+		goto destroy_segment_pool;
 
-	if (!pdev->segment_pool || !pdev->device_pool)
-		goto mem_init_fail;
 
 	/* Set up the command ring to have one segments for now. */
 	pdev->cmd_ring = cdnsp_ring_alloc(pdev, 1, TYPE_COMMAND, 0, flags);
 	if (!pdev->cmd_ring)
-		goto mem_init_fail;
+		goto destroy_device_pool;
 
 	/* Set the address in the Command Ring Control register */
 	val_64 = cdnsp_read_64(&pdev->op_regs->cmd_ring);

@@ -1280,11 +1282,11 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->event_ring = cdnsp_ring_alloc(pdev, ERST_NUM_SEGS, TYPE_EVENT,
 					    0, flags);
 	if (!pdev->event_ring)
-		goto mem_init_fail;
+		goto free_cmd_ring;
 
 	ret = cdnsp_alloc_erst(pdev, pdev->event_ring, &pdev->erst, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_event_ring;
 
 	/* Set ERST count with the number of entries in the segment table. */
 	val = readl(&pdev->ir_set->erst_size);

@@ -1303,22 +1305,30 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 
 	ret = cdnsp_setup_port_arrays(pdev, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_erst;
 
 	ret = cdnsp_alloc_priv_device(pdev, GFP_ATOMIC);
 	if (ret) {
 		dev_err(pdev->dev,
 			"Could not allocate cdnsp_device data structures\n");
-		goto mem_init_fail;
+		goto free_erst;
 	}
 
 	return 0;
 
-mem_init_fail:
-	dev_err(pdev->dev, "Couldn't initialize memory\n");
-	cdnsp_halt(pdev);
-	cdnsp_reset(pdev);
-	cdnsp_mem_cleanup(pdev);
+free_erst:
+	cdnsp_free_erst(pdev, &pdev->erst);
+free_event_ring:
+	cdnsp_ring_free(pdev, pdev->event_ring);
+free_cmd_ring:
+	cdnsp_ring_free(pdev, pdev->cmd_ring);
+destroy_device_pool:
+	dma_pool_destroy(pdev->device_pool);
+destroy_segment_pool:
+	dma_pool_destroy(pdev->segment_pool);
+release_dcbaa:
+	dma_free_coherent(dev, sizeof(*pdev->dcbaa), pdev->dcbaa,
+			  pdev->dcbaa->dma);
 
 	return ret;
 }
--

2.29.2

Hi Dan,

Quoted 6 lines

This is just an RFC patch because I couldn't figure out why we were calling halt and reset so I just deleted that.

	cdnsp_halt(pdev);
	v(pdev);

At first glance it looks like cdnsp_halt can be removed. I little afraid about cdnsp_reset because it reset some internal state that could be changed during initialization.

I think that you could move cdnsp_reset just before return instruction.

I will make some test to confirm it.

Quoted 112 lines

This function uses "One Function Cleans up Everything" style and that's basically impossible to do correctly. It's cleaner to write it with "clean up the most recent allocation". It's simple to review because you only have to remember the most recent successful allocation and verify that the goto matches. You never free anything that wasn't allocated so if avoids a lot of bugs. Also you can copy and paste the error handling from here, remove the labels, and add a call to cdnsp_free_priv_device(pdev) and it auto generates the cdnsp_mem_cleanup() function.

There are two problems that I see with the original code. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL dereference inside the cdnsp_free_priv_device() function. And if cdnsp_alloc_priv_device() fails that leads to a double free because we free pdev->out_ctx.bytes in several places.

---
drivers/usb/cdns3/cdnsp-mem.c | 36 ++++++++++++++++++++++-------------

1 file changed, 23 insertions(+), 13 deletions(-)

drivers/usb/cdns3/cdnsp-mem.c +23 -14

index 6a0a12e1f54c..6d3fe72e920e 100644
@@ -1229,7 +1229,7 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->dcbaa = dma_alloc_coherent(dev, sizeof(*pdev->dcbaa),
 					 &dma, GFP_KERNEL);
 	if (!pdev->dcbaa)
-		goto mem_init_fail;
+		return -ENOMEM;
 
 	memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa));
 	pdev->dcbaa->dma = dma;

@@ -1247,17 +1247,19 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->segment_pool = dma_pool_create("CDNSP ring segments", dev,
 					     TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
 					     page_size);
+	if (!pdev->segment_pool)
+		goto release_dcbaa;
 
 	pdev->device_pool = dma_pool_create("CDNSP input/output contexts", dev,
 					    CDNSP_CTX_SIZE, 64, page_size);
+	if (!pdev->device_pool)
+		goto destroy_segment_pool;
 
-	if (!pdev->segment_pool || !pdev->device_pool)
-		goto mem_init_fail;
 
 	/* Set up the command ring to have one segments for now. */
 	pdev->cmd_ring = cdnsp_ring_alloc(pdev, 1, TYPE_COMMAND, 0, flags);
 	if (!pdev->cmd_ring)
-		goto mem_init_fail;
+		goto destroy_device_pool;
 
 	/* Set the address in the Command Ring Control register */
 	val_64 = cdnsp_read_64(&pdev->op_regs->cmd_ring);

@@ -1280,11 +1282,11 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->event_ring = cdnsp_ring_alloc(pdev, ERST_NUM_SEGS, TYPE_EVENT,
 					    0, flags);
 	if (!pdev->event_ring)
-		goto mem_init_fail;
+		goto free_cmd_ring;
 
 	ret = cdnsp_alloc_erst(pdev, pdev->event_ring, &pdev->erst, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_event_ring;
 
 	/* Set ERST count with the number of entries in the segment table. */
 	val = readl(&pdev->ir_set->erst_size);

@@ -1303,22 +1305,30 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 
 	ret = cdnsp_setup_port_arrays(pdev, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_erst;
 
 	ret = cdnsp_alloc_priv_device(pdev, GFP_ATOMIC);
 	if (ret) {
 		dev_err(pdev->dev,
 			"Could not allocate cdnsp_device data structures\n");
-		goto mem_init_fail;
+		goto free_erst;
 	}
 
 	return 0;
 
-mem_init_fail:
-	dev_err(pdev->dev, "Couldn't initialize memory\n");
-	cdnsp_halt(pdev);
-	cdnsp_reset(pdev);
-	cdnsp_mem_cleanup(pdev);
+free_erst:
+	cdnsp_free_erst(pdev, &pdev->erst);
+free_event_ring:
+	cdnsp_ring_free(pdev, pdev->event_ring);
+free_cmd_ring:
+	cdnsp_ring_free(pdev, pdev->cmd_ring);
+destroy_device_pool:
+	dma_pool_destroy(pdev->device_pool);
+destroy_segment_pool:
+	dma_pool_destroy(pdev->segment_pool);
+release_dcbaa:
+	dma_free_coherent(dev, sizeof(*pdev->dcbaa), pdev->dcbaa,
+			  pdev->dcbaa->dma);
 
 	return ret;
 }
--

2.29.2

Quoted 8 lines

Hi Dan,

Quoted 6 lines

This is just an RFC patch because I couldn't figure out why we were calling halt and reset so I just deleted that.

	cdnsp_halt(pdev);
	cdnsp_reset(pdev);

Quoted 7 lines

At first glance it looks like cdnsp_halt can be removed. I little afraid about cdnsp_reset because it reset some internal state that could be changed during initialization.

I think that you could move cdnsp_reset just before return instruction.

I will make some test to confirm it.

It works correct, you can remove cdnsp_halt and move cndsp_reset as the last instruction before return.
Probably cdnsp_reset also is not required but it's good to restore the controller to the default state after error. Just in case.

Thanks,
Pawel

Quoted 112 lines

Quoted 112 lines

This function uses "One Function Cleans up Everything" style and that's basically impossible to do correctly. It's cleaner to write it with "clean up the most recent allocation". It's simple to review because you only have to remember the most recent successful allocation and verify that the goto matches. You never free anything that wasn't allocated so if avoids a lot of bugs. Also you can copy and paste the error handling from here, remove the labels, and add a call to cdnsp_free_priv_device(pdev) and it auto generates the cdnsp_mem_cleanup() function.

There are two problems that I see with the original code. If pdev->dcbaa = dma_alloc_coherent() fails then that leads to a NULL dereference inside the cdnsp_free_priv_device() function. And if cdnsp_alloc_priv_device() fails that leads to a double free because we free pdev->out_ctx.bytes in several places.

---
drivers/usb/cdns3/cdnsp-mem.c | 36 ++++++++++++++++++++++-------------

1 file changed, 23 insertions(+), 13 deletions(-)

drivers/usb/cdns3/cdnsp-mem.c +23 -14

index 6a0a12e1f54c..6d3fe72e920e 100644
@@ -1229,7 +1229,7 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->dcbaa = dma_alloc_coherent(dev, sizeof(*pdev->dcbaa),
 					 &dma, GFP_KERNEL);
 	if (!pdev->dcbaa)
-		goto mem_init_fail;
+		return -ENOMEM;
 
 	memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa));
 	pdev->dcbaa->dma = dma;

@@ -1247,17 +1247,19 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->segment_pool = dma_pool_create("CDNSP ring segments", dev,
 					     TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
 					     page_size);
+	if (!pdev->segment_pool)
+		goto release_dcbaa;
 
 	pdev->device_pool = dma_pool_create("CDNSP input/output contexts", dev,
 					    CDNSP_CTX_SIZE, 64, page_size);
+	if (!pdev->device_pool)
+		goto destroy_segment_pool;
 
-	if (!pdev->segment_pool || !pdev->device_pool)
-		goto mem_init_fail;
 
 	/* Set up the command ring to have one segments for now. */
 	pdev->cmd_ring = cdnsp_ring_alloc(pdev, 1, TYPE_COMMAND, 0, flags);
 	if (!pdev->cmd_ring)
-		goto mem_init_fail;
+		goto destroy_device_pool;
 
 	/* Set the address in the Command Ring Control register */
 	val_64 = cdnsp_read_64(&pdev->op_regs->cmd_ring);

@@ -1280,11 +1282,11 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 	pdev->event_ring = cdnsp_ring_alloc(pdev, ERST_NUM_SEGS, TYPE_EVENT,
 					    0, flags);
 	if (!pdev->event_ring)
-		goto mem_init_fail;
+		goto free_cmd_ring;
 
 	ret = cdnsp_alloc_erst(pdev, pdev->event_ring, &pdev->erst, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_event_ring;
 
 	/* Set ERST count with the number of entries in the segment table. */
 	val = readl(&pdev->ir_set->erst_size);

@@ -1303,22 +1305,30 @@ int cdnsp_mem_init(struct cdnsp_device *pdev, gfp_t flags)
 
 	ret = cdnsp_setup_port_arrays(pdev, flags);
 	if (ret)
-		goto mem_init_fail;
+		goto free_erst;
 
 	ret = cdnsp_alloc_priv_device(pdev, GFP_ATOMIC);
 	if (ret) {
 		dev_err(pdev->dev,
 			"Could not allocate cdnsp_device data structures\n");
-		goto mem_init_fail;
+		goto free_erst;
 	}
 
 	return 0;
 
-mem_init_fail:
-	dev_err(pdev->dev, "Couldn't initialize memory\n");
-	cdnsp_halt(pdev);
-	cdnsp_reset(pdev);
-	cdnsp_mem_cleanup(pdev);
+free_erst:
+	cdnsp_free_erst(pdev, &pdev->erst);
+free_event_ring:
+	cdnsp_ring_free(pdev, pdev->event_ring);
+free_cmd_ring:
+	cdnsp_ring_free(pdev, pdev->cmd_ring);
+destroy_device_pool:
+	dma_pool_destroy(pdev->device_pool);
+destroy_segment_pool:
+	dma_pool_destroy(pdev->segment_pool);
+release_dcbaa:
+	dma_free_coherent(dev, sizeof(*pdev->dcbaa), pdev->dcbaa,
+			  pdev->dcbaa->dma);
 
 	return ret;
 }
--

2.29.2