On Thu, Sep 16, 2021 at 03:36:41PM +0530, Chandan Babu R wrote:
> This commit changes xfs_dfork_nextents() to return an error code. The extent
> count itself is now returned through an out argument. This facility will be
> used by a future commit to indicate an inconsistent ondisk extent count.
>
> Signed-off-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_format.h | 14 ++---
> fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++--
> fs/xfs/libxfs/xfs_inode_fork.c | 21 ++++++--
> fs/xfs/scrub/inode.c | 94 +++++++++++++++++++++-------------
> fs/xfs/scrub/inode_repair.c | 34 ++++++++----
> 5 files changed, 118 insertions(+), 61 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index b4638052801f..dba868f2c3e3 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -931,28 +931,30 @@ enum xfs_dinode_fmt {
> (dip)->di_format : \
> (dip)->di_aformat)
>
> -static inline xfs_extnum_t
> +static inline int
> xfs_dfork_nextents(
> struct xfs_dinode *dip,
> - int whichfork)
> + int whichfork,
> + xfs_extnum_t *nextents)
> {
> - xfs_extnum_t nextents = 0;
> + int error = 0;
>
> switch (whichfork) {
> case XFS_DATA_FORK:
> - nextents = be32_to_cpu(dip->di_nextents);
> + *nextents = be32_to_cpu(dip->di_nextents);
> break;
>
> case XFS_ATTR_FORK:
> - nextents = be16_to_cpu(dip->di_anextents);
> + *nextents = be16_to_cpu(dip->di_anextents);
> break;
>
> default:
> ASSERT(0);
> + error = -EFSCORRUPTED;
> break;
> }
>
> - return nextents;
> + return error;
> }
So why do we need to do this? AFAICT, the only check that can return
errors that is added by the ned of the patch series is a
on-disk-format check that does:
if (inode_has_nrext64 && dip->di_nextents16 != 0)
return -EFSCORRUPTED;
This doesn't belong here - it is conflating verification with
extraction. Verfication of the on-disk format belongs in the
verifiers or verification code, not in the function that extracts
> /*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 176c98798aa4..dc511630cc7a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -345,7 +345,8 @@ xfs_dinode_verify_fork(
> xfs_extnum_t di_nextents;
> xfs_extnum_t max_extents;
>
> - di_nextents = xfs_dfork_nextents(dip, whichfork);
> + if (xfs_dfork_nextents(dip, whichfork, &di_nextents))
> + return __this_address;
>
> switch (XFS_DFORK_FORMAT(dip, whichfork)) {
> case XFS_DINODE_FMT_LOCAL:
> @@ -477,6 +478,7 @@ xfs_dinode_verify(
> uint64_t flags2;
> uint64_t di_size;
> xfs_extnum_t nextents;
> + xfs_extnum_t naextents;
> xfs_rfsblock_t nblocks;
>
> if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
> @@ -508,8 +510,13 @@ xfs_dinode_verify(
> if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
> return __this_address;
>
> - nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
> - nextents += xfs_dfork_nextents(dip, XFS_ATTR_FORK);
> + if (xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents))
> + return __this_address;
> +
> + if (xfs_dfork_nextents(dip, XFS_ATTR_FORK, &naextents))
> + return __this_address;
Yeah, so this should end up being:
xfs_failaddr_t
xfs_dfork_nextents_verify(
... )
{
if (ip->di_flags2 & NREXT64) {
if (!xfs_has_nrext64(mp))
return __this_address;
if (dip->di_nextents16 != 0)
return __this_address;
} else if (dip->di_nextents64 != 0)
return __this_address;
}
return NULL;
}
and
faddr = xfs_dfork_nextents_verify(dip, mp);
if (faddr)
return faddr;
nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);
Now all the verification can be removed from xfs_dfork_nextents(),
and anything that needs to verify that the extent counts are in a
valid format can call xfs_dfork_nextents_verify() to perform this
check (i.e. the dinode verifiers and scrub checking code).
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
