module: zfs: avoid a taking a destroyed lock in zfs_zevent bits #13220

kevans91 · 2022-03-15T15:17:39Z

Motivation and Context

My laptop attempted to shutdown because of thermal issues, but in the process of shutting down it ended up panicking due to this upon unload of the zfs module. Unfortunately, this prevented the thermal situation from improving, but hopefully it'll have better luck next time. :-)

Description

At shutdown time, we drain all of the zevents and set the
ZEVENT_SHUTDOWN flag. In parallel, at least on FreeBSD, we may still be
winding down and call zfs_zevent_destroy() in the process. For
instance, see: sysevent_worker() thread in
module/os/freebsd/spl/spl_sysevent.c -- we'll observe ESHUTDOWN then
back out.

There's no underlying zevent since we've drained zevent_list, so we can
just skip those bits and avoid leaking the zfs_zevent_t on the way out.

How Has This Been Tested?

Loaded, unloaded the module

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.

kevans91 · 2022-03-15T16:00:40Z

It's not immediately clear how this could have caused the ztest crash -- is this an intermittent/unrelated issue?

nabijaczleweli · 2022-03-15T16:43:15Z

It is, yeah

behlendorf · 2022-03-15T17:28:40Z

module/zfs/fm.c

+		if (ze->ze_zevent)
+			list_remove(&ze->ze_zevent->ev_ze_list, ze);
+		mutex_exit(&zevent_lock);
+	}


Perhaps this special handling should instead be placed in the sysevent_worker() thread around the zfs_zevent_destroy() call (or some other mechanism). It looks to me like all other callers of the zevent_lock must be initialized. Let's see if we can get @freqlabs or @amotin's thoughts on this.

No objection either way... we need the below kmem_free(), wasn't sure if we needed another new zfs_zevent_* KPI for just that (the conflation in this namespace of those operating on zevent_t vs. zfs_zevent_t is a bit confusing to me) or if the caller can just kmem_free directly instead.

I am thinking kmem_free in sysevent_worker, with a comment explaining the race we need to avoid.

OK, revised (and fixed the commit message) -- I tossed in an assertion, too.

At shutdown time, we drain all of the zevents and set the ZEVENT_SHUTDOWN flag. On FreeBSD, we may end up calling zfs_zevent_destroy() after the zevent_lock has been destroyed while the sysevent thread is winding down; we observe ESHUTDOWN, then back out. Events have already been drained, so just inline the kmem_free call in sysevent_worker() to avoid the race, and document the assumption that zfs_zevent_destroy doesn't do anything else useful at that point. This fixes a panic that can occur at module unload time. Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

behlendorf

That works for me.

At shutdown time, we drain all of the zevents and set the ZEVENT_SHUTDOWN flag. On FreeBSD, we may end up calling zfs_zevent_destroy() after the zevent_lock has been destroyed while the sysevent thread is winding down; we observe ESHUTDOWN, then back out. Events have already been drained, so just inline the kmem_free call in sysevent_worker() to avoid the race, and document the assumption that zfs_zevent_destroy doesn't do anything else useful at that point. This fixes a panic that can occur at module unload time. Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Kyle Evans <kevans@FreeBSD.org> Closes openzfs#13220

At shutdown time, we drain all of the zevents and set the ZEVENT_SHUTDOWN flag. On FreeBSD, we may end up calling zfs_zevent_destroy() after the zevent_lock has been destroyed while the sysevent thread is winding down; we observe ESHUTDOWN, then back out. Events have already been drained, so just inline the kmem_free call in sysevent_worker() to avoid the race, and document the assumption that zfs_zevent_destroy doesn't do anything else useful at that point. This fixes a panic that can occur at module unload time. Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Kyle Evans <kevans@FreeBSD.org> Closes #13220

At shutdown time, we drain all of the zevents and set the ZEVENT_SHUTDOWN flag. On FreeBSD, we may end up calling zfs_zevent_destroy() after the zevent_lock has been destroyed while the sysevent thread is winding down; we observe ESHUTDOWN, then back out. Events have already been drained, so just inline the kmem_free call in sysevent_worker() to avoid the race, and document the assumption that zfs_zevent_destroy doesn't do anything else useful at that point. This fixes a panic that can occur at module unload time. Reviewed-by: Ryan Moeller <freqlabs@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Kyle Evans <kevans@FreeBSD.org> Closes openzfs#13220

behlendorf reviewed Mar 15, 2022

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Mar 15, 2022

kevans91 force-pushed the shutfix branch 2 times, most recently from 8b71917 to 4f1b654 Compare March 16, 2022 17:07

kevans91 force-pushed the shutfix branch from 4f1b654 to 4647795 Compare March 16, 2022 17:32

ghost approved these changes Mar 16, 2022

View reviewed changes

behlendorf approved these changes Mar 16, 2022

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Mar 16, 2022

ahrens assigned behlendorf Mar 17, 2022

behlendorf merged commit bee314a into openzfs:master Mar 17, 2022

usaleem-ix mentioned this pull request Mar 18, 2022

Backport of #13219 and #13220 to the staging branch #13233

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: zfs: avoid a taking a destroyed lock in zfs_zevent bits #13220

module: zfs: avoid a taking a destroyed lock in zfs_zevent bits #13220

kevans91 commented Mar 15, 2022

kevans91 commented Mar 15, 2022

nabijaczleweli commented Mar 15, 2022 •

edited

Loading

behlendorf Mar 15, 2022

kevans91 Mar 15, 2022

ghost Mar 16, 2022

kevans91 Mar 16, 2022

behlendorf left a comment

module: zfs: avoid a taking a destroyed lock in zfs_zevent bits #13220

module: zfs: avoid a taking a destroyed lock in zfs_zevent bits #13220

Conversation

kevans91 commented Mar 15, 2022

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

kevans91 commented Mar 15, 2022

nabijaczleweli commented Mar 15, 2022 • edited Loading

behlendorf Mar 15, 2022

Choose a reason for hiding this comment

kevans91 Mar 15, 2022

Choose a reason for hiding this comment

ghost Mar 16, 2022

Choose a reason for hiding this comment

kevans91 Mar 16, 2022

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

nabijaczleweli commented Mar 15, 2022 •

edited

Loading