Decryption error handling improvements

Currently, the decryption and block authentication code in
the ZIO / ARC layers is a bit inconsistent with regards to
the ereports that are produces and the error codes that are
passed to calling functions. This patch ensures that all of
these errors (which begin as ECKSUM) are converted to EIO
before they leave the ZIO or ARC layer and that ereports
are correctly generated on each decryption / authentication
failure.

In addition, this patch fixes a bug in zio_decrypt() where
ECKSUM never gets written to zio->io_error.

Reviewed by: Matt Ahrens <matt@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tom Caputi <tcaputi@datto.com>
Closes #7372

diff --git a/include/sys/arc.h b/include/sys/arc.h
index de280362d4d2160232a5134f5d1881e89494f6ca..9d6bab505a2f1c5d5ca57dcb37f54f81729f802a 100644 (file)
--- a/include/sys/arc.h
+++ b/include/sys/arc.h
@@ -237,7 +237,7 @@ boolean_t arc_is_unauthenticated(arc_buf_t *buf);
 enum zio_compress arc_get_compression(arc_buf_t *buf);
 void arc_get_raw_params(arc_buf_t *buf, boolean_t *byteorder, uint8_t *salt,
     uint8_t *iv, uint8_t *mac);
-int arc_untransform(arc_buf_t *buf, spa_t *spa, uint64_t dsobj,
+int arc_untransform(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb,
     boolean_t in_place);
 void arc_convert_to_raw(arc_buf_t *buf, uint64_t dsobj, boolean_t byteorder,
     dmu_object_type_t ot, const uint8_t *salt, const uint8_t *iv,

diff --git a/include/sys/spa.h b/include/sys/spa.h
index 62832eff0e72de6cece442f6fabee40708eef228..4a1b1410f7446439775fac829d9627eab67c40a9 100644 (file)
--- a/include/sys/spa.h
+++ b/include/sys/spa.h
@@ -1024,7 +1024,8 @@ extern void spa_history_log_internal_dd(dsl_dir_t *dd, const char *operation,
 struct zbookmark_phys;
 extern void spa_log_error(spa_t *spa, const zbookmark_phys_t *zb);
 extern void zfs_ereport_post(const char *class, spa_t *spa, vdev_t *vd,
-    zbookmark_phys_t *zb, zio_t *zio, uint64_t stateoroffset, uint64_t length);
+    const zbookmark_phys_t *zb, zio_t *zio, uint64_t stateoroffset,
+    uint64_t length);
 extern nvlist_t *zfs_event_create(spa_t *spa, vdev_t *vd, const char *type,
     const char *name, nvlist_t *aux);
 extern void zfs_post_remove(spa_t *spa, vdev_t *vd);

diff --git a/include/sys/zio.h b/include/sys/zio.h
index 9d3adb7f50e60d7897cdd0f2c56e34d544f02c4e..be8e18b4bba76652ce129f58ce66ff19416dd331 100644 (file)
--- a/include/sys/zio.h
+++ b/include/sys/zio.h
@@ -649,8 +649,8 @@ extern hrtime_t zio_handle_io_delay(zio_t *zio);
  * Checksum ereport functions
  */
 extern void zfs_ereport_start_checksum(spa_t *spa, vdev_t *vd,
-    zbookmark_phys_t *zb, struct zio *zio, uint64_t offset, uint64_t length,
-    void *arg, struct zio_bad_cksum *info);
+    const zbookmark_phys_t *zb, struct zio *zio, uint64_t offset,
+    uint64_t length, void *arg, struct zio_bad_cksum *info);
 extern void zfs_ereport_finish_checksum(zio_cksum_report_t *report,
     const abd_t *good_data, const abd_t *bad_data, boolean_t drop_if_identical);
 
@@ -658,8 +658,9 @@ extern void zfs_ereport_free_checksum(zio_cksum_report_t *report);
 
 /* If we have the good data in hand, this function can be used */
 extern void zfs_ereport_post_checksum(spa_t *spa, vdev_t *vd,
-    zbookmark_phys_t *zb, struct zio *zio, uint64_t offset, uint64_t length,
-    const abd_t *good_data, const abd_t *bad_data, struct zio_bad_cksum *info);
+    const zbookmark_phys_t *zb, struct zio *zio, uint64_t offset,
+    uint64_t length, const abd_t *good_data, const abd_t *bad_data,
+    struct zio_bad_cksum *info);
 
 /* Called from spa_sync(), but primarily an injection handler */
 extern void spa_handle_ignored_writes(spa_t *spa);

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 81da36a42c5c4acf3a1cc00ec43f4bd947e1e598..893e42d315bb05fed92cbc49d9136cccd57b2bc8 100644 (file)
--- a/module/zfs/arc.c
+++ b/module/zfs/arc.c
@@ -2260,14 +2260,27 @@ byteswap:
  * callers.
  */
 int
-arc_untransform(arc_buf_t *buf, spa_t *spa, uint64_t dsobj, boolean_t in_place)
+arc_untransform(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb,
+    boolean_t in_place)
 {
+       int ret;
        arc_fill_flags_t flags = 0;
 
        if (in_place)
                flags |= ARC_FILL_IN_PLACE;
 
-       return (arc_buf_fill(buf, spa, dsobj, flags));
+       ret = arc_buf_fill(buf, spa, zb->zb_objset, flags);
+       if (ret == ECKSUM) {
+               /*
+                * Convert authentication and decryption errors to EIO
+                * (and generate an ereport) before leaving the ARC.
+                */
+               ret = SET_ERROR(EIO);
+               zfs_ereport_post(FM_EREPORT_ZFS_AUTHENTICATION,
+                   spa, NULL, zb, NULL, 0, 0);
+       }
+
+       return (ret);
 }
 
 /*
@@ -5810,7 +5823,8 @@ arc_read_done(zio_t *zio)
                 * Assert non-speculative zios didn't fail because an
                 * encryption key wasn't loaded
                 */
-               ASSERT((zio->io_flags & ZIO_FLAG_SPECULATIVE) || error == 0);
+               ASSERT((zio->io_flags & ZIO_FLAG_SPECULATIVE) ||
+                   error != ENOENT);
 
                /*
                 * If we failed to decrypt, report an error now (as the zio
@@ -6033,12 +6047,24 @@ top:
                        rc = arc_buf_alloc_impl(hdr, spa, zb->zb_objset,
                            private, encrypted_read, compressed_read,
                            noauth_read, B_TRUE, &buf);
+                       if (rc == ECKSUM) {
+                               /*
+                                * Convert authentication and decryption errors
+                                * to EIO (and generate an ereport) before
+                                * leaving the ARC.
+                                */
+                               rc = SET_ERROR(EIO);
+                               zfs_ereport_post(FM_EREPORT_ZFS_AUTHENTICATION,
+                                   spa, NULL, zb, NULL, 0, 0);
+                       }
                        if (rc != 0) {
                                arc_buf_destroy(buf, private);
                                buf = NULL;
                        }
 
-                       ASSERT((zio_flags & ZIO_FLAG_SPECULATIVE) || rc == 0);
+                       /* assert any errors weren't due to unloaded keys */
+                       ASSERT((zio_flags & ZIO_FLAG_SPECULATIVE) ||
+                           rc != ENOENT);
                } else if (*arc_flags & ARC_FLAG_PREFETCH &&
                    refcount_count(&hdr->b_l1hdr.b_refcnt) == 0) {
                        arc_hdr_set_flags(hdr, ARC_FLAG_PREFETCH);

diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c
index 0ecdbd5833b85e1c343c898a41b03e1b9880665f..18edd783431d490ae7e678e73af1345f564014f2 100644 (file)
--- a/module/zfs/dbuf.c
+++ b/module/zfs/dbuf.c
@@ -1194,8 +1194,10 @@ dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
                    DMU_OT_IS_ENCRYPTED(dn->dn_bonustype) &&
                    (flags & DB_RF_NO_DECRYPT) == 0 &&
                    arc_is_encrypted(dn_buf)) {
+                       SET_BOOKMARK(&zb, dmu_objset_id(db->db_objset),
+                           DMU_META_DNODE_OBJECT, 0, dn->dn_dbuf->db_blkid);
                        err = arc_untransform(dn_buf, dn->dn_objset->os_spa,
-                           dmu_objset_id(dn->dn_objset), B_TRUE);
+                           &zb, B_TRUE);
                        if (err != 0) {
                                DB_DNODE_EXIT(db);
                                mutex_exit(&db->db_mtx);
@@ -1264,8 +1266,7 @@ dbuf_read_impl(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
        if (DBUF_IS_L2CACHEABLE(db))
                aflags |= ARC_FLAG_L2CACHE;
 
-       SET_BOOKMARK(&zb, db->db_objset->os_dsl_dataset ?
-           db->db_objset->os_dsl_dataset->ds_object : DMU_META_OBJSET,
+       SET_BOOKMARK(&zb, dmu_objset_id(db->db_objset),
            db->db.db_object, db->db_level, db->db_blkid);
 
        /*
@@ -1409,9 +1410,12 @@ dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags)
                if (db->db_buf != NULL && (flags & DB_RF_NO_DECRYPT) == 0 &&
                    (arc_is_encrypted(db->db_buf) ||
                    arc_get_compression(db->db_buf) != ZIO_COMPRESS_OFF)) {
+                       zbookmark_phys_t zb;
+
+                       SET_BOOKMARK(&zb, dmu_objset_id(db->db_objset),
+                           db->db.db_object, db->db_level, db->db_blkid);
                        dbuf_fix_old_data(db, spa_syncing_txg(spa));
-                       err = arc_untransform(db->db_buf, spa,
-                           dmu_objset_id(db->db_objset), B_FALSE);
+                       err = arc_untransform(db->db_buf, spa, &zb, B_FALSE);
                        dbuf_set_data(db, db->db_buf);
                }
                mutex_exit(&db->db_mtx);
@@ -3458,6 +3462,8 @@ dbuf_check_crypt(dbuf_dirty_record_t *dr)
        ASSERT(MUTEX_HELD(&db->db_mtx));
 
        if (!dr->dt.dl.dr_raw && arc_is_encrypted(db->db_buf)) {
+               zbookmark_phys_t zb;
+
                /*
                 * Unfortunately, there is currently no mechanism for
                 * syncing context to handle decryption errors. An error
@@ -3465,8 +3471,10 @@ dbuf_check_crypt(dbuf_dirty_record_t *dr)
                 * changed a dnode block and updated the associated
                 * checksums going up the block tree.
                 */
+               SET_BOOKMARK(&zb, dmu_objset_id(db->db_objset),
+                   db->db.db_object, db->db_level, db->db_blkid);
                err = arc_untransform(db->db_buf, db->db_objset->os_spa,
-                   dmu_objset_id(db->db_objset), B_TRUE);
+                   &zb, B_TRUE);
                if (err)
                        panic("Invalid dnode block MAC");
        } else if (dr->dt.dl.dr_raw) {

diff --git a/module/zfs/dmu_objset.c b/module/zfs/dmu_objset.c
index de7bbf894ea3379da6c609e7356160d42e98db6f..8efc3154691e062ec8ef56b61203b14b6cbc0077 100644 (file)
--- a/module/zfs/dmu_objset.c
+++ b/module/zfs/dmu_objset.c
@@ -686,8 +686,12 @@ dmu_objset_own_impl(dsl_dataset_t *ds, dmu_objset_type_t type,
 
        /* if we are decrypting, we can now check MACs in os->os_phys_buf */
        if (decrypt && arc_is_unauthenticated((*osp)->os_phys_buf)) {
+               zbookmark_phys_t zb;
+
+               SET_BOOKMARK(&zb, ds->ds_object, ZB_ROOT_OBJECT,
+                   ZB_ROOT_LEVEL, ZB_ROOT_BLKID);
                err = arc_untransform((*osp)->os_phys_buf, (*osp)->os_spa,
-                   ds->ds_object, B_FALSE);
+                   &zb, B_FALSE);
                if (err != 0)
                        return (err);
 

diff --git a/module/zfs/dmu_send.c b/module/zfs/dmu_send.c
index 36e412bdf32a1c8d218c962ad401ef5b27750412..e0e5fe22c3be1471e3519b6c311a21338b7428cc 100644 (file)
--- a/module/zfs/dmu_send.c
+++ b/module/zfs/dmu_send.c
@@ -983,8 +983,12 @@ dmu_send_impl(void *tag, dsl_pool_t *dp, dsl_dataset_t *to_ds,
         */
        if (!rawok && os->os_encrypted &&
            arc_is_unauthenticated(os->os_phys_buf)) {
+               zbookmark_phys_t zb;
+
+               SET_BOOKMARK(&zb, to_ds->ds_object, ZB_ROOT_OBJECT,
+                   ZB_ROOT_LEVEL, ZB_ROOT_BLKID);
                err = arc_untransform(os->os_phys_buf, os->os_spa,
-                   to_ds->ds_object, B_FALSE);
+                   &zb, B_FALSE);
                if (err != 0) {
                        dsl_pool_rele(dp, tag);
                        return (err);

diff --git a/module/zfs/zfs_fm.c b/module/zfs/zfs_fm.c
index b3c38f5b21701489122348ea6bb48816564eaf58..e28e46e7ad3181509de844d72bb0ccdce0b8113d 100644 (file)
--- a/module/zfs/zfs_fm.c
+++ b/module/zfs/zfs_fm.c
@@ -142,7 +142,7 @@ zfs_is_ratelimiting_event(const char *subclass, vdev_t *vd)
 
 static void
 zfs_ereport_start(nvlist_t **ereport_out, nvlist_t **detector_out,
-    const char *subclass, spa_t *spa, vdev_t *vd, zbookmark_phys_t *zb,
+    const char *subclass, spa_t *spa, vdev_t *vd, const zbookmark_phys_t *zb,
     zio_t *zio, uint64_t stateoroffset, uint64_t size)
 {
        nvlist_t *ereport, *detector;
@@ -767,7 +767,8 @@ annotate_ecksum(nvlist_t *ereport, zio_bad_cksum_t *info,
 
 void
 zfs_ereport_post(const char *subclass, spa_t *spa, vdev_t *vd,
-    zbookmark_phys_t *zb, zio_t *zio, uint64_t stateoroffset, uint64_t size)
+    const zbookmark_phys_t *zb, zio_t *zio, uint64_t stateoroffset,
+    uint64_t size)
 {
 #ifdef _KERNEL
        nvlist_t *ereport = NULL;
@@ -788,7 +789,7 @@ zfs_ereport_post(const char *subclass, spa_t *spa, vdev_t *vd,
 }
 
 void
-zfs_ereport_start_checksum(spa_t *spa, vdev_t *vd, zbookmark_phys_t *zb,
+zfs_ereport_start_checksum(spa_t *spa, vdev_t *vd, const zbookmark_phys_t *zb,
     struct zio *zio, uint64_t offset, uint64_t length, void *arg,
     zio_bad_cksum_t *info)
 {
@@ -874,7 +875,7 @@ zfs_ereport_free_checksum(zio_cksum_report_t *rpt)
 
 
 void
-zfs_ereport_post_checksum(spa_t *spa, vdev_t *vd, zbookmark_phys_t *zb,
+zfs_ereport_post_checksum(spa_t *spa, vdev_t *vd, const zbookmark_phys_t *zb,
     struct zio *zio, uint64_t offset, uint64_t length,
     const abd_t *good_data, const abd_t *bad_data, zio_bad_cksum_t *zbc)
 {

diff --git a/module/zfs/zio.c b/module/zfs/zio.c
index 44cf984d0b00c75c95ef46c4d1e646e92a2bb640..c6379bfd4c4bf58f09649e32244b575aaf6a1822 100644 (file)
--- a/module/zfs/zio.c
+++ b/module/zfs/zio.c
@@ -506,7 +506,7 @@ error:
         * the io_error. If this was not a speculative zio, create an ereport.
         */
        if (ret == ECKSUM) {
-               ret = SET_ERROR(EIO);
+               zio->io_error = SET_ERROR(EIO);
                if ((zio->io_flags & ZIO_FLAG_SPECULATIVE) == 0) {
                        zfs_ereport_post(FM_EREPORT_ZFS_AUTHENTICATION,
                            spa, NULL, &zio->io_bookmark, zio, 0, 0);