[PATCH 0/5] remove BUG_ON()s in btrfs_close_one_device()

[PATCH 0/5] remove BUG_ON()s in btrfs_close_one_device()

[Johannes Thumshirn]

This series attempts to remove the BUG_ON()s in btrfs_close_one_device().
Therefore some reorganization of btrfs_close_one_device() and
close_fs_devices() was needed.

Forthermore a new BUG_ON() had to be temporarily introduced but is removed
again in the last patch of theis series.

Although it is generally legal to return -ENOMEM on umount(2), the error
handling up until close_ctree() as neither close_ctree() nor btrfs_put_super()
would be able to handle the error.

This series has passed fstests without any deviation from the baseline and
also the new error handling was tested via error injection using this snippet:

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7c55169c0613..c58802c9c39c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1069,6 +1069,9 @@ static int btrfs_close_one_device(struct btrfs_device *device)
 
 	new_device = btrfs_alloc_device(NULL, &device->devid,
 					device->uuid);
+	btrfs_free_device(new_device);
+	pr_err("%s() INJECTING -ENOMEM\n", __func__);
+	new_device = ERR_PTR(-ENOMEM);
 	if (IS_ERR(new_device))
 		return PTR_ERR(new_device);
 

Johannes Thumshirn (5):
  btrfs: decrement number of open devices after closing the device not
    before
  btrfs: handle device allocation failure in btrfs_close_one_device()
  btrfs: handle allocation failure in strdup
  btrfs: handle error return of close_fs_devices()
  btrfs: remove final BUG_ON() in close_fs_devices()

 fs/btrfs/volumes.c | 68 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 20 deletions(-)

-- 
2.16.4

[PATCH 1/5] btrfs: decrement number of open devices after closing the device not before

[Johannes Thumshirn]

In btrfs_close_one_device we're decrementing the number of open devices
before we're calling btrfs_close_bdev().

As there is no intermediate exit between these points in this function it
is technically OK to do so, but it a) is a bit harder to understand and b)
further refactoring to implement proper error handling in
btrfs_close_one_device() will change the layout of the function.

Move both operations closer together and move the decrement step after
btrfs_close_bdev().

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 22a5bd991e47..5ee26e7fca32 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1067,9 +1067,6 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	struct btrfs_device *new_device;
 	struct rcu_string *name;
 
-	if (device->bdev)
-		fs_devices->open_devices--;
-
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	    device->devid != BTRFS_DEV_REPLACE_DEVID) {
 		list_del_init(&device->dev_alloc_list);
@@ -1080,6 +1077,8 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 		fs_devices->missing_devices--;
 
 	btrfs_close_bdev(device);
+	if (device->bdev)
+		fs_devices->open_devices--;
 
 	new_device = btrfs_alloc_device(NULL, &device->devid,
 					device->uuid);
-- 
2.16.4

[PATCH 2/5] btrfs: handle device allocation failure in btrfs_close_one_device()

[Johannes Thumshirn]

In btrfs_close_one_device() we're allocating a new device and if this
fails we BUG().

Move the allocation to the top of the function and return an error in case
it failed.

The BUG_ON() is temporarily moved to close_fs_devices(), the caller of
btrfs_close_one_device() as further work is pending to untangle this.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5ee26e7fca32..0a2a73907563 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1061,12 +1061,17 @@ static void btrfs_close_bdev(struct btrfs_device *device)
 	blkdev_put(device->bdev, device->mode);
 }
 
-static void btrfs_close_one_device(struct btrfs_device *device)
+static int btrfs_close_one_device(struct btrfs_device *device)
 {
 	struct btrfs_fs_devices *fs_devices = device->fs_devices;
 	struct btrfs_device *new_device;
 	struct rcu_string *name;
 
+	new_device = btrfs_alloc_device(NULL, &device->devid,
+					device->uuid);
+	if (IS_ERR(new_device))
+		goto err_close_device;
+
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	    device->devid != BTRFS_DEV_REPLACE_DEVID) {
 		list_del_init(&device->dev_alloc_list);
@@ -1080,10 +1085,6 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	if (device->bdev)
 		fs_devices->open_devices--;
 
-	new_device = btrfs_alloc_device(NULL, &device->devid,
-					device->uuid);
-	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
-
 	/* Safe because we are under uuid_mutex */
 	if (device->name) {
 		name = rcu_string_strdup(device->name->str, GFP_NOFS);
@@ -1096,18 +1097,32 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 
 	synchronize_rcu();
 	btrfs_free_device(device);
+
+	return 0;
+
+err_close_device:
+	btrfs_close_bdev(device);
+	if (device->bdev) {
+		fs_devices->open_devices--;
+		btrfs_sysfs_rm_device_link(fs_devices, device);
+		device->bdev = NULL;
+	}
+
+	return -ENOMEM;
 }
 
 static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device, *tmp;
+	int ret;
 
 	if (--fs_devices->opened > 0)
 		return 0;
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
-		btrfs_close_one_device(device);
+		ret = btrfs_close_one_device(device);
+		BUG_ON(ret); /* -ENOMEM */
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-- 
2.16.4

[PATCH 3/5] btrfs: handle allocation failure in strdup

[Johannes Thumshirn]

Gracefully handle allocation failures in btrfs_close_one_device()'s
rcu_string_strdup() instead of crashing the machine.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0a2a73907563..e5864ca3bb3b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1064,7 +1064,7 @@ static void btrfs_close_bdev(struct btrfs_device *device)
 static int btrfs_close_one_device(struct btrfs_device *device)
 {
 	struct btrfs_fs_devices *fs_devices = device->fs_devices;
-	struct btrfs_device *new_device;
+	struct btrfs_device *new_device = NULL;
 	struct rcu_string *name;
 
 	new_device = btrfs_alloc_device(NULL, &device->devid,
@@ -1072,6 +1072,15 @@ static int btrfs_close_one_device(struct btrfs_device *device)
 	if (IS_ERR(new_device))
 		goto err_close_device;
 
+	/* Safe because we are under uuid_mutex */
+	if (device->name) {
+		name = rcu_string_strdup(device->name->str, GFP_NOFS);
+		if (!name)
+			goto err_free_device;
+
+		rcu_assign_pointer(new_device->name, name);
+	}
+
 	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
 	    device->devid != BTRFS_DEV_REPLACE_DEVID) {
 		list_del_init(&device->dev_alloc_list);
@@ -1085,13 +1094,6 @@ static int btrfs_close_one_device(struct btrfs_device *device)
 	if (device->bdev)
 		fs_devices->open_devices--;
 
-	/* Safe because we are under uuid_mutex */
-	if (device->name) {
-		name = rcu_string_strdup(device->name->str, GFP_NOFS);
-		BUG_ON(!name); /* -ENOMEM */
-		rcu_assign_pointer(new_device->name, name);
-	}
-
 	list_replace_rcu(&device->dev_list, &new_device->dev_list);
 	new_device->fs_devices = device->fs_devices;
 
@@ -1100,6 +1102,10 @@ static int btrfs_close_one_device(struct btrfs_device *device)
 
 	return 0;
 
+err_free_device:
+	if (new_device)
+		btrfs_free_device(new_device);
+
 err_close_device:
 	btrfs_close_bdev(device);
 	if (device->bdev) {
-- 
2.16.4

[PATCH 4/5] btrfs: handle error return of close_fs_devices()

[Johannes Thumshirn]

close_fs_devices() will be able to return an error instead of crashing
after the following patch.

Prepare btrfs_close_devices() for this.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e5864ca3bb3b..be1fd935edf7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1143,10 +1143,10 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_fs_devices *seed_devices = NULL;
-	int ret;
+	int err, err2 = 0;
 
 	mutex_lock(&uuid_mutex);
-	ret = close_fs_devices(fs_devices);
+	err = close_fs_devices(fs_devices);
 	if (!fs_devices->opened) {
 		seed_devices = fs_devices->seed;
 		fs_devices->seed = NULL;
@@ -1156,10 +1156,13 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 	while (seed_devices) {
 		fs_devices = seed_devices;
 		seed_devices = fs_devices->seed;
-		close_fs_devices(fs_devices);
+		err2 = close_fs_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
-	return ret;
+
+	if (err2)
+		return err2;
+	return err;
 }
 
 static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
-- 
2.16.4

[PATCH 5/5] btrfs: remove final BUG_ON() in close_fs_devices()

[Johannes Thumshirn]

Now that the preparation work is done, remove the temporary BUG_ON() in
close_fs_devices() and return an error instead.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 fs/btrfs/volumes.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index be1fd935edf7..844333b96075 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1128,7 +1128,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
 		ret = btrfs_close_one_device(device);
-		BUG_ON(ret); /* -ENOMEM */
+		if (ret) {
+			mutex_unlock(&fs_devices->device_list_mutex);
+			return ret;
+		}
+		fs_devices->opened--;
+		fs_devices->seeding--;
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-- 
2.16.4

Re: [PATCH 0/5] remove BUG_ON()s in btrfs_close_one_device()

[Qu Wenruo]

On 2019/11/12 下午8:24, Johannes Thumshirn wrote:
> This series attempts to remove the BUG_ON()s in btrfs_close_one_device().
> Therefore some reorganization of btrfs_close_one_device() and
> close_fs_devices() was needed.
>
> Forthermore a new BUG_ON() had to be temporarily introduced but is removed
> again in the last patch of theis series.
>
> Although it is generally legal to return -ENOMEM on umount(2), the error
> handling up until close_ctree() as neither close_ctree() nor btrfs_put_super()
> would be able to handle the error.
>
> This series has passed fstests without any deviation from the baseline and
> also the new error handling was tested via error injection using this snippet:
>

Good patchset, but for error injection we have more formal way to do that.

ALLOW_ERROR_INJECTION() macro along with BPF to override function return
value.

There is even a more generic script to do that:
https://github.com/adam900710/btrfs-profiler/blob/master/inject.py

Such tool allow us to only inject error when certain call sites are met,
so it can be pretty handful.

Thanks,
Qu

> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7c55169c0613..c58802c9c39c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1069,6 +1069,9 @@ static int btrfs_close_one_device(struct btrfs_device *device)
>
>  	new_device = btrfs_alloc_device(NULL, &device->devid,
>  					device->uuid);
> +	btrfs_free_device(new_device);
> +	pr_err("%s() INJECTING -ENOMEM\n", __func__);
> +	new_device = ERR_PTR(-ENOMEM);
>  	if (IS_ERR(new_device))
>  		return PTR_ERR(new_device);
>
>
> Johannes Thumshirn (5):
>   btrfs: decrement number of open devices after closing the device not
>     before
>   btrfs: handle device allocation failure in btrfs_close_one_device()
>   btrfs: handle allocation failure in strdup
>   btrfs: handle error return of close_fs_devices()
>   btrfs: remove final BUG_ON() in close_fs_devices()
>
>  fs/btrfs/volumes.c | 68 ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 20 deletions(-)
>

Re: [PATCH 5/5] btrfs: remove final BUG_ON() in close_fs_devices()

[Qu Wenruo]

On 2019/11/12 下午8:24, Johannes Thumshirn wrote:
> Now that the preparation work is done, remove the temporary BUG_ON() in
> close_fs_devices() and return an error instead.
>
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  fs/btrfs/volumes.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index be1fd935edf7..844333b96075 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1128,7 +1128,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>  	mutex_lock(&fs_devices->device_list_mutex);
>  	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
>  		ret = btrfs_close_one_device(device);
> -		BUG_ON(ret); /* -ENOMEM */
> +		if (ret) {
> +			mutex_unlock(&fs_devices->device_list_mutex);
> +			return ret;
> +		}
> +		fs_devices->opened--;
> +		fs_devices->seeding--;

This seeding-- doesn't look safe to me.

From what I see, fs_devices->seeding seems to be bool value (0 or 1).

Wouldn't this seeding-- underflow?

Thanks,
Qu

>  	}
>  	mutex_unlock(&fs_devices->device_list_mutex);
>
>

Re: [PATCH 0/5] remove BUG_ON()s in btrfs_close_one_device()

[David Sterba]

On Tue, Nov 12, 2019 at 01:24:11PM +0100, Johannes Thumshirn wrote:
> This series attempts to remove the BUG_ON()s in btrfs_close_one_device().
> Therefore some reorganization of btrfs_close_one_device() and
> close_fs_devices() was needed.
> 
> Forthermore a new BUG_ON() had to be temporarily introduced but is removed
> again in the last patch of theis series.

Yeah, that's fine.

> Although it is generally legal to return -ENOMEM on umount(2), the error
> handling up until close_ctree() as neither close_ctree() nor btrfs_put_super()
> would be able to handle the error.
> 
> This series has passed fstests without any deviation from the baseline and
> also the new error handling was tested via error injection using this snippet:

This BUG_ON is one of the syzbot reports so once this patchset is
reviewed, we can ask for a retest.

I have a WIP fix that tries to avoid ENOMEM completely as it's not
strictly necessary. The empty device is like a placeholder in the list
while the real device is RCU-removed. The fix is to mark the device as
"being deleted" so it can be skipped but this is more intrusive and more
error prone than handling the error.

As this is not a frequent error at all, syzbot artifically limits the
amount of memory, otherwise we haven't seen the ENOMEM in practice. So
the fix is IMO sufficient for now.

Re: [PATCH 5/5] btrfs: remove final BUG_ON() in close_fs_devices()

[David Sterba]

On Tue, Nov 12, 2019 at 08:41:50PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/11/12 下午8:24, Johannes Thumshirn wrote:
> > Now that the preparation work is done, remove the temporary BUG_ON() in
> > close_fs_devices() and return an error instead.
> >
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > ---
> >  fs/btrfs/volumes.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index be1fd935edf7..844333b96075 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1128,7 +1128,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
> >  	mutex_lock(&fs_devices->device_list_mutex);
> >  	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
> >  		ret = btrfs_close_one_device(device);
> > -		BUG_ON(ret); /* -ENOMEM */
> > +		if (ret) {
> > +			mutex_unlock(&fs_devices->device_list_mutex);
> > +			return ret;
> > +		}
> > +		fs_devices->opened--;
> > +		fs_devices->seeding--;
> 
> This seeding-- doesn't look safe to me.

Yeah, same here, it could be correct in the sense that it's 1 -> 0
exactly once, but otherwise its a bool, and handled in a special way.

Re: [PATCH 0/5] remove BUG_ON()s in btrfs_close_one_device()

[Johannes Thumshirn]

On 12/11/2019 13:40, Qu Wenruo wrote:
[...]
> Good patchset, but for error injection we have more formal way to do that.
> 
> ALLOW_ERROR_INJECTION() macro along with BPF to override function return
> value.
> 
> There is even a more generic script to do that:
> https://github.com/adam900710/btrfs-profiler/blob/master/inject.py
> 
> Such tool allow us to only inject error when certain call sites are met,
> so it can be pretty handful.

Yes I know, but my current test environment is not yet able to handle
this. So I resorted to just throw in some hand crafted error injection here.

If we find injecting errors in the alloc_device() code path is
worthwhile I'll add an ALLOW_ERROR_INJECTION() there.

Thanks,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 5/5] btrfs: remove final BUG_ON() in close_fs_devices()

[Johannes Thumshirn]

On 12/11/2019 13:55, David Sterba wrote:
> On Tue, Nov 12, 2019 at 08:41:50PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/11/12 下午8:24, Johannes Thumshirn wrote:
>>> Now that the preparation work is done, remove the temporary BUG_ON() in
>>> close_fs_devices() and return an error instead.
>>>
>>> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
>>> ---
>>>  fs/btrfs/volumes.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index be1fd935edf7..844333b96075 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1128,7 +1128,12 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>>>  	mutex_lock(&fs_devices->device_list_mutex);
>>>  	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
>>>  		ret = btrfs_close_one_device(device);
>>> -		BUG_ON(ret); /* -ENOMEM */
>>> +		if (ret) {
>>> +			mutex_unlock(&fs_devices->device_list_mutex);
>>> +			return ret;
>>> +		}
>>> +		fs_devices->opened--;
>>> +		fs_devices->seeding--;
>>
>> This seeding-- doesn't look safe to me.
> 
> Yeah, same here, it could be correct in the sense that it's 1 -> 0
> exactly once, but otherwise its a bool, and handled in a special way.

Yeah it looks like an overlook on my side, I'll correct it in the next
revision.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850