Fix cloning into already dirty dbufs.

Undirty the dbuf and destroy its buffer when cloning into it. Coverity ID: CID-1535375 Reported-by: Richard Yao Reported-by: Benjamin Coddington Reviewed-by: Richard Yao <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Pawel Jakub Dawidek <[email protected]> Closes #14655
openzfs · Mar 24, 2023 · ce0e1cc · ce0e1cc
1 parent 5b5f518
commit ce0e1cc
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 13 deletions.
diff --git a/include/sys/dbuf.h b/include/sys/dbuf.h
@@ -382,6 +382,7 @@ void dbuf_assign_arcbuf(dmu_buf_impl_t *db, arc_buf_t *buf, dmu_tx_t *tx);
 dbuf_dirty_record_t *dbuf_dirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 dbuf_dirty_record_t *dbuf_dirty_lightweight(dnode_t *dn, uint64_t blkid,
     dmu_tx_t *tx);
+boolean_t dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 arc_buf_t *dbuf_loan_arcbuf(dmu_buf_impl_t *db);
 void dmu_buf_write_embedded(dmu_buf_t *dbuf, void *data,
     bp_embedded_type_t etype, enum zio_compress comp,

diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c
@@ -175,7 +175,6 @@ struct {
 		continue;						\
 }
 
-static boolean_t dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx);
 static void dbuf_write(dbuf_dirty_record_t *dr, arc_buf_t *data, dmu_tx_t *tx);
 static void dbuf_sync_leaf_verify_bonus_dnode(dbuf_dirty_record_t *dr);
 static int dbuf_read_verify_dnode_crypt(dmu_buf_impl_t *db, uint32_t flags);
@@ -2518,7 +2517,7 @@ dbuf_undirty_bonus(dbuf_dirty_record_t *dr)
  * Undirty a buffer in the transaction group referenced by the given
  * transaction.  Return whether this evicted the dbuf.
  */
-static boolean_t
+boolean_t
 dbuf_undirty(dmu_buf_impl_t *db, dmu_tx_t *tx)
 {
 	uint64_t txg = tx->tx_txg;

diff --git a/module/zfs/dmu.c b/module/zfs/dmu.c
@@ -2190,7 +2190,8 @@ dmu_read_l0_bps(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
 	for (int i = 0; i < numbufs; i++) {
 		dbuf = dbp[i];
 		db = (dmu_buf_impl_t *)dbuf;
-		bp = db->db_blkptr;
+
+		mutex_enter(&db->db_mtx);
 
 		/*
 		 * If the block is not on the disk yet, it has no BP assigned.
@@ -2212,10 +2213,16 @@ dmu_read_l0_bps(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
 				 * The block was modified in the same
 				 * transaction group.
 				 */
+				mutex_exit(&db->db_mtx);
 				error = SET_ERROR(EAGAIN);
 				goto out;
 			}
+		} else {
+			bp = db->db_blkptr;
 		}
+
+		mutex_exit(&db->db_mtx);
+
 		if (bp == NULL) {
 			/*
 			 * The block was created in this transaction group,
@@ -2273,19 +2280,23 @@ dmu_brt_clone(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
 		ASSERT(db->db_blkid != DMU_BONUS_BLKID);
 		ASSERT(BP_IS_HOLE(bp) || dbuf->db_size == BP_GET_LSIZE(bp));
 
-		if (db->db_state == DB_UNCACHED) {
-			/*
-			 * XXX-PJD: If the dbuf is already cached, calling
-			 * dmu_buf_will_not_fill() will panic on assertion
-			 * (db->db_buf == NULL) in dbuf_clear_data(),
-			 * which is called from dbuf_noread() in DB_NOFILL
-			 * case. I'm not 100% sure this is the right thing
-			 * to do, but it seems to work.
-			 */
-			dmu_buf_will_not_fill(dbuf, tx);
+		mutex_enter(&db->db_mtx);
+
+		VERIFY(!dbuf_undirty(db, tx));
+		ASSERT(list_head(&db->db_dirty_records) == NULL);
+		if (db->db_buf != NULL) {
+			arc_buf_destroy(db->db_buf, db);
+			db->db_buf = NULL;
 		}
 
+		mutex_exit(&db->db_mtx);
+
+		dmu_buf_will_not_fill(dbuf, tx);
+
+		mutex_enter(&db->db_mtx);
+
 		dr = list_head(&db->db_dirty_records);
+		VERIFY(dr != NULL);
 		ASSERT3U(dr->dr_txg, ==, tx->tx_txg);
 		dl = &dr->dt.dl;
 		dl->dr_overridden_by = *bp;
@@ -2301,6 +2312,8 @@ dmu_brt_clone(objset_t *os, uint64_t object, uint64_t offset, uint64_t length,
 			    BP_PHYSICAL_BIRTH(bp);
 		}
 
+		mutex_exit(&db->db_mtx);
+
 		/*
 		 * When data in embedded into BP there is no need to create
 		 * BRT entry as there is no data block. Just copy the BP as