Re: [PATCH v2] xfs: fix forkoff miscalculation related to XFS_LITINO(mp)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Tested-By: Dennis Gilmore <dgilmore@xxxxxxxxxx>

On Thu, Nov 12, 2020 at 7:51 PM Gao Xiang <hsiangkao@xxxxxxxxxx> wrote:
>
> Currently, commit e9e2eae89ddb dropped a (int) decoration from
> XFS_LITINO(mp), and since sizeof() expression is also involved,
> the result of XFS_LITINO(mp) is simply as the size_t type
> (commonly unsigned long).
>
> Considering the expression in xfs_attr_shortform_bytesfit():
>   offset = (XFS_LITINO(mp) - bytes) >> 3;
> let "bytes" be (int)340, and
>     "XFS_LITINO(mp)" be (unsigned long)336.
>
> on 64-bit platform, the expression is
>   offset = ((unsigned long)336 - (int)340) >> 3 =
>            (int)(0xfffffffffffffffcUL >> 3) = -1
>
> but on 32-bit platform, the expression is
>   offset = ((unsigned long)336 - (int)340) >> 3 =
>            (int)(0xfffffffcUL >> 3) = 0x1fffffff
> instead.
>
> so offset becomes a large positive number on 32-bit platform, and
> cause xfs_attr_shortform_bytesfit() returns maxforkoff rather than 0.
>
> Therefore, one result is
>   "ASSERT(new_size <= XFS_IFORK_SIZE(ip, whichfork));"
>
> assertion failure in xfs_idata_realloc(), which was also the root
> cause of the original bugreport from Dennis, see:
>    https://bugzilla.redhat.com/show_bug.cgi?id=1894177
>
> And it can also be manually triggered with the following commands:
>   $ touch a;
>   $ setfattr -n user.0 -v "`seq 0 80`" a;
>   $ setfattr -n user.1 -v "`seq 0 80`" a
>
> on 32-bit platform.
>
> Fix the case in xfs_attr_shortform_bytesfit() by bailing out
> "XFS_LITINO(mp) < bytes" in advance suggested by Eric and a misleading
> comment together with this bugfix suggested by Darrick. It seems the
> other users of XFS_LITINO(mp) are not impacted.
>
> Reported-by: Dennis Gilmore <dgilmore@xxxxxxxxxx>
> Fixes: e9e2eae89ddb ("xfs: only check the superblock version for dinode size calculation")
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.7+
> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx>
> ---
> changes since v1:
>  - fix 2 typos ">> 8" to ">> 3" mentioned by Eric;
>  - directly bail out "XFS_LITINO(mp) < bytes" suggested
>    by Eric and Darrick;
>  - fix a misleading comment together suggested by Darrick;
>  - since (int) decorator doesn't need to be added, so update
>    the patch subject as well.
>
>  fs/xfs/libxfs/xfs_attr_leaf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index bb128db220ac..c8d91034850b 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -515,7 +515,7 @@ xfs_attr_copy_value(
>   *========================================================================*/
>
>  /*
> - * Query whether the requested number of additional bytes of extended
> + * Query whether the total requested number of attr fork bytes of extended
>   * attribute space will be able to fit inline.
>   *
>   * Returns zero if not, else the di_forkoff fork offset to be used in the
> @@ -535,6 +535,10 @@ xfs_attr_shortform_bytesfit(
>         int                     maxforkoff;
>         int                     offset;
>
> +       /* there is no chance we can fit */
> +       if (bytes > XFS_LITINO(mp))
> +               return 0;
> +
>         /* rounded down */
>         offset = (XFS_LITINO(mp) - bytes) >> 3;
>
> --
> 2.18.4
>


-- 
Dennis Gilmore
Multiple Architecture Portfolio Enablement
T: +1-312-660-3523




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux