[PATCH 0/3] Validation for names during creation

[PATCH 0/3] Validation for names during creation

[Mariusz Tkaczyk]

Hi Jes, Coly
Mdadm has to weak names policy and it is inconsistency with udev.
From IMSM side it also causes problem with VROC UEFI driver.

There is a small risk of regression because print_escape() is removed.
I think that these cases are incidental and can be fixed by updating
array name. All test passed.

Mariusz Tkaczyk (3):
  mdadm: create ident_init()
  mdadm: refactor ident->name handling
  Limit length and set of characters allowed of devname

 Detail.c        |  8 ++---
 config.c        | 81 ++++++++++++++++++++++++++++++++++---------------
 lib.c           | 80 +++++++++++++++++++++++++++++++++++++++---------
 mdadm.8.in      | 57 +++++++++++++++++-----------------
 mdadm.c         | 32 ++++---------------
 mdadm.conf.5.in |  4 ---
 mdadm.h         | 32 +++++++++++++------
 super-intel.c   | 25 +++++----------
 super1.c        |  3 +-
 util.c          | 24 +++++++++++++++
 10 files changed, 212 insertions(+), 134 deletions(-)

-- 
2.26.2

[PATCH 1/3] mdadm: create ident_init()

[Mariusz Tkaczyk]

Add a wrapper for repeated initializations in mdadm.c and config.c.
Move includes up.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 config.c | 45 +++++++++++++++++++++++++++++----------------
 mdadm.c  | 16 ++--------------
 mdadm.h  |  7 +++++--
 3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/config.c b/config.c
index dc1620c1..eeedd0c6 100644
--- a/config.c
+++ b/config.c
@@ -119,6 +119,34 @@ int match_keyword(char *word)
 	return -1;
 }
 
+/**
+ * ident_init() - Set defaults.
+ * @ident: ident pointer, not NULL.
+ */
+inline void ident_init(struct mddev_ident *ident)
+{
+	assert(ident);
+
+	ident->assembled = false;
+	ident->autof = 0;
+	ident->bitmap_fd = -1;
+	ident->bitmap_file = NULL;
+	ident->container = NULL;
+	ident->devices = NULL;
+	ident->devname = NULL;
+	ident->level = UnSet;
+	ident->member = NULL;
+	ident->name[0] = 0;
+	ident->next = NULL;
+	ident->raid_disks = UnSet;
+	ident->spare_group = NULL;
+	ident->spare_disks = 0;
+	ident->st = NULL;
+	ident->super_minor = UnSet;
+	ident->uuid[0] = 0;
+	ident->uuid_set = 0;
+}
+
 struct conf_dev {
 	struct conf_dev *next;
 	char *name;
@@ -363,22 +391,7 @@ void arrayline(char *line)
 	struct mddev_ident mis;
 	struct mddev_ident *mi;
 
-	mis.uuid_set = 0;
-	mis.super_minor = UnSet;
-	mis.level = UnSet;
-	mis.raid_disks = UnSet;
-	mis.spare_disks = 0;
-	mis.devices = NULL;
-	mis.devname = NULL;
-	mis.spare_group = NULL;
-	mis.autof = 0;
-	mis.next = NULL;
-	mis.st = NULL;
-	mis.bitmap_fd = -1;
-	mis.bitmap_file = NULL;
-	mis.name[0] = 0;
-	mis.container = NULL;
-	mis.member = NULL;
+	ident_init(&mis);
 
 	for (w = dl_next(line); w != line; w = dl_next(w)) {
 		if (w[0] == '/' || strchr(w, '=') == NULL) {
diff --git a/mdadm.c b/mdadm.c
index 972adb52..74fdec31 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -107,25 +107,13 @@ int main(int argc, char *argv[])
 
 	srandom(time(0) ^ getpid());
 
-	ident.uuid_set = 0;
-	ident.level = UnSet;
-	ident.raid_disks = UnSet;
-	ident.super_minor = UnSet;
-	ident.devices = 0;
-	ident.spare_group = NULL;
-	ident.autof = 0;
-	ident.st = NULL;
-	ident.bitmap_fd = -1;
-	ident.bitmap_file = NULL;
-	ident.name[0] = 0;
-	ident.container = NULL;
-	ident.member = NULL;
-
 	if (get_linux_version() < 2006015) {
 		pr_err("This version of mdadm does not support kernels older than 2.6.15\n");
 		exit(1);
 	}
 
+	ident_init(&ident);
+
 	while ((option_index = -1),
 	       (opt = getopt_long(argc, argv, shortopt, long_options,
 				  &option_index)) != -1) {
diff --git a/mdadm.h b/mdadm.h
index 3673494e..23ffe977 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -33,8 +33,10 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 # endif
 #endif
 
+#include	<assert.h>
 #include	<sys/types.h>
 #include	<sys/stat.h>
+#include	<stdarg.h>
 #include	<stdint.h>
 #include	<stdlib.h>
 #include	<time.h>
@@ -1552,6 +1554,8 @@ extern void enable_fds(int devices);
 extern void manage_fork_fds(int close_all);
 extern int continue_via_systemd(char *devnm, char *service_name);
 
+extern void ident_init(struct mddev_ident *ident);
+
 extern int parse_auto(char *str, char *msg, int config);
 extern struct mddev_ident *conf_get_ident(char *dev);
 extern struct mddev_dev *conf_get_devs(void);
@@ -1779,8 +1783,7 @@ static inline sighandler_t signal_s(int sig, sighandler_t handler)
 #define dprintf_cont(fmt, arg...) \
         ({ if (0) fprintf(stderr, fmt, ##arg); 0; })
 #endif
-#include <assert.h>
-#include <stdarg.h>
+
 static inline int xasprintf(char **strp, const char *fmt, ...) {
 	va_list ap;
 	int ret;
-- 
2.26.2

Re: [PATCH 1/3] mdadm: create ident_init()

[Jes Sorensen]

On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> Add a wrapper for repeated initializations in mdadm.c and config.c.
> Move includes up.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  config.c | 45 +++++++++++++++++++++++++++++----------------
>  mdadm.c  | 16 ++--------------
>  mdadm.h  |  7 +++++--
>  3 files changed, 36 insertions(+), 32 deletions(-)

Applied!

Thanks,
Jes

[PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

Move duplicated code for both config.c and mdadm.c to new functions.
Add error enum in mdadm.h. Use MD_NAME_MAX instead hardcoded value
in mddev_ident. Use secure functions.

In next patch POSIX validation is added.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 config.c | 35 +++++++++++++++++++++++++++--------
 lib.c    | 16 ++++++++++++++++
 mdadm.c  | 16 ++++------------
 mdadm.h  | 21 +++++++++++++++------
 util.c   | 20 ++++++++++++++++++++
 5 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/config.c b/config.c
index eeedd0c6..7d3eb6fc 100644
--- a/config.c
+++ b/config.c
@@ -147,6 +147,32 @@ inline void ident_init(struct mddev_ident *ident)
 	ident->uuid_set = 0;
 }
 
+/**
+ * ident_set_name()- set name in &mddev_ident.
+ * @ident: pointer to &mddev_ident.
+ * @name: name to be set.
+ *
+ * If criteria passed, set name in @ident.
+ *
+ * Return: %STATUS_SUCCESS or %STATUS_ERROR.
+ */
+status_t ident_set_name(struct mddev_ident *ident, const char *name)
+{
+	assert(name);
+	assert(ident);
+
+	if (ident->name[0]) {
+		pr_err("Name can be specified once, second value is '%s'.\n", name);
+		return STATUS_ERROR;
+	}
+
+	if (check_mdname(name) == STATUS_ERROR)
+		return STATUS_ERROR;
+
+	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
+	return STATUS_SUCCESS;
+}
+
 struct conf_dev {
 	struct conf_dev *next;
 	char *name;
@@ -445,14 +471,7 @@ void arrayline(char *line)
 					mis.super_minor = minor;
 			}
 		} else if (strncasecmp(w, "name=", 5) == 0) {
-			if (mis.name[0])
-				pr_err("only specify name once, %s ignored.\n",
-					w);
-			else if (strlen(w + 5) > 32)
-				pr_err("name too long, ignoring %s\n", w);
-			else
-				strcpy(mis.name, w + 5);
-
+			ident_set_name(&mis, w + 5);
 		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
 			if (mis.bitmap_file)
 				pr_err("only specify bitmap file once. %s ignored\n",
diff --git a/lib.c b/lib.c
index e395b28d..624580e1 100644
--- a/lib.c
+++ b/lib.c
@@ -27,6 +27,22 @@
 #include	<ctype.h>
 #include	<limits.h>
 
+/**
+ * is_strnlen_lq() - Check if string length is lower or equal to requested.
+ * @str: string to check.
+ * @max_len: max length.
+ *
+ * @str length must be bigger than 0 and be lower or equal @max_len, including termination byte.
+ */
+bool is_strnlen_lq(const char * const str, size_t max_len)
+{
+	assert(str);
+
+	size_t _len = strnlen(str, max_len);
+
+	return (_len < max_len && _len != 0);
+}
+
 bool is_dev_alive(char *path)
 {
 	if (!path)
diff --git a/mdadm.c b/mdadm.c
index 74fdec31..5bd3d5a7 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -686,20 +686,14 @@ int main(int argc, char *argv[])
 		case O(CREATE,'N'):
 		case O(ASSEMBLE,'N'):
 		case O(MISC,'N'):
-			if (ident.name[0]) {
-				pr_err("name cannot be set twice.   Second value %s.\n", optarg);
-				exit(2);
-			}
 			if (mode == MISC && !c.subarray) {
 				pr_err("-N/--name only valid with --update-subarray in misc mode\n");
 				exit(2);
 			}
-			if (strlen(optarg) > 32) {
-				pr_err("name '%s' is too long, 32 chars max.\n",
-					optarg);
+
+			if (ident_set_name(&ident, optarg) != STATUS_SUCCESS)
 				exit(2);
-			}
-			strcpy(ident.name, optarg);
+
 			continue;
 
 		case O(ASSEMBLE,'m'): /* super-minor for array */
@@ -1341,10 +1335,8 @@ int main(int argc, char *argv[])
 		} else {
 			char *bname = basename(devlist->devname);
 
-			if (strlen(bname) > MD_NAME_MAX) {
-				pr_err("Name %s is too long.\n", devlist->devname);
+			if (check_mdname(bname))
 				exit(1);
-			}
 
 			ret = stat(devlist->devname, &stb);
 			if (ident.super_minor == -2 && ret != 0) {
diff --git a/mdadm.h b/mdadm.h
index 23ffe977..b68fa4bc 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -275,6 +275,11 @@ static inline void __put_unaligned32(__u32 val, void *p)
 
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
+/**
+ * This is true for native and DDF, IMSM allows 16.
+ */
+#define MD_NAME_MAX 32
+
 extern const char Name[];
 
 struct md_bb_entry {
@@ -406,6 +411,12 @@ struct spare_criteria {
 	unsigned int sector_size;
 };
 
+typedef enum status {
+	STATUS_SUCCESS = 0,
+	STATUS_ERROR,
+	STATUS_UNDEF,
+} status_t;
+
 enum mode {
 	ASSEMBLE=1,
 	BUILD,
@@ -528,7 +539,7 @@ struct mddev_ident {
 
 	int	uuid_set;
 	int	uuid[4];
-	char	name[33];
+	char	name[MD_NAME_MAX + 1];
 
 	int super_minor;
 
@@ -1538,6 +1549,7 @@ extern int check_partitions(int fd, char *dname,
 extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
 extern int stat_is_blkdev(char *devname, dev_t *rdev);
 
+extern bool is_strnlen_lq(const char * const str, size_t max_len);
 extern bool is_dev_alive(char *path);
 extern int get_mdp_major(void);
 extern int get_maj_min(char *dev, int *major, int *minor);
@@ -1555,6 +1567,7 @@ extern void manage_fork_fds(int close_all);
 extern int continue_via_systemd(char *devnm, char *service_name);
 
 extern void ident_init(struct mddev_ident *ident);
+extern status_t ident_set_name(struct mddev_ident *ident, const char *name);
 
 extern int parse_auto(char *str, char *msg, int config);
 extern struct mddev_ident *conf_get_ident(char *dev);
@@ -1634,6 +1647,7 @@ extern void print_r10_layout(int layout);
 
 extern char *find_free_devnm(int use_partitions);
 
+extern error_t check_mdname(const char *name);
 extern void put_md_name(char *name);
 extern char *devid2kname(dev_t devid);
 extern char *devid2devnm(dev_t devid);
@@ -1923,11 +1937,6 @@ enum r0layout {
 /* And another special number needed for --data_offset=variable */
 #define VARIABLE_OFFSET 3
 
-/**
- * This is true for native and DDF, IMSM allows 16.
- */
-#define MD_NAME_MAX 32
-
 /**
  * is_container() - check if @level is &LEVEL_CONTAINER
  * @level: level value
diff --git a/util.c b/util.c
index 26ffdcea..b2a4ea21 100644
--- a/util.c
+++ b/util.c
@@ -932,6 +932,26 @@ int get_data_disks(int level, int layout, int raid_disks)
 	return data_disks;
 }
 
+/**
+ * check_md_name()- check if MD device name is correct.
+ * @name: name to check.
+ *
+ * In case of error, message is printed.
+ *
+ * Return: %STATUS_SUCCESS or %STATUS_ERROR.
+ */
+error_t check_mdname(const char * const name)
+{
+	assert(name);
+
+	if (!is_strnlen_lq(name, MD_NAME_MAX + 1)) {
+		pr_err("Name '%s' is too long or empty, %d characters max.\n", name, MD_NAME_MAX);
+		return STATUS_ERROR;
+	}
+
+	return STATUS_SUCCESS;
+}
+
 dev_t devnm2devid(char *devnm)
 {
 	/* First look in /sys/block/$DEVNM/dev for %d:%d
-- 
2.26.2

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Jes Sorensen]

On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> Move duplicated code for both config.c and mdadm.c to new functions.
> Add error enum in mdadm.h. Use MD_NAME_MAX instead hardcoded value
> in mddev_ident. Use secure functions.
> 
> In next patch POSIX validation is added.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Hi Mariusz,

I appreciate the work to consolidate duplicate code. However, I am not a
fan of new typedefs, in addition you return status_t codes in functions
changed to return error_t, which is inconsistent.

I would prefer if we move towards standard POSIX error codes instead of
trying to invent new ones.

Thanks,
Jes

> ---
>  config.c | 35 +++++++++++++++++++++++++++--------
>  lib.c    | 16 ++++++++++++++++
>  mdadm.c  | 16 ++++------------
>  mdadm.h  | 21 +++++++++++++++------
>  util.c   | 20 ++++++++++++++++++++
>  5 files changed, 82 insertions(+), 26 deletions(-)
> 
> diff --git a/config.c b/config.c
> index eeedd0c6..7d3eb6fc 100644
> --- a/config.c
> +++ b/config.c
> @@ -147,6 +147,32 @@ inline void ident_init(struct mddev_ident *ident)
>  	ident->uuid_set = 0;
>  }
>  
> +/**
> + * ident_set_name()- set name in &mddev_ident.
> + * @ident: pointer to &mddev_ident.
> + * @name: name to be set.
> + *
> + * If criteria passed, set name in @ident.
> + *
> + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> + */
> +status_t ident_set_name(struct mddev_ident *ident, const char *name)
> +{
> +	assert(name);
> +	assert(ident);
> +
> +	if (ident->name[0]) {
> +		pr_err("Name can be specified once, second value is '%s'.\n", name);
> +		return STATUS_ERROR;
> +	}
> +
> +	if (check_mdname(name) == STATUS_ERROR)
> +		return STATUS_ERROR;
> +
> +	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
> +	return STATUS_SUCCESS;
> +}
> +
>  struct conf_dev {
>  	struct conf_dev *next;
>  	char *name;
> @@ -445,14 +471,7 @@ void arrayline(char *line)
>  					mis.super_minor = minor;
>  			}
>  		} else if (strncasecmp(w, "name=", 5) == 0) {
> -			if (mis.name[0])
> -				pr_err("only specify name once, %s ignored.\n",
> -					w);
> -			else if (strlen(w + 5) > 32)
> -				pr_err("name too long, ignoring %s\n", w);
> -			else
> -				strcpy(mis.name, w + 5);
> -
> +			ident_set_name(&mis, w + 5);
>  		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
>  			if (mis.bitmap_file)
>  				pr_err("only specify bitmap file once. %s ignored\n",
> diff --git a/lib.c b/lib.c
> index e395b28d..624580e1 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -27,6 +27,22 @@
>  #include	<ctype.h>
>  #include	<limits.h>
>  
> +/**
> + * is_strnlen_lq() - Check if string length is lower or equal to requested.
> + * @str: string to check.
> + * @max_len: max length.
> + *
> + * @str length must be bigger than 0 and be lower or equal @max_len, including termination byte.
> + */
> +bool is_strnlen_lq(const char * const str, size_t max_len)
> +{
> +	assert(str);
> +
> +	size_t _len = strnlen(str, max_len);
> +
> +	return (_len < max_len && _len != 0);
> +}
> +
>  bool is_dev_alive(char *path)
>  {
>  	if (!path)
> diff --git a/mdadm.c b/mdadm.c
> index 74fdec31..5bd3d5a7 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -686,20 +686,14 @@ int main(int argc, char *argv[])
>  		case O(CREATE,'N'):
>  		case O(ASSEMBLE,'N'):
>  		case O(MISC,'N'):
> -			if (ident.name[0]) {
> -				pr_err("name cannot be set twice.   Second value %s.\n", optarg);
> -				exit(2);
> -			}
>  			if (mode == MISC && !c.subarray) {
>  				pr_err("-N/--name only valid with --update-subarray in misc mode\n");
>  				exit(2);
>  			}
> -			if (strlen(optarg) > 32) {
> -				pr_err("name '%s' is too long, 32 chars max.\n",
> -					optarg);
> +
> +			if (ident_set_name(&ident, optarg) != STATUS_SUCCESS)
>  				exit(2);
> -			}
> -			strcpy(ident.name, optarg);
> +
>  			continue;
>  
>  		case O(ASSEMBLE,'m'): /* super-minor for array */
> @@ -1341,10 +1335,8 @@ int main(int argc, char *argv[])
>  		} else {
>  			char *bname = basename(devlist->devname);
>  
> -			if (strlen(bname) > MD_NAME_MAX) {
> -				pr_err("Name %s is too long.\n", devlist->devname);
> +			if (check_mdname(bname))
>  				exit(1);
> -			}
>  
>  			ret = stat(devlist->devname, &stb);
>  			if (ident.super_minor == -2 && ret != 0) {
> diff --git a/mdadm.h b/mdadm.h
> index 23ffe977..b68fa4bc 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -275,6 +275,11 @@ static inline void __put_unaligned32(__u32 val, void *p)
>  
>  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
>  
> +/**
> + * This is true for native and DDF, IMSM allows 16.
> + */
> +#define MD_NAME_MAX 32
> +
>  extern const char Name[];
>  
>  struct md_bb_entry {
> @@ -406,6 +411,12 @@ struct spare_criteria {
>  	unsigned int sector_size;
>  };
>  
> +typedef enum status {
> +	STATUS_SUCCESS = 0,
> +	STATUS_ERROR,
> +	STATUS_UNDEF,
> +} status_t;
> +
>  enum mode {
>  	ASSEMBLE=1,
>  	BUILD,
> @@ -528,7 +539,7 @@ struct mddev_ident {
>  
>  	int	uuid_set;
>  	int	uuid[4];
> -	char	name[33];
> +	char	name[MD_NAME_MAX + 1];
>  
>  	int super_minor;
>  
> @@ -1538,6 +1549,7 @@ extern int check_partitions(int fd, char *dname,
>  extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
>  extern int stat_is_blkdev(char *devname, dev_t *rdev);
>  
> +extern bool is_strnlen_lq(const char * const str, size_t max_len);
>  extern bool is_dev_alive(char *path);
>  extern int get_mdp_major(void);
>  extern int get_maj_min(char *dev, int *major, int *minor);
> @@ -1555,6 +1567,7 @@ extern void manage_fork_fds(int close_all);
>  extern int continue_via_systemd(char *devnm, char *service_name);
>  
>  extern void ident_init(struct mddev_ident *ident);
> +extern status_t ident_set_name(struct mddev_ident *ident, const char *name);
>  
>  extern int parse_auto(char *str, char *msg, int config);
>  extern struct mddev_ident *conf_get_ident(char *dev);
> @@ -1634,6 +1647,7 @@ extern void print_r10_layout(int layout);
>  
>  extern char *find_free_devnm(int use_partitions);
>  
> +extern error_t check_mdname(const char *name);
>  extern void put_md_name(char *name);
>  extern char *devid2kname(dev_t devid);
>  extern char *devid2devnm(dev_t devid);
> @@ -1923,11 +1937,6 @@ enum r0layout {
>  /* And another special number needed for --data_offset=variable */
>  #define VARIABLE_OFFSET 3
>  
> -/**
> - * This is true for native and DDF, IMSM allows 16.
> - */
> -#define MD_NAME_MAX 32
> -
>  /**
>   * is_container() - check if @level is &LEVEL_CONTAINER
>   * @level: level v> 
> diff --git a/config.c b/config.c
> index dc1620c1..eeedd0c6 100644
> --- a/config.c
> +++ b/config.c
> @@ -119,6 +119,34 @@ int match_keyword(char *word)
>  	return -1;
>  }
>  
> +/**
> + * ident_init() - Set defaults.
> + * @ident: ident pointer, not NULL.
> + */
> +inline void ident_init(struct mddev_ident *ident)
> +{
> +	assert(ident);
> +
> +	ident->assembled = false;
> +	ident->autof = 0;
> +	ident->bitmap_fd = -1;
> +	ident->bitmap_file = NULL;
> +	ident->container = NULL;
> +	ident->devices = NULL;
> +	ident->devname = NULL;
> +	ident->level = UnSet;
> +	ident->member = NULL;
> +	ident->name[0] = 0;
> +	ident->next = NULL;
> +	ident->raid_disks = UnSet;
> +	ident->spare_group = NULL;
> +	ident->spare_disks = 0;
> +	ident->st = NULL;
> +	ident->super_minor = UnSet;
> +	ident->uuid[0] = 0;
> +	ident->uuid_set = 0;
> +}
> +
>  struct conf_dev {
>  	struct conf_dev *next;
>  	char *name;
> @@ -363,22 +391,7 @@ void arrayline(char *line)
>  	struct mddev_ident mis;
>  	struct mddev_ident *mi;
>  
> -	mis.uuid_set = 0;
> -	mis.super_minor = UnSet;
> -	mis.level = UnSet;
> -	mis.raid_disks = UnSet;
> -	mis.spare_disks = 0;
> -	mis.devices = NULL;
> -	mis.devname = NULL;
> -	mis.spare_group = NULL;
> -	mis.autof = 0;
> -	mis.next = NULL;
> -	mis.st = NULL;
> -	mis.bitmap_fd = -1;
> -	mis.bitmap_file = NULL;
> -	mis.name[0] = 0;
> -	mis.container = NULL;
> -	mis.member = NULL;
> +	ident_init(&mis);
>  
>  	for (w = dl_next(line); w != line; w = dl_next(w)) {
>  		if (w[0] == '/' || strchr(w, '=') == NULL) {
> diff --git a/mdadm.c b/mdadm.c
> index 972adb52..74fdec31 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -107,25 +107,13 @@ int main(int argc, char *argv[])
>  
>  	srandom(time(0) ^ getpid());
>  
> -	ident.uuid_set = 0;
> -	ident.level = UnSet;
> -	ident.raid_disks = UnSet;
> -	ident.super_minor = UnSet;
> -	ident.devices = 0;
> -	ident.spare_group = NULL;
> -	ident.autof = 0;
> -	ident.st = NULL;
> -	ident.bitmap_fd = -1;
> -	ident.bitmap_file = NULL;
> -	ident.name[0] = 0;
> -	ident.container = NULL;
> -	ident.member = NULL;
> -
>  	if (get_linux_version() < 2006015) {
>  		pr_err("This version of mdadm does not support kernels older than 2.6.15\n");
>  		exit(1);
>  	}
>  
> +	ident_init(&ident);
> +
>  	while ((option_index = -1),
>  	       (opt = getopt_long(argc, argv, shortopt, long_options,
>  				  &option_index)) != -1) {
> diff --git a/mdadm.h b/mdadm.h
> index 3673494e..23ffe977 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -33,8 +33,10 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>  # endif
>  #endif
>  
> +#include	<assert.h>
>  #include	<sys/types.h>
>  #include	<sys/stat.h>
> +#include	<stdarg.h>
>  #include	<stdint.h>
>  #include	<stdlib.h>
>  #include	<time.h>
> @@ -1552,6 +1554,8 @@ extern void enable_fds(int devices);
>  extern void manage_fork_fds(int close_all);
>  extern int continue_via_systemd(char *devnm, char *service_name);
>  
> +extern void ident_init(struct mddev_ident *ident);
> +
>  extern int parse_auto(char *str, char *msg, int config);
>  extern struct mddev_ident *conf_get_ident(char *dev);
>  extern struct mddev_dev *conf_get_devs(void);
> @@ -1779,8 +1783,7 @@ static inline sighandler_t signal_s(int sig, sighandler_t handler)
>  #define dprintf_cont(fmt, arg...) \
>          ({ if (0) fprintf(stderr, fmt, ##arg); 0; })
>  #endif
> -#include <assert.h>
> -#include <stdarg.h>
> +
>  static inline int xasprintf(char **strp, const char *fmt, ...) {
>  	va_list ap;
>  	int ret;alue
> diff --git a/util.c b/util.c
> index 26ffdcea..b2a4ea21 100644
> --- a/util.c
> +++ b/util.c
> @@ -932,6 +932,26 @@ int get_data_disks(int level, int layout, int raid_disks)
>  	return data_disks;
>  }
>  
> +/**
> + * check_md_name()- check if MD device name is correct.
> + * @name: name to check.
> + *
> + * In case of error, message is printed.
> + *
> + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> + */
> +error_t check_mdname(const char * const name)
> +{
> +	assert(name);
> +
> +	if (!is_strnlen_lq(name, MD_NAME_MAX + 1)) {
> +		pr_err("Name '%s' is too long or empty, %d characters max.\n", name, MD_NAME_MAX);
> +		return STATUS_ERROR;
> +	}
> +
> +	return STATUS_SUCCESS;
> +}
> +
>  dev_t devnm2devid(char *devnm)
>  {
>  	/* First look in /sys/block/$DEVNM/dev for %d:%d

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

On Wed, 28 Dec 2022 10:07:22 -0500
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> > Move duplicated code for both config.c and mdadm.c to new functions.
> > Add error enum in mdadm.h. Use MD_NAME_MAX instead hardcoded value
> > in mddev_ident. Use secure functions.
> > 
> > In next patch POSIX validation is added.
> > 
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>  
> 
> Hi Mariusz,
> 
> I appreciate the work to consolidate duplicate code. However, I am not a
> fan of new typedefs, in addition you return status_t codes in functions
> changed to return error_t, which is inconsistent.

Hi Jes,
Indeed, initially I named it as error_t and I forgot to update that part.
I'm surprised that compiler didn't catch it. Thanks!

About typedef, I did it same for IMSM already:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
I can change that but I wanted to define a common solution propagated later to
other mdadm parts.

> 
> I would prefer if we move towards standard POSIX error codes instead of
> trying to invent new ones.
> 

The POSIX errors are defined for communication with kernel space and
unfortunately they are not detailed enough. For example "undefined" or
just "general_error" statuses are not available.
https://man7.org/linux/man-pages/man3/errno.3.html
It the approach I proposed we are free to create exact errors we need.
Later we can create a map of error values to string and create dedicated
error print functions.

Thanks
Mariusz

> 
> > ---
> >  config.c | 35 +++++++++++++++++++++++++++--------
> >  lib.c    | 16 ++++++++++++++++
> >  mdadm.c  | 16 ++++------------
> >  mdadm.h  | 21 +++++++++++++++------
> >  util.c   | 20 ++++++++++++++++++++
> >  5 files changed, 82 insertions(+), 26 deletions(-)
> > 
> > diff --git a/config.c b/config.c
> > index eeedd0c6..7d3eb6fc 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -147,6 +147,32 @@ inline void ident_init(struct mddev_ident *ident)
> >  	ident->uuid_set = 0;
> >  }
> >  
> > +/**
> > + * ident_set_name()- set name in &mddev_ident.
> > + * @ident: pointer to &mddev_ident.
> > + * @name: name to be set.
> > + *
> > + * If criteria passed, set name in @ident.
> > + *
> > + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> > + */
> > +status_t ident_set_name(struct mddev_ident *ident, const char *name)
> > +{
> > +	assert(name);
> > +	assert(ident);
> > +
> > +	if (ident->name[0]) {
> > +		pr_err("Name can be specified once, second value is
> > '%s'.\n", name);
> > +		return STATUS_ERROR;
> > +	}
> > +
> > +	if (check_mdname(name) == STATUS_ERROR)
> > +		return STATUS_ERROR;
> > +
> > +	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
> > +	return STATUS_SUCCESS;
> > +}
> > +
> >  struct conf_dev {
> >  	struct conf_dev *next;
> >  	char *name;
> > @@ -445,14 +471,7 @@ void arrayline(char *line)
> >  					mis.super_minor = minor;
> >  			}
> >  		} else if (strncasecmp(w, "name=", 5) == 0) {
> > -			if (mis.name[0])
> > -				pr_err("only specify name once, %s
> > ignored.\n",
> > -					w);
> > -			else if (strlen(w + 5) > 32)
> > -				pr_err("name too long, ignoring %s\n", w);
> > -			else
> > -				strcpy(mis.name, w + 5);
> > -
> > +			ident_set_name(&mis, w + 5);
> >  		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
> >  			if (mis.bitmap_file)
> >  				pr_err("only specify bitmap file once. %s
> > ignored\n", diff --git a/lib.c b/lib.c
> > index e395b28d..624580e1 100644
> > --- a/lib.c
> > +++ b/lib.c
> > @@ -27,6 +27,22 @@
> >  #include	<ctype.h>
> >  #include	<limits.h>
> >  
> > +/**
> > + * is_strnlen_lq() - Check if string length is lower or equal to requested.
> > + * @str: string to check.
> > + * @max_len: max length.
> > + *
> > + * @str length must be bigger than 0 and be lower or equal @max_len,
> > including termination byte.
> > + */
> > +bool is_strnlen_lq(const char * const str, size_t max_len)
> > +{
> > +	assert(str);
> > +
> > +	size_t _len = strnlen(str, max_len);
> > +
> > +	return (_len < max_len && _len != 0);
> > +}
> > +
> >  bool is_dev_alive(char *path)
> >  {
> >  	if (!path)
> > diff --git a/mdadm.c b/mdadm.c
> > index 74fdec31..5bd3d5a7 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -686,20 +686,14 @@ int main(int argc, char *argv[])
> >  		case O(CREATE,'N'):
> >  		case O(ASSEMBLE,'N'):
> >  		case O(MISC,'N'):
> > -			if (ident.name[0]) {
> > -				pr_err("name cannot be set twice.   Second
> > value %s.\n", optarg);
> > -				exit(2);
> > -			}
> >  			if (mode == MISC && !c.subarray) {
> >  				pr_err("-N/--name only valid with
> > --update-subarray in misc mode\n"); exit(2);
> >  			}
> > -			if (strlen(optarg) > 32) {
> > -				pr_err("name '%s' is too long, 32 chars
> > max.\n",
> > -					optarg);
> > +
> > +			if (ident_set_name(&ident, optarg) !=
> > STATUS_SUCCESS) exit(2);
> > -			}
> > -			strcpy(ident.name, optarg);
> > +
> >  			continue;
> >  
> >  		case O(ASSEMBLE,'m'): /* super-minor for array */
> > @@ -1341,10 +1335,8 @@ int main(int argc, char *argv[])
> >  		} else {
> >  			char *bname = basename(devlist->devname);
> >  
> > -			if (strlen(bname) > MD_NAME_MAX) {
> > -				pr_err("Name %s is too long.\n",
> > devlist->devname);
> > +			if (check_mdname(bname))
> >  				exit(1);
> > -			}
> >  
> >  			ret = stat(devlist->devname, &stb);
> >  			if (ident.super_minor == -2 && ret != 0) {
> > diff --git a/mdadm.h b/mdadm.h
> > index 23ffe977..b68fa4bc 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -275,6 +275,11 @@ static inline void __put_unaligned32(__u32 val, void
> > *p) 
> >  #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
> >  
> > +/**
> > + * This is true for native and DDF, IMSM allows 16.
> > + */
> > +#define MD_NAME_MAX 32
> > +
> >  extern const char Name[];
> >  
> >  struct md_bb_entry {
> > @@ -406,6 +411,12 @@ struct spare_criteria {
> >  	unsigned int sector_size;
> >  };
> >  
> > +typedef enum status {
> > +	STATUS_SUCCESS = 0,
> > +	STATUS_ERROR,
> > +	STATUS_UNDEF,
> > +} status_t;
> > +
> >  enum mode {
> >  	ASSEMBLE=1,
> >  	BUILD,
> > @@ -528,7 +539,7 @@ struct mddev_ident {
> >  
> >  	int	uuid_set;
> >  	int	uuid[4];
> > -	char	name[33];
> > +	char	name[MD_NAME_MAX + 1];
> >  
> >  	int super_minor;
> >  
> > @@ -1538,6 +1549,7 @@ extern int check_partitions(int fd, char *dname,
> >  extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
> >  extern int stat_is_blkdev(char *devname, dev_t *rdev);
> >  
> > +extern bool is_strnlen_lq(const char * const str, size_t max_len);
> >  extern bool is_dev_alive(char *path);
> >  extern int get_mdp_major(void);
> >  extern int get_maj_min(char *dev, int *major, int *minor);
> > @@ -1555,6 +1567,7 @@ extern void manage_fork_fds(int close_all);
> >  extern int continue_via_systemd(char *devnm, char *service_name);
> >  
> >  extern void ident_init(struct mddev_ident *ident);
> > +extern status_t ident_set_name(struct mddev_ident *ident, const char
> > *name); 
> >  extern int parse_auto(char *str, char *msg, int config);
> >  extern struct mddev_ident *conf_get_ident(char *dev);
> > @@ -1634,6 +1647,7 @@ extern void print_r10_layout(int layout);
> >  
> >  extern char *find_free_devnm(int use_partitions);
> >  
> > +extern error_t check_mdname(const char *name);
> >  extern void put_md_name(char *name);
> >  extern char *devid2kname(dev_t devid);
> >  extern char *devid2devnm(dev_t devid);
> > @@ -1923,11 +1937,6 @@ enum r0layout {
> >  /* And another special number needed for --data_offset=variable */
> >  #define VARIABLE_OFFSET 3
> >  
> > -/**
> > - * This is true for native and DDF, IMSM allows 16.
> > - */
> > -#define MD_NAME_MAX 32
> > -
> >  /**
> >   * is_container() - check if @level is &LEVEL_CONTAINER
> >   * @level: level v> 
> > diff --git a/config.c b/config.c
> > index dc1620c1..eeedd0c6 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -119,6 +119,34 @@ int match_keyword(char *word)
> >  	return -1;
> >  }
> >  
> > +/**
> > + * ident_init() - Set defaults.
> > + * @ident: ident pointer, not NULL.
> > + */
> > +inline void ident_init(struct mddev_ident *ident)
> > +{
> > +	assert(ident);
> > +
> > +	ident->assembled = false;
> > +	ident->autof = 0;
> > +	ident->bitmap_fd = -1;
> > +	ident->bitmap_file = NULL;
> > +	ident->container = NULL;
> > +	ident->devices = NULL;
> > +	ident->devname = NULL;
> > +	ident->level = UnSet;
> > +	ident->member = NULL;
> > +	ident->name[0] = 0;
> > +	ident->next = NULL;
> > +	ident->raid_disks = UnSet;
> > +	ident->spare_group = NULL;
> > +	ident->spare_disks = 0;
> > +	ident->st = NULL;
> > +	ident->super_minor = UnSet;
> > +	ident->uuid[0] = 0;
> > +	ident->uuid_set = 0;
> > +}
> > +
> >  struct conf_dev {
> >  	struct conf_dev *next;
> >  	char *name;
> > @@ -363,22 +391,7 @@ void arrayline(char *line)
> >  	struct mddev_ident mis;
> >  	struct mddev_ident *mi;
> >  
> > -	mis.uuid_set = 0;
> > -	mis.super_minor = UnSet;
> > -	mis.level = UnSet;
> > -	mis.raid_disks = UnSet;
> > -	mis.spare_disks = 0;
> > -	mis.devices = NULL;
> > -	mis.devname = NULL;
> > -	mis.spare_group = NULL;
> > -	mis.autof = 0;
> > -	mis.next = NULL;
> > -	mis.st = NULL;
> > -	mis.bitmap_fd = -1;
> > -	mis.bitmap_file = NULL;
> > -	mis.name[0] = 0;
> > -	mis.container = NULL;
> > -	mis.member = NULL;
> > +	ident_init(&mis);
> >  
> >  	for (w = dl_next(line); w != line; w = dl_next(w)) {
> >  		if (w[0] == '/' || strchr(w, '=') == NULL) {
> > diff --git a/mdadm.c b/mdadm.c
> > index 972adb52..74fdec31 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -107,25 +107,13 @@ int main(int argc, char *argv[])
> >  
> >  	srandom(time(0) ^ getpid());
> >  
> > -	ident.uuid_set = 0;
> > -	ident.level = UnSet;
> > -	ident.raid_disks = UnSet;
> > -	ident.super_minor = UnSet;
> > -	ident.devices = 0;
> > -	ident.spare_group = NULL;
> > -	ident.autof = 0;
> > -	ident.st = NULL;
> > -	ident.bitmap_fd = -1;
> > -	ident.bitmap_file = NULL;
> > -	ident.name[0] = 0;
> > -	ident.container = NULL;
> > -	ident.member = NULL;
> > -
> >  	if (get_linux_version() < 2006015) {
> >  		pr_err("This version of mdadm does not support kernels
> > older than 2.6.15\n"); exit(1);
> >  	}
> >  
> > +	ident_init(&ident);
> > +
> >  	while ((option_index = -1),
> >  	       (opt = getopt_long(argc, argv, shortopt, long_options,
> >  				  &option_index)) != -1) {
> > diff --git a/mdadm.h b/mdadm.h
> > index 3673494e..23ffe977 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -33,8 +33,10 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t
> > __offset, int __whence)); # endif
> >  #endif
> >  
> > +#include	<assert.h>
> >  #include	<sys/types.h>
> >  #include	<sys/stat.h>
> > +#include	<stdarg.h>
> >  #include	<stdint.h>
> >  #include	<stdlib.h>
> >  #include	<time.h>
> > @@ -1552,6 +1554,8 @@ extern void enable_fds(int devices);
> >  extern void manage_fork_fds(int close_all);
> >  extern int continue_via_systemd(char *devnm, char *service_name);
> >  
> > +extern void ident_init(struct mddev_ident *ident);
> > +
> >  extern int parse_auto(char *str, char *msg, int config);
> >  extern struct mddev_ident *conf_get_ident(char *dev);
> >  extern struct mddev_dev *conf_get_devs(void);
> > @@ -1779,8 +1783,7 @@ static inline sighandler_t signal_s(int sig,
> > sighandler_t handler) #define dprintf_cont(fmt, arg...) \
> >          ({ if (0) fprintf(stderr, fmt, ##arg); 0; })
> >  #endif
> > -#include <assert.h>
> > -#include <stdarg.h>
> > +
> >  static inline int xasprintf(char **strp, const char *fmt, ...) {
> >  	va_list ap;
> >  	int ret;alue
> > diff --git a/util.c b/util.c
> > index 26ffdcea..b2a4ea21 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -932,6 +932,26 @@ int get_data_disks(int level, int layout, int
> > raid_disks) return data_disks;
> >  }
> >  
> > +/**
> > + * check_md_name()- check if MD device name is correct.
> > + * @name: name to check.
> > + *
> > + * In case of error, message is printed.
> > + *
> > + * Return: %STATUS_SUCCESS or %STATUS_ERROR.
> > + */
> > +error_t check_mdname(const char * const name)
> > +{
> > +	assert(name);
> > +
> > +	if (!is_strnlen_lq(name, MD_NAME_MAX + 1)) {
> > +		pr_err("Name '%s' is too long or empty, %d characters
> > max.\n", name, MD_NAME_MAX);
> > +		return STATUS_ERROR;
> > +	}
> > +
> > +	return STATUS_SUCCESS;
> > +}
> > +
> >  dev_t devnm2devid(char *devnm)
> >  {
> >  	/* First look in /sys/block/$DEVNM/dev for %d:%d  
> 

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

On Thu, 29 Dec 2022 10:39:31 +0100
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:

> On Wed, 28 Dec 2022 10:07:22 -0500
> Jes Sorensen <jes@trained-monkey.org> wrote:
> 
> > On 12/21/22 06:50, Mariusz Tkaczyk wrote:  
> > > Move duplicated code for both config.c and mdadm.c to new functions.
> > > Add error enum in mdadm.h. Use MD_NAME_MAX instead hardcoded value
> > > in mddev_ident. Use secure functions.
> > > 
> > > In next patch POSIX validation is added.
> > > 
> > > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>    
> > 
> > Hi Mariusz,
> > 
> > I appreciate the work to consolidate duplicate code. However, I am not a
> > fan of new typedefs, in addition you return status_t codes in functions
> > changed to return error_t, which is inconsistent.  
> 
> Hi Jes,
> Indeed, initially I named it as error_t and I forgot to update that part.
> I'm surprised that compiler didn't catch it. Thanks!
> 
> About typedef, I did it same for IMSM already:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
> I can change that but I wanted to define a common solution propagated later to
> other mdadm parts.
> 
> > 
> > I would prefer if we move towards standard POSIX error codes instead of
> > trying to invent new ones.
> >   
> 
> The POSIX errors are defined for communication with kernel space and
> unfortunately they are not detailed enough. For example "undefined" or
> just "general_error" statuses are not available.
> https://man7.org/linux/man-pages/man3/errno.3.html
> It the approach I proposed we are free to create exact errors we need.
> Later we can create a map of error values to string and create dedicated
> error print functions.
> 

Hi Jes,
Should I drop this enum in v2?

Thanks,
Mariusz

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Jes Sorensen]

On 12/29/22 04:39, Mariusz Tkaczyk wrote:

Hi Mariusz,

Apologies for the slow response on this one.

> On Wed, 28 Dec 2022 10:07:22 -0500
> Jes Sorensen <jes@trained-monkey.org> wrote:

>> I appreciate the work to consolidate duplicate code. However, I am not a
>> fan of new typedefs, in addition you return status_t codes in functions
>> changed to return error_t, which is inconsistent.
> 
> Hi Jes,
> Indeed, initially I named it as error_t and I forgot to update that part.
> I'm surprised that compiler didn't catch it. Thanks!
> 
> About typedef, I did it same for IMSM already:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
> I can change that but I wanted to define a common solution propagated later to
> other mdadm parts.

I am really on the fence on this one. I'd very much like to see us get
away from the nasty 0/1/2 error codes we currently have all over the
place, but I am also vary of reinventing the wheel.

I must admit I missed it in super-intel.c

>> I would prefer if we move towards standard POSIX error codes instead of
>> trying to invent new ones.
>>
> 
> The POSIX errors are defined for communication with kernel space and
> unfortunately they are not detailed enough. For example "undefined" or
> just "general_error" statuses are not available.
> https://man7.org/linux/man-pages/man3/errno.3.html
> It the approach I proposed we are free to create exact errors we need.
> Later we can create a map of error values to string and create dedicated
> error print functions.

I agree that POSIX codes aren't perfect, however at least for the
current errors I see reported in this patch -EINVAL or -E2BIG ought to
cover. If you think there are many cases where we cannot map well to
POSIX, then I would be OK with it, but I would prefer to go straight to
a global error code space rather than one per subsystem.

Thoughts?

Thanks,
Jes

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

On Thu, 2 Mar 2023 09:52:31 -0500
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 12/29/22 04:39, Mariusz Tkaczyk wrote:
> 
> Hi Mariusz,
> 
> Apologies for the slow response on this one.
> 
> > On Wed, 28 Dec 2022 10:07:22 -0500
> > Jes Sorensen <jes@trained-monkey.org> wrote:  
> 
> >> I appreciate the work to consolidate duplicate code. However, I am not a
> >> fan of new typedefs, in addition you return status_t codes in functions
> >> changed to return error_t, which is inconsistent.  
> > 
> > Hi Jes,
> > Indeed, initially I named it as error_t and I forgot to update that part.
> > I'm surprised that compiler didn't catch it. Thanks!
> > 
> > About typedef, I did it same for IMSM already:
> > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
> > I can change that but I wanted to define a common solution propagated later
> > to other mdadm parts.  
> 
> I am really on the fence on this one. I'd very much like to see us get
> away from the nasty 0/1/2 error codes we currently have all over the
> place, but I am also vary of reinventing the wheel.
> 
> I must admit I missed it in super-intel.c
> 
> >> I would prefer if we move towards standard POSIX error codes instead of
> >> trying to invent new ones.
> >>  
> > 
> > The POSIX errors are defined for communication with kernel space and
> > unfortunately they are not detailed enough. For example "undefined" or
> > just "general_error" statuses are not available.
> > https://man7.org/linux/man-pages/man3/errno.3.html
> > It the approach I proposed we are free to create exact errors we need.
> > Later we can create a map of error values to string and create dedicated
> > error print functions.  
> 
> I agree that POSIX codes aren't perfect, however at least for the
> current errors I see reported in this patch -EINVAL or -E2BIG ought to
> cover. If you think there are many cases where we cannot map well to
> POSIX, then I would be OK with it, but I would prefer to go straight to
> a global error code space rather than one per subsystem.
> 
> Thoughts?
> 
Hi Jes,

I was in this place with ledmon project:
https://github.com/intel/ledmon/blob/master/src/status.h

Yeah, it seems to be overhead to maintain error enum per each subsystem yet, you
are right here. I don't plan huge code reactor to make those enums common used.
If we don't plan mdadm library, then there is no need to handle define multiple
enums. It has real sense if we want to hide some statuses from library user
inside our code.

I would like to handle internal error definitions because I think that mdadm
deserves flexibility to define status what it needs. In general case we needs
just *error* but when it comes to more advanced flows I can see multiple
meaningful statuses we need to differentiate. The goal here is to make errors
straightforward.

I don't consider it as reinventing the wheel because the software is free
to define error handlers as it wants. There are no restrictions and POSIX codes
are not a solution. POSIX codes are available to us because we need to
understand kernel error codes and we need to handle them and react
appropriately.

Simply, I would like to have error possibly the best self described and the
error enum allows me to achieve that. It also forces developers to
follow the statuses we have there because if something like that is defined,
there is high probability that maintainer will ask developer to use this enum
in new function and use meaningful error codes respectively.

I would like to add this enum to mdadm.h but I will avoid to adding more enum
in the future if there is no need to. Let me know if that works for you.

Thanks for discussion!
Mariusz

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Jes Sorensen]

On 3/3/23 07:04, Mariusz Tkaczyk wrote:
> On Thu, 2 Mar 2023 09:52:31 -0500
> Jes Sorensen <jes@trained-monkey.org> wrote:
> 
>> On 12/29/22 04:39, Mariusz Tkaczyk wrote:
>>
>> Hi Mariusz,
>>
>> Apologies for the slow response on this one.
>>
>>> On Wed, 28 Dec 2022 10:07:22 -0500
>>> Jes Sorensen <jes@trained-monkey.org> wrote:  
>>
>>>> I appreciate the work to consolidate duplicate code. However, I am not a
>>>> fan of new typedefs, in addition you return status_t codes in functions
>>>> changed to return error_t, which is inconsistent.  
>>>
>>> Hi Jes,
>>> Indeed, initially I named it as error_t and I forgot to update that part.
>>> I'm surprised that compiler didn't catch it. Thanks!
>>>
>>> About typedef, I did it same for IMSM already:
>>> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
>>> I can change that but I wanted to define a common solution propagated later
>>> to other mdadm parts.  
>>
>> I am really on the fence on this one. I'd very much like to see us get
>> away from the nasty 0/1/2 error codes we currently have all over the
>> place, but I am also vary of reinventing the wheel.
>>
>> I must admit I missed it in super-intel.c
>>
>>>> I would prefer if we move towards standard POSIX error codes instead of
>>>> trying to invent new ones.
>>>>  
>>>
>>> The POSIX errors are defined for communication with kernel space and
>>> unfortunately they are not detailed enough. For example "undefined" or
>>> just "general_error" statuses are not available.
>>> https://man7.org/linux/man-pages/man3/errno.3.html
>>> It the approach I proposed we are free to create exact errors we need.
>>> Later we can create a map of error values to string and create dedicated
>>> error print functions.  
>>
>> I agree that POSIX codes aren't perfect, however at least for the
>> current errors I see reported in this patch -EINVAL or -E2BIG ought to
>> cover. If you think there are many cases where we cannot map well to
>> POSIX, then I would be OK with it, but I would prefer to go straight to
>> a global error code space rather than one per subsystem.
>>
>> Thoughts?
>>
> Hi Jes,
> 
> I was in this place with ledmon project:
> https://github.com/intel/ledmon/blob/master/src/status.h
> 
> Yeah, it seems to be overhead to maintain error enum per each subsystem yet, you
> are right here. I don't plan huge code reactor to make those enums common used.
> If we don't plan mdadm library, then there is no need to handle define multiple
> enums. It has real sense if we want to hide some statuses from library user
> inside our code.
> 
> I would like to handle internal error definitions because I think that mdadm
> deserves flexibility to define status what it needs. In general case we needs
> just *error* but when it comes to more advanced flows I can see multiple
> meaningful statuses we need to differentiate. The goal here is to make errors
> straightforward.
> 
> I don't consider it as reinventing the wheel because the software is free
> to define error handlers as it wants. There are no restrictions and POSIX codes
> are not a solution. POSIX codes are available to us because we need to
> understand kernel error codes and we need to handle them and react
> appropriately.
> 
> Simply, I would like to have error possibly the best self described and the
> error enum allows me to achieve that. It also forces developers to
> follow the statuses we have there because if something like that is defined,
> there is high probability that maintainer will ask developer to use this enum
> in new function and use meaningful error codes respectively.
> 
> I would like to add this enum to mdadm.h but I will avoid to adding more enum
> in the future if there is no need to. Let me know if that works for you.

Sounds fair!

Maybe it would be worth starting the enum outside the range of the
regular errno so they can overlap? Not sure if it adds any value, just a
thought.

Cheers,
Jes

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

On Wed, 8 Mar 2023 14:04:12 -0500
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 3/3/23 07:04, Mariusz Tkaczyk wrote:
> > On Thu, 2 Mar 2023 09:52:31 -0500
> > Jes Sorensen <jes@trained-monkey.org> wrote:
> >   
> >> On 12/29/22 04:39, Mariusz Tkaczyk wrote:
> >>
> >> Hi Mariusz,
> >>
> >> Apologies for the slow response on this one.
> >>  
> >>> On Wed, 28 Dec 2022 10:07:22 -0500
> >>> Jes Sorensen <jes@trained-monkey.org> wrote:    
> >>  
> >>>> I appreciate the work to consolidate duplicate code. However, I am not a
> >>>> fan of new typedefs, in addition you return status_t codes in functions
> >>>> changed to return error_t, which is inconsistent.    
> >>>
> >>> Hi Jes,
> >>> Indeed, initially I named it as error_t and I forgot to update that part.
> >>> I'm surprised that compiler didn't catch it. Thanks!
> >>>
> >>> About typedef, I did it same for IMSM already:
> >>> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/super-intel.c#n376
> >>> I can change that but I wanted to define a common solution propagated
> >>> later to other mdadm parts.    
> >>
> >> I am really on the fence on this one. I'd very much like to see us get
> >> away from the nasty 0/1/2 error codes we currently have all over the
> >> place, but I am also vary of reinventing the wheel.
> >>
> >> I must admit I missed it in super-intel.c
> >>  
> >>>> I would prefer if we move towards standard POSIX error codes instead of
> >>>> trying to invent new ones.
> >>>>    
> >>>
> >>> The POSIX errors are defined for communication with kernel space and
> >>> unfortunately they are not detailed enough. For example "undefined" or
> >>> just "general_error" statuses are not available.
> >>> https://man7.org/linux/man-pages/man3/errno.3.html
> >>> It the approach I proposed we are free to create exact errors we need.
> >>> Later we can create a map of error values to string and create dedicated
> >>> error print functions.    
> >>
> >> I agree that POSIX codes aren't perfect, however at least for the
> >> current errors I see reported in this patch -EINVAL or -E2BIG ought to
> >> cover. If you think there are many cases where we cannot map well to
> >> POSIX, then I would be OK with it, but I would prefer to go straight to
> >> a global error code space rather than one per subsystem.
> >>
> >> Thoughts?
> >>  
> > Hi Jes,
> > 
> > I was in this place with ledmon project:
> > https://github.com/intel/ledmon/blob/master/src/status.h
> > 
> > Yeah, it seems to be overhead to maintain error enum per each subsystem
> > yet, you are right here. I don't plan huge code reactor to make those enums
> > common used. If we don't plan mdadm library, then there is no need to
> > handle define multiple enums. It has real sense if we want to hide some
> > statuses from library user inside our code.
> > 
> > I would like to handle internal error definitions because I think that mdadm
> > deserves flexibility to define status what it needs. In general case we
> > needs just *error* but when it comes to more advanced flows I can see
> > multiple meaningful statuses we need to differentiate. The goal here is to
> > make errors straightforward.
> > 
> > I don't consider it as reinventing the wheel because the software is free
> > to define error handlers as it wants. There are no restrictions and POSIX
> > codes are not a solution. POSIX codes are available to us because we need to
> > understand kernel error codes and we need to handle them and react
> > appropriately.
> > 
> > Simply, I would like to have error possibly the best self described and the
> > error enum allows me to achieve that. It also forces developers to
> > follow the statuses we have there because if something like that is defined,
> > there is high probability that maintainer will ask developer to use this
> > enum in new function and use meaningful error codes respectively.
> > 
> > I would like to add this enum to mdadm.h but I will avoid to adding more
> > enum in the future if there is no need to. Let me know if that works for
> > you.  
> 
> Sounds fair!
> 
> Maybe it would be worth starting the enum outside the range of the
> regular errno so they can overlap? Not sure if it adds any value, just a
> thought.
> 
I see no value either.
What if someone will add new error definition to errno? Should we change our
enum start then? I think that nobody will notice it until issue but it is
unlikely too. For that reason I think that it is pointless from the beggining
because we are defnining rule which won't be honored later.

I think that we can just to redefine errno codes in enum if they are needed. We
are free to change particular enum constant value to make room for errno
compatible codes if there will be need to. That should be safe if some code
does not have ugly trick like calling external function and comparing it with
enum constant:

*/ let's say that SUCCESS is 0 */
if (strncmp(arg, arg1, arg2) == ENUM_STATUS_SUCCESS)

but it is our job to catch it on review so we are safe here, right? :)

Thanks,
Mariusz

Re: [PATCH 2/3] mdadm: refactor ident->name handling

[Jes Sorensen]

On 3/9/23 03:02, Mariusz Tkaczyk wrote:
> On Wed, 8 Mar 2023 14:04:12 -0500
>> Maybe it would be worth starting the enum outside the range of the
>> regular errno so they can overlap? Not sure if it adds any value, just a
>> thought.
>>
> I see no value either.
> What if someone will add new error definition to errno? Should we change our
> enum start then? I think that nobody will notice it until issue but it is
> unlikely too. For that reason I think that it is pointless from the beggining
> because we are defnining rule which won't be honored later.
> 
> I think that we can just to redefine errno codes in enum if they are needed. We
> are free to change particular enum constant value to make room for errno
> compatible codes if there will be need to. That should be safe if some code
> does not have ugly trick like calling external function and comparing it with
> enum constant:
> 
> */ let's say that SUCCESS is 0 */
> if (strncmp(arg, arg1, arg2) == ENUM_STATUS_SUCCESS)
> 
> but it is our job to catch it on review so we are safe here, right? :)

Fair enough

Jes

[PATCH 3/3] Limit length and set of characters allowed of devname

[Mariusz Tkaczyk]

When the user creates a device with a name that contains whitespace,
mdadm timeouts and throws an error. This issue is caused by udev, which
truncates /dev/md link until the first whitespace.

This patch introduces prohibition of characters other than A-Za-z0-9.-_
in the device name. Also, it prohibits using leading "-" in device name,
so name won't be confused with cli parameter.
Set of allowed characters is taken from POSIX 3.280 Portable Character
Set. Also, device name length now is limited to NAME_MAX.

In some places there are other requirements for string length (e.g. size
up to MD_NAME_MAX for device name). This routine is made to follow POSIX
and other, more strict limitations should be checked separately.
We are aware of the risk of regression in exceptional cases (as
escape_devname function is removed) that should be fixed by updating
the array name.

The posix validation is added for:
- 'name' parameter in every mode.
- secondary device name (first devlist entry), only for create and
assembly.

To limit regression risk, config entries are excluded from POSIX
validation.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Detail.c        |  8 ++-----
 config.c        |  7 +++---
 lib.c           | 64 +++++++++++++++++++++++++++++++++++++------------
 mdadm.8.in      | 57 +++++++++++++++++++++----------------------
 mdadm.c         |  4 ++--
 mdadm.conf.5.in |  4 ----
 mdadm.h         |  8 ++++---
 super-intel.c   | 25 ++++++-------------
 super1.c        |  3 +--
 util.c          |  6 ++++-
 10 files changed, 102 insertions(+), 84 deletions(-)

diff --git a/Detail.c b/Detail.c
index ce7a8445..b206159e 100644
--- a/Detail.c
+++ b/Detail.c
@@ -256,9 +256,7 @@ int Detail(char *dev, struct context *c)
 			mp = map_by_uuid(&map, info->uuid);
 			if (mp && mp->path &&
 			    strncmp(mp->path, "/dev/md/", 8) == 0) {
-				printf("MD_DEVNAME=");
-				print_escape(mp->path + 8);
-				putchar('\n');
+				printf("MD_DEVNAME=%s\n", mp->path + 8);
 			}
 
 			if (st->ss->export_detail_super)
@@ -274,9 +272,7 @@ int Detail(char *dev, struct context *c)
 			}
 			if (mp && mp->path &&
 			    strncmp(mp->path, "/dev/md/", 8) == 0) {
-				printf("MD_DEVNAME=");
-				print_escape(mp->path+8);
-				putchar('\n');
+				printf("MD_DEVNAME=%s\n", mp->path + 8);
 			}
 			map_free(map);
 		}
diff --git a/config.c b/config.c
index 7d3eb6fc..3b7a0921 100644
--- a/config.c
+++ b/config.c
@@ -151,12 +151,13 @@ inline void ident_init(struct mddev_ident *ident)
  * ident_set_name()- set name in &mddev_ident.
  * @ident: pointer to &mddev_ident.
  * @name: name to be set.
+ * @check_posix: if set, verify compatibility with POSIX.
  *
  * If criteria passed, set name in @ident.
  *
  * Return: %STATUS_SUCCESS or %STATUS_ERROR.
  */
-status_t ident_set_name(struct mddev_ident *ident, const char *name)
+status_t ident_set_name(struct mddev_ident *ident, const char *name, const bool check_posix)
 {
 	assert(name);
 	assert(ident);
@@ -166,7 +167,7 @@ status_t ident_set_name(struct mddev_ident *ident, const char *name)
 		return STATUS_ERROR;
 	}
 
-	if (check_mdname(name) == STATUS_ERROR)
+	if (check_mdname(name, check_posix) == STATUS_ERROR)
 		return STATUS_ERROR;
 
 	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
@@ -471,7 +472,7 @@ void arrayline(char *line)
 					mis.super_minor = minor;
 			}
 		} else if (strncasecmp(w, "name=", 5) == 0) {
-			ident_set_name(&mis, w + 5);
+			ident_set_name(&mis, w + 5, false);
 		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
 			if (mis.bitmap_file)
 				pr_err("only specify bitmap file once. %s ignored\n",
diff --git a/lib.c b/lib.c
index 624580e1..58ebfb83 100644
--- a/lib.c
+++ b/lib.c
@@ -481,24 +481,58 @@ void print_quoted(char *str)
 	putchar(q);
 }
 
-void print_escape(char *str)
+/**
+ * is_alphanum() - Check if sign is letter or digit.
+ * @c: char to analyze.
+ *
+ * Similar to isalnum() but additional locales are excluded.
+ *
+ * Return: 1 on success, 0 on fail, accordingly to isalnum().
+ */
+bool is_alphanum(const char c)
 {
-	/* print str, but change space and tab to '_'
-	 * as is suitable for device names
-	 */
-	for (; *str; str++) {
-		switch (*str) {
-		case ' ':
-		case '\t':
-			putchar('_');
-			break;
-		case '/':
-			putchar('-');
-			break;
-		default:
-			putchar(*str);
+	return isupper(c) || islower(c) || isdigit(c);
+}
+
+/**
+ * is_name_posix_compatible() - Check if name is POSIX compatible.
+ * @name: name to check.
+ *
+ *  POSIX portable file name character set contains ASCII letters,
+ *  digits, '_', '.', and '-'. Also forbid leading '-'.
+ *  The length of the name cannot exceed NAME_MAX - 1 (ensure NULL ending).
+ *
+ * Return: %true on success, %false otherwise.
+ */
+bool is_name_posix_compatible(const char * const name)
+{
+	assert(name);
+
+	char allowed_symbols[] = "-_.";
+	char *reason = NULL;
+	const char *n = name;
+
+	if (!is_strnlen_lq(name, NAME_MAX)) {
+		reason = "Wrong size";
+		goto err;
+	}
+	if (*name == '-') {
+		reason = "Leading '-' detected";
+		goto err;
+	}
+
+	while (*n != '\0') {
+		if (!is_alphanum(*n) && !strchr(allowed_symbols, *n)) {
+			reason = "Not allowed symbol detected";
+			goto err;
 		}
+
+		n++;
 	}
+	return true;
+err:
+	pr_err("Name \"%s\" in not POSIX compatible, reason: %s.\n", name, reason);
+	return false;
 }
 
 int check_env(char *name)
diff --git a/mdadm.8.in b/mdadm.8.in
index 70c79d1e..046fb71f 100644
--- a/mdadm.8.in
+++ b/mdadm.8.in
@@ -916,17 +916,19 @@ option will be ignored.
 .BR \-N ", " \-\-name=
 Set a
 .B name
-for the array.  This is currently only effective when creating an
-array with a version-1 superblock, or an array in a DDF container.
+for the array. This is currently only effective when creating an
+array with a version-1 superblock, or an array in a external container.
 The name is a simple textual string that can be used to identify array
-components when assembling.  If name is needed but not specified, it
-is taken from the basename of the device that is being created.
-e.g. when creating
-.I /dev/md/home
-the
-.B name
-will default to
-.IR home .
+components when assembling.
+
+A valid name can only consist of characters "A-Za-z0-9.-_".
+The name cannot start with a leading "-" and cannot exceed NAME_MAX.
+The long name could be truncated or rejected, it depends of strict checks for
+metadata type.
+
+If name is needed but not specified, it is taken from the basename of the device
+that is being created. See
+.BR "DEVICE NAMES"
 
 .TP
 .BR \-R ", " \-\-run
@@ -2163,14 +2165,17 @@ Usage:
 .I md-device
 .BI \-\-chunk= X
 .BI \-\-level= Y
-.br
 .BI \-\-raid\-devices= Z
 .I devices
 
 .PP
-This usage will initialise a new md array, associate some devices with
+This usage will initialize a new md array, associate some devices with
 it, and activate the array.
 
+.I md-device
+is a new device. This could be standard name or chosen name. For details see:
+.BR "DEVICE NAMES"
+
 The named device will normally not exist when
 .I "mdadm \-\-create"
 is run, but will be created by
@@ -2211,24 +2216,6 @@ array.  This feature can be overridden with the
 .B \-\-force
 option.
 
-When creating an array with version-1 metadata a name for the array is
-required.
-If this is not given with the
-.B \-\-name
-option,
-.I mdadm
-will choose a name based on the last component of the name of the
-device being created.  So if
-.B /dev/md3
-is being created, then the name
-.B 3
-will be chosen.
-If
-.B /dev/md/home
-is being created, then the name
-.B home
-will be used.
-
 When creating a partition based array, using
 .I mdadm
 with version-1.x metadata, the partition type should be set to
@@ -2420,6 +2407,11 @@ re\-assembled.  If updating
 would change the UUID of an active subarray this operation is blocked,
 and the command will end in an error.
 
+A valid name can only consist of characters "A-Za-z0-9.-_".
+The name cannot start with a leading "-" and cannot exceed NAME_MAX.
+The long name could be truncated or rejected, it depends of strict checks for
+metadata type.
+
 The
 .B ppl
 and
@@ -3366,6 +3358,11 @@ can be given, or just the suffix of the second sort of name, such as
 .I home
 can be given.
 
+A valid name can only consist of characters "A-Za-z0-9.-_".
+The name cannot start with a leading "-" and cannot exceed NAME_MAX.
+The long name could be truncated or rejected, it depends of strict checks for
+metadata type.
+
 When
 .I mdadm
 chooses device names during auto-assembly or incremental assembly, it
diff --git a/mdadm.c b/mdadm.c
index 5bd3d5a7..08754c15 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -691,7 +691,7 @@ int main(int argc, char *argv[])
 				exit(2);
 			}
 
-			if (ident_set_name(&ident, optarg) != STATUS_SUCCESS)
+			if (ident_set_name(&ident, optarg, true) != STATUS_SUCCESS)
 				exit(2);
 
 			continue;
@@ -1335,7 +1335,7 @@ int main(int argc, char *argv[])
 		} else {
 			char *bname = basename(devlist->devname);
 
-			if (check_mdname(bname))
+			if (check_mdname(bname, true))
 				exit(1);
 
 			ret = stat(devlist->devname, &stb);
diff --git a/mdadm.conf.5.in b/mdadm.conf.5.in
index bc2295c2..94e23dd0 100644
--- a/mdadm.conf.5.in
+++ b/mdadm.conf.5.in
@@ -717,10 +717,6 @@ ARRAY /dev/md/home UUID=9187a482:5dde19d9:eea3cc4a:d646ab8b
 .br
            auto=part
 .br
-# The name of this array contains a space.
-.br
-ARRAY /dev/md9 name='Data Storage'
-.sp
 POLICY domain=domain1 metadata=imsm path=pci-0000:00:1f.2-scsi-*
 .br
            action=spare
diff --git a/mdadm.h b/mdadm.h
index b68fa4bc..74628aaa 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1546,6 +1546,7 @@ extern int check_raid(int fd, char *name);
 extern int check_partitions(int fd, char *dname,
 			    unsigned long long freesize,
 			    unsigned long long size);
+extern bool is_name_posix_compatible(const char *path);
 extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
 extern int stat_is_blkdev(char *devname, dev_t *rdev);
 
@@ -1567,7 +1568,9 @@ extern void manage_fork_fds(int close_all);
 extern int continue_via_systemd(char *devnm, char *service_name);
 
 extern void ident_init(struct mddev_ident *ident);
-extern status_t ident_set_name(struct mddev_ident *ident, const char *name);
+extern status_t ident_set_name(struct mddev_ident *ident, const char *name,
+			       bool check_posix);
+
 
 extern int parse_auto(char *str, char *msg, int config);
 extern struct mddev_ident *conf_get_ident(char *dev);
@@ -1585,7 +1588,6 @@ extern int conf_get_monitor_delay(void);
 extern char *conf_line(FILE *file);
 extern char *conf_word(FILE *file, int allow_key);
 extern void print_quoted(char *str);
-extern void print_escape(char *str);
 extern int use_udev(void);
 extern unsigned long GCD(unsigned long a, unsigned long b);
 extern int conf_name_is_free(char *name);
@@ -1647,7 +1649,7 @@ extern void print_r10_layout(int layout);
 
 extern char *find_free_devnm(int use_partitions);
 
-extern error_t check_mdname(const char *name);
+extern error_t check_mdname(const char *name, const bool check_posix);
 extern void put_md_name(char *name);
 extern char *devid2kname(dev_t devid);
 extern char *devid2devnm(dev_t devid);
diff --git a/super-intel.c b/super-intel.c
index b0565610..da1e5062 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5480,26 +5480,14 @@ static void imsm_update_version_info(struct intel_super *super)
 	}
 }
 
-static int check_name(struct intel_super *super, char *name, int quiet)
+static int imsm_check_name(struct intel_super *super, char *name, int quiet)
 {
 	struct imsm_super *mpb = super->anchor;
 	char *reason = NULL;
-	char *start = name;
-	size_t len = strlen(name);
 	int i;
 
-	if (len > 0) {
-		while (isspace(start[len - 1]))
-			start[--len] = 0;
-		while (*start && isspace(*start))
-			++start, --len;
-		memmove(name, start, len + 1);
-	}
-
-	if (len > MAX_RAID_SERIAL_LEN)
-		reason = "must be 16 characters or less";
-	else if (len == 0)
-		reason = "must be a non-empty string";
+	if (strnlen(name, MAX_RAID_SERIAL_LEN + 1) > MAX_RAID_SERIAL_LEN)
+		reason = "is too long";
 
 	for (i = 0; i < mpb->num_raid_devs; i++) {
 		struct imsm_dev *dev = get_imsm_dev(super, i);
@@ -5608,7 +5596,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 		}
 	}
 
-	if (!check_name(super, name, 0))
+	if (!imsm_check_name(super, name, 0))
 		return 0;
 	dv = xmalloc(sizeof(*dv));
 	dev = xcalloc(1, sizeof(*dev) + sizeof(__u32) * (info->raid_disks - 1));
@@ -7919,7 +7907,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 			return 2;
 		}
 
-		if (!check_name(super, name, 0))
+		if (!imsm_check_name(super, name, 0))
 			return 2;
 
 		vol = strtoul(subarray, &ep, 10);
@@ -10246,7 +10234,8 @@ static void imsm_process_update(struct supertype *st,
 			if (a->info.container_member == target)
 				break;
 		dev = get_imsm_dev(super, u->dev_idx);
-		if (a || !check_name(super, name, 1)) {
+
+		if (a || !dev || !imsm_check_name(super, name, 1)) {
 			dprintf("failed to rename subarray-%d\n", target);
 			break;
 		}
diff --git a/super1.c b/super1.c
index 0b505a7e..11cffc75 100644
--- a/super1.c
+++ b/super1.c
@@ -642,8 +642,7 @@ static void brief_examine_super1(struct supertype *st, int verbose)
 
 	printf("ARRAY ");
 	if (nm) {
-		printf("/dev/md/");
-		print_escape(nm);
+		printf("/dev/md/%s", nm);
 		putchar(' ');
 	}
 	if (verbose && c)
diff --git a/util.c b/util.c
index b2a4ea21..1de1eddc 100644
--- a/util.c
+++ b/util.c
@@ -935,12 +935,13 @@ int get_data_disks(int level, int layout, int raid_disks)
 /**
  * check_md_name()- check if MD device name is correct.
  * @name: name to check.
+ * @check_posix: if set, verify compatibility with POSIX.
  *
  * In case of error, message is printed.
  *
  * Return: %STATUS_SUCCESS or %STATUS_ERROR.
  */
-error_t check_mdname(const char * const name)
+error_t check_mdname(const char * const name, const bool check_posix)
 {
 	assert(name);
 
@@ -949,6 +950,9 @@ error_t check_mdname(const char * const name)
 		return STATUS_ERROR;
 	}
 
+	if (check_posix && !is_name_posix_compatible(name))
+		return STATUS_ERROR;
+
 	return STATUS_SUCCESS;
 }
 
-- 
2.26.2

Re: [PATCH 3/3] Limit length and set of characters allowed of devname

[Jes Sorensen]

On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> When the user creates a device with a name that contains whitespace,
> mdadm timeouts and throws an error. This issue is caused by udev, which
> truncates /dev/md link until the first whitespace.
> 
> This patch introduces prohibition of characters other than A-Za-z0-9.-_
> in the device name. Also, it prohibits using leading "-" in device name,
> so name won't be confused with cli parameter.
> Set of allowed characters is taken from POSIX 3.280 Portable Character
> Set. Also, device name length now is limited to NAME_MAX.
> 
> In some places there are other requirements for string length (e.g. size
> up to MD_NAME_MAX for device name). This routine is made to follow POSIX
> and other, more strict limitations should be checked separately.
> We are aware of the risk of regression in exceptional cases (as
> escape_devname function is removed) that should be fixed by updating
> the array name.
> 
> The posix validation is added for:
> - 'name' parameter in every mode.
> - secondary device name (first devlist entry), only for create and
> assembly.
> 
> To limit regression risk, config entries are excluded from POSIX
> validation.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Hi Mariusz,

This no longer applies cleanly. Any chance you can post an updated version?

Thanks,
Jes

Re: [PATCH 3/3] Limit length and set of characters allowed of devname

[Mariusz Tkaczyk]

On Mon, 13 Mar 2023 10:22:47 -0400
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 12/21/22 06:50, Mariusz Tkaczyk wrote:
> > When the user creates a device with a name that contains whitespace,
> > mdadm timeouts and throws an error. This issue is caused by udev, which
> > truncates /dev/md link until the first whitespace.
> > 
> > This patch introduces prohibition of characters other than A-Za-z0-9.-_
> > in the device name. Also, it prohibits using leading "-" in device name,
> > so name won't be confused with cli parameter.
> > Set of allowed characters is taken from POSIX 3.280 Portable Character
> > Set. Also, device name length now is limited to NAME_MAX.
> > 
> > In some places there are other requirements for string length (e.g. size
> > up to MD_NAME_MAX for device name). This routine is made to follow POSIX
> > and other, more strict limitations should be checked separately.
> > We are aware of the risk of regression in exceptional cases (as
> > escape_devname function is removed) that should be fixed by updating
> > the array name.
> > 
> > The posix validation is added for:
> > - 'name' parameter in every mode.
> > - secondary device name (first devlist entry), only for create and
> > assembly.
> > 
> > To limit regression risk, config entries are excluded from POSIX
> > validation.
> > 
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>  
> 
> Hi Mariusz,
> 
> This no longer applies cleanly. Any chance you can post an updated version?
> 

Hi Jes,
Working on next version. I think that I omitted one place, need to dig into
again. I should make a test for that too.

Thanks,
Mariusz