v6 changelog for mmc ioctl patch

v6 changelog for mmc ioctl patch

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2408 bytes --]

The changes in this version are mostly to address Arnd's comments on v4.

Arnd, I added a flag to allow the caller to select between normal CMDs
and ACMDs.  This really makes it a generic passthrough.  However, I only
tested a few commands to see that passthrough was working.  I did not
try any advanced handshaking CMDs.  Hopefully this generic passthrough
is more useful to you than previous incarnations.

I chose not to create higher-level ioctl operations specifically for the
security functions because they don't really buy us anything.  At most,
I could make a function for each security opcode that would specify the
numeric opcode directly (instead of having it come from userspace).  On
the back end, all of these functions would have the same functionality
of packing the opcode and arguments into the appropriate structures,
then issuing the request - essentially the same behaviour as my current
mmc_blk_ioctl_cmd().  The "higher-level" then just becomes an opcode
filter, and that was already deemed to be undesirable by Andrei and
Michał in the v2 thread.  As I explained in the past, I can't implement
any more of the security functionality in the kernel than that.

Anyway, here's the full patch history:

- v6
    - refix 32+64 compat pointer for better portability
    - copy userspace pointer *before* using
    - apply upper limit to data buffer size
    - add flag to allow normal CMD opcodes as well as ACMD opcodes
    - remove unnecessary mutex grab

- v5
    - fix 32-bit compiler warning about the 32+64 compat pointer

- v4
    - replace postsleep udelay() with usleep_range()
    - add cmd_timeout_ms field for R1B commands

- v3
    - copy data from userspace before claiming host
    - break out copy from userspace into its own function
    - verify that caller has CAP_SYS_RAWIO
    - rename ``struct sd_ioc_cmd`` to ``struct mmc_ioc_cmd`` because it
      applies generally, not just to SD
    - make struct mmc_ioc_cmd the same between 32-bit and 64-bit to
      simplify compat_ioctl()
    - export include/linux/mmc/ioctl.h when you ``make headers_install``

- v2
    - make initialization of struct declarations match kernel style
    - only allow ioctl() on whole block device, not partition
    - remove extraneous printks
    - implement compat_ioctl()
    - remove version field from ``struct sd_ioc_cmd``

John

[PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

Allows appropriately-privileged applications to send CMD (normal) and
ACMD (application-specific; preceded with CMD55) commands to
cards/devices on the mmc bus.  This is primarily useful for enabling the
security functionality built in to every SD card.

It can also be used as a generic passthrough (e.g. to enable virtual
machines to control mmc bus devices directly).  However, this use case
has not been tested rigorously.  Generic passthrough testing was only
conducted for a few non-security opcodes to prove the feasibility of the
passthrough.

Since any opcode can be sent using this passthrough, it is very possible
to render the card/device unusable.  Applications that use this ioctl
must have CAP_SYS_RAWIO.

Security commands tested on TI PCIxx12 (SDHCI), Sigma Designs SMP8652
SoC, TI OMAP3621 SoC, TI OMAP3630 SoC, Samsung S5PC110 SoC, Qualcomm
MSM7200A SoC.

Signed-off-by: John Calixto <john.calixto@modsystems.com>
Reviewed-by: Andrei Warkentin <andreiw@motorola.com>
---
 drivers/mmc/card/block.c  |  201 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/sd_ops.c |    3 +-
 include/linux/Kbuild      |    1 +
 include/linux/mmc/Kbuild  |    1 +
 include/linux/mmc/core.h  |    1 +
 include/linux/mmc/ioctl.h |   46 ++++++++++
 6 files changed, 252 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/mmc/Kbuild
 create mode 100644 include/linux/mmc/ioctl.h

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 61d233a..19b7bb1 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -31,7 +31,11 @@
 #include <linux/mutex.h>
 #include <linux/scatterlist.h>
 #include <linux/string_helpers.h>
+#include <linux/delay.h>
+#include <linux/capability.h>
+#include <linux/compat.h>
 
+#include <linux/mmc/ioctl.h>
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/mmc.h>
@@ -158,11 +162,208 @@ mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+struct mmc_blk_ioc_data {
+	struct mmc_ioc_cmd ic;
+	unsigned char *buf;
+	u64 buf_bytes;
+	void __user *buf_user;
+};
+
+static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
+	struct mmc_ioc_cmd __user *user)
+{
+	struct mmc_blk_ioc_data *idata;
+	int err;
+
+	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
+	if (!idata) {
+		err = -ENOMEM;
+		goto copy_err;
+	}
+
+	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
+		err = -EFAULT;
+		goto copy_err;
+	}
+
+	idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks;
+	if (idata->buf_bytes > MMC_IOC_MAX_BYTES) {
+		err = -EOVERFLOW;
+		goto copy_err;
+	}
+
+	idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL);
+	if (!idata->buf) {
+		err = -ENOMEM;
+		goto copy_err;
+	}
+
+	memcpy(&idata->buf_user, &idata->ic.data_ptr, idata->ic.data_ptr_size);
+	if (copy_from_user(idata->buf, idata->buf_user, idata->buf_bytes)) {
+		err = -EFAULT;
+		goto copy_err;
+	}
+
+	return idata;
+
+copy_err:
+	kfree(idata->buf);
+	kfree(idata);
+	return ERR_PTR(err);
+
+}
+
+static int mmc_blk_ioctl_cmd(struct block_device *bdev,
+	struct mmc_ioc_cmd __user *ic_ptr)
+{
+	struct mmc_blk_ioc_data *idata;
+	struct mmc_blk_data *md;
+	struct mmc_card *card;
+	struct mmc_command cmd = {0};
+	struct mmc_data data = {0};
+	struct mmc_request mrq = {0};
+	struct scatterlist sg;
+	int err;
+
+	/*
+	 * The caller must have CAP_SYS_RAWIO, and must be calling this on the
+	 * whole block device, not on a partition.  This prevents overspray
+	 * between sibling partitions.
+	 */
+	if ((!capable(CAP_SYS_RAWIO)) || (bdev != bdev->bd_contains))
+		return -EPERM;
+
+	idata = mmc_blk_ioctl_copy_from_user(ic_ptr);
+	if (IS_ERR(idata))
+		return PTR_ERR(idata);
+
+	cmd.opcode = idata->ic.opcode;
+	cmd.arg = idata->ic.arg;
+	cmd.flags = idata->ic.flags;
+
+	data.sg = &sg;
+	data.sg_len = 1;
+	data.blksz = idata->ic.blksz;
+	data.blocks = idata->ic.blocks;
+
+	sg_init_one(data.sg, idata->buf, idata->buf_bytes);
+
+	if (idata->ic.write_flag)
+		data.flags = MMC_DATA_WRITE;
+	else
+		data.flags = MMC_DATA_READ;
+
+	mrq.cmd = &cmd;
+	mrq.data = &data;
+
+	md = mmc_blk_get(bdev->bd_disk);
+	if (!md) {
+		err = -EINVAL;
+		goto cmd_done;
+	}
+
+	card = md->queue.card;
+	if (IS_ERR(card)) {
+		err = PTR_ERR(card);
+		goto cmd_done;
+	}
+
+	mmc_claim_host(card->host);
+
+	if (idata->ic.is_acmd) {
+		err = mmc_app_cmd(card->host, card);
+		if (err)
+			goto cmd_rel_host;
+	}
+
+	/* data.flags must already be set before doing this. */
+	mmc_set_data_timeout(&data, card);
+	/* Allow overriding the timeout_ns for empirical tuning. */
+	if (idata->ic.data_timeout_ns)
+		data.timeout_ns = idata->ic.data_timeout_ns;
+
+	if ((cmd.flags & MMC_RSP_R1B) == MMC_RSP_R1B) {
+		/*
+		 * Pretend this is a data transfer and rely on the host driver
+		 * to compute timeout.  When all host drivers support
+		 * cmd.cmd_timeout for R1B, this can be changed to:
+		 *
+		 *     mrq.data = NULL;
+		 *     cmd.cmd_timeout = idata->ic.cmd_timeout_ms;
+		 */
+		data.timeout_ns = idata->ic.cmd_timeout_ms * 1000000;
+	}
+
+	mmc_wait_for_req(card->host, &mrq);
+
+	if (cmd.error) {
+		dev_err(mmc_dev(card->host), "%s: cmd error %d\n",
+						__func__, cmd.error);
+		err = cmd.error;
+		goto cmd_rel_host;
+	}
+	if (data.error) {
+		dev_err(mmc_dev(card->host), "%s: data error %d\n",
+						__func__, data.error);
+		err = data.error;
+		goto cmd_rel_host;
+	}
+
+	/*
+	 * According to the SD specs, some commands require a delay after
+	 * issuing the command.
+	 */
+	if (idata->ic.postsleep_min_us)
+		usleep_range(idata->ic.postsleep_min_us, idata->ic.postsleep_max_us);
+
+	if (copy_to_user(&(ic_ptr->response), cmd.resp, sizeof(cmd.resp))) {
+		err = -EFAULT;
+		goto cmd_rel_host;
+	}
+
+	if (!idata->ic.write_flag) {
+		if (copy_to_user(idata->buf_user, idata->buf, idata->buf_bytes)) {
+			err = -EFAULT;
+			goto cmd_rel_host;
+		}
+	}
+
+cmd_rel_host:
+	mmc_release_host(card->host);
+
+cmd_done:
+	mmc_blk_put(md);
+	kfree(idata->buf);
+	kfree(idata);
+	return err;
+}
+
+static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
+	unsigned int cmd, unsigned long arg)
+{
+	int ret = -EINVAL;
+	if (cmd == MMC_IOC_CMD)
+		ret = mmc_blk_ioctl_cmd(bdev, (struct mmc_ioc_cmd __user *)arg);
+	return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
+	unsigned int cmd, unsigned long arg)
+{
+	return mmc_blk_ioctl(bdev, mode, cmd, (unsigned long) compat_ptr(arg));
+}
+#endif
+
 static const struct block_device_operations mmc_bdops = {
 	.open			= mmc_blk_open,
 	.release		= mmc_blk_release,
 	.getgeo			= mmc_blk_getgeo,
 	.owner			= THIS_MODULE,
+	.ioctl			= mmc_blk_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl		= mmc_blk_compat_ioctl,
+#endif
 };
 
 struct mmc_blk_request {
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index 797cdb5..990dd43 100644
--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -20,7 +20,7 @@
 #include "core.h"
 #include "sd_ops.h"
 
-static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
+int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
 {
 	int err;
 	struct mmc_command cmd;
@@ -48,6 +48,7 @@ static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mmc_app_cmd);
 
 /**
  *	mmc_wait_for_app_cmd - start an application command and wait for
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 75cf611..ed38527 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -4,6 +4,7 @@ header-y += caif/
 header-y += dvb/
 header-y += hdlc/
 header-y += isdn/
+header-y += mmc/
 header-y += nfsd/
 header-y += raid/
 header-y += spi/
diff --git a/include/linux/mmc/Kbuild b/include/linux/mmc/Kbuild
new file mode 100644
index 0000000..1fb2644
--- /dev/null
+++ b/include/linux/mmc/Kbuild
@@ -0,0 +1 @@
+header-y += ioctl.h
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 07f27af..bfc6127 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -133,6 +133,7 @@ struct mmc_card;
 
 extern void mmc_wait_for_req(struct mmc_host *, struct mmc_request *);
 extern int mmc_wait_for_cmd(struct mmc_host *, struct mmc_command *, int);
+extern int mmc_app_cmd(struct mmc_host *, struct mmc_card *);
 extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card *,
 	struct mmc_command *, int);
 
diff --git a/include/linux/mmc/ioctl.h b/include/linux/mmc/ioctl.h
new file mode 100644
index 0000000..ad6a871
--- /dev/null
+++ b/include/linux/mmc/ioctl.h
@@ -0,0 +1,46 @@
+#ifndef _MMC_IOCTL_H
+#define _MMC_IOCTL_H
+struct mmc_ioc_cmd {
+	/* Implies direction of data.  true = write, false = read */
+	int write_flag;
+
+	/* Application-specific command.  true = precede with CMD55 */
+	int is_acmd;
+
+	__u32 opcode;
+	__u32 arg;
+	__u32 response[4];  /* CMD response */
+	unsigned int flags;
+	unsigned int blksz;
+	unsigned int blocks;
+
+	/*
+	 * Sleep at least postsleep_min_us useconds, and at most
+	 * postsleep_max_us useconds *after* issuing command.  Needed for some
+	 * read commands for which cards have no other way of indicating
+	 * they're ready for the next command (i.e. there is no equivalent of a
+	 * "busy" indicator for read operations).
+	 */
+	unsigned int postsleep_min_us;
+	unsigned int postsleep_max_us;
+
+	/*
+	 * Override driver-computed timeouts.  Note the difference in units!
+	 */
+	unsigned int data_timeout_ns;
+	unsigned int cmd_timeout_ms;
+
+	/* DAT buffer */
+	__u32 data_ptr_size;  /* size of the *pointer* */
+	__u64 data_ptr;
+};
+#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)
+
+/*
+ * Since this ioctl is only meant to enhance (and not replace) normal access to
+ * the mmc bus device, an upper data transfer limit of MMC_IOC_MAX_BYTES is
+ * enforced per ioctl call.  For larger data transfers, use the normal block
+ * device operations.
+ */
+#define MMC_IOC_MAX_BYTES  (512L * 256)
+#endif  /* _MMC_IOCTL_H */
-- 
1.7.4.1

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

Chris,

Have you had the chance to look at v6 of this patch?  If so, what do you
think?

John

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Chris Ball]

Hi John,

On Wed, Apr 20 2011, John Calixto wrote:
> Have you had the chance to look at v6 of this patch?  If so, what do you
> think?

Looks good to me.  Since Arnd's been reviewing, it'd be nice to get a
Reviewed-by: tag from him and acknowledgement that his concerns are all
addressed, and then I'll merge it.

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Wednesday 20 April 2011, Chris Ball wrote:
> On Wed, Apr 20 2011, John Calixto wrote:
> > Have you had the chance to look at v6 of this patch?  If so, what do you
> > think?
> 
> Looks good to me.  Since Arnd's been reviewing, it'd be nice to get a
> Reviewed-by: tag from him and acknowledgement that his concerns are all
> addressed, and then I'll merge it.

Once the pointer passing is worked out, I think we have basically resolved
the technical issues. One more thing that I just noticed: The 
__u64 data_ptr needs to be aligned, otherwise you get a problem
on x86, which uses different alignment for 64 bit members in structures
between 32 and 64 bit ABIs. Putting the 64 bit members first in the data
structure, and adding padding at the end to have a size that is a multiple
of 8 bytes should take care of this.

I don't quite understand the timeout stuff in there, so I'm not sure how
I'd fill these from an application like qemu that blindly passes down
commands. I'd feel more comfortable if we didn't have to specify these
in the interface, but I have no strict objection if you think that passing
those is a reasonable requirement.

The more important question that is still unresolved however is the
nontechnical one:

Do we want users to use this interface for DRM applications?

I would still prefer having a more high-level interface for this, ideally
something where we just export a block device for the encrypted partition,
with an ioctl interface to do the authentication. Alternatively, I could
imagine an ioctl interface that exports each SD security command as a
separate ioctl command, including the block read/write ones.

The information to do this seems to be available in
http://issuu.com/sravan/docs/sd_card, which describes the ACMDs.
It shouldn't be too hard to implement them in a high-level API.
What I don't know is whether we have a clear policy about implementing
SD standard interfaces in the kernel that are not part of the redacted
specs.

	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

2011/4/14 John Calixto <john.calixto@modsystems.com>:
[...]

> +       /* DAT buffer */
> +       __u32 data_ptr_size;  /* size of the *pointer* */
> +       __u64 data_ptr;

So... again... What's the problem with anonymous union of pointer and u64?

Example implementation:

struct mmc_ioc_cmd {
  ...
  union {
    void __user *data_ptr;
    __u64 __data_ptr_storage;
  };
};

static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
  struct mmc_ioc_cmd blk;

  if (cmd != MMC_IOC_CMD)
    return -EINVAL;

  copy_from_user((void __user *)arg, &blk) ...

  return mmc_blk_ioctl_cmd(bdev, &blk);
}

static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t
mode, unsigned int cmd, unsigned long arg)
{
  struct mmc_ioc_cmd blk;

  if (cmd != MMC_IOC_CMD)
    return -EINVAL;

  copy_from_user(compat_ptr(arg), &blk) ...
  blk.data_ptr = compat_ptr(*(__u32 *)&blk.data_ptr);

  return mmc_blk_ioctl_cmd(bdev, &blk);
}

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 220 bytes --]

Hi Michal,

On Wed, 20 Apr 2011, Michał Mirosław wrote:
> 
> So... again... What's the problem with anonymous union of pointer and u64?
> 

As Arnd pointed out, this would not work for big endian machines.

John

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

W dniu 20 kwietnia 2011 19:38 użytkownik John Calixto
<john.calixto@modsystems.com> napisał:
> Hi Michal,
>
> On Wed, 20 Apr 2011, Michał Mirosław wrote:
>>
>> So... again... What's the problem with anonymous union of pointer and u64?
>>
>
> As Arnd pointed out, this would not work for big endian machines.

There's no endianness dependency in the example I wrote. All fields in
a union start at the same address, so compat_ioctl will always take
u32 from the beginning of the space reserved, and 32-bit pointer set
by userspace will also be at the beginning.

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

W dniu 20 kwietnia 2011 19:31 użytkownik Michał Mirosław
<mirqus@gmail.com> napisał:
> 2011/4/14 John Calixto <john.calixto@modsystems.com>:
> [...]
>
>> +       /* DAT buffer */
>> +       __u32 data_ptr_size;  /* size of the *pointer* */
>> +       __u64 data_ptr;
>
> So... again... What's the problem with anonymous union of pointer and u64?
>
> Example implementation:
>
> struct mmc_ioc_cmd {
>  ...
>  union {
>    void __user *data_ptr;
>    __u64 __data_ptr_storage;
>  };
> };
>

Hmm. This might be even better:

static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
  struct mmc_ioc_cmd blk;

  if (cmd != MMC_IOC_CMD)
    return -EINVAL;

  copy_from_user((void __user *)arg, &blk) ...

#ifdef CONFIG_COMPAT
  if (is_compat_task())
    blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
#endif

  return mmc_blk_ioctl_cmd(bdev, &blk);
}

[no compat_ioctl needed]

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1004 bytes --]

Hi Michał,

On Wed, 20 Apr 2011, Michał Mirosław wrote:
> Hmm. This might be even better:
> 
> static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
> unsigned int cmd, unsigned long arg)
> {
>   struct mmc_ioc_cmd blk;
> 
>   if (cmd != MMC_IOC_CMD)
>     return -EINVAL;
> 
>   copy_from_user((void __user *)arg, &blk) ...
> 
> #ifdef CONFIG_COMPAT
>   if (is_compat_task())
>     blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
> #endif
> 
>   return mmc_blk_ioctl_cmd(bdev, &blk);
> }
> 
> [no compat_ioctl needed]
> 

I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
solution.  If everyone else thinks it is reasonable, I'll submit a v7
with it.

However, I still think it should be implemented in compat_ioctl()
because compat_blkdev_ioctl() expects it.  Either that, or I add to the
big switch in compat_blkdev_driver_ioctl(), and spreading this change
out to block/compat_ioctl.c does not seem like The Right Thing to me.

John

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Wednesday 20 April 2011 21:06:49 John Calixto wrote:
> On Wed, 20 Apr 2011, Michał Mirosław wrote:
> > Hmm. This might be even better:
> > 
> > static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
> > unsigned int cmd, unsigned long arg)
> > {
> >   struct mmc_ioc_cmd blk;
> > 
> >   if (cmd != MMC_IOC_CMD)
> >     return -EINVAL;
> > 
> >   copy_from_user((void __user *)arg, &blk) ...
> > 
> > #ifdef CONFIG_COMPAT
> >   if (is_compat_task())
> >     blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
> > #endif
> > 
> >   return mmc_blk_ioctl_cmd(bdev, &blk);
> > }

No, please don't try to invent random new ways of doing this.
Your example relies on the assumption that the task is calling
the entry point for its native word size. Some architectures
intentionally allow calling the 32 bit entry point from 64 bit
tasks and vice versa, e.g. for user space emulators converting
to a different ABI, and in that case is_compat_task() produces
the wrong result. Don't ever rely on that.

> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
> solution.  If everyone else thinks it is reasonable, I'll submit a v7
> with it.

No need for a union or a ptr_size member in the struct. Just use
a single __u64 and let the user cast the pointer to that. This
will work on all architectures.
 
> However, I still think it should be implemented in compat_ioctl()
> because compat_blkdev_ioctl() expects it.  Either that, or I add to the
> big switch in compat_blkdev_driver_ioctl(), and spreading this change
> out to block/compat_ioctl.c does not seem like The Right Thing to me.

Yes, you definitely need to fill the .compat_ioctl member. We don't want
new entries in the switch statement, in particular none that are specific
to a single driver.

	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

Hi Arnd,

On Wed, 20 Apr 2011, Arnd Bergmann wrote:
> 
> No need for a union or a ptr_size member in the struct. Just use
> a single __u64 and let the user cast the pointer to that. This
> will work on all architectures.
>  
> > However, I still think it should be implemented in compat_ioctl()
> > because compat_blkdev_ioctl() expects it.  Either that, or I add to the
> > big switch in compat_blkdev_driver_ioctl(), and spreading this change
> > out to block/compat_ioctl.c does not seem like The Right Thing to me.
> 

In the non-compat use case with a 32-bit kernel + 32-bit userspace (e.g.
ARM), the casting of the __u64 to a pointer causes a compiler warning.
It works as intended, so it's not an error, but it does not feel right
to just silence the compiler.  That's why I used a memcpy with with
explicit pointer size directly in the struct.  How else would you
recommend I handle this?

John

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Wednesday 20 April 2011 21:34:20 John Calixto wrote:
> On Wed, 20 Apr 2011, Arnd Bergmann wrote:
> > 
> > No need for a union or a ptr_size member in the struct. Just use
> > a single __u64 and let the user cast the pointer to that. This
> > will work on all architectures.
> >  
> > > However, I still think it should be implemented in compat_ioctl()
> > > because compat_blkdev_ioctl() expects it.  Either that, or I add to the
> > > big switch in compat_blkdev_driver_ioctl(), and spreading this change
> > > out to block/compat_ioctl.c does not seem like The Right Thing to me.
> > 
> 
> In the non-compat use case with a 32-bit kernel + 32-bit userspace (e.g.
> ARM), the casting of the __u64 to a pointer causes a compiler warning.
> It works as intended, so it's not an error, but it does not feel right
> to just silence the compiler.  That's why I used a memcpy with with
> explicit pointer size directly in the struct.  How else would you
> recommend I handle this?

Just cast to unsigned long and then to pointer.

	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

2011/4/20 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 20 April 2011 21:06:49 John Calixto wrote:
>> On Wed, 20 Apr 2011, Michał Mirosław wrote:
>> > Hmm. This might be even better:
>> >
>> > static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
>> > unsigned int cmd, unsigned long arg)
>> > {
>> >   struct mmc_ioc_cmd blk;
>> >
>> >   if (cmd != MMC_IOC_CMD)
>> >     return -EINVAL;
>> >
>> >   copy_from_user((void __user *)arg, &blk) ...
>> >
>> > #ifdef CONFIG_COMPAT
>> >   if (is_compat_task())
>> >     blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
>> > #endif
>> >
>> >   return mmc_blk_ioctl_cmd(bdev, &blk);
>> > }
>
> No, please don't try to invent random new ways of doing this.
> Your example relies on the assumption that the task is calling
> the entry point for its native word size. Some architectures
> intentionally allow calling the 32 bit entry point from 64 bit
> tasks and vice versa, e.g. for user space emulators converting
> to a different ABI, and in that case is_compat_task() produces
> the wrong result. Don't ever rely on that.

This doesn't make sense to me. If you call 32-bit entry point from
64-bit process, you can't reliably pass pointers through the call
(unless you limit 64-bit process to 32-bit address space).

Do you know a working example of something using this kind of cross-call?

>> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
>> solution.  If everyone else thinks it is reasonable, I'll submit a v7
>> with it.
> No need for a union or a ptr_size member in the struct. Just use
> a single __u64 and let the user cast the pointer to that. This
> will work on all architectures.

Union is just hiding this cast (it will be done in kernel) and allows
cleaner code for userspace (there's a single kernel and possibly
multiple applications that will implement this call).

>> However, I still think it should be implemented in compat_ioctl()
>> because compat_blkdev_ioctl() expects it.  Either that, or I add to the
>> big switch in compat_blkdev_driver_ioctl(), and spreading this change
>> out to block/compat_ioctl.c does not seem like The Right Thing to me.
> Yes, you definitely need to fill the .compat_ioctl member. We don't want
> new entries in the switch statement, in particular none that are specific
> to a single driver.

Hmm, you're right. fs/compat_ioctl.c falls back to plain .ioctl if
.compat_ioctl == NULL. That's not the case for block ioctls, though.
The separate compat_ioctl is the way to go.

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[John Calixto]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 897 bytes --]

On Wed, 20 Apr 2011, Michał Mirosław wrote:
> >> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
> >> solution.  If everyone else thinks it is reasonable, I'll submit a v7
> >> with it.
> > No need for a union or a ptr_size member in the struct. Just use
> > a single __u64 and let the user cast the pointer to that. This
> > will work on all architectures.
> 
> Union is just hiding this cast (it will be done in kernel) and allows
> cleaner code for userspace (there's a single kernel and possibly
> multiple applications that will implement this call).
> 

Chris,

Do you have a preference here?  I do not have a preference.  On the one
hand, not having the union makes for a cleaner-to-read struct.  On the
other hand, not having to cast the pointer in 32-bit userspace is nice
especially since I foresee using this ioctl on a lot of ARM SoCs.

John

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Chris Ball]

Hi John,

On Wed, Apr 20 2011, John Calixto wrote:
> Do you have a preference here?  I do not have a preference.  On the one
> hand, not having the union makes for a cleaner-to-read struct.  On the
> other hand, not having to cast the pointer in 32-bit userspace is nice
> especially since I foresee using this ioctl on a lot of ARM SoCs.

I'll defer to Arnd, which I guess suggests using the __u64 unless he
comes around to the union idea.  :)

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Wednesday 20 April 2011 21:46:04 Michał Mirosław wrote:
> 2011/4/20 Arnd Bergmann <arnd@arndb.de>:
> > No, please don't try to invent random new ways of doing this.
> > Your example relies on the assumption that the task is calling
> > the entry point for its native word size. Some architectures
> > intentionally allow calling the 32 bit entry point from 64 bit
> > tasks and vice versa, e.g. for user space emulators converting
> > to a different ABI, and in that case is_compat_task() produces
> > the wrong result. Don't ever rely on that.
> 
> This doesn't make sense to me. If you call 32-bit entry point from
> 64-bit process, you can't reliably pass pointers through the call
> (unless you limit 64-bit process to 32-bit address space).
> 
> Do you know a working example of something using this kind of cross-call?

There are people that use 32 bit programs on x86_64 in 64 bit mode
and switch on the ADDR_LIMIT_32BIT personality, IIRC.
This gives you more registers and lets you do 64 bit arithmetic
while not using any more memory to store long pointers.
There are a few problems with this, and the new x32 ABI will make it
cleaner.

I believe qemu also does this to run foreign user binaries. You
can use qemu-user to emulate user space with a different instruction
set, but when you call into the kernel, you have to use the native
data structures that the host understands.

> >> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
> >> solution.  If everyone else thinks it is reasonable, I'll submit a v7
> >> with it.
> > No need for a union or a ptr_size member in the struct. Just use
> > a single __u64 and let the user cast the pointer to that. This
> > will work on all architectures.
> 
> Union is just hiding this cast (it will be done in kernel) and allows
> cleaner code for userspace (there's a single kernel and possibly
> multiple applications that will implement this call).

As I explained, it doesn't work. Please read my earlier mails.

> >> However, I still think it should be implemented in compat_ioctl()
> >> because compat_blkdev_ioctl() expects it.  Either that, or I add to the
> >> big switch in compat_blkdev_driver_ioctl(), and spreading this change
> >> out to block/compat_ioctl.c does not seem like The Right Thing to me.
> > Yes, you definitely need to fill the .compat_ioctl member. We don't want
> > new entries in the switch statement, in particular none that are specific
> > to a single driver.
> 
> Hmm, you're right. fs/compat_ioctl.c falls back to plain .ioctl if
> .compat_ioctl == NULL.

No, it doesn't.


	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

W dniu 21 kwietnia 2011 07:11 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> On Wednesday 20 April 2011 21:46:04 Michał Mirosław wrote:
>> 2011/4/20 Arnd Bergmann <arnd@arndb.de>:
>> > No, please don't try to invent random new ways of doing this.
>> > Your example relies on the assumption that the task is calling
>> > the entry point for its native word size. Some architectures
>> > intentionally allow calling the 32 bit entry point from 64 bit
>> > tasks and vice versa, e.g. for user space emulators converting
>> > to a different ABI, and in that case is_compat_task() produces
>> > the wrong result. Don't ever rely on that.
>>
>> This doesn't make sense to me. If you call 32-bit entry point from
>> 64-bit process, you can't reliably pass pointers through the call
>> (unless you limit 64-bit process to 32-bit address space).
>>
>> Do you know a working example of something using this kind of cross-call?
>
> There are people that use 32 bit programs on x86_64 in 64 bit mode
> and switch on the ADDR_LIMIT_32BIT personality, IIRC.
> This gives you more registers and lets you do 64 bit arithmetic
> while not using any more memory to store long pointers.
> There are a few problems with this, and the new x32 ABI will make it
> cleaner.

Ok. Thanks for the pointers.

>> >> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
>> >> solution.  If everyone else thinks it is reasonable, I'll submit a v7
>> >> with it.
>> > No need for a union or a ptr_size member in the struct. Just use
>> > a single __u64 and let the user cast the pointer to that. This
>> > will work on all architectures.
>>
>> Union is just hiding this cast (it will be done in kernel) and allows
>> cleaner code for userspace (there's a single kernel and possibly
>> multiple applications that will implement this call).
>
> As I explained, it doesn't work. Please read my earlier mails.

If you're referring to your mail:

Subject: Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs
Date: Wed, 13 Apr 2011 01:00:39 +0200
Message-Id: <201104130100.39810.arnd@arndb.de>

Then I already provided an example of implementation that's
independent of endianness and avoids casts on userspace.

>> >> However, I still think it should be implemented in compat_ioctl()
>> >> because compat_blkdev_ioctl() expects it.  Either that, or I add to the
>> >> big switch in compat_blkdev_driver_ioctl(), and spreading this change
>> >> out to block/compat_ioctl.c does not seem like The Right Thing to me.
>> > Yes, you definitely need to fill the .compat_ioctl member. We don't want
>> > new entries in the switch statement, in particular none that are specific
>> > to a single driver.
>> Hmm, you're right. fs/compat_ioctl.c falls back to plain .ioctl if
>> .compat_ioctl == NULL.
> No, it doesn't.

I'll need to look at the code again then.

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Thursday 21 April 2011, Michał Mirosław wrote:
> Subject: Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs
> Date: Wed, 13 Apr 2011 01:00:39 +0200
> Message-Id: <201104130100.39810.arnd@arndb.de>
> 
> Then I already provided an example of implementation that's
> independent of endianness and avoids casts on userspace.
> 

Yes, v4 got that aspect right, as far as I can tell, aside
from an incorrect cast (u8* instead of u8 __user *).

	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

W dniu 21 kwietnia 2011 13:15 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> On Thursday 21 April 2011, Michał Mirosław wrote:
>> Subject: Re: [PATCH v4] mmc: Add ioctl to let userspace apps send ACMDs
>> Date: Wed, 13 Apr 2011 01:00:39 +0200
>> Message-Id: <201104130100.39810.arnd@arndb.de>
>>
>> Then I already provided an example of implementation that's
>> independent of endianness and avoids casts on userspace.
>
> Yes, v4 got that aspect right, as far as I can tell, aside
> from an incorrect cast (u8* instead of u8 __user *).

Just to have an understanding about the issue. In the mail I pointed
to, you argued that union will not work because of endianness
problems. I haven't seen other arguments against the union approach,
and I have provided a solution for this case.

BTW, kernel side can also avoid the cast if the union is extended with
32-bit field. This works because an address of a union is an address
of each of its fields. IOW all fields start at the same address
regardless if the they are 32 or 64 bits in size.

struct mmc_ioc_cmd {
  union {
    __u64 __data_ptr_storage64;
    __u32 __data_ptr_storage32;
    void __user *data_ptr;
  };
  ...
};

[...]

static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
    unsigned int cmd, unsigned long arg)
{
  struct mmc_ioc_cmd blk;

  if (cmd != MMC_IOC_CMD)
    return -EINVAL;

  copy_from_user(compat_ptr(arg), &blk) ...
  blk.data_ptr = compat_ptr(blk.__data_ptr_storage32);

  return mmc_blk_ioctl_cmd(bdev, &blk);
}

Best Regards,
Michał Mirosław

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann]

On Thursday 21 April 2011, Michał Mirosław wrote:
> W dniu 21 kwietnia 2011 13:15 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:

> static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
>     unsigned int cmd, unsigned long arg)
> {
>   struct mmc_ioc_cmd blk;
> 
>   if (cmd != MMC_IOC_CMD)
>     return -EINVAL;
> 
>   copy_from_user(compat_ptr(arg), &blk) ...
>   blk.data_ptr = compat_ptr(blk.__data_ptr_storage32);
> 
>   return mmc_blk_ioctl_cmd(bdev, &blk);
> }

Yes, this works, but it requires having a compat_ioctl() handler function that
knows about the data structure, which we generally try to avoid.
The same method would even work if you only had a pointer member in the
structure and did not even attempt to make the structure compatible.

	Arnd

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Michał Mirosław]

W dniu 21 kwietnia 2011 14:39 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
> On Thursday 21 April 2011, Michał Mirosław wrote:
>> W dniu 21 kwietnia 2011 13:15 użytkownik Arnd Bergmann <arnd@arndb.de> napisał:
>
>> static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
>>     unsigned int cmd, unsigned long arg)
>> {
>>   struct mmc_ioc_cmd blk;
>>
>>   if (cmd != MMC_IOC_CMD)
>>     return -EINVAL;
>>
>>   copy_from_user(compat_ptr(arg), &blk) ...
>>   blk.data_ptr = compat_ptr(blk.__data_ptr_storage32);
>>
>>   return mmc_blk_ioctl_cmd(bdev, &blk);
>> }
>
> Yes, this works, but it requires having a compat_ioctl() handler function that
> knows about the data structure, which we generally try to avoid.
> The same method would even work if you only had a pointer member in the
> structure and did not even attempt to make the structure compatible.

Not really. If the structures were different, you would need write
code to translate it fully (this doesn't really apply for this simple
case, where there is only one pointer at the end of the structure). My
example only fixes/converts pointers in place.

The compat/native handler could be made in one function that takes
another bool arg and ioctl handlers would look like the code below. In
this scheme you avoid having to duplicate ioctl handler at all and let
compiler optimize out conditionals (you can even force
__mmc_blk_ioctl() inline to make sure of it).

#ifndef CONFIG_COMPAT
static inline void *compat_ptr(unsigned long) { BUG(); return NULL; }
#endif

static int __mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
    unsigned int cmd, unsigned long arg, bool compat32)
{
  void __user *p;
...
  p = compat32 ? compat_ptr(arg) : (void *)arg;
...
  if (compat32)
    blk.data_ptr = compat_ptr(blk.__data_ptr_storage32);
...
}

static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
    unsigned int cmd, unsigned long arg)
{
  return __mmc_blk_ioctl(bdev, mode, cmd, arg, false);
}

#ifdef CONFIG_COMPAT
static int mmc_blk_compat_ioctl(struct block_device *bdev, fmode_t mode,
    unsigned int cmd, unsigned long arg)
{
  return __mmc_blk_ioctl(bdev, mode, cmd, arg, true);
}
#endif

Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Chris Ball]

Hi John, some trivial changes,

On Wed, Apr 13 2011, John Calixto wrote:
> diff --git a/include/linux/mmc/ioctl.h b/include/linux/mmc/ioctl.h
> new file mode 100644
> index 0000000..ad6a871
> --- /dev/null
> +++ b/include/linux/mmc/ioctl.h
> @@ -0,0 +1,46 @@
> +#ifndef _MMC_IOCTL_H
> +#define _MMC_IOCTL_H

Nitpick: LINUX_MMC_IOCTL_H is a little more consistent with the other
headers in include/linux/mmc.

> +struct mmc_ioc_cmd {
> +	/* Implies direction of data.  true = write, false = read */
> +	int write_flag;
> +
> +	/* Application-specific command.  true = precede with CMD55 */
> +	int is_acmd;
> +
> +	__u32 opcode;
> +	__u32 arg;
> +	__u32 response[4];  /* CMD response */
> +	unsigned int flags;
> +	unsigned int blksz;
> +	unsigned int blocks;
> +
> +	/*
> +	 * Sleep at least postsleep_min_us useconds, and at most
> +	 * postsleep_max_us useconds *after* issuing command.  Needed for some
> +	 * read commands for which cards have no other way of indicating
> +	 * they're ready for the next command (i.e. there is no equivalent of a
> +	 * "busy" indicator for read operations).
> +	 */
> +	unsigned int postsleep_min_us;
> +	unsigned int postsleep_max_us;
> +
> +	/*
> +	 * Override driver-computed timeouts.  Note the difference in units!
> +	 */
> +	unsigned int data_timeout_ns;
> +	unsigned int cmd_timeout_ms;
> +
> +	/* DAT buffer */
> +	__u32 data_ptr_size;  /* size of the *pointer* */
> +	__u64 data_ptr;
> +};
> +#define MMC_IOC_CMD _IOWR(MMC_BLOCK_MAJOR, 0, struct mmc_ioc_cmd)

I think we need to register the MMC_BLOCK_MAJOR (== 0xB3 00) prefix in
Documentation/ioctl/ioctl-number.txt.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child