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 <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Tom Caputi <[email protected]>
Closes #7372 

6 files changed, 60 insertions, 17 deletions

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 81da36a42..893e42d31 100644
--- 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 0ecdbd583..18edd7834 100644
--- 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 de7bbf894..8efc31546 100644
--- 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 36e412bdf..e0e5fe22c 100644
--- 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 b3c38f5b2..e28e46e7a 100644
--- 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 44cf984d0..c6379bfd4 100644
--- 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);