When a device is being removed with deferred remove it's
still possible to open and use the device. This change
introduces a flag called DM_DEFERRED_REMOVE_NO_OPEN_FLAG
which when used with DM_DEFERRED_REMOVE will cause any
new opens to fail with -ENXIO.
If this flag is used without DM_DEFERRED_REMOVE it will
result in an -EINVAL.
Signed-off-by: Brian Geffon <[email protected]>
---
drivers/md/dm-core.h | 1 +
drivers/md/dm-ioctl.c | 39 +++++++++++++++++++++++++++--------
drivers/md/dm.c | 21 ++++++++++++++++---
drivers/md/dm.h | 9 +++++++-
include/uapi/linux/dm-ioctl.h | 12 +++++++++--
5 files changed, 67 insertions(+), 15 deletions(-)
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index b855fef4f38a..b30e59deb4a8 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -139,6 +139,7 @@ struct mapped_device {
#define DMF_SUSPENDED_INTERNALLY 7
#define DMF_POST_SUSPENDING 8
#define DMF_EMULATE_ZONE_APPEND 9
+#define DMF_DEFERRED_REMOVE_NO_OPEN 10
void disable_discard(struct mapped_device *md);
void disable_write_same(struct mapped_device *md);
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 21fe8652b095..07bb679880de 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -60,7 +60,8 @@ struct vers_iter {
static struct rb_root name_rb_tree = RB_ROOT;
static struct rb_root uuid_rb_tree = RB_ROOT;
-static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred, bool only_deferred);
+static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,
+ bool deferred_no_open, bool only_deferred);
/*
* Guards access to both hash tables.
@@ -74,7 +75,7 @@ static DEFINE_MUTEX(dm_hash_cells_mutex);
static void dm_hash_exit(void)
{
- dm_hash_remove_all(false, false, false);
+ dm_hash_remove_all(false, false, false, false);
}
/*-----------------------------------------------------------------
@@ -315,7 +316,8 @@ static struct dm_table *__hash_remove(struct hash_cell *hc)
return table;
}
-static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred, bool only_deferred)
+static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred,
+ bool deferred_no_open, bool only_deferred)
{
int dev_skipped;
struct rb_node *n;
@@ -334,7 +336,8 @@ static void dm_hash_remove_all(bool keep_open_devices, bool mark_deferred, bool
dm_get(md);
if (keep_open_devices &&
- dm_lock_for_deletion(md, mark_deferred, only_deferred)) {
+ dm_lock_for_deletion(md, mark_deferred, deferred_no_open,
+ only_deferred)) {
dm_put(md);
dev_skipped++;
continue;
@@ -496,7 +499,7 @@ static struct mapped_device *dm_hash_rename(struct dm_ioctl *param,
void dm_deferred_remove(void)
{
- dm_hash_remove_all(true, false, true);
+ dm_hash_remove_all(true, false, false, true);
}
/*-----------------------------------------------------------------
@@ -510,7 +513,13 @@ typedef int (*ioctl_fn)(struct file *filp, struct dm_ioctl *param, size_t param_
static int remove_all(struct file *filp, struct dm_ioctl *param, size_t param_size)
{
- dm_hash_remove_all(true, !!(param->flags & DM_DEFERRED_REMOVE), false);
+ if (param->flags & DM_DEFERRED_REMOVE_NO_OPEN_FLAG &&
+ !(param->flags & DM_DEFERRED_REMOVE)) {
+ return -EINVAL;
+ }
+
+ dm_hash_remove_all(true, !!(param->flags & DM_DEFERRED_REMOVE),
+ !!(param->flags & DM_DEFERRED_REMOVE_NO_OPEN_FLAG), false);
param->data_size = 0;
return 0;
}
@@ -811,9 +820,13 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
if (dm_suspended_internally_md(md))
param->flags |= DM_INTERNAL_SUSPEND_FLAG;
- if (dm_test_deferred_remove_flag(md))
+ if (dm_test_deferred_remove_flag(md)) {
param->flags |= DM_DEFERRED_REMOVE;
+ if (dm_test_deferred_remove_no_open_flag(md))
+ param->flags |= DM_DEFERRED_REMOVE_NO_OPEN_FLAG;
+ }
+
param->dev = huge_encode_dev(disk_devt(disk));
/*
@@ -960,10 +973,18 @@ static int dev_remove(struct file *filp, struct dm_ioctl *param, size_t param_si
md = hc->md;
+ if (param->flags & DM_DEFERRED_REMOVE_NO_OPEN_FLAG &&
+ !(param->flags & DM_DEFERRED_REMOVE)) {
+ up_write(&_hash_lock);
+ dm_put(md);
+ return -EINVAL;
+ }
+
/*
* Ensure the device is not open and nothing further can open it.
*/
- r = dm_lock_for_deletion(md, !!(param->flags & DM_DEFERRED_REMOVE), false);
+ r = dm_lock_for_deletion(md, !!(param->flags & DM_DEFERRED_REMOVE),
+ !!(param->flags & DM_DEFERRED_REMOVE_NO_OPEN_FLAG), false);
if (r) {
if (r == -EBUSY && param->flags & DM_DEFERRED_REMOVE) {
up_write(&_hash_lock);
@@ -984,7 +1005,7 @@ static int dev_remove(struct file *filp, struct dm_ioctl *param, size_t param_si
dm_table_destroy(t);
}
- param->flags &= ~DM_DEFERRED_REMOVE;
+ param->flags &= ~(DM_DEFERRED_REMOVE | DM_DEFERRED_REMOVE_NO_OPEN_FLAG);
dm_ima_measure_on_device_remove(md, false);
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index c0ae8087c602..90b74043e162 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -315,7 +315,10 @@ static int dm_blk_open(struct block_device *bdev, fmode_t mode)
goto out;
if (test_bit(DMF_FREEING, &md->flags) ||
+ test_bit(DMF_DEFERRED_REMOVE_NO_OPEN, &md->flags) ||
dm_deleting_md(md)) {
+ BUG_ON(test_bit(DMF_DEFERRED_REMOVE_NO_OPEN, &md->flags) &&
+ !test_bit(DMF_DEFERRED_REMOVE, &md->flags));
md = NULL;
goto out;
}
@@ -355,7 +358,8 @@ int dm_open_count(struct mapped_device *md)
/*
* Guarantees nothing is using the device before it's deleted.
*/
-int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred, bool only_deferred)
+int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred,
+ bool deferred_no_open, bool only_deferred)
{
int r = 0;
@@ -363,8 +367,12 @@ int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred, bool only
if (dm_open_count(md)) {
r = -EBUSY;
- if (mark_deferred)
+ if (mark_deferred) {
set_bit(DMF_DEFERRED_REMOVE, &md->flags);
+
+ if (deferred_no_open)
+ set_bit(DMF_DEFERRED_REMOVE_NO_OPEN, &md->flags);
+ }
} else if (only_deferred && !test_bit(DMF_DEFERRED_REMOVE, &md->flags))
r = -EEXIST;
else
@@ -383,8 +391,10 @@ int dm_cancel_deferred_remove(struct mapped_device *md)
if (test_bit(DMF_DELETING, &md->flags))
r = -EBUSY;
- else
+ else {
clear_bit(DMF_DEFERRED_REMOVE, &md->flags);
+ clear_bit(DMF_DEFERRED_REMOVE_NO_OPEN, &md->flags);
+ }
spin_unlock(&_minor_lock);
@@ -2718,6 +2728,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md)
return test_bit(DMF_DEFERRED_REMOVE, &md->flags);
}
+int dm_test_deferred_remove_no_open_flag(struct mapped_device *md)
+{
+ return test_bit(DMF_DEFERRED_REMOVE_NO_OPEN, &md->flags);
+}
+
int dm_suspended(struct dm_target *ti)
{
return dm_suspended_md(ti->table->md);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 9013dc1a7b00..8d0d4344f882 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -158,6 +158,12 @@ void dm_internal_resume(struct mapped_device *md);
*/
int dm_test_deferred_remove_flag(struct mapped_device *md);
+/*
+ * Test if the device is scheduled for deferred remove while
+ * disallowing opens.
+ */
+int dm_test_deferred_remove_no_open_flag(struct mapped_device *md);
+
/*
* Try to remove devices marked for deferred removal.
*/
@@ -198,7 +204,8 @@ void dm_stripe_exit(void);
void dm_destroy(struct mapped_device *md);
void dm_destroy_immediate(struct mapped_device *md);
int dm_open_count(struct mapped_device *md);
-int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred, bool only_deferred);
+int dm_lock_for_deletion(struct mapped_device *md, bool mark_deferred,
+ bool deferred_no_open, bool only_deferred);
int dm_cancel_deferred_remove(struct mapped_device *md);
int dm_request_based(struct mapped_device *md);
int dm_get_table_device(struct mapped_device *md, dev_t dev, fmode_t mode,
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index c12ce30b52df..c0fee607b827 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -286,9 +286,9 @@ enum {
#define DM_DEV_SET_GEOMETRY _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
#define DM_VERSION_MAJOR 4
-#define DM_VERSION_MINOR 45
+#define DM_VERSION_MINOR 46
#define DM_VERSION_PATCHLEVEL 0
-#define DM_VERSION_EXTRA "-ioctl (2021-03-22)"
+#define DM_VERSION_EXTRA "-ioctl (2022-01-21)"
/* Status bits */
#define DM_READONLY_FLAG (1 << 0) /* In/Out */
@@ -382,4 +382,12 @@ enum {
*/
#define DM_IMA_MEASUREMENT_FLAG (1 << 19) /* In */
+/* If set with DF_DEFERRED_REMOVE if an immediate remove is not
+ * possible because the device is still open any new additional
+ * opens will also be rejected.
+ *
+ * It is an error to specify this flag without DM_DEFERRED_REMOVE.
+ */
+#define DM_DEFERRED_REMOVE_NO_OPEN_FLAG (1 << 20) /* In/Out */
+
#endif /* _LINUX_DM_IOCTL_H */
--
2.35.0.rc0.227.g00780c9af4-goog
On Mon, Jan 24, 2022 at 07:02:09AM -0800, Brian Geffon wrote:
> When a device is being removed with deferred remove it's
> still possible to open and use the device. This change
> introduces a flag called DM_DEFERRED_REMOVE_NO_OPEN_FLAG
> which when used with DM_DEFERRED_REMOVE will cause any
> new opens to fail with -ENXIO.
What is the need for this?
Does it break any semantics assumed by userspace?
Alasdair
On Mon, Jan 24, 2022 at 10:14 AM Alasdair G Kergon <[email protected]> wrote:
>
> On Mon, Jan 24, 2022 at 07:02:09AM -0800, Brian Geffon wrote:
> > When a device is being removed with deferred remove it's
> > still possible to open and use the device. This change
> > introduces a flag called DM_DEFERRED_REMOVE_NO_OPEN_FLAG
> > which when used with DM_DEFERRED_REMOVE will cause any
> > new opens to fail with -ENXIO.
>
> What is the need for this?
Hi Alasdair,
Thank you for looking at this. There are a few reasons this might be
useful, the first is if you're trying to speed up a graceful teardown
of the device by informing userspace that this device is going to be
removed in the near future. Another might be on systems where it might
be worthwhile to not have users with CAP_DAC_OVERRIDE be able to open
the device. The logic on this second case is that, suppose you have a
dm-crypt block device which is backing swap, the data on this device
is ephemeral so a flow might be to setup swap followed by dmsetup
remove --deferred /dev/mapper/encrypted-swap. This will guarantee that
as soon as swap is torn down the encrypted block device is dropped,
additionally with this new flag you'll be guaranteed that there can be
no further opens on it.
> Does it break any semantics assumed by userspace?
No, this is fully backwards compatible with the current deferred
remove behavior, it's not required. Additionally, since on the actual
remove userspace would receive an -ENXIO already once the remove
process has started it seems reasonable to return -ENXIO in the
deferred remove case when this flag is enabled.
Brian
On Mon, Jan 24, 2022 at 10:25:47AM -0500, Brian Geffon wrote:
> Thank you for looking at this. There are a few reasons this might be
> useful, the first is if you're trying to speed up a graceful teardown
> of the device by informing userspace that this device is going to be
> removed in the near future. Another might be on systems where it might
> be worthwhile to not have users with CAP_DAC_OVERRIDE be able to open
> the device. The logic on this second case is that, suppose you have a
> dm-crypt block device which is backing swap, the data on this device
> is ephemeral so a flow might be to setup swap followed by dmsetup
> remove --deferred /dev/mapper/encrypted-swap. This will guarantee that
> as soon as swap is torn down the encrypted block device is dropped,
> additionally with this new flag you'll be guaranteed that there can be
> no further opens on it.
And is that the reason you propose this?
- You want a special exclusive 'one time open' device that
self-destructs when closed?
> No, this is fully backwards compatible with the current deferred
> remove behavior, it's not required. Additionally, since on the actual
> remove userspace would receive an -ENXIO already once the remove
> process has started it seems reasonable to return -ENXIO in the
> deferred remove case when this flag is enabled.
Well I feel it does break existing semantics which is why we wrote
the code the way we did. The state can be long-lived, the code
that has it open might legitimately want to open it again in
parallel etc. - in general this seems a bad idea.
But if the reason for this is basically "make it harder for
anything else to access my encrypted swap" and to deliberately
prevent access, then let's approach the requirement from that angle.
Are there alternative implementations with interventions at different
points? Are there similar requirements for devices that don't need
deferred remove? If this is indeed the best place to insert this type
of restriction, then we should label it and document it accordingly so
people don't mistakenly use it for the 'normal' case. (We always keep
libdevmapper and dmsetup in sync with kernel interface extensions, so
we'd seek a tiny patch to do that too.)
Alasdair
On Mon, Jan 24, 2022 at 7:21 PM Alasdair G Kergon <[email protected]> wrote:
>
> On Mon, Jan 24, 2022 at 10:25:47AM -0500, Brian Geffon wrote:
> > Thank you for looking at this. There are a few reasons this might be
> > useful, the first is if you're trying to speed up a graceful teardown
> > of the device by informing userspace that this device is going to be
> > removed in the near future. Another might be on systems where it might
> > be worthwhile to not have users with CAP_DAC_OVERRIDE be able to open
> > the device. The logic on this second case is that, suppose you have a
> > dm-crypt block device which is backing swap, the data on this device
> > is ephemeral so a flow might be to setup swap followed by dmsetup
> > remove --deferred /dev/mapper/encrypted-swap. This will guarantee that
> > as soon as swap is torn down the encrypted block device is dropped,
> > additionally with this new flag you'll be guaranteed that there can be
> > no further opens on it.
>
> And is that the reason you propose this?
> - You want a special exclusive 'one time open' device that
> self-destructs when closed?
Yes, this is the primary reason I'm exploring this. I tried to find an
implementation that might be useful in other situations which is why I
landed on this approach.
>
> > No, this is fully backwards compatible with the current deferred
> > remove behavior, it's not required. Additionally, since on the actual
> > remove userspace would receive an -ENXIO already once the remove
> > process has started it seems reasonable to return -ENXIO in the
> > deferred remove case when this flag is enabled.
>
> Well I feel it does break existing semantics which is why we wrote
> the code the way we did. The state can be long-lived, the code
> that has it open might legitimately want to open it again in
> parallel etc. - in general this seems a bad idea.
Thanks for explaining and providing that context.
>
> But if the reason for this is basically "make it harder for
> anything else to access my encrypted swap" and to deliberately
> prevent access, then let's approach the requirement from that angle.
> Are there alternative implementations with interventions at different
> points?
Absolutely, we could perhaps create a new ioctl which allows the
caller to specify the maximum open count, and when the open count hits
that value it fails any new opens with -EBUSY. Perhaps this would be
enforced in dm_get_device? This type of behavior could theoretically
mimic the patch I mailed when used in conjunction with deferred
removal. Is this an approach you think might make more sense with the
existing design? Are there any implementation points you believe might
make more sense for such a feature?
> Are there similar requirements for devices that don't need
> deferred remove?
I cannot immediately think of a situation where you'd do this without
deferred removal.
> If this is indeed the best place to insert this type
> of restriction, then we should label it and document it accordingly so
> people don't mistakenly use it for the 'normal' case. (We always keep
> libdevmapper and dmsetup in sync with kernel interface extensions, so
> we'd seek a tiny patch to do that too.)
Yes, absolutely. I'll happily send patches for userspace libraries,
applications, and documentation once we converge on an acceptable
approach.
Thanks again for spending your time discussing this,
Brian
On Mon, Jan 24, 2022 at 7:21 PM Alasdair G Kergon <[email protected]> wrote:
>
> On Mon, Jan 24, 2022 at 10:25:47AM -0500, Brian Geffon wrote:
> > Thank you for looking at this. There are a few reasons this might be
> > useful, the first is if you're trying to speed up a graceful teardown
> > of the device by informing userspace that this device is going to be
> > removed in the near future. Another might be on systems where it might
> > be worthwhile to not have users with CAP_DAC_OVERRIDE be able to open
> > the device. The logic on this second case is that, suppose you have a
> > dm-crypt block device which is backing swap, the data on this device
> > is ephemeral so a flow might be to setup swap followed by dmsetup
> > remove --deferred /dev/mapper/encrypted-swap. This will guarantee that
> > as soon as swap is torn down the encrypted block device is dropped,
> > additionally with this new flag you'll be guaranteed that there can be
> > no further opens on it.
>
> And is that the reason you propose this?
> - You want a special exclusive 'one time open' device that
> self-destructs when closed?
>
> > No, this is fully backwards compatible with the current deferred
> > remove behavior, it's not required. Additionally, since on the actual
> > remove userspace would receive an -ENXIO already once the remove
> > process has started it seems reasonable to return -ENXIO in the
> > deferred remove case when this flag is enabled.
>
> Well I feel it does break existing semantics which is why we wrote
> the code the way we did. The state can be long-lived, the code
> that has it open might legitimately want to open it again in
> parallel etc. - in general this seems a bad idea.
>
> But if the reason for this is basically "make it harder for
> anything else to access my encrypted swap" and to deliberately
> prevent access, then let's approach the requirement from that angle.
> Are there alternative implementations with interventions at different
> points?
I was thinking perhaps another implementation might involve using
open_count on dm_ioctl as an in param on DM_DEV_CREATE only. Using
open_count as an in parameter on DM_DEV_CREATE could be activated by a
new flag, perhaps DM_ENFORCE_OPEN_COUNT_FLAG. This would allow the
behavior to be baked into the device from the start. We would then
enforce it in dm_blk_open. What would you think about an approach like
this?
Thanks,
Brian
This change introduces a new flag which can be used with
DM_DEV_CREATE to establish the maximum open count allowed
for a device. When this flag is set on DM_DEV_CREATE the
open_count on dm_ioctl will be intrpreted as an input
parameter. This value must be >= 1 or DM_DEV_CREATE will
return -ERANGE.
When this flag is set when the open count is equal to
the max open count any future opens will result in an
-EBUSY.
Signed-off-by: Brian Geffon <[email protected]>
---
drivers/md/dm-core.h | 2 ++
drivers/md/dm-ioctl.c | 13 ++++++++++++
drivers/md/dm.c | 39 ++++++++++++++++++++++++++++++++---
drivers/md/dm.h | 7 +++++++
include/uapi/linux/dm-ioctl.h | 9 +++++++-
5 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
index 55dccdfbcb22..57922a80026e 100644
--- a/drivers/md/dm-core.h
+++ b/drivers/md/dm-core.h
@@ -57,6 +57,7 @@ struct mapped_device {
atomic_t holders;
atomic_t open_count;
+ int max_open_count;
struct dm_target *immutable_target;
struct target_type *immutable_target_type;
@@ -139,6 +140,7 @@ struct mapped_device {
#define DMF_SUSPENDED_INTERNALLY 7
#define DMF_POST_SUSPENDING 8
#define DMF_EMULATE_ZONE_APPEND 9
+#define DMF_ENFORCE_OPEN_COUNT 10
void disable_discard(struct mapped_device *md);
void disable_write_same(struct mapped_device *md);
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 21fe8652b095..8ddf3ab99ef6 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -814,6 +814,9 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
if (dm_test_deferred_remove_flag(md))
param->flags |= DM_DEFERRED_REMOVE;
+ if (dm_test_enforce_open_count_flag(md))
+ param->flags |= DM_ENFORCE_OPEN_COUNT_FLAG;
+
param->dev = huge_encode_dev(disk_devt(disk));
/*
@@ -866,6 +869,16 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
if (r)
return r;
+ if (param->flags & DM_ENFORCE_OPEN_COUNT_FLAG) {
+ if (param->open_count < 1) {
+ dm_put(md);
+ dm_destroy(md);
+ return -ERANGE;
+ }
+
+ dm_set_max_open_count(md, param->open_count);
+ }
+
r = dm_hash_insert(param->name, *param->uuid ? param->uuid : NULL, md);
if (r) {
dm_put(md);
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 76d9da49fda7..718bc9fce7c1 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -307,6 +307,7 @@ int dm_deleting_md(struct mapped_device *md)
static int dm_blk_open(struct block_device *bdev, fmode_t mode)
{
struct mapped_device *md;
+ int ret = -ENXIO;
spin_lock(&_minor_lock);
@@ -316,16 +317,28 @@ static int dm_blk_open(struct block_device *bdev, fmode_t mode)
if (test_bit(DMF_FREEING, &md->flags) ||
dm_deleting_md(md)) {
- md = NULL;
goto out;
}
dm_get(md);
+
+ if (test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags)) {
+ /*
+ * No opens or closes can happen in parallel as both
+ * paths hold the _minor_lock.
+ */
+ if (atomic_read(&md->open_count) + 1 > md->max_open_count) {
+ dm_put(md);
+ ret = -EBUSY;
+ goto out;
+ }
+ }
+
atomic_inc(&md->open_count);
+ ret = 0;
out:
spin_unlock(&_minor_lock);
-
- return md ? 0 : -ENXIO;
+ return ret;
}
static void dm_blk_close(struct gendisk *disk, fmode_t mode)
@@ -2219,6 +2232,21 @@ void dm_put(struct mapped_device *md)
}
EXPORT_SYMBOL_GPL(dm_put);
+/*
+ * dm_set_max_open count can only be called when the device is created,
+ * it cannot be changed once set.
+ */
+void dm_set_max_open_count(struct mapped_device *md, int count)
+{
+ /*
+ * The max open count cannot be changed
+ */
+ BUG_ON(test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags));
+
+ set_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags);
+ md->max_open_count = count;
+}
+
static bool md_in_flight_bios(struct mapped_device *md)
{
int cpu;
@@ -2795,6 +2823,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md)
return test_bit(DMF_DEFERRED_REMOVE, &md->flags);
}
+int dm_test_enforce_open_count_flag(struct mapped_device *md)
+{
+ return test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags);
+}
+
int dm_suspended(struct dm_target *ti)
{
return dm_suspended_md(ti->table->md);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index 742d9c80efe1..82f56a066b83 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -84,6 +84,8 @@ void dm_set_md_type(struct mapped_device *md, enum dm_queue_mode type);
enum dm_queue_mode dm_get_md_type(struct mapped_device *md);
struct target_type *dm_get_immutable_target_type(struct mapped_device *md);
+void dm_set_max_open_count(struct mapped_device *md, int count);
+
int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t);
/*
@@ -162,6 +164,11 @@ void dm_internal_resume(struct mapped_device *md);
*/
int dm_test_deferred_remove_flag(struct mapped_device *md);
+/*
+ * Test if the device is enforcing an open count.
+ */
+int dm_test_enforce_open_count_flag(struct mapped_device *md);
+
/*
* Try to remove devices marked for deferred removal.
*/
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index c12ce30b52df..9da3700c0442 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -123,7 +123,7 @@ struct dm_ioctl {
* relative to start of this struct */
__u32 target_count; /* in/out */
- __s32 open_count; /* out */
+ __s32 open_count; /* in/out, in on DM_DEV_CREATE only */
__u32 flags; /* in/out */
/*
@@ -382,4 +382,11 @@ enum {
*/
#define DM_IMA_MEASUREMENT_FLAG (1 << 19) /* In */
+/*
+ * If set with DM_DEV_CREATE then the open_count on device creation
+ * will be set as the maximum concurrent opens allowed on the device.
+ * Once the open_count has been hit any new opens will result in
+ * -EBUSY until other users close the device.
+ */
+#define DM_ENFORCE_OPEN_COUNT_FLAG (1 << 20) /* In/Out */
#endif /* _LINUX_DM_IOCTL_H */
--
2.35.0.rc0.227.g00780c9af4-goog
On Wed, Jan 26, 2022 at 2:22 PM Brian Geffon <[email protected]> wrote:
>
> This change introduces a new flag which can be used with
> DM_DEV_CREATE to establish the maximum open count allowed
> for a device. When this flag is set on DM_DEV_CREATE the
> open_count on dm_ioctl will be intrpreted as an input
> parameter. This value must be >= 1 or DM_DEV_CREATE will
> return -ERANGE.
>
> When this flag is set when the open count is equal to
> the max open count any future opens will result in an
> -EBUSY.
>
Hi Alasdair,
I was curious if you had any thoughts on this particular alternative
approach to this problem, I'm open to any suggestions of alternative
implementations.
Thank you in advance,
Brian
>
> Signed-off-by: Brian Geffon <[email protected]>
> ---
> drivers/md/dm-core.h | 2 ++
> drivers/md/dm-ioctl.c | 13 ++++++++++++
> drivers/md/dm.c | 39 ++++++++++++++++++++++++++++++++---
> drivers/md/dm.h | 7 +++++++
> include/uapi/linux/dm-ioctl.h | 9 +++++++-
> 5 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 55dccdfbcb22..57922a80026e 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -57,6 +57,7 @@ struct mapped_device {
>
> atomic_t holders;
> atomic_t open_count;
> + int max_open_count;
>
> struct dm_target *immutable_target;
> struct target_type *immutable_target_type;
> @@ -139,6 +140,7 @@ struct mapped_device {
> #define DMF_SUSPENDED_INTERNALLY 7
> #define DMF_POST_SUSPENDING 8
> #define DMF_EMULATE_ZONE_APPEND 9
> +#define DMF_ENFORCE_OPEN_COUNT 10
>
> void disable_discard(struct mapped_device *md);
> void disable_write_same(struct mapped_device *md);
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 21fe8652b095..8ddf3ab99ef6 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -814,6 +814,9 @@ static void __dev_status(struct mapped_device *md, struct dm_ioctl *param)
> if (dm_test_deferred_remove_flag(md))
> param->flags |= DM_DEFERRED_REMOVE;
>
> + if (dm_test_enforce_open_count_flag(md))
> + param->flags |= DM_ENFORCE_OPEN_COUNT_FLAG;
> +
> param->dev = huge_encode_dev(disk_devt(disk));
>
> /*
> @@ -866,6 +869,16 @@ static int dev_create(struct file *filp, struct dm_ioctl *param, size_t param_si
> if (r)
> return r;
>
> + if (param->flags & DM_ENFORCE_OPEN_COUNT_FLAG) {
> + if (param->open_count < 1) {
> + dm_put(md);
> + dm_destroy(md);
> + return -ERANGE;
> + }
> +
> + dm_set_max_open_count(md, param->open_count);
> + }
> +
> r = dm_hash_insert(param->name, *param->uuid ? param->uuid : NULL, md);
> if (r) {
> dm_put(md);
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 76d9da49fda7..718bc9fce7c1 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -307,6 +307,7 @@ int dm_deleting_md(struct mapped_device *md)
> static int dm_blk_open(struct block_device *bdev, fmode_t mode)
> {
> struct mapped_device *md;
> + int ret = -ENXIO;
>
> spin_lock(&_minor_lock);
>
> @@ -316,16 +317,28 @@ static int dm_blk_open(struct block_device *bdev, fmode_t mode)
>
> if (test_bit(DMF_FREEING, &md->flags) ||
> dm_deleting_md(md)) {
> - md = NULL;
> goto out;
> }
>
> dm_get(md);
> +
> + if (test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags)) {
> + /*
> + * No opens or closes can happen in parallel as both
> + * paths hold the _minor_lock.
> + */
> + if (atomic_read(&md->open_count) + 1 > md->max_open_count) {
> + dm_put(md);
> + ret = -EBUSY;
> + goto out;
> + }
> + }
> +
> atomic_inc(&md->open_count);
> + ret = 0;
> out:
> spin_unlock(&_minor_lock);
> -
> - return md ? 0 : -ENXIO;
> + return ret;
> }
>
> static void dm_blk_close(struct gendisk *disk, fmode_t mode)
> @@ -2219,6 +2232,21 @@ void dm_put(struct mapped_device *md)
> }
> EXPORT_SYMBOL_GPL(dm_put);
>
> +/*
> + * dm_set_max_open count can only be called when the device is created,
> + * it cannot be changed once set.
> + */
> +void dm_set_max_open_count(struct mapped_device *md, int count)
> +{
> + /*
> + * The max open count cannot be changed
> + */
> + BUG_ON(test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags));
> +
> + set_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags);
> + md->max_open_count = count;
> +}
> +
> static bool md_in_flight_bios(struct mapped_device *md)
> {
> int cpu;
> @@ -2795,6 +2823,11 @@ int dm_test_deferred_remove_flag(struct mapped_device *md)
> return test_bit(DMF_DEFERRED_REMOVE, &md->flags);
> }
>
> +int dm_test_enforce_open_count_flag(struct mapped_device *md)
> +{
> + return test_bit(DMF_ENFORCE_OPEN_COUNT, &md->flags);
> +}
> +
> int dm_suspended(struct dm_target *ti)
> {
> return dm_suspended_md(ti->table->md);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 742d9c80efe1..82f56a066b83 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -84,6 +84,8 @@ void dm_set_md_type(struct mapped_device *md, enum dm_queue_mode type);
> enum dm_queue_mode dm_get_md_type(struct mapped_device *md);
> struct target_type *dm_get_immutable_target_type(struct mapped_device *md);
>
> +void dm_set_max_open_count(struct mapped_device *md, int count);
> +
> int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t);
>
> /*
> @@ -162,6 +164,11 @@ void dm_internal_resume(struct mapped_device *md);
> */
> int dm_test_deferred_remove_flag(struct mapped_device *md);
>
> +/*
> + * Test if the device is enforcing an open count.
> + */
> +int dm_test_enforce_open_count_flag(struct mapped_device *md);
> +
> /*
> * Try to remove devices marked for deferred removal.
> */
> diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
> index c12ce30b52df..9da3700c0442 100644
> --- a/include/uapi/linux/dm-ioctl.h
> +++ b/include/uapi/linux/dm-ioctl.h
> @@ -123,7 +123,7 @@ struct dm_ioctl {
> * relative to start of this struct */
>
> __u32 target_count; /* in/out */
> - __s32 open_count; /* out */
> + __s32 open_count; /* in/out, in on DM_DEV_CREATE only */
> __u32 flags; /* in/out */
>
> /*
> @@ -382,4 +382,11 @@ enum {
> */
> #define DM_IMA_MEASUREMENT_FLAG (1 << 19) /* In */
>
> +/*
> + * If set with DM_DEV_CREATE then the open_count on device creation
> + * will be set as the maximum concurrent opens allowed on the device.
> + * Once the open_count has been hit any new opens will result in
> + * -EBUSY until other users close the device.
> + */
> +#define DM_ENFORCE_OPEN_COUNT_FLAG (1 << 20) /* In/Out */
> #endif /* _LINUX_DM_IOCTL_H */
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>
Withmy block hat on: nak to this. No block driver has any business at
all rejecting "too many openers". In fact any opener but the first of
a partition is already not counted as an opener, and I plan to complete
hide the open count from the device driver.
On Wed, Feb 02 2022 at 9:42P -0500,
Christoph Hellwig <[email protected]> wrote:
> Withmy block hat on: nak to this. No block driver has any business at
> all rejecting "too many openers". In fact any opener but the first of
> a partition is already not counted as an opener, and I plan to complete
> hide the open count from the device driver.
I agree that this proposal exposes controls to userspace that simply
shouldn't be meaningful given the arbitrary nature of openers. And
preventing openers can result in a race where systemd or some other
service opens before the intended primary consumer of the device.
Seriously brittle and even if finely tuned to have a suitable value
for some niche usecase like android: an absolute hack.
Sorry, not interested in taking this.