race window in zvol_open() resulting in use after free, zv_name modified without checking zv_open_count, deadlock between zvol_open() and __zvol_create_minor() #4344
Milestone
Comments
bprotopopov
changed the title
|
The work is done on branch issue-4344 |
|
Tested as follows. |
This was referenced Feb 19, 2016
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this issue
Feb 22, 2016
Close the race window in zvol_open() to prevent removal of zvol_state in the 'first open' code path. Move the call to check_disk_change() under zvol_state_lock to make sure the zvol_media_changed() and zvol_revalidate_disk() called by check_disk_change() are invoked with positive zv_open_count. Skip opened zvols when removing minors and set private_data to NULL for zvols that are not in use whose minors are being removed, to indicate to zvol_open() that the state is gone. Skip opened zvols when renaming minors to avoid modifying zv_name that might be in use, e.g. in zvol_ioctl(). Drop zvol_state_lock before calling add_disk() when creating minors to avoid deadlocks with zvol_open(). Wrap dmu_objset_find() with spl_fstran_mark()/unmark(). Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com> Signed-off-by: Richard Yao <ryao@gentoo.org> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#4344
nedbass
pushed a commit
that referenced
this issue
Mar 23, 2016
Close the race window in zvol_open() to prevent removal of zvol_state in the 'first open' code path. Move the call to check_disk_change() under zvol_state_lock to make sure the zvol_media_changed() and zvol_revalidate_disk() called by check_disk_change() are invoked with positive zv_open_count. Skip opened zvols when removing minors and set private_data to NULL for zvols that are not in use whose minors are being removed, to indicate to zvol_open() that the state is gone. Skip opened zvols when renaming minors to avoid modifying zv_name that might be in use, e.g. in zvol_ioctl(). Drop zvol_state_lock before calling add_disk() when creating minors to avoid deadlocks with zvol_open(). Wrap dmu_objset_find() with spl_fstran_mark()/unmark(). Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <ryao@gentoo.org> Closes #4344
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
A pointer to zvol_state_t for a given zvol is stored in bdev->bd_disk->private_data so that it can be used in various zvol device methods. It is assumed that such use occurs in the context of an open file descriptor referencing the zvol in question, and the corresponding reference count (zv_open_count) is maintained in the zvol_state_t structure.
This allows for a simple way of knowing when it is and it not OK to free that structure in zvol_free() - one can check the zv_open_count under zvol_state_lock, and if it is positive, the structure is in use, and should be left alone; zvol_remove_minors() should follow this rule.
There is an issue with the code path in zvol_open() when zv_open_count is zero on entry to the function. The private_data is saved in the local variable while not under protection of zvol_state_lock, and only then the lock is taken. This creates a small race window for zvol_remove_minors() to come in take the lock, find the zv_open_count equal to zero, and call zvol_free(), leaving the zvol_open() code path to experience the "use after free" situation.
Function zvol_rename_minors() does not check the reference count before modifying the zv_name, which is not good since zvol_ioctl(), for instance, accesses zv_name without holding zvol_state_lock (but with zv_open_count being positive).
Function __zvol_create_minor() calls add_disk() while holding zvol_state_lock, which results in a deadlock with zvol_open() if both are invoked concurrently.
The text was updated successfully, but these errors were encountered: