FreeBSD: Clean up zfsdev_close to match Linux

[ghost]

Motivation and Context

We received reports of panics in zfs_onexit_destroy() caused by it being called with a NULL zs_onexit pointer. Upon investigation I discovered oddities in zfsdev_close() that were not present in the equivalent function for Linux.

When opening /dev/zfs, FreeBSD invokes zfsdev_open(), which takes zfsdev_state_lock around a call to zfs_ctldev_init(). Here we allocate a unique minor number and find a free state object in the global state list or allocate a new one if all existing objects are in use. We assign the state to the per-open filedescriptor data along with the destructor zfsdev_close(). This ensures zfsdev_close() is called when the last reference to the descriptor is dropped.

In zfsdev_close(), we receive the pointer to our state and take zfsdev_state_lock. It should be safe to assume this points to a valid state, as we are guaranteed to be called exactly once for this descriptor data and nothing else should be reusing the same state. This is where the existing code went off into the weeds. We tried to find the state object in the global state list to verify the state we have actually exists, which it surely must for us to be here. If we did find our state in the list, it was further validated by looking for a sentinel value in zs_minor to check if it had already been destructed, which it surely hasn't (if the code is correct) because we haven't done that yet. Neither of these checks are performed on Linux.

Now we actually invalidate the state and, still under zfsdev_state_lock, begin to destroy it. Then the lock is released, and the rest of the state is nulled out. Because we are not protected by the lock at this point, another open may have claimed this state before we finished nulling it, leading to the observed panic when the destructor is eventually called for that state. On Linux, the state is fully destroyed and nulled under the protection of the lock.

Description

Eliminate the gratuitous checks.
Assert that we do not have an impossible value in zs_minor, for consistency with Linux.
Keep zfsdev_state_lock held until we have fully destroyed and nulled the state.
While looking at minor numbers I spotted an unused #define ZFS_MIN_MINOR (ZFSDEV_MAX_MINOR + 1), and pruned it.

How Has This Been Tested?

Many rounds of ZTS, then a fixed module was provided to the reporter who reported back that so far they have been able to push the system much harder without encountering the issue.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[ghost]

After this cleanup, I think it should be possible to avoid taking the lock at all by moving the state invalidation to the end of the destructor. Nothing else is supposed to access the fields of the state we need to destroy until zs_minor is set to -1, so doing that last should make the other operations safe without locking.

[amotin]

After this cleanup, I think it should be possible to avoid taking the lock at all by moving the state invalidation to the end of the destructor. Nothing else is supposed to access the fields of the state we need to destroy until zs_minor is set to -1, so doing that last should make the other operations safe without locking.

I think you are right. But there should be some fence around that zs_minor setting to -1, either wmb() or atomic_store_rel_int() to not hit the issue you are just fixing on some less ordered platforms.