Skip to content

Commit

Permalink
Validate GUID on find_by_path, unless creating or splitting a pool.
Browse files Browse the repository at this point in the history 
Verified this works for cache vdevs on manual import, testing auto-import from cachefile.
  • Loading branch information
@evansus
evansus committed Jun 14, 2014
1 parent 7c83bc2 commit ed1463a
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 20 deletions.
4 changes: 2 additions & 2 deletions include/sys/vdev_iokit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,11 @@ extern void vdev_iokit_rele(vdev_t *);

extern void vdev_iokit_state_change(vdev_t *, int, int);

extern int vdev_iokit_open_by_path(vdev_iokit_t *, char *);
extern int vdev_iokit_open_by_path(vdev_iokit_t *, char *, uint64_t);

extern int vdev_iokit_open_by_guid(vdev_iokit_t *, uint64_t);

extern int vdev_iokit_find_by_path(vdev_iokit_t *, char *, bool);
extern int vdev_iokit_find_by_path(vdev_iokit_t *, char *, uint64_t);

extern int vdev_iokit_find_by_guid(vdev_iokit_t *, uint64_t);

Expand Down
42 changes: 33 additions & 9 deletions module/zfs/vdev_iokit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

#include <sys/zfs_context.h>
#include <sys/spa.h>
#include <sys/spa_impl.h>
#include <sys/vdev_iokit.h>
#include <sys/vdev_impl.h>
#include <sys/fs/zfs.h>
Expand Down Expand Up @@ -109,6 +110,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
{
vdev_iokit_t *dvd = 0;
int error = 0;
uint64_t checkguid = 0;

if (!vd)
return (EINVAL);
Expand Down Expand Up @@ -147,6 +149,12 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
return (error != 0 ? error : ENOMEM);
}

/* When creating or splitting pools, don't validate guid */
if (vd->vdev_spa->spa_load_state != SPA_LOAD_NONE &&
vd->vdev_spa->spa_config_splitting == 0) {
checkguid = vd->vdev_guid;
}

/*
* When opening a disk device, we want to preserve the user's original
* intent. We always want to open the device by the path the user gave
Expand Down Expand Up @@ -175,7 +183,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,

(void) snprintf(buf, len, "%ss0", vd->vdev_path);

error = vdev_iokit_open_by_path(dvd, buf);
error = vdev_iokit_open_by_path(dvd, buf, checkguid);

if (error == 0) {
spa_strfree(vd->vdev_path);
Expand All @@ -191,7 +199,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
* specified path.
*/
if (error != 0) {
error = vdev_iokit_open_by_path(dvd, vd->vdev_path);
error = vdev_iokit_open_by_path(dvd, vd->vdev_path, checkguid);
}

/*
Expand All @@ -211,7 +219,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
if (error) {

if (vd->vdev_physpath != NULL) {
error = vdev_iokit_open_by_path(dvd, vd->vdev_physpath);
error = vdev_iokit_open_by_path(dvd, vd->vdev_physpath, checkguid);
}

/*
Expand All @@ -220,7 +228,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
* don't need to propagate its oddities to this edge condition.
*/
if (error && vd->vdev_path != NULL) {
error = vdev_iokit_open_by_path(dvd, vd->vdev_path);
error = vdev_iokit_open_by_path(dvd, vd->vdev_path, checkguid);
}

/*
Expand All @@ -232,7 +240,7 @@ vdev_iokit_open(vdev_t *vd, uint64_t *size,
* been re-cabled, moved, removed, or otherwise.
*/
if (error && vd->vdev_guid != 0) {
error = vdev_iokit_open_by_guid(dvd, vd->vdev_guid);
error = vdev_iokit_open_by_guid(dvd, checkguid);
}
}

Expand Down Expand Up @@ -577,11 +585,27 @@ vdev_iokit_read_label(vdev_iokit_t * dvd, nvlist_t **config)
continue;
}

/*
* Check that a valid config was loaded
* skip devices that are unavailable,
* uninitialized, or potentially active
*/
if (nvlist_lookup_uint64(*config,
ZPOOL_CONFIG_POOL_TXG, &txg) != 0 ||
nvlist_lookup_uint64(*config,
ZPOOL_CONFIG_POOL_STATE, &state) != 0 ||
state >= POOL_STATE_DESTROYED || txg == 0) {
state > POOL_STATE_L2CACHE) {

nvlist_free(*config);
*config = NULL;
continue;
}

/*
* Check and fetch txg number
*/
if (state != POOL_STATE_SPARE &&
state != POOL_STATE_L2CACHE &&
(nvlist_lookup_uint64(*config, ZPOOL_CONFIG_POOL_TXG,
&txg) != 0 || txg == 0)) {

nvlist_free(*config);
*config = NULL;
Expand Down Expand Up @@ -620,7 +644,7 @@ vdev_iokit_read_rootlabel(char *devpath, char *devid, nvlist_t **config)
return (error);

/* Locate the vdev by pathname, without validating the GUID */
error = vdev_iokit_find_by_path(dvd, devpath, FALSE);
error = vdev_iokit_find_by_path(dvd, devpath, 0);

if (error) {
goto error;
Expand Down
41 changes: 32 additions & 9 deletions module/zfs/vdev_iokit_util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -573,16 +573,19 @@ extern OSSet * vdev_iokit_get_disks()

/* Returned object will have a reference count and should be released */
int
vdev_iokit_find_by_path(vdev_iokit_t * dvd, char * diskPath, bool validate)
vdev_iokit_find_by_path(vdev_iokit_t * dvd, char * diskPath, uint64_t guid = 0)
{
OSSet * allDisks = 0;
OSObject * currentEntry = 0;
IOMedia * currentDisk = 0;
IORegistryEntry * matchedDisk = 0;
IOMedia * matchedDisk = 0;
OSObject * bsdnameosobj = 0;
OSString * bsdnameosstr = 0;
char * diskName = 0;
nvlist_t * config = 0;

uint64_t min_size = 128<<20; /* 128 Mb */
uint64_t current_guid = 0;

if (!dvd || !diskPath) {
return (EINVAL);
Expand Down Expand Up @@ -656,11 +659,6 @@ vdev_iokit_find_by_path(vdev_iokit_t * dvd, char * diskPath, bool validate)
/* Check if the name matches */
if (bsdnameosstr->isEqualTo(diskName)) {
/* Success - save match */
if (matchedDisk) {
matchedDisk->release();
matchedDisk = 0;
}

matchedDisk = currentDisk;
matchedDisk->retain();
}
Expand All @@ -676,6 +674,31 @@ vdev_iokit_find_by_path(vdev_iokit_t * dvd, char * diskPath, bool validate)
}
}

/* Check GUID */
if (guid > 0 && matchedDisk) {
/* Temporarily assign currentDisk to the dvd */
dvd->vd_iokit_hl = (void *)matchedDisk;

/* Try to read a config label from this disk */
if (vdev_iokit_read_label(dvd, &config) != 0 ||
nvlist_lookup_uint64(config,
ZPOOL_CONFIG_GUID, &current_guid) != 0 ||
current_guid != guid) {

/* Clear matchedDisk */
matchedDisk->release();
matchedDisk = 0;
}

/* Clear the vd_iokit_hl */
dvd->vd_iokit_hl = 0;

/* Clear the config */
if (config)
nvlist_free(config);
config = 0;
}

if (allDisks) {
allDisks->flushCollection();
allDisks->release();
Expand Down Expand Up @@ -1107,13 +1130,13 @@ vdev_iokit_handle_close(vdev_iokit_t *dvd, int fmode = 0)
}

extern int
vdev_iokit_open_by_path(vdev_iokit_t * dvd, char * path)
vdev_iokit_open_by_path(vdev_iokit_t * dvd, char * path, uint64_t guid = 0)
{
if (!dvd || !path) {
return (EINVAL);
}

if (vdev_iokit_find_by_path(dvd, path, TRUE) != 0 ||
if (vdev_iokit_find_by_path(dvd, path, guid) != 0 ||
!dvd->vd_iokit_hl) {

return (ENOENT);
Expand Down

1 comment on commit ed1463a

@evansus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested cache and log devices, and renumbering of data, cache, and log vdevs.
Manual zpool import works as expected, updating the device paths automatically.
Import from cachefile is working, however it does not reflect the new path.

A log vdev that was located at disk3s1 then moved to disk2s1, still shows disk3s1 even though there is no device at that path. However the vdev is working properly according to zpool status and after zpool scrub was performed.

Manual import reflected the new disk2s1 path, and subsequent import from cachefile continues to show disk3s1.

bash-3.2# zpool import -c /tmp/zpool.cache.tmp test
bash-3.2# zpool status
  pool: test
 state: ONLINE
  scan: scrub repaired 0 in 0h0m with 0 errors on Sat Jun 14 15:02:40 2014
config:

        NAME        STATE     READ WRITE CKSUM
        test        ONLINE       0     0     0
          disk1s1   ONLINE       0     0     0
        logs
          disk3s1   ONLINE       0     0     0

errors: No known data errors
bash-3.2# diskutil list
/dev/disk0
   #:                       TYPE NAME                    SIZE       IDENTIFIER
   0:      GUID_partition_scheme                        *21.5 GB    disk0
   1:                        EFI EFI                     209.7 MB   disk0s1
   2:                  Apple_HFS vMacRAID                21.0 GB    disk0s2
/dev/disk1
   #:                       TYPE NAME                    SIZE       IDENTIFIER
   0:      GUID_partition_scheme                        *2.1 GB     disk1
   1:                  Apple_HFS                         2.0 GB     disk1s1
/dev/disk2
   #:                       TYPE NAME                    SIZE       IDENTIFIER
   0:      GUID_partition_scheme                        *181.3 MB   disk2
   1:                  Apple_HFS                         181.2 MB   disk2s1

bash-3.2# zpool export test
bash-3.2# zpool import test
bash-3.2# zpool status
  pool: test
 state: ONLINE
  scan: scrub repaired 0 in 0h0m with 0 errors on Sat Jun 14 15:02:40 2014
config:

        NAME        STATE     READ WRITE CKSUM
        test        ONLINE       0     0     0
          disk1s1   ONLINE       0     0     0
        logs
          disk2s1   ONLINE       0     0     0

errors: No known data errors
bash-3.2# zpool export test
bash-3.2# zpool import -c /tmp/zpool.cache.tmp test
bash-3.2# zpool status
  pool: test
 state: ONLINE
  scan: scrub repaired 0 in 0h0m with 0 errors on Sat Jun 14 15:02:40 2014
config:

        NAME        STATE     READ WRITE CKSUM
        test        ONLINE       0     0     0
          disk1s1   ONLINE       0     0     0
        logs
          disk3s1   ONLINE       0     0     0

errors: No known data errors

Please sign in to comment.