[dm-devel] [PATCH v2] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[dm-devel] [PATCH v2] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[Mikulas Patocka]

When LVM needs to find a device with a particular UUID it needs to ask for
UUID for each device. This patch returns UUID directly in the list of
devices, so that LVM doesn't have to query all the devices with an ioctl.
The UUID is returned if the flag DM_UUID_FLAG is set in the parameters.

Returning UUID is done in backward-compatible way. There's one unused
32-bit word value after the event number. This patch sets the bit
DM_NAME_LIST_FLAG_HAS_UUID if UUID is present and
DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID if it isn't (if none of these bits is
set, then we have an old kernel that doesn't support returning UUIDs). The
UUID is stored after this word. The 'next' value is updated to point after
the UUID, so that old version of libdevmapper will skip the UUID without
attempting to interpret it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-ioctl.c         |   20 +++++++++++++++++---
 include/uapi/linux/dm-ioctl.h |    7 +++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c	2021-03-09 21:04:07.000000000 +0100
+++ linux-2.6/drivers/md/dm-ioctl.c	2021-03-11 18:53:58.000000000 +0100
@@ -558,7 +558,9 @@ static int list_devices(struct file *fil
 	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
 		hc = container_of(n, struct hash_cell, name_node);
 		needed += align_val(offsetof(struct dm_name_list, name) + strlen(hc->name) + 1);
-		needed += align_val(sizeof(uint32_t));
+		needed += align_val(sizeof(uint32_t) * 2);
+		if (param->flags & DM_UUID_FLAG && hc->uuid)
+			needed += align_val(strlen(hc->uuid) + 1);
 	}
 
 	/*
@@ -577,6 +579,7 @@ static int list_devices(struct file *fil
 	 * Now loop through filling out the names.
 	 */
 	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
+		void *uuid_ptr;
 		hc = container_of(n, struct hash_cell, name_node);
 		if (old_nl)
 			old_nl->next = (uint32_t) ((void *) nl -
@@ -588,8 +591,19 @@ static int list_devices(struct file *fil
 
 		old_nl = nl;
 		event_nr = align_ptr(nl->name + strlen(hc->name) + 1);
-		*event_nr = dm_get_event_nr(hc->md);
-		nl = align_ptr(event_nr + 1);
+		event_nr[0] = dm_get_event_nr(hc->md);
+		event_nr[1] = 0;
+		uuid_ptr = align_ptr(event_nr + 2);
+		if (param->flags & DM_UUID_FLAG) {
+			if (hc->uuid) {
+				event_nr[1] |= DM_NAME_LIST_FLAG_HAS_UUID;
+				strcpy(uuid_ptr, hc->uuid);
+				uuid_ptr = align_ptr(uuid_ptr + strlen(hc->uuid) + 1);
+			} else {
+				event_nr[1] |= DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID;
+			}
+		}
+		nl = uuid_ptr;
 	}
 	/*
 	 * If mismatch happens, security may be compromised due to buffer
Index: linux-2.6/include/uapi/linux/dm-ioctl.h
===================================================================
--- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
+++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-11 18:42:14.000000000 +0100
@@ -193,8 +193,15 @@ struct dm_name_list {
 	__u32 next;		/* offset to the next record from
 				   the _start_ of this */
 	char name[0];
+
+	/* uint32_t event_nr; */
+	/* uint32_t flags; */
+	/* char uuid[0]; */
 };
 
+#define DM_NAME_LIST_FLAG_HAS_UUID		1
+#define DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID	2
+
 /*
  * Used to retrieve the target versions
  */

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[Mike Snitzer]

On Thu, Mar 11 2021 at  1:26pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> When LVM needs to find a device with a particular UUID it needs to ask for
> UUID for each device. This patch returns UUID directly in the list of
> devices, so that LVM doesn't have to query all the devices with an ioctl.
> The UUID is returned if the flag DM_UUID_FLAG is set in the parameters.
> 
> Returning UUID is done in backward-compatible way. There's one unused
> 32-bit word value after the event number. This patch sets the bit
> DM_NAME_LIST_FLAG_HAS_UUID if UUID is present and
> DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID if it isn't (if none of these bits is
> set, then we have an old kernel that doesn't support returning UUIDs). The
> UUID is stored after this word. The 'next' value is updated to point after
> the UUID, so that old version of libdevmapper will skip the UUID without
> attempting to interpret it.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-ioctl.c         |   20 +++++++++++++++++---
>  include/uapi/linux/dm-ioctl.h |    7 +++++++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/md/dm-ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-ioctl.c	2021-03-09 21:04:07.000000000 +0100
> +++ linux-2.6/drivers/md/dm-ioctl.c	2021-03-11 18:53:58.000000000 +0100
> @@ -558,7 +558,9 @@ static int list_devices(struct file *fil
>  	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
>  		hc = container_of(n, struct hash_cell, name_node);
>  		needed += align_val(offsetof(struct dm_name_list, name) + strlen(hc->name) + 1);
> -		needed += align_val(sizeof(uint32_t));
> +		needed += align_val(sizeof(uint32_t) * 2);
> +		if (param->flags & DM_UUID_FLAG && hc->uuid)
> +			needed += align_val(strlen(hc->uuid) + 1);
>  	}
>  
>  	/*
> @@ -577,6 +579,7 @@ static int list_devices(struct file *fil
>  	 * Now loop through filling out the names.
>  	 */
>  	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
> +		void *uuid_ptr;
>  		hc = container_of(n, struct hash_cell, name_node);
>  		if (old_nl)
>  			old_nl->next = (uint32_t) ((void *) nl -
> @@ -588,8 +591,19 @@ static int list_devices(struct file *fil
>  
>  		old_nl = nl;
>  		event_nr = align_ptr(nl->name + strlen(hc->name) + 1);
> -		*event_nr = dm_get_event_nr(hc->md);
> -		nl = align_ptr(event_nr + 1);
> +		event_nr[0] = dm_get_event_nr(hc->md);
> +		event_nr[1] = 0;
> +		uuid_ptr = align_ptr(event_nr + 2);
> +		if (param->flags & DM_UUID_FLAG) {
> +			if (hc->uuid) {
> +				event_nr[1] |= DM_NAME_LIST_FLAG_HAS_UUID;
> +				strcpy(uuid_ptr, hc->uuid);
> +				uuid_ptr = align_ptr(uuid_ptr + strlen(hc->uuid) + 1);
> +			} else {
> +				event_nr[1] |= DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID;
> +			}
> +		}
> +		nl = uuid_ptr;
>  	}
>  	/*
>  	 * If mismatch happens, security may be compromised due to buffer
> Index: linux-2.6/include/uapi/linux/dm-ioctl.h
> ===================================================================
> --- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
> +++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-11 18:42:14.000000000 +0100
> @@ -193,8 +193,15 @@ struct dm_name_list {
>  	__u32 next;		/* offset to the next record from
>  				   the _start_ of this */
>  	char name[0];
> +
> +	/* uint32_t event_nr; */
> +	/* uint32_t flags; */
> +	/* char uuid[0]; */
>  };

If extra padding is being leveraged here (from the __u32 next), why not
at least explicitly add the members and then pad out the balance of that
__u32?  I'm not liking the usage of phantom struct members.. e.g.
the games played with accessing them.

Mike

>  
> +#define DM_NAME_LIST_FLAG_HAS_UUID		1
> +#define DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID	2
> +
>  /*
>   * Used to retrieve the target versions
>   */

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[Mikulas Patocka]

On Thu, 11 Mar 2021, Mike Snitzer wrote:

> > Index: linux-2.6/include/uapi/linux/dm-ioctl.h
> > ===================================================================
> > --- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
> > +++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-11 18:42:14.000000000 +0100
> > @@ -193,8 +193,15 @@ struct dm_name_list {
> >  	__u32 next;		/* offset to the next record from
> >  				   the _start_ of this */
> >  	char name[0];
> > +
> > +	/* uint32_t event_nr; */
> > +	/* uint32_t flags; */
> > +	/* char uuid[0]; */
> >  };
> 
> If extra padding is being leveraged here (from the __u32 next), why not
> at least explicitly add the members and then pad out the balance of that
> __u32?  I'm not liking the usage of phantom struct members.. e.g.
> the games played with accessing them.
> 
> Mike

What exactly do you mean?

Do you want to create another structure that holds event_nr, flags and 
uuid? Or something else?

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[Mike Snitzer]

On Thu, Mar 11 2021 at  2:43pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 11 Mar 2021, Mike Snitzer wrote:
> 
> > > Index: linux-2.6/include/uapi/linux/dm-ioctl.h
> > > ===================================================================
> > > --- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
> > > +++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-11 18:42:14.000000000 +0100
> > > @@ -193,8 +193,15 @@ struct dm_name_list {
> > >  	__u32 next;		/* offset to the next record from
> > >  				   the _start_ of this */
> > >  	char name[0];
> > > +
> > > +	/* uint32_t event_nr; */
> > > +	/* uint32_t flags; */
> > > +	/* char uuid[0]; */
> > >  };
> > 
> > If extra padding is being leveraged here (from the __u32 next), why not
> > at least explicitly add the members and then pad out the balance of that
> > __u32?  I'm not liking the usage of phantom struct members.. e.g.
> > the games played with accessing them.
> > 
> > Mike
> 
> What exactly do you mean?
> 
> Do you want to create another structure that holds event_nr, flags and 
> uuid? Or something else?

Just not liking the comments you added in lieu of explicit struct
members.

Can't you remove __u32 next; and replace with named members of
appropriate size? Adding explicit padding to end to get you to 32bit
offset?  I'd need to look closer at the way the code is written, but I
just feel like this patch makes the code even more fiddley.

But I can let it go if you don't see a way forward to make it better..

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v3] dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

[Mikulas Patocka]

On Thu, 11 Mar 2021, Mike Snitzer wrote:

> On Thu, Mar 11 2021 at  2:43pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Thu, 11 Mar 2021, Mike Snitzer wrote:
> > 
> > > > Index: linux-2.6/include/uapi/linux/dm-ioctl.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
> > > > +++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-11 18:42:14.000000000 +0100
> > > > @@ -193,8 +193,15 @@ struct dm_name_list {
> > > >  	__u32 next;		/* offset to the next record from
> > > >  				   the _start_ of this */
> > > >  	char name[0];
> > > > +
> > > > +	/* uint32_t event_nr; */
> > > > +	/* uint32_t flags; */
> > > > +	/* char uuid[0]; */
> > > >  };
> > > 
> > > If extra padding is being leveraged here (from the __u32 next), why not
> > > at least explicitly add the members and then pad out the balance of that
> > > __u32?  I'm not liking the usage of phantom struct members.. e.g.
> > > the games played with accessing them.
> > > 
> > > Mike
> > 
> > What exactly do you mean?
> > 
> > Do you want to create another structure that holds event_nr, flags and 
> > uuid? Or something else?
> 
> Just not liking the comments you added in lieu of explicit struct
> members.
> 
> Can't you remove __u32 next; and replace with named members of
> appropriate size? Adding explicit padding to end to get you to 32bit
> offset?  I'd need to look closer at the way the code is written, but I
> just feel like this patch makes the code even more fiddley.
> 
> But I can let it go if you don't see a way forward to make it better..
> 
> Mike

Hi

Here I added a comment to "struct dm_name_list" that explains how to 
access fields "event_nr", "flags" and "uuid". Hopefully it makes the 
structure layout more understandable.

Mikulas


From: Mikulas Patocka <mpatocka@redhat.com>

dm-ioctl: return UUID in DM_LIST_DEVICES_CMD result

When LVM needs to find a device with a particular UUID it needs to ask for
UUID for each device. This patch returns UUID directly in the list of
devices, so that LVM doesn't have to query all the devices with an ioctl.
The UUID is returned if the flag DM_UUID_FLAG is set in the parameters.

Returning UUID is done in backward-compatible way. There's one unused
32-bit word value after the event number. This patch sets the bit
DM_NAME_LIST_FLAG_HAS_UUID if UUID is present and
DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID if it isn't (if none of these bits is
set, then we have an old kernel that doesn't support returning UUIDs). The
UUID is stored after this word. The 'next' value is updated to point after
the UUID, so that old version of libdevmapper will skip the UUID without
attempting to interpret it.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-ioctl.c         |   20 +++++++++++++++++---
 include/uapi/linux/dm-ioctl.h |   14 ++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c	2021-03-09 21:04:07.000000000 +0100
+++ linux-2.6/drivers/md/dm-ioctl.c	2021-03-12 15:02:42.000000000 +0100
@@ -558,7 +558,9 @@ static int list_devices(struct file *fil
 	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
 		hc = container_of(n, struct hash_cell, name_node);
 		needed += align_val(offsetof(struct dm_name_list, name) + strlen(hc->name) + 1);
-		needed += align_val(sizeof(uint32_t));
+		needed += align_val(sizeof(uint32_t) * 2);
+		if (param->flags & DM_UUID_FLAG && hc->uuid)
+			needed += align_val(strlen(hc->uuid) + 1);
 	}
 
 	/*
@@ -577,6 +579,7 @@ static int list_devices(struct file *fil
 	 * Now loop through filling out the names.
 	 */
 	for (n = rb_first(&name_rb_tree); n; n = rb_next(n)) {
+		void *uuid_ptr;
 		hc = container_of(n, struct hash_cell, name_node);
 		if (old_nl)
 			old_nl->next = (uint32_t) ((void *) nl -
@@ -588,8 +591,19 @@ static int list_devices(struct file *fil
 
 		old_nl = nl;
 		event_nr = align_ptr(nl->name + strlen(hc->name) + 1);
-		*event_nr = dm_get_event_nr(hc->md);
-		nl = align_ptr(event_nr + 1);
+		event_nr[0] = dm_get_event_nr(hc->md);
+		event_nr[1] = 0;
+		uuid_ptr = align_ptr(event_nr + 2);
+		if (param->flags & DM_UUID_FLAG) {
+			if (hc->uuid) {
+				event_nr[1] |= DM_NAME_LIST_FLAG_HAS_UUID;
+				strcpy(uuid_ptr, hc->uuid);
+				uuid_ptr = align_ptr(uuid_ptr + strlen(hc->uuid) + 1);
+			} else {
+				event_nr[1] |= DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID;
+			}
+		}
+		nl = uuid_ptr;
 	}
 	/*
 	 * If mismatch happens, security may be compromised due to buffer
Index: linux-2.6/include/uapi/linux/dm-ioctl.h
===================================================================
--- linux-2.6.orig/include/uapi/linux/dm-ioctl.h	2021-03-09 12:20:23.000000000 +0100
+++ linux-2.6/include/uapi/linux/dm-ioctl.h	2021-03-12 15:03:11.000000000 +0100
@@ -193,8 +193,22 @@ struct dm_name_list {
 	__u32 next;		/* offset to the next record from
 				   the _start_ of this */
 	char name[0];
+
+	/*
+	   The following members can be accessed by taking a pointer that points
+	   immediatelly after the terminating zero character in "name" and
+	   aligning this pointer to next 8-byte boundary.
+	   Uuid is present if the flag DM_NAME_LIST_FLAG_HAS_UUID is set.
+
+	   __u32 event_nr;
+	   __u32 flags;
+	   char uuid[0];
+	*/
 };
 
+#define DM_NAME_LIST_FLAG_HAS_UUID		1
+#define DM_NAME_LIST_FLAG_DOESNT_HAVE_UUID	2
+
 /*
  * Used to retrieve the target versions
  */

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel