dm-ioctl on amd64 with 32bit userspace

dm-ioctl on amd64 with 32bit userspace

[Guido Guenther]

Hi,

using something like:

dmsetup message <mpath> 0 fail_if_no_path

on a 32 bit dmsetup with a amd64 kernel currently fails with:
 "Invalid target message parameters."

since the structure padding is 8 bytes and so we're looking at a 4 byte
to high address in target_message() - this also affects dev_rename() and
dev_set_geometry(). The attached patch works around the problem but
doesn't look like a proper fix. But since all the 32bit to 64bit ioctl
conversion in device mapper seems a bit strange, I'm not sure where to
fix this properly.

The bad thing about this is that multipath failover after a trespas on
such a system won't work (among other things). This at least has been
broken since 2.6.18 (I doubt it ever worked) and is still broken in
2.6.23-rc6-git7. With this patch, failover after a trespas and device
renaming works as expected.
Cheers,
 -- Guido

Signed-off-by: Guido Guenther <agx@sigxcpu.org>

--- aa-amd64-smp/drivers/md/dm-ioctl.c	2007-09-17 14:51:10.000000000 +0000
+++ linux-2.6.22/drivers/md/dm-ioctl.c	2007-07-08 23:32:17.000000000 +0000
@@ -700,7 +700,7 @@
 	int r;
 	char *new_name = (char *) param + param->data_start;
 
-	if (new_name < (char *) (param + 1) ||
+	if (new_name < (char *) ((void*)(param + 1) - 4) ||
 	    invalid_str(new_name, (void *) param + param_size)) {
 		DMWARN("Invalid new logical volume name supplied.");
 		return -EINVAL;
@@ -726,7 +726,7 @@
 	if (!md)
 		return -ENXIO;
 
-	if (geostr < (char *) (param + 1) ||
+	if (geostr < (char *) ((void *)(param + 1) - 4) ||
 	    invalid_str(geostr, (void *) param + param_size)) {
 		DMWARN("Invalid geometry supplied.");
 		goto out;
@@ -1233,7 +1233,7 @@
 	if (r)
 		goto out;
 
-	if (tmsg < (struct dm_target_msg *) (param + 1) ||
+	if (tmsg < (struct dm_target_msg *) ((void*)(param + 1) - 4) ||
 	    invalid_str(tmsg->message, (void *) param + param_size)) {
 		DMWARN("Invalid target message parameters.");
 		r = -EINVAL;

Re: dm-ioctl on amd64 with 32bit userspace

[Andrew Morton]

On Mon, 17 Sep 2007 18:57:26 +0200
Guido Guenther <agx@sigxcpu.org> wrote:

> using something like:
> 
> dmsetup message <mpath> 0 fail_if_no_path
> 
> on a 32 bit dmsetup with a amd64 kernel currently fails with:
>  "Invalid target message parameters."
> 
> since the structure padding is 8 bytes and so we're looking at a 4 byte
> to high address in target_message() - this also affects dev_rename() and
> dev_set_geometry(). The attached patch works around the problem but
> doesn't look like a proper fix. But since all the 32bit to 64bit ioctl
> conversion in device mapper seems a bit strange, I'm not sure where to
> fix this properly.
> 
> The bad thing about this is that multipath failover after a trespas on
> such a system won't work (among other things). This at least has been
> broken since 2.6.18 (I doubt it ever worked) and is still broken in
> 2.6.23-rc6-git7. With this patch, failover after a trespas and device
> renaming works as expected.

I don't understand why people aren't running around with their hair on fire
over this bug.    Is everyone running 64-bit dmsetup, or what?

This looks like 2.6.22 and 2.6.23 material to me.  Alasdair?

Re: dm-ioctl on amd64 with 32bit userspace

[Alasdair G Kergon]

On Wed, Sep 19, 2007 at 05:26:50PM -0700, Andrew Morton wrote:
> I don't understand why people aren't running around with their hair on fire
> over this bug.    Is everyone running 64-bit dmsetup, or what?
 
1) There probably aren't many users in a mixed 32/64 environment;
2) The operations triggering the problem are fairly uncommon.

It's still a surprise nobody noticed this before (with lvrename) as it looks
like this has always been broken.

This particular patch looks inadequate though - fixes it for some people
but breaks it for others?  Other parts of this code call alignment
functions and store offsets explicitly and probably something similar
should be happening here - with userspace libdevmapper changes too.
Let me consider the alternatives and their consequences.

> This looks like 2.6.22 and 2.6.23 material to me.  Alasdair?

When we have a complete fix, yes.

Alasdair
-- 
agk@redhat.com

Re: dm-ioctl on amd64 with 32bit userspace

[Guido Guenther]

Hi,
On Thu, Sep 20, 2007 at 02:24:48AM +0100, Alasdair G Kergon wrote:
> On Wed, Sep 19, 2007 at 05:26:50PM -0700, Andrew Morton wrote:
> > I don't understand why people aren't running around with their hair on fire
> > over this bug.    Is everyone running 64-bit dmsetup, or what?
>  
> 1) There probably aren't many users in a mixed 32/64 environment;
> 2) The operations triggering the problem are fairly uncommon.
> 
> It's still a surprise nobody noticed this before (with lvrename) as it looks
> like this has always been broken.
> 
> This particular patch looks inadequate though - fixes it for some people
> but breaks it for others?  Other parts of this code call alignment
Well at least it shouldn't break things for others, it just makes the
comparision a bit less strict - I agree that the code doesn't look nice
though.
Cheers,
 -- Guido

Re: dm-ioctl on amd64 with 32bit userspace

[Alasdair G Kergon]

On Mon, Sep 17, 2007 at 06:57:26PM +0200, Guido Guenther wrote:
> -	if (new_name < (char *) (param + 1) ||
> +	if (new_name < (char *) ((void*)(param + 1) - 4) ||

Does this (untested) also work for you?

+	if (new_name < (char *) (align_ptr(param + 1) - 4) ||

I think that's slightly better as it should leave the 32-bit
case unchanged.

One day, we'll fix all this properly by making the structure size in the
interface architecture-independent as it should have been in the first
place...

[http://www.kernel.org/pub/linux/kernel/people/agk/patches/2.6/editing/dm-ioctl-fix-compat-bounds-test.patch
but wait for it to resync]

Alasdair
-- 
agk@redhat.com

Re: dm-ioctl on amd64 with 32bit userspace

[Guido Guenther]

Hi Alasdair,
On Thu, Sep 20, 2007 at 04:32:10PM +0100, Alasdair G Kergon wrote:
> Does this (untested) also work for you?
> 
> +	if (new_name < (char *) (align_ptr(param + 1) - 4) ||
> 
Yes this seems to work, I used:

Signed-off-by: Guido Guenther <agx@sigxcpu.org>

--- aa-amd64-smp/drivers/md/dm-ioctl.c	2007-09-17 14:51:10.000000000 +0000
+++ linux-2.6.22/drivers/md/dm-ioctl.c	2007-07-08 23:32:17.000000000 +0000
@@ -700,7 +700,7 @@
 	int r;
 	char *new_name = (char *) param + param->data_start;
 
-	if (new_name < (char *) (param + 1) ||
+	if (new_name < (char *) (align_ptr(param + 1) - 4) ||
 	    invalid_str(new_name, (void *) param + param_size)) {
 		DMWARN("Invalid new logical volume name supplied.");
 		return -EINVAL;
@@ -726,7 +726,7 @@
 	if (!md)
 		return -ENXIO;
 
-	if (geostr < (char *) (param + 1) ||
+	if (geostr < (char *) (align_ptr(param + 1) - 4) ||
 	    invalid_str(geostr, (void *) param + param_size)) {
 		DMWARN("Invalid geometry supplied.");
 		goto out;
@@ -1233,7 +1233,7 @@
 	if (r)
 		goto out;
 
-	if (tmsg < (struct dm_target_msg *) (param + 1) ||
+	if (tmsg < (struct dm_target_msg *) (align_ptr(param + 1) - 4) ||
 	    invalid_str(tmsg->message, (void *) param + param_size)) {
 		DMWARN("Invalid target message parameters.");
 		r = -EINVAL;

Re: Re: dm-ioctl on amd64 with 32bit userspace

[Alasdair G Kergon]

Here's the latest - hopefully final - incarnation of the patch.

Alasdair.


From: Milan Broz <mbroz@redhat.com>

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

---
 drivers/md/dm-ioctl.c    |    6 +++---
 include/linux/dm-ioctl.h |    5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

Index: linux-2.6.23-rc8/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.23-rc8.orig/drivers/md/dm-ioctl.c	2007-10-04 17:24:00.000000000 +0100
+++ linux-2.6.23-rc8/drivers/md/dm-ioctl.c	2007-10-04 17:24:02.000000000 +0100
@@ -700,7 +700,7 @@ static int dev_rename(struct dm_ioctl *p
 	int r;
 	char *new_name = (char *) param + param->data_start;
 
-	if (new_name < (char *) (param + 1) ||
+	if (new_name < (char *) param->data ||
 	    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_io
 	if (!md)
 		return -ENXIO;
 
-	if (geostr < (char *) (param + 1) ||
+	if (geostr < (char *) param->data ||
 	    invalid_str(geostr, (void *) param + param_size)) {
 		DMWARN("Invalid geometry supplied.");
 		goto out;
@@ -1233,7 +1233,7 @@ static int target_message(struct dm_ioct
 	if (r)
 		goto out;
 
-	if (tmsg < (struct dm_target_msg *) (param + 1) ||
+	if (tmsg < (struct dm_target_msg *) param->data ||
 	    invalid_str(tmsg->message, (void *) param + param_size)) {
 		DMWARN("Invalid target message parameters.");
 		r = -EINVAL;
Index: linux-2.6.23-rc8/include/linux/dm-ioctl.h
===================================================================
--- linux-2.6.23-rc8.orig/include/linux/dm-ioctl.h	2007-10-04 17:24:00.000000000 +0100
+++ linux-2.6.23-rc8/include/linux/dm-ioctl.h	2007-10-04 17:24:02.000000000 +0100
@@ -131,6 +131,7 @@ struct dm_ioctl {
 	char name[DM_NAME_LEN];	/* device name */
 	char uuid[DM_UUID_LEN];	/* unique identifier for
 				 * the block device */
+	char data[7];		/* padding or data */
 };
 
 /*
@@ -285,9 +286,9 @@ typedef char ioctl_struct[308];
 #define DM_DEV_SET_GEOMETRY	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
 
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	11
+#define DM_VERSION_MINOR	12
 #define DM_VERSION_PATCHLEVEL	0
-#define DM_VERSION_EXTRA	"-ioctl (2006-10-12)"
+#define DM_VERSION_EXTRA	"-ioctl (2007-10-02)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */