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