| Message ID | 20211129073533.414008-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma_fence_array: Fix PENDING_ERROR leak in dma_fence_array_signaled() | expand |
Am 29.11.21 um 08:35 schrieb Thomas Hellström: > If a dma_fence_array is reported signaled by a call to > dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > > Fix this by clearing the PENDING_ERROR status if we return true in > dma_fence_array_signaled(). > > Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence-array container") > Cc: linaro-mm-sig@lists.linaro.org > Cc: Christian König <ckoenig.leichtzumerken@gmail.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence-array.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index d3fbd950be94..3e07f961e2f3 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct dma_fence *fence) > { > struct dma_fence_array *array = to_dma_fence_array(fence); > > - return atomic_read(&array->num_pending) <= 0; > + if (atomic_read(&array->num_pending) > 0) > + return false; > + > + dma_fence_array_clear_pending_error(array); > + return true; > } > > static void dma_fence_array_release(struct dma_fence *fence)
Hi, Christian, On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: > Am 29.11.21 um 08:35 schrieb Thomas Hellström: > > If a dma_fence_array is reported signaled by a call to > > dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > > > > Fix this by clearing the PENDING_ERROR status if we return true in > > dma_fence_array_signaled(). > > > > Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- > > array container") > > Cc: linaro-mm-sig@lists.linaro.org > > Cc: Christian König <ckoenig.leichtzumerken@gmail.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> How are the dma-buf / dma-fence patches typically merged? If i915 is the only fence->error user, could we take this through drm-intel to avoid a backmerge for upcoming i915 work? /Thomas > > > --- > > drivers/dma-buf/dma-fence-array.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- > > buf/dma-fence-array.c > > index d3fbd950be94..3e07f961e2f3 100644 > > --- a/drivers/dma-buf/dma-fence-array.c > > +++ b/drivers/dma-buf/dma-fence-array.c > > @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct > > dma_fence *fence) > > { > > struct dma_fence_array *array = to_dma_fence_array(fence); > > > > - return atomic_read(&array->num_pending) <= 0; > > + if (atomic_read(&array->num_pending) > 0) > > + return false; > > + > > + dma_fence_array_clear_pending_error(array); > > + return true; > > } > > > > static void dma_fence_array_release(struct dma_fence *fence) >
Am 29.11.21 um 13:23 schrieb Thomas Hellström: > Hi, Christian, > > On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: >> Am 29.11.21 um 08:35 schrieb Thomas Hellström: >>> If a dma_fence_array is reported signaled by a call to >>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. >>> >>> Fix this by clearing the PENDING_ERROR status if we return true in >>> dma_fence_array_signaled(). >>> >>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- >>> array container") >>> Cc: linaro-mm-sig@lists.linaro.org >>> Cc: Christian König <ckoenig.leichtzumerken@gmail.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> > How are the dma-buf / dma-fence patches typically merged? If i915 is > the only fence->error user, could we take this through drm-intel to > avoid a backmerge for upcoming i915 work? Well that one here looks like a bugfix to me, so either through drm-misc-fixes ore some i915 -fixes branch sounds fine to me. If you have any new development based on that a backmerge of the -fixes into your -next branch is unavoidable anyway. Regards, Christian. > > /Thomas > > >>> --- >>> drivers/dma-buf/dma-fence-array.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- >>> buf/dma-fence-array.c >>> index d3fbd950be94..3e07f961e2f3 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct >>> dma_fence *fence) >>> { >>> struct dma_fence_array *array = to_dma_fence_array(fence); >>> >>> - return atomic_read(&array->num_pending) <= 0; >>> + if (atomic_read(&array->num_pending) > 0) >>> + return false; >>> + >>> + dma_fence_array_clear_pending_error(array); >>> + return true; >>> } >>> >>> static void dma_fence_array_release(struct dma_fence *fence) >
On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote: > Am 29.11.21 um 13:23 schrieb Thomas Hellström: > > Hi, Christian, > > > > On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: > > > Am 29.11.21 um 08:35 schrieb Thomas Hellström: > > > > If a dma_fence_array is reported signaled by a call to > > > > dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > > > > > > > > Fix this by clearing the PENDING_ERROR status if we return true > > > > in > > > > dma_fence_array_signaled(). > > > > > > > > Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- > > > > array container") > > > > Cc: linaro-mm-sig@lists.linaro.org > > > > Cc: Christian König <ckoenig.leichtzumerken@gmail.com> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Thomas Hellström > > > > <thomas.hellstrom@linux.intel.com> > > > Reviewed-by: Christian König <christian.koenig@amd.com> > > How are the dma-buf / dma-fence patches typically merged? If i915 > > is > > the only fence->error user, could we take this through drm-intel to > > avoid a backmerge for upcoming i915 work? > > Well that one here looks like a bugfix to me, so either through > drm-misc-fixes ore some i915 -fixes branch sounds fine to me. > > If you have any new development based on that a backmerge of the - > fixes > into your -next branch is unavoidable anyway. Ok, I'll check with Joonas if I can take it through drm-intel-gt-next, since fixes are cherry-picked from that one. Patch will then appear in both the -fixes and the -next branch. Thanks, /Thomas > > Regards, > Christian. > > > > > /Thomas > > > > > > > > --- > > > > drivers/dma-buf/dma-fence-array.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- > > > > buf/dma-fence-array.c > > > > index d3fbd950be94..3e07f961e2f3 100644 > > > > --- a/drivers/dma-buf/dma-fence-array.c > > > > +++ b/drivers/dma-buf/dma-fence-array.c > > > > @@ -104,7 +104,11 @@ static bool > > > > dma_fence_array_signaled(struct > > > > dma_fence *fence) > > > > { > > > > struct dma_fence_array *array = > > > > to_dma_fence_array(fence); > > > > > > > > - return atomic_read(&array->num_pending) <= 0; > > > > + if (atomic_read(&array->num_pending) > 0) > > > > + return false; > > > > + > > > > + dma_fence_array_clear_pending_error(array); > > > > + return true; > > > > } > > > > > > > > static void dma_fence_array_release(struct dma_fence *fence) > > >
Am 29.11.21 um 13:46 schrieb Thomas Hellström: > On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote: >> Am 29.11.21 um 13:23 schrieb Thomas Hellström: >>> Hi, Christian, >>> >>> On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: >>>> Am 29.11.21 um 08:35 schrieb Thomas Hellström: >>>>> If a dma_fence_array is reported signaled by a call to >>>>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. >>>>> >>>>> Fix this by clearing the PENDING_ERROR status if we return true >>>>> in >>>>> dma_fence_array_signaled(). >>>>> >>>>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- >>>>> array container") >>>>> Cc: linaro-mm-sig@lists.linaro.org >>>>> Cc: Christian König <ckoenig.leichtzumerken@gmail.com> >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Signed-off-by: Thomas Hellström >>>>> <thomas.hellstrom@linux.intel.com> >>>> Reviewed-by: Christian König <christian.koenig@amd.com> >>> How are the dma-buf / dma-fence patches typically merged? If i915 >>> is >>> the only fence->error user, could we take this through drm-intel to >>> avoid a backmerge for upcoming i915 work? >> Well that one here looks like a bugfix to me, so either through >> drm-misc-fixes ore some i915 -fixes branch sounds fine to me. >> >> If you have any new development based on that a backmerge of the - >> fixes >> into your -next branch is unavoidable anyway. > Ok, I'll check with Joonas if I can take it through > drm-intel-gt-next, since fixes are cherry-picked from that one. Patch > will then appear in both the -fixes and the -next branch. Well exactly that's the stuff Daniel told me to avoid :) But maybe your i915 workflow is somehow better handling that than the AMD workflow. Christian. > > Thanks, > /Thomas > > >> Regards, >> Christian. >> >>> /Thomas >>> >>> >>>>> --- >>>>> drivers/dma-buf/dma-fence-array.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- >>>>> buf/dma-fence-array.c >>>>> index d3fbd950be94..3e07f961e2f3 100644 >>>>> --- a/drivers/dma-buf/dma-fence-array.c >>>>> +++ b/drivers/dma-buf/dma-fence-array.c >>>>> @@ -104,7 +104,11 @@ static bool >>>>> dma_fence_array_signaled(struct >>>>> dma_fence *fence) >>>>> { >>>>> struct dma_fence_array *array = >>>>> to_dma_fence_array(fence); >>>>> >>>>> - return atomic_read(&array->num_pending) <= 0; >>>>> + if (atomic_read(&array->num_pending) > 0) >>>>> + return false; >>>>> + >>>>> + dma_fence_array_clear_pending_error(array); >>>>> + return true; >>>>> } >>>>> >>>>> static void dma_fence_array_release(struct dma_fence *fence) >
(Switching to my @linux.intel.com address) Quoting Christian König (2021-11-29 14:55:37) > Am 29.11.21 um 13:46 schrieb Thomas Hellström: > > On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote: > >> Am 29.11.21 um 13:23 schrieb Thomas Hellström: > >>> Hi, Christian, > >>> > >>> On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: > >>>> Am 29.11.21 um 08:35 schrieb Thomas Hellström: > >>>>> If a dma_fence_array is reported signaled by a call to > >>>>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > >>>>> > >>>>> Fix this by clearing the PENDING_ERROR status if we return true > >>>>> in > >>>>> dma_fence_array_signaled(). > >>>>> > >>>>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- > >>>>> array container") > >>>>> Cc: linaro-mm-sig@lists.linaro.org > >>>>> Cc: Christian König <ckoenig.leichtzumerken@gmail.com> > >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>>>> Signed-off-by: Thomas Hellström > >>>>> <thomas.hellstrom@linux.intel.com> > >>>> Reviewed-by: Christian König <christian.koenig@amd.com> > >>> How are the dma-buf / dma-fence patches typically merged? If i915 > >>> is > >>> the only fence->error user, could we take this through drm-intel to > >>> avoid a backmerge for upcoming i915 work? > >> Well that one here looks like a bugfix to me, so either through > >> drm-misc-fixes ore some i915 -fixes branch sounds fine to me. > >> > >> If you have any new development based on that a backmerge of the - > >> fixes > >> into your -next branch is unavoidable anyway. > > Ok, I'll check with Joonas if I can take it through > > drm-intel-gt-next, since fixes are cherry-picked from that one. Patch > > will then appear in both the -fixes and the -next branch. > > Well exactly that's the stuff Daniel told me to avoid :) > > But maybe your i915 workflow is somehow better handling that than the > AMD workflow. If it's a bugfix to a patch that merged through drm-misc-next, I'd always be inclined to merge the fixup using the same process (which would be drm-next-fixes). In i915 we do always merge the patches to -next first, and never do a backmerge of -fixes (as it's a cherry-picked branch) so the workflows differ there. Here the time between the fixup and the previous patch is so long that either way is fine with. So feel free to apply to drm-intel-gt-next. Regards, Joonas > Christian. > > > > > Thanks, > > /Thomas > > > > > >> Regards, > >> Christian. > >> > >>> /Thomas > >>> > >>> > >>>>> --- > >>>>> drivers/dma-buf/dma-fence-array.c | 6 +++++- > >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- > >>>>> buf/dma-fence-array.c > >>>>> index d3fbd950be94..3e07f961e2f3 100644 > >>>>> --- a/drivers/dma-buf/dma-fence-array.c > >>>>> +++ b/drivers/dma-buf/dma-fence-array.c > >>>>> @@ -104,7 +104,11 @@ static bool > >>>>> dma_fence_array_signaled(struct > >>>>> dma_fence *fence) > >>>>> { > >>>>> struct dma_fence_array *array = > >>>>> to_dma_fence_array(fence); > >>>>> > >>>>> - return atomic_read(&array->num_pending) <= 0; > >>>>> + if (atomic_read(&array->num_pending) > 0) > >>>>> + return false; > >>>>> + > >>>>> + dma_fence_array_clear_pending_error(array); > >>>>> + return true; > >>>>> } > >>>>> > >>>>> static void dma_fence_array_release(struct dma_fence *fence) > > >
On Mon, Nov 29, 2021 at 03:14:35PM +0200, Joonas Lahtinen wrote: > (Switching to my @linux.intel.com address) > > Quoting Christian König (2021-11-29 14:55:37) > > Am 29.11.21 um 13:46 schrieb Thomas Hellström: > > > On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote: > > >> Am 29.11.21 um 13:23 schrieb Thomas Hellström: > > >>> Hi, Christian, > > >>> > > >>> On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: > > >>>> Am 29.11.21 um 08:35 schrieb Thomas Hellström: > > >>>>> If a dma_fence_array is reported signaled by a call to > > >>>>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > > >>>>> > > >>>>> Fix this by clearing the PENDING_ERROR status if we return true > > >>>>> in > > >>>>> dma_fence_array_signaled(). > > >>>>> > > >>>>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- > > >>>>> array container") > > >>>>> Cc: linaro-mm-sig@lists.linaro.org > > >>>>> Cc: Christian König <ckoenig.leichtzumerken@gmail.com> > > >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >>>>> Signed-off-by: Thomas Hellström > > >>>>> <thomas.hellstrom@linux.intel.com> > > >>>> Reviewed-by: Christian König <christian.koenig@amd.com> > > >>> How are the dma-buf / dma-fence patches typically merged? If i915 > > >>> is > > >>> the only fence->error user, could we take this through drm-intel to > > >>> avoid a backmerge for upcoming i915 work? > > >> Well that one here looks like a bugfix to me, so either through > > >> drm-misc-fixes ore some i915 -fixes branch sounds fine to me. > > >> > > >> If you have any new development based on that a backmerge of the - > > >> fixes > > >> into your -next branch is unavoidable anyway. > > > Ok, I'll check with Joonas if I can take it through > > > drm-intel-gt-next, since fixes are cherry-picked from that one. Patch > > > will then appear in both the -fixes and the -next branch. > > > > Well exactly that's the stuff Daniel told me to avoid :) > > > > But maybe your i915 workflow is somehow better handling that than the > > AMD workflow. > > If it's a bugfix to a patch that merged through drm-misc-next, I'd > always be inclined to merge the fixup using the same process (which > would be drm-next-fixes). > > In i915 we do always merge the patches to -next first, and never do a > backmerge of -fixes (as it's a cherry-picked branch) so the workflows > differ there. > > Here the time between the fixup and the previous patch is so long that > either way is fine with. So feel free to apply to drm-intel-gt-next. To make this clear and avoid confusion: drm-misc and drm-intel work differently for bugfixes. drm-intel has paid maintainers who take care of cherry-picking and testing and making sure nothing is lost. drm-misc is all volunteers, so committers need to make sure stuff ends up in the right place. Hence different rules. -Daniel > > Regards, Joonas > > > Christian. > > > > > > > > Thanks, > > > /Thomas > > > > > > > > >> Regards, > > >> Christian. > > >> > > >>> /Thomas > > >>> > > >>> > > >>>>> --- > > >>>>> drivers/dma-buf/dma-fence-array.c | 6 +++++- > > >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) > > >>>>> > > >>>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- > > >>>>> buf/dma-fence-array.c > > >>>>> index d3fbd950be94..3e07f961e2f3 100644 > > >>>>> --- a/drivers/dma-buf/dma-fence-array.c > > >>>>> +++ b/drivers/dma-buf/dma-fence-array.c > > >>>>> @@ -104,7 +104,11 @@ static bool > > >>>>> dma_fence_array_signaled(struct > > >>>>> dma_fence *fence) > > >>>>> { > > >>>>> struct dma_fence_array *array = > > >>>>> to_dma_fence_array(fence); > > >>>>> > > >>>>> - return atomic_read(&array->num_pending) <= 0; > > >>>>> + if (atomic_read(&array->num_pending) > 0) > > >>>>> + return false; > > >>>>> + > > >>>>> + dma_fence_array_clear_pending_error(array); > > >>>>> + return true; > > >>>>> } > > >>>>> > > >>>>> static void dma_fence_array_release(struct dma_fence *fence) > > > > > > _______________________________________________ > Linaro-mm-sig mailing list > Linaro-mm-sig@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index d3fbd950be94..3e07f961e2f3 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); - return atomic_read(&array->num_pending) <= 0; + if (atomic_read(&array->num_pending) > 0) + return false; + + dma_fence_array_clear_pending_error(array); + return true; } static void dma_fence_array_release(struct dma_fence *fence)
If a dma_fence_array is reported signaled by a call to dma_fence_is_signaled(), it may leak the PENDING_ERROR status. Fix this by clearing the PENDING_ERROR status if we return true in dma_fence_array_signaled(). Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence-array container") Cc: linaro-mm-sig@lists.linaro.org Cc: Christian König <ckoenig.leichtzumerken@gmail.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/dma-buf/dma-fence-array.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)