[bug report] btrfs: locking: Add extra check in btrfs_init_new_buffer() to avoid deadlock

[bug report] btrfs: locking: Add extra check in btrfs_init_new_buffer() to avoid deadlock

[Dan Carpenter]

[ This one is not actually your fault.  It's the core code which is
  confusing.  -dan ]

Hello Qu Wenruo,

The patch b72c3aba09a5: "btrfs: locking: Add extra check in
btrfs_init_new_buffer() to avoid deadlock" from Aug 21, 2018, leads
to the following static checker warning:

	fs/btrfs/extent-tree.c:8556 btrfs_init_new_buffer()
	warn: possible NULL dereference of 'buf'

fs/btrfs/extent-tree.c
    8540 static struct extent_buffer *
    8541 btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
    8542 		      u64 bytenr, int level, u64 owner)
    8543 {
    8544 	struct btrfs_fs_info *fs_info = root->fs_info;
    8545 	struct extent_buffer *buf;
    8546 
    8547 	buf = btrfs_find_create_tree_block(fs_info, bytenr);

The btrfs_find_create_tree_block() function either returns
alloc_test_extent_buffer() which returns NULL on error or
alloc_extent_buffer() which returns error pointers on error.  It
confuses static checkers and me also.

    8548 	if (IS_ERR(buf))
    8549 		return buf;
    8550 
    8551 	/*
    8552 	 * Extra safety check in case the extent tree is corrupted and extent
    8553 	 * allocator chooses to use a tree block which is already used and
    8554 	 * locked.
    8555 	 */
--> 8556 	if (buf->lock_owner == current->pid) {
    8557 		btrfs_err_rl(fs_info,
    8558 "tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
    8559 			buf->start, btrfs_header_owner(buf), current->pid);
    8560 		free_extent_buffer(buf);

regards,
dan carpenter

Re: [bug report] btrfs: locking: Add extra check in btrfs_init_new_buffer() to avoid deadlock

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2010 bytes --]



On 2019/2/21 下午10:13, Dan Carpenter wrote:
> [ This one is not actually your fault.  It's the core code which is
>   confusing.  -dan ]
> 
> Hello Qu Wenruo,
> 
> The patch b72c3aba09a5: "btrfs: locking: Add extra check in
> btrfs_init_new_buffer() to avoid deadlock" from Aug 21, 2018, leads
> to the following static checker warning:
> 
> 	fs/btrfs/extent-tree.c:8556 btrfs_init_new_buffer()
> 	warn: possible NULL dereference of 'buf'
> 
> fs/btrfs/extent-tree.c
>     8540 static struct extent_buffer *
>     8541 btrfs_init_new_buffer(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>     8542 		      u64 bytenr, int level, u64 owner)
>     8543 {
>     8544 	struct btrfs_fs_info *fs_info = root->fs_info;
>     8545 	struct extent_buffer *buf;
>     8546 
>     8547 	buf = btrfs_find_create_tree_block(fs_info, bytenr);
> 
> The btrfs_find_create_tree_block() function either returns
> alloc_test_extent_buffer() which returns NULL on error

You caught me!

Indeed that's a possible NULL return case.

The only good news is it shouldn't affect release build which normally
doesn't enabled CONFIG_BTRFS_FS_RUN_SANITY_TESTS.

I'll fix it by unifying the return pointer.

Thanks for pointing this out,
Qu

> or
> alloc_extent_buffer() which returns error pointers on error.  It
> confuses static checkers and me also.
> 
>     8548 	if (IS_ERR(buf))
>     8549 		return buf;
>     8550 
>     8551 	/*
>     8552 	 * Extra safety check in case the extent tree is corrupted and extent
>     8553 	 * allocator chooses to use a tree block which is already used and
>     8554 	 * locked.
>     8555 	 */
> --> 8556 	if (buf->lock_owner == current->pid) {
>     8557 		btrfs_err_rl(fs_info,
>     8558 "tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
>     8559 			buf->start, btrfs_header_owner(buf), current->pid);
>     8560 		free_extent_buffer(buf);
> 
> regards,
> dan carpenter
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]