zfs crypto discuss - May 2008 - [Bug 1757] New: upgrade fails to see datasets

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757

           Summary: upgrade fails to see datasets
    Classification: Development
           Product: zfs-crypto
           Version: unspecified
          Platform: Other
        OS/Version: Solaris
            Status: NEW
          Severity: major
          Priority: P2
         Component: other
        AssignedTo: ajscarp at yahoo.com
        ReportedBy: ajscarp at yahoo.com
         QAContact: hua.tang at sun.com
                CC: zfs-crypto-discuss at opensolaris.org
   Estimated Hours: 0.0


When creating a pool a nevada build with spa_version=10 (pre-crypto), importing
and upgrading the pool in a spa_version=11 (w/ crypto) works.  However there is
a problem obtaining the dataset''s underneath it..

doing any zfs command, ''zfs get all tank'' used below, cause a
core that would
look something like:

libc.so.1`_lwp_kill+8(6, 0, 5, 6, ffffffff, 6)
libc.so.1`abort+0x108(0, 1, 6, ff2ee000, 114d08, 0)
libzfs.so.1`make_dataset_handle+0x25c(a7388, c32cc, c32c8, ff108000, fffa5e48, 
12)
libzfs.so.1`zfs_open+0xb8(a7388, ffbffba8, 7, 20000, 7f840, ff108000)
zfs_for_each+0x100(1, ffbffac8, 0, 33680, 0, 33400)
zfs_do_get+0x50c(1, ffbffac0, 1c000, 1, ffbffac8, 0)
main+0x2b0(4, ffbffabc, 30, c, ffbffb9c, ffbffba0)
_start+0x108(0, 0, 0, 0, 0, 0)


make_dataset_handle aborts and the zhp->zfs_dmustats.dds_type value equals
''0''.. Something is wrong with the dataset or the
pool''s dataset list.. Given I
haven''t seen any indication that datasets exist, i''m wondering
if the pool is
messed up

-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757


Darren J Moffat <darrenm at opensolaris.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darrenm at opensolaris.org




--- Comment #1 from Darren J Moffat <darrenm at opensolaris.org> 
2008-05-02 02:44:52 ---
I''ve tried a similar test using a version=10 pool.

I imported the pool running latest zfs-crypto binaries and it imported fine,
however it didn''t mount the datasets (tank/ and tank/clear) during
import but
an explicit "zfs mount -a" did mount both of them.

I believe the problem is that zpool_enable_datasets() checks that crypt
!ZIO_CRYPT_OFF.  When for an older pool datasets will actually have 0 for the
value of ZFS_PROP_ENCRYPTION (which is ZIO_CRYPT_INHERIT).  While we could
check for != ZIO_CRYPT_OFF && != ZIO_CRYPT_INHERIT that would be wrong
too,
what should really be done is a spa_version check and not even look at those
properties for older pools.

I believe this is the correct fix:

--- a/usr/src/lib/libzfs/common/libzfs_mount.c  Thu May 01 13:15:20 2008 +0100
+++ b/usr/src/lib/libzfs/common/libzfs_mount.c  Fri May 02 10:43:01 2008 +0100
@@ -1176,6 +1176,7 @@ zpool_enable_datasets(zpool_handle_t *zh
        zfs_handle_t *zfsp;
        int i, ret = -1;
        int *good;
+       uint64_t spa_version;

        /*
         * Gather all non-snap datasets within the pool.
@@ -1198,6 +1199,8 @@ zpool_enable_datasets(zpool_handle_t *zh
         */
        qsort(cb.cb_datasets, cb.cb_used, sizeof (void *), dataset_cmp);

+       spa_version = zpool_prop_get_int(zhp, ZPOOL_PROP_VERSION, NULL);
+
        /*
         * And mount all the datasets, keeping track of which ones
         * succeeded or failed. By using zfs_alloc(), the good pointer
@@ -1207,10 +1210,13 @@ zpool_enable_datasets(zpool_handle_t *zh
        ret = 0;
        for (i = 0; i < cb.cb_used; i++) {

-               if ((encrypted == B_TRUE) &&
-                   (zfs_prop_get_int(cb.cb_datasets[i], ZFS_PROP_ENCRYPTION)
-                   == ZIO_CRYPT_OFF || zfs_prop_get_int(cb.cb_datasets[i],
-                   ZFS_PROP_KEYSCOPE) == ZFS_KEYSCOPE_DSL)) {
+               if (spa_version >= SPA_VERSION_CRYPTO && encrypted ==
B_TRUE) {
+                   uint64_t crypt = zfs_prop_get_int(cb.cb_datasets[i],
+                       ZFS_PROP_ENCRYPTION);
+                   uint64_t keyscope = zfs_prop_get_int(cb.cb_datasets[i],
+                       ZFS_PROP_KEYSCOPE);
+                   if (crypt != ZIO_CRYPT_OFF && crypt !=
ZIO_CRYPT_INHERIT
+                       && keyscope != ZFS_KEYSCOPE_POOL)
                        continue;
                }

Note also that I reversed the keyscope check to be anything other than
ZFS_KEYSCOPE_POOL rather than checking for ZFS_KEYSCOPE_DSL, this ensures that
we don''t have to change this code in the future when
ZFS_KEYSCOPE_POOL_DSL
(where keys are double wrapped with both the pool and dsl key) is added in a
future phase.

However I''ll leave it to Tony to check and implement this because he
added this
code.

-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757


Darren J Moffat <darrenm at opensolaris.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |CAUSEKNOWN




-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757


Darren J Moffat <darrenm at opensolaris.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CAUSEKNOWN                  |FIXINPROGRESS




--- Comment #2 from Darren J Moffat <darrenm at opensolaris.org> 
2008-05-02 09:07:05 ---
Functions changed to return boolean_t and new kstats added.

-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757


ajscarp at yahoo.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|FIXINPROGRESS               |FIXUNDERSTOOD




--- Comment #3 from ajscarp at yahoo.com  2008-05-02 11:30:54 ---
that was the problem..  it makes sense that the problem was during import..

-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757


ajscarp at yahoo.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|FIXUNDERSTOOD               |RESOLVED
         Resolution|                            |FIXINSOURCE




-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

http://defect.opensolaris.org/bz/show_bug.cgi?id=1757





--- Comment #4 from Darren J Moffat <darrenm at opensolaris.org> 
2008-05-08 03:01:16 ---
I''m doing a tiny followup change to what Tony integrated so that
instead of
passing over datasets with keyscope == ZFS_KEYSCOPE_DSL we pass over those that
don''t have ZFS_KEYSCOPE_POOL.  At the moment this is equivalent but
this will
change when more keyscope values are made available in a later phase.

-- 
Configure bugmail: http://defect.opensolaris.org/bz/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.