[PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[Brian Foster]

bcachefs grabs s_umount and sets SB_RDONLY when the fs is shutdown
via the ioctl() interface. This has a couple issues related to
interactions between shutdown and freeze:

1. The flags == FSOP_GOING_FLAGS_DEFAULT case is a deadlock vector
   because freeze_bdev() calls into freeze_super(), which also
   acquires s_umount.

2. If an explicit shutdown occurs while the sb is frozen, SB_RDONLY
   alters the thaw path as if the sb was read-only at freeze time.
   This effectively leaks the frozen state and leaves the sb frozen
   indefinitely.

The usage of SB_RDONLY here goes back to the initial bcachefs commit
and AFAICT is simply historical behavior. This behavior is unique to
bcachefs relative to the handful of other filesystems that support
the shutdown ioctl(). Typically, SB_RDONLY is reserved for the
proper remount path, which itself is restricted from modifying
frozen superblocks in reconfigure_super(). Drop the unnecessary sb
lock and flags update bch2_ioc_goingdown() to address both of these
issues.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

This is currently churning through CI here (though I had already pushed
an early version that ran clean):

https://evilpiepirate.org/~testdashboard/ci?branch=bfoster

Brian

 fs/bcachefs/fs-ioctl.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/bcachefs/fs-ioctl.c b/fs/bcachefs/fs-ioctl.c
index 5a39bcb597a3..0baa23c35ef5 100644
--- a/fs/bcachefs/fs-ioctl.c
+++ b/fs/bcachefs/fs-ioctl.c
@@ -287,34 +287,26 @@ static int bch2_ioc_goingdown(struct bch_fs *c, u32 __user *arg)
 
 	bch_notice(c, "shutdown by ioctl type %u", flags);
 
-	down_write(&c->vfs_sb->s_umount);
-
 	switch (flags) {
 	case FSOP_GOING_FLAGS_DEFAULT:
 		ret = freeze_bdev(c->vfs_sb->s_bdev);
 		if (ret)
-			goto err;
-
+			break;
 		bch2_journal_flush(&c->journal);
-		c->vfs_sb->s_flags |= SB_RDONLY;
 		bch2_fs_emergency_read_only(c);
 		thaw_bdev(c->vfs_sb->s_bdev);
 		break;
-
 	case FSOP_GOING_FLAGS_LOGFLUSH:
 		bch2_journal_flush(&c->journal);
 		fallthrough;
-
 	case FSOP_GOING_FLAGS_NOLOGFLUSH:
-		c->vfs_sb->s_flags |= SB_RDONLY;
 		bch2_fs_emergency_read_only(c);
 		break;
 	default:
 		ret = -EINVAL;
 		break;
 	}
-err:
-	up_write(&c->vfs_sb->s_umount);
+
 	return ret;
 }
 
-- 
2.41.0

Re: [PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[Kent Overstreet]

On Thu, Nov 30, 2023 at 02:17:11PM -0500, Brian Foster wrote:
> bcachefs grabs s_umount and sets SB_RDONLY when the fs is shutdown
> via the ioctl() interface. This has a couple issues related to
> interactions between shutdown and freeze:
> 
> 1. The flags == FSOP_GOING_FLAGS_DEFAULT case is a deadlock vector
>    because freeze_bdev() calls into freeze_super(), which also
>    acquires s_umount.
> 
> 2. If an explicit shutdown occurs while the sb is frozen, SB_RDONLY
>    alters the thaw path as if the sb was read-only at freeze time.
>    This effectively leaks the frozen state and leaves the sb frozen
>    indefinitely.
> 
> The usage of SB_RDONLY here goes back to the initial bcachefs commit
> and AFAICT is simply historical behavior. This behavior is unique to
> bcachefs relative to the handful of other filesystems that support
> the shutdown ioctl(). Typically, SB_RDONLY is reserved for the
> proper remount path, which itself is restricted from modifying
> frozen superblocks in reconfigure_super(). Drop the unnecessary sb
> lock and flags update bch2_ioc_goingdown() to address both of these
> issues.

Nice, I noticed a deadlock issue recently with s_umount here but didn't
have time to fully debug it - applied!

Re: [PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[Brian Foster]

On Thu, Nov 30, 2023 at 09:00:06PM -0500, Kent Overstreet wrote:
> On Thu, Nov 30, 2023 at 02:17:11PM -0500, Brian Foster wrote:
> > bcachefs grabs s_umount and sets SB_RDONLY when the fs is shutdown
> > via the ioctl() interface. This has a couple issues related to
> > interactions between shutdown and freeze:
> > 
> > 1. The flags == FSOP_GOING_FLAGS_DEFAULT case is a deadlock vector
> >    because freeze_bdev() calls into freeze_super(), which also
> >    acquires s_umount.
> > 
> > 2. If an explicit shutdown occurs while the sb is frozen, SB_RDONLY
> >    alters the thaw path as if the sb was read-only at freeze time.
> >    This effectively leaks the frozen state and leaves the sb frozen
> >    indefinitely.
> > 
> > The usage of SB_RDONLY here goes back to the initial bcachefs commit
> > and AFAICT is simply historical behavior. This behavior is unique to
> > bcachefs relative to the handful of other filesystems that support
> > the shutdown ioctl(). Typically, SB_RDONLY is reserved for the
> > proper remount path, which itself is restricted from modifying
> > frozen superblocks in reconfigure_super(). Drop the unnecessary sb
> > lock and flags update bch2_ioc_goingdown() to address both of these
> > issues.
> 
> Nice, I noticed a deadlock issue recently with s_umount here but didn't
> have time to fully debug it - applied!
> 

Thanks!

Hmm.. got a reference to that observed deadlock handy? I'd be a little
cautious associating here because 1. I dont think any test tooling
currently uses the default shutdown mode for the ioctl and 2. the
non-ioctl emergency shutdown paths dont set SB_RDONLY, so shouldn't
trigger the freeze issue.

So IOW while this path was broken in the ways described in the commit
log, it still seems rather unlikely for a normal use or test case to
trigger this. It would be good to know if there's still a lingering
issue to resolve..

Brian

Re: [PATCH] bcachefs: remove sb lock and flags update on explicit shutdown

[Kent Overstreet]

On Fri, Dec 01, 2023 at 08:43:52AM -0500, Brian Foster wrote:
> On Thu, Nov 30, 2023 at 09:00:06PM -0500, Kent Overstreet wrote:
> > On Thu, Nov 30, 2023 at 02:17:11PM -0500, Brian Foster wrote:
> > > bcachefs grabs s_umount and sets SB_RDONLY when the fs is shutdown
> > > via the ioctl() interface. This has a couple issues related to
> > > interactions between shutdown and freeze:
> > > 
> > > 1. The flags == FSOP_GOING_FLAGS_DEFAULT case is a deadlock vector
> > >    because freeze_bdev() calls into freeze_super(), which also
> > >    acquires s_umount.
> > > 
> > > 2. If an explicit shutdown occurs while the sb is frozen, SB_RDONLY
> > >    alters the thaw path as if the sb was read-only at freeze time.
> > >    This effectively leaks the frozen state and leaves the sb frozen
> > >    indefinitely.
> > > 
> > > The usage of SB_RDONLY here goes back to the initial bcachefs commit
> > > and AFAICT is simply historical behavior. This behavior is unique to
> > > bcachefs relative to the handful of other filesystems that support
> > > the shutdown ioctl(). Typically, SB_RDONLY is reserved for the
> > > proper remount path, which itself is restricted from modifying
> > > frozen superblocks in reconfigure_super(). Drop the unnecessary sb
> > > lock and flags update bch2_ioc_goingdown() to address both of these
> > > issues.
> > 
> > Nice, I noticed a deadlock issue recently with s_umount here but didn't
> > have time to fully debug it - applied!
> > 
> 
> Thanks!
> 
> Hmm.. got a reference to that observed deadlock handy? I'd be a little
> cautious associating here because 1. I dont think any test tooling
> currently uses the default shutdown mode for the ioctl and 2. the
> non-ioctl emergency shutdown paths dont set SB_RDONLY, so shouldn't
> trigger the freeze issue.

No, only observed it once and I was looking at other stuff at the time,
alas.

> So IOW while this path was broken in the ways described in the commit
> log, it still seems rather unlikely for a normal use or test case to
> trigger this. It would be good to know if there's still a lingering
> issue to resolve..

Touching a VFS lock here at all was always a bit suspect; I have no idea
where I originally cribbed that from. I generally try to keep a clearer
separation between VFS locks/state and filesystem locks/state...