Project

General

Profile

Bug #11947

zfs diff on encrypted dataset leaks key reference

Added by Jason King 13 days ago. Updated 1 day ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
zfs - Zettabyte File System
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

Reported by Igor Kozhukhov.

The original issue was a triggered ASSERT on a DEBUG build:

assertion failed: avl_is_empty(&sk->sk_dsl_keys), file: ../../common/fs/zfs/dsl_crypt.c, line: 344

By setting reference_tracking_enable to 1 (via mdb -kw), and then doing the following:

zfs load-key test/test
zfs mount test/test
zfs diff -Ft test/test@1 test/test@2
zpool export test

I could see a hold was placed on the wrapping key in the resulting crash:

> ::spa
ADDR                 STATE NAME
fffffe0c03b58000  EXPORTED test
fffffe0bfff93000    ACTIVE zones
> fffffe0c03b58000::print spa_t spa_keystore.sk_wkeys | ::walk avl | ::print dsl_wrapping_key_t wk_refcnt | ::zfs_refcount
zfs_refcount_t at fffffe0c39fdb548 has 1 current holds, 3 recently released holds
current holds:
reference with tag fffffe0c297e3540 "", held at:
fffffe0bee0f5200 is allocated from reference_cache:
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
fffffe0bec1a8cd0 fffffe0bee0f5200      130ded135bc fffffe0bf1cb7480
                 fffffe0be5cde008 fffffe0bd1393300 fffffe0bdcae1500
                 kmem_cache_alloc_debug+0x2fc
                 kmem_cache_alloc+0x135
                 zfs_refcount_add_many+0x82
                 zfs_refcount_add+0x13
                 dsl_wrapping_key_hold+0xf
                 dsl_crypto_key_open+0x2b8
                 spa_keystore_dsl_key_hold_dd+0x120
                 spa_keystore_create_mapping+0x7e
                 dsl_dataset_create_key_mapping+0x39
                 dsl_dataset_hold_obj_flags+0x68
                 dsl_dataset_hold_flags+0xda
                 dsl_dataset_own+0x3a
                 dmu_objset_own+0xc4
                 zfsvfs_create+0x6c
                 zfs_domount+0x50

I then tried to recreate the test, this time, however doing a zfs unmount test/test immediately prior to zpool export test did not reproduce the panic.

This suggests something the the zpool export path is skipping a step that occurs during zfs unmount which causes the hold on the dsl_wrapping_key_t to be released.

History

#1

Updated by Jason King 12 days ago

A little more research (and looking at the right set of keys :P), showed that doing the zfs diff causes the sk_dsl_key leak:

zfs_refcount_t at fffffe0c4d259058 has 2 current holds, 3 recently released holds
current holds:
reference with tag fffffe0bfcb4f678 "", held at:
fffffe0c9b4f5540 is allocated from reference_cache:
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
fffffe0c957302f8 fffffe0c9b4f5540      2ded0296e87 fffffe0bfdd7bbc0
                 fffffe0be5cbe008 fffffe0bd56567c0 fffffe0be0355e68
                 kmem_cache_alloc_debug+0x2fc
                 kmem_cache_alloc+0x135
                 zfs_refcount_add_many+0x82
                 zfs_refcount_add+0x13
                 spa_keystore_dsl_key_hold_impl+0x81
                 spa_keystore_dsl_key_hold_dd+0x9c
                 spa_keystore_create_mapping+0x7e
                 dsl_dataset_create_key_mapping+0x39
                 dsl_dataset_hold_obj_flags+0x68
                 dsl_dataset_hold_flags+0x1ee
                 dmu_objset_hold_flags+0x8b
                 zfs_ioc_next_obj+0x3b
                 zfsdev_ioctl+0x1fd
                 cdev_ioctl+0x2b
                 spec_ioctl+0x45

Looking at the code,


static int
zfs_ioc_next_obj(zfs_cmd_t *zc)
{
    objset_t *os = NULL;
    int error;

    error = dmu_objset_hold_flags(zc->zc_name, B_TRUE, FTAG, &os);
    if (error != 0)
        return (error);

    error = dmu_object_next(os, &zc->zc_obj, B_FALSE,
        dsl_dataset_phys(os->os_dsl_dataset)->ds_prev_snap_txg);

    dmu_objset_rele(os, FTAG);
    return (error);
}
...
void
dmu_objset_rele_flags(objset_t *os, boolean_t decrypt, void *tag)
{
    ds_hold_flags_t flags = (decrypt) ? DS_HOLD_FLAG_DECRYPT : 0;

    dsl_pool_t *dp = dmu_objset_pool(os);
    dsl_dataset_rele_flags(os->os_dsl_dataset, flags, tag);
    dsl_pool_rele(dp, tag);
}

void
dmu_objset_rele(objset_t *os, void *tag)
{
    dmu_objset_rele_flags(os, B_FALSE, tag);
}

and the cause becomes obvious -- we're holding the objset w/ DS_HOLD_FLAG_DECRYPT, but not releasing it with the flag set (which skips releasing the keys).

Looking at ZoL for comparison, they are just doing dmu_objset_hold(zc->zc_name, FTAG, &os); in zfs_ioc_next_obj. I'm unsure of the reasons for the difference in illumos. It may just be an artifact of the initial illumos port of zfs encryption.

#2

Updated by Jason King 12 days ago

  • Subject changed from export of zpool with mounted and encrypted datasets leaks wrapping key to zfs diff on encrypted dataset leaks key reference
#3

Updated by Electric Monk 8 days ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 28df1ae01e4451dbbaeff3611e2544dda0f99f2b

commit  28df1ae01e4451dbbaeff3611e2544dda0f99f2b
Author: Jason King <jason.king@joyent.com>
Date:   2019-11-13T18:40:50.000Z

    11947 zfs diff on encrypted dataset leaks key reference
    Reviewed by: Andrew Stormont <astormont@racktopsystems.com>
    Reviewed by: Andy Fiddaman <omnios@citrus-it.co.uk>
    Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
    Approved by: Gordon Ross <gordon.w.ross@gmail.com>

#4

Updated by Joshua M. Clulow 1 day ago

Testing Notes (from Jason)

To test, I built a DEBUG SmartOS platform image (PI) with the change (DEBUG illumos-joyent build), and ran the zfs test suite on a VM running the new PI. All the expected tests pass. I also ran the (soon to be RTIed) zfs diff tests on the same PI, which also passed (except for the zfs diff + large dnodes, however that will be fixed with #11950.

In addition, the original trigger of this bug was a DEBUG build ASSERT that gets tripped if a ‘zfs diff’ is done on an encrypted dataset whose zpool is later exported (the ASSERT is triggered because of the leaked key reference). I was able to reproduce the ASSERT panic on an unmodified SmartOS DEBUG image, and could not reproduce it on the DEBUG PI with the fix applied. I also used the (also soon to be RTIed) fixed ::zfs_refcount dcmd (#11944) before and after running ‘zfs diff’ with reference tracking enabled to verify the hold taken by ‘zfs diff’ did not show up after the command completes.

Also available in: Atom PDF