From: Nikolay Borisov <nborisov@suse.com>
To: Wentao_Liang <Wentao_Liang_g@163.com>, clm@fb.com
Cc: josef@toxicpanda.com, dsterba@suse.com,
linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] btrfs: fix a potential double put bug and some related use-after-free bugs
Date: Wed, 18 Aug 2021 15:11:45 +0300 [thread overview]
Message-ID: <1dea8c28-8ca6-83d4-8eb0-84fe1ebb7cc9@suse.com> (raw)
In-Reply-To: <20210818091518.4825-1-Wentao_Liang_g@163.com>
On 18.08.21 г. 12:15, Wentao_Liang wrote:
> In line 2955 (#1), "btrfs_put_block_group(cache);" drops the reference to
> cache and may cause the cache to be released. However, in line 3014, the
> cache is dropped again by the same put function (#4). Double putting the
> cache can lead to an incorrect reference count.
>
> Furthermore, according to the definition of btrfs_put_block_group() in fs/
> btrfs/block-group.c, if the reference count of the cache is one at the
> first put, it will be freed by kfree(). Using it again may result in the
> use-after-free flaw. In fact, after the first put (line 2955), the cache
> is also accessed in a few places (#2, #3), e.g., lines 2967, 2973, 2974,
> ….
>
> We believe that the first put of the cache is unnecessary (#1).
> We can fix the above bugs by removing the redundant
> "btrfs_put_block_group(cache);" in line 2955 (#1).
>
> 2951 if (!list_empty(&cache->io_list)) {
> ...
> 2955 btrfs_put_block_group(cache);
> //#1 the first drop to cache (unnecessary)
> ...
> 2957 }
> ...
> 2967 cache_save_setup(cache, trans, path); //#2 use the cache
> ...
> 2972 //#3 use the cache several times
>
> 2973 if (!ret && cache->disk_cache_state == BTRFS_DC_SETUP) {
> 2974 cache->io_ctl.inode = NULL;
> 2975 ret = btrfs_write_out_cache(trans, cache, path);
> 2976 if (ret == 0 && cache->io_ctl.inode) {
> 2977 num_started++;
> 2978 should_put = 0;
> 2979 list_add_tail(&cache->io_list, io);
> 2980 } else {
> ...
> 2985 ret = 0;
> 2986 }
> 2987 }
> 2988 if (!ret) {
> 2989 ret = update_block_group_item(trans, path, cache);
> ...
> 3003 if (ret == -ENOENT) {
> ...
> 3006 ret = update_block_group_item(trans, path, cache);
> 3007 }
> ...
> 3010 }
> 3011
> ...
> 3013 if (should_put)
> 3014 btrfs_put_block_group(cache);
> //#4 the second drop to cache
>
> Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
Apart form the patch being buggy you have not demonstrated why doing 2
put block groups is actually given that there are invariant that
guarantee bg will have at least 2 refs held. So it seems you have
produced the patch without considering the big picture of how btrfs'
block group state machine works.
prev parent reply other threads:[~2021-08-18 12:12 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-18 9:15 [PATCH] btrfs: fix a potential double put bug and some related use-after-free bugs Wentao_Liang
2021-08-18 10:46 ` David Sterba
2021-08-18 12:11 ` Nikolay Borisov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1dea8c28-8ca6-83d4-8eb0-84fe1ebb7cc9@suse.com \
--to=nborisov@suse.com \
--cc=Wentao_Liang_g@163.com \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.