| Message ID | c715ef30-9d91-46f3-8a0f-1039f3d25e7d@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [next] drm/syncobj: Fix leak in drm_syncobj_import_sync_file_fence() | expand |
On Thu, Apr 10, 2025 at 9:33 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > We need to cleanup if the chain = dma_fence_chain_alloc() allocation > fails. Now that we have multiple error returns in this function, switch > to using an unwind ladder for cleanup. > > Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/gpu/drm/drm_syncobj.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 636cd83ca29e..c136d0c772dc 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -745,21 +745,24 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, > { > struct dma_fence *fence = sync_file_get_fence(fd); > struct drm_syncobj *syncobj; > + int ret; > > if (!fence) > return -EINVAL; > > syncobj = drm_syncobj_find(file_private, handle); > if (!syncobj) { > - dma_fence_put(fence); > - return -ENOENT; > + ret = -ENOENT; > + goto err_put_fence; > } > > if (point) { > struct dma_fence_chain *chain = dma_fence_chain_alloc(); > > - if (!chain) > - return -ENOMEM; > + if (!chain) { > + ret = -ENOMEM; > + goto err_put_syncobj; > + } > > drm_syncobj_add_point(syncobj, chain, fence, point); > } else { > @@ -769,6 +772,13 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, > dma_fence_put(fence); > drm_syncobj_put(syncobj); > return 0; > + > +err_put_syncobj: > + drm_syncobj_put(syncobj); > +err_put_fence: > + dma_fence_put(fence); > + > + return ret; I suppose you could just initialize ret to zero and collapse the two return paths? Either way, Reviewed-by: Rob Clark <robdclark@gmail.com> > } > > static int drm_syncobj_export_sync_file(struct drm_file *file_private, > -- > 2.47.2 >
Am 11.04.25 um 00:06 schrieb Rob Clark: > On Thu, Apr 10, 2025 at 9:33 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: >> We need to cleanup if the chain = dma_fence_chain_alloc() allocation >> fails. Now that we have multiple error returns in this function, switch >> to using an unwind ladder for cleanup. >> >> Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs") >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> drivers/gpu/drm/drm_syncobj.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c >> index 636cd83ca29e..c136d0c772dc 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -745,21 +745,24 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, >> { >> struct dma_fence *fence = sync_file_get_fence(fd); >> struct drm_syncobj *syncobj; >> + int ret; >> >> if (!fence) >> return -EINVAL; >> >> syncobj = drm_syncobj_find(file_private, handle); >> if (!syncobj) { >> - dma_fence_put(fence); >> - return -ENOENT; >> + ret = -ENOENT; >> + goto err_put_fence; >> } >> >> if (point) { >> struct dma_fence_chain *chain = dma_fence_chain_alloc(); >> >> - if (!chain) >> - return -ENOMEM; >> + if (!chain) { >> + ret = -ENOMEM; >> + goto err_put_syncobj; >> + } >> >> drm_syncobj_add_point(syncobj, chain, fence, point); >> } else { >> @@ -769,6 +772,13 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, >> dma_fence_put(fence); >> drm_syncobj_put(syncobj); >> return 0; >> + >> +err_put_syncobj: >> + drm_syncobj_put(syncobj); >> +err_put_fence: >> + dma_fence_put(fence); >> + >> + return ret; > I suppose you could just initialize ret to zero and collapse the two > return paths? Either way, Yep, good point. With that done Reviewed-by: Christian König <christian.koenig@amd.com> as well. Regards, Christian. > > Reviewed-by: Rob Clark <robdclark@gmail.com> > >> } >> >> static int drm_syncobj_export_sync_file(struct drm_file *file_private, >> -- >> 2.47.2 >>
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 636cd83ca29e..c136d0c772dc 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -745,21 +745,24 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, { struct dma_fence *fence = sync_file_get_fence(fd); struct drm_syncobj *syncobj; + int ret; if (!fence) return -EINVAL; syncobj = drm_syncobj_find(file_private, handle); if (!syncobj) { - dma_fence_put(fence); - return -ENOENT; + ret = -ENOENT; + goto err_put_fence; } if (point) { struct dma_fence_chain *chain = dma_fence_chain_alloc(); - if (!chain) - return -ENOMEM; + if (!chain) { + ret = -ENOMEM; + goto err_put_syncobj; + } drm_syncobj_add_point(syncobj, chain, fence, point); } else { @@ -769,6 +772,13 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, dma_fence_put(fence); drm_syncobj_put(syncobj); return 0; + +err_put_syncobj: + drm_syncobj_put(syncobj); +err_put_fence: + dma_fence_put(fence); + + return ret; } static int drm_syncobj_export_sync_file(struct drm_file *file_private,
We need to cleanup if the chain = dma_fence_chain_alloc() allocation fails. Now that we have multiple error returns in this function, switch to using an unwind ladder for cleanup. Fixes: c2d3a7300695 ("drm/syncobj: Extend EXPORT_SYNC_FILE for timeline syncobjs") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/gpu/drm/drm_syncobj.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)