Re: [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp)

[Eric Sandeen <sandeen@sandeen.net>]

On 11/12/20 12:30 AM, 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).

Thanks for finding this!  Let me think through it a little.
 
> 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) >> 8 =

This should be >> 3, right.

>            (int)(0xfffffffffffffffcUL >> 3) = -1
> 
> but on 32-bit platform, the expression is
>   offset = ((unsigned long)336 - (int)340) >> 8 =

and >> 3 here as well.

>            (int)(0xfffffffcUL >> 3) = 0x1fffffff
> instead.

Ok.  But wow, that magical cast to (int) in XFS_LITINO isn't at
all clear to me.

XFS_LITINO() indicates a /size/ - fixing this problem by making it a
signed value feels very strange, but I'm not sure what would be better,
yet.

> Therefore, one result is
>   "ASSERT(new_size <= XFS_IFORK_SIZE(ip, whichfork));"
>   assertion failure in xfs_idata_realloc().
> 
> , which can be 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.

Can you please write an xfstest for this? :)

> Fix it by restoring (int) decorator to XFS_LITINO(mp) since
> int type for XFS_LITINO(mp) is safe and all pre-exist signed
> calculations are correct.
> 
> 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>
> ---
> I'm not sure this is the preferred way or just simply fix
> xfs_attr_shortform_bytesfit() since I don't look into the
> rest of XFS_LITINO(mp) users. Add (int) to XFS_LITINO(mp)
> will avoid all potential regression at least.
> 
>  fs/xfs/libxfs/xfs_format.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index dd764da08f6f..f58f0a44c024 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -1061,7 +1061,7 @@ enum xfs_dinode_fmt {
>  		sizeof(struct xfs_dinode) : \
>  		offsetof(struct xfs_dinode, di_crc))>  #define XFS_LITINO(mp) \
> -	((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(&(mp)->m_sb))
> +	((int)((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(&(mp)->m_sb)))

If we do keep the (int) cast here we at least need a comment explaining why
it cannot be removed, unless there is a better way to solve the problem.

I wonder if explicitly making XFS_LITINO() cast to a size_t would be
best, and then in xfs_attr_shortform_bytesfit() we just quickly reject
the query if the bytes are larger than the literal area:

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index bb128db..5588c2e 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -535,6 +535,10 @@ STATIC void xfs_attr3_leaf_moveents(struct xfs_da_args *args,
        int                     maxforkoff;
        int                     offset;
 
+       /* Is there no chance we can fit? */
+       if (bytes > XFS_LITINO(mp))
+               return 0;
+
        /* rounded down */
        offset = (XFS_LITINO(mp) - bytes) >> 3;

or, maybe simply:

-        offset = (XFS_LITINO(mp) - bytes) >> 3;
+        offset = (int)(XFS_LITINO(mp) - bytes) >> 3;

to be sure that the arithmetic doesn't get promoted to unsigned before the shift?

or maybe others have better ideas.

-Eric

  
>  /*
>   * Inode data & attribute fork sizes, per inode.
>