Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use-after-free in vdev_disk_physio_completion #3920

Closed
wants to merge 1 commit into from

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Oct 13, 2015

Currently, vdev_disk_physio_completion will try to wake up an waiter without
first checking the existence. This creates a race window in which complete is
called after dr is freed.

We add dr_wait in dio_request to indicate the existence of waiter. Also,
remove dr_rw since no one is using it, and reorder dr_ref to make the struct
more compact in 64bit.

Signed-off-by: Chunwei Chen [email protected]

Currently, vdev_disk_physio_completion will try to wake up an waiter without
first checking the existence. This creates a race window in which complete is
called after dr is freed.

We add dr_wait in dio_request to indicate the existence of waiter. Also,
remove dr_rw since no one is using it, and reorder dr_ref to make the struct
more compact in 64bit.

Signed-off-by: Chunwei Chen <[email protected]>
@tuxoko
Copy link
Contributor Author

tuxoko commented Oct 13, 2015

Likely fixes #3917 and #3880.

@behlendorf
Copy link
Contributor

Yes, I remember thinking about the no-waiter case in the context of 5592404 but I mistakenly convinced myself this was OK. You're definitely right, the dio_request_t could be freed by the vdev_disk_dio_put() and immediately reused. Assuming nothing is uncovered in the testing I'll merge this to master. LGTM.

@behlendorf behlendorf added this to the 0.6.5.3 milestone Oct 13, 2015
@tuxoko
Copy link
Contributor Author

tuxoko commented Oct 13, 2015

Yeah, well original implementation is also wrong anyway, because it check waiter after put. Which makes the check unreliable.

@behlendorf
Copy link
Contributor

Indeed.

@ryao
Copy link
Contributor

ryao commented Oct 14, 2015

@tuxoko Nice catch.

@behlendorf
Copy link
Contributor

Merged as:

aa159af Fix use-after-free in vdev_disk_physio_completion

@behlendorf behlendorf closed this Oct 15, 2015
MorpheusTeam pushed a commit to Xyratex/lustre-stable that referenced this pull request Dec 3, 2015
Bug Fixes

* Fix CPU hotplug openzfs/spl#482
* Disable dynamic taskqs by default to avoid deadlock
  openzfs/spl#484
* Don't import all visible pools in zfs-import init script
  openzfs/zfs#3777
* Fix use-after-free in vdev_disk_physio_completion
  openzfs/zfs#3920
* Fix avl_is_empty(&dn->dn_dbufs) assertion openzfs/zfs#3865

Signed-off-by: Nathaniel Clark <[email protected]>
Change-Id: I36347630be2506bee4ff0a05f1b236ba2ba7a0ae
Reviewed-on: http://review.whamcloud.com/16877
Reviewed-by: Alex Zhuravlev <[email protected]>
Reviewed-by: Andreas Dilger <[email protected]>
Tested-by: Jenkins
Tested-by: Maloo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants