| Message ID | 20211204205504.6550-1-amhamza.mgc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: venus: vdec: fixed possible memory leak issue | expand |
Hi Ameer, Thank you for investigating the alternative suggestion I made. Quoting Ameer Hamza (2021-12-04 20:55:04) > Fixed coverity warning by freeing the allocated memory before return We could probably say that fixing the coverity warning isn't so much the target of the patch as fixing the memory leak. It's just helpful that coverity spotted it for us. I'd write: The venus_helper_alloc_dpb_bufs() implementation allows an early return on an error path when checking the id from ida_alloc_min() which would not release the earlier buffer allocation. Move the direct kfree() from the error checking of dma_alloc_attrs() to the common fail path to ensure that allocations are released on all error paths in this function. > Addresses-Coverity: 1494120 ("Resource leak") > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> Of course having suggested it, I believe this is the right fix so: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > Changes in v2: > move kfree() immediately after kfree() as suggested by Kieran Bingham > --- > drivers/media/platform/qcom/venus/helpers.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > index 84c3a511ec31..0bca95d01650 100644 > --- a/drivers/media/platform/qcom/venus/helpers.c > +++ b/drivers/media/platform/qcom/venus/helpers.c > @@ -189,7 +189,6 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) > buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL, > buf->attrs); > if (!buf->va) { > - kfree(buf); > ret = -ENOMEM; > goto fail; > } > @@ -209,6 +208,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) > return 0; > > fail: > + kfree(buf); > venus_helper_free_dpb_bufs(inst); > return ret; > } > -- > 2.25.1 >
Quoting Kieran Bingham (2021-12-06 09:55:54) > Hi Ameer, > > Thank you for investigating the alternative suggestion I made. > > Quoting Ameer Hamza (2021-12-04 20:55:04) > > Fixed coverity warning by freeing the allocated memory before return > > We could probably say that fixing the coverity warning isn't so much the > target of the patch as fixing the memory leak. It's just helpful that > coverity spotted it for us. > > > I'd write: > > The venus_helper_alloc_dpb_bufs() implementation allows an early return > on an error path when checking the id from ida_alloc_min() which would > not release the earlier buffer allocation. > > Move the direct kfree() from the error checking of dma_alloc_attrs() to > the common fail path to ensure that allocations are released on all > error paths in this function. > > > Addresses-Coverity: 1494120 ("Resource leak") > > > > Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> > > Of course having suggested it, I believe this is the right fix so: > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Oh - and we should probably add a fixes tag: Fixes: 40d87aafee29 ("media: venus: vdec: decoded picture buffer handling during reconfig sequence") > > --- > > Changes in v2: > > move kfree() immediately after kfree() as suggested by Kieran Bingham > > --- > > drivers/media/platform/qcom/venus/helpers.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > > index 84c3a511ec31..0bca95d01650 100644 > > --- a/drivers/media/platform/qcom/venus/helpers.c > > +++ b/drivers/media/platform/qcom/venus/helpers.c > > @@ -189,7 +189,6 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) > > buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL, > > buf->attrs); > > if (!buf->va) { > > - kfree(buf); > > ret = -ENOMEM; > > goto fail; > > } > > @@ -209,6 +208,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) > > return 0; > > > > fail: > > + kfree(buf); > > venus_helper_free_dpb_bufs(inst); > > return ret; > > } > > -- > > 2.25.1 > >
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c index 84c3a511ec31..0bca95d01650 100644 --- a/drivers/media/platform/qcom/venus/helpers.c +++ b/drivers/media/platform/qcom/venus/helpers.c @@ -189,7 +189,6 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) buf->va = dma_alloc_attrs(dev, buf->size, &buf->da, GFP_KERNEL, buf->attrs); if (!buf->va) { - kfree(buf); ret = -ENOMEM; goto fail; } @@ -209,6 +208,7 @@ int venus_helper_alloc_dpb_bufs(struct venus_inst *inst) return 0; fail: + kfree(buf); venus_helper_free_dpb_bufs(inst); return ret; }
Fixed coverity warning by freeing the allocated memory before return Addresses-Coverity: 1494120 ("Resource leak") Signed-off-by: Ameer Hamza <amhamza.mgc@gmail.com> --- Changes in v2: move kfree() immediately after kfree() as suggested by Kieran Bingham --- drivers/media/platform/qcom/venus/helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)