ARC changes to fix memory related crashes with encryption

- Bail early in arc_buf_fill() when hdr->b_crypt_hdr.b_rabd is NULL - In arc_write(), avoid arc_hdr_free_abd() when HDR_IO_IN_PROGRESS indicates it's still in use. - Added additional zfs debug message loggging for unexpected edge cases to assist in future diagnosis. Sponsored-By: Odoo SA Sponsored-By: Klara Inc. Signed-off-by: Don Brady <[email protected]>
openzfs · Nov 16, 2023 · 3090be2 · 3090be2
1 parent 5796e3a
commit 3090be2
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 5 deletions.
diff --git a/module/zfs/arc.c b/module/zfs/arc.c
@@ -2008,6 +2008,20 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb,
 	 * further transforms on it.
 	 */
 	if (encrypted) {
+		if (hdr->b_crypt_hdr.b_rabd == NULL) {
+			zfs_dbgmsg("hdr rabd is NULL! compress=%d encrypted=%d "
+			    "psize=%d lsize=%d objset=%llu object=%llu "
+			    "iopro=%d hdrempty=%d hdrlock=%d refcount=%llu",
+			    arc_hdr_get_compress(hdr), encrypted,
+			    HDR_GET_PSIZE(hdr), HDR_GET_LSIZE(hdr),
+			    (u_longlong_t)zb->zb_objset,
+			    (u_longlong_t)zb->zb_object,
+			    HDR_IO_IN_PROGRESS(hdr), HDR_EMPTY(hdr),
+			    MUTEX_HELD(HDR_LOCK(hdr)),
+			    (u_longlong_t)zfs_refcount_count(
+			    &hdr->b_l1hdr.b_refcnt));
+			return (ECKSUM);
+		}
 		ASSERT(HDR_HAS_RABD(hdr));
 		abd_copy_to_buf(buf->b_data, hdr->b_crypt_hdr.b_rabd,
 		    HDR_GET_PSIZE(hdr));
@@ -2056,6 +2070,18 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb,
 
 			if (hash_lock != NULL)
 				mutex_enter(hash_lock);
+			if (hdr->b_l1hdr.b_pabd == NULL) {
+				zfs_dbgmsg("in-place dnode but pabd is NULL! "
+				    "compress=%d encrypted=%d psize=%d "
+				    "lsize=%d objset=%llu object=%llu",
+				    arc_hdr_get_compress(hdr), encrypted,
+				    HDR_GET_PSIZE(hdr), HDR_GET_LSIZE(hdr),
+				    (u_longlong_t)zb->zb_objset,
+				    (u_longlong_t)zb->zb_object);
+				if (hash_lock != NULL)
+					mutex_exit(hash_lock);
+				return (EACCES);
+			}
 			arc_buf_untransform_in_place(buf);
 			if (hash_lock != NULL)
 				mutex_exit(hash_lock);
@@ -2071,6 +2097,15 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, const zbookmark_phys_t *zb,
 		if (ARC_BUF_SHARED(buf)) {
 			ASSERT(arc_buf_is_shared(buf));
 		} else {
+			if (hdr->b_l1hdr.b_pabd == NULL) {
+				zfs_dbgmsg("hdr pabd is NULL! compress=%d "
+				    "encrypted=%d psize=%d lsize=%d "
+				    "objset=%llu object=%llu",
+				    arc_hdr_get_compress(hdr), encrypted,
+				    HDR_GET_PSIZE(hdr), HDR_GET_LSIZE(hdr),
+				    (u_longlong_t)zb->zb_objset,
+				    (u_longlong_t)zb->zb_object);
+			}
 			abd_copy_to_buf(buf->b_data, hdr->b_l1hdr.b_pabd,
 			    arc_buf_size(buf));
 		}
@@ -2739,6 +2774,10 @@ arc_buf_alloc_impl(arc_buf_hdr_t *hdr, spa_t *spa, const zbookmark_phys_t *zb,
 	 * hold the hash_lock or be undiscoverable.
 	 */
 	ASSERT(HDR_EMPTY_OR_LOCKED(hdr));
+	if (!HDR_EMPTY_OR_LOCKED(hdr)) {
+		zfs_dbgmsg("without HDR_EMPTY_OR_LOCKED. empty=%d locked=%d",
+		    HDR_EMPTY(hdr), MUTEX_HELD(HDR_LOCK(hdr)));
+	}
 
 	/*
 	 * Only honor requests for compressed bufs if the hdr is actually
@@ -3165,6 +3204,11 @@ arc_hdr_alloc_abd(arc_buf_hdr_t *hdr, int alloc_flags)
 	IMPLY(alloc_rdata, HDR_PROTECTED(hdr));
 
 	if (alloc_rdata) {
+		if (HDR_PROTECTED(hdr) == B_FALSE) {
+			zfs_dbgmsg("allocating rabd on a not-encrypted HDR");
+		}
+		ASSERT(HDR_PROTECTED(hdr));
+
 		size = HDR_GET_PSIZE(hdr);
 		ASSERT3P(hdr->b_crypt_hdr.b_rabd, ==, NULL);
 		hdr->b_crypt_hdr.b_rabd = arc_get_data_abd(hdr, size, hdr,
@@ -3265,8 +3309,11 @@ arc_hdr_alloc(uint64_t spa, int32_t psize, int32_t lsize,
 	arc_hdr_set_flags(hdr, arc_bufc_to_flags(type) | ARC_FLAG_HAS_L1HDR);
 	arc_hdr_set_compress(hdr, compression_type);
 	hdr->b_complevel = complevel;
-	if (protected)
+	if (protected) {
 		arc_hdr_set_flags(hdr, ARC_FLAG_PROTECTED);
+		if (hdr->b_crypt_hdr.b_rabd != NULL)
+			zfs_dbgmsg("allocating but rabd is not NULL");
+	}
 
 	hdr->b_l1hdr.b_state = arc_anon;
 	hdr->b_l1hdr.b_arc_access = 0;
@@ -6704,8 +6751,17 @@ arc_write(zio_t *pio, spa_t *spa, uint64_t txg,
 		VERIFY3P(buf->b_data, !=, NULL);
 	}
 
-	if (HDR_HAS_RABD(hdr))
-		arc_hdr_free_abd(hdr, B_TRUE);
+	if (HDR_HAS_RABD(hdr)) {
+		if (HDR_IO_IN_PROGRESS(hdr)) {
+			zfs_dbgmsg("tried to free an rABD while IO was in "
+			    "progress refcount=%llu",
+			    (u_longlong_t)zfs_refcount_count(
+			    &hdr->b_l1hdr.b_refcnt));
+		} else {
+			arc_hdr_free_abd(hdr, B_TRUE);
+		}
+
+	}
 
 	if (!(zio_flags & ZIO_FLAG_RAW))
 		arc_hdr_set_compress(hdr, ZIO_COMPRESS_OFF);

diff --git a/module/zfs/sa.c b/module/zfs/sa.c
@@ -360,7 +360,7 @@ sa_attr_op(sa_handle_t *hdl, sa_bulk_attr_t *bulk, int count,
 			}
 		}
 		if (error && error != ENOENT) {
-			return ((error == ECKSUM) ? EIO : error);
+			return (error);
 		}
 
 		switch (data_op) {
@@ -1114,7 +1114,7 @@ sa_setup(objset_t *os, uint64_t sa_obj, const sa_attr_reg_t *reg_attrs,
 	avl_destroy(&sa->sa_layout_num_tree);
 	mutex_destroy(&sa->sa_lock);
 	kmem_free(sa, sizeof (sa_os_t));
-	return ((error == ECKSUM) ? EIO : error);
+	return (error);
 }
 
 void