Re: [PATCH] xfs: fix incorrect usage of xfs_btree_check_block

Eric Sandeen
2022-11-17T16:15:13Z

On 11/7/22 7:50 PM, Guo Xuenan wrote:
> On 2022/11/8 0:58, Darrick J. Wong wrote:
>> On Thu, Nov 03, 2022 at 07:37:09PM +0800, Guo Xuenan wrote:
>>> xfs_btree_check_block contains a tag XFS_ERRTAG_BTREE_CHECK_{L,S}BLOCK,
>>> it is a fault injection tag, better not use it in the macro ASSERT.
>>>
>>> Since with XFS_DEBUG setting up, we can always trigger assert by `echo 1
>>>> /sys/fs/xfs/${disk}/errortag/btree_chk_{s,l}blk`.
>>> It's confusing and strange.
>> Please be more specific about how this is confusing or strange.
> I meant in current code, the ASSERT will alway happen,when we
> `echo 1 > /sys/fs/xfs/${disk}/errortag/btree_chk_{s,l}blk`.
> xfs_btree_islastblock
>   ->ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);
>     ->xfs_btree_check_{l/s}block
>       ->XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BTREE_CHECK_{S,L}BLOCK)
> we can use error injection to trigger this ASSERT.

Hmmm...

> I think ASERRT macro and error injection are to find some effective problems,
> not to create some kernel panic.

You can avoid a panic by turning XFS_ASSERT_FATAL off in Kconfig, or
at runtime by setting fs.xfs.bug_on_assert to 0, but ...

> So, putting the error injection function in
> ASSERT is a little strange.

Ok, so I think the argument is that in the default config, setting this error
injection tag will immediately result in a system panic, which probably isn't
what we want. Is my understanding correct?

But in the bigger picture, isn't this:

ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);

putting a disk corruption check into an ASSERT? That in itself seems a bit
suspect. However, the ASSERT was all introduced in:

commit 27d9ee577dccec94fb0fc1a14728de64db342f86
Author: Darrick J. Wong <darrick.wong@oracle.com>
Date: Wed Nov 6 08:47:09 2019 -0800

xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock

Coverity points out that xfs_btree_islastblock doesn't check the return
value of xfs_btree_check_block. Since the question "Does the cursor
point to the last block in this level?" only makes sense if the caller
previously performed a lookup or seek operation, the block should
already have been checked.

Therefore, check the return value in an ASSERT and turn the whole thing
into a static inline predicate.

Coverity-id: 114069
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>

which seems to imply that we really should not get here with a corrupt block during
normal operation.

Perhaps the error tag can get set after the block "should already have been checked"
but before this test in the ASSERT?

-Eric