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