dma_fence_dedup_array() returns 1 when called with num_fences == 0: the for-loop body never executes, j stays at 0, and the final `return ++j` yields 1. This contradicts both the kernel-doc ("Return: Number of unique fences remaining in the array") and the natural expectation that 0 input gives 0 output.
All in-tree callers currently filter num_fences == 0 before invoking this helper (__dma_fence_unwrap_merge() bails out via the `if (count == 0 || count == 1)` fast path; amdgpu_userq_wait_*() cannot reach the dedup call with a zero local count because the amdgpu_userq_wait_add_fence() helper guarantees num_fences stays in [0, wait_info->num_fences], and wait_info->num_fences > 0 is enforced at the ioctl entry).
However, dma_fence_dedup_array() is EXPORT_SYMBOL_GPL, so any future caller that forgets to pre-filter the zero case will get a misleading return value of 1. Depending on how that caller uses the result, it could dereference an uninitialized fence slot in the array, since the caller's array may have been allocated but not yet populated.
Make the contract match the documentation by returning 0 early. This also skips an unnecessary sort() call on an empty array.
Signed-off-by: Baineng Shou shoubaineng@gmail.com --- drivers/dma-buf/dma-fence-unwrap.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c index 53bb40e70b27..364cbf79ad73 100644 --- a/drivers/dma-buf/dma-fence-unwrap.c +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -97,6 +97,9 @@ int dma_fence_dedup_array(struct dma_fence **fences, int num_fences) { int i, j;
+ if (!num_fences) + return 0; + sort(fences, num_fences, sizeof(*fences), fence_cmp, NULL);
/*
On Mon, 2026-06-29 at 11:13 +0800, Baineng Shou wrote:
dma_fence_dedup_array() returns 1 when called with num_fences == 0: the for-loop body never executes, j stays at 0, and the final `return ++j` yields 1. This contradicts both the kernel-doc ("Return: Number of unique fences remaining in the array") and the natural expectation that 0 input gives 0 output.
All in-tree callers currently filter num_fences == 0 before invoking this helper (__dma_fence_unwrap_merge() bails out via the `if (count == 0 || count == 1)` fast path; amdgpu_userq_wait_*() cannot reach the dedup call with a zero local count because the amdgpu_userq_wait_add_fence() helper guarantees num_fences stays in [0, wait_info->num_fences], and wait_info->num_fences > 0 is enforced at the ioctl entry).
However, dma_fence_dedup_array() is EXPORT_SYMBOL_GPL, so any future caller that forgets to pre-filter the zero case will get a misleading return value of 1. Depending on how that caller uses the result, it could dereference an uninitialized fence slot in the array, since the caller's array may have been allocated but not yet populated.
Make the contract match the documentation by returning 0 early. This also skips an unnecessary sort() call on an empty array.
Signed-off-by: Baineng Shou shoubaineng@gmail.com
drivers/dma-buf/dma-fence-unwrap.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c index 53bb40e70b27..364cbf79ad73 100644 --- a/drivers/dma-buf/dma-fence-unwrap.c +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -97,6 +97,9 @@ int dma_fence_dedup_array(struct dma_fence **fences, int num_fences) { int i, j;
- if (!num_fences)
return 0;
Hm, since you're adding this, what about num_fences = -5 ?
Let me use this opportunity to also highlight that we should, in C, in general get away from using int for everything.
__dma_fence_unwrap_merge() directly below uses unsigned int, as you would expect. dma_fence_dedup_array() has almost no users. Probably we should change that API to unsigned int, too?
Regards P.
sort(fences, num_fences, sizeof(*fences), fence_cmp, NULL); /*
On 6/29/26 05:13, Baineng Shou wrote:
dma_fence_dedup_array() returns 1 when called with num_fences == 0: the for-loop body never executes, j stays at 0, and the final `return ++j` yields 1. This contradicts both the kernel-doc ("Return: Number of unique fences remaining in the array") and the natural expectation that 0 input gives 0 output.
Good catch.
All in-tree callers currently filter num_fences == 0 before invoking this helper (__dma_fence_unwrap_merge() bails out via the `if (count == 0 || count == 1)` fast path; amdgpu_userq_wait_*() cannot reach the dedup call with a zero local count because the amdgpu_userq_wait_add_fence() helper guarantees num_fences stays in [0, wait_info->num_fences], and wait_info->num_fences > 0 is enforced at the ioctl entry).
That's not correct, wait_info->num_fences is just the maximum amount of fences we return.
It is perfectly possible that amdgpu never finds any fences to add to the array.
However, dma_fence_dedup_array() is EXPORT_SYMBOL_GPL, so any future caller that forgets to pre-filter the zero case will get a misleading return value of 1. Depending on how that caller uses the result, it could dereference an uninitialized fence slot in the array, since the caller's array may have been allocated but not yet populated.
Make the contract match the documentation by returning 0 early. This also skips an unnecessary sort() call on an empty array.
Signed-off-by: Baineng Shou shoubaineng@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I will add a CC stable before pushing to drm-misc-fixes.
Thanks, Christian.
drivers/dma-buf/dma-fence-unwrap.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c index 53bb40e70b27..364cbf79ad73 100644 --- a/drivers/dma-buf/dma-fence-unwrap.c +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -97,6 +97,9 @@ int dma_fence_dedup_array(struct dma_fence **fences, int num_fences) { int i, j;
if (!num_fences)return 0;sort(fences, num_fences, sizeof(*fences), fence_cmp, NULL); /*-- 2.34.1
On Mon, 2026-06-29 at 10:45 +0200, Christian König wrote:
On 6/29/26 05:13, Baineng Shou wrote:
dma_fence_dedup_array() returns 1 when called with num_fences == 0: the for-loop body never executes, j stays at 0, and the final `return ++j` yields 1. This contradicts both the kernel-doc ("Return: Number of unique fences remaining in the array") and the natural expectation that 0 input gives 0 output.
Good catch.
All in-tree callers currently filter num_fences == 0 before invoking this helper (__dma_fence_unwrap_merge() bails out via the `if (count == 0 || count == 1)` fast path; amdgpu_userq_wait_*() cannot reach the dedup call with a zero local count because the amdgpu_userq_wait_add_fence() helper guarantees num_fences stays in [0, wait_info->num_fences], and wait_info->num_fences > 0 is enforced at the ioctl entry).
That's not correct, wait_info->num_fences is just the maximum amount of fences we return.
It is perfectly possible that amdgpu never finds any fences to add to the array.
However, dma_fence_dedup_array() is EXPORT_SYMBOL_GPL, so any future caller that forgets to pre-filter the zero case will get a misleading return value of 1. Depending on how that caller uses the result, it could dereference an uninitialized fence slot in the array, since the caller's array may have been allocated but not yet populated.
Make the contract match the documentation by returning 0 early. This also skips an unnecessary sort() call on an empty array.
Signed-off-by: Baineng Shou shoubaineng@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
What about my comments?
P.
On 6/29/26 10:49, Philipp Stanner wrote:
On Mon, 2026-06-29 at 10:45 +0200, Christian König wrote:
On 6/29/26 05:13, Baineng Shou wrote:
dma_fence_dedup_array() returns 1 when called with num_fences == 0: the for-loop body never executes, j stays at 0, and the final `return ++j` yields 1. This contradicts both the kernel-doc ("Return: Number of unique fences remaining in the array") and the natural expectation that 0 input gives 0 output.
Good catch.
All in-tree callers currently filter num_fences == 0 before invoking this helper (__dma_fence_unwrap_merge() bails out via the `if (count == 0 || count == 1)` fast path; amdgpu_userq_wait_*() cannot reach the dedup call with a zero local count because the amdgpu_userq_wait_add_fence() helper guarantees num_fences stays in [0, wait_info->num_fences], and wait_info->num_fences > 0 is enforced at the ioctl entry).
That's not correct, wait_info->num_fences is just the maximum amount of fences we return.
It is perfectly possible that amdgpu never finds any fences to add to the array.
However, dma_fence_dedup_array() is EXPORT_SYMBOL_GPL, so any future caller that forgets to pre-filter the zero case will get a misleading return value of 1. Depending on how that caller uses the result, it could dereference an uninitialized fence slot in the array, since the caller's array may have been allocated but not yet populated.
Make the contract match the documentation by returning 0 early. This also skips an unnecessary sort() call on an empty array.
Signed-off-by: Baineng Shou shoubaineng@gmail.com
Reviewed-by: Christian König christian.koenig@amd.com
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
I haven't seen that before writing the response.
I usually go over my mails till the end and wait a couple of hours before pushing anything.
What about my comments?
Looks valid to me as well, but I think that is a separate issue.
If I'm not completely mistaken we should use size_t instead of int for array sizes all around the place in those functions.
Regards, Christian.
P.
On Mon, 2026-06-29 at 10:52 +0200, Christian König wrote:
On 6/29/26 10:49, Philipp Stanner wrote:
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
I haven't seen that before writing the response.
I usually go over my mails till the end and wait a couple of hours before pushing anything.
What about my comments?
Looks valid to me as well, but I think that is a separate issue.
But if we keep it an integer for now, and if that check is added, and it most certainly should also catch negative integers, shouldn't it?
P.
On 6/29/26 11:06, Philipp Stanner wrote:
On Mon, 2026-06-29 at 10:52 +0200, Christian König wrote:
On 6/29/26 10:49, Philipp Stanner wrote:
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
I haven't seen that before writing the response.
I usually go over my mails till the end and wait a couple of hours before pushing anything.
What about my comments?
Looks valid to me as well, but I think that is a separate issue.
But if we keep it an integer for now, and if that check is added, and it most certainly should also catch negative integers, shouldn't it?
Maybe with a WARN_ON(), but not as regular code path.
The thing is I think we need to backport this fix to stable kernels, but switching from signed to unsigned is only a minor cleanup when no real users are currently affected.
Christian.
P.
On Mon, 2026-06-29 at 11:52 +0200, Christian König wrote:
On 6/29/26 11:06, Philipp Stanner wrote:
On Mon, 2026-06-29 at 10:52 +0200, Christian König wrote:
On 6/29/26 10:49, Philipp Stanner wrote:
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
I haven't seen that before writing the response.
I usually go over my mails till the end and wait a couple of hours before pushing anything.
What about my comments?
Looks valid to me as well, but I think that is a separate issue.
But if we keep it an integer for now, and if that check is added, and it most certainly should also catch negative integers, shouldn't it?
Maybe with a WARN_ON(), but not as regular code path.
The thing is I think we need to backport this fix to stable kernels, but switching from signed to unsigned is only a minor cleanup when no real users are currently affected.
Fine by me.
P.
On 6/29/26 12:16, Philipp Stanner wrote:
On Mon, 2026-06-29 at 11:52 +0200, Christian König wrote:
On 6/29/26 11:06, Philipp Stanner wrote:
On Mon, 2026-06-29 at 10:52 +0200, Christian König wrote:
On 6/29/26 10:49, Philipp Stanner wrote:
I will add a CC stable before pushing to drm-misc-fixes.
No offense intended or taken, but don't the DRM rules say that things do not get merged while there are outstanding concerns or significant points in review feedback?
I haven't seen that before writing the response.
I usually go over my mails till the end and wait a couple of hours before pushing anything.
What about my comments?
Looks valid to me as well, but I think that is a separate issue.
But if we keep it an integer for now, and if that check is added, and it most certainly should also catch negative integers, shouldn't it?
Maybe with a WARN_ON(), but not as regular code path.
The thing is I think we need to backport this fix to stable kernels, but switching from signed to unsigned is only a minor cleanup when no real users are currently affected.
Fine by me.
Thanks, I updated the commit message a bit and added Fixes and CC stable tags and pushed the result to drm-misc-fixes.
@Baineng it would be cool if you could write a test cases for this as well. E.g. just ~10 line in drivers/dma-buf/st-dma-fence-unwrap.c.
Thanks in advance, Christian.
P.
linaro-mm-sig@lists.linaro.org
-
Baineng Shou -
Christian König -
Philipp Stanner