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@xxxxxxxxxxxxxx>
Looks good, one minor nit below though and you can add:
Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> 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]);
> }
>
