mdadm segfault with incremental

mdadm segfault with incremental

[Bjørnar Ness]

While working with mdadm policy/udev to add slaves when they are
inserted, I notices
mdadm is segfaulting if it gets a blank drive and action is set to
spare or above.

Looking into the code, this segfault is caused by the following code:

Incremental.c line 965

                     st2->ss->avail_size(st2, devsize,
                                         sra->devs
                                         ? sra->devs->data_offset
                                         : INVALID_SECTORS)

avail_size in my case is super1.c avail_size1

and here the code sets:

struct mdp_superblock_1 *super = st->sb;

and later tries accessing i.e super->feature_map, where it segfaults
because in the case
of an empty drive, st2 is created in super1.c/match_metadata_desc1
where it sets:

        st->sb = NULL;

I am not entirely sure how this is supposed to work, but atleast
currently it segfaults.

-- 
Bj(/)rnar

Re: mdadm segfault with incremental

[NeilBrown]

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

On Thu, Oct 19 2017, Bjørnar Ness wrote:

> While working with mdadm policy/udev to add slaves when they are
> inserted, I notices
> mdadm is segfaulting if it gets a blank drive and action is set to
> spare or above.
>
> Looking into the code, this segfault is caused by the following code:
>
> Incremental.c line 965
>
>                      st2->ss->avail_size(st2, devsize,
>                                          sra->devs
>                                          ? sra->devs->data_offset
>                                          : INVALID_SECTORS)
>
> avail_size in my case is super1.c avail_size1
>
> and here the code sets:
>
> struct mdp_superblock_1 *super = st->sb;
>
> and later tries accessing i.e super->feature_map, where it segfaults
> because in the case
> of an empty drive, st2 is created in super1.c/match_metadata_desc1
> where it sets:
>
>         st->sb = NULL;
>
> I am not entirely sure how this is supposed to work, but atleast
> currently it segfaults.

Hi Bjørnar,
 thanks for the report.

This was broken by
Commit: 641da7459192 ("super1: separate to version of _avail_space1().")

in mdadm-3.3.  The code in Incremental.c should really be using
->validate_geometry, rather than ->avail_size.

This patch should fix it.  Could you please try and report?

Thanks,
NeilBrown


diff --git a/Incremental.c b/Incremental.c
index 91301eb5e609..baea9761cee1 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -870,7 +870,7 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		struct supertype *st2;
 		struct domainlist *dl = NULL;
 		struct mdinfo *sra;
-		unsigned long long devsize;
+		unsigned long long devsize, freesize = 0;
 		struct spare_criteria sc = {0, 0};
 
 		if (is_subarray(mp->metadata))
@@ -942,10 +942,13 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 			close(mdfd);
 		}
 		if ((sra->component_size > 0 &&
-		     st2->ss->avail_size(st2, devsize,
-					 sra->devs ? sra->devs->data_offset :
-					 INVALID_SECTORS) <
-		     sra->component_size) ||
+		     st2->ss->validate_geometry(st2, sra->array.level, sra->array.layout,
+						sra->array.raid_disks, &sra->array.chunk_size,
+						sra->component_size,
+						sra->devs ? sra->devs->data_offset : INVALID_SECTORS,
+						devname, &freesize, sra->consistency_policy,
+						0) &&
+		     freesize < sra->component_size) ||
 		    (sra->component_size == 0 && devsize < sc.min_size)) {
 			if (verbose > 1)
 				pr_err("not adding %s to %s as it is too small\n",

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

Re: mdadm segfault with incremental

[Bjørnar Ness]

Hello, Neil.

This patch seems to solve the issues I am experiencing. So fine to apply.

-- 
Bj(/)rnar

Re: mdadm segfault with incremental

[NeilBrown]

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

On Tue, Oct 24 2017, Bjørnar Ness wrote:

> Hello, Neil.
>
> This patch seems to solve the issues I am experiencing. So fine to apply.
>

Thanks for testing and confirming.  I'll turn it into a proper patch and
send it upstream. There might be other places where validate_geometry
should be used instead of avail_size.

Thanks,
NeilBrown

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