On 17/10/2023 13:43, Akram Ahmad wrote:

Introduce a compat version of the struct `mmc_ioc_cmd` and struct

Nit: backticks are not normally used in either commit messages or comments, since they are already considered code / plain text.

`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 | 107 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 2f8884e3d4c2..3d0440fe63df 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,54 @@ struct mmc_blk_ioc_data { struct mmc_rpmb_data *rpmb; }; +/*

• • Copy the data from a `compat_mmc_ioc_cmd` user pointer, `src`

Comma after src, otherwise it reads like src is pointing to kernel space :)

• • 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 = {};

No need to initialise the struct, since the next line will set it in full (or fail).

• 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->response[0] = compat_cmd.response[0];
• native_cmd->response[1] = compat_cmd.response[1];
• native_cmd->response[2] = compat_cmd.response[2];
• native_cmd->response[3] = compat_cmd.response[3];

It sounds like you hesitated between memcpy() and explicit assignment, and included both :D

• 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 mmc_ioc_cmd_copy_from_user(

Nit: so far we've tried to name such functions copy_{struct name}_from_user.

• struct mmc_ioc_cmd *to,
• void * __user src)

It should all fit in just one line (even a few characters beyond 80 are not a concern). If a line break is unavoidable, better to keep the first argument on the same line and align on the opening parentheses, if possible.

+{

• 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 +488,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 (mmc_ioc_cmd_copy_from_user(&idata->ic, user)) { err = -EFAULT; goto idata_err; }

@@ -449,8 +524,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)))
  

• void __user *response_uptr = in_compat64() ?

Nit: could make it a __u32 __user * (just to make it a bit more self-documenting).

• 	&((struct compat_mmc_ioc_cmd __user *)ic_ptr)->response :
  

• 	&ic_ptr->response;
  

• if (copy_to_user(response_uptr, ic->response, sizeof(*response_uptr)))

I'm surprised this builds - at the moment *response_uptr is void, which has no size. There's nothing wrong with keeping sizeof(ic->response).

return -EFAULT;

if (!idata->ic.write_flag) { @@ -666,20 +744,35 @@ static int mmc_blk_ioctl_cmd(struct mmc_blk_data *md, return ioc_err ? ioc_err : err; } +static inline void __user *get_ith_mmc_ioc_cmd_uptr(

• void __user *cmds_array,

As such, using void __user * makes sense, since both the input and output can be either native or compat. However, since we are leaving the existing functions unchanged, I think it would be preferable to use the native types here too and cast in the compat case only.

• unsigned int i)

+{

• if (in_compat64())

• return (void __user *)&((struct compat_mmc_ioc_cmd __user *)cmds_array)[i];
  

• return (void __user *)&((struct mmc_ioc_cmd __user *)cmds_array)[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; __u64 num_of_cmds; unsigned int i, n; struct request *req;

• void __user *ioc_cmds = in_compat64() ?

• 	(void __user *)((struct compat_mmc_ioc_multi_cmd __user *)user)->cmds :
  

• 	(void __user *)user->cmds;
  

To reduce the amount of conditional code (and casts!), this could be folded into get_ith_mmc_ioc_cmd_uptr(), in which case it would take the user pointer directly.

• __u64 __user *num_of_cmds_uptr = in_compat64() ?

• 	(__u64 __user *)&((struct compat_mmc_ioc_multi_cmd __user *)user)->num_of_cmds :
  

• 	&user->num_of_cmds;
  

In this particular case, we can save ourselves some trouble and use the native layout unconditionally. That's because num_of_cmds is at the beginning of the struct and thus has offset 0 regardless. That would deserve a comment though.

Nice patch overall, those suggestions are all minor!

Kevin

• if (copy_from_user(&num_of_cmds, &user->num_of_cmds,

• if (copy_from_user(&num_of_cmds, num_of_cmds_uptr, sizeof(num_of_cmds))) return -EFAULT;

@@ -695,7 +788,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(&ioc_cmds, i));
  

  if (IS_ERR(idata[i])) { err = PTR_ERR(idata[i]); n = i;

@@ -732,7 +825,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(&ioc_cmds, i), idata[i]);
  

blk_mq_free_request(req);