Hi everyone,
This patch aims to enable purecap applications to make use of the MMC block driver by adding PCuABI support to the MMC_IOC_CMD and MMC_IOC_MULTI_CMD ioctls. This also includes compat64 support for the ioctls.
V4 -> V5: Improvement to readability and fix to user-space macro.
V3 -> V4: Minor corrections and improvements to readability of code. - Remove unnecessary explicit checks on capabilities. - Revert modification to struct mmc_ioc_multi_cmd. - Remove unnecessary parentheses.
V2 -> V3: - Implement support for capabilities in the ioctls. - Correct formatting errors in patches. - Correct formatting and syntax errors in code.
V1 -> V2: Various improvements to the code including: - Preference for native structs over (void __user *) - Complying with code styling guidelines - Improvement in code readability via removing unnecessary casts
GitLab Issue: https://git.morello-project.org/morello/kernel/linux/-/issues/51
Review branch: https://git.morello-project.org/arkamnite/linux/-/commits/morello%2Fmmc_v5
Many thanks, Akram
Akram Ahmad (2): mmc: Implement compat handling for struct mmc_ioc_{multi_}cmd mmc: Support capabilities in MMC_IOC_{MULTI_}CMD ioctls
drivers/mmc/core/block.c | 100 ++++++++++++++++++++++++++++++--- include/uapi/linux/mmc/ioctl.h | 7 ++- 2 files changed, 97 insertions(+), 10 deletions(-)
Introduce a compat version of the struct mmc_ioc_cmd and struct mmc_ioc_multi_cmd. Also implement helper functions which convert between the native and compat versions of these two structs.
A subsequent patch will change the structs to enable it to support new architectures. On such architectures, the current struct layout must still be supported for compat tasks.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 96 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 2f8884e3d4c2..3333c4917028 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -101,6 +101,33 @@ struct mmc_blk_busy_data { u32 status; };
+static inline bool in_compat64(void) +{ + return IS_ENABLED(CONFIG_COMPAT64) && in_compat_syscall(); +} + +struct compat_mmc_ioc_cmd { + int write_flag; + int is_acmd; + __u32 opcode; + __u32 arg; + __u32 response[4]; + unsigned int flags; + unsigned int blksz; + unsigned int blocks; + unsigned int postsleep_min_us; + unsigned int postsleep_max_us; + unsigned int data_timeout_ns; + unsigned int cmd_timeout_ms; + __u32 __pad; + __u64 data_ptr; +}; + +struct compat_mmc_ioc_multi_cmd { + __u64 num_of_cmds; + struct compat_mmc_ioc_cmd cmds[]; +}; + /* * There is one mmc_blk_data per slot. */ @@ -401,6 +428,47 @@ struct mmc_blk_ioc_data { struct mmc_rpmb_data *rpmb; };
+/* + * Copy the data from a compat_mmc_ioc_cmd user pointer, src, + * to kernel space, storing it in native_cmd. Returns 0 for + * a successful copy. + */ +static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, + void * __user src) +{ + struct compat_mmc_ioc_cmd compat_cmd; + + if (copy_from_user(&compat_cmd, src, sizeof(struct compat_mmc_ioc_cmd))) + return -EFAULT; + + native_cmd->arg = compat_cmd.arg; + native_cmd->is_acmd = compat_cmd.is_acmd; + native_cmd->opcode = compat_cmd.opcode; + native_cmd->arg = compat_cmd.arg; + memcpy(native_cmd->response, compat_cmd.response, sizeof(compat_cmd.response)); + native_cmd->flags = compat_cmd.flags; + native_cmd->blksz = compat_cmd.blksz; + native_cmd->blocks = compat_cmd.blocks; + native_cmd->postsleep_min_us = compat_cmd.postsleep_min_us; + native_cmd->postsleep_max_us = compat_cmd.postsleep_max_us; + native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; + native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; + native_cmd->__pad = compat_cmd.__pad; + native_cmd->data_ptr = compat_cmd.data_ptr; + + return 0; +} + +static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) +{ + if (in_compat64()) + return get_mmc_ioc_cmd_from_compat64(to, src); + + if (copy_from_user(to, src, sizeof(*to))) + return -EFAULT; + return 0; +} + static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( struct mmc_ioc_cmd __user *user) { @@ -413,7 +481,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( goto out; }
- if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) { + if (copy_mmc_ioc_cmd_from_user(&idata->ic, user)) { err = -EFAULT; goto idata_err; } @@ -449,8 +517,11 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, { struct mmc_ioc_cmd *ic = &idata->ic;
- if (copy_to_user(&(ic_ptr->response), ic->response, - sizeof(ic->response))) + __u32 __user *response_uptr = in_compat64() ? + &((struct compat_mmc_ioc_cmd __user *)ic_ptr)->response[0] : + &ic_ptr->response[0]; + + if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;
if (!idata->ic.write_flag) { @@ -666,12 +737,20 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md, return ioc_err ? ioc_err : err; }
+static inline struct mmc_ioc_cmd __user *get_ith_mmc_ioc_cmd_uptr( + struct mmc_ioc_multi_cmd __user *user, + unsigned int i) +{ + if (in_compat64()) + return (struct mmc_ioc_cmd __user *)&((struct compat_mmc_ioc_multi_cmd __user *)user)->cmds[i]; + return &(user->cmds[i]); +} + static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, struct mmc_ioc_multi_cmd __user *user, struct mmc_rpmb_data *rpmb) { struct mmc_blk_ioc_data **idata = NULL; - struct mmc_ioc_cmd __user *cmds = user->cmds; struct mmc_card *card; struct mmc_queue *mq; int err = 0, ioc_err = 0; @@ -679,6 +758,11 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, unsigned int i, n; struct request *req;
+ /* + * Both native and compat64 versions of mmc_ioc_multi_cmd + * have num_of_cmds as the first field, so the offset does + * not need to be recalculated for compat64. + */ if (copy_from_user(&num_of_cmds, &user->num_of_cmds, sizeof(num_of_cmds))) return -EFAULT; @@ -695,7 +779,7 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md, return -ENOMEM;
for (i = 0; i < n; i++) { - idata[i] = mmc_blk_ioctl_copy_from_user(&cmds[i]); + idata[i] = mmc_blk_ioctl_copy_from_user(get_ith_mmc_ioc_cmd_uptr(user, i)); if (IS_ERR(idata[i])) { err = PTR_ERR(idata[i]); n = i; @@ -732,7 +816,7 @@ static int mmc_blk_ioctl_multi_cmd(struct mmc_blk_data *md,
/* copy to user if data and response */ for (i = 0; i < n && !err; i++) - err = mmc_blk_ioctl_copy_to_user(&cmds[i], idata[i]); + err = mmc_blk_ioctl_copy_to_user(get_ith_mmc_ioc_cmd_uptr(user, i), idata[i]);
blk_mq_free_request(req);
@@ -449,8 +517,11 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, { struct mmc_ioc_cmd *ic = &idata->ic;
- if (copy_to_user(&(ic_ptr->response), ic->response,
sizeof(ic->response)))
- __u32 __user *response_uptr = in_compat64() ?
&((struct compat_mmc_ioc_cmd __user *)ic_ptr)->response[0] :&ic_ptr->response[0];
Hi Akram,
Great job with the series! Looks wonderful! 👏 I have a short question: Why do you have &ic_ptr->response[0] here? That should be equivalent to ic_ptr->response, unless I'm missing something. ic_ptr->response looks correct here, but this is different from what it was before &(ic_ptr->response), so I'm sligthly confused. Is this a bug in the upstream kernel...?
Best, Tudor
- if (copy_to_user(response_uptr, ic->response, sizeof(ic->response))) return -EFAULT;
if (!idata->ic.write_flag) {
On 04/11/2023 00:21, Tudor Cretu wrote:
Why do you have &ic_ptr->response[0] here? That should be equivalent to ic_ptr->response, unless I'm missing something. ic_ptr->response looks correct here, but this is different from what it was before &(ic_ptr->response), so I'm sligthly confused. Is this a bug in the upstream kernel...?
Ah, that is a good point, I hadn't realised what the original code was going. We talked about this offline with Akram, clearly what we want here is a standard pointer to the buffer, which can be obtained in two ways: 1. Array to pointer decay: ic_ptr->response 2. Taking the address of the first member: &ic_ptr->response[0]
Both approaches are fairly common, but I agree it would make sense to prefer the first one, to match the source pointer (ic->response).
The existing code does seem to be confused. &ic_ptr->response yields a pointer to array, of type __u32 (*)[4]. This goes unnoticed because copy_to_user() takes a void *, so any pointer type is acceptable. This is certainly unnecessary complication though (and arithmetic on that pointer would not work as intended). It wouldn't hurt to fix that upstream, though it doesn't seem to be essential either.
Kevin
On 06/11/2023 16:28, Kevin Brodsky wrote:
On 04/11/2023 00:21, Tudor Cretu wrote:
Why do you have &ic_ptr->response[0] here? That should be equivalent to ic_ptr->response, unless I'm missing something. ic_ptr->response looks correct here, but this is different from what it was before &(ic_ptr->response), so I'm sligthly confused. Is this a bug in the upstream kernel...?
Ah, that is a good point, I hadn't realised what the original code was going. We talked about this offline with Akram, clearly what we want here is a standard pointer to the buffer, which can be obtained in two ways:
- Array to pointer decay: ic_ptr->response
- Taking the address of the first member: &ic_ptr->response[0]
Both approaches are fairly common, but I agree it would make sense to prefer the first one, to match the source pointer (ic->response).
The existing code does seem to be confused. &ic_ptr->response yields a pointer to array, of type __u32 (*)[4]. This goes unnoticed because copy_to_user() takes a void *, so any pointer type is acceptable. This is certainly unnecessary complication though (and arithmetic on that pointer would not work as intended). It wouldn't hurt to fix that upstream, though it doesn't seem to be essential either.
Kevin _______________________________________________ linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
Thanks for the feedback Tudor! Just to confirm in that case, shall I correct this in V6? I.e. use the array to pointer decay rather than the address of the first member.
Many thanks,
Akram
Akram Ahmad wrote:
On 06/11/2023 16:28, Kevin Brodsky wrote:
On 04/11/2023 00:21, Tudor Cretu wrote: Why do you have &ic_ptr->response[0] here? That should be equivalent to ic_ptr->response, unless I'm missing something. ic_ptr->response looks correct here, but this is different from what it was before &(ic_ptr->response), so I'm sligthly confused. Is this a bug in the upstream kernel...? Ah, that is a good point, I hadn't realised what the original code was going. We talked about this offline with Akram, clearly what we want here is a standard pointer to the buffer, which can be obtained in two ways:
Array to pointer decay: ic_ptr->response Taking the address of the first member: &ic_ptr->response[0]
Both approaches are fairly common, but I agree it would make sense to prefer the first one, to match the source pointer (ic->response). The existing code does seem to be confused. &ic_ptr->response yields a pointer to array, of type __u32 (*)[4]. This goes unnoticed because copy_to_user() takes a void *, so any pointer type is acceptable. This is certainly unnecessary complication though (and arithmetic on that pointer would not work as intended). It wouldn't hurt to fix that upstream, though it doesn't seem to be essential either. Kevin _______________________________________________ linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org Thanks for the feedback Tudor! Just to confirm in that case, shall I
correct this in V6? I.e. use the array to pointer decay rather than the address of the first member.
As Kevin said, both approaches are fairly common. Up to you which one you prefer. I have no preference myself.
Best, Tudor
Many thanks, Akram
The mmc_ioc_cmd and mmc_ioc_multi_cmd structs are used to hold information and data about an MMC ioctl.
The PCuABI uses 129-bit capabilities as user pointers, which means that the __u64 type must be replaced with the __kernel_uintptr_t type, which is large enough to hold capabilities, yet will remain 64-bit on other architectures. Additional modifications have been made to the copy routines.
Signed-off-by: Akram Ahmad Akram.Ahmad@arm.com --- drivers/mmc/core/block.c | 8 ++++---- include/uapi/linux/mmc/ioctl.h | 7 +++++-- 2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 3333c4917028..547941096c0e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -454,7 +454,7 @@ static int get_mmc_ioc_cmd_from_compat64(struct mmc_ioc_cmd *native_cmd, native_cmd->data_timeout_ns = compat_cmd.data_timeout_ns; native_cmd->cmd_timeout_ms = compat_cmd.cmd_timeout_ms; native_cmd->__pad = compat_cmd.__pad; - native_cmd->data_ptr = compat_cmd.data_ptr; + native_cmd->data_ptr = (__kernel_uintptr_t)compat_ptr(compat_cmd.data_ptr);
return 0; } @@ -464,7 +464,7 @@ static int copy_mmc_ioc_cmd_from_user(struct mmc_ioc_cmd *to, void * __user src) if (in_compat64()) return get_mmc_ioc_cmd_from_compat64(to, src);
- if (copy_from_user(to, src, sizeof(*to))) + if (copy_from_user_with_ptr(to, src, sizeof(*to))) return -EFAULT; return 0; } @@ -497,7 +497,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user( return idata; }
- idata->buf = memdup_user(uaddr_to_user_ptr(idata->ic.data_ptr), + idata->buf = memdup_user((void __user *)idata->ic.data_ptr, idata->buf_bytes); if (IS_ERR(idata->buf)) { err = PTR_ERR(idata->buf); @@ -525,7 +525,7 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr, return -EFAULT;
if (!idata->ic.write_flag) { - if (copy_to_user(uaddr_to_user_ptr(ic->data_ptr), + if (copy_to_user((void __user *)ic->data_ptr, idata->buf, idata->buf_bytes)) return -EFAULT; } diff --git a/include/uapi/linux/mmc/ioctl.h b/include/uapi/linux/mmc/ioctl.h index e7401ade6822..75caa3dac4f6 100644 --- a/include/uapi/linux/mmc/ioctl.h +++ b/include/uapi/linux/mmc/ioctl.h @@ -2,6 +2,9 @@ #ifndef LINUX_MMC_IOCTL_H #define LINUX_MMC_IOCTL_H
+#ifndef __KERNEL__ +#include <stdint.h> +#endif #include <linux/types.h> #include <linux/major.h>
@@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad;
/* DAT buffer */ - __u64 data_ptr; + __kernel_uintptr_t data_ptr; }; -#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t)(unsigned long) ptr
/** * struct mmc_ioc_multi_cmd - multi command information
On 31/10/2023 17:20, Akram Ahmad wrote:
-#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t)(unsigned long) ptr
The problem remains the same in purecap if we do this: unsigned long will truncate the pointer. The point of using uintptr_t is that it is the same size as pointers, so there is no need for the intermediate cast - we can cast to uintptr_t directly. In 32-bit, there will then be an implicit conversion from 32-bit integer to 64-bit integer, but that's not a problem.
I've got no other comment so I can make this change directly before merging, if that's fine with you.
Kevin
On 01/11/2023 11:05, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
-#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr +#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (uintptr_t)(unsigned long) ptr
The problem remains the same in purecap if we do this: unsigned long will truncate the pointer. The point of using uintptr_t is that it is the same size as pointers, so there is no need for the intermediate cast
- we can cast to uintptr_t directly. In 32-bit, there will then be an
implicit conversion from 32-bit integer to 64-bit integer, but that's not a problem.
I've got no other comment so I can make this change directly before merging, if that's fine with you.
Kevin
That would make sense, I was a little unsure whether the implicit cast would be a problem or not. I'd also be very grateful if you could make the change directly please, that's fine by me.
Many thanks,
Akram
On 31/10/2023 17:20, Akram Ahmad wrote:
@@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad; /* DAT buffer */
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
So... we have a bit of a problem here. It seems to be the first time that we hit it, probably because we haven't converted many (any?) ioctls yet. The ioctl command corresponding to this struct is defined thus:
#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
What this means is that the size of the struct is part of the command numerical value (see the definition of _IOWR and _IOC). Unavoidably, if we modify the struct as we do here, its size increases and the command value changes.
This is not really a problem in purecap, as applications need to be rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working as-is (without rebuilding). Sure enough, mmc-utils built against unmodified headers fails with this patch, as the value of MMC_IOC_CMD it uses is not what the kernel expects any more.
Unfortunately I don't see a way to solve this problem without hardcoding the size in the command definition (something like _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very welcome however!
Kevin
On 01/11/2023 15:28, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
@@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad; /* DAT buffer */
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
So... we have a bit of a problem here. It seems to be the first time that we hit it, probably because we haven't converted many (any?) ioctls yet. The ioctl command corresponding to this struct is defined thus:
#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
What this means is that the size of the struct is part of the command numerical value (see the definition of _IOWR and _IOC). Unavoidably, if we modify the struct as we do here, its size increases and the command value changes.
This is not really a problem in purecap, as applications need to be rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working as-is (without rebuilding). Sure enough, mmc-utils built against unmodified headers fails with this patch, as the value of MMC_IOC_CMD it uses is not what the kernel expects any more.
Unfortunately I don't see a way to solve this problem without hardcoding the size in the command definition (something like _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very welcome however!
Kevin
I'm not 100% confident on this, but in drivers/mmc/core/block.c, we have the mmc_blk_ioctl function which currently contains the following:
switch (cmd) { case MMC_IOC_CMD: // ... case MMC_IOC_MULTI_CMD: // ... default: return -EINVAL; }
If my understanding is correct, the statement always falls through to default due to the now incorrect values for MMC_IOC_CMD and MMC_IOC_MULTI_CMD. Can we perhaps use the existing in_compat64() function to selectively match against a new hard-coded value, such as COMPAT_MMC_IOC_{MULTI_}CMD which we define in block.c? Hence not requiring compat64 applications to be rebuilt with the new uapi headers. Something like this:
if (in_compat64()) {
// New switch statement with COMPAT... values
} else {
// Original switch statement
}
Just an idea that came to mind, please excuse the lack of styling guide compliance.
Akram
On 01/11/2023 16:51, Akram Ahmad wrote:
On 01/11/2023 15:28, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
@@ -46,9 +49,9 @@ struct mmc_ioc_cmd { __u32 __pad; /* DAT buffer */ - __u64 data_ptr; + __kernel_uintptr_t data_ptr;
So... we have a bit of a problem here. It seems to be the first time that we hit it, probably because we haven't converted many (any?) ioctls yet. The ioctl command corresponding to this struct is defined thus:
#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
What this means is that the size of the struct is part of the command numerical value (see the definition of _IOWR and _IOC). Unavoidably, if we modify the struct as we do here, its size increases and the command value changes.
This is not really a problem in purecap, as applications need to be rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working as-is (without rebuilding). Sure enough, mmc-utils built against unmodified headers fails with this patch, as the value of MMC_IOC_CMD it uses is not what the kernel expects any more.
Unfortunately I don't see a way to solve this problem without hardcoding the size in the command definition (something like _IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very welcome however!
Kevin
I'm not 100% confident on this, but in drivers/mmc/core/block.c, we have the mmc_blk_ioctl function which currently contains the following:
switch (cmd) { case MMC_IOC_CMD: // ... case MMC_IOC_MULTI_CMD: // ... default: return -EINVAL; }
If my understanding is correct, the statement always falls through to default due to the now incorrect values for MMC_IOC_CMD and MMC_IOC_MULTI_CMD. Can we perhaps use the existing in_compat64() function to selectively match against a new hard-coded value, such as COMPAT_MMC_IOC_{MULTI_}CMD which we define in block.c? Hence not requiring compat64 applications to be rebuilt with the new uapi headers. Something like this:
if (in_compat64()) {
// New switch statement with COMPAT... values
} else {
// Original switch statement
}
Just an idea that came to mind, please excuse the lack of styling guide compliance.
Styling guide is definitely not a concern for pseudocode :D
It is an interesting idea. Unfortunately, it doesn't work, as in fact we need to consider 3 situations: 1. Base ABI, built against old kernel headers 2. Base ABI, built against new kernel headers 3. PCuABI, built against new kernel headers
This approach would work for 1. and 3. but not 2. It could be made to work by accepting both the old and new command value in compat64, but at this point the added complexity becomes pretty debatable. Another angle is that we want to make as few uapi changes as possible. Not changing the command value does mean that the struct size no longer matches the command in PCuABI, but I don't think that's a concern in practice, unless the struct and command are defined in other headers (probably not the case here).
Kevin
On 11/1/23 15:28, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
Yup. I've hit this too - battled with it. I was a bit bemused for a while with uintptr_t not working... the above was the solution. It moved me forward. I am unhappy with it but it's one of those "come back to it at a later stage". I have quite a few of these in my drm work by now.
rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working
Yup. That's been a lot of my fiddling in keeping drm compat64 working and making purecap work at the same time. I've made purecap work several times only to find I broke compat subtly and had to spend a lot of printk()ing and rebooting to figure out why (or ... I broke libdrm in userspace which is a separate thing I have to also adapt to a new purecap ABI AND keep it working with the original 64 and 32bit APIs). libdrm has it's own copy of drm kernel header files too btw... but I've managed to keep my compat binaries working so far. It's a slow and exacting process. Though for now size of ioctl structs will change and at least the drm infra seems to copy in the right sized struct based on compat vs purecap (thanks to much printk debugging in the ioctl core handling in drm)
_IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very
Well my structs are beginning to look like:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ __kernel_uintptr_t props_ptr; __kernel_uintptr_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same struct - keep the previous compat behavior but CHERI gets the new type sizes. The drm folk seems to have oscillated between unsigned longs and __u64's being "good enough for pointers from userspace". It's not consistent, but it is what it is. This above is the kernel header. My copy of the same structs in libdrm:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ uintcap_t props_ptr; uintcap_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same pattern but using the cleaner type. i could typdef this and use those instead. I am actually wanting to be a bit more explicit here due to the "sometimes long, sometimes __u64 is good enough". I actually am ignoring the doubling up of kernel headers for now (include/uapi/drm/ and tools/include/uapi/drm/ often duplicate the same drm struct types etc. ... hooray - I'm ignoring tools/... for now and will worry about that at the end).
So for vblank I'm hybrid re-using the compat32 infra that for us is actually compat64 and the shared common infra. For other things I've morphed into doing things like:
int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { #ifdef CONFIG_COMPAT struct drm_mode_obj_get_properties32 { __u64 props_ptr; __u64 prop_values_ptr; __u32 count_props; __u32 obj_id; __u32 obj_type; }; struct drm_mode_obj_get_properties32 *arg32 = data; #endif struct drm_mode_obj_get_properties *arg = data;
...
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { count_props = arg32->count_props; obj_id = arg32->obj_id; obj_type = arg32->obj_type; props_ptr = uaddr_to_user_ptr(arg32->props_ptr); prop_values_ptr = uaddr_to_user_ptr(arg32->prop_values_ptr); } else { #endif count_props = arg->count_props; obj_id = arg->obj_id; obj_type = arg->obj_type; props_ptr = (uint32_t __user *)(arg->props_ptr); prop_values_ptr = (uint64_t __user *)(arg->prop_values_ptr); #ifdef CONFIG_COMPAT } #endif
...
Yes - I know it's "32". It lies. drm infra has already accepted that "compat" == named 32bit by convention within its tree but for us compat == 64bit, so I'm just following the convention. count_props, obj_id, obj_type etc. are local vars and then to write them back out on successful ioctl call before return:
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { arg32->count_props = count_props; } else { #endif arg->count_props = count_props; #ifdef CONFIG_COMPAT } #endif DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; }
I have to restructure the code to basically have these import+export blocks. It's not pretty but it works. I could do some weird macros like
#ifdef CONFIG_COMPAT # define VALMIRROR(structname, member) if (test_thread_flag(TIF_64BIT_COMPAT)) { member = structname ## 32->member; } else { member = structname->member; } #else # define VALMIRROR(structname, member) member = structname->member #endif
But... that'd be a lot of expanded if's and it makes it a less obvious to me what's going on so i chose not to get tricky. Also the way members were used from the ioctls was often passing them in to funcs so I still need to create my local mirror types and i need to be nice with some casting stuff too so I've kept it more obvious.
All in the fun of defining a new ABI and still keeping compat working. 🙂
(Resend from my properly subscribed email)
On 11/1/23 15:28, Kevin Brodsky wrote:
On 31/10/2023 17:20, Akram Ahmad wrote:
- __u64 data_ptr;
- __kernel_uintptr_t data_ptr;
Yup. I've hit this too - battled with it. I was a bit bemused for a while with uintptr_t not working... the above was the solution. It moved me forward. I am unhappy with it but it's one of those "come back to it at a later stage". I have quite a few of these in my drm work by now.
rebuilt against the updated kernel headers anyway, but it is a major issue in compat64, as it is essential for userspace to keep working
Yup. That's been a lot of my fiddling in keeping drm compat64 working and making purecap work at the same time. I've made purecap work several times only to find I broke compat subtly and had to spend a lot of printk()ing and rebooting to figure out why (or ... I broke libdrm in userspace which is a separate thing I have to also adapt to a new purecap ABI AND keep it working with the original 64 and 32bit APIs). libdrm has it's own copy of drm kernel header files too btw... but I've managed to keep my compat binaries working so far. It's a slow and exacting process. Though for now size of ioctl structs will change and at least the drm infra seems to copy in the right sized struct based on compat vs purecap (thanks to much printk debugging in the ioctl core handling in drm)
_IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs are not defined in uapi headers, so we cannot use them. Opinions very
Well my structs are beginning to look like:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ __kernel_uintptr_t props_ptr; __kernel_uintptr_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same struct - keep the previous compat behavior but CHERI gets the new type sizes. The drm folk seems to have oscillated between unsigned longs and __u64's being "good enough for pointers from userspace". It's not consistent, but it is what it is. This above is the kernel header. My copy of the same structs in libdrm:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ uintcap_t props_ptr; uintcap_t prop_values_ptr; #else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same pattern but using the cleaner type. i could typdef this and use those instead. I am actually wanting to be a bit more explicit here due to the "sometimes long, sometimes __u64 is good enough". I actually am ignoring the doubling up of kernel headers for now (include/uapi/drm/ and tools/include/uapi/drm/ often duplicate the same drm struct types etc. ... hooray - I'm ignoring tools/... for now and will worry about that at the end).
So for vblank I'm hybrid re-using the compat32 infra that for us is actually compat64 and the shared common infra. For other things I've morphed into doing things like:
int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { #ifdef CONFIG_COMPAT struct drm_mode_obj_get_properties32 { __u64 props_ptr; __u64 prop_values_ptr; __u32 count_props; __u32 obj_id; __u32 obj_type; }; struct drm_mode_obj_get_properties32 *arg32 = data; #endif struct drm_mode_obj_get_properties *arg = data;
...
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { count_props = arg32->count_props; obj_id = arg32->obj_id; obj_type = arg32->obj_type; props_ptr = uaddr_to_user_ptr(arg32->props_ptr); prop_values_ptr = uaddr_to_user_ptr(arg32->prop_values_ptr); } else { #endif count_props = arg->count_props; obj_id = arg->obj_id; obj_type = arg->obj_type; props_ptr = (uint32_t __user *)(arg->props_ptr); prop_values_ptr = (uint64_t __user *)(arg->prop_values_ptr); #ifdef CONFIG_COMPAT } #endif
...
Yes - I know it's "32". It lies. drm infra has already accepted that "compat" == named 32bit by convention within its tree but for us compat == 64bit, so I'm just following the convention. count_props, obj_id, obj_type etc. are local vars and then to write them back out on successful ioctl call before return:
#ifdef CONFIG_COMPAT if (test_thread_flag(TIF_64BIT_COMPAT)) { arg32->count_props = count_props; } else { #endif arg->count_props = count_props; #ifdef CONFIG_COMPAT } #endif DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); return ret; }
I have to restructure the code to basically have these import+export blocks. It's not pretty but it works. I could do some weird macros like
#ifdef CONFIG_COMPAT # define VALMIRROR(structname, member) if (test_thread_flag(TIF_64BIT_COMPAT)) { member = structname ## 32->member; } else { member = structname->member; } #else # define VALMIRROR(structname, member) member = structname->member #endif
But... that'd be a lot of expanded if's and it makes it a less obvious to me what's going on so i chose not to get tricky. Also the way members were used from the ioctls was often passing them in to funcs so I still need to create my local mirror types and i need to be nice with some casting stuff too so I've kept it more obvious.
All in the fun of defining a new ABI and still keeping compat working. 🙂
On 01/11/2023 18:31, Carsten Haitzler wrote:
_IOC(_IOC_READ|_IOC_WRITE, MMC_BLOCK_MAJOR, 0, 64)) - compat structs
are
not defined in uapi headers, so we cannot use them. Opinions very
Well my structs are beginning to look like:
struct drm_mode_obj_get_properties { #ifdef __CHERI__ __kernel_uintptr_t props_ptr; __kernel_uintptr_t prop_values_ptr;
__kernel_uintptr_t is specifically intended to be __u64 in !PCuABI, so the #ifdef'ing makes no difference at all. Also worth noting __CHERI__ is not about the ABI, it's about whether we are compiling with CHERI support - i.e. it's defined in both hybrid and purecap. __CHERI_PURE_CAPABILITY__ is the one that is defined in purecap only (but of course cannot be used in uapi headers since the kernel itself is not purecap).
Is there any struct with pointers represented as unsigned long? If that's the case, we'll have to define a new type that is equivalent to uintptr_t from a userspace perspective... That's where we may have made a mistake in naming __kernel_uintptr_t :/
#else __u64 props_ptr; __u64 prop_values_ptr; #endif __u32 count_props; __u32 obj_id; __u32 obj_type; };
So same struct - keep the previous compat behavior but CHERI gets the new type sizes. The drm folk seems to have oscillated between unsigned longs and __u64's being "good enough for pointers from userspace". It's not consistent, but it is what it is. This above is the kernel header. My copy of the same structs in libdrm:
Is there a particular reason why we couldn't directly use/import the kernel headers into libdrm?
[...]
Yes - I know it's "32". It lies. drm infra has already accepted that "compat" == named 32bit by convention within its tree but for us compat == 64bit, so I'm just following the convention.
That's fine, we're trying to have reasonably minimal diffs so lying on compat32/compat64 is acceptable (and we've already done it in other subsystems).
Now, coming back to the original topic ;) How do you handle command definitions, in such a way that the struct gets larger in PCuABI, but without the command value changing? Taking the struct above as example, the value of DRM_IOCTL_MODE_OBJ_GETPROPERTIES is going to change if its definition remains unchanged, and that's something we want to avoid.
Kevin
linux-morello@op-lists.linaro.org
-
Akram Ahmad -
Carsten Haitzler -
Carsten Haitzler -
Kevin Brodsky -
Tudor Cretu