On 4/9/24 3:58 PM, Kevin Brodsky wrote:

Typo in title: "trasnport"

Or maybe simply say "to preserve capabilities"?

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

...

From: Carsten Haitzler carsten.haitzler@arm.com

DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. Also don't use stack as the kernel side buffer (optimization for smaller payloads) doesn't work.

Any idea why it doesn't work?

I don't actually know - I was just getting invalid capabilities as long as it was copying onto the stack buffer (128 bytes or less). I discovered moving it to allocated memory and it worked. It was a trivial change so I moved on.

...

Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com

drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_ioctl.c | 20 ++++++++------------ 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 446458aca8e9..76e3616158f3 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -651,7 +651,7 @@ ssize_t drm_read(struct file *filp, char __user *buffer, break; }

• 	if (copy_to_user(buffer + ret, e->event, length)) {
  

• 	if (copy_to_user_with_ptr(buffer + ret, e->event, length)) {
  	if (ret == 0)
  		ret = -EFAULT;
  	goto put_back_event;
  

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f2cda382f8ed..950a83828cc9 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -469,9 +469,9 @@ EXPORT_SYMBOL(drm_invalid_op); /*

• Copy and IOCTL return string to user space

*/ -static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) +static int drm_copy_field(char __user *buf, size_t __user *buf_len, const char *value)

buf_len is not a user pointer. Clang has this very unfortunate bug in hybrid where a integer pointer can be implicitly converted to a capability... that's probably why this went unnoticed.

Ummm... oh yeah... i may have gone overboard :) it happened to work and nothing pointed me to it. the buf is userspace - buf_len is not provided as such by userspace...

...

{

• size_t len;

• size_t len, len_orig;

/* don't attempt to copy a NULL pointer */ if (WARN_ONCE(!value, "BUG: the value to copy was not set!")) { @@ -480,13 +480,13 @@ static int drm_copy_field(char __user *buf, size_t *buf_len, const char *value) } /* don't overflow userbuf */

• len = strlen(value);

• len = len_orig = strlen(value); if (len > *buf_len) len = *buf_len;

/* let userspace know exact length of driver value (which could be * larger than the userspace-supplied buffer) */

• *buf_len = strlen(value);

• *buf_len = len_orig;

That looks like a nice improvement (removing an unnecessary call to strlen()) but isn't it unrelated to what this patch does overall?

Good point - it snuck in with it... I can split this out.

Kevin

...

/* finally, try filling in the userbuf */ if (len && buf) @@ -821,7 +821,6 @@ long drm_ioctl(struct file *filp, drm_ioctl_t *func; unsigned int nr = DRM_IOCTL_NR(cmd); int retcode = -EINVAL;

• char stack_kdata[128]; char *kdata = NULL; unsigned int in_size, out_size, drv_size, ksize; bool is_driver_ioctl;

@@ -874,9 +873,7 @@ long drm_ioctl(struct file *filp, goto err_i1; }

• if (ksize <= sizeof(stack_kdata)) {

• kdata = stack_kdata;
  

• } else {

• if (ksize > 0) { kdata = kmalloc(ksize, GFP_KERNEL); if (!kdata) { retcode = -ENOMEM;

@@ -884,16 +881,15 @@ long drm_ioctl(struct file *filp, } }

• if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) {

• if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) { retcode = -EFAULT; goto err_i1; }

• if (ksize > in_size) memset(kdata + in_size, 0, ksize - in_size);

retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);

• if (copy_to_user((void __user *)arg, kdata, out_size) != 0)

• if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0) retcode = -EFAULT;

err_i1: @@ -904,7 +900,7 @@ long drm_ioctl(struct file *filp, (long)old_encode_dev(file_priv->minor->kdev->devt), file_priv->authenticated, cmd, nr);

• if (kdata != stack_kdata)

• if (kdata != NULL) kfree(kdata); if (retcode) drm_dbg_core(dev, "comm="%s", pid=%d, ret=%d\n",

On 4/12/24 10:42 AM, Kevin Brodsky wrote:

On 11/04/2024 14:19, Carsten Haitzler wrote:

...

On 4/9/24 3:58 PM, Kevin Brodsky wrote:

...

Typo in title: "trasnport"

Or maybe simply say "to preserve capabilities"?

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

...

From: Carsten Haitzler carsten.haitzler@arm.com

DRM makes copies of ioctl payload data to/from userspace and some of this can also have pointers to other memory (or capabilities for morello) and thus it needs to assume the worst and copy while retaining capabilities. Also don't use stack as the kernel side buffer (optimization for smaller payloads) doesn't work.

Any idea why it doesn't work?

I don't actually know - I was just getting invalid capabilities as long as it was copying onto the stack buffer (128 bytes or less). I discovered moving it to allocated memory and it worked. It was a trivial change so I moved on.

Actually I know why, having had another look. stack_kdata is an array of char, so its alignment is 1. Considering the other local variables, it's very likely that it will not end up being 16-byte aligned, and as a result it cannot be used as storage for a struct with capabilities. That could be fixed by adding some alignment attribute. Getting rid of it is probably ok too, let's just explain why.

oh good point - i found the heap soln immediately and moved on - alignment very likely is it. in fact.. .this could have been triggered elsewhere too in theory... x86 wouldn't care. a bit of a pain to align on the stack (gcc extns), but as the kernel does believe in gcc ... i assume this will be ok (clang should support it...)

btw this pattern of putting some ioctl data on the heap is used in a few other places too. dma_heap_ioctl() in dmabuf, habanalabs driver, amd driver... this is a bit of a bomb waiting to explode :)

Kevin

...

[...]

...

...

@@ -821,7 +821,6 @@ long drm_ioctl(struct file *filp,       drm_ioctl_t *func;       unsigned int nr = DRM_IOCTL_NR(cmd);       int retcode = -EINVAL; -    char stack_kdata[128];       char *kdata = NULL;       unsigned int in_size, out_size, drv_size, ksize;       bool is_driver_ioctl; @@ -874,9 +873,7 @@ long drm_ioctl(struct file *filp,           goto err_i1;       }   -    if (ksize <= sizeof(stack_kdata)) { -        kdata = stack_kdata; -    } else { +    if (ksize > 0) {           kdata = kmalloc(ksize, GFP_KERNEL);           if (!kdata) {               retcode = -ENOMEM; @@ -884,16 +881,15 @@ long drm_ioctl(struct file *filp,           }       }   -    if (copy_from_user(kdata, (void __user *)arg, in_size) != 0) { +    if (copy_from_user_with_ptr(kdata, (void __user *)arg, in_size) != 0) {           retcode = -EFAULT;           goto err_i1;       }

if (ksize > in_size)           memset(kdata + in_size, 0, ksize - in_size);         retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags); -    if (copy_to_user((void __user *)arg, kdata, out_size) != 0) +    if (copy_to_user_with_ptr((void __user *)arg, kdata, out_size) != 0)           retcode = -EFAULT;           err_i1: @@ -904,7 +900,7 @@ long drm_ioctl(struct file *filp,                    (long)old_encode_dev(file_priv->minor->kdev->devt),                    file_priv->authenticated, cmd, nr);   -    if (kdata != stack_kdata) +    if (kdata != NULL)           kfree(kdata);       if (retcode)           drm_dbg_core(dev, "comm="%s", pid=%d, ret=%d\n",

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

From: Carsten Haitzler carsten.haitzler@arm.com

Vblank is a bit different because it produces events that then can copy bpointers/capabilities back to userspace via a read on the DRM

s/bpointers/pointers/

device as opposed to the ioctl. The compat handler called the normal native/purecap one and this happened to work for compat but not for purecap. Make the shared "native" purecap handling work for both and share that infra properly to avoid copy and paste.

Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com

drivers/gpu/drm/drm_atomic_uapi.c | 3 +- drivers/gpu/drm/drm_internal.h | 4 +++ drivers/gpu/drm/drm_ioc32.c | 8 +++++- drivers/gpu/drm/drm_vblank.c | 46 +++++++++++++++++++++++-------- include/drm/drm_vblank.h | 23 ++++++++++++++++ include/uapi/drm/drm.h | 6 ++-- 6 files changed, 74 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 3e4668a20157..08474a1ea90e 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -940,7 +940,8 @@ int drm_atomic_get_property(struct drm_mode_object *obj, */ static struct drm_pending_vblank_event *create_vblank_event(

• struct drm_crtc *crtc, uint64_t user_data)
  

• struct drm_crtc *crtc,
  

• __kernel_uintptr_t user_data)
  

We're not changing struct drm_mode_atomic in this patch so I suspect this belongs to the next one.

{ struct drm_pending_vblank_event *e = NULL; diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 8462b657c375..2d588df41cb5 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -23,6 +23,7 @@ #include <linux/kthread.h> +#include <drm/drm.h>

Doesn't seem to be needed.

#include <drm/drm_ioctl.h> #include <drm/drm_vblank.h> @@ -113,6 +114,9 @@ void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank); void drm_handle_vblank_works(struct drm_vblank_crtc *vblank); /* IOCTLS */ +int drm_wait_vblank_ioctl_internal(struct drm_device *dev, void *data,

• 		   struct drm_file *filp,
  

• 		   bool compat);
  

int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int drm_legacy_modeset_ctl_ioctl(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..d6a8c7250dda 100644 --- a/drivers/gpu/drm/drm_ioc32.c +++ b/drivers/gpu/drm/drm_ioc32.c @@ -33,6 +33,7 @@ #include <drm/drm_file.h> #include <drm/drm_print.h> +#include <drm/drm_drv.h> #include "drm_crtc_internal.h" #include "drm_internal.h" @@ -841,11 +842,16 @@ typedef union drm_wait_vblank32 { static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, user_uintptr_t arg) {

• struct drm_file *file_priv = file->private_data;
• struct drm_device *dev = file_priv->minor->dev; drm_wait_vblank32_t __user *argp = (void __user *)arg; drm_wait_vblank32_t req32; union drm_wait_vblank req; int err;

• if (drm_dev_is_unplugged(dev))

• return -ENODEV;
  

• if (copy_from_user(&req32, argp, sizeof(req32))) return -EFAULT;

@@ -854,7 +860,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, req.request.type = req32.request.type; req.request.sequence = req32.request.sequence; req.request.signal = req32.request.signal;

• err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED);

• err = drm_wait_vblank_ioctl_internal(dev, &req, file_priv, true);

I'm not sure bypassing drm_ioctl_kernel() is 100% correct, as it does extra things like calling drm_ioctl_permit(). Couldn't we define our own callback in this file that would itself call drm_wait_vblank_ioctl_internal(..., true)?

req32.reply.type = req.reply.type; req32.reply.sequence = req.reply.sequence;

[...]

/**

• struct drm_pending_vblank_event - pending vblank event tracking

*/ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event { * @sequence: frame event should be triggered at */ u64 sequence;

• /**

• * @compat: This is a compat32 event
  

Aren't we precisely using this for compat64 (arguably not so useful in compat32)?

• */
  

• bool compat; /**
  • @event: Actual event which will be sent to userspace.
  */

@@ -75,6 +95,9 @@ struct drm_pending_vblank_event { * @event.seq: Event payload for the MODE_QUEUEU_SEQUENCE IOCTL. */ struct drm_event_crtc_sequence seq;

• struct drm_event_vblank32 vbl32;
  

• struct drm_event_crtc_sequence32 seq32;
  

  } event;

}; diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index de723566c5ae..5347b76005ee 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -494,7 +494,7 @@ enum drm_vblank_seq_type { struct drm_wait_vblank_request { enum drm_vblank_seq_type type; unsigned int sequence;

• unsigned long signal;

• __kernel_uintptr_t signal;

Just checking I'm getting this right, this is used to hold some user_data, and as a result needs to be expanded too, right?

}; struct drm_wait_vblank_reply { @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */

• __u64 user_data; /* user data passed to event */

• __u64 user_data; /* user data passed to event */

The actual type change (to __kernel_uintptr_t) is in patch 3, it should be in this patch instead.

}; #if defined(__cplusplus) @@ -1277,7 +1277,7 @@ struct drm_event { struct drm_event_vblank { struct drm_event base;

• __u64 user_data;

• uintptr_t user_data;

__kernel_uintptr_t surely?

It is surprising that you didn't run into trouble here, because the kernel and userspace views will diverge in purecap (the former will see user_data as 64-bit and the latter as a capability).

Kevin

__u32 tv_sec; __u32 tv_usec; __u32 sequence;

On 11/04/2024 14:18, Carsten Haitzler wrote:

[...]

...

...

@@ -854,7 +860,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,       req.request.type = req32.request.type;       req.request.sequence = req32.request.sequence;       req.request.signal = req32.request.signal; -    err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED); +    err = drm_wait_vblank_ioctl_internal(dev, &req, file_priv, true);

I'm not sure bypassing drm_ioctl_kernel() is 100% correct, as it does extra things like calling drm_ioctl_permit(). Couldn't we define our own callback in this file that would itself call drm_wait_vblank_ioctl_internal(..., true)?

but compat_drm_wait_vblank() should have already been through these checks... right as this is called deeper down from the existing kernel ioctl handling infra and we should be done with all of that.

I don't have much understanding of the DRM infrastructure, but I can see that drm_ioctl_permit() is only called from drm_ioctl_kernel(), so I don't think it will ever get called if we call drm_wait_vblank_ioctl_internal() directly here.

...

...

req32.reply.type = req.reply.type;       req32.reply.sequence = req.reply.sequence;

[...]

/**    * struct drm_pending_vblank_event - pending vblank event tracking    */ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event {        * @sequence: frame event should be triggered at        */       u64 sequence; +    /** +     * @compat: This is a compat32 event

Aren't we precisely using this for compat64 (arguably not so useful in compat32)?

This is a naming thing - drm thinks compat is compat32 by name. we're re-using that to fake a compat64 system. there is an argument where there should be a whole separate compat64 impl in addition to compat32 but that would make our patchset a lot larger (and thus harder to maintain). So where you see these things it means here the vblank event being stored was requested by the drm_ioctl32 (compat3) subsystem which is actually for us compat64... but in the event this were built for a old armv7+armv8 system it'd still work and it would literally be the real compat32 code... :)

Sure I get that. My point is that saying "compat32" in the comment is misleading because what this really means is that we come from compat. It could be compat32 or compat64, that makes no difference to how we handle it.

[...] @@ -1277,7 +1277,7 @@ struct drm_event {

...

...

struct drm_event_vblank {       struct drm_event base; -    __u64 user_data; +    uintptr_t user_data;

__kernel_uintptr_t surely?

It is surprising that you didn't run into trouble here, because the kernel and userspace views will diverge in purecap (the former will see user_data as 64-bit and the latter as a capability).

hmmm how did i mess this up (and yet vblank tests work)... its the tools/include/uapi/drm/drm.h that should be uintptr_t and this one __kernel_uintptr_t.

We shouldn't use uintptr_t in tools/include either. uintptr_t is 32-bit on a 32-bit arch, but this is always at least 64-bit. __kernel_uintptr_t can be used in those headers as well thanks to [1].

Kevin

[1] https://git.morello-project.org/morello/kernel/linux/-/commit/eb1e26c38f05

On 4/12/24 10:53 AM, Kevin Brodsky wrote:

On 11/04/2024 14:18, Carsten Haitzler wrote:

...

[...]

...

...

@@ -854,7 +860,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,       req.request.type = req32.request.type;       req.request.sequence = req32.request.sequence;       req.request.signal = req32.request.signal; -    err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED); +    err = drm_wait_vblank_ioctl_internal(dev, &req, file_priv, true);

I'm not sure bypassing drm_ioctl_kernel() is 100% correct, as it does extra things like calling drm_ioctl_permit(). Couldn't we define our own callback in this file that would itself call drm_wait_vblank_ioctl_internal(..., true)?

but compat_drm_wait_vblank() should have already been through these checks... right as this is called deeper down from the existing kernel ioctl handling infra and we should be done with all of that.

I don't have much understanding of the DRM infrastructure, but I can see that drm_ioctl_permit() is only called from drm_ioctl_kernel(), so I don't think it will ever get called if we call drm_wait_vblank_ioctl_internal() directly here.

...

...

...

req32.reply.type = req.reply.type;       req32.reply.sequence = req.reply.sequence;

[...]

/**    * struct drm_pending_vblank_event - pending vblank event tracking    */ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event {        * @sequence: frame event should be triggered at        */       u64 sequence; +    /** +     * @compat: This is a compat32 event

Aren't we precisely using this for compat64 (arguably not so useful in compat32)?

This is a naming thing - drm thinks compat is compat32 by name. we're re-using that to fake a compat64 system. there is an argument where there should be a whole separate compat64 impl in addition to compat32 but that would make our patchset a lot larger (and thus harder to maintain). So where you see these things it means here the vblank event being stored was requested by the drm_ioctl32 (compat3) subsystem which is actually for us compat64... but in the event this were built for a old armv7+armv8 system it'd still work and it would literally be the real compat32 code... :)

Sure I get that. My point is that saying "compat32" in the comment is misleading because what this really means is that we come from compat. It could be compat32 or compat64, that makes no difference to how we handle it.

...

[...] @@ -1277,7 +1277,7 @@ struct drm_event {

...

...

struct drm_event_vblank {       struct drm_event base; -    __u64 user_data; +    uintptr_t user_data;

__kernel_uintptr_t surely?

It is surprising that you didn't run into trouble here, because the kernel and userspace views will diverge in purecap (the former will see user_data as 64-bit and the latter as a capability).

hmmm how did i mess this up (and yet vblank tests work)... its the tools/include/uapi/drm/drm.h that should be uintptr_t and this one __kernel_uintptr_t.

We shouldn't use uintptr_t in tools/include either. uintptr_t is 32-bit on a 32-bit arch, but this is always at least 64-bit. __kernel_uintptr_t can be used in those headers as well thanks to [1].

user data for vblank events was originally defined as a ulong - thus uintptr_t is actually correct here. it's called signal in public drm headers - but it is the user_data. vblank is special in drm because of this. :)

On 4/12/24 12:40 PM, Kevin Brodsky wrote:

On 12/04/2024 13:20, Carsten Haitzler wrote:

...

[...]

...

...

[...] @@ -1277,7 +1277,7 @@ struct drm_event {

...

...

struct drm_event_vblank {        struct drm_event base; -    __u64 user_data; +    uintptr_t user_data;

__kernel_uintptr_t surely?

It is surprising that you didn't run into trouble here, because the kernel and userspace views will diverge in purecap (the former will see user_data as 64-bit and the latter as a capability).

hmmm how did i mess this up (and yet vblank tests work)... its the tools/include/uapi/drm/drm.h that should be uintptr_t and this one __kernel_uintptr_t.

We shouldn't use uintptr_t in tools/include either. uintptr_t is 32-bit on a 32-bit arch, but this is always at least 64-bit. __kernel_uintptr_t can be used in those headers as well thanks to [1].

user data for vblank events was originally defined as a ulong - thus uintptr_t is actually correct here. it's called signal in public drm headers - but it is the user_data. vblank is special in drm because of this. :)

But struct drm_event_vblank::user_data is __u64 in include/uapi/drm/drm.h. Are you saying that userspace doesn't use that? In which case, was the uapi header broken all along and should it be fixed upstream?

it's not used. libdrm ships its own drm headers and thus i've patched those separately in libdrm itself. they have this as a ulong. :) it's always been this way... broken by design™ :)

On 4/12/24 2:57 PM, Kevin Brodsky wrote:

On 12/04/2024 14:49, Carsten Haitzler wrote:

...

On 4/12/24 12:40 PM, Kevin Brodsky wrote:

...

On 12/04/2024 13:20, Carsten Haitzler wrote:

...

[...]

...

...

[...] @@ -1277,7 +1277,7 @@ struct drm_event { >>       struct drm_event_vblank { >>         struct drm_event base; >> -    __u64 user_data; >> +    uintptr_t user_data; > > __kernel_uintptr_t surely? > > It is surprising that you didn't run into trouble here, because the > kernel and userspace views will diverge in purecap (the former will > see > user_data as 64-bit and the latter as a capability).

hmmm how did i mess this up (and yet vblank tests work)... its the tools/include/uapi/drm/drm.h that should be uintptr_t and this one __kernel_uintptr_t.

We shouldn't use uintptr_t in tools/include either. uintptr_t is 32-bit on a 32-bit arch, but this is always at least 64-bit. __kernel_uintptr_t can be used in those headers as well thanks to [1].

user data for vblank events was originally defined as a ulong - thus uintptr_t is actually correct here. it's called signal in public drm headers - but it is the user_data. vblank is special in drm because of this. :)

But struct drm_event_vblank::user_data is __u64 in include/uapi/drm/drm.h. Are you saying that userspace doesn't use that? In which case, was the uapi header broken all along and should it be fixed upstream?

it's not used. libdrm ships its own drm headers and thus i've patched those separately in libdrm itself. they have this as a ulong. :) it's always been this way... broken by design™ :)

Amazing... but then does it even make sense to bother patching any header in tools/include?

That's why I didn't notice the mix-up - as its an unused appendix thing. Nothing catches the "you forgot to change all the duplicates". I did it for completeness and correctness sake so it SHOULD be there. It's just easier to miss or get wrong.

On 4/12/24 3:55 PM, Kevin Brodsky wrote:

On 12/04/2024 16:46, Carsten Haitzler wrote:

...

On 4/12/24 2:57 PM, Kevin Brodsky wrote:

...

On 12/04/2024 14:49, Carsten Haitzler wrote:

...

On 4/12/24 12:40 PM, Kevin Brodsky wrote:

...

On 12/04/2024 13:20, Carsten Haitzler wrote:

...

[...] > >> [...] >> @@ -1277,7 +1277,7 @@ struct drm_event { >>>>        struct drm_event_vblank { >>>>          struct drm_event base; >>>> -    __u64 user_data; >>>> +    uintptr_t user_data; >>> >>> __kernel_uintptr_t surely? >>> >>> It is surprising that you didn't run into trouble here, because >>> the >>> kernel and userspace views will diverge in purecap (the former >>> will >>> see >>> user_data as 64-bit and the latter as a capability). >> >> hmmm how did i mess this up (and yet vblank tests work)... its the >> tools/include/uapi/drm/drm.h that should be uintptr_t and this one >> __kernel_uintptr_t. > > We shouldn't use uintptr_t in tools/include either. uintptr_t is > 32-bit > on a 32-bit arch, but this is always at least 64-bit. > __kernel_uintptr_t > can be used in those headers as well thanks to [1].

user data for vblank events was originally defined as a ulong - thus uintptr_t is actually correct here. it's called signal in public drm headers - but it is the user_data. vblank is special in drm because of this. :)

But struct drm_event_vblank::user_data is __u64 in include/uapi/drm/drm.h. Are you saying that userspace doesn't use that? In which case, was the uapi header broken all along and should it be fixed upstream?

it's not used. libdrm ships its own drm headers and thus i've patched those separately in libdrm itself. they have this as a ulong. :) it's always been this way... broken by design™ :)

Amazing... but then does it even make sense to bother patching any header in tools/include?

That's why I didn't notice the mix-up - as its an unused appendix thing. Nothing catches the "you forgot to change all the duplicates". I did it for completeness and correctness sake so it SHOULD be there. It's just easier to miss or get wrong.

I'd rather not touch tools/include/uapi/drm then. Changes there are completely untested and apparently these headers are unused anyway. We do not generally sync the headers in tools/include/uapi anyway (e.g. linux/bpf.h) since we don't need to.

well have a look over v3. i can strip those out if everything is ok.

On 4/9/24 3:58 PM, Kevin Brodsky wrote:

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

...

From: Carsten Haitzler carsten.haitzler@arm.com

Vblank is a bit different because it produces events that then can copy bpointers/capabilities back to userspace via a read on the DRM

s/bpointers/pointers/

yah - typo :)

...

device as opposed to the ioctl. The compat handler called the normal native/purecap one and this happened to work for compat but not for purecap. Make the shared "native" purecap handling work for both and share that infra properly to avoid copy and paste.

Signed-off-by: Carsten Haitzler Carsten.Haitzler@arm.com

drivers/gpu/drm/drm_atomic_uapi.c | 3 +- drivers/gpu/drm/drm_internal.h | 4 +++ drivers/gpu/drm/drm_ioc32.c | 8 +++++- drivers/gpu/drm/drm_vblank.c | 46 +++++++++++++++++++++++-------- include/drm/drm_vblank.h | 23 ++++++++++++++++ include/uapi/drm/drm.h | 6 ++-- 6 files changed, 74 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 3e4668a20157..08474a1ea90e 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -940,7 +940,8 @@ int drm_atomic_get_property(struct drm_mode_object *obj, */ static struct drm_pending_vblank_event *create_vblank_event(

• struct drm_crtc *crtc, uint64_t user_data)
  

• struct drm_crtc *crtc,
  

• __kernel_uintptr_t user_data)
  

We're not changing struct drm_mode_atomic in this patch so I suspect this belongs to the next one.

this belongs here (forgot to reply in previous mail). what i did mess up is the splitting of the patch - the changes to drm_mode_atomic should be in this patch

...

{ struct drm_pending_vblank_event *e = NULL; diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 8462b657c375..2d588df41cb5 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -23,6 +23,7 @@ #include <linux/kthread.h> +#include <drm/drm.h>

Doesn't seem to be needed.

...

#include <drm/drm_ioctl.h> #include <drm/drm_vblank.h> @@ -113,6 +114,9 @@ void drm_vblank_cancel_pending_works(struct drm_vblank_crtc *vblank); void drm_handle_vblank_works(struct drm_vblank_crtc *vblank); /* IOCTLS */ +int drm_wait_vblank_ioctl_internal(struct drm_device *dev, void *data,

• 		   struct drm_file *filp,
  

• 		   bool compat);
  

  int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, struct drm_file *filp); int drm_legacy_modeset_ctl_ioctl(struct drm_device *dev, void *data,

diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c index 504c2fcd9c89..d6a8c7250dda 100644 --- a/drivers/gpu/drm/drm_ioc32.c +++ b/drivers/gpu/drm/drm_ioc32.c @@ -33,6 +33,7 @@ #include <drm/drm_file.h> #include <drm/drm_print.h> +#include <drm/drm_drv.h> #include "drm_crtc_internal.h" #include "drm_internal.h" @@ -841,11 +842,16 @@ typedef union drm_wait_vblank32 { static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, user_uintptr_t arg) {

• struct drm_file *file_priv = file->private_data;
• struct drm_device *dev = file_priv->minor->dev; drm_wait_vblank32_t __user *argp = (void __user *)arg; drm_wait_vblank32_t req32; union drm_wait_vblank req; int err;

• if (drm_dev_is_unplugged(dev))

• return -ENODEV;
  

• if (copy_from_user(&req32, argp, sizeof(req32))) return -EFAULT;

@@ -854,7 +860,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd, req.request.type = req32.request.type; req.request.sequence = req32.request.sequence; req.request.signal = req32.request.signal;

• err = drm_ioctl_kernel(file, drm_wait_vblank_ioctl, &req, DRM_UNLOCKED);

• err = drm_wait_vblank_ioctl_internal(dev, &req, file_priv, true);

I'm not sure bypassing drm_ioctl_kernel() is 100% correct, as it does extra things like calling drm_ioctl_permit(). Couldn't we define our own callback in this file that would itself call drm_wait_vblank_ioctl_internal(..., true)?

...

req32.reply.type = req.reply.type; req32.reply.sequence = req.reply.sequence;

[...]

/**

• struct drm_pending_vblank_event - pending vblank event tracking

*/ @@ -52,6 +68,10 @@ struct drm_pending_vblank_event { * @sequence: frame event should be triggered at */ u64 sequence;

• /**

• * @compat: This is a compat32 event
  

Aren't we precisely using this for compat64 (arguably not so useful in compat32)?

...

• */
  

• bool compat; /**
  • @event: Actual event which will be sent to userspace.
  */

@@ -75,6 +95,9 @@ struct drm_pending_vblank_event { * @event.seq: Event payload for the MODE_QUEUEU_SEQUENCE IOCTL. */ struct drm_event_crtc_sequence seq;

• struct drm_event_vblank32 vbl32;
  

• struct drm_event_crtc_sequence32 seq32;
  

  } event; };

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index de723566c5ae..5347b76005ee 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -494,7 +494,7 @@ enum drm_vblank_seq_type { struct drm_wait_vblank_request { enum drm_vblank_seq_type type; unsigned int sequence;

• unsigned long signal;

• __kernel_uintptr_t signal;

Just checking I'm getting this right, this is used to hold some user_data, and as a result needs to be expanded too, right?

...

}; struct drm_wait_vblank_reply { @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */

• __u64 user_data; /* user data passed to event */

• __u64 user_data; /* user data passed to event */

The actual type change (to __kernel_uintptr_t) is in patch 3, it should be in this patch instead.

...

}; #if defined(__cplusplus) @@ -1277,7 +1277,7 @@ struct drm_event { struct drm_event_vblank { struct drm_event base;

• __u64 user_data;

• uintptr_t user_data;

__kernel_uintptr_t surely?

It is surprising that you didn't run into trouble here, because the kernel and userspace views will diverge in purecap (the former will see user_data as 64-bit and the latter as a capability).

Kevin

...

__u32 tv_sec; __u32 tv_usec; __u32 sequence;

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

[...]

@@ -2919,16 +2962,9 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied++; } }

• out_resp->count_encoders = encoders_count;
• out_resp->connector_id = connector->base.id;
• out_resp->connector_type = connector->connector_type;
• out_resp->connector_type_id = connector->connector_type_id;
• is_current_master = drm_is_current_master(file_priv);

Nit: probably want to keep the empty lines around that line.

mutex_lock(&dev->mode_config.mutex);

• if (out_resp->count_modes == 0) {

• if (count_modes == 0) { if (is_current_master) connector->funcs->fill_modes(connector, dev->mode_config.max_width,

@@ -2938,11 +2974,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, connector->base.id, connector->name); }

• out_resp->mm_width = connector->display_info.width_mm;
• out_resp->mm_height = connector->display_info.height_mm;
• out_resp->subpixel = connector->display_info.subpixel_order;
• out_resp->connection = connector->status;
• /* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) { WARN_ON(mode->expose_to_userspace);

@@ -2958,9 +2989,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */

• if ((out_resp->count_modes >= mode_count) && mode_count) {

• if ((count_modes >= mode_count) && mode_count) { copied = 0;

• mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr);
  

  list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue;

@@ -2998,25 +3028,50 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, mode->expose_to_userspace = false; }

• out_resp->count_modes = mode_count; mutex_unlock(&dev->mode_config.mutex);

drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector);

• if (encoder)

• out_resp->encoder_id = encoder->base.id;
  

• else

• out_resp->encoder_id = 0;
  

/* Only grab properties after probing, to make sure EDID and other * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,

• 	uaddr_to_user_ptr(out_resp->props_ptr),
  

• 	uaddr_to_user_ptr(out_resp->prop_values_ptr),
  

• 	&out_resp->count_props);
  

• 	prop_ptr, prop_values,
  

• 	&count_props);
  

I think this will result in UB, because drm_mode_object_get_properties() reads from arg_count_props (last argument) *before* writing to it, and count_props is uninitialised at this point. Maybe we should initialise it like the other properties at the beginning of this function.

drm_modeset_unlock(&dev->mode_config.connection_mutex);

• if (drm_test_compat64()) {

• out_resp32->count_encoders = encoders_count;
  

• out_resp32->count_modes = mode_count;
  

• out_resp32->count_props = count_props;
  

• out_resp32->connector_id = connector->base.id;
  

• out_resp32->connector_type = connector->connector_type;
  

• out_resp32->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp32->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp32->encoder_id = 0;
  

• out_resp32->mm_width = connector->display_info.width_mm;
  

• out_resp32->mm_height = connector->display_info.height_mm;
  

• out_resp32->subpixel = connector->display_info.subpixel_order;
  

• out_resp32->connection = connector->status;
  

• } else {

• out_resp->count_encoders = encoders_count;
  

• out_resp->count_modes = mode_count;
  

• out_resp->count_props = count_props;
  

• out_resp->connector_id = connector->base.id;
  

• out_resp->connector_type = connector->connector_type;
  

• out_resp->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp->encoder_id = 0;
  

• out_resp->mm_width = connector->display_info.width_mm;
  

• out_resp->mm_height = connector->display_info.height_mm;
  

• out_resp->subpixel = connector->display_info.subpixel_order;
  

• out_resp->connection = connector->status;
  

• }

out: drm_connector_put(connector); diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 8525ef851540..eab94e74b56a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data;

• struct drm_mode_card_res32 {

• __u64 fb_id_ptr;
  

• __u64 crtc_id_ptr;
  

• __u64 connector_id_ptr;
  

• __u64 encoder_id_ptr;
  

• __u32 count_fbs;
  

• __u32 count_crtcs;
  

• __u32 count_connectors;
  

• __u32 count_encoders;
  

• __u32 min_width;
  

• __u32 max_width;
  

• __u32 min_height;
  

• __u32 max_height;
  

• };
• struct drm_mode_card_res32 *card_res32 = data; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder;

• int count, ret = 0;

• int count_fb = 0, count_fbs;
• int count_crtc = 0, count_crtcs;
• int count_encoder = 0, count_encoders;
• int count_connector = 0, count_connectors;
• int ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id;

@@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data, return -EOPNOTSUPP; mutex_lock(&file_priv->fbs_lock);

• count = 0;
• fb_id = u64_to_user_ptr(card_res->fb_id_ptr);

• if (drm_test_compat64()) {

• fb_id        = compat_ptr(card_res32->fb_id_ptr);
  

• crtc_id      = compat_ptr(card_res32->crtc_id_ptr);
  

• encoder_id   = compat_ptr(card_res32->encoder_id_ptr);
  

• connector_id = compat_ptr(card_res32->connector_id_ptr);
  

• count_fbs        = card_res32->count_fbs;
  

• count_crtcs      = card_res32->count_crtcs;
  

• count_encoders   = card_res32->count_encoders;
  

• count_connectors = card_res32->count_connectors;
  

• } else {

• fb_id        = (uint32_t __user *)(card_res->fb_id_ptr);
  

• crtc_id      = (uint32_t __user *)(card_res->crtc_id_ptr);
  

• encoder_id   = (uint32_t __user *)(card_res->encoder_id_ptr);
  

• connector_id = (uint32_t __user *)(card_res->connector_id_ptr);
  

• count_fbs        = card_res->count_fbs;
  

• count_crtcs      = card_res->count_crtcs;
  

• count_encoders   = card_res->count_encoders;
  

• count_connectors = card_res->count_connectors;
  

• } list_for_each_entry(fb, &file_priv->fbs, filp_head) {

• if (count < card_res->count_fbs &&
  

•     put_user(fb->base.id, fb_id + count)) {
  

• 	mutex_unlock(&file_priv->fbs_lock);
  

• 	return -EFAULT;
  

• if (count_fb < count_fbs &&
  

•     put_user(fb->base.id, fb_id + count_fb)) {
  

• 	ret = -EFAULT;
  

• 	goto error;
  

  }

• count++;
  

• count_fb++;
  

  }

• card_res->count_fbs = count;
• mutex_unlock(&file_priv->fbs_lock);

I can't quite convince myself that postponing the mutex_unlock() until the end of the function is a good idea or even correct. Either way it seems completely unrelated to the rest of the patch.

[...]

int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_plane32 {

• __u32 plane_id;
  

• __u32 crtc_id;
  

• __u32 fb_id;
  

• __u32 possible_crtcs;
  

• __u32 gamma_size;
  

• __u32 count_format_types;
  

• __u64 format_type_ptr;
  

• };
• struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr;
• __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• plane = drm_plane_find(dev, file_priv, plane_resp->plane_id);

• if (drm_test_compat64()) {

• format_ptr = uaddr_to_user_ptr(plane_resp32->format_type_ptr);
  

• plane_id = plane_resp32->plane_id;
  

• count_format_types = plane_resp32->count_format_types;
  

• } else {

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

The pointer conversions need to be updated here (compat_ptr / simple cast).

• plane_id = plane_resp->plane_id;
  

• count_format_types = plane_resp->count_format_types;
  

• }
• plane = drm_plane_find(dev, file_priv, plane_id); if (!plane) return -ENOENT;

drm_modeset_lock(&plane->mutex, NULL); if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id))

• plane_resp->crtc_id = plane->state->crtc->base.id;
  

• crtc_id = plane->state->crtc->base.id;
  

  else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id))

• plane_resp->crtc_id = plane->crtc->base.id;
  

• crtc_id = plane->crtc->base.id;
  

  else

• plane_resp->crtc_id = 0;
  

• crtc_id = 0;
  

if (plane->state && plane->state->fb)

• plane_resp->fb_id = plane->state->fb->base.id;
  

• fb_id = plane->state->fb->base.id;
  

  else if (!plane->state && plane->fb)

• plane_resp->fb_id = plane->fb->base.id;
  

• fb_id = plane->fb->base.id;
  

  else

• plane_resp->fb_id = 0;
  

• fb_id = 0;
  

  drm_modeset_unlock(&plane->mutex);

• plane_resp->plane_id = plane->base.id;
• plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 					    plane->possible_crtcs);
  

• plane_id = plane->base.id;
• possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 				plane->possible_crtcs);
  

• plane_resp->gamma_size = 0;

• gamma_size = 0;

/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ if (plane->format_count &&

•    (plane_resp->count_format_types >= plane->format_count)) {
  

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

•    (count_format_types >= plane->format_count)) {
  

  if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) {

@@ -741,6 +781,22 @@ int drm_mode_getplane(struct drm_device *dev, void *data, } plane_resp->count_format_types = plane->format_count;

Should be deleted (it's now in the if/else below).

• if (drm_test_compat64()) {

• plane_resp32->crtc_id = crtc_id;
  

• plane_resp32->fb_id = fb_id;
  

• plane_resp32->plane_id = plane_id;
  

• plane_resp32->possible_crtcs = possible_crtcs;
  

• plane_resp32->gamma_size = gamma_size;
  

• plane_resp32->count_format_types = count_format_types;
  

• } else {

• plane_resp->crtc_id = crtc_id;
  

• plane_resp->fb_id = fb_id;
  

• plane_resp->plane_id = plane_id;
  

• plane_resp->possible_crtcs = possible_crtcs;
  

• plane_resp->gamma_size = gamma_size;
  

• plane_resp->count_format_types = count_format_types;
  

• }
• return 0;

} diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..6ae86d4510a8 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -457,6 +457,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_property32 {

• __u64 values_ptr;
  

• __u64 enum_blob_ptr;
  

• __u32 prop_id;
  

• __u32 flags;
  

• char name[DRM_PROP_NAME_LEN];
  

• __u32 count_values;
  

• __u32 count_enum_blobs;
  

• };
• struct drm_mode_get_property32 *out_resp32 = data; struct drm_mode_get_property *out_resp = data; struct drm_property *property; int enum_count = 0;

@@ -465,22 +475,39 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, struct drm_property_enum *prop_enum; struct drm_mode_property_enum __user *enum_ptr; uint64_t __user *values_ptr;

• __u32 prop_id, flags, count_values, count_enum_blobs;
• char *name;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• property = drm_property_find(dev, file_priv, out_resp->prop_id);

• if (drm_test_compat64()) {

• values_ptr = compat_ptr(out_resp32->values_ptr);
  

• enum_ptr = compat_ptr(out_resp32->enum_blob_ptr);
  

• prop_id = out_resp32->prop_id;
  

• name = out_resp32->name;
  

• count_values = out_resp32->count_values;
  

• count_enum_blobs = out_resp32->count_enum_blobs;
  

• } else {

• values_ptr = (uint64_t __user *)out_resp->values_ptr;
  

• enum_ptr = (struct drm_mode_property_enum __user *)out_resp->enum_blob_ptr;
  

• prop_id = out_resp->prop_id;
  

• name = out_resp->name;
  

• count_values = out_resp->count_values;
  

• count_enum_blobs = out_resp->count_enum_blobs;
  

• }
• property = drm_property_find(dev, file_priv, prop_id); if (!property) return -ENOENT;

• strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN);
• out_resp->flags = property->flags;

• strscpy_pad(name, property->name, DRM_PROP_NAME_LEN);
• flags = property->flags;

value_count = property->num_values;

• values_ptr = u64_to_user_ptr(out_resp->values_ptr);

for (i = 0; i < value_count; i++) {

• if (i < out_resp->count_values &&
  

• if (i < count_values &&
  

  put_user(property->values[i], values_ptr + i)) { return -EFAULT; }

@@ -488,13 +515,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, out_resp->count_values = value_count;

Should be deleted too.

copied = 0;

• enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);

if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { list_for_each_entry(prop_enum, &property->enum_list, head) { enum_count++;

• 	if (out_resp->count_enum_blobs < enum_count)
  

• 	if (count_enum_blobs < enum_count)
  	continue;
  

if (copy_to_user(&enum_ptr[copied].value, @@ -506,7 +532,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -EFAULT; copied++; }

• out_resp->count_enum_blobs = enum_count;
  

• count_enum_blobs = enum_count;
  

  }

/* @@ -518,7 +544,18 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, * the property itself. */ if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))

• out_resp->count_enum_blobs = 0;
  

• count_enum_blobs = 0;
  

Nit: empty line here.

• if (drm_test_compat64()) {

• out_resp32->prop_id = prop_id;
  

• out_resp32->flags = flags;
  

• out_resp32->count_values = count_values;
  

• out_resp32->count_enum_blobs = count_enum_blobs;
  

• } else {

• out_resp->prop_id = prop_id;
  

• out_resp->flags = flags;
  

• out_resp->count_values = count_values;
  

• out_resp->count_enum_blobs = count_enum_blobs;
  

• }

return 0; } @@ -754,29 +791,53 @@ EXPORT_SYMBOL(drm_property_replace_blob); int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_blob32 {

• __u32 blob_id;
  

• __u32 length;
  

• __u64 data;
  

• };
• struct drm_mode_get_blob32 *out_resp32 = data; struct drm_mode_get_blob *out_resp = data; struct drm_property_blob *blob; int ret = 0;
• __u32 blob_id;
• __u32 length;
• uint8_t __user *blob_data;
• if (drm_test_compat64()) {

• blob_id = out_resp32->blob_id;
  

• length = out_resp32->length;
  

• blob_data = compat_ptr(out_resp32->data);
  

• } else {

• blob_id = out_resp->blob_id;
  

• length = out_resp->length;
  

• blob_data = (uint8_t __user *)(out_resp->data);
  

• }

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• blob = drm_property_lookup_blob(dev, out_resp->blob_id);

• blob = drm_property_lookup_blob(dev, blob_id); if (!blob) return -ENOENT;

• if (out_resp->length == blob->length) {

• if (copy_to_user(u64_to_user_ptr(out_resp->data),
  

• if (length == blob->length) {

• if (copy_to_user(blob_data,
  	 blob->data,
  	 blob->length)) {
  ret = -EFAULT;
  goto unref;
  

  } }

• out_resp->length = blob->length;

• length = blob->length;

unref: drm_property_blob_put(blob);

• if (drm_test_compat64()) {

• out_resp32->length = length;
  

• } else {

• out_resp->length = length;
  

• } return ret;

} diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index e2640dc64e08..05e048c16c10 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -34,6 +34,15 @@ #include <drm/drm_device.h> +static inline bool drm_test_compat64(void)

We now have in_compat64_syscall() that does exactly the same thing (but isn't arm64-specific).

+{ +#ifdef CONFIG_COMPAT

• return test_thread_flag(TIF_64BIT_COMPAT);

+#else

• return false;

+#endif +}

[...] struct drm_mode_fb_cmd { @@ -1029,7 +1029,7 @@ struct drm_mode_crtc_page_flip_target { __u32 fb_id; __u32 flags; __u32 sequence;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Aren't we missing the corresponding compat handling for this change?

}; /** @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Ditto here.

Kevin

}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..7a0524d7d9f5 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */

• __u64 user_data; /* user data passed to event */

• uintptr_t user_data; /* user data passed to event */

}; #if defined(__cplusplus) @@ -1289,7 +1289,7 @@ struct drm_event_vblank { */ struct drm_event_crtc_sequence { struct drm_event base;

• __u64 user_data;

• uintptr_t user_data; __s64 time_ns; __u64 sequence;

};

On 11/04/2024 14:18, Carsten Haitzler wrote:

[...]

...

...

@@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data,                 struct drm_file *file_priv)   {       struct drm_mode_card_res *card_res = data; +    struct drm_mode_card_res32 { +        __u64 fb_id_ptr; +        __u64 crtc_id_ptr; +        __u64 connector_id_ptr; +        __u64 encoder_id_ptr; +        __u32 count_fbs; +        __u32 count_crtcs; +        __u32 count_connectors; +        __u32 count_encoders; +        __u32 min_width; +        __u32 max_width; +        __u32 min_height; +        __u32 max_height; +    }; +    struct drm_mode_card_res32 *card_res32 = data;       struct drm_framebuffer *fb;       struct drm_connector *connector;       struct drm_crtc *crtc;       struct drm_encoder *encoder; -    int count, ret = 0; +    int count_fb = 0, count_fbs; +    int count_crtc = 0, count_crtcs; +    int count_encoder = 0, count_encoders; +    int count_connector = 0, count_connectors; +    int ret = 0;       uint32_t __user *fb_id;       uint32_t __user *crtc_id;       uint32_t __user *connector_id; @@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data,           return -EOPNOTSUPP;         mutex_lock(&file_priv->fbs_lock); -    count = 0; -    fb_id = u64_to_user_ptr(card_res->fb_id_ptr); +    if (drm_test_compat64()) { +        fb_id        = compat_ptr(card_res32->fb_id_ptr); +        crtc_id      = compat_ptr(card_res32->crtc_id_ptr); +        encoder_id   = compat_ptr(card_res32->encoder_id_ptr); +        connector_id = compat_ptr(card_res32->connector_id_ptr);

+        count_fbs        = card_res32->count_fbs; +        count_crtcs      = card_res32->count_crtcs; +        count_encoders   = card_res32->count_encoders; +        count_connectors = card_res32->count_connectors; +    } else { +        fb_id        = (uint32_t __user *)(card_res->fb_id_ptr); +        crtc_id      = (uint32_t __user *)(card_res->crtc_id_ptr); +        encoder_id   = (uint32_t __user *)(card_res->encoder_id_ptr); +        connector_id = (uint32_t __user *)(card_res->connector_id_ptr);

+        count_fbs        = card_res->count_fbs; +        count_crtcs      = card_res->count_crtcs; +        count_encoders   = card_res->count_encoders; +        count_connectors = card_res->count_connectors; +    }       list_for_each_entry(fb, &file_priv->fbs, filp_head) { -        if (count < card_res->count_fbs && -            put_user(fb->base.id, fb_id + count)) { -            mutex_unlock(&file_priv->fbs_lock); -            return -EFAULT; +        if (count_fb < count_fbs && +            put_user(fb->base.id, fb_id + count_fb)) { +            ret = -EFAULT; +            goto error;           } -        count++; +        count_fb++;       } -    card_res->count_fbs = count; -    mutex_unlock(&file_priv->fbs_lock);

I can't quite convince myself that postponing the mutex_unlock() until the end of the function is a good idea or even correct. Either way it seems completely unrelated to the rest of the patch.

as the return is only at the end now, error handling is done with the goto wich then immediately unlocks mutex and returns in one spot instead of N spots. as i needed an encode+decode pass basically i kept the code consistent and easy to manage this way - the mutex is held a bit longer but the way i see it... what kind of real contention do you ever get with drm apps doing this... at the same time? :) you pretty much don't. you have some process that owns the display now. X. a wl compositor etc. and it's going to query the device - it and only it. no one else is because ... you only really have one vt owning process at a time doing this. it's not going to do this from multiple threads. it'll have some serial logic and then make a decision and be done. :)

so it's  less hairy code with less to go wrong when done this way with a mutex held a bit longer for ... no real life loss. :) that's my take.

Still not sure this has anything to do with adding conversions, but I suppose it doesn't hurt either. Interestingly the function already has this ret variable and ends with return ret; despite never setting it... I guess it was asking to be used!

[...]

...

...

@@ -1029,7 +1029,7 @@ struct drm_mode_crtc_page_flip_target {       __u32 fb_id;       __u32 flags;       __u32 sequence; -    __u64 user_data; +    __kernel_uintptr_t user_data;

Aren't we missing the corresponding compat handling for this change?

oh yeah - this may have been me flagging "stuff to do" that i neeed more stack up to go drive it and i switcvhed over to getting userspace up more... so it was the first steps but not the rest as i had no test case yet.

...

...

};     /** @@ -1135,7 +1135,7 @@ struct drm_mode_atomic {       __u64 props_ptr;       __u64 prop_values_ptr;       __u64 reserved; -    __u64 user_data; +    __kernel_uintptr_t user_data;

Ditto here.

same as above. will remove as this is a "need more of userspace" problem.

Makes sense. Better leave the structs we don't handle unchanged :)

Kevin