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

[Gao Xiang <hsiangkao@redhat.com>]

On Sat, Nov 14, 2020 at 11:32:49AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 13, 2020 at 09:50:44AM +0800, Gao Xiang 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@redhat.com>
> > Fixes: e9e2eae89ddb ("xfs: only check the superblock version for dinode size calculation")
> > Cc: <stable@vger.kernel.org> # 5.7+
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> > 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 */
> 
> Maybe:
> 
> 	/* 
> 	 * Check if the new size could fit at all first:
> 	 */

ok, let me quick revise it as the next version.

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

Thanks,
Gao Xiang

>