LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Alasdair G Kergon <agk@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	dm-devel@redhat.com, linux-kernel@vger.kernel.org,
	Milan Broz <mbroz@redhat.com>, Guido Guenther <agx@sigxcpu.org>,
	Kevin Corry <kevcorry@us.ibm.com>,
	stable@kernel.org
Subject: Re: [2.6.24 PATCH 02/25] dm io:ctl use constant struct size
Date: Sat, 13 Oct 2007 00:16:29 +0200	[thread overview]
Message-ID: <200710130016.29937.arnd@arndb.de> (raw)
In-Reply-To: <20071012170509.GM24157@agk.fab.redhat.com>

On Friday 12 October 2007, Alasdair G Kergon wrote:
> Make size of dm_ioctl struct always 312 bytes on all supported
> architectures.
> 
> This change retains compatibility with already-compiled code because
> it uses an embedded offset to locate the payload that follows the
> structure.
> 
> On 64-bit architectures there is no change at all; on 32-bit
> we are increasing the size of dm-ioctl from 308 to 312 bytes.
> 
> Currently with 32-bit userspace / 64-bit kernel on x86_64
> some ioctls (including rename, message) are incorrectly rejected
> by the comparison against 'param + 1'.  This breaks userspace
> lvrename and multipath 'fail_if_no_path' changes, for example.
> 
> (BTW Device-mapper uses its own versioning and ignores the ioctl
> size bits.  Only the generic ioctl compat code on mixed arches
> checks them, and that will continue to accept both sizes for now,
> but we intend to list 308 as deprecated and eventually remove it.)
> 
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> Signed-off-by: Alasdair G Kergon <agk@redhat.com>
> Cc: Guido Guenther <agx@sigxcpu.org>
> Cc: Kevin Corry <kevcorry@us.ibm.com>
> Cc: stable@kernel.org
> 

This change seems rather bogus, you're changing the ABI just to work
around a bug in the compat_ioctl layer. Why not just do the compat
code the right way, like the patch below?

	Arnd <><

---
dm: move compat_ioctl handling to dm-ioctl.c

Device mapper ioctl numbers use a variable size field

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index b441d82..a662279 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -700,7 +700,7 @@ static int dev_rename(struct dm_ioctl *param, size_t param_size)
 	int r;
 	char *new_name = (char *) param + param->data_start;
 
-	if (new_name < (char *) (param + 1) ||
+	if (new_name < (char *)param + param->data_size ||
 	    invalid_str(new_name, (void *) param + param_size)) {
 		DMWARN("Invalid new logical volume name supplied.");
 		return -EINVAL;
@@ -726,7 +726,7 @@ static int dev_set_geometry(struct dm_ioctl *param, size_t param_size)
 	if (!md)
 		return -ENXIO;
 
-	if (geostr < (char *) (param + 1) ||
+	if (geostr < (char *)param + param->data_size ||
 	    invalid_str(geostr, (void *) param + param_size)) {
 		DMWARN("Invalid geometry supplied.");
 		goto out;
@@ -1233,7 +1233,7 @@ static int target_message(struct dm_ioctl *param, size_t param_size)
 	if (r)
 		goto out;
 
-	if (tmsg < (struct dm_target_msg *) (param + 1) ||
+	if (tmsg < (struct dm_target_msg *) ((char *)param + param->data_size) ||
 	    invalid_str(tmsg->message, (void *) param + param_size)) {
 		DMWARN("Invalid target message parameters.");
 		r = -EINVAL;
@@ -1348,11 +1348,11 @@ static void free_params(struct dm_ioctl *param)
 	vfree(param);
 }
 
-static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param)
+static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param, uint ulen)
 {
 	struct dm_ioctl tmp, *dmi;
 
-	if (copy_from_user(&tmp, user, sizeof(tmp)))
+	if (copy_from_user(&tmp, user, ulen))
 		return -EFAULT;
 
 	if (tmp.data_size < sizeof(tmp))
@@ -1399,13 +1399,11 @@ static int validate_params(uint cmd, struct dm_ioctl *param)
 	return 0;
 }
 
-static int ctl_ioctl(struct inode *inode, struct file *file,
-		     uint command, ulong u)
+static int ctl_ioctl(uint command, struct dm_ioctl __user *user, uint ulen)
 {
 	int r = 0;
 	unsigned int cmd;
 	struct dm_ioctl *param;
-	struct dm_ioctl __user *user = (struct dm_ioctl __user *) u;
 	ioctl_fn fn = NULL;
 	size_t param_size;
 
@@ -1447,7 +1445,7 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
 	/*
 	 * Copy the parameters into kernel space.
 	 */
-	r = copy_params(user, &param);
+	r = copy_params(user, &param, ulen);
 
 	current->flags &= ~PF_MEMALLOC;
 
@@ -1459,7 +1457,7 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
 		goto out;
 
 	param_size = param->data_size;
-	param->data_size = sizeof(*param);
+	param->data_size = ulen;
 	r = fn(param, param_size);
 
 	/*
@@ -1473,8 +1471,64 @@ static int ctl_ioctl(struct inode *inode, struct file *file,
 	return r;
 }
 
+static int ctl_ioctl(struct inode *inode, struct file *file,
+		     uint command, ulong u)
+{
+	return ctl_do_ioctl(command, (void __user *)u, sizeof (struct dm_ioctl));
+}
+
+#ifdef CONFIG_COMPAT
+struct compat_dm_ioctl {
+	/*
+	 * The version number is made up of three parts:
+	 * major - no backward or forward compatibility,
+	 * minor - only backwards compatible,
+	 * patch - both backwards and forwards compatible.
+	 *
+	 * All clients of the ioctl interface should fill in the
+	 * version number of the interface that they were
+	 * compiled with.
+	 *
+	 * All recognised ioctl commands (ie. those that don't
+	 * return -ENOTTY) fill out this field, even if the
+	 * command failed.
+	 */
+	uint32_t version[3];	/* in/out */
+	uint32_t data_size;	/* total size of data passed in
+				 * including this struct */
+
+	uint32_t data_start;	/* offset to start of data
+				 * relative to start of this struct */
+
+	uint32_t target_count;	/* in/out */
+	int32_t open_count;	/* out */
+	uint32_t flags;		/* in/out */
+	uint32_t event_nr;	/* in/out */
+	uint32_t padding;
+
+	compat_u64 dev;		/* in/out */
+
+	char name[DM_NAME_LEN];	/* device name */
+	char uuid[DM_UUID_LEN];	/* unique identifier for
+				 * the block device */
+};
+
+static int compat_ctl_ioctl(struct inode *inode, struct file *file,
+		     uint command, ulong u)
+{
+	int ret = 0;
+	lock_kernel();
+	ret = ctl_do_ioctl(command, compat_ptr(u), sizeof(struct compat_dm_ioctl));
+	unlock_kernel();
+	return ret;
+}
+#else
+#define compat_ctl_ioctl NULL
+#endif
+
 static const struct file_operations _ctl_fops = {
 	.ioctl	 = ctl_ioctl,
+	.compat_ioctl = compat_ctl_ioctl,
 	.owner	 = THIS_MODULE,
 };
 
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -76,7 +76,6 @@
 #include <linux/mii.h>
 #include <linux/if_bonding.h>
 #include <linux/watchdog.h>
-#include <linux/dm-ioctl.h>
 
 #include <linux/soundcard.h>
 #include <linux/lp.h>
@@ -1986,39 +1985,6 @@ COMPATIBLE_IOCTL(STOP_ARRAY_RO)
 COMPATIBLE_IOCTL(RESTART_ARRAY_RW)
 COMPATIBLE_IOCTL(GET_BITMAP_FILE)
 ULONG_IOCTL(SET_BITMAP_FILE)
-/* DM */
-COMPATIBLE_IOCTL(DM_VERSION_32)
-COMPATIBLE_IOCTL(DM_REMOVE_ALL_32)
-COMPATIBLE_IOCTL(DM_LIST_DEVICES_32)
-COMPATIBLE_IOCTL(DM_DEV_CREATE_32)
-COMPATIBLE_IOCTL(DM_DEV_REMOVE_32)
-COMPATIBLE_IOCTL(DM_DEV_RENAME_32)
-COMPATIBLE_IOCTL(DM_DEV_SUSPEND_32)
-COMPATIBLE_IOCTL(DM_DEV_STATUS_32)
-COMPATIBLE_IOCTL(DM_DEV_WAIT_32)
-COMPATIBLE_IOCTL(DM_TABLE_LOAD_32)
-COMPATIBLE_IOCTL(DM_TABLE_CLEAR_32)
-COMPATIBLE_IOCTL(DM_TABLE_DEPS_32)
-COMPATIBLE_IOCTL(DM_TABLE_STATUS_32)
-COMPATIBLE_IOCTL(DM_LIST_VERSIONS_32)
-COMPATIBLE_IOCTL(DM_TARGET_MSG_32)
-COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY_32)
-COMPATIBLE_IOCTL(DM_VERSION)
-COMPATIBLE_IOCTL(DM_REMOVE_ALL)
-COMPATIBLE_IOCTL(DM_LIST_DEVICES)
-COMPATIBLE_IOCTL(DM_DEV_CREATE)
-COMPATIBLE_IOCTL(DM_DEV_REMOVE)
-COMPATIBLE_IOCTL(DM_DEV_RENAME)
-COMPATIBLE_IOCTL(DM_DEV_SUSPEND)
-COMPATIBLE_IOCTL(DM_DEV_STATUS)
-COMPATIBLE_IOCTL(DM_DEV_WAIT)
-COMPATIBLE_IOCTL(DM_TABLE_LOAD)
-COMPATIBLE_IOCTL(DM_TABLE_CLEAR)
-COMPATIBLE_IOCTL(DM_TABLE_DEPS)
-COMPATIBLE_IOCTL(DM_TABLE_STATUS)
-COMPATIBLE_IOCTL(DM_LIST_VERSIONS)
-COMPATIBLE_IOCTL(DM_TARGET_MSG)
-COMPATIBLE_IOCTL(DM_DEV_SET_GEOMETRY)
 /* Big K */
 COMPATIBLE_IOCTL(PIO_FONT)
 COMPATIBLE_IOCTL(GIO_FONT)

  reply	other threads:[~2007-10-12 22:16 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-10-12 17:05 Alasdair G Kergon
2007-10-12 22:16 ` Arnd Bergmann [this message]
2007-10-15  1:26   ` Alasdair G Kergon
2007-10-15  7:34     ` Arnd Bergmann
2007-11-13 22:22       ` [stable] " Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200710130016.29937.arnd@arndb.de \
    --to=arnd@arndb.de \
    --cc=agk@redhat.com \
    --cc=agx@sigxcpu.org \
    --cc=dm-devel@redhat.com \
    --cc=kevcorry@us.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mbroz@redhat.com \
    --cc=stable@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [2.6.24 PATCH 02/25] dm io:ctl use constant struct size' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).