[PATCH] fix: do not overwrite existing devices' symlinks

Subsystems: the rest

STALE5299d

5 messages, 2 authors, 2012-01-31 · open the first message on its own page

[PATCH] fix: do not overwrite existing devices' symlinks

From: Lukasz Dorau <hidden>
Date: 2012-01-23 12:06:52

If the device's name is given in /etc/mdadm.conf, create_mddev()
does not check if the map contains a device of this name (mdopen.c:140).
If it does, the symlink of that name will be overwritten.

create_mddev() has been changed. Now it checks if the map contains
a device of the name given in /etc/mdadm.conf.
If it does, the appropriate suffix is added to the given name.

Signed-off-by: Lukasz Dorau <redacted>
---
 mdopen.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/mdopen.c b/mdopen.c
index eac1c1f..3078de6 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	char *cname;
 	char devname[20];
 	char cbuf[400];
+	struct map_ent *map = NULL;
+	int dev_conflict = 0;
+
 	if (chosen == NULL)
 		chosen = cbuf;
 
-
 	if (autof == 0)
 		autof = ci->autof;
 
@@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	else
 		sprintf(devname, "/dev/md%d", num);
 
-	if (cname[0] == 0 && name) {
+	if ((cname[0] != 0) && map_by_name(&map, cname))
+		dev_conflict = 1;
+
+	if ((cname[0] == 0 && name) || dev_conflict) {
 		/* Need to find a name if we can
 		 * We don't completely trust 'name'.  Truncate to
 		 * reasonable length and remove '/'
 		 */
 		char *cp;
-		struct map_ent *map = NULL;
 		int conflict = 1;
 		int unum = 0;
 		int cnlen;
-		strncpy(cname, name, 200);
+
+		if (!dev_conflict)
+			strncpy(cname, name, 200);
 		cname[200] = 0;
 		while ((cp = strchr(cname, '/')) != NULL)
 			*cp = '-';
@@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 		}
 	}
 
-	if (dev && dev[0] == '/')
+	if ((dev && dev[0] == '/') && (!dev_conflict))
 		strcpy(chosen, dev);
 	else if (cname[0] == 0)
 		strcpy(chosen, devname);

Re: [PATCH] fix: do not overwrite existing devices' symlinks

From: NeilBrown <hidden>
Date: 2012-01-30 01:27:28

On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau [off-list ref]
wrote:
If the device's name is given in /etc/mdadm.conf, create_mddev()
does not check if the map contains a device of this name (mdopen.c:140).
If it does, the symlink of that name will be overwritten.

create_mddev() has been changed. Now it checks if the map contains
a device of the name given in /etc/mdadm.conf.
If it does, the appropriate suffix is added to the given name.

Signed-off-by: Lukasz Dorau <redacted>
Can you please remind me what the big picture problem is here??

It seem like you are suggesting that if
   /dev/md/thing

is given in mdadm.conf, but some other array is already assembled with the
name /dev/md/thing, then the array from mdadm.conf should be assembled as
   /dev/md/thing0
or something like that - is that correct?

I don't think we want that.  If there is a name conflict like  this with a
name given in mdadm.conf, then one of the arrays should fail to assemble as
this is really a fairly serious configuration error.

Or did I misunderstand?

Thanks,
NeilBrown


quoted hunk
---
 mdopen.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/mdopen.c b/mdopen.c
index eac1c1f..3078de6 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	char *cname;
 	char devname[20];
 	char cbuf[400];
+	struct map_ent *map = NULL;
+	int dev_conflict = 0;
+
 	if (chosen == NULL)
 		chosen = cbuf;
 
-
 	if (autof == 0)
 		autof = ci->autof;
 
@@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	else
 		sprintf(devname, "/dev/md%d", num);
 
-	if (cname[0] == 0 && name) {
+	if ((cname[0] != 0) && map_by_name(&map, cname))
+		dev_conflict = 1;
+
+	if ((cname[0] == 0 && name) || dev_conflict) {
 		/* Need to find a name if we can
 		 * We don't completely trust 'name'.  Truncate to
 		 * reasonable length and remove '/'
 		 */
 		char *cp;
-		struct map_ent *map = NULL;
 		int conflict = 1;
 		int unum = 0;
 		int cnlen;
-		strncpy(cname, name, 200);
+
+		if (!dev_conflict)
+			strncpy(cname, name, 200);
 		cname[200] = 0;
 		while ((cp = strchr(cname, '/')) != NULL)
 			*cp = '-';
@@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 		}
 	}
 
-	if (dev && dev[0] == '/')
+	if ((dev && dev[0] == '/') && (!dev_conflict))
 		strcpy(chosen, dev);
 	else if (cname[0] == 0)
 		strcpy(chosen, devname);

RE: [PATCH] fix: do not overwrite existing devices' symlinks

From: Dorau, Lukasz <hidden>
Date: 2012-01-30 12:13:05

-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:27 AM
To: Dorau, Lukasz
Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski,
Ed
Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks

On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau [off-list ref]
wrote:
quoted
If the device's name is given in /etc/mdadm.conf, create_mddev()
does not check if the map contains a device of this name (mdopen.c:140).
If it does, the symlink of that name will be overwritten.

create_mddev() has been changed. Now it checks if the map contains
a device of the name given in /etc/mdadm.conf.
If it does, the appropriate suffix is added to the given name.

Signed-off-by: Lukasz Dorau <redacted>
Can you please remind me what the big picture problem is here??

It seem like you are suggesting that if
   /dev/md/thing

is given in mdadm.conf, but some other array is already assembled with the
name /dev/md/thing, then the array from mdadm.conf should be assembled as
   /dev/md/thing0
or something like that - is that correct?

I don't think we want that.  If there is a name conflict like  this with a
name given in mdadm.conf, then one of the arrays should fail to assemble as
this is really a fairly serious configuration error.

Or did I misunderstand?
-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:30 AM
To: Dorau, Lukasz
Subject: Re: mdadm: checking dev's names from mdadm.conf - question

On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz" [off-list ref]
wrote:
quoted
Hi

Is it OK that mdadm does not check if a symlink of the name given in
/etc/mdadm.conf already exists (in function create_mddev() in mdopen.c:140) ?
quoted
For example:
If we modify the original /etc/mdadm.conf file:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY /dev/md/imsm0 metadata=imsm
UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
quoted
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
mdadm will create the first container /dev/md127 and the symlink of default
name /dev/md/imsm0 (and the first volume /dev/md126 with symlink
/dev/md/vol1).
quoted
Later it will create the second container /dev/md125 and the symlink of the
name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of
the first container.
quoted
Mdadm does not check if the symlink of the given name already exists and it
_overwrites_ the first symlink. Is it OK or maybe should it be corrected?
quoted
Ahhh.. this is where the big-picture bit is...

I don't have a big problem with it over-writing the symlink - that is what
you asked for in a way.

However I also wouldn't have a problem with mdadm refusing the assemble the
second container as its name is already in use.

So, are you going to apply this patch or do you want it to be fixed in another way?

Regards,
Lukasz

quoted
---
 mdopen.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/mdopen.c b/mdopen.c
index eac1c1f..3078de6 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -147,10 +147,12 @@ int create_mddev(char *dev, char *name, int autof,
int trustworthy,
quoted
 	char *cname;
 	char devname[20];
 	char cbuf[400];
+	struct map_ent *map = NULL;
+	int dev_conflict = 0;
+
 	if (chosen == NULL)
 		chosen = cbuf;

-
 	if (autof == 0)
 		autof = ci->autof;
@@ -277,17 +279,21 @@ int create_mddev(char *dev, char *name, int autof,
int trustworthy,
quoted
 	else
 		sprintf(devname, "/dev/md%d", num);

-	if (cname[0] == 0 && name) {
+	if ((cname[0] != 0) && map_by_name(&map, cname))
+		dev_conflict = 1;
+
+	if ((cname[0] == 0 && name) || dev_conflict) {
 		/* Need to find a name if we can
 		 * We don't completely trust 'name'.  Truncate to
 		 * reasonable length and remove '/'
 		 */
 		char *cp;
-		struct map_ent *map = NULL;
 		int conflict = 1;
 		int unum = 0;
 		int cnlen;
-		strncpy(cname, name, 200);
+
+		if (!dev_conflict)
+			strncpy(cname, name, 200);
 		cname[200] = 0;
 		while ((cp = strchr(cname, '/')) != NULL)
 			*cp = '-';
@@ -312,7 +318,7 @@ int create_mddev(char *dev, char *name, int autof,
int trustworthy,
quoted
 		}
 	}

-	if (dev && dev[0] == '/')
+	if ((dev && dev[0] == '/') && (!dev_conflict))
 		strcpy(chosen, dev);
 	else if (cname[0] == 0)
 		strcpy(chosen, devname);

Re: [PATCH] fix: do not overwrite existing devices' symlinks

From: NeilBrown <hidden>
Date: 2012-01-30 22:13:57

On Mon, 30 Jan 2012 12:13:05 +0000 "Dorau, Lukasz" [off-list ref]
wrote:
quoted
-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:27 AM
To: Dorau, Lukasz
Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski,
Ed
Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks

On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau [off-list ref]
wrote:
quoted
If the device's name is given in /etc/mdadm.conf, create_mddev()
does not check if the map contains a device of this name (mdopen.c:140).
If it does, the symlink of that name will be overwritten.

create_mddev() has been changed. Now it checks if the map contains
a device of the name given in /etc/mdadm.conf.
If it does, the appropriate suffix is added to the given name.

Signed-off-by: Lukasz Dorau <redacted>
Can you please remind me what the big picture problem is here??

It seem like you are suggesting that if
   /dev/md/thing

is given in mdadm.conf, but some other array is already assembled with the
name /dev/md/thing, then the array from mdadm.conf should be assembled as
   /dev/md/thing0
or something like that - is that correct?

I don't think we want that.  If there is a name conflict like  this with a
name given in mdadm.conf, then one of the arrays should fail to assemble as
this is really a fairly serious configuration error.

Or did I misunderstand?
quoted
-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:30 AM
To: Dorau, Lukasz
Subject: Re: mdadm: checking dev's names from mdadm.conf - question

On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz" [off-list ref]
wrote:
quoted
Hi

Is it OK that mdadm does not check if a symlink of the name given in
/etc/mdadm.conf already exists (in function create_mddev() in mdopen.c:140) ?
quoted
For example:
If we modify the original /etc/mdadm.conf file:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY /dev/md/imsm0 metadata=imsm
UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
quoted
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
mdadm will create the first container /dev/md127 and the symlink of default
name /dev/md/imsm0 (and the first volume /dev/md126 with symlink
/dev/md/vol1).
quoted
Later it will create the second container /dev/md125 and the symlink of the
name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of
the first container.
quoted
Mdadm does not check if the symlink of the given name already exists and it
_overwrites_ the first symlink. Is it OK or maybe should it be corrected?
quoted
Ahhh.. this is where the big-picture bit is...

I don't have a big problem with it over-writing the symlink - that is what
you asked for in a way.

However I also wouldn't have a problem with mdadm refusing the assemble the
second container as its name is already in use.

So, are you going to apply this patch or do you want it to be fixed in another way?
The approaches I said were OK were:
  1/ over-write the symlink
  2/ refuse to assemble the second container

The approach the patch takes is
  3/ use a different name to the one in mdadm.conf

As 3 != 1 and 3 != 2, I'm not going to apply the patch.

NeilBrown

RE: [PATCH] fix: do not overwrite existing devices' symlinks

From: Dorau, Lukasz <hidden>
Date: 2012-01-31 08:00:50

-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 11:14 PM
To: Dorau, Lukasz
Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin; Ciechanowski,
Ed
Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks

On Mon, 30 Jan 2012 12:13:05 +0000 "Dorau, Lukasz"
[off-list ref]
wrote:
quoted
quoted
-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:27 AM
To: Dorau, Lukasz
Cc: linux-raid@vger.kernel.org; Williams, Dan J; Labun, Marcin;
Ciechanowski,
quoted
quoted
Ed
Subject: Re: [PATCH] fix: do not overwrite existing devices' symlinks

On Mon, 23 Jan 2012 13:06:52 +0100 Lukasz Dorau
[off-list ref]
quoted
quoted
wrote:
quoted
If the device's name is given in /etc/mdadm.conf, create_mddev()
does not check if the map contains a device of this name (mdopen.c:140).
If it does, the symlink of that name will be overwritten.

create_mddev() has been changed. Now it checks if the map contains
a device of the name given in /etc/mdadm.conf.
If it does, the appropriate suffix is added to the given name.

Signed-off-by: Lukasz Dorau <redacted>
Can you please remind me what the big picture problem is here??

It seem like you are suggesting that if
   /dev/md/thing

is given in mdadm.conf, but some other array is already assembled with the
name /dev/md/thing, then the array from mdadm.conf should be assembled
as
quoted
quoted
   /dev/md/thing0
or something like that - is that correct?

I don't think we want that.  If there is a name conflict like  this with a
name given in mdadm.conf, then one of the arrays should fail to assemble as
this is really a fairly serious configuration error.

Or did I misunderstand?
quoted
-----Original Message-----
From: NeilBrown [mailto:neilb@suse.de]
Sent: Monday, January 30, 2012 2:30 AM
To: Dorau, Lukasz
Subject: Re: mdadm: checking dev's names from mdadm.conf - question

On Fri, 20 Jan 2012 09:27:58 +0000 "Dorau, Lukasz"
[off-list ref]
quoted
quoted
wrote:
quoted
Hi

Is it OK that mdadm does not check if a symlink of the name given in
/etc/mdadm.conf already exists (in function create_mddev() in
mdopen.c:140) ?
quoted
quoted
quoted
For example:
If we modify the original /etc/mdadm.conf file:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY metadata=imsm UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
by adding the 2nd container 's name /dev/md/imsm0 to the 3rd line:

ARRAY metadata=imsm UUID=e92bcf10:ca6b3fbd:95904441:5472d320
ARRAY /dev/md/vol1 container=e92bcf10:ca6b3fbd:95904441:5472d320
member=0 UUID=ea776aa6:4691d6ee:9400457e:73e1e9d9
quoted
ARRAY /dev/md/imsm0 metadata=imsm
UUID=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
quoted
ARRAY /dev/md/vol2 container=d61a8e6a:ed4e8ed6:dc0f4fb7:f839907e
member=0 UUID=b33bbd5e:964c7acc:66cfdfcc:7a938902
quoted
mdadm will create the first container /dev/md127 and the symlink of
default
quoted
quoted
name /dev/md/imsm0 (and the first volume /dev/md126 with symlink
/dev/md/vol1).
quoted
Later it will create the second container /dev/md125 and the symlink of the
name given in /etc/mdadm.conf - /dev/md/imsm0 - the same as the name of
the first container.
quoted
Mdadm does not check if the symlink of the given name already exists and
it
quoted
quoted
_overwrites_ the first symlink. Is it OK or maybe should it be corrected?
quoted
Ahhh.. this is where the big-picture bit is...

I don't have a big problem with it over-writing the symlink - that is what
you asked for in a way.

However I also wouldn't have a problem with mdadm refusing the assemble
the
quoted
quoted
second container as its name is already in use.

So, are you going to apply this patch or do you want it to be fixed in another
way?

The approaches I said were OK were:
  1/ over-write the symlink
  2/ refuse to assemble the second container

The approach the patch takes is
  3/ use a different name to the one in mdadm.conf

As 3 != 1 and 3 != 2, I'm not going to apply the patch.
OK, I had misunderstood you previously.

Regards,
Lukasz
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help