* [PATCH 2/2] btrfs: fix block group UAF bug with nocow
2020-07-01 20:22 [PATCH 1/2] btrfs: convert block group refcount to refcount_t Josef Bacik
@ 2020-07-01 20:22 ` Josef Bacik
2020-07-02 11:24 ` Filipe Manana
2020-07-02 1:25 ` [PATCH 1/2] btrfs: convert block group refcount to refcount_t kernel test robot
` (3 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Josef Bacik @ 2020-07-01 20:22 UTC (permalink / raw)
To: linux-btrfs, kernel-team
While debugging a patch that I wrote I was hitting UAF panics when
accessing block groups on unmount. This turned out to be because in the
nocow case if we bail out of doing the nocow for whatever reason we need
to call btrfs_dec_nocow_writers() if we called the inc. This puts our
block group, but a few error cases does
if (nocow) {
btrfs_dec_nocow_writers();
goto error;
}
unfortunately, error is
error:
if (nocow)
btrfs_dec_nocow_writers();
so we get a double put on our block group. Fix this by dropping the
error cases calling of btrfs_dec_nocow_writers(), as it's handled at the
error label now.
Fixes: 762bf09893b4 ("btrfs: improve error handling in run_delalloc_nocow")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
fs/btrfs/inode.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d301550b9c70..cb18b1a13dca 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1694,12 +1694,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
ret = fallback_to_cow(inode, locked_page, cow_start,
found_key.offset - 1,
page_started, nr_written);
- if (ret) {
- if (nocow)
- btrfs_dec_nocow_writers(fs_info,
- disk_bytenr);
+ if (ret)
goto error;
- }
cow_start = (u64)-1;
}
@@ -1715,9 +1711,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
ram_bytes, BTRFS_COMPRESS_NONE,
BTRFS_ORDERED_PREALLOC);
if (IS_ERR(em)) {
- if (nocow)
- btrfs_dec_nocow_writers(fs_info,
- disk_bytenr);
ret = PTR_ERR(em);
goto error;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] btrfs: fix block group UAF bug with nocow
2020-07-01 20:22 ` [PATCH 2/2] btrfs: fix block group UAF bug with nocow Josef Bacik
@ 2020-07-02 11:24 ` Filipe Manana
0 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2020-07-02 11:24 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
On Wed, Jul 1, 2020 at 9:23 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> While debugging a patch that I wrote I was hitting UAF panics when
> accessing block groups on unmount. This turned out to be because in the
> nocow case if we bail out of doing the nocow for whatever reason we need
> to call btrfs_dec_nocow_writers() if we called the inc. This puts our
> block group, but a few error cases does
>
> if (nocow) {
> btrfs_dec_nocow_writers();
> goto error;
> }
>
> unfortunately, error is
>
> error:
> if (nocow)
> btrfs_dec_nocow_writers();
>
> so we get a double put on our block group. Fix this by dropping the
> error cases calling of btrfs_dec_nocow_writers(), as it's handled at the
> error label now.
>
> Fixes: 762bf09893b4 ("btrfs: improve error handling in run_delalloc_nocow")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Looks good and the patch compiles and works just fine here on the
latest misc-next.
Thanks.
> ---
> fs/btrfs/inode.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d301550b9c70..cb18b1a13dca 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1694,12 +1694,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
> ret = fallback_to_cow(inode, locked_page, cow_start,
> found_key.offset - 1,
> page_started, nr_written);
> - if (ret) {
> - if (nocow)
> - btrfs_dec_nocow_writers(fs_info,
> - disk_bytenr);
> + if (ret)
> goto error;
> - }
> cow_start = (u64)-1;
> }
>
> @@ -1715,9 +1711,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
> ram_bytes, BTRFS_COMPRESS_NONE,
> BTRFS_ORDERED_PREALLOC);
> if (IS_ERR(em)) {
> - if (nocow)
> - btrfs_dec_nocow_writers(fs_info,
> - disk_bytenr);
> ret = PTR_ERR(em);
> goto error;
> }
> --
> 2.24.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs: convert block group refcount to refcount_t
2020-07-01 20:22 [PATCH 1/2] btrfs: convert block group refcount to refcount_t Josef Bacik
2020-07-01 20:22 ` [PATCH 2/2] btrfs: fix block group UAF bug with nocow Josef Bacik
@ 2020-07-02 1:25 ` kernel test robot
2020-07-02 5:22 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-07-02 1:25 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team; +Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7798 bytes --]
Hi Josef,
I love your patch! Yet something to improve:
[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.8-rc3 next-20200701]
[cannot apply to btrfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Josef-Bacik/btrfs-convert-block-group-refcount-to-refcount_t/20200702-042305
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from fs/btrfs/free-space-cache.c:14:
fs/btrfs/ctree.h:2216:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
2216 | size_t __const btrfs_get_num_csums(void);
| ^~~~~~~
fs/btrfs/free-space-cache.c: In function 'btrfs_return_cluster_to_free_space':
>> fs/btrfs/free-space-cache.c:2930:13: error: passing argument 1 of 'atomic_inc' from incompatible pointer type [-Werror=incompatible-pointer-types]
2930 | atomic_inc(&block_group->count);
| ^~~~~~~~~~~~~~~~~~~
| |
| refcount_t * {aka struct refcount_struct *}
In file included from include/linux/atomic.h:74,
from include/asm-generic/bitops/lock.h:5,
from include/asm-generic/bitops.h:32,
from ./arch/nds32/include/generated/asm/bitops.h:1,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from ./arch/nds32/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from include/linux/pagemap.h:8,
from fs/btrfs/free-space-cache.c:6:
include/linux/atomic-fallback.h:266:22: note: expected 'atomic_t *' {aka 'struct <anonymous> *'} but argument is of type 'refcount_t *' {aka 'struct refcount_struct *'}
266 | atomic_inc(atomic_t *v)
| ~~~~~~~~~~^
fs/btrfs/free-space-cache.c: In function 'btrfs_find_space_cluster':
fs/btrfs/free-space-cache.c:3361:14: error: passing argument 1 of 'atomic_inc' from incompatible pointer type [-Werror=incompatible-pointer-types]
3361 | atomic_inc(&block_group->count);
| ^~~~~~~~~~~~~~~~~~~
| |
| refcount_t * {aka struct refcount_struct *}
In file included from include/linux/atomic.h:74,
from include/asm-generic/bitops/lock.h:5,
from include/asm-generic/bitops.h:32,
from ./arch/nds32/include/generated/asm/bitops.h:1,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from ./arch/nds32/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from include/linux/pagemap.h:8,
from fs/btrfs/free-space-cache.c:6:
include/linux/atomic-fallback.h:266:22: note: expected 'atomic_t *' {aka 'struct <anonymous> *'} but argument is of type 'refcount_t *' {aka 'struct refcount_struct *'}
266 | atomic_inc(atomic_t *v)
| ~~~~~~~~~~^
cc1: some warnings being treated as errors
vim +/atomic_inc +2930 fs/btrfs/free-space-cache.c
fa9c0d795f7b57c Chris Mason 2009-04-03 2901
fa9c0d795f7b57c Chris Mason 2009-04-03 2902 /*
fa9c0d795f7b57c Chris Mason 2009-04-03 2903 * given a cluster, put all of its extents back into the free space
fa9c0d795f7b57c Chris Mason 2009-04-03 2904 * cache. If a block group is passed, this function will only free
fa9c0d795f7b57c Chris Mason 2009-04-03 2905 * a cluster that belongs to the passed block group.
fa9c0d795f7b57c Chris Mason 2009-04-03 2906 *
fa9c0d795f7b57c Chris Mason 2009-04-03 2907 * Otherwise, it'll get a reference on the block group pointed to by the
fa9c0d795f7b57c Chris Mason 2009-04-03 2908 * cluster and remove the cluster from it.
fa9c0d795f7b57c Chris Mason 2009-04-03 2909 */
fa9c0d795f7b57c Chris Mason 2009-04-03 2910 int btrfs_return_cluster_to_free_space(
32da5386d9a4fd5 David Sterba 2019-10-29 2911 struct btrfs_block_group *block_group,
fa9c0d795f7b57c Chris Mason 2009-04-03 2912 struct btrfs_free_cluster *cluster)
fa9c0d795f7b57c Chris Mason 2009-04-03 2913 {
34d52cb6c50b5a4 Li Zefan 2011-03-29 2914 struct btrfs_free_space_ctl *ctl;
fa9c0d795f7b57c Chris Mason 2009-04-03 2915 int ret;
fa9c0d795f7b57c Chris Mason 2009-04-03 2916
fa9c0d795f7b57c Chris Mason 2009-04-03 2917 /* first, get a safe pointer to the block group */
fa9c0d795f7b57c Chris Mason 2009-04-03 2918 spin_lock(&cluster->lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2919 if (!block_group) {
fa9c0d795f7b57c Chris Mason 2009-04-03 2920 block_group = cluster->block_group;
fa9c0d795f7b57c Chris Mason 2009-04-03 2921 if (!block_group) {
fa9c0d795f7b57c Chris Mason 2009-04-03 2922 spin_unlock(&cluster->lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2923 return 0;
fa9c0d795f7b57c Chris Mason 2009-04-03 2924 }
fa9c0d795f7b57c Chris Mason 2009-04-03 2925 } else if (cluster->block_group != block_group) {
fa9c0d795f7b57c Chris Mason 2009-04-03 2926 /* someone else has already freed it don't redo their work */
fa9c0d795f7b57c Chris Mason 2009-04-03 2927 spin_unlock(&cluster->lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2928 return 0;
fa9c0d795f7b57c Chris Mason 2009-04-03 2929 }
fa9c0d795f7b57c Chris Mason 2009-04-03 @2930 atomic_inc(&block_group->count);
fa9c0d795f7b57c Chris Mason 2009-04-03 2931 spin_unlock(&cluster->lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2932
34d52cb6c50b5a4 Li Zefan 2011-03-29 2933 ctl = block_group->free_space_ctl;
34d52cb6c50b5a4 Li Zefan 2011-03-29 2934
fa9c0d795f7b57c Chris Mason 2009-04-03 2935 /* now return any extents the cluster had on it */
34d52cb6c50b5a4 Li Zefan 2011-03-29 2936 spin_lock(&ctl->tree_lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2937 ret = __btrfs_return_cluster_to_free_space(block_group, cluster);
34d52cb6c50b5a4 Li Zefan 2011-03-29 2938 spin_unlock(&ctl->tree_lock);
fa9c0d795f7b57c Chris Mason 2009-04-03 2939
6e80d4f8c422d3b Dennis Zhou 2019-12-13 2940 btrfs_discard_queue_work(&block_group->fs_info->discard_ctl, block_group);
6e80d4f8c422d3b Dennis Zhou 2019-12-13 2941
fa9c0d795f7b57c Chris Mason 2009-04-03 2942 /* finally drop our ref */
fa9c0d795f7b57c Chris Mason 2009-04-03 2943 btrfs_put_block_group(block_group);
fa9c0d795f7b57c Chris Mason 2009-04-03 2944 return ret;
fa9c0d795f7b57c Chris Mason 2009-04-03 2945 }
fa9c0d795f7b57c Chris Mason 2009-04-03 2946
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56114 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] btrfs: convert block group refcount to refcount_t
2020-07-01 20:22 [PATCH 1/2] btrfs: convert block group refcount to refcount_t Josef Bacik
2020-07-01 20:22 ` [PATCH 2/2] btrfs: fix block group UAF bug with nocow Josef Bacik
2020-07-02 1:25 ` [PATCH 1/2] btrfs: convert block group refcount to refcount_t kernel test robot
@ 2020-07-02 5:22 ` kernel test robot
2020-07-02 11:24 ` Filipe Manana
2020-07-03 12:54 ` David Sterba
4 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-07-02 5:22 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team; +Cc: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7865 bytes --]
Hi Josef,
I love your patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Josef-Bacik/btrfs-convert-block-group-refcount-to-refcount_t/20200702-042305
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-a003-20200701 (attached as .config)
compiler: gcc-4.9 (Ubuntu 4.9.3-13ubuntu2) 4.9.3
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from fs/btrfs/free-space-cache.c:14:0:
fs/btrfs/ctree.h:2216:16: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
size_t __const btrfs_get_num_csums(void);
^
fs/btrfs/free-space-cache.c: In function 'btrfs_return_cluster_to_free_space':
>> fs/btrfs/free-space-cache.c:2930:13: warning: passing argument 1 of 'atomic_inc' from incompatible pointer type
atomic_inc(&block_group->count);
^
In file included from arch/x86/include/asm/atomic.h:265:0,
from include/linux/atomic.h:7,
from include/linux/jump_label.h:249,
from include/linux/static_key.h:1,
from arch/x86/include/asm/nospec-branch.h:6,
from arch/x86/include/asm/paravirt_types.h:46,
from arch/x86/include/asm/ptrace.h:94,
from arch/x86/include/asm/math_emu.h:5,
from arch/x86/include/asm/processor.h:13,
from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:6,
from include/linux/mm.h:10,
from include/linux/pagemap.h:8,
from fs/btrfs/free-space-cache.c:6:
include/asm-generic/atomic-instrumented.h:237:1: note: expected 'struct atomic_t *' but argument is of type 'struct refcount_t *'
atomic_inc(atomic_t *v)
^
fs/btrfs/free-space-cache.c: In function 'btrfs_find_space_cluster':
fs/btrfs/free-space-cache.c:3361:14: warning: passing argument 1 of 'atomic_inc' from incompatible pointer type
atomic_inc(&block_group->count);
^
In file included from arch/x86/include/asm/atomic.h:265:0,
from include/linux/atomic.h:7,
from include/linux/jump_label.h:249,
from include/linux/static_key.h:1,
from arch/x86/include/asm/nospec-branch.h:6,
from arch/x86/include/asm/paravirt_types.h:46,
from arch/x86/include/asm/ptrace.h:94,
from arch/x86/include/asm/math_emu.h:5,
from arch/x86/include/asm/processor.h:13,
from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:6,
from include/linux/mm.h:10,
from include/linux/pagemap.h:8,
from fs/btrfs/free-space-cache.c:6:
include/asm-generic/atomic-instrumented.h:237:1: note: expected 'struct atomic_t *' but argument is of type 'struct refcount_t *'
atomic_inc(atomic_t *v)
^
vim +/atomic_inc +2930 fs/btrfs/free-space-cache.c
fa9c0d795f7b57 Chris Mason 2009-04-03 2901
fa9c0d795f7b57 Chris Mason 2009-04-03 2902 /*
fa9c0d795f7b57 Chris Mason 2009-04-03 2903 * given a cluster, put all of its extents back into the free space
fa9c0d795f7b57 Chris Mason 2009-04-03 2904 * cache. If a block group is passed, this function will only free
fa9c0d795f7b57 Chris Mason 2009-04-03 2905 * a cluster that belongs to the passed block group.
fa9c0d795f7b57 Chris Mason 2009-04-03 2906 *
fa9c0d795f7b57 Chris Mason 2009-04-03 2907 * Otherwise, it'll get a reference on the block group pointed to by the
fa9c0d795f7b57 Chris Mason 2009-04-03 2908 * cluster and remove the cluster from it.
fa9c0d795f7b57 Chris Mason 2009-04-03 2909 */
fa9c0d795f7b57 Chris Mason 2009-04-03 2910 int btrfs_return_cluster_to_free_space(
32da5386d9a4fd David Sterba 2019-10-29 2911 struct btrfs_block_group *block_group,
fa9c0d795f7b57 Chris Mason 2009-04-03 2912 struct btrfs_free_cluster *cluster)
fa9c0d795f7b57 Chris Mason 2009-04-03 2913 {
34d52cb6c50b5a Li Zefan 2011-03-29 2914 struct btrfs_free_space_ctl *ctl;
fa9c0d795f7b57 Chris Mason 2009-04-03 2915 int ret;
fa9c0d795f7b57 Chris Mason 2009-04-03 2916
fa9c0d795f7b57 Chris Mason 2009-04-03 2917 /* first, get a safe pointer to the block group */
fa9c0d795f7b57 Chris Mason 2009-04-03 2918 spin_lock(&cluster->lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2919 if (!block_group) {
fa9c0d795f7b57 Chris Mason 2009-04-03 2920 block_group = cluster->block_group;
fa9c0d795f7b57 Chris Mason 2009-04-03 2921 if (!block_group) {
fa9c0d795f7b57 Chris Mason 2009-04-03 2922 spin_unlock(&cluster->lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2923 return 0;
fa9c0d795f7b57 Chris Mason 2009-04-03 2924 }
fa9c0d795f7b57 Chris Mason 2009-04-03 2925 } else if (cluster->block_group != block_group) {
fa9c0d795f7b57 Chris Mason 2009-04-03 2926 /* someone else has already freed it don't redo their work */
fa9c0d795f7b57 Chris Mason 2009-04-03 2927 spin_unlock(&cluster->lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2928 return 0;
fa9c0d795f7b57 Chris Mason 2009-04-03 2929 }
fa9c0d795f7b57 Chris Mason 2009-04-03 @2930 atomic_inc(&block_group->count);
fa9c0d795f7b57 Chris Mason 2009-04-03 2931 spin_unlock(&cluster->lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2932
34d52cb6c50b5a Li Zefan 2011-03-29 2933 ctl = block_group->free_space_ctl;
34d52cb6c50b5a Li Zefan 2011-03-29 2934
fa9c0d795f7b57 Chris Mason 2009-04-03 2935 /* now return any extents the cluster had on it */
34d52cb6c50b5a Li Zefan 2011-03-29 2936 spin_lock(&ctl->tree_lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2937 ret = __btrfs_return_cluster_to_free_space(block_group, cluster);
34d52cb6c50b5a Li Zefan 2011-03-29 2938 spin_unlock(&ctl->tree_lock);
fa9c0d795f7b57 Chris Mason 2009-04-03 2939
6e80d4f8c422d3 Dennis Zhou 2019-12-13 2940 btrfs_discard_queue_work(&block_group->fs_info->discard_ctl, block_group);
6e80d4f8c422d3 Dennis Zhou 2019-12-13 2941
fa9c0d795f7b57 Chris Mason 2009-04-03 2942 /* finally drop our ref */
fa9c0d795f7b57 Chris Mason 2009-04-03 2943 btrfs_put_block_group(block_group);
fa9c0d795f7b57 Chris Mason 2009-04-03 2944 return ret;
fa9c0d795f7b57 Chris Mason 2009-04-03 2945 }
fa9c0d795f7b57 Chris Mason 2009-04-03 2946
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34994 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] btrfs: convert block group refcount to refcount_t
2020-07-01 20:22 [PATCH 1/2] btrfs: convert block group refcount to refcount_t Josef Bacik
` (2 preceding siblings ...)
2020-07-02 5:22 ` kernel test robot
@ 2020-07-02 11:24 ` Filipe Manana
2020-07-03 12:54 ` David Sterba
4 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2020-07-02 11:24 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
On Wed, Jul 1, 2020 at 9:24 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> I was experimenting with some new allocator changes and I noticed that I
> was getting a UAF with the block groups. In order to help figure this
> out I converted the block group to use the refcount_t infrastructure.
> This is a generally good idea anyway, so kill the atomic and use
> refcount_t so we can get the benefit of loud complaints when refcounting
> goes wrong.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Looks good and the patch compiles and works just fine here on the
latest misc-next.
Thanks.
> ---
> fs/btrfs/block-group.c | 8 ++++----
> fs/btrfs/block-group.h | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 09b796a081dd..7c0075413b08 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -118,12 +118,12 @@ u64 btrfs_get_alloc_profile(struct btrfs_fs_info *fs_info, u64 orig_flags)
>
> void btrfs_get_block_group(struct btrfs_block_group *cache)
> {
> - atomic_inc(&cache->count);
> + refcount_inc(&cache->count);
> }
>
> void btrfs_put_block_group(struct btrfs_block_group *cache)
> {
> - if (atomic_dec_and_test(&cache->count)) {
> + if (refcount_dec_and_test(&cache->count)) {
> WARN_ON(cache->pinned > 0);
> WARN_ON(cache->reserved > 0);
>
> @@ -1805,7 +1805,7 @@ static struct btrfs_block_group *btrfs_create_block_group_cache(
>
> cache->discard_index = BTRFS_DISCARD_INDEX_UNUSED;
>
> - atomic_set(&cache->count, 1);
> + refcount_set(&cache->count, 1);
> spin_lock_init(&cache->lock);
> init_rwsem(&cache->data_rwsem);
> INIT_LIST_HEAD(&cache->list);
> @@ -3427,7 +3427,7 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
> ASSERT(list_empty(&block_group->dirty_list));
> ASSERT(list_empty(&block_group->io_list));
> ASSERT(list_empty(&block_group->bg_list));
> - ASSERT(atomic_read(&block_group->count) == 1);
> + ASSERT(refcount_read(&block_group->count) == 1);
> btrfs_put_block_group(block_group);
>
> spin_lock(&info->block_group_cache_lock);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index b6ee70a039c7..705e48050163 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -115,7 +115,7 @@ struct btrfs_block_group {
> struct list_head list;
>
> /* Usage count */
> - atomic_t count;
> + refcount_t count;
>
> /*
> * List of struct btrfs_free_clusters for this block group.
> --
> 2.24.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] btrfs: convert block group refcount to refcount_t
2020-07-01 20:22 [PATCH 1/2] btrfs: convert block group refcount to refcount_t Josef Bacik
` (3 preceding siblings ...)
2020-07-02 11:24 ` Filipe Manana
@ 2020-07-03 12:54 ` David Sterba
4 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-07-03 12:54 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
On Wed, Jul 01, 2020 at 04:22:18PM -0400, Josef Bacik wrote:
> I was experimenting with some new allocator changes and I noticed that I
> was getting a UAF with the block groups. In order to help figure this
> out I converted the block group to use the refcount_t infrastructure.
> This is a generally good idea anyway, so kill the atomic and use
> refcount_t so we can get the benefit of loud complaints when refcounting
> goes wrong.
I don't mind adding some background or motivation of the patch but the
technical part and explanation should be still there and after reading
this paragraph I'm still mssing it.
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/btrfs/block-group.c | 8 ++++----
> fs/btrfs/block-group.h | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 09b796a081dd..7c0075413b08 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -118,12 +118,12 @@ u64 btrfs_get_alloc_profile(struct btrfs_fs_info *fs_info, u64 orig_flags)
>
> void btrfs_get_block_group(struct btrfs_block_group *cache)
> {
> - atomic_inc(&cache->count);
> + refcount_inc(&cache->count);
> }
>
> void btrfs_put_block_group(struct btrfs_block_group *cache)
> {
> - if (atomic_dec_and_test(&cache->count)) {
> + if (refcount_dec_and_test(&cache->count)) {
> WARN_ON(cache->pinned > 0);
> WARN_ON(cache->reserved > 0);
>
> @@ -1805,7 +1805,7 @@ static struct btrfs_block_group *btrfs_create_block_group_cache(
>
> cache->discard_index = BTRFS_DISCARD_INDEX_UNUSED;
>
> - atomic_set(&cache->count, 1);
> + refcount_set(&cache->count, 1);
> spin_lock_init(&cache->lock);
> init_rwsem(&cache->data_rwsem);
> INIT_LIST_HEAD(&cache->list);
> @@ -3427,7 +3427,7 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
> ASSERT(list_empty(&block_group->dirty_list));
> ASSERT(list_empty(&block_group->io_list));
> ASSERT(list_empty(&block_group->bg_list));
> - ASSERT(atomic_read(&block_group->count) == 1);
> + ASSERT(refcount_read(&block_group->count) == 1);
> btrfs_put_block_group(block_group);
>
> spin_lock(&info->block_group_cache_lock);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index b6ee70a039c7..705e48050163 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -115,7 +115,7 @@ struct btrfs_block_group {
> struct list_head list;
>
> /* Usage count */
> - atomic_t count;
> + refcount_t count;
Originally the comment says 'usage', which is a bit different from
refcounts. The atomics will allow 1->0 and 0->1, which might be valid
pattern and reflects the 'usage counter' semantics. The refcounts catch
the 0->1 increment as bug and complain. This is what we want and that's
why you switch that.
So I suggest to drop the comment and rename it to 'refs' that we use
for refcounts elsewhere.
^ permalink raw reply [flat|nested] 7+ messages in thread