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

[Mike Snitzer <snitzer@redhat.com>]

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