diff mbox series

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

Message ID 20230624230950.2272-3-demi@invisiblethingslab.com
State New
Headers
Series Diskseq support in device-mapper |

Commit Message

Demi Marie Obenour June 24, 2023, 11:09 p.m. UTC 
  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(-)

Comments

Demi Marie Obenour June 25, 2023, 5:39 p.m. UTC | #1 
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
Dan Carpenter June 26, 2023, 12:59 p.m. UTC | #2 
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
Dan Carpenter June 26, 2023, 2:51 p.m. UTC | #3 
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
Dan Carpenter June 27, 2023, 6:14 a.m. UTC | #4 
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
diff mbox series

Patch

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.