[PATCH v2 0/2] Mdmonitor improvements

[PATCH v2 0/2] Mdmonitor improvements

[Kinga Tanska]

Series of patches contains fix segfault for running
monitor for devices different than md devices. Also
logging was improved to have informations about monitor
in verbose mode.

Kinga Tanska (2):
  Mdmonitor: Fix segfault
  Mdmonitor: Improve logging method

 Monitor.c | 36 ++++++++++++++++++++++++------------
 mdadm.h   |  1 +
 mdopen.c  | 17 +++++++++++++++++
 util.c    |  2 +-
 4 files changed, 43 insertions(+), 13 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] Mdmonitor: Fix segfault

[Kinga Tanska]

Mdadm with "--monitor" parameter requires md device
as an argument to be monitored. If given argument is
not a md device, error shall be returned. Previously
it was not checked and invalid argument caused
segmentation fault. This commit adds checking
that devices passed to mdmonitor are md devices.

Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
---
 Monitor.c | 11 ++++++++++-
 mdadm.h   |  1 +
 mdopen.c  | 17 +++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Monitor.c b/Monitor.c
index f5412299..0b24b656 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -183,6 +183,10 @@ int Monitor(struct mddev_dev *devlist,
 				continue;
 			if (strcasecmp(mdlist->devname, "<ignore>") == 0)
 				continue;
+
+			if (!is_mddev(mdlist->devname))
+				return 1;
+
 			st = xcalloc(1, sizeof *st);
 			if (mdlist->devname[0] == '/')
 				st->devname = xstrdup(mdlist->devname);
@@ -204,7 +208,12 @@ int Monitor(struct mddev_dev *devlist,
 		struct mddev_dev *dv;
 
 		for (dv = devlist; dv; dv = dv->next) {
-			struct state *st = xcalloc(1, sizeof *st);
+			struct state *st;
+
+			if (!is_mddev(dv->devname))
+				return 1;
+
+			st = xcalloc(1, sizeof *st);
 			mdlist = conf_get_ident(dv->devname);
 			st->devname = xstrdup(dv->devname);
 			st->next = statelist;
diff --git a/mdadm.h b/mdadm.h
index 8f8841d8..03151c34 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1607,6 +1607,7 @@ extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
 #define	FOREIGN	2
 #define	METADATA 3
 extern int open_mddev(char *dev, int report_errors);
+extern int is_mddev(char *dev);
 extern int open_container(int fd);
 extern int metadata_container_matches(char *metadata, char *devnm);
 extern int metadata_subdev_matches(char *metadata, char *devnm);
diff --git a/mdopen.c b/mdopen.c
index 245be537..d18c9319 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -475,6 +475,23 @@ int open_mddev(char *dev, int report_errors)
 	return mdfd;
 }
 
+/**
+ * is_mddev() - check that file name passed is an md device.
+ * @dev: file name that has to be checked.
+ * Return: 1 if file passed is an md device, 0 if not.
+ */
+int is_mddev(char *dev)
+{
+	int fd = open_mddev(dev, 1);
+
+	if (fd >= 0) {
+		close(fd);
+		return 1;
+	}
+
+	return 0;
+}
+
 char *find_free_devnm(int use_partitions)
 {
 	static char devnm[32];
-- 
2.26.2

[PATCH v2 2/2] Mdmonitor: Improve logging method

[Kinga Tanska]

Change logging, and as a result, mdmonitor in verbose
mode will report its configuration.

Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
---
 Monitor.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 0b24b656..bd417d04 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -137,24 +137,27 @@ int Monitor(struct mddev_dev *devlist,
 	struct mddev_ident *mdlist;
 	int delay_for_event = c->delay;
 
-	if (!mailaddr) {
+	if (!mailaddr)
 		mailaddr = conf_get_mailaddr();
-		if (mailaddr && ! c->scan)
-			pr_err("Monitor using email address \"%s\" from config file\n",
-			       mailaddr);
-	}
-	mailfrom = conf_get_mailfrom();
 
-	if (!alert_cmd) {
+	if (!alert_cmd)
 		alert_cmd = conf_get_program();
-		if (alert_cmd && !c->scan)
-			pr_err("Monitor using program \"%s\" from config file\n",
-			       alert_cmd);
-	}
+
+	mailfrom = conf_get_mailfrom();
+
 	if (c->scan && !mailaddr && !alert_cmd && !dosyslog) {
 		pr_err("No mail address or alert command - not monitoring.\n");
 		return 1;
 	}
+
+	if (c->verbose) {
+		pr_err("Monitor is started with delay %ds\n", c->delay);
+		if (mailaddr)
+			pr_err("Monitor using email address %s\n", mailaddr);
+		if (alert_cmd)
+			pr_err("Monitor using program %s\n", alert_cmd);
+	}
+
 	info.alert_cmd = alert_cmd;
 	info.mailaddr = mailaddr;
 	info.mailfrom = mailfrom;
-- 
2.26.2

Re: [PATCH v2 1/2] Mdmonitor: Fix segfault

[Coly Li]

On 4/5/22 7:40 PM, Kinga Tanska wrote:
> Mdadm with "--monitor" parameter requires md device
> as an argument to be monitored. If given argument is
> not a md device, error shall be returned. Previously
> it was not checked and invalid argument caused
> segmentation fault. This commit adds checking
> that devices passed to mdmonitor are md devices.
>
> Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
> Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
> ---
>   Monitor.c | 11 ++++++++++-
>   mdadm.h   |  1 +
>   mdopen.c  | 17 +++++++++++++++++
>   3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index f5412299..0b24b656 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -183,6 +183,10 @@ int Monitor(struct mddev_dev *devlist,
>   				continue;
>   			if (strcasecmp(mdlist->devname, "<ignore>") == 0)
>   				continue;
> +
> +			if (!is_mddev(mdlist->devname))
> +				return 1;
> +
>   			st = xcalloc(1, sizeof *st);
>   			if (mdlist->devname[0] == '/')
>   				st->devname = xstrdup(mdlist->devname);


HI Kinga,


Suddenly I realize that your test for the above change might not be 
complete.

mdlist->devname is permitted to be a relative path under /dev/md/, with 
your change such devname will be regarded as invalid by is_mddev(). 
Because open_mddev() only takes absolute path. Maybe is_mddev() should 
be called several lines behind current location.

The rested part of the patch is fine to me.


Thanks.


Coly Li

> @@ -204,7 +208,12 @@ int Monitor(struct mddev_dev *devlist,
>   		struct mddev_dev *dv;
>   
>   		for (dv = devlist; dv; dv = dv->next) {
> -			struct state *st = xcalloc(1, sizeof *st);
> +			struct state *st;
> +
> +			if (!is_mddev(dv->devname))
> +				return 1;
> +
> +			st = xcalloc(1, sizeof *st);
>   			mdlist = conf_get_ident(dv->devname);
>   			st->devname = xstrdup(dv->devname);
>   			st->next = statelist;
> diff --git a/mdadm.h b/mdadm.h
> index 8f8841d8..03151c34 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1607,6 +1607,7 @@ extern int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   #define	FOREIGN	2
>   #define	METADATA 3
>   extern int open_mddev(char *dev, int report_errors);
> +extern int is_mddev(char *dev);
>   extern int open_container(int fd);
>   extern int metadata_container_matches(char *metadata, char *devnm);
>   extern int metadata_subdev_matches(char *metadata, char *devnm);
> diff --git a/mdopen.c b/mdopen.c
> index 245be537..d18c9319 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -475,6 +475,23 @@ int open_mddev(char *dev, int report_errors)
>   	return mdfd;
>   }
>   
> +/**
> + * is_mddev() - check that file name passed is an md device.
> + * @dev: file name that has to be checked.
> + * Return: 1 if file passed is an md device, 0 if not.
> + */
> +int is_mddev(char *dev)
> +{
> +	int fd = open_mddev(dev, 1);
> +
> +	if (fd >= 0) {
> +		close(fd);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>   char *find_free_devnm(int use_partitions)
>   {
>   	static char devnm[32];

Re: [PATCH v2 2/2] Mdmonitor: Improve logging method

[Coly Li]

On 4/5/22 7:40 PM, Kinga Tanska wrote:
> Change logging, and as a result, mdmonitor in verbose
> mode will report its configuration.
>
> Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
> Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>


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


Thanks.


Coly Li


> ---
>   Monitor.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 0b24b656..bd417d04 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -137,24 +137,27 @@ int Monitor(struct mddev_dev *devlist,
>   	struct mddev_ident *mdlist;
>   	int delay_for_event = c->delay;
>   
> -	if (!mailaddr) {
> +	if (!mailaddr)
>   		mailaddr = conf_get_mailaddr();
> -		if (mailaddr && ! c->scan)
> -			pr_err("Monitor using email address \"%s\" from config file\n",
> -			       mailaddr);
> -	}
> -	mailfrom = conf_get_mailfrom();
>   
> -	if (!alert_cmd) {
> +	if (!alert_cmd)
>   		alert_cmd = conf_get_program();
> -		if (alert_cmd && !c->scan)
> -			pr_err("Monitor using program \"%s\" from config file\n",
> -			       alert_cmd);
> -	}
> +
> +	mailfrom = conf_get_mailfrom();
> +
>   	if (c->scan && !mailaddr && !alert_cmd && !dosyslog) {
>   		pr_err("No mail address or alert command - not monitoring.\n");
>   		return 1;
>   	}
> +
> +	if (c->verbose) {
> +		pr_err("Monitor is started with delay %ds\n", c->delay);
> +		if (mailaddr)
> +			pr_err("Monitor using email address %s\n", mailaddr);
> +		if (alert_cmd)
> +			pr_err("Monitor using program %s\n", alert_cmd);
> +	}
> +
>   	info.alert_cmd = alert_cmd;
>   	info.mailaddr = mailaddr;
>   	info.mailfrom = mailfrom;