| Message ID | X9Cd6VGllTSlZtvV@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usb: cdnsp: fix error handling in cdnsp_mem_init() | expand |
Hi Dan, > >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. Thanks, Pawel > >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(-) > >diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c >index 6a0a12e1f54c..6d3fe72e920e 100644 >--- a/drivers/usb/cdns3/cdnsp-mem.c >+++ b/drivers/usb/cdns3/cdnsp-mem.c >@@ -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, >> >>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. 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 > >> >>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(-) >> >>diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c >>index 6a0a12e1f54c..6d3fe72e920e 100644 >>--- a/drivers/usb/cdns3/cdnsp-mem.c >>+++ b/drivers/usb/cdns3/cdnsp-mem.c >>@@ -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, Can you send this patch or can I do it for you. It has to be generated one again on top of Peter Chen for-usb-next branch. Regards, Pawel Laszczak >>Hi Dan, >>> >>>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); >> >>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 >> >>> >>>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(-) >>> >>>diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c >>>index 6a0a12e1f54c..6d3fe72e920e 100644 >>>--- a/drivers/usb/cdns3/cdnsp-mem.c >>>+++ b/drivers/usb/cdns3/cdnsp-mem.c >>>@@ -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
diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c index 6a0a12e1f54c..6d3fe72e920e 100644 --- a/drivers/usb/cdns3/cdnsp-mem.c +++ b/drivers/usb/cdns3/cdnsp-mem.c @@ -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; }