Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused

From: Dave Chinner <david@fromorbit.com>
To: Stephen Zhang <starzhangzsd@gmail.com>
Cc: djwong@kernel.org, dchinner@redhat.com, chandan.babu@oracle.com,
	zhangshida@kylinos.cn, linux-kernel@vger.kernel.org,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused
Date: Mon, 12 Sep 2022 09:12:51 +1000	[thread overview]
Message-ID: <20220911231251.GA3600936@dread.disaster.area> (raw)
In-Reply-To: <20220909030756.3916297-1-zhangshida@kylinos.cn>

On Fri, Sep 09, 2022 at 11:07:56AM +0800, Stephen Zhang wrote:
> Given that
>         max >= lowest,
> hence if
>         got.br_startoff >= max + len,
> then, at the same time,
>         got.br_startoff >= lowest + len,
> 
> So the check here is redundant, remove it.

Check your types: what happens when *first_unused =
XFS_DIR2_LEAF_OFFSET?

> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index e56723dc9cd5..f8a984c41b01 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1230,8 +1230,7 @@ xfs_bmap_first_unused(
>  		/*
>  		 * See if the hole before this extent will work.
>  		 */
> -		if (got.br_startoff >= lowest + len &&
> -		    got.br_startoff - max >= len)
> +		if (got.br_startoff - max >= len)
>  			break;
>  		lastaddr = got.br_startoff + got.br_blockcount;
>  		max = XFS_FILEOFF_MAX(lastaddr, lowest);

This loop does a linear scan of the extent list, so it starts at
extent index zero which will be got.br_startoff = 0 for the
first directory data block.

When we are called from xfs_da_grow_inode_int(), we're trying to add
blocks in the directory leaf btree segment here. Hence the lowest
file offset we want to search for a hole is XFS_DIR2_LEAF_OFFSET.

Given that all the types and comparisons involved are 64 bit
unsigned:

typedef uint64_t        xfs_fileoff_t;  /* block number in a file */ 

#define XFS_FILEOFF_MAX(a,b) max_t(xfs_fileoff_t, (a), (b))

	xfs_fileoff_t br_startoff;

        xfs_fileoff_t           lastaddr = 0;
	xfs_fileoff_t           lowest, max;

We end up with the following calculations (in FSBs, not bytes):

	lowest + len	= 0x800000ULL + 1 
			= 0x800001ULL

	got.br_startoff - max	= 0ULL - 0x800000
				= 0xffffffffff800000ULL

and so the existing check is:

	if (0 >= 0x800001ULL && 0xffffffffff800000 >= 1)

which evaluates as false because the extent that was found is not
beyond the initial offset (first_unused) that we need to start
searching at.

With your modification, this would now evaluate as:

	if (0xffffffffff800000 >= 1)

Because of the underflow, this would then evaluate as true  and we'd
return 0 as the first unused offset. This is incorrect as we do not
have a hole at offset 0, nor is it within the correct directory
offset segment, nor is it within the search bounds we have
specified.

If these were all signed types, then your proposed code might be
correct. But they are unsigned and hence we have to ensure that we
handle overflow/underflow appropriately.

Which leads me to ask: did you test this change before you send
it to the list?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com