Date: Sun, 9 Aug 2015 19:26:22 +0000 (UTC) From: Alexander Motin <mav@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r286541 - in head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs: . sys Message-ID: <201508091926.t79JQMrW093808@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mav Date: Sun Aug 9 19:26:21 2015 New Revision: 286541 URL: https://svnweb.freebsd.org/changeset/base/286541 Log: MFV 286540: 5531 NULL pointer dereference in dsl_prop_get_ds() Reviewed by: Matthew Ahrens <mahrens@delphix.com> Reviewed by: Dan McDonald <danmcd@omniti.com> Reviewed by: George Wilson <george@delphix.com> Reviewed by: Bayard Bell <buffer.g.overflow@gmail.com> Approved by: Robert Mustacchi <rm@joyent.com> Author: Justin T. Gibbs <justing@spectralogic.com> illumos/illumos-gate@e57a022b8f718889ffa92adbde47a8f08abcdb25 Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dnode_sync.c head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_prop.c head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dbuf.h head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dmu.h head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dsl_dataset.h Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dbuf.c Sun Aug 9 19:26:21 2015 (r286541) @@ -118,11 +118,9 @@ dbuf_hash(void *os, uint64_t obj, uint8_ (dbuf)->db_blkid == (blkid)) dmu_buf_impl_t * -dbuf_find(dnode_t *dn, uint8_t level, uint64_t blkid) +dbuf_find(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid) { dbuf_hash_table_t *h = &dbuf_hash_table; - objset_t *os = dn->dn_objset; - uint64_t obj = dn->dn_object; uint64_t hv = DBUF_HASH(os, obj, level, blkid); uint64_t idx = hv & h->hash_table_mask; dmu_buf_impl_t *db; @@ -142,6 +140,24 @@ dbuf_find(dnode_t *dn, uint8_t level, ui return (NULL); } +static dmu_buf_impl_t * +dbuf_find_bonus(objset_t *os, uint64_t object) +{ + dnode_t *dn; + dmu_buf_impl_t *db = NULL; + + if (dnode_hold(os, object, FTAG, &dn) == 0) { + rw_enter(&dn->dn_struct_rwlock, RW_READER); + if (dn->dn_bonus != NULL) { + db = dn->dn_bonus; + mutex_enter(&db->db_mtx); + } + rw_exit(&dn->dn_struct_rwlock); + dnode_rele(dn, FTAG); + } + return (db); +} + /* * Insert an entry into the hash table. If there is already an element * equal to elem in the hash table, then the already existing element @@ -1852,7 +1868,7 @@ dbuf_prefetch(dnode_t *dn, uint64_t blki return; /* dbuf_find() returns with db_mtx held */ - if (db = dbuf_find(dn, 0, blkid)) { + if (db = dbuf_find(dn->dn_objset, dn->dn_object, 0, blkid)) { /* * This dbuf is already in the cache. We assume that * it is already CACHED, or else about to be either @@ -1899,7 +1915,7 @@ dbuf_hold_impl(dnode_t *dn, uint8_t leve *dbp = NULL; top: /* dbuf_find() returns with db_mtx held */ - db = dbuf_find(dn, level, blkid); + db = dbuf_find(dn->dn_objset, dn->dn_object, level, blkid); if (db == NULL) { blkptr_t *bp = NULL; @@ -2035,6 +2051,30 @@ dbuf_add_ref(dmu_buf_impl_t *db, void *t ASSERT(holds > 1); } +#pragma weak dmu_buf_try_add_ref = dbuf_try_add_ref +boolean_t +dbuf_try_add_ref(dmu_buf_t *db_fake, objset_t *os, uint64_t obj, uint64_t blkid, + void *tag) +{ + dmu_buf_impl_t *db = (dmu_buf_impl_t *)db_fake; + dmu_buf_impl_t *found_db; + boolean_t result = B_FALSE; + + if (db->db_blkid == DMU_BONUS_BLKID) + found_db = dbuf_find_bonus(os, obj); + else + found_db = dbuf_find(os, obj, 0, blkid); + + if (found_db != NULL) { + if (db == found_db && dbuf_refcount(db) > db->db_dirtycnt) { + (void) refcount_add(&db->db_holds, tag); + result = B_TRUE; + } + mutex_exit(&db->db_mtx); + } + return (result); +} + /* * If you call dbuf_rele() you had better not be referencing the dnode handle * unless you have some other direct or indirect hold on the dnode. (An indirect Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dnode_sync.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dnode_sync.c Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dnode_sync.c Sun Aug 9 19:26:21 2015 (r286541) @@ -76,7 +76,8 @@ dnode_increase_indirection(dnode_t *dn, /* set dbuf's parent pointers to new indirect buf */ for (i = 0; i < nblkptr; i++) { - dmu_buf_impl_t *child = dbuf_find(dn, old_toplvl, i); + dmu_buf_impl_t *child = + dbuf_find(dn->dn_objset, dn->dn_object, old_toplvl, i); if (child == NULL) continue; Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dataset.c Sun Aug 9 19:26:21 2015 (r286541) @@ -369,6 +369,13 @@ dsl_dataset_snap_remove(dsl_dataset_t *d return (err); } +boolean_t +dsl_dataset_try_add_ref(dsl_pool_t *dp, dsl_dataset_t *ds, void *tag) +{ + return (dmu_buf_try_add_ref(ds->ds_dbuf, dp->dp_meta_objset, + ds->ds_object, DMU_BONUS_BLKID, tag)); +} + int dsl_dataset_hold_obj(dsl_pool_t *dp, uint64_t dsobj, void *tag, dsl_dataset_t **dsp) Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_prop.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_prop.c Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_prop.c Sun Aug 9 19:26:21 2015 (r286541) @@ -442,9 +442,31 @@ dsl_prop_notify_all_cb(dsl_pool_t *dp, d cbr = list_next(&dd->dd_prop_cbs, cbr)) { uint64_t value; + /* + * Callback entries do not have holds on their datasets + * so that datasets with registered callbacks are still + * eligible for eviction. Unlike operations on callbacks + * for a single dataset, we are performing a recursive + * descent of related datasets and the calling context + * for this iteration only has a dataset hold on the root. + * Without a hold, the callback's pointer to the dataset + * could be invalidated by eviction at any time. + * + * Use dsl_dataset_try_add_ref() to verify that the + * dataset has not begun eviction processing and to + * prevent eviction from occurring for the duration + * of the callback. If the hold attempt fails, this + * object is already being evicted and the callback can + * be safely ignored. + */ + if (!dsl_dataset_try_add_ref(dp, cbr->cbr_ds, FTAG)) + continue; + if (dsl_prop_get_ds(cbr->cbr_ds, cbr->cbr_propname, sizeof (value), 1, &value, NULL) == 0) cbr->cbr_func(cbr->cbr_arg, value); + + dsl_dataset_rele(cbr->cbr_ds, FTAG); } mutex_exit(&dd->dd_lock); @@ -497,19 +519,28 @@ dsl_prop_changed_notify(dsl_pool_t *dp, mutex_enter(&dd->dd_lock); for (cbr = list_head(&dd->dd_prop_cbs); cbr; cbr = list_next(&dd->dd_prop_cbs, cbr)) { - uint64_t propobj = dsl_dataset_phys(cbr->cbr_ds)->ds_props_obj; + uint64_t propobj; - if (strcmp(cbr->cbr_propname, propname) != 0) + /* + * cbr->cbf_ds may be invalidated due to eviction, + * requiring the use of dsl_dataset_try_add_ref(). + * See comment block in dsl_prop_notify_all_cb() + * for details. + */ + if (strcmp(cbr->cbr_propname, propname) != 0 || + !dsl_dataset_try_add_ref(dp, cbr->cbr_ds, FTAG)) continue; + propobj = dsl_dataset_phys(cbr->cbr_ds)->ds_props_obj; + /* - * If the property is set on this ds, then it is not - * inherited here; don't call the callback. + * If the property is not set on this ds, then it is + * inherited here; call the callback. */ - if (propobj && 0 == zap_contains(mos, propobj, propname)) - continue; + if (propobj == 0 || zap_contains(mos, propobj, propname) != 0) + cbr->cbr_func(cbr->cbr_arg, value); - cbr->cbr_func(cbr->cbr_arg, value); + dsl_dataset_rele(cbr->cbr_ds, FTAG); } mutex_exit(&dd->dd_lock); Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dbuf.h ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dbuf.h Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dbuf.h Sun Aug 9 19:26:21 2015 (r286541) @@ -264,12 +264,15 @@ int dbuf_hold_impl(struct dnode *dn, uin void dbuf_prefetch(struct dnode *dn, uint64_t blkid, zio_priority_t prio); void dbuf_add_ref(dmu_buf_impl_t *db, void *tag); +boolean_t dbuf_try_add_ref(dmu_buf_t *db, objset_t *os, uint64_t obj, + uint64_t blkid, void *tag); uint64_t dbuf_refcount(dmu_buf_impl_t *db); void dbuf_rele(dmu_buf_impl_t *db, void *tag); void dbuf_rele_and_unlock(dmu_buf_impl_t *db, void *tag); -dmu_buf_impl_t *dbuf_find(struct dnode *dn, uint8_t level, uint64_t blkid); +dmu_buf_impl_t *dbuf_find(struct objset *os, uint64_t object, uint8_t level, + uint64_t blkid); int dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags); void dmu_buf_will_not_fill(dmu_buf_t *db, dmu_tx_t *tx); Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dmu.h ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dmu.h Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dmu.h Sun Aug 9 19:26:21 2015 (r286541) @@ -462,7 +462,23 @@ int dmu_spill_hold_existing(dmu_buf_t *b */ int dmu_buf_hold(objset_t *os, uint64_t object, uint64_t offset, void *tag, dmu_buf_t **, int flags); + +/* + * Add a reference to a dmu buffer that has already been held via + * dmu_buf_hold() in the current context. + */ void dmu_buf_add_ref(dmu_buf_t *db, void* tag); + +/* + * Attempt to add a reference to a dmu buffer that is in an unknown state, + * using a pointer that may have been invalidated by eviction processing. + * The request will succeed if the passed in dbuf still represents the + * same os/object/blkid, is ineligible for eviction, and has at least + * one hold by a user other than the syncer. + */ +boolean_t dmu_buf_try_add_ref(dmu_buf_t *, objset_t *os, uint64_t object, + uint64_t blkid, void *tag); + void dmu_buf_rele(dmu_buf_t *db, void *tag); uint64_t dmu_buf_refcount(dmu_buf_t *db); Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dsl_dataset.h ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dsl_dataset.h Sun Aug 9 19:25:40 2015 (r286540) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/dsl_dataset.h Sun Aug 9 19:26:21 2015 (r286541) @@ -208,6 +208,8 @@ dsl_dataset_is_snapshot(dsl_dataset_t *d int dsl_dataset_hold(struct dsl_pool *dp, const char *name, void *tag, dsl_dataset_t **dsp); +boolean_t dsl_dataset_try_add_ref(struct dsl_pool *dp, dsl_dataset_t *ds, + void *tag); int dsl_dataset_hold_obj(struct dsl_pool *dp, uint64_t dsobj, void *tag, dsl_dataset_t **); void dsl_dataset_rele(dsl_dataset_t *ds, void *tag);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201508091926.t79JQMrW093808>