[PATCH][v2] btrfs: use refcount_inc_not_zero in kill_all_nodes

[PATCH][v2] btrfs: use refcount_inc_not_zero in kill_all_nodes

[Josef Bacik]

We hit the following warning while running down a different problem

[ 6197.175850] ------------[ cut here ]------------
[ 6197.185082] refcount_t: underflow; use-after-free.
[ 6197.194704] WARNING: CPU: 47 PID: 966 at lib/refcount.c:190 refcount_sub_and_test_checked+0x53/0x60
[ 6197.521792] Call Trace:
[ 6197.526687]  __btrfs_release_delayed_node+0x76/0x1c0
[ 6197.536615]  btrfs_kill_all_delayed_nodes+0xec/0x130
[ 6197.546532]  ? __btrfs_btree_balance_dirty+0x60/0x60
[ 6197.556482]  btrfs_clean_one_deleted_snapshot+0x71/0xd0
[ 6197.566910]  cleaner_kthread+0xfa/0x120
[ 6197.574573]  kthread+0x111/0x130
[ 6197.581022]  ? kthread_create_on_node+0x60/0x60
[ 6197.590086]  ret_from_fork+0x1f/0x30
[ 6197.597228] ---[ end trace 424bb7ae00509f56 ]---

This is because the free side drops the ref without the lock, and then
takes the lock if our refcount is 0.  So you can have nodes on the tree
that have a refcount of 0.  Fix this by zero'ing out that element in our
temporary array so we don't try to kill it again.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v1->v2:
- I'm an idiot.

 fs/btrfs/delayed-inode.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 1f7f39b10bd0..81b2fd46886f 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1948,13 +1948,16 @@ void btrfs_kill_all_delayed_nodes(struct btrfs_root *root)
 			break;
 		}
 
-		inode_id = delayed_nodes[n - 1]->inode_id + 1;
-
-		for (i = 0; i < n; i++)
-			refcount_inc(&delayed_nodes[i]->refs);
+		for (i = 0; i < n; i++) {
+			inode_id = delayed_nodes[i]->inode_id + 1;
+			if (!refcount_inc_not_zero(&delayed_nodes[i]->refs))
+				delayed_nodes[i] = NULL;
+		}
 		spin_unlock(&root->inode_lock);
 
 		for (i = 0; i < n; i++) {
+			if (!delayed_nodes[i])
+				continue;
 			__btrfs_kill_delayed_node(delayed_nodes[i]);
 			btrfs_release_delayed_node(delayed_nodes[i]);
 		}
-- 
2.21.0

Re: [PATCH][v2] btrfs: use refcount_inc_not_zero in kill_all_nodes

[Nikolay Borisov]

On 26.09.19 г. 15:08 ч., Josef Bacik wrote:
> We hit the following warning while running down a different problem
> 
> [ 6197.175850] ------------[ cut here ]------------
> [ 6197.185082] refcount_t: underflow; use-after-free.
> [ 6197.194704] WARNING: CPU: 47 PID: 966 at lib/refcount.c:190 refcount_sub_and_test_checked+0x53/0x60
> [ 6197.521792] Call Trace:
> [ 6197.526687]  __btrfs_release_delayed_node+0x76/0x1c0
> [ 6197.536615]  btrfs_kill_all_delayed_nodes+0xec/0x130
> [ 6197.546532]  ? __btrfs_btree_balance_dirty+0x60/0x60
> [ 6197.556482]  btrfs_clean_one_deleted_snapshot+0x71/0xd0
> [ 6197.566910]  cleaner_kthread+0xfa/0x120
> [ 6197.574573]  kthread+0x111/0x130
> [ 6197.581022]  ? kthread_create_on_node+0x60/0x60
> [ 6197.590086]  ret_from_fork+0x1f/0x30
> [ 6197.597228] ---[ end trace 424bb7ae00509f56 ]---
> 
> This is because the free side drops the ref without the lock, and then
> takes the lock if our refcount is 0.  So you can have nodes on the tree
> that have a refcount of 0.  Fix this by zero'ing out that element in our
> temporary array so we don't try to kill it again.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Looks good, one minor nit below though and you can add:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> v1->v2:
> - I'm an idiot.
> 
>  fs/btrfs/delayed-inode.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 1f7f39b10bd0..81b2fd46886f 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1948,13 +1948,16 @@ void btrfs_kill_all_delayed_nodes(struct btrfs_root *root)
>  			break;
>  		}
>  
> -		inode_id = delayed_nodes[n - 1]->inode_id + 1;
> -
> -		for (i = 0; i < n; i++)
> -			refcount_inc(&delayed_nodes[i]->refs);
> +		for (i = 0; i < n; i++) {
> +			inode_id = delayed_nodes[i]->inode_id + 1;

Since you no longer are doing a break in the loop there is no point in
assigning inode_id  in the loop. You can  retain the old code that does:
inode_id = delayed_nodes[n - 1]->inode_id + 1;

Since with the current structure it's always guaranteed that inode_id
will be 1 higher than the inode_Id of the last inode in delayed_nodes .

> +			if (!refcount_inc_not_zero(&delayed_nodes[i]->refs))
> +				delayed_nodes[i] = NULL;
> +		}
>  		spin_unlock(&root->inode_lock);
>  
>  		for (i = 0; i < n; i++) {
> +			if (!delayed_nodes[i])
> +				continue;
>  			__btrfs_kill_delayed_node(delayed_nodes[i]);
>  			btrfs_release_delayed_node(delayed_nodes[i]);
>  		}
>