device-mapper patchset 2.5.63-dm-1

device-mapper patchset 2.5.63-dm-1

[Joe Thornber]

Linus,

Here's the latest set of device mapper patches against 2.5.63.  All
trivial.  Please apply.

Thanks,

- Joe

[PATCH 1/8] dm: ioctl interface wasn't dropping a table reference

[Joe Thornber]

When reloading a device the ioctl interface was forgetting to drop a
reference on the new table.

--- diff/drivers/md/dm-ioctl.c	2003-01-13 14:18:15.000000000 +0000
+++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:42.000000000 +0000
@@ -887,6 +887,7 @@
 		dm_table_put(t);
 		return r;
 	}
+	dm_table_put(t);	/* md will have taken its own reference */
 
 	set_disk_ro(dm_disk(md), (param->flags & DM_READONLY_FLAG));
 	dm_put(md);

[PATCH 2/8] dm: __LOW macro fix no. 1

[Joe Thornber]

Fix __LOW macro.  [Kevin Corry]

--- diff/drivers/md/dm-table.c	2003-01-13 14:18:15.000000000 +0000
+++ source/drivers/md/dm-table.c	2003-02-26 16:09:47.000000000 +0000
@@ -79,7 +79,7 @@
 }
 
 #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) < (r)) *(l) = (r)
+#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
 
 /*
  * Combine two io_restrictions, always taking the lower value.

[PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Joe Thornber]

Use the correct size for "name" in register_with_devfs().

During Al Viro's devfs cleanup a few versions ago, this function was
rewritten, and the "name" string added. The 32-byte size is not large
enough to prevent a possible buffer overflow in the sprintf() call,
since the hash cell can have a name up to 128 characters.

[Kevin Corry]

--- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:42.000000000 +0000
+++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:52.000000000 +0000
@@ -173,7 +173,7 @@
  */
 static int register_with_devfs(struct hash_cell *hc)
 {
-	char name[32];
+	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
 	struct gendisk *disk = dm_disk(hc->md);
 
 	sprintf(name, DM_DIR "/%s", hc->name);

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Horst von Brand]

Joe Thornber <joe@fib011235813.fsnet.co.uk> said:
> Use the correct size for "name" in register_with_devfs().
> 
> During Al Viro's devfs cleanup a few versions ago, this function was
> rewritten, and the "name" string added. The 32-byte size is not large
> enough to prevent a possible buffer overflow in the sprintf() call,
> since the hash cell can have a name up to 128 characters.
> 
> [Kevin Corry]
> 
> --- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:42.000000000 +0000
> +++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:52.000000000 +0000
> @@ -173,7 +173,7 @@
>   */
>  static int register_with_devfs(struct hash_cell *hc)
>  {
> -	char name[32];
> +	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];

This either makes a large name array or generates a possibly huge array at
runtime (bad if your stack is < 8KiB).
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

On Wednesday 26 February 2003 15:04, Horst von Brand wrote:
> Joe Thornber <joe@fib011235813.fsnet.co.uk> said:
> > Use the correct size for "name" in register_with_devfs().
> >
> > During Al Viro's devfs cleanup a few versions ago, this function was
> > rewritten, and the "name" string added. The 32-byte size is not large
> > enough to prevent a possible buffer overflow in the sprintf() call,
> > since the hash cell can have a name up to 128 characters.
> >
> > [Kevin Corry]
> >
> > --- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:42.000000000 +0000
> > +++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:52.000000000 +0000
> > @@ -173,7 +173,7 @@
> >   */
> >  static int register_with_devfs(struct hash_cell *hc)
> >  {
> > -	char name[32];
> > +	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
>
> This either makes a large name array or generates a possibly huge array at
> runtime (bad if your stack is < 8KiB).

Would this be better?

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/


--- linux-2.5.60a/drivers/md/dm-ioctl.c	2003/02/13 16:43:26
+++ linux-2.5.60b/drivers/md/dm-ioctl.c	2003/02/27 14:17:00
@@ -173,8 +173,11 @@
  */
 static int register_with_devfs(struct hash_cell *hc)
 {
-	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
 	struct gendisk *disk = dm_disk(hc->md);
+	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+	if (!name) {
+		return -ENOMEM;
+	}
 
 	sprintf(name, DM_DIR "/%s", hc->name);
 	devfs_register(NULL, name, DEVFS_FL_CURRENT_OWNER,
\x10

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

On Thursday 27 February 2003 08:20, Kevin Corry wrote:
> On Wednesday 26 February 2003 15:04, Horst von Brand wrote:
> > Joe Thornber <joe@fib011235813.fsnet.co.uk> said:
> > > Use the correct size for "name" in register_with_devfs().
> > >
> > > During Al Viro's devfs cleanup a few versions ago, this function was
> > > rewritten, and the "name" string added. The 32-byte size is not large
> > > enough to prevent a possible buffer overflow in the sprintf() call,
> > > since the hash cell can have a name up to 128 characters.
> > >
> > > [Kevin Corry]
> > >
> > > --- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:42.000000000 +0000
> > > +++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:52.000000000 +0000
> > > @@ -173,7 +173,7 @@
> > >   */
> > >  static int register_with_devfs(struct hash_cell *hc)
> > >  {
> > > -	char name[32];
> > > +	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
> >
> > This either makes a large name array or generates a possibly huge array
> > at runtime (bad if your stack is < 8KiB).
>
> Would this be better?

As Joe pointed out to me, that should have been:

--- linux-2.5.60a/drivers/md/dm-ioctl.c	2003/02/13 16:43:26
+++ linux-2.5.60b/drivers/md/dm-ioctl.c	2003/02/27 14:35:20
@@ -173,14 +173,18 @@
  */
 static int register_with_devfs(struct hash_cell *hc)
 {
-	char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
 	struct gendisk *disk = dm_disk(hc->md);
+	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+	if (!name) {
+		return -ENOMEM;
+	}
 
 	sprintf(name, DM_DIR "/%s", hc->name);
 	devfs_register(NULL, name, DEVFS_FL_CURRENT_OWNER,
 		       disk->major, disk->first_minor,
 		       S_IFBLK | S_IRUSR | S_IWUSR | S_IRGRP,
 		       &dm_blk_dops, NULL);
+	kfree(name);
 	return 0;
 }
 

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Roland Dreier]

   > +	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
   > +	if (!name) {
   > +		return -ENOMEM;
   > +	}

Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
in this case, although I don't know the context this function is
called from).

 - Roland

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

On Thursday 27 February 2003 10:25, Roland Dreier wrote:
>    > +	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
>    > +	if (!name) {
>    > +		return -ENOMEM;
>    > +	}
>
> Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
> in this case, although I don't know the context this function is
> called from).
>
>  - Roland

Dammit! I'm not having a good morning. :(

Unfortunately, Linus seems to have committed that patch already. So here is a 
patch to fix just that line.

Thanks for catching that.

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/

--- a/drivers/md/dm-ioctl.c	2003/02/27 16:29:58
+++ b/drivers/md/dm-ioctl.c	2003/02/27 16:30:03
@@ -174,7 +174,7 @@
 static int register_with_devfs(struct hash_cell *hc)
 {
 	struct gendisk *disk = dm_disk(hc->md);
-	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
 	if (!name) {
 		return -ENOMEM;
 	}

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

[Sent this earlier, but it doesn't seem to have shown up yet. My outgoing 
email server seems to flake out at times. Sorry if this is a duplicate, but 
it seemed important enough to resend, since we'd like to avoid the extra 
compile failure.]


On Thursday 27 February 2003 10:25, Roland Dreier wrote:
>    > +         char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
>    > +         if (!name) {
>    > +                 return -ENOMEM;
>    > +         }
>
> Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
> in this case, although I don't know the context this function is
> called from).
>
>  - Roland


Dammit! I'm not having a good morning. :(

Unfortunately, Linus seems to have committed that patch already. So here is a 
patch to fix just that line.

Thanks for catching that.

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/


--- a/drivers/md/dm-ioctl.c	2003/02/27 16:29:58
+++ b/drivers/md/dm-ioctl.c	2003/02/27 17:21:54
@@ -174,7 +174,7 @@
 static int register_with_devfs(struct hash_cell *hc)
 {
 	struct gendisk *disk = dm_disk(hc->md);
-	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
 	if (!name) {
 		return -ENOMEM;
 	}

[PATCH 4/8] dm: deregister the misc device before removing /dev/mapper

[Joe Thornber]

Fix problem with devfs when unloading the dm module.

dm-ioctl.c: deregister the misc device, and its associated symlink
*before* removing the /dev/mapper dir.  [Alasdair Kergon]

--- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:52.000000000 +0000
+++ source/drivers/md/dm-ioctl.c	2003-02-26 16:09:57.000000000 +0000
@@ -1123,17 +1123,17 @@
 	return 0;
 
       failed:
+	devfs_remove(DM_DIR "/control");
+	if (misc_deregister(&_dm_misc) < 0)
+		DMERR("misc_deregister failed for control device");
 	dm_hash_exit();
-	misc_deregister(&_dm_misc);
 	return r;
 }
 
 void dm_interface_exit(void)
 {
-	dm_hash_exit();
-
 	devfs_remove(DM_DIR "/control");
-
 	if (misc_deregister(&_dm_misc) < 0)
 		DMERR("misc_deregister failed for control device");
+	dm_hash_exit();
 }

[PATCH 5/8] dm: bug in error path for unknown target type

[Joe Thornber]

Silly mistake in error path when an unknown target type is requested.

--- diff/drivers/md/dm-table.c	2003-02-26 16:09:47.000000000 +0000
+++ source/drivers/md/dm-table.c	2003-02-26 16:10:02.000000000 +0000
@@ -591,7 +591,7 @@
 	tgt->type = dm_get_target_type(type);
 	if (!tgt->type) {
 		tgt->error = "unknown target type";
-		goto bad;
+		return r;
 	}
 
 	tgt->table = t;

[PATCH 6/8] dm: allow slashes in dm device names

[Joe Thornber]

Allow slashes ('/') within a DM device name, but not at the beginning.

Devfs will automatically create all necessary sub-directories if a name
with embedded slashes is registered.  [Kevin Corry]

--- diff/drivers/md/dm-ioctl.c	2003-02-26 16:09:57.000000000 +0000
+++ source/drivers/md/dm-ioctl.c	2003-02-26 16:10:07.000000000 +0000
@@ -545,7 +545,7 @@
 
 static int check_name(const char *name)
 {
-	if (strchr(name, '/')) {
+	if (name[0] == '/') {
 		DMWARN("invalid device name");
 		return -EINVAL;
 	}

Re: [PATCH 6/8] dm: allow slashes in dm device names

[Greg KH]

On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> Allow slashes ('/') within a DM device name, but not at the beginning.
> 
> Devfs will automatically create all necessary sub-directories if a name
> with embedded slashes is registered.  [Kevin Corry]

Does this interact with the block device name representation in sysfs?
I don't think sysfs can handle names with '/' very well.

thanks,

greg k-h

Re: [PATCH 6/8] dm: allow slashes in dm device names

[Kevin Corry]

On Wednesday 26 February 2003 11:38, Greg KH wrote:
> On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> > Allow slashes ('/') within a DM device name, but not at the beginning.
> >
> > Devfs will automatically create all necessary sub-directories if a name
> > with embedded slashes is registered.  [Kevin Corry]
>
> Does this interact with the block device name representation in sysfs?
> I don't think sysfs can handle names with '/' very well.

This patch shouldn't affect DM's interaction with sysfs.

The names in sysfs are generated by the DM core, and are all called "dm-x", 
where x is the device minor number.

The names referred to in the above patch are used only by the device-mapper 
ioctl interface, and are not seen by the DM core. These are the names 
generated by the user-space tools, and are the ones that are passed to devfs, 
which doesn't have a problem with embedded slashes.

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/

Re: [PATCH 6/8] dm: allow slashes in dm device names

[Greg KH]

On Wed, Feb 26, 2003 at 12:17:12PM -0600, Kevin Corry wrote:
> On Wednesday 26 February 2003 11:38, Greg KH wrote:
> > On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> > > Allow slashes ('/') within a DM device name, but not at the beginning.
> > >
> > > Devfs will automatically create all necessary sub-directories if a name
> > > with embedded slashes is registered.  [Kevin Corry]
> >
> > Does this interact with the block device name representation in sysfs?
> > I don't think sysfs can handle names with '/' very well.
> 
> This patch shouldn't affect DM's interaction with sysfs.
> 
> The names in sysfs are generated by the DM core, and are all called "dm-x", 
> where x is the device minor number.

Ok, but it sure would be nice to see the "name" in sysfs instead of the
minor number, right?  Just thinking of how this would affect my
much-out-of-date sysfs dm patches :)

thanks,

greg k-h

[PATCH 7/8] dm: __LOW macro fix no. 2

[Joe Thornber]

Another fix for the __LOW macro.

When dm_table and dm_target structures are initialized, the "limits" fields 
(struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
in dm-table.c). However, zero is not a useable value in these fields. The
request queue will never let an I/O through, regardless of how small it might
be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
This change to the __LOW() macro sets these fields correctly when they are
first initialized.  [Kevin Corry]

--- diff/drivers/md/dm-table.c	2003-02-26 16:10:02.000000000 +0000
+++ source/drivers/md/dm-table.c	2003-02-26 16:10:19.000000000 +0000
@@ -79,7 +79,7 @@
 }
 
 #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
+#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
 
 /*
  * Combine two io_restrictions, always taking the lower value.

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Greg KH]

On Wed, Feb 26, 2003 at 05:12:49PM +0000, Joe Thornber wrote:
> Another fix for the __LOW macro.
> 
> When dm_table and dm_target structures are initialized, the "limits" fields 
> (struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
> in dm-table.c). However, zero is not a useable value in these fields. The
> request queue will never let an I/O through, regardless of how small it might
> be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
> This change to the __LOW() macro sets these fields correctly when they are
> first initialized.  [Kevin Corry]
> 
> --- diff/drivers/md/dm-table.c	2003-02-26 16:10:02.000000000 +0000
> +++ source/drivers/md/dm-table.c	2003-02-26 16:10:19.000000000 +0000
> @@ -79,7 +79,7 @@
>  }
>  
>  #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
> +#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)

Any reason to not use the existing min() and max() macros instead of
these?  Then:
	__HIGH(foo, bar);
can be written as:
	foo = max(foo, bar);
which is (IMHO) easier to read.

By special casing the logic in your __LOW() macro, you're only asking
for trouble in the long run :)

thanks,

greg k-h

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Joe Thornber]

On Wed, Feb 26, 2003 at 10:14:54AM -0800, Greg KH wrote:
> By special casing the logic in your __LOW() macro, you're only asking
> for trouble in the long run :)

I think you're right, the __HIGH and __LOW macros are just obfuscating
things.  I'll fix in the next patchset.

Thanks,

- Joe

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Joe Thornber]

Greg,

Any happier with this ?  The second hunk of the patch may disappear at
some point.

- Joe


Replace __HIGH() and __LOW() with max() and min_not_zero().


--- diff/drivers/md/dm-table.c	2003-02-26 16:10:24.000000000 +0000
+++ source/drivers/md/dm-table.c	2003-02-27 09:44:31.000000000 +0000
@@ -78,22 +78,33 @@
 	return result;
 }
 
-#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
+/*
+ * Returns the minimum that is _not_ zero, unless both are zero.
+ */
+#define min_not_zero(l, r) (l == 0) ? r : ((r == 0) ? l : min(l, r))
 
 /*
  * Combine two io_restrictions, always taking the lower value.
  */
-
 static void combine_restrictions_low(struct io_restrictions *lhs,
 				     struct io_restrictions *rhs)
 {
-	__LOW(&lhs->max_sectors, rhs->max_sectors);
-	__LOW(&lhs->max_phys_segments, rhs->max_phys_segments);
-	__LOW(&lhs->max_hw_segments, rhs->max_hw_segments);
-	__HIGH(&lhs->hardsect_size, rhs->hardsect_size);
-	__LOW(&lhs->max_segment_size, rhs->max_segment_size);
-	__LOW(&lhs->seg_boundary_mask, rhs->seg_boundary_mask);
+	lhs->max_sectors =
+		min_not_zero(lhs->max_sectors, rhs->max_sectors);
+
+	lhs->max_phys_segments =
+		min_not_zero(lhs->max_phys_segments, rhs->max_phys_segments);
+
+	lhs->max_hw_segments =
+		min_not_zero(lhs->max_hw_segments, rhs->max_hw_segments);
+
+	lhs->hardsect_size = max(lhs->hardsect_size, rhs->hardsect_size);
+
+	lhs->max_segment_size =
+		min_not_zero(lhs->max_segment_size, rhs->max_segment_size);
+
+	lhs->seg_boundary_mask =
+		min_not_zero(lhs->seg_boundary_mask, rhs->seg_boundary_mask);
 }
 
 /*
@@ -486,13 +497,31 @@
 		request_queue_t *q = bdev_get_queue((*result)->bdev);
 		struct io_restrictions *rs = &ti->limits;
 
-		/* combine the device limits low */
-		__LOW(&rs->max_sectors, q->max_sectors);
-		__LOW(&rs->max_phys_segments, q->max_phys_segments);
-		__LOW(&rs->max_hw_segments, q->max_hw_segments);
-		__HIGH(&rs->hardsect_size, q->hardsect_size);
-		__LOW(&rs->max_segment_size, q->max_segment_size);
-		__LOW(&rs->seg_boundary_mask, q->seg_boundary_mask);
+		/*
+		 * Combine the device limits low.
+		 *
+		 * FIXME: if we move an io_restriction struct
+		 *        into q this would just be a call to
+		 *        combine_restrictions_low()
+		 */
+		rs->max_sectors =
+			min_not_zero(rs->max_sectors, q->max_sectors);
+
+		rs->max_phys_segments =
+			min_not_zero(rs->max_phys_segments,
+				     q->max_phys_segments);
+
+		rs->max_hw_segments =
+			min_not_zero(rs->max_hw_segments, q->max_hw_segments);
+
+		rs->hardsect_size = max(rs->hardsect_size, q->hardsect_size);
+
+		rs->max_segment_size =
+			min_not_zero(rs->max_segment_size, q->max_segment_size);
+
+		rs->seg_boundary_mask =
+			min_not_zero(rs->seg_boundary_mask,
+				     q->seg_boundary_mask);
 	}
 
 	return r;

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Horst von Brand]

Joe Thornber <joe@fib011235813.fsnet.co.uk> said:
> Any happier with this ?  The second hunk of the patch may disappear at
> some point.
> 
> - Joe
> 
> 
> Replace __HIGH() and __LOW() with max() and min_not_zero().
> 
> 
> --- diff/drivers/md/dm-table.c	2003-02-26 16:10:24.000000000 +0000
> +++ source/drivers/md/dm-table.c	2003-02-27 09:44:31.000000000 +0000
> @@ -78,22 +78,33 @@
>  	return result;
>  }
>  
> -#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
> +/*
> + * Returns the minimum that is _not_ zero, unless both are zero.
> + */
> +#define min_not_zero(l, r) (l == 0) ? r : ((r == 0) ? l : min(l, r))

I'd add () around r and l just for paranoia's sake. Plus make sure they
aren't ever going to be called with sideeffects... why not inline functions?

Besides, I'd call them args a and b (no real reason for l and r, and l is
almost 1 for some fonts...)
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Joe Thornber]

On Thu, Feb 27, 2003 at 01:17:05PM -0300, Horst von Brand wrote:
> I'd add () around r and l just for paranoia's sake. Plus make sure they
> aren't ever going to be called with sideeffects... why not inline functions?

because the types vary :(

> Besides, I'd call them args a and b (no real reason for l and r, and l is
> almost 1 for some fonts...)

l and r stood for left and right from when it was a compare and assign
operator.

- Joe

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Greg KH]

On Thu, Feb 27, 2003 at 09:55:22AM +0000, Joe Thornber wrote:
> Greg,
> 
> Any happier with this ?  The second hunk of the patch may disappear at
> some point.

Much nicer, thanks.

greg k-h

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Jakob Oestergaard]

On Wed, Feb 26, 2003 at 05:12:49PM +0000, Joe Thornber wrote:
> Another fix for the __LOW macro.
> 
> When dm_table and dm_target structures are initialized, the "limits" fields 
> (struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
> in dm-table.c). However, zero is not a useable value in these fields. The
> request queue will never let an I/O through, regardless of how small it might
> be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
> This change to the __LOW() macro sets these fields correctly when they are
> first initialized.  [Kevin Corry]
> 
> --- diff/drivers/md/dm-table.c	2003-02-26 16:10:02.000000000 +0000
> +++ source/drivers/md/dm-table.c	2003-02-26 16:10:19.000000000 +0000
> @@ -79,7 +79,7 @@
>  }
>  
>  #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
> +#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)

As someone else suggested, it would be good style to just use the
existing min() and max() macros.

Special-casing like the above is a recipe for disaster - having a
macro named "__LOW" which actually translates to  "min() unless left
argument is zero in which case we do a max() instead" is going to get
someone in trouble one day.

I'd say use "min()" and if there are places in the code where you want
min() unless that will be zero, then make that condition *explicit* at
those places in the code.

If you want a magic "usually_min_but_sometimes_max()" macro, then make
it's *name* reflect it's voodoo propeties.

Just my 0.02 Euro

-- 
................................................................
:   jakob@unthought.net   : And I see the elder races,         :
:.........................: putrid forms of man                :
:   Jakob Østergaard      : See him rise and claim the earth,  :
:        OZ9ABN           : his downfall is at hand.           :
:.........................:............{Konkhra}...............:

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Joe Thornber]

On Thu, Feb 27, 2003 at 10:34:42AM +0100, Jakob Oestergaard wrote:
> If you want a magic "usually_min_but_sometimes_max()" macro, then make
> it's *name* reflect it's voodoo propeties.

See the patch I just posted in response to Greg.

- Joe

Re: [PATCH 7/8] dm: __LOW macro fix no. 2

[Jakob Oestergaard]

On Thu, Feb 27, 2003 at 10:01:44AM +0000, Joe Thornber wrote:
> On Thu, Feb 27, 2003 at 10:34:42AM +0100, Jakob Oestergaard wrote:
> > If you want a magic "usually_min_but_sometimes_max()" macro, then make
> > it's *name* reflect it's voodoo propeties.
> 
> See the patch I just posted in response to Greg.

That makes me happy, for all it matters   ;)

Cheers,

-- 
................................................................
:   jakob@unthought.net   : And I see the elder races,         :
:.........................: putrid forms of man                :
:   Jakob Østergaard      : See him rise and claim the earth,  :
:        OZ9ABN           : his downfall is at hand.           :
:.........................:............{Konkhra}...............:

[PATCH 8/8] dm: return correct error codes from dm_table_add_target()

[Joe Thornber]

Return correct error codes from dm_table_add_target().  [Kevin Corry]

--- diff/drivers/md/dm-table.c	2003-02-26 16:10:19.000000000 +0000
+++ source/drivers/md/dm-table.c	2003-02-26 16:10:24.000000000 +0000
@@ -591,7 +591,7 @@
 	tgt->type = dm_get_target_type(type);
 	if (!tgt->type) {
 		tgt->error = "unknown target type";
-		return r;
+		return -EINVAL;
 	}
 
 	tgt->table = t;
@@ -604,6 +604,7 @@
 	 */
 	if (!adjoin(t, tgt)) {
 		tgt->error = "Gap in table";
+		r = -EINVAL;
 		goto bad;
 	}
 

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

On Friday 28 February 2003 08:32, you wrote:
> On Thu, 2003-02-27 at 14:05, Kevin Corry wrote:
> > Unfortunately, Linus seems to have committed that patch already. So here
> > is a patch to fix just that line.
> >
> > Thanks for catching that.
>
> Third time, strlen isn't necessary, it can be done at compile time.
>
> --- a/drivers/md/dm-ioctl.c     2003/02/27 16:29:58
> +++ b/drivers/md/dm-ioctl.c     2003/02/27 17:21:54
> @@ -174,7 +174,7 @@
>  static int register_with_devfs(struct hash_cell *hc)
>  {
>         struct gendisk *disk = dm_disk(hc->md);
> -       char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> +       char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR));
>         if (!name) {
>                 return -ENOMEM;
>         }

Sorry, I sent the last patch before I got your email.

Also, the "+1" is still necessary, even if we switch to sizeof. The sprintf 
call that follows copies DM_DIR, followed by a slash, followed by the name 
from the hash table into the allocated string. The "+1" is for the slash in 
the middle. The terminating NULL character is accounted for in DM_NAME_LEN.

Linus, here is (yet another!) patch against current BK.

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/


--- linux-2.5.63-bk4a/drivers/md/dm-ioctl.c	Fri Feb 28 08:43:19 2003
+++ linux-2.5.63-bk4b/drivers/md/dm-ioctl.c	Fri Feb 28 08:44:08 2003
@@ -174,7 +174,7 @@
 static int register_with_devfs(struct hash_cell *hc)
 {
 	struct gendisk *disk = dm_disk(hc->md);
-	char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
+	char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR) + 1, GFP_KERNEL);
 	if (!name) {
 		return -ENOMEM;
 	}

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Horst von Brand]

Kevin Corry <corryk@us.ibm.com> said:
> On Friday 28 February 2003 08:32, you wrote:
> > On Thu, 2003-02-27 at 14:05, Kevin Corry wrote:
> > > Unfortunately, Linus seems to have committed that patch already. So here
> > > is a patch to fix just that line.
> > >
> > > Thanks for catching that.
> >
> > Third time, strlen isn't necessary, it can be done at compile time.
> >
> > --- a/drivers/md/dm-ioctl.c     2003/02/27 16:29:58
> > +++ b/drivers/md/dm-ioctl.c     2003/02/27 17:21:54
> > @@ -174,7 +174,7 @@
> >  static int register_with_devfs(struct hash_cell *hc)
> >  {
> >         struct gendisk *disk = dm_disk(hc->md);
> > -       char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> > +       char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR));
> >         if (!name) {
> >                 return -ENOMEM;
> >         }
> 
> Sorry, I sent the last patch before I got your email.
> 
> Also, the "+1" is still necessary, even if we switch to sizeof. The sprintf 
> call that follows copies DM_DIR, followed by a slash, followed by the name 
> from the hash table into the allocated string. The "+1" is for the slash in 
> the middle. The terminating NULL character is accounted for in
> DM_NAME_LEN.

Then it was broken before.

sizeof("1234") == strlen("1234") + 1 == 5

Re: [PATCH 3/8] dm: prevent possible buffer overflow in ioctl interface

[Kevin Corry]

On Friday 28 February 2003 12:14, Horst von Brand wrote:
> Kevin Corry <corryk@us.ibm.com> said:
> > Also, the "+1" is still necessary, even if we switch to sizeof. The
> > sprintf call that follows copies DM_DIR, followed by a slash, followed by
> > the name from the hash table into the allocated string. The "+1" is for
> > the slash in the middle. The terminating NULL character is accounted for
> > in
> > DM_NAME_LEN.
>
> Then it was broken before.
>
> sizeof("1234") == strlen("1234") + 1 == 5

Hmmm...wasn't aware of that. I guess I never expected there to be a 
difference. If that's the case, then Joe Perches' earlier patch should do the 
trick, albiet for obscure reasons.

And I wouldn't say it was broken the other way; it was simply allocating one 
byte more than necessary.

-- 
Kevin Corry
corryk@us.ibm.com
http://evms.sourceforge.net/