From b526e52dc7cbdde98db9c9f8765be28ba6d71d78 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 16 Jun 2010 17:26:04 -0700 Subject: 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 Signed-off-by: Dan Williams --- Grow.c | 20 +++++++++++++------- Incremental.c | 5 +++++ managemon.c | 2 +- mapfile.c | 5 +++-- mdadm.h | 1 - mdmon.c | 3 +-- super-ddf.c | 8 +------- super-intel.c | 7 +------ sysfs.c | 20 +++++++++----------- 9 files changed, 34 insertions(+), 37 deletions(-) diff --git a/Grow.c b/Grow.c index 28ed8d7..3923a90 100644 --- 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 d6dd0f4..8062e2b 100644 --- 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 454c39d..debca97 100644 --- 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 0f12559..ffe8e16 100644 --- 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 a0797e8..62bfc44 100644 --- 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 69c320e..2191814 100644 --- 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 b01c68d..6145e3c 100644 --- 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 d6d8b09..e09ce5e 100644 --- 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 ebf9d8a..17f2567 100644 --- 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 */ -- cgit