mmc: block: Add new ioctl to send multi commands

drivers/mmc/card/block.c +6 -0

+
+       idata = kcalloc(mcci.num_of_cmds, sizeof(*idata), GFP_KERNEL);
+       if (!idata) {
+               err = -ENOMEM;
+               goto cmd_err;
+       }

Quoted 12 lines · On Wednesday 09 September 2015 16:06:01 Jon Hunter wrote:

drivers/mmc/card/block.c +12 -0

+
+       idata = kcalloc(mcci.num_of_cmds, sizeof(*idata), GFP_KERNEL);
+       if (!idata) {
+               err = -ENOMEM;
+               goto cmd_err;
+       }
+
+       cmds = (struct mmc_ioc_cmd __user *)(unsigned long)mcci.cmds_ptr;
+       for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
+               idata[n_cmds] = mmc_blk_ioctl_copy_from_user(&cmds[n_cmds]);
+               if (IS_ERR(idata[n_cmds])) {
+                       err = PTR_ERR(idata[n_cmds]);

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 629

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+                       goto cmd_err;
+               }
+       }
+

You have no upper bound on the number of commands, which means you end up catching overly large arguments only through -ENOMEM. Can you come up with an upper bound that is guaranteed to succeed with the allocation?

Or would it be possible to process the user data one at a time while going through the commands?

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 4 lines

include/uapi/linux/mmc/ioctl.h +4 -0

+struct mmc_ioc_multi_cmd {
+       __u64 cmds_ptr;
+       uint8_t num_of_cmds;
+};

complex commands are always nasty in one way or another. Can you describe in the patch description why you picked an indirect pointer over something like

struct mmc_ioc_multi_cmd {
	__u64 num_of_cmds;
	struct mmc_ioc_cmd cmds[0];
};

as I said, both are ugly. My first choice would have been the other one, but I'm sure you have some reasons yourself.

Quoted 13 lines · Arnd Bergmann

Quoted 12 lines · On Wednesday 09 September 2015 16:06:01 Jon Hunter wrote:

drivers/mmc/card/block.c +12 -0

+
+       idata = kcalloc(mcci.num_of_cmds, sizeof(*idata), GFP_KERNEL);
+       if (!idata) {
+               err = -ENOMEM;
+               goto cmd_err;
+       }
+
+       cmds = (struct mmc_ioc_cmd __user *)(unsigned long)mcci.cmds_ptr;
+       for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
+               idata[n_cmds] = mmc_blk_ioctl_copy_from_user(&cmds[n_cmds]);
+               if (IS_ERR(idata[n_cmds])) {
+                       err = PTR_ERR(idata[n_cmds]);

Quoted 8 lines

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 629

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+                       goto cmd_err;
+               }
+       }
+

You have no upper bound on the number of commands, which means you end up catching overly large arguments only through -ENOMEM. Can you come up with an upper bound that is guaranteed to succeed with the allocation?

The uint8 type would limit you to 256 commands (if you have the memory), although admittedly that is probably overkill.

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 4 lines

include/uapi/linux/mmc/ioctl.h +4 -0

+struct mmc_ioc_multi_cmd {
+       __u64 cmds_ptr;
+       uint8_t num_of_cmds;
+};

complex commands are always nasty in one way or another. Can you describe in the patch description why you picked an indirect pointer over something like

Quoted 7 lines

struct mmc_ioc_multi_cmd {
	__u64 num_of_cmds;
	struct mmc_ioc_cmd cmds[0];
};

as I said, both are ugly. My first choice would have been the other one, but I'm sure you have some reasons yourself.

It was a suggestion from Olof to ensure the structure size is constant for both 32-bit and 64-bit userspaces. I am not sure if it is worth adding a macro similar to the below for this?

#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr

However, yes can update the changelog.

Quoted 24 lines · On Wednesday 09 September 2015 17:44:54 Jon Hunter wrote:

Quoted 13 lines · Arnd Bergmann

Quoted 12 lines · On Wednesday 09 September 2015 16:06:01 Jon Hunter wrote:

drivers/mmc/card/block.c +12 -0

+
+       idata = kcalloc(mcci.num_of_cmds, sizeof(*idata), GFP_KERNEL);
+       if (!idata) {
+               err = -ENOMEM;
+               goto cmd_err;
+       }
+
+       cmds = (struct mmc_ioc_cmd __user *)(unsigned long)mcci.cmds_ptr;
+       for (n_cmds = 0; n_cmds < mcci.num_of_cmds; n_cmds++) {
+               idata[n_cmds] = mmc_blk_ioctl_copy_from_user(&cmds[n_cmds]);
+               if (IS_ERR(idata[n_cmds])) {
+                       err = PTR_ERR(idata[n_cmds]);

Quoted 8 lines

drivers/mmc/card/block.c static int mmc_blk_ioctl_cmd(struct block_device *bdev, · line 629

Quoted 4 lines

drivers/mmc/card/block.c +4 -0

+                       goto cmd_err;
+               }
+       }
+

You have no upper bound on the number of commands, which means you end up catching overly large arguments only through -ENOMEM. Can you come up with an upper bound that is guaranteed to succeed with the allocation?

Quoted 2 lines

The uint8 type would limit you to 256 commands (if you have the memory), although admittedly that is probably overkill.

Good point.

Please note a few details here:

- in uabi headers, we need to use __u8 instead of uint8, because we cannot rely on libc header file inclusion for kernel headers.

- you have some implicit padding after the structure and should replace that with explictit pad bytes to extend the structure to a multiple of its alignment (8 bytes).

Quoted 17 lines

Quoted 9 lines

include/uapi/linux/mmc/ioctl.h struct mmc_ioc_cmd { · line 56

Quoted 4 lines

include/uapi/linux/mmc/ioctl.h +4 -0

+struct mmc_ioc_multi_cmd {
+       __u64 cmds_ptr;
+       uint8_t num_of_cmds;
+};

complex commands are always nasty in one way or another. Can you describe in the patch description why you picked an indirect pointer over something like

Quoted 7 lines

struct mmc_ioc_multi_cmd {
	__u64 num_of_cmds;
	struct mmc_ioc_cmd cmds[0];
};

as I said, both are ugly. My first choice would have been the other one, but I'm sure you have some reasons yourself.

Quoted 7 lines

It was a suggestion from Olof to ensure the structure size is constant for both 32-bit and 64-bit userspaces. I am not sure if it is worth adding a macro similar to the below for this?

#define mmc_ioc_cmd_set_data(ic, ptr) ic.data_ptr = (__u64)(unsigned long) ptr

However, yes can update the changelog.

I was not referring to the use of an __u64 variable to pass a pointer, that is expected (and the macro would make it harder to understand).

What I meant instead was the use of a pointer to an array as opposed to passing the array itself. With the definition I gave above, the size would still be the same on all architectures (you can replace the __u64 with an __u8 plus padding if you like), as sizeof(struct mmc_ioc_multi_cmd) is just '8' here.

Alternatively, you could just use an array of struct mmc_ioc_cmd by itself and encode the length in the ioctl command:

#define MMC_COMBO_IOC_CMD(n) _IOC(_IOC_READ|_IOC_WRITE, 1, sizeof(struct mmc_ioc_cmd) * (n))

This is of course also ugly because the ioctl command number is not fixed, and because the limit for the number of mmc command blocks is architecture dependent, depending on the definition of the _IOC macro that can have either 13 or 14 bits to encode the argument length in bytes.

Quoted 3 lines · Arnd Bergmann

Quoted 2 lines · On Wednesday 09 September 2015 17:44:54 Jon Hunter wrote:

Quoted 1 line · Arnd Bergmann

On Wednesday 09 September 2015 16:06:01 Jon Hunter wrote:

Quoted 3 lines

- you have some implicit padding after the structure and should replace that with explictit pad bytes to extend the structure to a multiple of its alignment (8 bytes).

Would padding with __u32 at the end be sufficient here? I assume the __u32 would be 32-bit aligned. However, was not sure if this would always be the case.

Quoted 8 lines

I was not referring to the use of an __u64 variable to pass a pointer, that is expected (and the macro would make it harder to understand).

What I meant instead was the use of a pointer to an array as opposed to passing the array itself. With the definition I gave above, the size would still be the same on all architectures (you can replace the __u64 with an __u8 plus padding if you like), as sizeof(struct mmc_ioc_multi_cmd) is just '8' here.

Do you have any strong preference here? I guess I don't and agree neither are ideal.

Quoted 5 lines

Alternatively, you could just use an array of struct mmc_ioc_cmd by itself and encode the length in the ioctl command:

#define MMC_COMBO_IOC_CMD(n) _IOC(_IOC_READ|_IOC_WRITE, 1, sizeof(struct mmc_ioc_cmd) * (n))

Quoted 5 lines

This is of course also ugly because the ioctl command number is not fixed, and because the limit for the number of mmc command blocks is architecture dependent, depending on the definition of the _IOC macro that can have either 13 or 14 bits to encode the argument length in bytes.

Interesting idea. However, given your comments above, I think that I would rather place the size in the structure.

Quoted 3 lines

- you have some implicit padding after the structure and should replace that with explictit pad bytes to extend the structure to a multiple of its alignment (8 bytes).

Would padding with __u32 at the end be sufficient here? I assume the __u32 would be 32-bit aligned. However, was not sure if this would always be the case.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

struct mmc_ioc_multi_cmd {
    __u64 num_of_cmds;
    struct mmc_ioc_cmd cmds[0];
};

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

as I said, both are ugly. My first choice would have been the other one, but I'm sure you have some reasons yourself.

On Thu, Sep 10, 2015 at 1:24 AM, Jon Hunter <[email protected]> wrote:
...

Quoted 7 lines

Quoted 3 lines

- you have some implicit padding after the structure and should replace that with explictit pad bytes to extend the structure to a multiple of its alignment (8 bytes).

Would padding with __u32 at the end be sufficient here? I assume the __u32 would be 32-bit aligned. However, was not sure if this would always be the case.

Is there something wrong with implicit padding?
Only one copy of the structure is passed to the kernel for any given call.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

struct mmc_ioc_multi_cmd {
    __u64 num_of_cmds;
    struct mmc_ioc_cmd cmds[0];
};

I think this would work just as well. But doesn't "pointer to an array" require 32-bit ioctl compat handling?
We were trying to avoid a 32-bit user space compatibility handler.

On 10/09/15 18:10, Grant Grundler wrote:

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

struct mmc_ioc_multi_cmd {
    __u64 num_of_cmds;
    struct mmc_ioc_cmd cmds[0];
};

I think this would work just as well. But doesn't "pointer to an array" require 32-bit ioctl compat handling?
We were trying to avoid a 32-bit user space compatibility handler.

I think that this is fine as it is a zero length array [0] and not a pointer.

Cheers
Jon

[0] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html