drm/syncobj: Limit the number of handles and use kvmalloc for user-sized arrays

From: Fei Yu <[email protected]>

The syncobj ioctls allocate several temporary arrays whose size is taken directly from the u32 count_handles provided by userspace:

- handles and syncobjs in drm_syncobj_array_find()
- points and entries in drm_syncobj_array_wait_timeout()
- points and chains in drm_syncobj_timeline_signal_ioctl()

These are kmalloc() allocations, so a count above KMALLOC_MAX_SIZE / element size trips the WARN_ON_ONCE_GFP() in ___kmalloc_large_node():

  WARNING: mm/slub.c:5459 at ___kmalloc_large_node+0x10c/0x170 mm/slub.c:5459
  Call Trace:
   __kmalloc_large_node_noprof+0x18/0x90 mm/slub.c:5502
   __do_kmalloc_node mm/slub.c:5519 [inline]
   __kmalloc_noprof+0x3f6/0x710 mm/slub.c:5556
   _kmalloc_noprof include/linux/slab.h:995 [inline]
   _kmalloc_array_noprof include/linux/slab.h:1211 [inline]
   drm_syncobj_array_find+0x3a/0x440 drivers/gpu/drm/drm_syncobj.c:1241
   drm_syncobj_timeline_wait_ioctl+0x18a/0x640 drivers/gpu/drm/drm_syncobj.c:1356
   drm_ioctl_kernel+0x2df/0x3b0 drivers/gpu/drm/drm_ioctl.c:817
   drm_ioctl+0x70e/0xba0 drivers/gpu/drm/drm_ioctl.c:914

The wait entries are 64 bytes each on 64-bit, so with a 4 MiB KMALLOC_MAX_SIZE a wait on more than about 64K handles already fails today, with a warning.

Fix this in two parts:

- Reject more than DRM_SYNCOBJ_MAX_HANDLES handles with -EINVAL in drm_syncobj_array_find(), which every ioctl taking a handle array goes through. The limit is derived from the largest per-handle allocation (the wait entries) and the INT_MAX size limit of kvmalloc(), so none of the allocations above can exceed it. That is about 33M handles on 64-bit.

- Switch the six allocations to kvmalloc(), so that large but valid handle counts no longer depend on physically contiguous memory.

Using kvmalloc() alone is not enough: the reproducer asks for more than INT_MAX bytes, which hits the WARN_ON_ONCE() in __kvmalloc_node_noprof() instead.

Fixes 3e6fb72d6cef ("drm/syncobj: Add a syncobj_array_find helper")

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=95416f957d84e858b377

Suggested-by Christian König <[email protected]>

Signed-off-by Fei Yu <[email protected]>

---
v2:
- Drop __GFP_NOWARN (Christian).
- Add DRM_SYNCOBJ_MAX_HANDLES, return -EINVAL above it, and switch all six arrays to kvmalloc() (Christian).
- v1: https://lore.kernel.org/all/[email protected]/

About the limit: INT_MAX / sizeof(struct syncobj_wait_entry) is about 33M handles on 64-bit. It is the largest count for which every allocation stays below the kvmalloc() INT_MAX size limit, so it does not add a new policy number. Today the real limit is much lower: a wait on more than ~64K handles already fails with -ENOMEM (plus the warning), because the wait entries are a kmalloc() allocation larger than 4 MiB. If you prefer a lower, fixed limit (e.g. SZ_1M) or GFP_KERNEL_ACCOUNT for these arrays, I can change it.

Testing on v7.4-rc1 (x86_64, KASAN, virtio-gpu in QEMU):
- syzbot C reproducer: base warns in ___kmalloc_large_node() (size 0x100000920), patched is clean, ioctl returns -EINVAL.
- wait / timeline_wait / timeline_signal / reset / signal on 200000 signaled syncobjs: base fails wait and timeline_wait with -ENOMEM and the same warning; patched all succeed.
- count == DRM_SYNCOBJ_MAX_HANDLES gets past the check (-EFAULT from the short user buffer), count == DRM_SYNCOBJ_MAX_HANDLES + 1 returns -EINVAL for wait, reset and timeline_signal.
- kmemleak clean, checkpatch --strict clean, W=1 clean,
DRM KUnit 667/667 passed.
- IGT syncobj_basic, syncobj_wait, syncobj_timeline on virtio-gpu:
202/202 subtests pass, same as base.

 drivers/gpu/drm/drm_syncobj.c | 40 ++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 15 deletions(-)

drivers/gpu/drm/drm_syncobj.c +0 -0

index f1d4da1685457..c01cf164a4f9e 100644

drivers/gpu/drm/drm_syncobj.c +5 -4

+
 static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
                                      struct syncobj_wait_entry *wait);
 

@@ -1051,7 +1058,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                lockdep_assert_none_held_once();
        }
 
-       points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
+       points = kvmalloc_array(count, sizeof(*points), GFP_KERNEL);
        if (points == NULL)
                return -ENOMEM;
 

@@ -1064,7 +1071,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                goto err_free_points;
        }
 
-       entries = kzalloc_objs(*entries, count);
+       entries = kvzalloc_objs(*entries, count);
        if (!entries) {
                timeout = -ENOMEM;
                goto err_free_points;

@@ -1185,10 +1192,10 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                                                  &entries[i].fence_cb);
                dma_fence_put(entries[i].fence);
        }
-       kfree(entries);
+       kvfree(entries);
 
 err_free_points:
-       kfree(points);
+       kvfree(points);
 
        return timeout;
 }

drivers/gpu/drm/drm_syncobj.c +5 -5

        if (handles == NULL)
                return -ENOMEM;
 

@@ -1248,7 +1258,7 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
                goto err_free_handles;
        }
 
-       syncobjs = kmalloc_objs(*syncobjs, count_handles);
+       syncobjs = kvmalloc_objs(*syncobjs, count_handles);
        if (syncobjs == NULL) {
                ret = -ENOMEM;
                goto err_free_handles;

@@ -1262,16 +1272,16 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
                }
        }
 
-       kfree(handles);
+       kvfree(handles);
        *syncobjs_out = syncobjs;
        return 0;
 
 err_put_syncobjs:
        while (i-- > 0)
                drm_syncobj_put(syncobjs[i]);
-       kfree(syncobjs);
+       kvfree(syncobjs);
 err_free_handles:
-       kfree(handles);
+       kvfree(handles);
 
        return ret;
 }

@@ -1283,7 +1293,7 @@ static void drm_syncobj_array_free(struct drm_syncobj **syncobjs,
 
        for (i = 0; i < count; i++)
                drm_syncobj_put(syncobjs[i]);
-       kfree(syncobjs);
+       kvfree(syncobjs);
 }
 
 int

drivers/gpu/drm/drm_syncobj.c +3 -3

        if (!points) {
                ret = -ENOMEM;
                goto out;

@@ -1586,7 +1596,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
                goto err_points;
        }
 
-       chains = kmalloc_objs(*chains, args->count_handles);
+       chains = kvmalloc_objs(*chains, args->count_handles);
        if (!chains) {
                ret = -ENOMEM;
                goto err_points;

@@ -1609,9 +1619,9 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
                dma_fence_put(fence);
        }
 err_chains:
-       kfree(chains);
+       kvfree(chains);
 err_points:
-       kfree(points);
+       kvfree(points);
 out:
        drm_syncobj_array_free(syncobjs, args->count_handles);
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4 --
2.43.0

Quoted 87 lines · Fei

From: Fei Yu <[email protected]>

The syncobj ioctls allocate several temporary arrays whose size is taken directly from the u32 count_handles provided by userspace:

- handles and syncobjs in drm_syncobj_array_find()
- points and entries in drm_syncobj_array_wait_timeout()
- points and chains in drm_syncobj_timeline_signal_ioctl()

These are kmalloc() allocations, so a count above KMALLOC_MAX_SIZE / element size trips the WARN_ON_ONCE_GFP() in ___kmalloc_large_node():

   WARNING: mm/slub.c:5459 at ___kmalloc_large_node+0x10c/0x170 mm/slub.c:5459
   Call Trace:
    __kmalloc_large_node_noprof+0x18/0x90 mm/slub.c:5502
    __do_kmalloc_node mm/slub.c:5519 [inline]
    __kmalloc_noprof+0x3f6/0x710 mm/slub.c:5556
    _kmalloc_noprof include/linux/slab.h:995 [inline]
    _kmalloc_array_noprof include/linux/slab.h:1211 [inline]
    drm_syncobj_array_find+0x3a/0x440 drivers/gpu/drm/drm_syncobj.c:1241
    drm_syncobj_timeline_wait_ioctl+0x18a/0x640 drivers/gpu/drm/drm_syncobj.c:1356
    drm_ioctl_kernel+0x2df/0x3b0 drivers/gpu/drm/drm_ioctl.c:817
    drm_ioctl+0x70e/0xba0 drivers/gpu/drm/drm_ioctl.c:914

The wait entries are 64 bytes each on 64-bit, so with a 4 MiB KMALLOC_MAX_SIZE a wait on more than about 64K handles already fails today, with a warning.

Fix this in two parts:

- Reject more than DRM_SYNCOBJ_MAX_HANDLES handles with -EINVAL in drm_syncobj_array_find(), which every ioctl taking a handle array goes through. The limit is derived from the largest per-handle allocation (the wait entries) and the INT_MAX size limit of kvmalloc(), so none of the allocations above can exceed it. That is about 33M handles on 64-bit.

- Switch the six allocations to kvmalloc(), so that large but valid handle counts no longer depend on physically contiguous memory.

Using kvmalloc() alone is not enough: the reproducer asks for more than INT_MAX bytes, which hits the WARN_ON_ONCE() in __kvmalloc_node_noprof() instead.

Fixes 3e6fb72d6cef ("drm/syncobj: Add a syncobj_array_find helper")

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=95416f957d84e858b377

Suggested-by Christian König <[email protected]>

Signed-off-by Fei Yu <[email protected]>

---
v2:
- Drop __GFP_NOWARN (Christian).
- Add DRM_SYNCOBJ_MAX_HANDLES, return -EINVAL above it, and switch all six arrays to kvmalloc() (Christian).
- v1: https://lore.kernel.org/all/[email protected]/

About the limit: INT_MAX / sizeof(struct syncobj_wait_entry) is about 33M handles on 64-bit. It is the largest count for which every allocation stays below the kvmalloc() INT_MAX size limit, so it does not add a new policy number. Today the real limit is much lower: a wait on more than ~64K handles already fails with -ENOMEM (plus the warning), because the wait entries are a kmalloc() allocation larger than 4 MiB. If you prefer a lower, fixed limit (e.g. SZ_1M) or GFP_KERNEL_ACCOUNT for these arrays, I can change it.

Testing on v7.4-rc1 (x86_64, KASAN, virtio-gpu in QEMU):
- syzbot C reproducer: base warns in ___kmalloc_large_node() (size

    0x100000920), patched is clean, ioctl returns -EINVAL.
  - wait / timeline_wait / timeline_signal / reset / signal on 200000
    signaled syncobjs: base fails wait and timeline_wait with -ENOMEM and
    the same warning; patched all succeed.
  - count == DRM_SYNCOBJ_MAX_HANDLES gets past the check (-EFAULT from the
    short user buffer), count == DRM_SYNCOBJ_MAX_HANDLES + 1 returns -EINVAL
    for wait, reset and timeline_signal.
  - kmemleak clean, checkpatch --strict clean, W=1 clean,
    DRM KUnit 667/667 passed.

- IGT syncobj_basic, syncobj_wait, syncobj_timeline on virtio-gpu:
202/202 subtests pass, same as base.

  drivers/gpu/drm/drm_syncobj.c | 40 ++++++++++++++++++++++-------------
  1 file changed, 25 insertions(+), 15 deletions(-)

drivers/gpu/drm/drm_syncobj.c +0 -0

index f1d4da1685457..c01cf164a4f9e 100644

drivers/gpu/drm/drm_syncobj.c struct syncobj_wait_entry { · line 228

Quoted 9 lines

drivers/gpu/drm/drm_syncobj.c +6 -0

@@ -220,6 +220,13 @@ struct syncobj_wait_entry {
         u64    point;
  };
 
+/*
+ * Upper limit for the number of handles passed to a single ioctl. The
+ * largest per-handle allocation is the wait entry array, keep it below the
+ * INT_MAX size limit of kvmalloc().
+ */
+#define DRM_SYNCOBJ_MAX_HANDLES        (INT_MAX / sizeof(struct syncobj_wait_entry))

I really wonder if we should put that into the uAPI files?

Not that userspace is potentially using it, but this is part of the uAPI and makes sense for documentation purposes.

Quoted 39 lines

drivers/gpu/drm/drm_syncobj.c +5 -4

+
  static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
                                       struct syncobj_wait_entry *wait);
 

@@ -1051,7 +1058,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                 lockdep_assert_none_held_once();
         }
 
-       points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
+       points = kvmalloc_array(count, sizeof(*points), GFP_KERNEL);
         if (points == NULL)
                 return -ENOMEM;
 

@@ -1064,7 +1071,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                 goto err_free_points;
         }
 
-       entries = kzalloc_objs(*entries, count);
+       entries = kvzalloc_objs(*entries, count);
         if (!entries) {
                 timeout = -ENOMEM;
                 goto err_free_points;

@@ -1185,10 +1192,10 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
                                                   &entries[i].fence_cb);
                 dma_fence_put(entries[i].fence);
         }
-       kfree(entries);
+       kvfree(entries);
 
  err_free_points:
-       kfree(points);
+       kvfree(points);
 
         return timeout;
  }

@@ -1238,7 +1245,10 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
         struct drm_syncobj **syncobjs;
         int ret;
 

drivers/gpu/drm/drm_syncobj.c static int drm_syncobj_array_find(struct drm_file *file_private, · line 1251

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +4 -1

-       handles = kmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL);
+       if (count_handles > DRM_SYNCOBJ_MAX_HANDLES)
+               return -EINVAL;
+
+       handles = kvmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL);

That kvmalloc_array() + the following copy_from_user should probably be replaced by using vmemdup_array_user(). But that should of course be a separate patch.

Quoted 45 lines

drivers/gpu/drm/drm_syncobj.c +5 -5

         if (handles == NULL)
                 return -ENOMEM;
 

@@ -1248,7 +1258,7 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
                 goto err_free_handles;
         }
 
-       syncobjs = kmalloc_objs(*syncobjs, count_handles);
+       syncobjs = kvmalloc_objs(*syncobjs, count_handles);
         if (syncobjs == NULL) {
                 ret = -ENOMEM;
                 goto err_free_handles;

@@ -1262,16 +1272,16 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
                 }
         }
 
-       kfree(handles);
+       kvfree(handles);
         *syncobjs_out = syncobjs;
         return 0;
 
  err_put_syncobjs:
         while (i-- > 0)
                 drm_syncobj_put(syncobjs[i]);
-       kfree(syncobjs);
+       kvfree(syncobjs);
  err_free_handles:
-       kfree(handles);
+       kvfree(handles);
 
         return ret;
  }

@@ -1283,7 +1293,7 @@ static void drm_syncobj_array_free(struct drm_syncobj **syncobjs,
 
         for (i = 0; i < count; i++)
                 drm_syncobj_put(syncobjs[i]);
-       kfree(syncobjs);
+       kvfree(syncobjs);
  }
 
  int

@@ -1572,8 +1582,8 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
         if (ret < 0)
                 return ret;
 

drivers/gpu/drm/drm_syncobj.c drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, · line 1586

Quoted 4 lines

drivers/gpu/drm/drm_syncobj.c +2 -2

-       points = kmalloc_array(args->count_handles, sizeof(*points),
-                              GFP_KERNEL);
+       points = kvmalloc_array(args->count_handles, sizeof(*points),
+                               GFP_KERNEL);

Looking at the code the handling of allocating the array and the just memset()ing it to zero when it is not used is really sub-optimal.

But that is just a side comment and should probably be changed in a separate patch as well.

Apart from those minor comments the patch looks good to me.

Regards,
Christian.

Quoted 29 lines

drivers/gpu/drm/drm_syncobj.c +3 -3

         if (!points) {
                 ret = -ENOMEM;
                 goto out;

@@ -1586,7 +1596,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
                 goto err_points;
         }
 
-       chains = kmalloc_objs(*chains, args->count_handles);
+       chains = kvmalloc_objs(*chains, args->count_handles);
         if (!chains) {
                 ret = -ENOMEM;
                 goto err_points;

@@ -1609,9 +1619,9 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
                 dma_fence_put(fence);
         }
  err_chains:
-       kfree(chains);
+       kvfree(chains);
  err_points:
-       kfree(points);
+       kvfree(points);
  out:
         drm_syncobj_array_free(syncobjs, args->count_handles);
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4 --
2.43.0

Quoted 7 lines · Christian König

Quoted 1 line · Fei

+#define DRM_SYNCOBJ_MAX_HANDLES (INT_MAX / sizeof(struct syncobj_wait_entry))

I really wonder if we should put that into the uAPI files?

Not that userspace is potentially using it, but this is part of the uAPI and makes sense for documentation purposes.

Quoted 3 lines

That kvmalloc_array() + the following copy_from_user should probably be replaced by using vmemdup_array_user(). But that should of course be a separate patch.

Btw I tried to do that some time ago:

https://lore.kernel.org/dri-devel/[email protected]/#t

I have as local, more recent version which applies, but apparently never sent it.

Interesting part should also be the other patches from the above linked series which avoid allocations altogether for the vast majority of cases.

If there is interest I can re-base and re-send after this is merged.

From: Fei Yu <[email protected]>

Quoted 8 lines · Tvrtko Ursulin

Quoted 7 lines · Christian König

Quoted 1 line · Fei

+#define DRM_SYNCOBJ_MAX_HANDLES (INT_MAX / sizeof(struct syncobj_wait_entry))

I really wonder if we should put that into the uAPI files?

Not that userspace is potentially using it, but this is part of the uAPI and makes sense for documentation purposes.

I thought about that, but I'd prefer to keep it kernel internal, at least for this fix:

- The value is derived from the size of a kernel internal struct and from the kvmalloc() size limit, so it is not the same on 32-bit and 64-bit. To put it in drm.h I would have to replace it with a fixed number, and that number becomes ABI. We could not easily raise or lower it later.

- Tvrtko's series below avoids most of these allocations. Once that lands, the right limit may be different, and an internal define is easier to change.

- The patch has a Fixes tag, and keeping it out of the uAPI header keeps the stable backport small.

For documentation, I could send a follow-up that adds a sentence to the uAPI struct comments, saying that count_handles above an implementation defined limit is rejected with -EINVAL. That documents the behavior without fixing a number. Would that work for you?

Quoted 11 lines

Quoted 3 lines

That kvmalloc_array() + the following copy_from_user should probably be replaced by using vmemdup_array_user(). But that should of course be a separate patch.

Btw I tried to do that some time ago:

https://lore.kernel.org/dri-devel/[email protected]/#t

I have as local, more recent version which applies, but apparently never sent it.

Quoted 4 lines

Interesting part should also be the other patches from the above linked series which avoid allocations altogether for the vast majority of cases.

If there is interest I can re-base and re-send after this is merged.

Thanks, I wasn't aware of that series. I'll leave the vmemdup_array_user() and points array cleanups to it, so we don't duplicate work. Happy to test it once this fix is in.

Quoted 11 lines · Fei

From: Fei Yu <[email protected]>

Quoted 8 lines · Tvrtko Ursulin

Quoted 7 lines · Christian König

Quoted 1 line · Fei

+#define DRM_SYNCOBJ_MAX_HANDLES        (INT_MAX / sizeof(struct syncobj_wait_entry))

I really wonder if we should put that into the uAPI files?

Not that userspace is potentially using it, but this is part of the uAPI and makes sense for documentation purposes.

Quoted 6 lines

I thought about that, but I'd prefer to keep it kernel internal, at least for this fix:

  - The value is derived from the size of a kernel internal struct and from the kvmalloc() size limit, so it is not the same on 32-bit and 64-bit.

Quoted 3 lines

To put it in drm.h I would have to replace it with a fixed
    number, and that number becomes ABI. We could not easily raise or lower it later.

Quoted 5 lines

  - Tvrtko's series below avoids most of these allocations. Once that lands, the right limit may be different, and an internal define is easier to change.

Quoted 7 lines

  - The patch has a Fixes tag, and keeping it out of the uAPI header keeps the stable backport small.

For documentation, I could send a follow-up that adds a sentence to the uAPI struct comments, saying that count_handles above an implementation defined limit is rejected with -EINVAL. That documents the behavior without fixing a number. Would that work for you?

Quoted 15 lines

Quoted 15 lines

Quoted 4 lines

That kvmalloc_array() + the following copy_from_user should probably be replaced by using vmemdup_array_user(). But that should of course be a separate patch.

Btw I tried to do that some time ago:

https://lore.kernel.org/dri-devel/[email protected]/#t

I have as local, more recent version which applies, but apparently never sent it.

Interesting part should also be the other patches from the above linked series which avoid allocations altogether for the vast majority of cases.

Quoted 5 lines

Quoted 1 line

If there is interest I can re-base and re-send after this is merged.

Thanks, I wasn't aware of that series. I'll leave the vmemdup_array_user() and points array cleanups to it, so we don't duplicate work. Happy to test it once this fix is in.

Thanks - just to clarify though - I understand I wait for your patch to land and then rebase my series, right?

Regards,

Tvrtko

Quoted 4 lines · Tvrtko Ursulin

Quoted 3 lines · Fei

- The value is derived from the size of a kernel internal struct and from the kvmalloc() size limit, so it is not the same on 32-bit and 64-bit.

Oh that is a really good point, but most certainly a bad idea.

This means that code working on 32bit then suddenly fails on 64bit, not something we should do.

I think I prefer a fixed number here. Something reasonable large enough, like 64k or similar, should do for backporting.

But I would still use a fixed number and maybe a static assert that we don't hit the kvmalloc limit when we allocate that fixed number of structures.

From: Fei Yu <[email protected]>

Quoted 10 lines · Christian König

Quoted 4 lines · Tvrtko Ursulin

Quoted 3 lines · Fei

- The value is derived from the size of a kernel internal struct and from the kvmalloc() size limit, so it is not the same on 32-bit and 64-bit.

Oh that is a really good point, but most certainly a bad idea.

This means that code working on 32bit then suddenly fails on 64bit, not something we should do.

Agreed, a fixed number is better.

Quoted 2 lines

I think I prefer a fixed number here. Something reasonable large enough, like 64k or similar, should do for backporting.

One concern with 64K: today only wait and timeline_wait break above ~64K handles, because of the 64-byte wait entries. The other ioctls only allocate 4-8 bytes per handle, so on x86_64 reset, signal, query and timeline_signal currently work up to ~512K handles. In my v2 testing, reset, signal and timeline_signal on 200000 handles succeed on the base kernel. With a 64K limit they would start returning -EINVAL after this fix, which I would rather avoid in a stable backport.

So I would suggest SZ_1M instead:

- it is above what any of these ioctls can handle today, so nothing that works now starts failing;
- 1M wait entries are 64 MiB, far below the kvmalloc() INT_MAX limit;
- it is the same on 32-bit and 64-bit.

If you still prefer 64K, I'll go with that.

Quoted 3 lines

But I would still use a fixed number and maybe a static assert that we don't hit the kvmalloc limit when we allocate that fixed number of structures.

Will do. Something like:

  #define DRM_SYNCOBJ_MAX_HANDLES	SZ_1M
  static_assert(DRM_SYNCOBJ_MAX_HANDLES <=
                INT_MAX / sizeof(struct syncobj_wait_entry));

Plan for v3, for -fixes:

- fixed DRM_SYNCOBJ_MAX_HANDLES plus the static_assert, define kept in drm_syncobj.c;
- keep the kvmalloc() conversion;
- update the commit message accordingly.

The uAPI documentation of the limit will be a separate patch for -next once this has landed.

Regards,
Fei