| Message ID | 20210624174732.1754546-1-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf/sync_file: Don't leak fences on merge failure | expand |
Am 24.06.21 um 19:47 schrieb Jason Ekstrand: > Each add_fence() call does a dma_fence_get() on the relevant fence. In > the error path, we weren't calling dma_fence_put() so all those fences > got leaked. Also, in the krealloc_array failure case, we weren't > freeing the fences array. Instead, ensure that i and fences are always > zero-initialized and dma_fence_put() all the fences and kfree(fences) on > every error path. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Cc: Christian König <christian.koenig@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/sync_file.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 20d9bddbb985b..394e6e1e96860 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -211,8 +211,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > struct sync_file *b) > { > struct sync_file *sync_file; > - struct dma_fence **fences, **nfences, **a_fences, **b_fences; > - int i, i_a, i_b, num_fences, a_num_fences, b_num_fences; > + struct dma_fence **fences = NULL, **nfences, **a_fences, **b_fences; > + int i = 0, i_a, i_b, num_fences, a_num_fences, b_num_fences; > > sync_file = sync_file_alloc(); > if (!sync_file) > @@ -236,7 +236,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > * If a sync_file can only be created with sync_file_merge > * and sync_file_create, this is a reasonable assumption. > */ > - for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > + for (i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > struct dma_fence *pt_a = a_fences[i_a]; > struct dma_fence *pt_b = b_fences[i_b]; > > @@ -277,15 +277,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > fences = nfences; > } > > - if (sync_file_set_fence(sync_file, fences, i) < 0) { > - kfree(fences); > + if (sync_file_set_fence(sync_file, fences, i) < 0) > goto err; > - } > > strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); > return sync_file; > > err: > + while (i) > + dma_fence_put(fences[--i]); > + kfree(fences); > fput(sync_file->file); > return NULL; >
I don't have drm-misc access. Mind pushing? On Thu, Jun 24, 2021 at 12:59 PM Christian König <christian.koenig@amd.com> wrote: > > Am 24.06.21 um 19:47 schrieb Jason Ekstrand: > > Each add_fence() call does a dma_fence_get() on the relevant fence. In > > the error path, we weren't calling dma_fence_put() so all those fences > > got leaked. Also, in the krealloc_array failure case, we weren't > > freeing the fences array. Instead, ensure that i and fences are always > > zero-initialized and dma_fence_put() all the fences and kfree(fences) on > > every error path. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") > > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Cc: Christian König <christian.koenig@amd.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> > > > --- > > drivers/dma-buf/sync_file.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > index 20d9bddbb985b..394e6e1e96860 100644 > > --- a/drivers/dma-buf/sync_file.c > > +++ b/drivers/dma-buf/sync_file.c > > @@ -211,8 +211,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > struct sync_file *b) > > { > > struct sync_file *sync_file; > > - struct dma_fence **fences, **nfences, **a_fences, **b_fences; > > - int i, i_a, i_b, num_fences, a_num_fences, b_num_fences; > > + struct dma_fence **fences = NULL, **nfences, **a_fences, **b_fences; > > + int i = 0, i_a, i_b, num_fences, a_num_fences, b_num_fences; > > > > sync_file = sync_file_alloc(); > > if (!sync_file) > > @@ -236,7 +236,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > * If a sync_file can only be created with sync_file_merge > > * and sync_file_create, this is a reasonable assumption. > > */ > > - for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > > + for (i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > > struct dma_fence *pt_a = a_fences[i_a]; > > struct dma_fence *pt_b = b_fences[i_b]; > > > > @@ -277,15 +277,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > fences = nfences; > > } > > > > - if (sync_file_set_fence(sync_file, fences, i) < 0) { > > - kfree(fences); > > + if (sync_file_set_fence(sync_file, fences, i) < 0) > > goto err; > > - } > > > > strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); > > return sync_file; > > > > err: > > + while (i) > > + dma_fence_put(fences[--i]); > > + kfree(fences); > > fput(sync_file->file); > > return NULL; > > >
Sorry for the vacation and sick leave induced delay. I've just pushed this to drm-misc-fixes. Thanks, Christian. Am 24.06.21 um 21:43 schrieb Jason Ekstrand: > I don't have drm-misc access. Mind pushing? > > On Thu, Jun 24, 2021 at 12:59 PM Christian König > <christian.koenig@amd.com> wrote: >> Am 24.06.21 um 19:47 schrieb Jason Ekstrand: >>> Each add_fence() call does a dma_fence_get() on the relevant fence. In >>> the error path, we weren't calling dma_fence_put() so all those fences >>> got leaked. Also, in the krealloc_array failure case, we weren't >>> freeing the fences array. Instead, ensure that i and fences are always >>> zero-initialized and dma_fence_put() all the fences and kfree(fences) on >>> every error path. >>> >>> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> >>> Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") >>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> Cc: Christian König <christian.koenig@amd.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> >> >>> --- >>> drivers/dma-buf/sync_file.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c >>> index 20d9bddbb985b..394e6e1e96860 100644 >>> --- a/drivers/dma-buf/sync_file.c >>> +++ b/drivers/dma-buf/sync_file.c >>> @@ -211,8 +211,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, >>> struct sync_file *b) >>> { >>> struct sync_file *sync_file; >>> - struct dma_fence **fences, **nfences, **a_fences, **b_fences; >>> - int i, i_a, i_b, num_fences, a_num_fences, b_num_fences; >>> + struct dma_fence **fences = NULL, **nfences, **a_fences, **b_fences; >>> + int i = 0, i_a, i_b, num_fences, a_num_fences, b_num_fences; >>> >>> sync_file = sync_file_alloc(); >>> if (!sync_file) >>> @@ -236,7 +236,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, >>> * If a sync_file can only be created with sync_file_merge >>> * and sync_file_create, this is a reasonable assumption. >>> */ >>> - for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { >>> + for (i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { >>> struct dma_fence *pt_a = a_fences[i_a]; >>> struct dma_fence *pt_b = b_fences[i_b]; >>> >>> @@ -277,15 +277,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, >>> fences = nfences; >>> } >>> >>> - if (sync_file_set_fence(sync_file, fences, i) < 0) { >>> - kfree(fences); >>> + if (sync_file_set_fence(sync_file, fences, i) < 0) >>> goto err; >>> - } >>> >>> strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); >>> return sync_file; >>> >>> err: >>> + while (i) >>> + dma_fence_put(fences[--i]); >>> + kfree(fences); >>> fput(sync_file->file); >>> return NULL; >>>
No worries. Thanks for pushing! On Mon, Jul 12, 2021 at 6:37 AM Christian König <christian.koenig@amd.com> wrote: > > Sorry for the vacation and sick leave induced delay. > > I've just pushed this to drm-misc-fixes. > > Thanks, > Christian. > > Am 24.06.21 um 21:43 schrieb Jason Ekstrand: > > I don't have drm-misc access. Mind pushing? > > > > On Thu, Jun 24, 2021 at 12:59 PM Christian König > > <christian.koenig@amd.com> wrote: > >> Am 24.06.21 um 19:47 schrieb Jason Ekstrand: > >>> Each add_fence() call does a dma_fence_get() on the relevant fence. In > >>> the error path, we weren't calling dma_fence_put() so all those fences > >>> got leaked. Also, in the krealloc_array failure case, we weren't > >>> freeing the fences array. Instead, ensure that i and fences are always > >>> zero-initialized and dma_fence_put() all the fences and kfree(fences) on > >>> every error path. > >>> > >>> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > >>> Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") > >>> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> Cc: Christian König <christian.koenig@amd.com> > >> Reviewed-by: Christian König <christian.koenig@amd.com> > >> > >>> --- > >>> drivers/dma-buf/sync_file.c | 13 +++++++------ > >>> 1 file changed, 7 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > >>> index 20d9bddbb985b..394e6e1e96860 100644 > >>> --- a/drivers/dma-buf/sync_file.c > >>> +++ b/drivers/dma-buf/sync_file.c > >>> @@ -211,8 +211,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > >>> struct sync_file *b) > >>> { > >>> struct sync_file *sync_file; > >>> - struct dma_fence **fences, **nfences, **a_fences, **b_fences; > >>> - int i, i_a, i_b, num_fences, a_num_fences, b_num_fences; > >>> + struct dma_fence **fences = NULL, **nfences, **a_fences, **b_fences; > >>> + int i = 0, i_a, i_b, num_fences, a_num_fences, b_num_fences; > >>> > >>> sync_file = sync_file_alloc(); > >>> if (!sync_file) > >>> @@ -236,7 +236,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > >>> * If a sync_file can only be created with sync_file_merge > >>> * and sync_file_create, this is a reasonable assumption. > >>> */ > >>> - for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > >>> + for (i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { > >>> struct dma_fence *pt_a = a_fences[i_a]; > >>> struct dma_fence *pt_b = b_fences[i_b]; > >>> > >>> @@ -277,15 +277,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > >>> fences = nfences; > >>> } > >>> > >>> - if (sync_file_set_fence(sync_file, fences, i) < 0) { > >>> - kfree(fences); > >>> + if (sync_file_set_fence(sync_file, fences, i) < 0) > >>> goto err; > >>> - } > >>> > >>> strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); > >>> return sync_file; > >>> > >>> err: > >>> + while (i) > >>> + dma_fence_put(fences[--i]); > >>> + kfree(fences); > >>> fput(sync_file->file); > >>> return NULL; > >>> >
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 20d9bddbb985b..394e6e1e96860 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -211,8 +211,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, struct sync_file *b) { struct sync_file *sync_file; - struct dma_fence **fences, **nfences, **a_fences, **b_fences; - int i, i_a, i_b, num_fences, a_num_fences, b_num_fences; + struct dma_fence **fences = NULL, **nfences, **a_fences, **b_fences; + int i = 0, i_a, i_b, num_fences, a_num_fences, b_num_fences; sync_file = sync_file_alloc(); if (!sync_file) @@ -236,7 +236,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, * If a sync_file can only be created with sync_file_merge * and sync_file_create, this is a reasonable assumption. */ - for (i = i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { + for (i_a = i_b = 0; i_a < a_num_fences && i_b < b_num_fences; ) { struct dma_fence *pt_a = a_fences[i_a]; struct dma_fence *pt_b = b_fences[i_b]; @@ -277,15 +277,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, fences = nfences; } - if (sync_file_set_fence(sync_file, fences, i) < 0) { - kfree(fences); + if (sync_file_set_fence(sync_file, fences, i) < 0) goto err; - } strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); return sync_file; err: + while (i) + dma_fence_put(fences[--i]); + kfree(fences); fput(sync_file->file); return NULL;
Each add_fence() call does a dma_fence_get() on the relevant fence. In the error path, we weren't calling dma_fence_put() so all those fences got leaked. Also, in the krealloc_array failure case, we weren't freeing the fences array. Instead, ensure that i and fences are always zero-initialized and dma_fence_put() all the fences and kfree(fences) on every error path. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Fixes: a02b9dc90d84 ("dma-buf/sync_file: refactor fence storage in struct sync_file") Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Cc: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/sync_file.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)