Always assume SKIP_GONE_DEVS behaviour and kill the flag

...i.e. GET_DEVS == (GET_DEVS|SKIP_GONE_DEVS)

A null pointer dereference in Incremental.c can be triggered by
replugging a disk while the old name is in use.  When mdadm -I is called
on the new disk we fail the call to sysfs_read().  I audited all the
locations that use GET_DEVS and it appears they can tolerate missing a
drive.  So just make SKIP_GONE_DEVS the default behaviour.

Also fix up remaining unchecked usages of the sysfs_read() return value.

Reported-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>

diff --git a/Grow.c b/Grow.c
index 28ed8d7390cdf3267ace5795ea96c8d99c384cb2..3923a90fded10dd1dd050101b7f7d40bc41b08b2 100644 (file)
--- a/Grow.c
+++ b/Grow.c
@@ -546,7 +546,13 @@ int Grow_reshape(char *devname, int fd, int quiet, char *backup_file,
                return 1;
        }
        sra = sysfs_read(fd, 0, GET_LEVEL);
-       frozen = freeze_array(sra);
+       if (sra)
+               frozen = freeze_array(sra);
+       else {
+               fprintf(stderr, Name ": failed to read sysfs parameters for %s\n",
+                       devname);
+               return 1;
+       }
        if (frozen < 0) {
                fprintf(stderr, Name ": %s is performing resync/recovery and cannot"
                        " be reshaped\n", devname);
@@ -1970,6 +1976,12 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
        int cache;
        int done = 0;
 
+       sra = sysfs_read(-1, devname2devnum(info->sys_name),
+                        GET_COMPONENT|GET_DEVS|GET_OFFSET|GET_STATE|
+                        GET_CACHE);
+       if (!sra)
+               return 1;
+
        err = sysfs_set_str(info, NULL, "array_state", "readonly");
        if (err)
                return err;
@@ -1990,7 +2002,6 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
        ochunk = info->array.chunk_size;
        nchunk = info->new_chunk;
 
-
        a = (ochunk/512) * odata;
        b = (nchunk/512) * ndata;
        /* Find GCD */
@@ -2003,11 +2014,6 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
        /* LCM == product / GCD */
        blocks = (ochunk/512) * (nchunk/512) * odata * ndata / a;
 
-       sra = sysfs_read(-1, devname2devnum(info->sys_name),
-                        GET_COMPONENT|GET_DEVS|GET_OFFSET|GET_STATE|
-                        GET_CACHE);
-
-
        if (ndata == odata)
                while (blocks * 32 < sra->component_size &&
                       blocks < 16*1024*2)

diff --git a/Incremental.c b/Incremental.c
index d6dd0f43bef1e84786cb7dfb9fb75607edff3856..8062e2b17e154aab8c8224d8c81c4dfc7ecc2ebb 100644 (file)
--- a/Incremental.c
+++ b/Incremental.c
@@ -369,6 +369,8 @@ int Incremental(char *devname, int verbose, int runstop,
                        strcpy(chosen_name, devnum2devname(mp->devnum));
 
                sra = sysfs_read(mdfd, fd2devnum(mdfd), (GET_DEVS | GET_STATE));
+               if (!sra)
+                       return 2;
 
                if (sra->devs) {
                        sprintf(dn, "%d:%d", sra->devs->disk.major,
@@ -586,6 +588,9 @@ static int count_active(struct supertype *st, int mdfd, char **availp,
        struct mdinfo *sra = sysfs_read(mdfd, -1, GET_DEVS | GET_STATE);
        char *avail = NULL;
 
+       if (!sra)
+               return 0;
+
        for (d = sra->devs ; d ; d = d->next) {
                char dn[30];
                int dfd;

diff --git a/managemon.c b/managemon.c
index 454c39dc7e4193d52e48c149689604d9997ca7f1..debca974fc83224949b5fc3277df0fe278b22148 100644 (file)
--- a/managemon.c
+++ b/managemon.c
@@ -315,7 +315,7 @@ static void manage_container(struct mdstat_ent *mdstat,
                 * To see what is removed and what is added.
                 * These need to be remove from, or added to, the array
                 */
-               mdi = sysfs_read(-1, mdstat->devnum, GET_DEVS|SKIP_GONE_DEVS);
+               mdi = sysfs_read(-1, mdstat->devnum, GET_DEVS);
                if (!mdi) {
                        /* invalidate the current count so we can try again */
                        container->devcnt = -1;

diff --git a/mapfile.c b/mapfile.c
index 0f12559777b26b0bbbe5c9dbf17ea7dac06e4d54..ffe8e16f1ef582023467844d7bdbb1c44462b99e 100644 (file)
--- a/mapfile.c
+++ b/mapfile.c
@@ -368,7 +368,7 @@ void RebuildMap(void)
        }
 
        for (md = mdstat ; md ; md = md->next) {
-               struct mdinfo *sra = sysfs_read(-1, md->devnum, GET_DEVS|SKIP_GONE_DEVS);
+               struct mdinfo *sra = sysfs_read(-1, md->devnum, GET_DEVS);
                struct mdinfo *sd;
 
                if (!sra)
@@ -486,7 +486,8 @@ void RebuildMap(void)
                for (md = mdstat ; md ; md = md->next) {
                        struct mdinfo *sra = sysfs_read(-1, md->devnum,
                                                        GET_VERSION);
-                       sysfs_uevent(sra, "change");
+                       if (sra)
+                               sysfs_uevent(sra, "change");
                        sysfs_free(sra);
                }
        map_free(map);

diff --git a/mdadm.h b/mdadm.h
index a0797e8c9be26eeb9a63246b1e2ba7bdd89d745d..62bfc44587bbadf1b816cf626b26a24f0c9bef02 100644 (file)
--- a/mdadm.h
+++ b/mdadm.h
@@ -404,7 +404,6 @@ enum sysfs_read_flags {
        GET_SIZE        = (1 << 12),
        GET_STATE       = (1 << 13),
        GET_ERROR       = (1 << 14),
-       SKIP_GONE_DEVS  = (1 << 15),
 };
 
 /* If fd >= 0, get the array it is open on,

diff --git a/mdmon.c b/mdmon.c
index 69c320e66da27e683759307bb8a0047afce2da38..2191814447f34be7517f0b715f87a94098fe43b4 100644 (file)
--- a/mdmon.c
+++ b/mdmon.c
@@ -394,8 +394,7 @@ static int mdmon(char *devname, int devnum, int must_fork, int takeover)
                exit(3);
        }
 
-       mdi = sysfs_read(mdfd, container->devnum,
-                        GET_VERSION|GET_LEVEL|GET_DEVS|SKIP_GONE_DEVS);
+       mdi = sysfs_read(mdfd, container->devnum, GET_VERSION|GET_LEVEL|GET_DEVS);
 
        if (!mdi) {
                fprintf(stderr, "mdmon: failed to load sysfs info for %s\n",

diff --git a/super-ddf.c b/super-ddf.c
index b01c68d5a40b4472b74595c7693d705871e82f04..6145e3ca4d0fd3fba3ffe63ea79b82528830b7b6 100644 (file)
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2807,14 +2807,8 @@ static int load_super_ddf_all(struct supertype *st, int fd,
        int seq;
        char nm[20];
        int dfd;
-       int devnum = fd2devnum(fd);
-       enum sysfs_read_flags flags;
 
-       flags = GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE;
-       if (mdmon_running(devnum))
-               flags |= SKIP_GONE_DEVS;
-
-       sra = sysfs_read(fd, 0, flags);
+       sra = sysfs_read(fd, 0, GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE);
        if (!sra)
                return 1;
        if (sra->array.major_version != -1 ||

diff --git a/super-intel.c b/super-intel.c
index d6d8b090b1aa4244b986534d4db0c3834bc52834..e09ce5e13b6af29f77016627136d7c777958b411 100644 (file)
--- a/super-intel.c
+++ b/super-intel.c
@@ -2747,14 +2747,9 @@ static int load_super_imsm_all(struct supertype *st, int fd, void **sbp,
        int retry;
        int err = 0;
        int i;
-       enum sysfs_read_flags flags;
-
-       flags = GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE;
-       if (mdmon_running(devnum))
-               flags |= SKIP_GONE_DEVS;
 
        /* check if 'fd' an opened container */
-       sra = sysfs_read(fd, 0, flags);
+       sra = sysfs_read(fd, 0, GET_LEVEL|GET_VERSION|GET_DEVS|GET_STATE);
        if (!sra)
                return 1;
 

diff --git a/sysfs.c b/sysfs.c
index ebf9d8a65646d1e42ab28bac2c134c523eb58bc2..17f25673d7bdd14c86716b43370de4b7b094b8ef 100644 (file)
--- a/sysfs.c
+++ b/sysfs.c
@@ -273,22 +273,20 @@ struct mdinfo *sysfs_read(int fd, int devnum, unsigned long options)
 
                strcpy(dbase, "block/dev");
                if (load_sys(fname, buf)) {
+                       /* assume this is a stale reference to a hot
+                        * removed device
+                        */
                        free(dev);
-                       if (options & SKIP_GONE_DEVS)
-                               continue;
-                       else
-                               goto abort;
+                       continue;
                }
                sscanf(buf, "%d:%d", &dev->disk.major, &dev->disk.minor);
 
                /* special case check for block devices that can go 'offline' */
-               if (options & SKIP_GONE_DEVS) {
-                       strcpy(dbase, "block/device/state");
-                       if (load_sys(fname, buf) == 0 &&
-                           strncmp(buf, "offline", 7) == 0) {
-                               free(dev);
-                               continue;
-                       }
+               strcpy(dbase, "block/device/state");
+               if (load_sys(fname, buf) == 0 &&
+                   strncmp(buf, "offline", 7) == 0) {
+                       free(dev);
+                       continue;
                }
 
                /* finally add this disk to the array */