[PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Satya Tangirala]

freeze/thaw_bdev() currently use bdev->bd_fsfreeze_count to infer
whether or not bdev->bd_fsfreeze_sb is valid (it's valid iff
bd_fsfreeze_count is non-zero). thaw_bdev() doesn't nullify
bd_fsfreeze_sb.

But this means a freeze_bdev() call followed by a thaw_bdev() call can
leave bd_fsfreeze_sb with a non-null value, while bd_fsfreeze_count is
zero. If freeze_bdev() is called again, and this time
get_active_super() returns NULL (e.g. because the FS is unmounted),
we'll end up with bd_fsfreeze_count > 0, but bd_fsfreeze_sb is
*untouched* - it stays the same (now garbage) value. A subsequent
thaw_bdev() will decide that the bd_fsfreeze_sb value is legitimate
(since bd_fsfreeze_count > 0), and attempt to use it.

Fix this by always setting bd_fsfreeze_sb to NULL when
bd_fsfreeze_count is successfully decremented to 0 in thaw_sb().
Alternatively, we could set bd_fsfreeze_sb to whatever
get_active_super() returns in freeze_bdev() whenever bd_fsfreeze_count
is successfully incremented to 1 from 0 (which can be achieved cleanly
by moving the line currently setting bd_fsfreeze_sb to immediately
after the "sync:" label, but it might be a little too subtle/easily
overlooked in future).

This fixes the currently panicking xfstests generic/085.

Fixes: 040f04bd2e82 ("fs: simplify freeze_bdev/thaw_bdev")
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/block_dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9e56ee1f2652..12a811a9ae4b 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -606,6 +606,8 @@ int thaw_bdev(struct block_device *bdev)
 		error = thaw_super(sb);
 	if (error)
 		bdev->bd_fsfreeze_count++;
+	else
+		bdev->bd_fsfreeze_sb = NULL;
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
-- 
2.29.2.729.g45daf8777d-goog

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Darrick J. Wong]

On Thu, Dec 24, 2020 at 04:49:54AM +0000, Satya Tangirala wrote:
> freeze/thaw_bdev() currently use bdev->bd_fsfreeze_count to infer
> whether or not bdev->bd_fsfreeze_sb is valid (it's valid iff
> bd_fsfreeze_count is non-zero). thaw_bdev() doesn't nullify
> bd_fsfreeze_sb.
> 
> But this means a freeze_bdev() call followed by a thaw_bdev() call can
> leave bd_fsfreeze_sb with a non-null value, while bd_fsfreeze_count is
> zero. If freeze_bdev() is called again, and this time
> get_active_super() returns NULL (e.g. because the FS is unmounted),
> we'll end up with bd_fsfreeze_count > 0, but bd_fsfreeze_sb is
> *untouched* - it stays the same (now garbage) value. A subsequent
> thaw_bdev() will decide that the bd_fsfreeze_sb value is legitimate
> (since bd_fsfreeze_count > 0), and attempt to use it.
> 
> Fix this by always setting bd_fsfreeze_sb to NULL when
> bd_fsfreeze_count is successfully decremented to 0 in thaw_sb().
> Alternatively, we could set bd_fsfreeze_sb to whatever
> get_active_super() returns in freeze_bdev() whenever bd_fsfreeze_count
> is successfully incremented to 1 from 0 (which can be achieved cleanly
> by moving the line currently setting bd_fsfreeze_sb to immediately
> after the "sync:" label, but it might be a little too subtle/easily
> overlooked in future).
> 
> This fixes the currently panicking xfstests generic/085.
> 
> Fixes: 040f04bd2e82 ("fs: simplify freeze_bdev/thaw_bdev")
> Signed-off-by: Satya Tangirala <satyat@google.com>

I came up with the same solution to the same crash, so:

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9e56ee1f2652..12a811a9ae4b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -606,6 +606,8 @@ int thaw_bdev(struct block_device *bdev)
>  		error = thaw_super(sb);
>  	if (error)
>  		bdev->bd_fsfreeze_count++;
> +	else
> +		bdev->bd_fsfreeze_sb = NULL;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> -- 
> 2.29.2.729.g45daf8777d-goog
> 

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Christoph Hellwig]

Can someone pick this up?  Maybe through Jens' block tree as that is
where my commit this is fixing up came from.

For reference:


Reviewed-by: Christoph Hellwig <hch@lst.de>

On Thu, Dec 24, 2020 at 04:49:54AM +0000, Satya Tangirala wrote:
> freeze/thaw_bdev() currently use bdev->bd_fsfreeze_count to infer
> whether or not bdev->bd_fsfreeze_sb is valid (it's valid iff
> bd_fsfreeze_count is non-zero). thaw_bdev() doesn't nullify
> bd_fsfreeze_sb.
> 
> But this means a freeze_bdev() call followed by a thaw_bdev() call can
> leave bd_fsfreeze_sb with a non-null value, while bd_fsfreeze_count is
> zero. If freeze_bdev() is called again, and this time
> get_active_super() returns NULL (e.g. because the FS is unmounted),
> we'll end up with bd_fsfreeze_count > 0, but bd_fsfreeze_sb is
> *untouched* - it stays the same (now garbage) value. A subsequent
> thaw_bdev() will decide that the bd_fsfreeze_sb value is legitimate
> (since bd_fsfreeze_count > 0), and attempt to use it.
> 
> Fix this by always setting bd_fsfreeze_sb to NULL when
> bd_fsfreeze_count is successfully decremented to 0 in thaw_sb().
> Alternatively, we could set bd_fsfreeze_sb to whatever
> get_active_super() returns in freeze_bdev() whenever bd_fsfreeze_count
> is successfully incremented to 1 from 0 (which can be achieved cleanly
> by moving the line currently setting bd_fsfreeze_sb to immediately
> after the "sync:" label, but it might be a little too subtle/easily
> overlooked in future).
> 
> This fixes the currently panicking xfstests generic/085.
> 
> Fixes: 040f04bd2e82 ("fs: simplify freeze_bdev/thaw_bdev")
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9e56ee1f2652..12a811a9ae4b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -606,6 +606,8 @@ int thaw_bdev(struct block_device *bdev)
>  		error = thaw_super(sb);
>  	if (error)
>  		bdev->bd_fsfreeze_count++;
> +	else
> +		bdev->bd_fsfreeze_sb = NULL;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> -- 
> 2.29.2.729.g45daf8777d-goog
---end quoted text---

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Bob Peterson]

----- Original Message -----
> Can someone pick this up?  Maybe through Jens' block tree as that is
> where my commit this is fixing up came from.
> 
> For reference:
> 
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> On Thu, Dec 24, 2020 at 04:49:54AM +0000, Satya Tangirala wrote:
> > freeze/thaw_bdev() currently use bdev->bd_fsfreeze_count to infer
> > whether or not bdev->bd_fsfreeze_sb is valid (it's valid iff
> > bd_fsfreeze_count is non-zero). thaw_bdev() doesn't nullify
> > bd_fsfreeze_sb.
> > 
> > But this means a freeze_bdev() call followed by a thaw_bdev() call can
> > leave bd_fsfreeze_sb with a non-null value, while bd_fsfreeze_count is
> > zero. If freeze_bdev() is called again, and this time
> > get_active_super() returns NULL (e.g. because the FS is unmounted),
> > we'll end up with bd_fsfreeze_count > 0, but bd_fsfreeze_sb is
> > *untouched* - it stays the same (now garbage) value. A subsequent
> > thaw_bdev() will decide that the bd_fsfreeze_sb value is legitimate
> > (since bd_fsfreeze_count > 0), and attempt to use it.
> > 
> > Fix this by always setting bd_fsfreeze_sb to NULL when
> > bd_fsfreeze_count is successfully decremented to 0 in thaw_sb().
> > Alternatively, we could set bd_fsfreeze_sb to whatever
> > get_active_super() returns in freeze_bdev() whenever bd_fsfreeze_count
> > is successfully incremented to 1 from 0 (which can be achieved cleanly
> > by moving the line currently setting bd_fsfreeze_sb to immediately
> > after the "sync:" label, but it might be a little too subtle/easily
> > overlooked in future).
> > 
> > This fixes the currently panicking xfstests generic/085.
> > 
> > Fixes: 040f04bd2e82 ("fs: simplify freeze_bdev/thaw_bdev")
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  fs/block_dev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 9e56ee1f2652..12a811a9ae4b 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -606,6 +606,8 @@ int thaw_bdev(struct block_device *bdev)
> >  		error = thaw_super(sb);
> >  	if (error)
> >  		bdev->bd_fsfreeze_count++;
> > +	else
> > +		bdev->bd_fsfreeze_sb = NULL;
> >  out:
> >  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
> >  	return error;
> > --
> > 2.29.2.729.g45daf8777d-goog
> ---end quoted text---
> 
> 
Funny you should ask. I came across this bug in my testing of gfs2
and my patch is slightly different. I was wondering who to send it to.
Perhaps Viro?

Regards,

Bob Peterson

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Jens Axboe]

On 1/7/21 9:20 AM, Christoph Hellwig wrote:
> Can someone pick this up?  Maybe through Jens' block tree as that is
> where my commit this is fixing up came from.
> 
> For reference:
> 
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Bob Peterson]

----- Original Message -----
> Can someone pick this up?  Maybe through Jens' block tree as that is
> where my commit this is fixing up came from.
Christoph and Al,

Here is my version:

Bob Peterson

fs: fix freeze count problem in freeze_bdev

Before this patch, if you tried to freeze a device (function freeze_bdev)
while it was being unmounted, it would get NULL back from get_active_super
and correctly bypass the freeze calls. Unfortunately, it forgot to decrement
its bd_fsfreeze_count. Subsequent calls to device thaw (thaw_bdev) would
see the non-zero bd_fsfreeze_count and assume the bd_fsfreeze_sb value was
still valid. That's not a safe assumption and resulted in use-after-free,
which often caused fatal kernel errors like: "unable to handle page fault
for address."

This patch adds the necessary decrement of bd_fsfreeze_count for that
error path. It also adds code to set the bd_fsfreeze_sb to NULL when the
last reference is reached in thaw_bdev.

Reviewed-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/block_dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9e56ee1f2652..c6daf7d12546 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -555,8 +555,10 @@ int freeze_bdev(struct block_device *bdev)
 		goto done;
 
 	sb = get_active_super(bdev);
-	if (!sb)
+	if (!sb) {
+		bdev->bd_fsfreeze_count--;
 		goto sync;
+	}
 	if (sb->s_op->freeze_super)
 		error = sb->s_op->freeze_super(sb);
 	else
@@ -600,6 +602,7 @@ int thaw_bdev(struct block_device *bdev)
 	if (!sb)
 		goto out;
 
+	bdev->bd_fsfreeze_sb = NULL;
 	if (sb->s_op->thaw_super)
 		error = sb->s_op->thaw_super(sb);
 	else

[fs PATCH] fs: fix freeze count problem in freeze_bdev

[Bob Peterson]

Hi,

I wrote this patch to fix the freeze/thaw device problem before I saw
the patch "fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb"
from Satya Tangirala. That one, however, does not fix the bd_freeze_count
problem and this patch does. Jens, Christoph, what do you think?
This is a very recreatable problem via repeated runs of generic/085,
at least on gfs2.

Description:

Before this patch, if you tried to freeze a device (function freeze_bdev)
while it was being unmounted, it would get NULL back from get_active_super
and correctly bypass the freeze calls. Unfortunately, it forgot to decrement
its bd_fsfreeze_count. Subsequent calls to device thaw (thaw_bdev) would
see the non-zero bd_fsfreeze_count and assume the bd_fsfreeze_sb value was
still valid. That's not a safe assumption and resulted in use-after-free,
which often caused fatal kernel errors like: "unable to handle page fault
for address."

This patch adds the necessary decrement of bd_fsfreeze_count for that
error path. It also adds code to set the bd_fsfreeze_sb to NULL when the
last reference is reached in thaw_bdev.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/block_dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9e56ee1f2652..c6daf7d12546 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -555,8 +555,10 @@ int freeze_bdev(struct block_device *bdev)
 		goto done;
 
 	sb = get_active_super(bdev);
-	if (!sb)
+	if (!sb) {
+		bdev->bd_fsfreeze_count--;
 		goto sync;
+	}
 	if (sb->s_op->freeze_super)
 		error = sb->s_op->freeze_super(sb);
 	else
@@ -600,6 +602,7 @@ int thaw_bdev(struct block_device *bdev)
 	if (!sb)
 		goto out;
 
+	bdev->bd_fsfreeze_sb = NULL;
 	if (sb->s_op->thaw_super)
 		error = sb->s_op->thaw_super(sb);
 	else

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Satya Tangirala]

On Thu, Jan 07, 2021 at 11:27:37AM -0500, Bob Peterson wrote:
> ----- Original Message -----
> > Can someone pick this up?  Maybe through Jens' block tree as that is
> > where my commit this is fixing up came from.
> Christoph and Al,
> 
> Here is my version:
> 
> Bob Peterson
> 
> fs: fix freeze count problem in freeze_bdev
> 
> Before this patch, if you tried to freeze a device (function freeze_bdev)
> while it was being unmounted, it would get NULL back from get_active_super
> and correctly bypass the freeze calls. Unfortunately, it forgot to decrement
> its bd_fsfreeze_count. Subsequent calls to device thaw (thaw_bdev) would
> see the non-zero bd_fsfreeze_count and assume the bd_fsfreeze_sb value was
> still valid. That's not a safe assumption and resulted in use-after-free,
> which often caused fatal kernel errors like: "unable to handle page fault
> for address."
> 
> This patch adds the necessary decrement of bd_fsfreeze_count for that
> error path. It also adds code to set the bd_fsfreeze_sb to NULL when the
> last reference is reached in thaw_bdev.
> 
> Reviewed-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/block_dev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9e56ee1f2652..c6daf7d12546 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -555,8 +555,10 @@ int freeze_bdev(struct block_device *bdev)
>  		goto done;
>  
>  	sb = get_active_super(bdev);
> -	if (!sb)
> +	if (!sb) {
> +		bdev->bd_fsfreeze_count--;
>  		goto sync;
> +	}
>  	if (sb->s_op->freeze_super)
>  		error = sb->s_op->freeze_super(sb);
>  	else
> @@ -600,6 +602,7 @@ int thaw_bdev(struct block_device *bdev)
>  	if (!sb)
>  		goto out;
>  
> +	bdev->bd_fsfreeze_sb = NULL;
This causes bdev->bd_fsfreeze_sb to be set to NULL even if the call to
thaw_super right after this line fail. So if a caller tries to call
thaw_bdev() again after receiving such an error, that next call won't even
try to call thaw_super(). Is that what we want here?  (I don't know much
about this code, but from a cursory glance I think this difference is
visible to emergency_thaw_bdev() in fs/buffer.c)

In my version of the patch, I set bdev->bd_fsfreeze_sb to NULL only
*after* we check that the call to thaw_super() succeeded to avoid this.
>  	if (sb->s_op->thaw_super)
>  		error = sb->s_op->thaw_super(sb);
>  	else
>
In another mail, you mentioned
> I wrote this patch to fix the freeze/thaw device problem before I saw
> the patch "fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb"
> from Satya Tangirala. That one, however, does not fix the bd_freeze_count
> problem and this patch does.
Thanks a lot for investigating the bug and the patch I sent :)
Was there actually an issue with that patch I sent? As you said, the bug
is very reproduceable on master with generic/085. But with my patch, I
don't see any issues even after having run the test many, many times
(admittedly, I tested it on f2fs and ext4 rather than gfs2, but I don't
think that should cause much differences). Did you have a test case that
actually causes a failure with my patch?

The only two differences between the patch I sent and this patch are

1) The point at which the bd_fsfreeze_bd is set to NULL in thaw_bdev(), as
I mentioned above already.

2) Whether or not to decrement bd_fsfreeze_count when we get a NULL from
get_active_super() in freeze_bdev() - I don't do this in my patch.

I think setting bd_fsfreeze_sb to NULL in thaw_bdev (in either the place
your patch does it or my patch does it) is enough to fix the bug w.r.t the
use-after-free. Fwiw, I do think it should be set to NULL after we check for
all the errors though.

I think the second difference (decrementing bd_fsfreeze_count when
get_active_super() returns NULL) doesn't change anything w.r.t the
use-after-free. It does however, change the behaviour of the function
slightly, and it might be caller visible (because from a cursory glance, it
looks like we're reading the bd_fsfreeze_count from some other places like
fs/super.c). Even before 040f04bd2e82, the code wouldn't decrement
bd_fsfreeze_count when get_active_super() returned NULL - so is this change
in behaviour intentional? And if so, maybe it should go in a separate
patch?

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Christoph Hellwig]

On Thu, Jan 07, 2021 at 11:08:39PM +0000, Satya Tangirala wrote:
> >  		error = sb->s_op->freeze_super(sb);
> >  	else
> > @@ -600,6 +602,7 @@ int thaw_bdev(struct block_device *bdev)
> >  	if (!sb)
> >  		goto out;
> >  
> > +	bdev->bd_fsfreeze_sb = NULL;
> This causes bdev->bd_fsfreeze_sb to be set to NULL even if the call to
> thaw_super right after this line fail. So if a caller tries to call
> thaw_bdev() again after receiving such an error, that next call won't even
> try to call thaw_super(). Is that what we want here?  (I don't know much
> about this code, but from a cursory glance I think this difference is
> visible to emergency_thaw_bdev() in fs/buffer.c)

Yes, that definitively is an issue.

> 
> I think the second difference (decrementing bd_fsfreeze_count when
> get_active_super() returns NULL) doesn't change anything w.r.t the
> use-after-free. It does however, change the behaviour of the function
> slightly, and it might be caller visible (because from a cursory glance, it
> looks like we're reading the bd_fsfreeze_count from some other places like
> fs/super.c). Even before 040f04bd2e82, the code wouldn't decrement
> bd_fsfreeze_count when get_active_super() returned NULL - so is this change
> in behaviour intentional? And if so, maybe it should go in a separate
> patch?

Yes, that would be a change in behavior.  And I'm not sure why we would
want to change it.  But if so we should do it in a separate patch that
documents the why, on top of the patch that already is in the block tree.

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Bob Peterson]

----- Original Message -----
> This causes bdev->bd_fsfreeze_sb to be set to NULL even if the call to
> thaw_super right after this line fail. So if a caller tries to call
> thaw_bdev() again after receiving such an error, that next call won't even
> try to call thaw_super(). Is that what we want here?  (I don't know much
> about this code, but from a cursory glance I think this difference is
> visible to emergency_thaw_bdev() in fs/buffer.c)
> 
> In my version of the patch, I set bdev->bd_fsfreeze_sb to NULL only
> *after* we check that the call to thaw_super() succeeded to avoid this.

Yes, I see your point. Your patch is superior and I'll mine accordingly.

> Thanks a lot for investigating the bug and the patch I sent :)
> Was there actually an issue with that patch I sent? As you said, the bug

No, I never saw your patch until I saw Christoph's reference to it yesterday,
after I had been using my patch to fix the problem. AFAIK, there is no
problem with your patch.

> I think the second difference (decrementing bd_fsfreeze_count when
> get_active_super() returns NULL) doesn't change anything w.r.t the
> use-after-free. It does however, change the behaviour of the function
> slightly, and it might be caller visible (because from a cursory glance, it
> looks like we're reading the bd_fsfreeze_count from some other places like
> fs/super.c). Even before 040f04bd2e82, the code wouldn't decrement
> bd_fsfreeze_count when get_active_super() returned NULL - so is this change
> in behaviour intentional? And if so, maybe it should go in a separate
> patch?

This is the bigger issue, and I'm not very familiar with this code either,
so I'll defer to the experts. Yes, it's a change in behavior, but I think
it makes sense to decrement the bd_fsfreeze_count in this case. Here's why:

If the blockdev is frozen by freeze_bdev while it's being unmounted, the
bd_fsfreeze_count is incremented, but the freeze is ignored. Subsequent
attempts to thaw the device will be ignored but return 0 because the sb
is not found. When the device is mounted again, calls to freeze_bdev
will bypass the call to freeze_super for the newly mounted sb, because
bdev->bd_fsfreeze_count was then incremented from 1 to 2 in freeze_bdev.

	if (++bdev->bd_fsfreeze_count > 1)
		goto done;

So you're freezing the device without really freezing the superblock.
Seems like dangerous behavior to me. The new sb will only be frozen if
a second thaw is done, which gets them back in sync. I suppose we could
say this is acceptable loss, and your number of thaws should match your
freezes, and if they don't: user error. Still, it seems like we should do
something about it, like refuse to mount a frozen device. Perhaps it already
does that; I'll need to do some research.

Like I said, I don't know this code. I'm just trying to fix a problem
I observed. I'll defer to the experts.

Regards,

Bob Peterson

Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze_sb

[Bob Peterson]

Hi,

> This is the bigger issue, and I'm not very familiar with this code either,
> so I'll defer to the experts. Yes, it's a change in behavior, but I think
> it makes sense to decrement the bd_fsfreeze_count in this case. Here's why:
> 
> If the blockdev is frozen by freeze_bdev while it's being unmounted, the
> bd_fsfreeze_count is incremented, but the freeze is ignored. Subsequent
> attempts to thaw the device will be ignored but return 0 because the sb
> is not found. When the device is mounted again, calls to freeze_bdev
> will bypass the call to freeze_super for the newly mounted sb, because
> bdev->bd_fsfreeze_count was then incremented from 1 to 2 in freeze_bdev.
> 
> 	if (++bdev->bd_fsfreeze_count > 1)
> 		goto done;
> 
> So you're freezing the device without really freezing the superblock.
> Seems like dangerous behavior to me. The new sb will only be frozen if
> a second thaw is done, which gets them back in sync. I suppose we could
> say this is acceptable loss, and your number of thaws should match your
> freezes, and if they don't: user error. Still, it seems like we should do
> something about it, like refuse to mount a frozen device. Perhaps it already
> does that; I'll need to do some research.

After some experiments, I've determined that my fears about the count are unfounded.
Consider my patch withdrawn. Sorry for the noise.

Bob Peterson