Page MenuHomeFreeBSD

Fix rare double free in vdev_geom_attrchanged
ClosedPublic

Authored by asomers on Feb 23 2016, 11:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 3:18 AM
Unknown Object (File)
Tue, Nov 5, 8:44 PM
Unknown Object (File)
Oct 27 2024, 8:52 AM
Unknown Object (File)
Oct 4 2024, 11:01 PM
Unknown Object (File)
Oct 1 2024, 12:35 PM
Unknown Object (File)
Sep 29 2024, 8:24 PM
Unknown Object (File)
Sep 18 2024, 7:32 PM
Unknown Object (File)
Sep 8 2024, 2:34 PM
Subscribers

Details

Summary

Don't drop the g_topology_lock before freeing old_physpath. That opens up a
race where one thread can call vdev_geom_attrchanged, set old_physpath, drop
the g_topology_lock, then block trying to acquire the SCL_STATE lock. Then
another thread can come into vdev_geom_attrchanged, set old_physpath to the
same value, and proceed to free it. When the first thread resumes, it will
free the same location.

It turns out that the SCL_STATE lock isn't needed. It was originally added by
gibbs to protect vd->vdev_physpath while updating the same. However, the
update process subsequently was switched to an atomic operation (a pointer
swap). Now, there is no need for the SCL_STATE lock, and hence no need to drop
the g_topology_lock.

Test Plan

2 runs through the ZFS test suite on HEAD. 10 runs on SpectraBSD, which has
some additional changes that increase the probability of the race.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

asomers retitled this revision from to Fix rare double free in vdev_geom_attrchanged.
asomers updated this object.
asomers edited the test plan for this revision. (Show Details)
asomers added a reviewer: gibbs.

This looks like it might leave old data in the zpool.cache?

In D5413#125618, @smh wrote:

This looks like it might leave old data in the zpool.cache?

Please explain?

delphij added a reviewer: delphij.

Looks good to me.

This revision is now accepted and ready to land.Apr 11 2016, 5:40 AM
This revision was automatically updated to reflect the committed changes.