[PATCH] Fix race between --create and --incremental

[PATCH] Fix race between --create and --incremental

[Artur Paszkiewicz]

This modifies locking in Create to eliminate a situation where
--incremental can assemble a device between write_init_super() and
add_disk(), which causes Create to fail.

It sporadically occurs e.g. when metadata is written on a device,
causing an udev change event which triggers mdadm --incremental.

Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 Create.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Create.c b/Create.c
index 692ff52..bac9181 100644
--- a/Create.c
+++ b/Create.c
@@ -833,6 +833,7 @@ int Create(struct supertype *st, char *mddev,
 
 	infos = xmalloc(sizeof(*infos) * total_slots);
 	enable_fds(total_slots);
+	map_lock(&map);
 	for (pass=1; pass <=2 ; pass++) {
 		struct mddev_dev *moved_disk = NULL; /* the disk that was moved out of the insert point */
 
@@ -886,7 +887,7 @@ int Create(struct supertype *st, char *mddev,
 						pr_err("failed to open %s "
 							"after earlier success - aborting\n",
 							dv->devname);
-						goto abort;
+						goto abort_locked;
 					}
 					fstat(fd, &stb);
 					inf->disk.major = major(stb.st_rdev);
@@ -898,7 +899,7 @@ int Create(struct supertype *st, char *mddev,
 							 fd, dv->devname,
 							 dv->data_offset)) {
 					ioctl(mdfd, STOP_ARRAY, NULL);
-					goto abort;
+					goto abort_locked;
 				}
 				st->ss->getinfo_super(st, inf, NULL);
 				safe_mode_delay = inf->safe_mode_delay;
@@ -924,7 +925,7 @@ int Create(struct supertype *st, char *mddev,
 					pr_err("ADD_NEW_DISK for %s "
 					       "failed: %s\n",
 					       dv->devname, strerror(errno));
-					goto abort;
+					goto abort_locked;
 				}
 				break;
 			}
@@ -941,7 +942,6 @@ int Create(struct supertype *st, char *mddev,
 			 * the subarray cursor such that ->getinfo_super once
 			 * again returns container info.
 			 */
-			map_lock(&map);
 			st->ss->getinfo_super(st, &info_new, NULL);
 			if (st->ss->external && s->level != LEVEL_CONTAINER &&
 			    !same_uuid(info_new.uuid, info.uuid, 0)) {
@@ -966,12 +966,12 @@ int Create(struct supertype *st, char *mddev,
 					   info_new.uuid, path);
 				free(path);
 			}
-			map_unlock(&map);
 
 			flush_metadata_updates(st);
 			st->ss->free_super(st);
 		}
 	}
+	map_unlock(&map);
 	free(infos);
 
 	if (s->level == LEVEL_CONTAINER) {
-- 
1.8.4.5

Re: [PATCH] Fix race between --create and --incremental

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5263 bytes --]

On Wed,  9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz
<artur.paszkiewicz@intel.com> wrote:

> This modifies locking in Create to eliminate a situation where
> --incremental can assemble a device between write_init_super() and
> add_disk(), which causes Create to fail.
> 
> It sporadically occurs e.g. when metadata is written on a device,
> causing an udev change event which triggers mdadm --incremental.
> 
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>

Thanks for the patch.
I've taken the liberty of changing it a little.  I didn't like the fact that
we dropped the lock and then took it again.  It probably doesn't matter, but
it feels cleaner to hold it the whole time.

So this is the result.  Let me know if you disagree at all.

Thanks,
NeilBrown

From a22b82ef078a37f98e0a08c23dca32fa38792d97 Mon Sep 17 00:00:00 2001
From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Date: Wed, 9 Apr 2014 17:14:59 +0200
Subject: [PATCH] Fix race between --create and --incremental

This modifies locking in Create to eliminate a situation where
--incremental can assemble a device between write_init_super() and
add_disk(), which causes Create to fail.

It sporadically occurs e.g. when metadata is written on a device,
causing an udev change event which triggers mdadm --incremental.

Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Create.c b/Create.c
index 9247d46bbfac..98bbdd40a161 100644
--- a/Create.c
+++ b/Create.c
@@ -740,7 +740,9 @@ int Create(struct supertype *st, char *mddev,
 
 	map_update(&map, fd2devnm(mdfd), info.text_version,
 		   info.uuid, chosen_name);
-	map_unlock(&map);
+	/* Keep map locked until devices have been added to array
+	 * to stop another mdadm from finding and using those devices.
+	 */
 
 	if (s->bitmap_file && vers < 9003) {
 		major_num = BITMAP_MAJOR_HOSTENDIAN;
@@ -753,18 +755,18 @@ int Create(struct supertype *st, char *mddev,
 	if (s->bitmap_file && strcmp(s->bitmap_file, "internal")==0) {
 		if ((vers%100) < 2) {
 			pr_err("internal bitmaps not supported by this kernel.\n");
-			goto abort;
+			goto abort_locked;
 		}
 		if (!st->ss->add_internal_bitmap) {
 			pr_err("internal bitmaps not supported with %s metadata\n",
 				st->ss->name);
-			goto abort;
+			goto abort_locked;
 		}
 		if (!st->ss->add_internal_bitmap(st, &s->bitmap_chunk,
 						 c->delay, s->write_behind,
 						 bitmapsize, 1, major_num)) {
 			pr_err("Given bitmap chunk size not supported.\n");
-			goto abort;
+			goto abort_locked;
 		}
 		s->bitmap_file = NULL;
 	}
@@ -790,7 +792,7 @@ int Create(struct supertype *st, char *mddev,
 		if (container_fd < 0) {
 			pr_err("Cannot get exclusive "
 				"open on container - weird.\n");
-			goto abort;
+			goto abort_locked;
 		}
 		if (mdmon_running(st->container_devnm)) {
 			if (c->verbose)
@@ -805,7 +807,7 @@ int Create(struct supertype *st, char *mddev,
 	if (rv) {
 		pr_err("failed to set array info for %s: %s\n",
 			mddev, strerror(errno));
-		goto abort;
+		goto abort_locked;
 	}
 
 	if (s->bitmap_file) {
@@ -816,18 +818,18 @@ int Create(struct supertype *st, char *mddev,
 				 c->delay, s->write_behind,
 				 bitmapsize,
 				 major_num)) {
-			goto abort;
+			goto abort_locked;
 		}
 		bitmap_fd = open(s->bitmap_file, O_RDWR);
 		if (bitmap_fd < 0) {
 			pr_err("weird: %s cannot be openned\n",
 				s->bitmap_file);
-			goto abort;
+			goto abort_locked;
 		}
 		if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
 			pr_err("Cannot set bitmap file for %s: %s\n",
 				mddev, strerror(errno));
-			goto abort;
+			goto abort_locked;
 		}
 	}
 
@@ -884,7 +886,7 @@ int Create(struct supertype *st, char *mddev,
 						pr_err("failed to open %s "
 							"after earlier success - aborting\n",
 							dv->devname);
-						goto abort;
+						goto abort_locked;
 					}
 					fstat(fd, &stb);
 					inf->disk.major = major(stb.st_rdev);
@@ -896,7 +898,7 @@ int Create(struct supertype *st, char *mddev,
 							 fd, dv->devname,
 							 dv->data_offset)) {
 					ioctl(mdfd, STOP_ARRAY, NULL);
-					goto abort;
+					goto abort_locked;
 				}
 				st->ss->getinfo_super(st, inf, NULL);
 				safe_mode_delay = inf->safe_mode_delay;
@@ -922,7 +924,7 @@ int Create(struct supertype *st, char *mddev,
 					pr_err("ADD_NEW_DISK for %s "
 					       "failed: %s\n",
 					       dv->devname, strerror(errno));
-					goto abort;
+					goto abort_locked;
 				}
 				break;
 			}
@@ -939,7 +941,6 @@ int Create(struct supertype *st, char *mddev,
 			 * the subarray cursor such that ->getinfo_super once
 			 * again returns container info.
 			 */
-			map_lock(&map);
 			st->ss->getinfo_super(st, &info_new, NULL);
 			if (st->ss->external && s->level != LEVEL_CONTAINER &&
 			    !same_uuid(info_new.uuid, info.uuid, 0)) {
@@ -964,12 +965,12 @@ int Create(struct supertype *st, char *mddev,
 					   info_new.uuid, path);
 				free(path);
 			}
-			map_unlock(&map);
 
 			flush_metadata_updates(st);
 			st->ss->free_super(st);
 		}
 	}
+	map_unlock(&map);
 	free(infos);
 
 	if (s->level == LEVEL_CONTAINER) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Fix race between --create and --incremental

[Artur Paszkiewicz]

On 04/10/2014 02:33 AM, NeilBrown wrote:
> On Wed,  9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz
> <artur.paszkiewicz@intel.com> wrote:
>
>> This modifies locking in Create to eliminate a situation where
>> --incremental can assemble a device between write_init_super() and
>> add_disk(), which causes Create to fail.
>>
>> It sporadically occurs e.g. when metadata is written on a device,
>> causing an udev change event which triggers mdadm --incremental.
>>
>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
>
> Thanks for the patch.
> I've taken the liberty of changing it a little.  I didn't like the fact that
> we dropped the lock and then took it again.  It probably doesn't matter, but
> it feels cleaner to hold it the whole time.
>
> So this is the result.  Let me know if you disagree at all.

Hello Neil,

Your changes are OK, thanks.

Regards,
Artur

Re: [PATCH] Fix race between --create and --incremental

[Artur Paszkiewicz]

On 04/11/2014 12:08 PM, Artur Paszkiewicz wrote:
> On 04/10/2014 02:33 AM, NeilBrown wrote:
>> On Wed,  9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz
>> <artur.paszkiewicz@intel.com> wrote:
>>
>>> This modifies locking in Create to eliminate a situation where
>>> --incremental can assemble a device between write_init_super() and
>>> add_disk(), which causes Create to fail.
>>>
>>> It sporadically occurs e.g. when metadata is written on a device,
>>> causing an udev change event which triggers mdadm --incremental.
>>>
>>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
>>
>> Thanks for the patch.
>> I've taken the liberty of changing it a little.  I didn't like the
>> fact that
>> we dropped the lock and then took it again.  It probably doesn't
>> matter, but
>> it feels cleaner to hold it the whole time.
>>
>> So this is the result.  Let me know if you disagree at all.
> 
> Hello Neil,
> 
> Your changes are OK, thanks.

Hello Neil,

Are you going to apply this patch?

Thanks,
Artur

Re: [PATCH] Fix race between --create and --incremental

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Wed, 30 Apr 2014 13:50:02 +0200 Artur Paszkiewicz
<artur.paszkiewicz@intel.com> wrote:

> On 04/11/2014 12:08 PM, Artur Paszkiewicz wrote:
> > On 04/10/2014 02:33 AM, NeilBrown wrote:
> >> On Wed,  9 Apr 2014 17:14:59 +0200 Artur Paszkiewicz
> >> <artur.paszkiewicz@intel.com> wrote:
> >>
> >>> This modifies locking in Create to eliminate a situation where
> >>> --incremental can assemble a device between write_init_super() and
> >>> add_disk(), which causes Create to fail.
> >>>
> >>> It sporadically occurs e.g. when metadata is written on a device,
> >>> causing an udev change event which triggers mdadm --incremental.
> >>>
> >>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> >>
> >> Thanks for the patch.
> >> I've taken the liberty of changing it a little.  I didn't like the
> >> fact that
> >> we dropped the lock and then took it again.  It probably doesn't
> >> matter, but
> >> it feels cleaner to hold it the whole time.
> >>
> >> So this is the result.  Let me know if you disagree at all.
> > 
> > Hello Neil,
> > 
> > Your changes are OK, thanks.
> 
> Hello Neil,
> 
> Are you going to apply this patch?

I had, but I hadn't pushed it out.
I have now.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]