OpenZFS 9439 - ZFS double-free due to failure to dirty indirect block

Follow up commit for OpenZFS 9438.  See the OpenZFS-issue link below
for a complete analysis.

Authored by: Matthew Ahrens <mahrens@delphix.com>
Reviewed by: George Wilson <george.wilson@delphix.com>
Reviewed by: Paul Dagnelie <pcd@delphix.com>
Approved by: Robert Mustacchi <rm@joyent.com>
Ported-by: Brian Behlendorf <behlendorf1@llnl.gov>
OpenZFS-issue: https://illumos.org/issues/9439
OpenZFS-commit: https://github.com/openzfs/openzfs/commit/779220d
External-issue: DLPX-46861
Closes #7746

diff --git a/module/zfs/dnode.c b/module/zfs/dnode.c
index 0be72e90a6bf59eb515a9050eab9e2bb05809553..26471fda0b2a63245edf70b5edf5fa04f4bee146 100644 (file)
--- a/module/zfs/dnode.c
+++ b/module/zfs/dnode.c
@@ -1987,13 +1987,11 @@ dnode_free_range(dnode_t *dn, uint64_t off, uint64_t len, dmu_tx_t *tx)
                if (off == 0 && len >= blksz) {
                        /*
                         * Freeing the whole block; fast-track this request.
-                        * Note that we won't dirty any indirect blocks,
-                        * which is fine because we will be freeing the entire
-                        * file and thus all indirect blocks will be freed
-                        * by free_children().
                         */
                        blkid = 0;
                        nblks = 1;
+                       if (dn->dn_nlevels > 1)
+                               dnode_dirty_l1(dn, 0, tx);
                        goto done;
                } else if (off >= blksz) {
                        /* Freeing past end-of-data */

diff --git a/module/zfs/dnode_sync.c b/module/zfs/dnode_sync.c
index 3202faf49dac43319f714bbcc95363ae42516d9a..b1f734a8210e289fc7272c528da291d9d774d29d 100644 (file)
--- a/module/zfs/dnode_sync.c
+++ b/module/zfs/dnode_sync.c
@@ -264,6 +264,24 @@ free_children(dmu_buf_impl_t *db, uint64_t blkid, uint64_t nblks,
        if (db->db_state != DB_CACHED)
                (void) dbuf_read(db, NULL, DB_RF_MUST_SUCCEED);
 
+       /*
+        * If we modify this indirect block, and we are not freeing the
+        * dnode (!free_indirects), then this indirect block needs to get
+        * written to disk by dbuf_write().  If it is dirty, we know it will
+        * be written (otherwise, we would have incorrect on-disk state
+        * because the space would be freed but still referenced by the BP
+        * in this indirect block).  Therefore we VERIFY that it is
+        * dirty.
+        *
+        * Our VERIFY covers some cases that do not actually have to be
+        * dirty, but the open-context code happens to dirty.  E.g. if the
+        * blocks we are freeing are all holes, because in that case, we
+        * are only freeing part of this indirect block, so it is an
+        * ancestor of the first or last block to be freed.  The first and
+        * last L1 indirect blocks are always dirtied by dnode_free_range().
+        */
+       VERIFY(BP_GET_FILL(db->db_blkptr) == 0 || db->db_dirtycnt > 0);
+
        dbuf_release_bp(db);
        bp = db->db.db_data;