Page MenuHomeFreeBSD

UFS SU: handle races on remounts rw<->ro
ClosedPublic

Authored by kib on Mar 10 2021, 2:59 AM.

Details

Summary

rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.

Instead, set fs_ronly = 1 and clear MNT_SOFTDEP flags very early in the process of unmount, while allowing SU code that needs to modify filesystem for final sync to process by changing the test from MNT_SOFTDEP flag to um_softdep != NULL.
Also ensure that non-NULL um_softdep is fully initialized when observed, which is important for ro->rw remounts.

Nullfs changes are not strictly related but were discovered during tests. They are separate commits with its own explanations.

The split into individual commits is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/ufs

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Mar 10 2021, 2:59 AM
kib created this revision.

In your summary you say ``rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.'' Can you elaborate on how having other processes reading the filesystem causes trouble?

In your summary you say ``rw<->ro remounts are not atomic, filesystem is accessible by other threads during the process. As result, its internal state is inconsistent. Just blocking writers with suspend is not enough.'' Can you elaborate on how having other processes reading the filesystem causes trouble?

Suppose that we remount rw->ro and in parallel some reader tries to instantiate a vnode, e.g. during lookup. Suppose that softdep_unmount() already started, but we did not cleared the MNT_SOFTDEP flag yet. Then ffs_vgetf() calls into softdep_load_inodeblock() which accessed destroyed hashes and freed memory. This was the biggest offender.

The solution I propose is to set fs_ronly early, and clear MNT_SOFTDEP early. Then I need to arrange that existing dependency flush code paths still work.

Nearly all of the changes in ffs_softdep.c are code cleanups and not related to this bug fix. I would prefer to see the code cleanups in a separate commit so that it is easier to see the changes that are needed to fix this problem. That said, this update appears to solve the problem that you describe.

sys/ufs/ffs/ffs_extern.h
131 ↗(On Diff #85457)

It would be helpful to document/comment these flags.

It would help if I had looked at your commit logs before my previous comment. You have in fact separated everything out appropriately.

This revision is now accepted and ready to land.Mar 11 2021, 6:31 AM
kib marked an inline comment as done.

Document ffs_vgetf() flags

This revision now requires review to proceed.Mar 11 2021, 7:04 AM

Nearly all of the changes in ffs_softdep.c are code cleanups and not related to this bug fix. I would prefer to see the code cleanups in a separate commit so that it is easier to see the changes that are needed to fix this problem. That said, this update appears to solve the problem that you describe.

In fact only a simplification for jourmal_mount() (removal of error == 0 check) and some style fixes can be accounted as unrelated. All other non-nullfs changes do fix bugs occuring at rw/ro remounts, or prepare for other commits that are bug fixes.

Flag definitions look good.
Breakdown of commits is excellent.
Changes should resolve the problem.

This revision is now accepted and ready to land.Mar 11 2021, 7:38 PM
This revision was automatically updated to reflect the committed changes.