[PATCH v2 0/4] Diskseq support in device-mapper

[PATCH v2 0/4] Diskseq support in device-mapper

[Demi Marie Obenour]

This work aims to allow userspace to create and destroy device-mapper
devices in a race-free way.

Changes since v1:

- Potentially backwards-incompatible changes to device-mapper now
  require userspace opt-in.
- The code has been tested: I have a block script written in C that uses
  these changes to successfully boot a Xen VM.
- The core block layer is completely untouched.  Instead of exposing a
  block device inode directly to userspace, device-mapper ioctls that
  create a block device now return that device's diskseq.  Userspace can
  then use that diskseq to safely open the device.  Furthermore, ioctls
  that operate on an existing device-mapper device now accept a diskseq
  parameter, which can be used to prevent races.

Demi Marie Obenour (4):
  dm ioctl: Allow userspace to opt-in to strict parameter checks
  dm ioctl: Allow userspace to provide expected diskseq
  dm ioctl: Allow userspace to suppress uevent generation
  dm ioctl: inform caller about already-existing device

 drivers/md/dm-core.h          |   2 +
 drivers/md/dm-ioctl.c         | 351 ++++++++++++++++++++++++++++------
 drivers/md/dm.c               |   5 +-
 include/linux/device-mapper.h |   2 +-
 include/uapi/linux/dm-ioctl.h |  90 ++++++++-
 5 files changed, 382 insertions(+), 68 deletions(-)

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v2 1/4] dm ioctl: Allow userspace to opt-in to strict parameter checks

[Demi Marie Obenour]

Currently, device-mapper ioctls ignore unknown flags.  This makes
adding new flags to a given ioctl risky, as it could potentially break
old userspace.

To solve this problem, allow userspace to pass 5 as the major version to
any ioctl.  This causes the kernel to reject any flags that are not
supported by the ioctl, as well as nonzero padding and names and UUIDs
that are not NUL-terminated.  New flags will only be recognized if major
version 5 is used.  Kernels without this patch return -EINVAL if the
major version is 5, so this is backwards compatible.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c         | 286 ++++++++++++++++++++++++++++------
 include/uapi/linux/dm-ioctl.h |  55 ++++++-
 2 files changed, 284 insertions(+), 57 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 16244a7b193c0ab868fe8b255ab22d9a6e7d01b0..e7693479c0cd974ddde69b3b1c4c67abc2ae3ad6 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -26,6 +26,10 @@
 
 #define DM_MSG_PREFIX "ioctl"
 #define DM_DRIVER_EMAIL "dm-devel@redhat.com"
+#define DM_CLEARED_FLAGS (DM_BUFFER_FULL_FLAG | DM_UEVENT_GENERATED_FLAG | DM_DATA_OUT_FLAG)
+#if DM_CLEARED_FLAGS & ~DM_STRICT_ONLY_FLAGS
+#error incorrect device-mapper flags
+#endif
 
 struct dm_file {
 	/*
@@ -1432,6 +1436,32 @@ static int next_target(struct dm_target_spec *last, uint32_t next, const char *e
 	return 0;
 }
 
+static inline bool sloppy_checks(const struct dm_ioctl *param)
+{
+	return param->version[0] < DM_VERSION_MAJOR_STRICT;
+}
+
+static bool no_non_nul_after_nul(const char *untrusted_str, size_t size,
+				 unsigned int cmd, const char *msg)
+{
+	const char *cursor;
+	const char *endp = untrusted_str + size;
+	const char *nul_terminator = memchr(untrusted_str, '\0', size);
+
+	if (nul_terminator == NULL) {
+		DMERR("%s not NUL-terminated, cmd(%u)", msg, cmd);
+		return false;
+	}
+	for (cursor = nul_terminator; cursor < endp; cursor++) {
+		if (*cursor != 0) {
+			DMERR("%s has non-NUL byte at %zd after NUL byte at %zd, cmd(%u)",
+			      msg, cursor - untrusted_str, nul_terminator - untrusted_str, cmd);
+			return false;
+		}
+	}
+	return true;
+}
+
 static int populate_table(struct dm_table *table,
 			  struct dm_ioctl *param, size_t param_size)
 {
@@ -1442,12 +1472,19 @@ static int populate_table(struct dm_table *table,
 	const char *const end = (const char *) param + param_size;
 	char *target_params;
 	size_t min_size = sizeof(struct dm_ioctl);
+	bool const strict = !sloppy_checks(param);
 
 	if (!param->target_count) {
 		DMERR("%s: no targets specified", __func__);
 		return -EINVAL;
 	}
 
+	if (strict && param_size % 8 != 0) {
+		DMERR("%s: parameter size %zu not multiple of 8",
+		      __func__, param_size);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < param->target_count; i++) {
 		const char *nul_terminator;
 
@@ -1472,6 +1509,18 @@ static int populate_table(struct dm_table *table,
 		/* Add 1 for NUL terminator */
 		min_size = (size_t)(nul_terminator - (const char *)spec) + 1;
 
+		if (strict) {
+			if (!no_non_nul_after_nul(spec->target_type, sizeof(spec->target_type),
+						  DM_TABLE_LOAD_CMD, "target type"))
+				return -EINVAL;
+
+			if (spec->status) {
+				DMERR("%s: status in target spec must be zero, not %u",
+				      __func__, spec->status);
+				return -EINVAL;
+			}
+		}
+
 		r = dm_table_add_target(table, spec->target_type,
 					(sector_t) spec->sector_start,
 					(sector_t) spec->length,
@@ -1482,6 +1531,32 @@ static int populate_table(struct dm_table *table,
 		}
 
 		next = spec->next;
+
+		if (strict) {
+			uint64_t zero = 0;
+			/*
+			 * param_size is a multiple of 8 so this is still in
+			 * bounds (or 1 past the end).
+			 */
+			size_t expected_next = round_up(min_size, 8);
+
+			if (expected_next != next) {
+				DMERR("%s: in strict mode, expected next to be %zu but it was %u",
+				      __func__, expected_next, next);
+				return -EINVAL;
+			}
+
+			if (memcmp(&zero, nul_terminator, next - min_size + 1) != 0) {
+				DMERR("%s: in strict mode, padding must be zeroed", __func__);
+				return -EINVAL;
+			}
+		}
+	}
+
+	if (strict && next != (size_t)(end - (const char *)spec)) {
+		DMERR("%s: last target size is %u, but %zd bytes remaining in target spec",
+		      __func__, next, end - (const char *)spec);
+		return -EINVAL;
 	}
 
 	return dm_table_complete(table);
@@ -1829,48 +1904,72 @@ static int target_message(struct file *filp, struct dm_ioctl *param, size_t para
  * the ioctl.
  */
 #define IOCTL_FLAGS_NO_PARAMS		1
-#define IOCTL_FLAGS_ISSUE_GLOBAL_EVENT	2
+#define IOCTL_FLAGS_TAKES_EVENT_NR      2
+#define IOCTL_FLAGS_ISSUE_GLOBAL_EVENT	(IOCTL_FLAGS_TAKES_EVENT_NR | 4)
 
 /*
  *---------------------------------------------------------------
  * Implementation of open/close/ioctl on the special char device.
  *---------------------------------------------------------------
  */
-static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
+static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags, uint32_t *supported_flags)
 {
 	static const struct {
 		int cmd;
 		int flags;
 		ioctl_fn fn;
+		uint32_t supported_flags;
 	} _ioctls[] = {
-		{DM_VERSION_CMD, 0, NULL}, /* version is dealt with elsewhere */
-		{DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, remove_all},
-		{DM_LIST_DEVICES_CMD, 0, list_devices},
+		/* Macro to make the structure initializers somewhat readable */
+#define I(cmd, flags, fn, supported_flags) ((typeof(_ioctls[0])) {				\
+	(cmd),											\
+	(flags),										\
+	(fn),											\
+	/*
+	 * Supported flags in sloppy mode must not include anything in DM_STRICT_ONLY_FLAGS.
+	 * Use BUILD_BUG_ON_ZERO to check for that.  Currently there are no strict-only flags
+	 * defined.
+	 */											\
+	(supported_flags) | BUILD_BUG_ON_ZERO((supported_flags) & DM_STRICT_ONLY_FLAGS),	\
+})
+		I(DM_VERSION_CMD, 0, NULL, 0), /* version is dealt with elsewhere */
+		I(DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
+		  remove_all, DM_DEFERRED_REMOVE),
+		I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG),
+		I(DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
+		  dev_create, DM_PERSISTENT_DEV_FLAG),
+		I(DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
+		  dev_remove, DM_DEFERRED_REMOVE),
+		I(DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename,
+		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG),
+		I(DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend,
+		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG | DM_SKIP_LOCKFS_FLAG |
+		 DM_NOFLUSH_FLAG),
+		I(DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status,
+		  DM_QUERY_INACTIVE_TABLE_FLAG),
+		I(DM_DEV_WAIT_CMD, IOCTL_FLAGS_TAKES_EVENT_NR, dev_wait,
+		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
+		I(DM_TABLE_LOAD_CMD, 0, table_load, DM_QUERY_INACTIVE_TABLE_FLAG |
+		  DM_READONLY_FLAG),
+		I(DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear,
+		  DM_QUERY_INACTIVE_TABLE_FLAG),
+		I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG),
+		I(DM_TABLE_STATUS_CMD, 0, table_status,
+		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
 
-		{DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_create},
-		{DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_remove},
-		{DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename},
-		{DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend},
-		{DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status},
-		{DM_DEV_WAIT_CMD, 0, dev_wait},
+		I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0),
 
-		{DM_TABLE_LOAD_CMD, 0, table_load},
-		{DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear},
-		{DM_TABLE_DEPS_CMD, 0, table_deps},
-		{DM_TABLE_STATUS_CMD, 0, table_status},
-
-		{DM_LIST_VERSIONS_CMD, 0, list_versions},
-
-		{DM_TARGET_MSG_CMD, 0, target_message},
-		{DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry},
-		{DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll},
-		{DM_GET_TARGET_VERSION_CMD, 0, get_target_version},
+		I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG),
+		I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0),
+		I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0),
+		I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0),
 	};
 
 	if (unlikely(cmd >= ARRAY_SIZE(_ioctls)))
 		return NULL;
 
 	cmd = array_index_nospec(cmd, ARRAY_SIZE(_ioctls));
+	*supported_flags = _ioctls[cmd].supported_flags;
 	*ioctl_flags = _ioctls[cmd].flags;
 	return _ioctls[cmd].fn;
 }
@@ -1883,6 +1982,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
 			 struct dm_ioctl *kernel_params)
 {
 	int r = 0;
+	uint32_t expected_major_version = DM_VERSION_MAJOR;
 
 	/* Make certain version is first member of dm_ioctl struct */
 	BUILD_BUG_ON(offsetof(struct dm_ioctl, version) != 0);
@@ -1890,10 +1990,14 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
 	if (copy_from_user(kernel_params->version, user->version, sizeof(kernel_params->version)))
 		return -EFAULT;
 
-	if ((kernel_params->version[0] != DM_VERSION_MAJOR) ||
+	if (kernel_params->version[0] >= DM_VERSION_MAJOR_STRICT)
+		expected_major_version = DM_VERSION_MAJOR_STRICT;
+
+	if ((kernel_params->version[0] != expected_major_version) ||
 	    (kernel_params->version[1] > DM_VERSION_MINOR)) {
 		DMERR("ioctl interface mismatch: kernel(%u.%u.%u), user(%u.%u.%u), cmd(%d)",
-		      DM_VERSION_MAJOR, DM_VERSION_MINOR,
+		      expected_major_version,
+		      DM_VERSION_MINOR,
 		      DM_VERSION_PATCHLEVEL,
 		      kernel_params->version[0],
 		      kernel_params->version[1],
@@ -1905,7 +2009,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
 	/*
 	 * Fill in the kernel version.
 	 */
-	kernel_params->version[0] = DM_VERSION_MAJOR;
+	kernel_params->version[0] = expected_major_version;
 	kernel_params->version[1] = DM_VERSION_MINOR;
 	kernel_params->version[2] = DM_VERSION_PATCHLEVEL;
 	if (copy_to_user(user->version, kernel_params->version, sizeof(kernel_params->version)))
@@ -1931,7 +2035,8 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 {
 	struct dm_ioctl *dmi;
 	int secure_data;
-	const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
+	const size_t minimum_data_size = sloppy_checks(param_kernel) ?
+		offsetof(struct dm_ioctl, data) : sizeof(struct dm_ioctl);
 	unsigned int noio_flag;
 
 	/* check_version() already copied version from userspace, avoid TOCTOU */
@@ -1941,12 +2046,13 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 		return -EFAULT;
 
 	if (param_kernel->data_size < minimum_data_size) {
-		DMERR("Invalid data size in the ioctl structure: %u",
-		      param_kernel->data_size);
+		DMERR("Invalid data size in the ioctl structure: %u (minimum %zu)",
+		      param_kernel->data_size, minimum_data_size);
 		return -EINVAL;
 	}
 
 	secure_data = param_kernel->flags & DM_SECURE_DATA_FLAG;
+	param_kernel->flags &= ~DM_SECURE_DATA_FLAG;
 
 	*param_flags = secure_data ? DM_WIPE_BUFFER : 0;
 
@@ -1977,7 +2083,8 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	/* Copy from param_kernel (which was already copied from user) */
 	memcpy(dmi, param_kernel, minimum_data_size);
 
-	if (copy_from_user(&dmi->data, (char __user *)user + minimum_data_size,
+	if (copy_from_user((char *)dmi + minimum_data_size,
+			   (char __user *)user + minimum_data_size,
 			   param_kernel->data_size - minimum_data_size))
 		goto bad;
 data_copied:
@@ -1994,33 +2101,99 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	return -EFAULT;
 }
 
-static int validate_params(uint cmd, struct dm_ioctl *param)
+static int validate_params(uint cmd, struct dm_ioctl *param,
+			   uint32_t ioctl_flags, uint32_t supported_flags)
 {
-	/* Always clear this flag */
-	param->flags &= ~DM_BUFFER_FULL_FLAG;
-	param->flags &= ~DM_UEVENT_GENERATED_FLAG;
-	param->flags &= ~DM_SECURE_DATA_FLAG;
-	param->flags &= ~DM_DATA_OUT_FLAG;
+	static_assert(__same_type(param->flags, supported_flags));
 
-	/* Ignores parameters */
-	if (cmd == DM_REMOVE_ALL_CMD ||
-	    cmd == DM_LIST_DEVICES_CMD ||
-	    cmd == DM_LIST_VERSIONS_CMD)
-		return 0;
+	if (sloppy_checks(param)) {
+		/* Clear flags other code expects to be cleared */
+		param->flags &= ~DM_CLEARED_FLAGS;
+
+		/* Ignore validation for certain commands */
+		if (cmd == DM_REMOVE_ALL_CMD ||
+		    cmd == DM_LIST_DEVICES_CMD ||
+		    cmd == DM_LIST_VERSIONS_CMD)
+			return 0;
+	}
 
 	if (cmd == DM_DEV_CREATE_CMD) {
 		if (!*param->name) {
 			DMERR("name not supplied when creating device");
 			return -EINVAL;
 		}
-	} else if (*param->uuid && *param->name) {
-		DMERR("only supply one of name or uuid, cmd(%u)", cmd);
+	} else {
+		if (*param->uuid && *param->name) {
+			DMERR("only supply one of name or uuid, cmd(%u)", cmd);
+			return -EINVAL;
+		}
+	}
+
+	if (sloppy_checks(param)) {
+		/* Ensure strings are terminated */
+		param->name[DM_NAME_LEN - 1] = '\0';
+		param->uuid[DM_UUID_LEN - 1] = '\0';
+		/* Mask off bits that could confuse other code */
+		param->flags &= ~DM_STRICT_ONLY_FLAGS;
+		/* Skip strict checks */
+		return 0;
+	}
+
+	/* Check that strings are terminated */
+	if (!no_non_nul_after_nul(param->name, DM_NAME_LEN, cmd, "Name") ||
+	    !no_non_nul_after_nul(param->uuid, DM_UUID_LEN, cmd, "UUID"))
+		return -EINVAL;
+
+	/*
+	 * This also checks the last byte of the UUID field, but that was
+	 * checked to be zero above.
+	 */
+	if (*(const u64 *)((const char *)param + (sizeof(*param) - 8)) != 0) {
+		DMERR("second padding field not zeroed in strict mode (cmd %u)", cmd);
 		return -EINVAL;
 	}
 
-	/* Ensure strings are terminated */
-	param->name[DM_NAME_LEN - 1] = '\0';
-	param->uuid[DM_UUID_LEN - 1] = '\0';
+	if ((param->flags & ~supported_flags) != 0) {
+		DMERR("unsupported flags 0x%x specified, cmd(%u)",
+		      param->flags & ~supported_flags, cmd);
+		return -EINVAL;
+	}
+
+	if (param->padding != 0) {
+		DMERR("padding not zeroed in strict mode (got %u, cmd %u)",
+		      param->padding, cmd);
+		return -EINVAL;
+	}
+
+	if (param->open_count != 0) {
+		DMERR("open_count not zeroed in strict mode (got %d, cmd %u)",
+		      param->open_count, cmd);
+		return -EINVAL;
+	}
+
+	if (param->event_nr != 0 && (ioctl_flags & IOCTL_FLAGS_TAKES_EVENT_NR) == 0) {
+		DMERR("Event number not zeroed for command that does not take one (got %u, cmd %u)",
+		      param->event_nr, cmd);
+		return -EINVAL;
+	}
+
+	if (ioctl_flags & IOCTL_FLAGS_NO_PARAMS) {
+		/* Ignores parameters */
+		if (param->data_size != sizeof(struct dm_ioctl)) {
+			DMERR("command %u must not have parameters", cmd);
+			return -EINVAL;
+		}
+
+		if (param->target_count != 0) {
+			DMERR("command %u must have zero target_count", cmd);
+			return -EINVAL;
+		}
+
+		if (param->data_start) {
+			DMERR("command %u must have zero data_start", cmd);
+			return -EINVAL;
+		}
+	}
 
 	return 0;
 }
@@ -2035,6 +2208,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	ioctl_fn fn = NULL;
 	size_t input_param_size;
 	struct dm_ioctl param_kernel;
+	uint32_t supported_flags, old_flags;
 
 	/* only root can play with this */
 	if (!capable(CAP_SYS_ADMIN))
@@ -2050,7 +2224,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	 * writes out the kernel's interface version.
 	 */
 	r = check_version(cmd, user, &param_kernel);
-	if (r)
+	if (r != 0)
 		return r;
 
 	/*
@@ -2059,7 +2233,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	if (cmd == DM_VERSION_CMD)
 		return 0;
 
-	fn = lookup_ioctl(cmd, &ioctl_flags);
+	fn = lookup_ioctl(cmd, &ioctl_flags, &supported_flags);
 	if (!fn) {
 		DMERR("dm_ctl_ioctl: unknown command 0x%x", command);
 		return -ENOTTY;
@@ -2074,11 +2248,22 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 		return r;
 
 	input_param_size = param->data_size;
-	r = validate_params(cmd, param);
+
+	/*
+	 * In sloppy mode, validate_params will clear some
+	 * flags to ensure other code does not get confused.
+	 * Save the original flags here, except for some that
+	 * have always been cleared.
+	 */
+	old_flags = param->flags & ~DM_CLEARED_FLAGS;
+	r = validate_params(cmd, param, ioctl_flags, supported_flags);
 	if (r)
 		goto out;
+	/* This XOR keeps only the flags validate_params has changed. */
+	old_flags ^= param->flags;
 
-	param->data_size = offsetof(struct dm_ioctl, data);
+	param->data_size = sloppy_checks(param) ?
+		offsetof(struct dm_ioctl, data) : sizeof(struct dm_ioctl);
 	r = fn(file, param, input_param_size);
 
 	if (unlikely(param->flags & DM_BUFFER_FULL_FLAG) &&
@@ -2088,6 +2273,9 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	if (!r && ioctl_flags & IOCTL_FLAGS_ISSUE_GLOBAL_EVENT)
 		dm_issue_global_event();
 
+	/* Resture the flags that validate_params cleared */
+	param->flags |= old_flags;
+
 	/*
 	 * Copy the results back to userland.
 	 */
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 1990b5700f6948243def314cec22f380926aca2e..62bfdc95ebccb2f1c20c24496a449fe3e2a76113 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -98,7 +98,11 @@
  * All ioctl arguments consist of a single chunk of memory, with
  * this structure at the start.  If a uuid is specified any
  * lookup (eg. for a DM_INFO) will be done on that, *not* the
- * name.
+ * name.  In strict mode, the name and UUID fields must be
+ * NUL-terminated and must not have a non-NUL byte following a
+ * NUL byte.  In sloppy mode, the last byte of these fields will
+ * be overwritten with NUL, and any bytes after the first NUL are
+ * ignored.
  */
 struct dm_ioctl {
 	/*
@@ -117,10 +121,16 @@ struct dm_ioctl {
 	 */
 	__u32 version[3];	/* in/out */
 	__u32 data_size;	/* total size of data passed in
-				 * including this struct */
+				 * including this struct, must be
+				 * multiple of 8 in strict mode
+				 */
 
 	__u32 data_start;	/* offset to start of data
-				 * relative to start of this struct */
+				 * relative to start of this struct,
+				 * must be at least offsetof(struct dm_ioctl, data)
+				 * in sloppy mode and at least sizeof(struct dm_ioctl)
+				 * in strict mode
+				 */
 
 	__u32 target_count;	/* in/out */
 	__s32 open_count;	/* out */
@@ -143,7 +153,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 */
+	char data[7];		/* padding or data, must be zero in strict mode */
 };
 
 /*
@@ -171,8 +181,11 @@ struct dm_target_spec {
 
 	/*
 	 * Parameter string starts immediately after this object.
-	 * Be careful to add padding after string to ensure correct
-	 * alignment of subsequent dm_target_spec.
+	 * Be careful to add padding after string to ensure 8-byte
+	 * alignment of subsequent dm_target_spec.  If the major version
+	 * is DM_VERSION_MAJOR_STRICT, the padding must be at most 7 bytes,
+	 * (not including the terminating NULt that ends the string) and
+	 * must be zeroed.
 	 */
 };
 
@@ -285,14 +298,28 @@ enum {
 #define DM_TARGET_MSG	 _IOWR(DM_IOCTL, DM_TARGET_MSG_CMD, struct dm_ioctl)
 #define DM_DEV_SET_GEOMETRY	_IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
 
+/* Legacy major version.  Activates "sloppy mode" in the parameter parser. */
 #define DM_VERSION_MAJOR	4
-#define DM_VERSION_MINOR	48
+/*
+ * New major version.  Enforces strict parameter checks and is required for
+ * using some new features, such as new flags.  Should be used by all new code.
+ * Activates "strict mode" in the parameter parser.
+ *
+ * If one uses DM_VERSION_MAJOR_STRICT, it is possible for the behavior of
+ * ioctls to depend on the minor version passed by userspace.  Userspace must
+ * not pass a minor version greater than the version it was designed for.
+ */
+#define DM_VERSION_MAJOR_STRICT 5
+#define DM_VERSION_MINOR	49
 #define DM_VERSION_PATCHLEVEL	0
 #define DM_VERSION_EXTRA	"-ioctl (2023-03-01)"
 
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */
 #define DM_SUSPEND_FLAG		(1 << 1) /* In/Out */
+#define DM_EXISTS_FLAG		(1 << 2) /* Not used by kernel, reserved for
+					  * libdevmapper in userland
+					  */
 #define DM_PERSISTENT_DEV_FLAG	(1 << 3) /* In */
 
 /*
@@ -315,7 +342,8 @@ enum {
 #define DM_BUFFER_FULL_FLAG	(1 << 8) /* Out */
 
 /*
- * This flag is now ignored.
+ * This flag is now ignored if DM_VERSION_MAJOR is used, and causes
+ * -EINVAL if DM_VERSION_MAJOR_STRICT is used.
  */
 #define DM_SKIP_BDGET_FLAG	(1 << 9) /* In */
 
@@ -382,4 +410,15 @@ enum {
  */
 #define DM_IMA_MEASUREMENT_FLAG	(1 << 19) /* In */
 
+/*
+ * If DM_VERSION_MAJOR is used, these flags are ignored by the kernel.
+ * If DM_VERSION_MAJOR_STRICT is used, these flags are reserved and
+ * must be zeroed.
+ */
+#define DM_STRICT_ONLY_FLAGS					\
+	(DM_ACTIVE_PRESENT_FLAG | DM_INACTIVE_PRESENT_FLAG |	\
+	 1 << 7 | DM_BUFFER_FULL_FLAG | DM_SKIP_BDGET_FLAG |	\
+	 DM_UEVENT_GENERATED_FLAG  | DM_DATA_OUT_FLAG |		\
+	 DM_INTERNAL_SUSPEND_FLAG | -(1 << 20))
+
 #endif				/* _LINUX_DM_IOCTL_H */
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH v2 2/4] dm ioctl: Allow userspace to provide expected diskseq

[Demi Marie Obenour]

This can be used to avoid race conditions in which a device is destroyed
and recreated with the same major/minor, name, or UUID.  diskseqs are
only honored if strict parameter checking is on, to avoid any risk of
breaking old userspace.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c         | 41 +++++++++++++++++++++++++++++------
 include/uapi/linux/dm-ioctl.h | 31 ++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index e7693479c0cd974ddde69b3b1c4c67abc2ae3ad6..7abaeec33f1884d4588e8563fb02e9ea1a6782c8 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -878,6 +878,9 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
 		}
 		dm_put_live_table(md, srcu_idx);
 	}
+
+	if (param->version[0] >= DM_VERSION_MAJOR_STRICT)
+		dm_set_diskseq(param, disk->diskseq);
 }
 
 static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_size)
@@ -918,6 +921,9 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
 static struct hash_cell *__find_device_hash_cell(struct dm_ioctl *param)
 {
 	struct hash_cell *hc = NULL;
+	static_assert(offsetof(struct dm_ioctl, diskseq_high) ==
+		      offsetof(struct dm_ioctl, data) + 3,
+		      "diskseq_high field misplaced");
 
 	if (*param->uuid) {
 		if (*param->name || param->dev) {
@@ -946,6 +952,27 @@ static struct hash_cell *__find_device_hash_cell(struct dm_ioctl *param)
 	} else
 		return NULL;
 
+	if (param->version[0] >= DM_VERSION_MAJOR_STRICT) {
+		u64 expected_diskseq = dm_get_diskseq(param);
+		u64 diskseq;
+		struct mapped_device *md = hc->md;
+
+		if (WARN_ON_ONCE(md->disk == NULL))
+			return NULL;
+		diskseq = md->disk->diskseq;
+		if (WARN_ON_ONCE(diskseq == 0))
+			return NULL;
+		if (expected_diskseq != 0) {
+			if (expected_diskseq != diskseq) {
+				DMERR("Diskseq mismatch: expected %llu actual %llu",
+				      expected_diskseq, diskseq);
+				return NULL;
+			}
+		} else {
+			dm_set_diskseq(param, diskseq);
+		}
+	}
+
 	/*
 	 * Sneakily write in both the name and the uuid
 	 * while we have the cell.
@@ -2139,6 +2166,12 @@ static int validate_params(uint cmd, struct dm_ioctl *param,
 		return 0;
 	}
 
+	if (param->data_size < sizeof(struct dm_ioctl)) {
+		DMERR("Entire struct dm_ioctl (size %zu) must be valid, but only %u was valid",
+		      sizeof(struct dm_ioctl), param->data_size);
+		return -EINVAL;
+	}
+
 	/* Check that strings are terminated */
 	if (!no_non_nul_after_nul(param->name, DM_NAME_LEN, cmd, "Name") ||
 	    !no_non_nul_after_nul(param->uuid, DM_UUID_LEN, cmd, "UUID"))
@@ -2148,7 +2181,7 @@ static int validate_params(uint cmd, struct dm_ioctl *param,
 	 * This also checks the last byte of the UUID field, but that was
 	 * checked to be zero above.
 	 */
-	if (*(const u64 *)((const char *)param + (sizeof(*param) - 8)) != 0) {
+	if (*(const u32 *)((const char *)param + (sizeof(*param) - 8)) != 0) {
 		DMERR("second padding field not zeroed in strict mode (cmd %u)", cmd);
 		return -EINVAL;
 	}
@@ -2159,12 +2192,6 @@ static int validate_params(uint cmd, struct dm_ioctl *param,
 		return -EINVAL;
 	}
 
-	if (param->padding != 0) {
-		DMERR("padding not zeroed in strict mode (got %u, cmd %u)",
-		      param->padding, cmd);
-		return -EINVAL;
-	}
-
 	if (param->open_count != 0) {
 		DMERR("open_count not zeroed in strict mode (got %d, cmd %u)",
 		      param->open_count, cmd);
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 62bfdc95ebccb2f1c20c24496a449fe3e2a76113..1d33109aece2ff9854e752066baa96fdf7d85857 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -146,16 +146,43 @@ struct dm_ioctl {
 	 * For output, the ioctls return the event number, not the cookie.
 	 */
 	__u32 event_nr;      	/* in/out */
-	__u32 padding;
+
+	union {
+		/* valid if DM_VERSION_MAJOR is used */
+		__u32 padding;		/* padding */
+		/* valid if DM_VERSION_MAJOR_STRICT is used */
+		__u32 diskseq_low;	/* in/out: low 4 bytes of the diskseq */
+	};
 
 	__u64 dev;		/* in/out */
 
 	char name[DM_NAME_LEN];	/* device name */
 	char uuid[DM_UUID_LEN];	/* unique identifier for
 				 * the block device */
-	char data[7];		/* padding or data, must be zero in strict mode */
+	union {
+		/* valid if DM_VERSION_MAJOR is used */
+		char data[7];	/* padding or data */
+		/* valid if DM_VERSION_MAJOR_STRICT is used */
+		struct {
+			char _padding[3];	/* padding, must be zeroed */
+			__u32 diskseq_high;	/* in/out: high 4 bytes of the diskseq */
+		} __attribute__((packed));
+	};
 };
 
+__attribute__((always_inline)) static inline __u64
+dm_get_diskseq(const struct dm_ioctl *_i)
+{
+	return (__u64)_i->diskseq_high << 32 | (__u64)_i->diskseq_low;
+}
+
+__attribute__((always_inline)) static inline void
+dm_set_diskseq(struct dm_ioctl *_i, __u64 _diskseq)
+{
+	_i->diskseq_low = (__u32)(_diskseq & 0xFFFFFFFFU);
+	_i->diskseq_high = (__u32)(_diskseq >> 32);
+}
+
 /*
  * Used to specify tables.  These structures appear after the
  * dm_ioctl.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v2 2/4] dm ioctl: Allow userspace to provide expected diskseq

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Sun, Jun 25, 2023 at 01:23:40PM +0200, Markus Elfring wrote:
> > This can be used to avoid race conditions in which a device is destroyed
> > and recreated with the same major/minor, name, or UUID. …
> 
> Please add an imperative change suggestion.

Will fix in v3.

> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4-rc7#n94
> 
> Regards,
> Markus

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] dm ioctl: Allow userspace to provide expected diskseq

[Dan Carpenter]

On Sun, Jun 25, 2023 at 01:39:40PM -0400, Demi Marie Obenour wrote:
> On Sun, Jun 25, 2023 at 01:23:40PM +0200, Markus Elfring wrote:
> > > This can be used to avoid race conditions in which a device is destroyed
> > > and recreated with the same major/minor, name, or UUID. …
> > 
> > Please add an imperative change suggestion.
> 
> Will fix in v3.

You don't have to listen to Markus.  Most of us can't see Markus's
emails because he's banned from the vger mailing lists.

Markus, stop bothering people about trivial nonsense.  I've said this
to you before, that if you spot a bug in a patch that's welcome feedback
but if you just have comments about grammar then no one wants that.

regards,
dan carpenter

Re: [v2 2/4] dm ioctl: Allow userspace to provide expected diskseq

[Dan Carpenter]

The reason why Markus was banned was because he doesn't listen to
feedback.  I've told him several times in the past week to stop
bothering people who are trying to work but he is not going to listen.

Meanwhile, I have seen Markus fix bugs so if he wanted to focus on
fixing bugs he probably could.

regards,
dan carpenter

Re: [PATCH v2 2/4] dm ioctl: Allow userspace to provide expected diskseq

[Dan Carpenter]

On Mon, Jun 26, 2023 at 06:20:14PM +0200, Markus Elfring wrote:
> > …, stop bothering people about trivial nonsense. …
> 
> See also another bit of background information once more:
> [PATCH v2] certs/extract-cert: Fix checkpatch issues
> 2023-06-09
> https://lore.kernel.org/kernel-janitors/c464c4ee-038c-47bf-857a-b11a89680e82@kadam.mountain/
> https://lkml.org/lkml/2023/6/9/879

Markus, it's not about imperative tense.  It's about you wasting
people's time.

Read the subject again.  "Allow userspace to provide expected diskseq".
That is imperative tense.  I have not pointed it out to you because it
just doesn't matter at all.  If it's in imperative tense or if it's not
in imperative tense, it doesn't matter.

You're sending out a lot of messages and quite a few times it looks like
your targeting newbies.  One new developer sent me an email privately
who was over the top grateful when I told him he could ignore you.  The
guy was like, "I was so puzzled, because it's my first patch and I
didn't know how to respond."  This was an experienced programmer who we
want, but he was new to the kernel community so he didn't know if we had
bizarre rules or whatever.

I've looked through your patches that have recently been merged.  Some
of those maintainers know that you are banned and that your patches are
not getting any review from the mailing list.  We are really trying to
be nice and to work around your situation.  But don't start bothering
newbies who don't know what the situation is.

regards,
dan carpenter

[PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Demi Marie Obenour]

Userspace can use this to avoid spamming udev with events that udev
should ignore.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-core.h          |  2 +
 drivers/md/dm-ioctl.c         | 71 ++++++++++++++++++-----------------
 drivers/md/dm.c               |  5 ++-
 include/linux/device-mapper.h |  2 +-
 include/uapi/linux/dm-ioctl.h | 14 +++++--
 5 files changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 0d93661f88d306e0d0aa3a1ac085880a8b63d9d6..5a4acdbf119f9b3f9a1c60de36d23f0658449701 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -115,6 +115,8 @@ struct mapped_device {
 
 	/* for blk-mq request-based DM support */
 	bool init_tio_pdu:1;
+	/* If set, do not emit any uevents. */
+	bool disable_uevents:1;
 	struct blk_mq_tag_set *tag_set;
 
 	struct dm_stats stats;
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 7abaeec33f1884d4588e8563fb02e9ea1a6782c8..7eab9a8c77ff3286346a1c44581d3b6370a731eb 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -821,6 +821,11 @@ static struct dm_table *dm_get_live_or_inactive_table(struct mapped_device *md,
 		dm_get_inactive_table(md, srcu_idx) : dm_get_live_table(md, srcu_idx);
 }
 
+static inline bool sloppy_checks(const struct dm_ioctl *param)
+{
+	return param->version[0] < DM_VERSION_MAJOR_STRICT;
+}
+
 /*
  * Fills in a dm_ioctl structure, ready for sending back to
  * userland.
@@ -879,7 +884,7 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
 		dm_put_live_table(md, srcu_idx);
 	}
 
-	if (param->version[0] >= DM_VERSION_MAJOR_STRICT)
+	if (!sloppy_checks(param))
 		dm_set_diskseq(param, disk->diskseq);
 }
 
@@ -895,7 +900,7 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
 	if (param->flags & DM_PERSISTENT_DEV_FLAG)
 		m = MINOR(huge_decode_dev(param->dev));
 
-	r = dm_create(m, &md);
+	r = dm_create(m, &md, param->flags & DM_DISABLE_UEVENTS_FLAG);
 	if (r)
 		return r;
 
@@ -1463,11 +1468,6 @@ static int next_target(struct dm_target_spec *last, uint32_t next, const char *e
 	return 0;
 }
 
-static inline bool sloppy_checks(const struct dm_ioctl *param)
-{
-	return param->version[0] < DM_VERSION_MAJOR_STRICT;
-}
-
 static bool no_non_nul_after_nul(const char *untrusted_str, size_t size,
 				 unsigned int cmd, const char *msg)
 {
@@ -1939,64 +1939,67 @@ static int target_message(struct file *filp, struct dm_ioctl *param, size_t para
  * Implementation of open/close/ioctl on the special char device.
  *---------------------------------------------------------------
  */
-static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags, uint32_t *supported_flags)
+static ioctl_fn lookup_ioctl(unsigned int cmd, bool strict, int *ioctl_flags,
+			     uint32_t *supported_flags)
 {
 	static const struct {
 		int cmd;
 		int flags;
 		ioctl_fn fn;
 		uint32_t supported_flags;
+		uint32_t strict_flags;
 	} _ioctls[] = {
 		/* Macro to make the structure initializers somewhat readable */
-#define I(cmd, flags, fn, supported_flags) ((typeof(_ioctls[0])) {				\
+#define I(cmd, flags, fn, supported_flags, strict_flags) ((typeof(_ioctls[0])) {		\
 	(cmd),											\
 	(flags),										\
 	(fn),											\
 	/*
 	 * Supported flags in sloppy mode must not include anything in DM_STRICT_ONLY_FLAGS.
-	 * Use BUILD_BUG_ON_ZERO to check for that.  Currently there are no strict-only flags
-	 * defined.
+	 * Use BUILD_BUG_ON_ZERO to check for that.
 	 */											\
 	(supported_flags) | BUILD_BUG_ON_ZERO((supported_flags) & DM_STRICT_ONLY_FLAGS),	\
+	(strict_flags) | (supported_flags) |							\
+	 BUILD_BUG_ON_ZERO((supported_flags) & (strict_flags)),					\
 })
-		I(DM_VERSION_CMD, 0, NULL, 0), /* version is dealt with elsewhere */
+		I(DM_VERSION_CMD, 0, NULL, 0, 0), /* version is dealt with elsewhere */
 		I(DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
-		  remove_all, DM_DEFERRED_REMOVE),
-		I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG),
+		  remove_all, DM_DEFERRED_REMOVE, 0),
+		I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG, 0),
 		I(DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
-		  dev_create, DM_PERSISTENT_DEV_FLAG),
+		  dev_create, DM_PERSISTENT_DEV_FLAG, DM_DISABLE_UEVENTS_FLAG),
 		I(DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
-		  dev_remove, DM_DEFERRED_REMOVE),
+		  dev_remove, DM_DEFERRED_REMOVE, 0),
 		I(DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename,
-		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG),
+		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG, 0),
 		I(DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend,
-		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG | DM_SKIP_LOCKFS_FLAG |
-		 DM_NOFLUSH_FLAG),
+		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG |
+		  DM_SKIP_LOCKFS_FLAG | DM_NOFLUSH_FLAG, 0),
 		I(DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status,
-		  DM_QUERY_INACTIVE_TABLE_FLAG),
+		  DM_QUERY_INACTIVE_TABLE_FLAG, 0),
 		I(DM_DEV_WAIT_CMD, IOCTL_FLAGS_TAKES_EVENT_NR, dev_wait,
-		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
-		I(DM_TABLE_LOAD_CMD, 0, table_load, DM_QUERY_INACTIVE_TABLE_FLAG |
-		  DM_READONLY_FLAG),
+		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG, 0),
+		I(DM_TABLE_LOAD_CMD, 0, table_load,
+		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_READONLY_FLAG, 0),
 		I(DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear,
-		  DM_QUERY_INACTIVE_TABLE_FLAG),
-		I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG),
+		  DM_QUERY_INACTIVE_TABLE_FLAG, 0),
+		I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG, 0),
 		I(DM_TABLE_STATUS_CMD, 0, table_status,
-		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
+		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG, 0),
 
-		I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0),
+		I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0, 0),
 
-		I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG),
-		I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0),
-		I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0),
-		I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0),
+		I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG, 0),
+		I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0, 0),
+		I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0, 0),
+		I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0, 0),
 	};
 
 	if (unlikely(cmd >= ARRAY_SIZE(_ioctls)))
 		return NULL;
 
 	cmd = array_index_nospec(cmd, ARRAY_SIZE(_ioctls));
-	*supported_flags = _ioctls[cmd].supported_flags;
+	*supported_flags = strict ? _ioctls[cmd].strict_flags : _ioctls[cmd].supported_flags;
 	*ioctl_flags = _ioctls[cmd].flags;
 	return _ioctls[cmd].fn;
 }
@@ -2260,7 +2263,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	if (cmd == DM_VERSION_CMD)
 		return 0;
 
-	fn = lookup_ioctl(cmd, &ioctl_flags, &supported_flags);
+	fn = lookup_ioctl(cmd, !sloppy_checks(&param_kernel), &ioctl_flags, &supported_flags);
 	if (!fn) {
 		DMERR("dm_ctl_ioctl: unknown command 0x%x", command);
 		return -ENOTTY;
@@ -2480,7 +2483,7 @@ int __init dm_early_create(struct dm_ioctl *dmi,
 		m = MINOR(huge_decode_dev(dmi->dev));
 
 	/* alloc dm device */
-	r = dm_create(m, &md);
+	r = dm_create(m, &md, false);
 	if (r)
 		return r;
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ea1671c39ba131ab2e49b93289d1094cda5cfb25..b7817b4aea52033afeeea9f2b93b9215de682e9c 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2281,13 +2281,14 @@ static struct dm_table *__unbind(struct mapped_device *md)
 /*
  * Constructor for a new device.
  */
-int dm_create(int minor, struct mapped_device **result)
+int dm_create(int minor, struct mapped_device **result, bool disable_uevents)
 {
 	struct mapped_device *md;
 
 	md = alloc_dev(minor);
 	if (!md)
 		return -ENXIO;
+	md->disable_uevents = disable_uevents;
 
 	dm_ima_reset_data(md);
 
@@ -3005,6 +3006,8 @@ int dm_kobject_uevent(struct mapped_device *md, enum kobject_action action,
 	char udev_cookie[DM_COOKIE_LENGTH];
 	char *envp[3] = { NULL, NULL, NULL };
 	char **envpp = envp;
+	if (md->disable_uevents)
+		return 0;
 	if (cookie) {
 		snprintf(udev_cookie, DM_COOKIE_LENGTH, "%s=%u",
 			 DM_COOKIE_ENV_VAR_NAME, cookie);
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 69d0435c7ebb0d7b712e2b8bf32d9ba34c54e09e..2be940c2c6f42ed8e13b97ea8b07d0895566f3e2 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -461,7 +461,7 @@ void dm_consume_args(struct dm_arg_set *as, unsigned int num_args);
  * DM_ANY_MINOR chooses the next available minor number.
  */
 #define DM_ANY_MINOR (-1)
-int dm_create(int minor, struct mapped_device **md);
+int dm_create(int minor, struct mapped_device **md, bool disable_uevents);
 
 /*
  * Reference counting for md.
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 1d33109aece2ff9854e752066baa96fdf7d85857..b338a4f9a299acda9430995d63fbb490e70c8cd8 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -369,8 +369,16 @@ enum {
 #define DM_BUFFER_FULL_FLAG	(1 << 8) /* Out */
 
 /*
- * This flag is now ignored if DM_VERSION_MAJOR is used, and causes
- * -EINVAL if DM_VERSION_MAJOR_STRICT is used.
+ * This flag is only recognized when DM_VERSION_MAJOR_STRICT is used.
+ * It tells the kernel to not generate any uevents for the newly-created
+ * device.  Using it outside of DM_DEV_CREATE results in -EINVAL.  When
+ * DM_VERSION_MAJOR is used this flag is ignored.
+ */
+#define DM_DISABLE_UEVENTS_FLAG	(1 << 9) /* In */
+
+/*
+ * This flag is now ignored if DM_VERSION_MAJOR is used.  When
+ * DM_VERSION_MAJOR_STRICT is used it is an alias for DM_DISABLE_UEVENT_FLAG.
  */
 #define DM_SKIP_BDGET_FLAG	(1 << 9) /* In */
 
@@ -439,8 +447,6 @@ enum {
 
 /*
  * If DM_VERSION_MAJOR is used, these flags are ignored by the kernel.
- * If DM_VERSION_MAJOR_STRICT is used, these flags are reserved and
- * must be zeroed.
  */
 #define DM_STRICT_ONLY_FLAGS					\
 	(DM_ACTIVE_PRESENT_FLAG | DM_INACTIVE_PRESENT_FLAG |	\
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [dm-devel] [PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Milan Broz]

On 6/25/23 01:09, Demi Marie Obenour wrote:
> Userspace can use this to avoid spamming udev with events that udev
> should ignore.

Well, does it also mean that udev will not create /dev/disk/by-* symlinks
(as response to the change udev event followed by internal udev blkid scan)?

If it is a private device, that is ok. But for a visible device I think
that it breaks some assumptions in userspace (presence of symlinks mentioned
above etc).

So, what is the exact use for this patch?

Milan



> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>   drivers/md/dm-core.h          |  2 +
>   drivers/md/dm-ioctl.c         | 71 ++++++++++++++++++-----------------
>   drivers/md/dm.c               |  5 ++-
>   include/linux/device-mapper.h |  2 +-
>   include/uapi/linux/dm-ioctl.h | 14 +++++--
>   5 files changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 0d93661f88d306e0d0aa3a1ac085880a8b63d9d6..5a4acdbf119f9b3f9a1c60de36d23f0658449701 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -115,6 +115,8 @@ struct mapped_device {
>   
>   	/* for blk-mq request-based DM support */
>   	bool init_tio_pdu:1;
> +	/* If set, do not emit any uevents. */
> +	bool disable_uevents:1;
>   	struct blk_mq_tag_set *tag_set;
>   
>   	struct dm_stats stats;
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 7abaeec33f1884d4588e8563fb02e9ea1a6782c8..7eab9a8c77ff3286346a1c44581d3b6370a731eb 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -821,6 +821,11 @@ static struct dm_table *dm_get_live_or_inactive_table(struct mapped_device *md,
>   		dm_get_inactive_table(md, srcu_idx) : dm_get_live_table(md, srcu_idx);
>   }
>   
> +static inline bool sloppy_checks(const struct dm_ioctl *param)
> +{
> +	return param->version[0] < DM_VERSION_MAJOR_STRICT;
> +}
> +
>   /*
>    * Fills in a dm_ioctl structure, ready for sending back to
>    * userland.
> @@ -879,7 +884,7 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
>   		dm_put_live_table(md, srcu_idx);
>   	}
>   
> -	if (param->version[0] >= DM_VERSION_MAJOR_STRICT)
> +	if (!sloppy_checks(param))
>   		dm_set_diskseq(param, disk->diskseq);
>   }
>   
> @@ -895,7 +900,7 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
>   	if (param->flags & DM_PERSISTENT_DEV_FLAG)
>   		m = MINOR(huge_decode_dev(param->dev));
>   
> -	r = dm_create(m, &md);
> +	r = dm_create(m, &md, param->flags & DM_DISABLE_UEVENTS_FLAG);
>   	if (r)
>   		return r;
>   
> @@ -1463,11 +1468,6 @@ static int next_target(struct dm_target_spec *last, uint32_t next, const char *e
>   	return 0;
>   }
>   
> -static inline bool sloppy_checks(const struct dm_ioctl *param)
> -{
> -	return param->version[0] < DM_VERSION_MAJOR_STRICT;
> -}
> -
>   static bool no_non_nul_after_nul(const char *untrusted_str, size_t size,
>   				 unsigned int cmd, const char *msg)
>   {
> @@ -1939,64 +1939,67 @@ static int target_message(struct file *filp, struct dm_ioctl *param, size_t para
>    * Implementation of open/close/ioctl on the special char device.
>    *---------------------------------------------------------------
>    */
> -static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags, uint32_t *supported_flags)
> +static ioctl_fn lookup_ioctl(unsigned int cmd, bool strict, int *ioctl_flags,
> +			     uint32_t *supported_flags)
>   {
>   	static const struct {
>   		int cmd;
>   		int flags;
>   		ioctl_fn fn;
>   		uint32_t supported_flags;
> +		uint32_t strict_flags;
>   	} _ioctls[] = {
>   		/* Macro to make the structure initializers somewhat readable */
> -#define I(cmd, flags, fn, supported_flags) ((typeof(_ioctls[0])) {				\
> +#define I(cmd, flags, fn, supported_flags, strict_flags) ((typeof(_ioctls[0])) {		\
>   	(cmd),											\
>   	(flags),										\
>   	(fn),											\
>   	/*
>   	 * Supported flags in sloppy mode must not include anything in DM_STRICT_ONLY_FLAGS.
> -	 * Use BUILD_BUG_ON_ZERO to check for that.  Currently there are no strict-only flags
> -	 * defined.
> +	 * Use BUILD_BUG_ON_ZERO to check for that.
>   	 */											\
>   	(supported_flags) | BUILD_BUG_ON_ZERO((supported_flags) & DM_STRICT_ONLY_FLAGS),	\
> +	(strict_flags) | (supported_flags) |							\
> +	 BUILD_BUG_ON_ZERO((supported_flags) & (strict_flags)),					\
>   })
> -		I(DM_VERSION_CMD, 0, NULL, 0), /* version is dealt with elsewhere */
> +		I(DM_VERSION_CMD, 0, NULL, 0, 0), /* version is dealt with elsewhere */
>   		I(DM_REMOVE_ALL_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
> -		  remove_all, DM_DEFERRED_REMOVE),
> -		I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG),
> +		  remove_all, DM_DEFERRED_REMOVE, 0),
> +		I(DM_LIST_DEVICES_CMD, 0, list_devices, DM_UUID_FLAG, 0),
>   		I(DM_DEV_CREATE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
> -		  dev_create, DM_PERSISTENT_DEV_FLAG),
> +		  dev_create, DM_PERSISTENT_DEV_FLAG, DM_DISABLE_UEVENTS_FLAG),
>   		I(DM_DEV_REMOVE_CMD, IOCTL_FLAGS_NO_PARAMS | IOCTL_FLAGS_ISSUE_GLOBAL_EVENT,
> -		  dev_remove, DM_DEFERRED_REMOVE),
> +		  dev_remove, DM_DEFERRED_REMOVE, 0),
>   		I(DM_DEV_RENAME_CMD, IOCTL_FLAGS_ISSUE_GLOBAL_EVENT, dev_rename,
> -		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG),
> +		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_UUID_FLAG, 0),
>   		I(DM_DEV_SUSPEND_CMD, IOCTL_FLAGS_NO_PARAMS, dev_suspend,
> -		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG | DM_SKIP_LOCKFS_FLAG |
> -		 DM_NOFLUSH_FLAG),
> +		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_SUSPEND_FLAG |
> +		  DM_SKIP_LOCKFS_FLAG | DM_NOFLUSH_FLAG, 0),
>   		I(DM_DEV_STATUS_CMD, IOCTL_FLAGS_NO_PARAMS, dev_status,
> -		  DM_QUERY_INACTIVE_TABLE_FLAG),
> +		  DM_QUERY_INACTIVE_TABLE_FLAG, 0),
>   		I(DM_DEV_WAIT_CMD, IOCTL_FLAGS_TAKES_EVENT_NR, dev_wait,
> -		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
> -		I(DM_TABLE_LOAD_CMD, 0, table_load, DM_QUERY_INACTIVE_TABLE_FLAG |
> -		  DM_READONLY_FLAG),
> +		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG, 0),
> +		I(DM_TABLE_LOAD_CMD, 0, table_load,
> +		  DM_QUERY_INACTIVE_TABLE_FLAG | DM_READONLY_FLAG, 0),
>   		I(DM_TABLE_CLEAR_CMD, IOCTL_FLAGS_NO_PARAMS, table_clear,
> -		  DM_QUERY_INACTIVE_TABLE_FLAG),
> -		I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG),
> +		  DM_QUERY_INACTIVE_TABLE_FLAG, 0),
> +		I(DM_TABLE_DEPS_CMD, 0, table_deps, DM_QUERY_INACTIVE_TABLE_FLAG, 0),
>   		I(DM_TABLE_STATUS_CMD, 0, table_status,
> -		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG),
> +		 DM_QUERY_INACTIVE_TABLE_FLAG | DM_STATUS_TABLE_FLAG | DM_NOFLUSH_FLAG, 0),
>   
> -		I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0),
> +		I(DM_LIST_VERSIONS_CMD, 0, list_versions, 0, 0),
>   
> -		I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG),
> -		I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0),
> -		I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0),
> -		I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0),
> +		I(DM_TARGET_MSG_CMD, 0, target_message, DM_QUERY_INACTIVE_TABLE_FLAG, 0),
> +		I(DM_DEV_SET_GEOMETRY_CMD, 0, dev_set_geometry, 0, 0),
> +		I(DM_DEV_ARM_POLL_CMD, IOCTL_FLAGS_NO_PARAMS, dev_arm_poll, 0, 0),
> +		I(DM_GET_TARGET_VERSION_CMD, 0, get_target_version, 0, 0),
>   	};
>   
>   	if (unlikely(cmd >= ARRAY_SIZE(_ioctls)))
>   		return NULL;
>   
>   	cmd = array_index_nospec(cmd, ARRAY_SIZE(_ioctls));
> -	*supported_flags = _ioctls[cmd].supported_flags;
> +	*supported_flags = strict ? _ioctls[cmd].strict_flags : _ioctls[cmd].supported_flags;
>   	*ioctl_flags = _ioctls[cmd].flags;
>   	return _ioctls[cmd].fn;
>   }
> @@ -2260,7 +2263,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
>   	if (cmd == DM_VERSION_CMD)
>   		return 0;
>   
> -	fn = lookup_ioctl(cmd, &ioctl_flags, &supported_flags);
> +	fn = lookup_ioctl(cmd, !sloppy_checks(&param_kernel), &ioctl_flags, &supported_flags);
>   	if (!fn) {
>   		DMERR("dm_ctl_ioctl: unknown command 0x%x", command);
>   		return -ENOTTY;
> @@ -2480,7 +2483,7 @@ int __init dm_early_create(struct dm_ioctl *dmi,
>   		m = MINOR(huge_decode_dev(dmi->dev));
>   
>   	/* alloc dm device */
> -	r = dm_create(m, &md);
> +	r = dm_create(m, &md, false);
>   	if (r)
>   		return r;
>   
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index ea1671c39ba131ab2e49b93289d1094cda5cfb25..b7817b4aea52033afeeea9f2b93b9215de682e9c 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2281,13 +2281,14 @@ static struct dm_table *__unbind(struct mapped_device *md)
>   /*
>    * Constructor for a new device.
>    */
> -int dm_create(int minor, struct mapped_device **result)
> +int dm_create(int minor, struct mapped_device **result, bool disable_uevents)
>   {
>   	struct mapped_device *md;
>   
>   	md = alloc_dev(minor);
>   	if (!md)
>   		return -ENXIO;
> +	md->disable_uevents = disable_uevents;
>   
>   	dm_ima_reset_data(md);
>   
> @@ -3005,6 +3006,8 @@ int dm_kobject_uevent(struct mapped_device *md, enum kobject_action action,
>   	char udev_cookie[DM_COOKIE_LENGTH];
>   	char *envp[3] = { NULL, NULL, NULL };
>   	char **envpp = envp;
> +	if (md->disable_uevents)
> +		return 0;
>   	if (cookie) {
>   		snprintf(udev_cookie, DM_COOKIE_LENGTH, "%s=%u",
>   			 DM_COOKIE_ENV_VAR_NAME, cookie);
> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> index 69d0435c7ebb0d7b712e2b8bf32d9ba34c54e09e..2be940c2c6f42ed8e13b97ea8b07d0895566f3e2 100644
> --- a/include/linux/device-mapper.h
> +++ b/include/linux/device-mapper.h
> @@ -461,7 +461,7 @@ void dm_consume_args(struct dm_arg_set *as, unsigned int num_args);
>    * DM_ANY_MINOR chooses the next available minor number.
>    */
>   #define DM_ANY_MINOR (-1)
> -int dm_create(int minor, struct mapped_device **md);
> +int dm_create(int minor, struct mapped_device **md, bool disable_uevents);
>   
>   /*
>    * Reference counting for md.
> diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
> index 1d33109aece2ff9854e752066baa96fdf7d85857..b338a4f9a299acda9430995d63fbb490e70c8cd8 100644
> --- a/include/uapi/linux/dm-ioctl.h
> +++ b/include/uapi/linux/dm-ioctl.h
> @@ -369,8 +369,16 @@ enum {
>   #define DM_BUFFER_FULL_FLAG	(1 << 8) /* Out */
>   
>   /*
> - * This flag is now ignored if DM_VERSION_MAJOR is used, and causes
> - * -EINVAL if DM_VERSION_MAJOR_STRICT is used.
> + * This flag is only recognized when DM_VERSION_MAJOR_STRICT is used.
> + * It tells the kernel to not generate any uevents for the newly-created
> + * device.  Using it outside of DM_DEV_CREATE results in -EINVAL.  When
> + * DM_VERSION_MAJOR is used this flag is ignored.
> + */
> +#define DM_DISABLE_UEVENTS_FLAG	(1 << 9) /* In */
> +
> +/*
> + * This flag is now ignored if DM_VERSION_MAJOR is used.  When
> + * DM_VERSION_MAJOR_STRICT is used it is an alias for DM_DISABLE_UEVENT_FLAG.
>    */
>   #define DM_SKIP_BDGET_FLAG	(1 << 9) /* In */
>   
> @@ -439,8 +447,6 @@ enum {
>   
>   /*
>    * If DM_VERSION_MAJOR is used, these flags are ignored by the kernel.
> - * If DM_VERSION_MAJOR_STRICT is used, these flags are reserved and
> - * must be zeroed.
>    */
>   #define DM_STRICT_ONLY_FLAGS					\
>   	(DM_ACTIVE_PRESENT_FLAG | DM_INACTIVE_PRESENT_FLAG |	\

Re: [dm-devel] [PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Sun, Jun 25, 2023 at 03:25:38PM +0200, Milan Broz wrote:
> On 6/25/23 01:09, Demi Marie Obenour wrote:
> > Userspace can use this to avoid spamming udev with events that udev
> > should ignore.
> 
> Well, does it also mean that udev will not create /dev/disk/by-* symlinks
> (as response to the change udev event followed by internal udev blkid scan)?

In the use-case I have for this feature (block devices for Qubes VMs)
the blkid scan is unwanted and there are udev rules to prevent this.

> If it is a private device, that is ok. But for a visible device I think
> that it breaks some assumptions in userspace (presence of symlinks mentioned
> above etc).

The devices I am considering are implementation details of a userspace
process.  Nobody else should be opening them.  Ideally, no other
userspace process would even know they exist, at least without mucking
around in /proc or using ptrace.

> So, what is the exact use for this patch?

Ephemeral devices that are created, opened, marked for deferred removal,
assigned to a Xen VM (needs another patch currently being worked on),
and then closed.  udev has no business scanning these devices, and
indeed for it to scan them at all would be a security vulnerability
since their contents are under guest control.  There are udev rules to
ignore these devices, but for udev to even process the event wastes CPU
time and delays processing of other events that actually matter.  The
only symlink that possibly ought to be created is /dev/disk/by-diskseq
and I can just do that myself.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [dm-devel] [PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Milan Broz]

On 6/25/23 18:02, Demi Marie Obenour wrote:
> On Sun, Jun 25, 2023 at 03:25:38PM +0200, Milan Broz wrote:
>> On 6/25/23 01:09, Demi Marie Obenour wrote:
>>> Userspace can use this to avoid spamming udev with events that udev
>>> should ignore.
>>
>> Well, does it also mean that udev will not create /dev/disk/by-* symlinks
>> (as response to the change udev event followed by internal udev blkid scan)?
> 
> In the use-case I have for this feature (block devices for Qubes VMs)
> the blkid scan is unwanted and there are udev rules to prevent this.
> 
>> If it is a private device, that is ok. But for a visible device I think
>> that it breaks some assumptions in userspace (presence of symlinks mentioned
>> above etc).
> 
> The devices I am considering are implementation details of a userspace
> process.  Nobody else should be opening them.  Ideally, no other
> userspace process would even know they exist, at least without mucking
> around in /proc or using ptrace.
> 
>> So, what is the exact use for this patch?
> 
> Ephemeral devices that are created, opened, marked for deferred removal,
> assigned to a Xen VM (needs another patch currently being worked on),
> and then closed.  udev has no business scanning these devices, and
> indeed for it to scan them at all would be a security vulnerability
> since their contents are under guest control.  There are udev rules to
> ignore these devices, but for udev to even process the event wastes CPU
> time and delays processing of other events that actually matter.  The
> only symlink that possibly ought to be created is /dev/disk/by-diskseq
> and I can just do that myself.
But this is not clear from the patch header. I guess you also need
to disable udev inotify on close on write, which will trigger device scan too.

BTW we use exactly this scenario in cryptsetup for years with existing flags
(DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG | DM_UDEV_DISABLE_DISK_RULES_FLAG
DM_UDEV_DISABLE_OTHER_RULES_FLAG) - just rules are ignored while uevent is still
sent.
Anyway, not sure we need another way to disable it; I just asked do you need it.

Milan

Re: [dm-devel] [PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

On Sun, Jun 25, 2023 at 06:33:33PM +0200, Milan Broz wrote:
> On 6/25/23 18:02, Demi Marie Obenour wrote:
> > On Sun, Jun 25, 2023 at 03:25:38PM +0200, Milan Broz wrote:
> > > On 6/25/23 01:09, Demi Marie Obenour wrote:
> > > > Userspace can use this to avoid spamming udev with events that udev
> > > > should ignore.
> > > 
> > > Well, does it also mean that udev will not create /dev/disk/by-* symlinks
> > > (as response to the change udev event followed by internal udev blkid scan)?
> > 
> > In the use-case I have for this feature (block devices for Qubes VMs)
> > the blkid scan is unwanted and there are udev rules to prevent this.
> > 
> > > If it is a private device, that is ok. But for a visible device I think
> > > that it breaks some assumptions in userspace (presence of symlinks mentioned
> > > above etc).
> > 
> > The devices I am considering are implementation details of a userspace
> > process.  Nobody else should be opening them.  Ideally, no other
> > userspace process would even know they exist, at least without mucking
> > around in /proc or using ptrace.
> > 
> > > So, what is the exact use for this patch?
> > 
> > Ephemeral devices that are created, opened, marked for deferred removal,
> > assigned to a Xen VM (needs another patch currently being worked on),
> > and then closed.  udev has no business scanning these devices, and
> > indeed for it to scan them at all would be a security vulnerability
> > since their contents are under guest control.  There are udev rules to
> > ignore these devices, but for udev to even process the event wastes CPU
> > time and delays processing of other events that actually matter.  The
> > only symlink that possibly ought to be created is /dev/disk/by-diskseq
> > and I can just do that myself.
> But this is not clear from the patch header. I guess you also need
> to disable udev inotify on close on write, which will trigger device scan too.
> 
> BTW we use exactly this scenario in cryptsetup for years with existing flags
> (DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG | DM_UDEV_DISABLE_DISK_RULES_FLAG
> DM_UDEV_DISABLE_OTHER_RULES_FLAG) - just rules are ignored while uevent is still
> sent.
> Anyway, not sure we need another way to disable it; I just asked do you need it.

How can one set these flags using the raw kernel ioctls?  The code I am
working on does not use libdevmapper at all and just uses the kernel API
directly.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [dm-devel] [PATCH v2 3/4] dm ioctl: Allow userspace to suppress uevent generation

[Milan Broz]

On 6/25/23 18:43, Demi Marie Obenour wrote:
> On Sun, Jun 25, 2023 at 06:33:33PM +0200, Milan Broz wrote:

...

>> BTW we use exactly this scenario in cryptsetup for years with existing flags
>> (DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG | DM_UDEV_DISABLE_DISK_RULES_FLAG
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG) - just rules are ignored while uevent is still
>> sent.
>> Anyway, not sure we need another way to disable it; I just asked do you need it.
> 
> How can one set these flags using the raw kernel ioctls?  The code I am
> working on does not use libdevmapper at all and just uses the kernel API
> directly.

IIRC you need to set cookie (kernel DM_COOKIE env), then default udev dm.rules decode
the flags from it.

(It is libdevmapper internal magic for me, the design is kind of complicated, but works.)

Better ask on LVM list (but the udev DM rules are pretty well inline documented).

Milan

[PATCH v2 4/4] dm ioctl: inform caller about already-existing device

[Demi Marie Obenour]

Not only is this helpful for debugging, it also saves the caller an
ioctl in the case where a device should be used if it exists or created
otherwise.  To ensure existing userspace is not broken, this feature is
only enabled in strict mode.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c         | 31 +++++++++++++++++++++++++------
 include/uapi/linux/dm-ioctl.h |  4 ++--
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 7eab9a8c77ff3286346a1c44581d3b6370a731eb..4eb3eda2fe169f64259458dd678833f444d76ce0 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -259,11 +259,14 @@ static void free_cell(struct hash_cell *hc)
 	}
 }
 
+static void __dev_status(struct mapped_device *md, struct dm_ioctl *param);
+
 /*
  * The kdev_t and uuid of a device can never change once it is
  * initially inserted.
  */
-static int dm_hash_insert(const char *name, const char *uuid, struct mapped_device *md)
+static int dm_hash_insert(const char *name, const char *uuid, struct mapped_device *md,
+			  struct dm_ioctl *param)
 {
 	struct hash_cell *cell, *hc;
 
@@ -280,6 +283,8 @@ static int dm_hash_insert(const char *name, const char *uuid, struct mapped_devi
 	down_write(&_hash_lock);
 	hc = __get_name_cell(name);
 	if (hc) {
+		if (param)
+			__dev_status(hc->md, param);
 		dm_put(hc->md);
 		goto bad;
 	}
@@ -290,6 +295,8 @@ static int dm_hash_insert(const char *name, const char *uuid, struct mapped_devi
 		hc = __get_uuid_cell(uuid);
 		if (hc) {
 			__unlink_name(cell);
+			if (param)
+				__dev_status(hc->md, param);
 			dm_put(hc->md);
 			goto bad;
 		}
@@ -901,10 +908,12 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
 		m = MINOR(huge_decode_dev(param->dev));
 
 	r = dm_create(m, &md, param->flags & DM_DISABLE_UEVENTS_FLAG);
-	if (r)
+	if (r) {
+		DMERR("Could not create device-mapper device");
 		return r;
+	}
 
-	r = dm_hash_insert(param->name, *param->uuid ? param->uuid : NULL, md);
+	r = dm_hash_insert(param->name, *param->uuid ? param->uuid : NULL, md, param);
 	if (r) {
 		dm_put(md);
 		dm_destroy(md);
@@ -2234,6 +2243,7 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	int ioctl_flags;
 	int param_flags;
 	unsigned int cmd;
+	bool do_copy;
 	struct dm_ioctl *param;
 	ioctl_fn fn = NULL;
 	size_t input_param_size;
@@ -2307,9 +2317,18 @@ static int ctl_ioctl(struct file *file, uint command, struct dm_ioctl __user *us
 	param->flags |= old_flags;
 
 	/*
-	 * Copy the results back to userland.
+	 * Copy the results back to userland if either:
+	 *
+	 * - The ioctl succeeded.
+	 * - The ioctl is DM_DEV_CREATE, the return value is -EBUSY,
+	 *   and strict parameter checking is enabled.
 	 */
-	if (!r && copy_to_user(user, param, param->data_size))
+	do_copy = r == 0;
+	if (r == -EBUSY && !sloppy_checks(param) && cmd == DM_DEV_CREATE_CMD) {
+		param->flags |= DM_EXISTS_FLAG;
+		do_copy = true;
+	}
+	if (do_copy && copy_to_user(user, param, param->data_size))
 		r = -EFAULT;
 
 out:
@@ -2488,7 +2507,7 @@ int __init dm_early_create(struct dm_ioctl *dmi,
 		return r;
 
 	/* hash insert */
-	r = dm_hash_insert(dmi->name, *dmi->uuid ? dmi->uuid : NULL, md);
+	r = dm_hash_insert(dmi->name, *dmi->uuid ? dmi->uuid : NULL, md, NULL);
 	if (r)
 		goto err_destroy_dm;
 
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index b338a4f9a299acda9430995d63fbb490e70c8cd8..195d60be4aba4f20b7220b900c87d6d3a86014a2 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -344,8 +344,8 @@ enum {
 /* Status bits */
 #define DM_READONLY_FLAG	(1 << 0) /* In/Out */
 #define DM_SUSPEND_FLAG		(1 << 1) /* In/Out */
-#define DM_EXISTS_FLAG		(1 << 2) /* Not used by kernel, reserved for
-					  * libdevmapper in userland
+#define DM_EXISTS_FLAG		(1 << 2) /* Set when trying to create a
+					  * device that already exists
 					  */
 #define DM_PERSISTENT_DEV_FLAG	(1 << 3) /* In */
 
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [dm-devel] [PATCH v2 0/4] Diskseq support in device-mapper

[Martin Wilck]

On Sat, 2023-06-24 at 19:09 -0400, Demi Marie Obenour wrote:
> This work aims to allow userspace to create and destroy device-mapper
> devices in a race-free way.

The discussion about this feature seems to have stalled ... will there
be a v3 of this series any time soon?

Also, I am wondering what should happen if a device-mapper table is
changed in a SUSPEND/LOAD/RESUME cycle. Such operations can change the
content of the device, thus I assume that the diskseq should also
change. But AFAICS this wasn't part of your patch set.

In general, whether the content changes in a reload operation depends
on the target. The multipath target, for example, reloads frequently
without changing the content of the dm device. An ever-changing diskseq
wouldn't make a lot of sense for dm-multipath. But I doubt we want to
start making distinctions on this level, so I guess that diskseq and
multipath just won't go well together.

Regards,
Martin 

Re: [dm-devel] [PATCH v2 0/4] Diskseq support in device-mapper

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On Mon, Jan 15, 2024 at 06:56:16PM +0100, Martin Wilck wrote:
> On Sat, 2023-06-24 at 19:09 -0400, Demi Marie Obenour wrote:
> > This work aims to allow userspace to create and destroy device-mapper
> > devices in a race-free way.
> 
> The discussion about this feature seems to have stalled ... will there
> be a v3 of this series any time soon?

I’m still interested in a v3, but it might take a while.  If you are
willing and able to do it first, I recommend that you do so.

> Also, I am wondering what should happen if a device-mapper table is
> changed in a SUSPEND/LOAD/RESUME cycle. Such operations can change the
> content of the device, thus I assume that the diskseq should also
> change. But AFAICS this wasn't part of your patch set.
> 
> In general, whether the content changes in a reload operation depends
> on the target. The multipath target, for example, reloads frequently
> without changing the content of the dm device. An ever-changing diskseq
> wouldn't make a lot of sense for dm-multipath. But I doubt we want to
> start making distinctions on this level, so I guess that diskseq and
> multipath just won't go well together.

Should this be controlled by userspace?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [dm-devel] [PATCH v2 0/4] Diskseq support in device-mapper

[Martin Wilck]

On Mon, 2024-01-15 at 16:44 -0500, Demi Marie Obenour wrote:
> On Mon, Jan 15, 2024 at 06:56:16PM +0100, Martin Wilck wrote:
> > On Sat, 2023-06-24 at 19:09 -0400, Demi Marie Obenour wrote:
> > > This work aims to allow userspace to create and destroy device-
> > > mapper
> > > devices in a race-free way.
> > 
> > The discussion about this feature seems to have stalled ... will
> > there
> > be a v3 of this series any time soon?
> 
> I’m still interested in a v3, but it might take a while.  If you are
> willing and able to do it first, I recommend that you do so.

No, I was just trying to understand the status.

> 
> > Also, I am wondering what should happen if a device-mapper table is
> > changed in a SUSPEND/LOAD/RESUME cycle. Such operations can change
> > the
> > content of the device, thus I assume that the diskseq should also
> > change. But AFAICS this wasn't part of your patch set.
> > 
> > In general, whether the content changes in a reload operation
> > depends
> > on the target. The multipath target, for example, reloads
> > frequently
> > without changing the content of the dm device. An ever-changing
> > diskseq
> > wouldn't make a lot of sense for dm-multipath. But I doubt we want
> > to
> > start making distinctions on this level, so I guess that diskseq
> > and
> > multipath just won't go well together.
> 
> Should this be controlled by userspace?

Personally, I don't think so, but I guess this deserves a broader
discussion.

IMO users who want to benefit from the diskseq feature would not want
to be surprised by device-mapper devices changing under them, and would
also not want to have some block devices with diskseq semantics and
others without. Therefore I believe that it's sufficient to be able to
have some global switch to enable or disable the use of diskseq. But
I've only learned about this feature pretty recently, so I may easily
be misunderstanding something.

Martin