> On Jan. 16, 2015, 11:20 p.m., Matthew Ahrens wrote: > > dsl_prop_get_ds() used to work even if ds_phys was NULL. Now that ds_phys > > is implied by ds_dbuf, could we fix this by making it work if ds_dbuf is > > NULL? i.e. > > > > if (ds->dbuf != NULL) > > zapobj = dsl_dataset_phys(ds)->ds_props_obj;
Without a hold on the dataset, ds->ds_dbuf can be evicted at any time. You could fully lose the race by seeing NULL, or you could partially lose the race and find a non-zero pointer to memory that is or will be repurposed. This is why a NULL check isn't sufficient. > On Jan. 16, 2015, 11:20 p.m., Matthew Ahrens wrote: > > usr/src/uts/common/fs/zfs/dsl_prop.c, line 444 > > <https://reviews.csiden.org/r/153/diff/1/?file=13801#file13801line444> > > > > How can this fail? If it failed, how do we know that the callbacks > > will be removed eventually? The *_try_addref() APIs fail if the dbuf (bonus buffer in this case) isn't in the dbuf hash table, or the dbuf has been repurposed, or its holds <= dirties. Since dsl_dataset_hold_obj() uses the dmu_buf_set_user_ie() API (eviction occurs when holds == dirties), a failure of dsl_dataset_try_add_ref() means eviction processing has started for the dsl_dataset, but the asynchronous call to dsl_datset_evict() hasn't progressed to the point of actually removing the callback item. - Justin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/153/#review443 ----------------------------------------------------------- On Jan. 16, 2015, 11:44 p.m., Justin Gibbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/153/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 11:44 p.m.) > > > Review request for OpenZFS Developer Mailing List. > > > Bugs: 5531 > https://www.illumos.org/projects/illumos-gate//issues/5531 > > > Repository: illumos-gate > > > Description > ------- > > 5531 NULL pointer dereference in dsl_prop_get_ds() > > Close race between dataset property callbacks and dataset eviction by > attempting to add a temporary hold on the dataset and only calling the > callback if it succeeds. > > uts/common/fs/zfs/sys/dbuf.h: > uts/common/fs/zfs/dbuf.c: > uts/common/fs/zfs/dnode_sync.c: > Modify the function signature of dbuf_find() so that the objset > and object can be passed directly, rather than requiring a > dnode. > > uts/common/fs/zfs/sys/dbuf.h: > uts/common/fs/zfs/sys/dmu.h: > uts/common/fs/zfs/sys/dsl_dataset.h: > uts/common/fs/zfs/dbuf.c: > uts/common/fs/zfs/dsl_dataset.c: > Add dbuf_try_add_ref() and dmu_buf_try_add_ref() as an alias. > This API attempts 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 (dbuf_find() > finds a dbuf at the same address referencing the same data), is > ineligible for eviction, and has at least one hold by a user other > than the syncer (holds > dirties). > > uts/common/fs/zfs/dsl_dataset.c: > Implement dsl_dataset_try_add_ref(), which performs a > dmu_buf_try_add_ref on the bonus buffer representing the dataset. > > uts/common/fs/zfs/dsl_prop.c: > When executing property callbacks on datasets, the dsl_dir > lock of the dsl_dir holding the callback list is held. This > gurantees that the callback list and callback entries are coherent. > This same lock must also be acquired during eviction processing of > the dataset/objset in order to deregister the callback entry. > While this ensures that the dataset object the callback entry points > to is valid, if eviction processing for the dataset has started, > the ds_dbuf field in the datset object will be invalid. > Use dsl_dataset_try_add_ref() to verify that the dataset has not > begun eviction processing and to prevent eviction from occuring > for the duration of the callback. > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/sys/dsl_dataset.h > f0c78c971bef6ddd7b60b09e4f04b96c6ffc0357 > usr/src/uts/common/fs/zfs/sys/dmu.h > 9b8b9ddc2016fe2b3e93a3865a0e31ac4398f062 > usr/src/uts/common/fs/zfs/sys/dbuf.h > 6da5dccc05f7bc875815a0c025903ad8515644b4 > usr/src/uts/common/fs/zfs/dsl_prop.c > e795d735c6f0dcc576d1182354dec4e27c5f2e71 > usr/src/uts/common/fs/zfs/dsl_dataset.c > d3ca43af47d11e7d8d0676b5d3163085936274ad > usr/src/uts/common/fs/zfs/dnode_sync.c > 99ecb9e3ff73a12e7395d60e3142f4702862ca3d > usr/src/uts/common/fs/zfs/dbuf.c 49c659a51b4ef8f79bacc91f537eac5bd9fc817a > > Diff: https://reviews.csiden.org/r/153/diff/ > > > Testing > ------- > > > Thanks, > > Justin Gibbs > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
