diff mbox series

drm/lease: use kvcalloc instead of kcalloc to avoid warn triggering.

Message ID 20240617035258.2774032-1-airlied@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/lease: use kvcalloc instead of kcalloc to avoid warn triggering. | expand

Commit Message

Dave Airlie June 17, 2024, 3:52 a.m. UTC 
From: Dave Airlie <airlied@redhat.com>

Attempt to use kvmalloc to avoid kmalloc warnings on larger allocations.

It might make sense to try and make a limit for the number of objects allowed here.

------------[ cut here ]------------
WARNING: CPU: 1 PID: 18929 at mm/page_alloc.c:4545 __alloc_pages+0x402/0x21b0 mm/page_alloc.c:4545
Modules linked in:
CPU: 1 PID: 18929 Comm: syz-executor.3 Not tainted 6.8.0 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:__alloc_pages+0x402/0x21b0 mm/page_alloc.c:4545
Code: ff 00 0f 84 15 fe ff ff 80 ce 01 e9 0d fe ff ff 83 fe 0a 0f 86 0e fd ff ff 80 3d c7 cf 6a 0d 00 75 0b c6 05 be cf 6a 0d 01 90 <0f> 0b 90 45 31 e4 e9 87 fe ff ff e8 5e 3e 9b ff 84 c0 0f 85 7a fe
RSP: 0018:ffffc90001cc7808 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffff92000398f14
RDX: 0000000000000001 RSI: 000000000000000b RDI: 0000000000040dc0
RBP: ffffc90001cc7ab8 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 00000000007a1200
R13: 000000000000000b R14: 0000000000040dc0 R15: 000000000000000b
FS:  00007f1717ba5640(0000) GS:ffff88807ec00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000203d0000 CR3: 000000001f5c0000 CR4: 0000000000750ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 <TASK>
 __alloc_pages_node include/linux/gfp.h:238 [inline]
 alloc_pages_node include/linux/gfp.h:261 [inline]
 __kmalloc_large_node+0x88/0x1a0 mm/slub.c:3926
 __do_kmalloc_node mm/slub.c:3969 [inline]
 __kmalloc+0x370/0x480 mm/slub.c:3994
 kmalloc_array include/linux/slab.h:627 [inline]
 kcalloc include/linux/slab.h:658 [inline]
 fill_object_idr drivers/gpu/drm/drm_lease.c:389 [inline]
 drm_mode_create_lease_ioctl+0x4ca/0x1f70 drivers/gpu/drm/drm_lease.c:522
 drm_ioctl_kernel+0x1eb/0x3f0 drivers/gpu/drm/drm_ioctl.c:744
 drm_ioctl+0x582/0xb70 drivers/gpu/drm/drm_ioctl.c:841
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:871 [inline]
 __se_sys_ioctl fs/ioctl.c:857 [inline]
 __x64_sys_ioctl+0x1a1/0x210 fs/ioctl.c:857
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xd5/0x270 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x6f/0x77
RIP: 0033:0x7f1716e8eeed
Code: c3 e8 97 2b 00 00 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f1717ba5028 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f1716fe3f80 RCX: 00007f1716e8eeed
RDX: 00000000200003c0 RSI: 00000000c01864c6 RDI: 0000000000000003
RBP: 00007f1716f13014 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000000b R14: 00007f1716fe3f80 R15: 00007f1717b85000
 </TASK>

Reported-by: Ubisectech Sirius <bugreport@ubisectech.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_lease.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Daniel Vetter June 17, 2024, 3:06 p.m. UTC | #1 
On Mon, Jun 17, 2024 at 01:52:58PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Attempt to use kvmalloc to avoid kmalloc warnings on larger allocations.
> 
> It might make sense to try and make a limit for the number of objects allowed here.
> 
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 18929 at mm/page_alloc.c:4545 __alloc_pages+0x402/0x21b0 mm/page_alloc.c:4545
> Modules linked in:
> CPU: 1 PID: 18929 Comm: syz-executor.3 Not tainted 6.8.0 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:__alloc_pages+0x402/0x21b0 mm/page_alloc.c:4545
> Code: ff 00 0f 84 15 fe ff ff 80 ce 01 e9 0d fe ff ff 83 fe 0a 0f 86 0e fd ff ff 80 3d c7 cf 6a 0d 00 75 0b c6 05 be cf 6a 0d 01 90 <0f> 0b 90 45 31 e4 e9 87 fe ff ff e8 5e 3e 9b ff 84 c0 0f 85 7a fe
> RSP: 0018:ffffc90001cc7808 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 1ffff92000398f14
> RDX: 0000000000000001 RSI: 000000000000000b RDI: 0000000000040dc0
> RBP: ffffc90001cc7ab8 R08: 0000000000000007 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 00000000007a1200
> R13: 000000000000000b R14: 0000000000040dc0 R15: 000000000000000b
> FS:  00007f1717ba5640(0000) GS:ffff88807ec00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000203d0000 CR3: 000000001f5c0000 CR4: 0000000000750ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  __alloc_pages_node include/linux/gfp.h:238 [inline]
>  alloc_pages_node include/linux/gfp.h:261 [inline]
>  __kmalloc_large_node+0x88/0x1a0 mm/slub.c:3926
>  __do_kmalloc_node mm/slub.c:3969 [inline]
>  __kmalloc+0x370/0x480 mm/slub.c:3994
>  kmalloc_array include/linux/slab.h:627 [inline]
>  kcalloc include/linux/slab.h:658 [inline]
>  fill_object_idr drivers/gpu/drm/drm_lease.c:389 [inline]
>  drm_mode_create_lease_ioctl+0x4ca/0x1f70 drivers/gpu/drm/drm_lease.c:522
>  drm_ioctl_kernel+0x1eb/0x3f0 drivers/gpu/drm/drm_ioctl.c:744
>  drm_ioctl+0x582/0xb70 drivers/gpu/drm/drm_ioctl.c:841
>  vfs_ioctl fs/ioctl.c:51 [inline]
>  __do_sys_ioctl fs/ioctl.c:871 [inline]
>  __se_sys_ioctl fs/ioctl.c:857 [inline]
>  __x64_sys_ioctl+0x1a1/0x210 fs/ioctl.c:857
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xd5/0x270 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x6f/0x77
> RIP: 0033:0x7f1716e8eeed
> Code: c3 e8 97 2b 00 00 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f1717ba5028 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00007f1716fe3f80 RCX: 00007f1716e8eeed
> RDX: 00000000200003c0 RSI: 00000000c01864c6 RDI: 0000000000000003
> RBP: 00007f1716f13014 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 000000000000000b R14: 00007f1716fe3f80 R15: 00007f1717b85000
>  </TASK>
> 
> Reported-by: Ubisectech Sirius <bugreport@ubisectech.com>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Hm this is a bit silly, because we really don't even need the objects[]
array:
- the first loop in could be put into the last one that builds the idr, at
  the cost of us having to tear down the entire idr on failure paths
  instead of releasing the objects[] array. Not a big deal because we
  don't have any real references to the objects, those are all in the main
  device idr, so an idr_destroy should be all it takes.

- validate_lease could also be put into that loop, which means we can
  drm_mode_object_put them as we go.

- because we fail on duplicated entries (not sure we have igts for that)
  with ENOSPC (bit an accidental error code) it's not really possible to
  fail

- I think I just realized that we have a use-after-free in the lease code
  for hotunpluggable connectors, because the lease idr does not hold a
  reference of its own for each object. So maybe more bugs here? Or at
  least the code that re-checks that the object still exists in
  drm_lease_create() and fails with ENOENT is a bit silly and maybe should
  be deleted. But maybe in another patch?

Anyway tldr; I think the unbounded objects[] array is a needless kernel
memory exhaustion path and should just go away completely ...

Cheers, Sima

> ---
>  drivers/gpu/drm/drm_lease.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 94375c6a5425..338716640725 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -386,8 +386,8 @@ static int fill_object_idr(struct drm_device *dev,
>  	int ret;
>  	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
>  
> -	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
> -			  GFP_KERNEL);
> +	objects = kvcalloc(object_count, sizeof(struct drm_mode_object *),
> +			   GFP_KERNEL);
>  	if (!objects)
>  		return -ENOMEM;
>  
> @@ -463,7 +463,7 @@ static int fill_object_idr(struct drm_device *dev,
>  		if (objects[o])
>  			drm_mode_object_put(objects[o]);
>  	}
> -	kfree(objects);
> +	kvfree(objects);
>  	return ret;
>  }
>  
> -- 
> 2.45.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index 94375c6a5425..338716640725 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -386,8 +386,8 @@  static int fill_object_idr(struct drm_device *dev,
 	int ret;
 	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
 
-	objects = kcalloc(object_count, sizeof(struct drm_mode_object *),
-			  GFP_KERNEL);
+	objects = kvcalloc(object_count, sizeof(struct drm_mode_object *),
+			   GFP_KERNEL);
 	if (!objects)
 		return -ENOMEM;
 
@@ -463,7 +463,7 @@  static int fill_object_idr(struct drm_device *dev,
 		if (objects[o])
 			drm_mode_object_put(objects[o]);
 	}
-	kfree(objects);
+	kvfree(objects);
 	return ret;
 }