----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/153/#review462 -----------------------------------------------------------
Ship it! usr/src/uts/common/fs/zfs/dsl_prop.c <https://reviews.csiden.org/r/153/#comment377> I think it would help to add a comment describing how this might fail and why it's necessary to perform this check. usr/src/uts/common/fs/zfs/dsl_prop.c <https://reviews.csiden.org/r/153/#comment378> Maybe a similar comment here. - George Wilson On Jan. 27, 2015, 5:59 a.m., Justin Gibbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/153/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2015, 5:59 a.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/dsl_prop.c > e795d735c6f0dcc576d1182354dec4e27c5f2e71 > usr/src/uts/common/fs/zfs/dnode_sync.c > 99ecb9e3ff73a12e7395d60e3142f4702862ca3d > usr/src/uts/common/fs/zfs/dbuf.c 49c659a51b4ef8f79bacc91f537eac5bd9fc817a > 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_dataset.c > d3ca43af47d11e7d8d0676b5d3163085936274ad > > Diff: https://reviews.csiden.org/r/153/diff/ > > > Testing > ------- > > ztest via zloop on illumos. ZFS test suite on FreeBSD. > > > Thanks, > > Justin Gibbs > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
