[md PATCH 0/2] Make it possible to disable create_on_open semantics.

[md PATCH 0/2] Make it possible to disable create_on_open semantics.

[NeilBrown]

Currently, opening an md /dev node will create the array object.
This makes it hard to destroy the object as udev will typically
re-open the device node when handling REMOVE events.

The "new_array" module parameter was created to work towards avoiding
this problem, and it can be used when
  CREATE names=yes

is given in /etc/mdadm.conf.
How this doesn't currently support names like "md%d", which lots of
people use and expect, so we need more work before we can transition
away from create_on_open.

These patches add support to "new_array" so that md%d devices
can be created.  This will make it, once again, possible to have
md%d devices with numbers > 511. (3.17 make this impossible).

An enhancement to mdadm that uses this will cause new_array to always
be used (where available), and we can then disable create_on_open
completely (after suitable transition periods).

NeilBrown



---

NeilBrown (2):
      md: allow creation of mdNNN arrays via md_mod/parameters/new_array
      md: support disabling of create-on-open semantics.


 drivers/md/md.c |   48 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

--
Signature

[md PATCH 2/2] md: support disabling of create-on-open semantics.

[NeilBrown]

md allows a new array device to be created by simply
opening a device file.  This make it difficult to
remove the device and udev is likely to open the device file
as part of processing the REMOVE event.

There is an alternate mechanism for creating arrays
by writing to the new_array module parameter.
When using tools that work with this parameter, it is
best to disable the old semantics.
This new module parameter allows that.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/md/md.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index c3d3bae947a1..a7ab769eacc3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -174,6 +174,16 @@ static const struct block_device_operations md_fops;
 
 static int start_readonly;
 
+/*
+ * The original mechanism for creating an md device is to create
+ * a device node in /dev and to open it.  This causes races with device-close.
+ * The preferred method is to write to the "new_array" module parameter.
+ * This can avoid races.
+ * Setting create_on_open to false disables the original mechanism
+ * so all the races disappear.
+ */
+static bool create_on_open = true;
+
 /* bio_clone_mddev
  * like bio_clone_bioset, but with a local bio set
  */
@@ -5286,7 +5296,8 @@ static int md_alloc(dev_t dev, char *name)
 
 static struct kobject *md_probe(dev_t dev, int *part, void *data)
 {
-	md_alloc(dev, NULL);
+	if (create_on_open)
+		md_alloc(dev, NULL);
 	return NULL;
 }
 
@@ -9202,6 +9213,7 @@ static int set_ro(const char *val, struct kernel_param *kp)
 module_param_call(start_ro, set_ro, get_ro, NULL, S_IRUSR|S_IWUSR);
 module_param(start_dirty_degraded, int, S_IRUGO|S_IWUSR);
 module_param_call(new_array, add_named_array, NULL, NULL, S_IWUSR);
+module_param(create_on_open, bool, S_IRUSR|S_IWUSR);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("MD RAID framework");

Re: [md PATCH 2/2] md: support disabling of create-on-open semantics.

[Coly Li]

On 2017/4/12 下午2:26, NeilBrown wrote:
> md allows a new array device to be created by simply
> opening a device file.  This make it difficult to
> remove the device and udev is likely to open the device file
> as part of processing the REMOVE event.
> 
> There is an alternate mechanism for creating arrays
> by writing to the new_array module parameter.
> When using tools that work with this parameter, it is
> best to disable the old semantics.
> This new module parameter allows that.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

Acked-by: Coly Li <colyli@suse.de>

> ---
>  drivers/md/md.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index c3d3bae947a1..a7ab769eacc3 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -174,6 +174,16 @@ static const struct block_device_operations md_fops;
>  
>  static int start_readonly;
>  
> +/*
> + * The original mechanism for creating an md device is to create
> + * a device node in /dev and to open it.  This causes races with device-close.
> + * The preferred method is to write to the "new_array" module parameter.
> + * This can avoid races.
> + * Setting create_on_open to false disables the original mechanism
> + * so all the races disappear.
> + */
> +static bool create_on_open = true;
> +
>  /* bio_clone_mddev
>   * like bio_clone_bioset, but with a local bio set
>   */
> @@ -5286,7 +5296,8 @@ static int md_alloc(dev_t dev, char *name)
>  
>  static struct kobject *md_probe(dev_t dev, int *part, void *data)
>  {
> -	md_alloc(dev, NULL);
> +	if (create_on_open)
> +		md_alloc(dev, NULL);
>  	return NULL;
>  }
>  
> @@ -9202,6 +9213,7 @@ static int set_ro(const char *val, struct kernel_param *kp)
>  module_param_call(start_ro, set_ro, get_ro, NULL, S_IRUSR|S_IWUSR);
>  module_param(start_dirty_degraded, int, S_IRUGO|S_IWUSR);
>  module_param_call(new_array, add_named_array, NULL, NULL, S_IWUSR);
> +module_param(create_on_open, bool, S_IRUSR|S_IWUSR);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("MD RAID framework");
> 
> 

[md PATCH 1/2] md: allow creation of mdNNN arrays via md_mod/parameters/new_array

[NeilBrown]

The intention when creating the "new_array" parameter and the
possibility of having array names line "md_HOME" was to transition
away from the old way of creating arrays and to eventually only use
this new way.

The "old" way of creating array is to create a device node in /dev
and then open it.  The act of opening creates the array.
This is problematic because sometimes the device node can be opened
when we don't want to create an array.  This can easily happen
when some rule triggered by udev looks at a device as it is being
destroyed.  The node in /dev continues to exist for a short period
after an array is stopped, and opening it during this time recreates
the array (as an inactive array).

Unfortunately no clear plan for the transition was created.  It is now
time to fix that.

This patch allows devices with numeric names, like "md999" to be
created by writing to "new_array".  This will only work if the minor
number given is not already in use.  This will allow mdadm to
support the creation of arrays with numbers > 511 (currently not
possible) by writing to new_array.
mdadm can, at some point, use this approach to create *all* arrays,
which will allow the transition to only using the new-way.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/md/md.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9fe930109012..c3d3bae947a1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5164,6 +5164,14 @@ static void no_op(struct percpu_ref *r) {}
 
 static int md_alloc(dev_t dev, char *name)
 {
+	/* If dev is zero, name is the name of a device to allocate with
+	 * an arbitrary minor number.  It will be "md_???"
+	 * If dev is non-zero it must be a device number with a MAJOR of
+	 * MD_MAJOR or mdp_major.  In this case, if "name" is NULL, then
+	 * the device is being created by opening a node in /dev.
+	 * If "name" is not NULL, the device is being created by
+	 * writing to /sys/module/md_mod/parameters/new_array.
+	 */
 	static DEFINE_MUTEX(disks_mutex);
 	struct mddev *mddev = mddev_find(dev);
 	struct gendisk *disk;
@@ -5189,7 +5197,7 @@ static int md_alloc(dev_t dev, char *name)
 	if (mddev->gendisk)
 		goto abort;
 
-	if (name) {
+	if (name && !dev) {
 		/* Need to ensure that 'name' is not a duplicate.
 		 */
 		struct mddev *mddev2;
@@ -5203,6 +5211,11 @@ static int md_alloc(dev_t dev, char *name)
 			}
 		spin_unlock(&all_mddevs_lock);
 	}
+	if (name && dev)
+		/*
+		 * Creating /dev/mdNNN via "newarray", so adjust hold_active.
+		 */
+		mddev->hold_active = UNTIL_STOP;
 
 	error = -ENOMEM;
 	mddev->queue = blk_alloc_queue(GFP_KERNEL);
@@ -5279,21 +5292,30 @@ static struct kobject *md_probe(dev_t dev, int *part, void *data)
 
 static int add_named_array(const char *val, struct kernel_param *kp)
 {
-	/* val must be "md_*" where * is not all digits.
-	 * We allocate an array with a large free minor number, and
+	/* val must be "md_*" or "mdNNN".
+	 * For "md_*" we allocate an array with a large free minor number, and
 	 * set the name to val.  val must not already be an active name.
+	 * For "mdNNN" we allocate an array with the minor number NNN
+	 * which must not already be in use.
 	 */
 	int len = strlen(val);
 	char buf[DISK_NAME_LEN];
+	unsigned long devnum;
 
 	while (len && val[len-1] == '\n')
 		len--;
 	if (len >= DISK_NAME_LEN)
 		return -E2BIG;
 	strlcpy(buf, val, len+1);
-	if (strncmp(buf, "md_", 3) != 0)
-		return -EINVAL;
-	return md_alloc(0, buf);
+	if (strncmp(buf, "md_", 3) == 0)
+		return md_alloc(0, buf);
+	if (strncmp(buf, "md", 2) == 0 &&
+	    isdigit(buf[2]) &&
+	    kstrtoul(buf+2, 10, &devnum) == 0 &&
+	    devnum <= MINORMASK)
+		return md_alloc(MKDEV(MD_MAJOR, devnum), NULL);
+
+	return -EINVAL;
 }
 
 static void md_safemode_timeout(unsigned long data)

Re: [md PATCH 1/2] md: allow creation of mdNNN arrays via md_mod/parameters/new_array

[Coly Li]

On 2017/4/12 下午2:26, NeilBrown wrote:
> The intention when creating the "new_array" parameter and the
> possibility of having array names line "md_HOME" was to transition
> away from the old way of creating arrays and to eventually only use
> this new way.
> 
> The "old" way of creating array is to create a device node in /dev
> and then open it.  The act of opening creates the array.
> This is problematic because sometimes the device node can be opened
> when we don't want to create an array.  This can easily happen
> when some rule triggered by udev looks at a device as it is being
> destroyed.  The node in /dev continues to exist for a short period
> after an array is stopped, and opening it during this time recreates
> the array (as an inactive array).
> 
> Unfortunately no clear plan for the transition was created.  It is now
> time to fix that.
> 
> This patch allows devices with numeric names, like "md999" to be
> created by writing to "new_array".  This will only work if the minor
> number given is not already in use.  This will allow mdadm to
> support the creation of arrays with numbers > 511 (currently not
> possible) by writing to new_array.
> mdadm can, at some point, use this approach to create *all* arrays,
> which will allow the transition to only using the new-way.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

Acted-by: Coly Li <colyli@suse.de>


> ---
>  drivers/md/md.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9fe930109012..c3d3bae947a1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5164,6 +5164,14 @@ static void no_op(struct percpu_ref *r) {}
>  
>  static int md_alloc(dev_t dev, char *name)
>  {
> +	/* If dev is zero, name is the name of a device to allocate with
> +	 * an arbitrary minor number.  It will be "md_???"
> +	 * If dev is non-zero it must be a device number with a MAJOR of
> +	 * MD_MAJOR or mdp_major.  In this case, if "name" is NULL, then
> +	 * the device is being created by opening a node in /dev.
> +	 * If "name" is not NULL, the device is being created by
> +	 * writing to /sys/module/md_mod/parameters/new_array.
> +	 */
>  	static DEFINE_MUTEX(disks_mutex);
>  	struct mddev *mddev = mddev_find(dev);
>  	struct gendisk *disk;
> @@ -5189,7 +5197,7 @@ static int md_alloc(dev_t dev, char *name)
>  	if (mddev->gendisk)
>  		goto abort;
>  
> -	if (name) {
> +	if (name && !dev) {
>  		/* Need to ensure that 'name' is not a duplicate.
>  		 */
>  		struct mddev *mddev2;
> @@ -5203,6 +5211,11 @@ static int md_alloc(dev_t dev, char *name)
>  			}
>  		spin_unlock(&all_mddevs_lock);
>  	}
> +	if (name && dev)
> +		/*
> +		 * Creating /dev/mdNNN via "newarray", so adjust hold_active.
> +		 */
> +		mddev->hold_active = UNTIL_STOP;
>  
>  	error = -ENOMEM;
>  	mddev->queue = blk_alloc_queue(GFP_KERNEL);
> @@ -5279,21 +5292,30 @@ static struct kobject *md_probe(dev_t dev, int *part, void *data)
>  
>  static int add_named_array(const char *val, struct kernel_param *kp)
>  {
> -	/* val must be "md_*" where * is not all digits.
> -	 * We allocate an array with a large free minor number, and
> +	/* val must be "md_*" or "mdNNN".
> +	 * For "md_*" we allocate an array with a large free minor number, and
>  	 * set the name to val.  val must not already be an active name.
> +	 * For "mdNNN" we allocate an array with the minor number NNN
> +	 * which must not already be in use.
>  	 */
>  	int len = strlen(val);
>  	char buf[DISK_NAME_LEN];
> +	unsigned long devnum;
>  
>  	while (len && val[len-1] == '\n')
>  		len--;
>  	if (len >= DISK_NAME_LEN)
>  		return -E2BIG;
>  	strlcpy(buf, val, len+1);
> -	if (strncmp(buf, "md_", 3) != 0)
> -		return -EINVAL;
> -	return md_alloc(0, buf);
> +	if (strncmp(buf, "md_", 3) == 0)
> +		return md_alloc(0, buf);
> +	if (strncmp(buf, "md", 2) == 0 &&
> +	    isdigit(buf[2]) &&
> +	    kstrtoul(buf+2, 10, &devnum) == 0 &&
> +	    devnum <= MINORMASK)
> +		return md_alloc(MKDEV(MD_MAJOR, devnum), NULL);
> +
> +	return -EINVAL;
>  }
>  
>  static void md_safemode_timeout(unsigned long data)
> 
> 

Re: [md PATCH 0/2] Make it possible to disable create_on_open semantics.

[Shaohua Li]

On Wed, Apr 12, 2017 at 04:26:12PM +1000, Neil Brown wrote:
> Currently, opening an md /dev node will create the array object.
> This makes it hard to destroy the object as udev will typically
> re-open the device node when handling REMOVE events.
> 
> The "new_array" module parameter was created to work towards avoiding
> this problem, and it can be used when
>   CREATE names=yes
> 
> is given in /etc/mdadm.conf.
> How this doesn't currently support names like "md%d", which lots of
> people use and expect, so we need more work before we can transition
> away from create_on_open.
> 
> These patches add support to "new_array" so that md%d devices
> can be created.  This will make it, once again, possible to have
> md%d devices with numbers > 511. (3.17 make this impossible).
> 
> An enhancement to mdadm that uses this will cause new_array to always
> be used (where available), and we can then disable create_on_open
> completely (after suitable transition periods).

Thanks, applied! The md device creation interface especially create_on_open is
a disaster, hopefully the future sysfs/configfs interface deprecates all of these.

Thanks,
Shaohua