* [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) @ 2020-11-12 6:30 Gao Xiang 2020-11-12 15:53 ` Eric Sandeen 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang 0 siblings, 2 replies; 11+ messages in thread From: Gao Xiang @ 2020-11-12 6:30 UTC (permalink / raw) To: linux-xfs Cc: Christoph Hellwig, Darrick J. Wong, Eric Sandeen, Gao Xiang, stable 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) >> 8 = (int)(0xfffffffffffffffcUL >> 3) = -1 but on 32-bit platform, the expression is offset = ((unsigned long)336 - (int)340) >> 8 = (int)(0xfffffffcUL >> 3) = 0x1fffffff instead. so offset becomes a large 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 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. 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))) /* * Inode data & attribute fork sizes, per inode. -- 2.18.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) 2020-11-12 6:30 [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) Gao Xiang @ 2020-11-12 15:53 ` Eric Sandeen 2020-11-12 18:30 ` Darrick J. Wong 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang 1 sibling, 1 reply; 11+ messages in thread From: Eric Sandeen @ 2020-11-12 15:53 UTC (permalink / raw) To: Gao Xiang, linux-xfs Cc: Christoph Hellwig, Darrick J. Wong, Eric Sandeen, stable 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. > ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) 2020-11-12 15:53 ` Eric Sandeen @ 2020-11-12 18:30 ` Darrick J. Wong 2020-11-13 2:04 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Darrick J. Wong @ 2020-11-12 18:30 UTC (permalink / raw) To: Eric Sandeen Cc: Gao Xiang, linux-xfs, Christoph Hellwig, Eric Sandeen, stable On Thu, Nov 12, 2020 at 09:53:53AM -0600, Eric Sandeen wrote: > 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. TBH I think this needs to be cleaned up -- what is "LITINO", anyway? I'm pretty sure it's the size of the literal area, aka everything after the inode core, where the forks live? And, uh, can these things get turned into static inline helpers instead of weird macros? Separate patches, obviously. > > > 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? :) Seconded. If this is the fix for the corruption report that dgilmore reported on IRC, this should credit him as the reporter and/or tester. Especially because I thought this was just a "oh I found this via code review" until someone else pointed out that this was actually a fix for something that a user hit in the field. The difference is that we're headed towards -rc4 and I'm much more willing to hold up posting the xfs-5.11-merge branch to get in fixes for user-reported problems. > > 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. It's still weird, because "size of literal inode area" isn't a signed quantity because you can't have a negative size. > 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; So if LITINO is 336 and the caller asked for 350 bytes, offset will be negative here. However, offset is the proposed forkoff, right? It doesn't make any sense to have a negative forkoff, so I think Eric's (bytes > LITINO) suggestion above is correct. This patch was hard to review because the comment for xfs_attr_shortform_bytesfit mentions "...the requested number of additional bytes", but the bytes parameter represents the total number of attr fork bytes we want, not a delta over what we have right now. Can someone please fix that comment too? --D > > 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. > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) 2020-11-12 18:30 ` Darrick J. Wong @ 2020-11-13 2:04 ` Gao Xiang 2020-11-13 2:12 ` Gao Xiang 0 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2020-11-13 2:04 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen Cc: linux-xfs, Christoph Hellwig, Eric Sandeen, stable Hi, On Thu, Nov 12, 2020 at 10:30:04AM -0800, Darrick J. Wong wrote: > On Thu, Nov 12, 2020 at 09:53:53AM -0600, Eric Sandeen wrote: > > 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. > > TBH I think this needs to be cleaned up -- what is "LITINO", anyway? > > I'm pretty sure it's the size of the literal area, aka everything after > the inode core, where the forks live? > > And, uh, can these things get turned into static inline helpers instead > of weird macros? Separate patches, obviously. Thanks, I've addressed all comments in these two reviews in v2: https://lore.kernel.org/r/20201113015044.844213-1-hsiangkao@redhat.com As for LITINO itself, btw, what would be the suggested name for this? I'm not good at naming, and will seek time working on cleaning up this. > > > > > > 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? :) > > Seconded. Will seek time on this later as well. > > If this is the fix for the corruption report that dgilmore reported on > IRC, this should credit him as the reporter and/or tester. Especially > because I thought this was just a "oh I found this via code review" > until someone else pointed out that this was actually a fix for > something that a user hit in the field. > > The difference is that we're headed towards -rc4 and I'm much more > willing to hold up posting the xfs-5.11-merge branch to get in fixes for > user-reported problems. Yeah, sorry for ignoring this original bugreport, since I thought the original bugzilla couldn't open publicly. https://bugzilla.redhat.com/show_bug.cgi?id=1894177 It would be better to get a "Tested-by:" tag to check the original case for v2. :) > > > > 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. > > It's still weird, because "size of literal inode area" isn't a signed > quantity because you can't have a negative size. I'm fine with either way, since my starting point was to address the regression of e9e2eae89ddb as I mentioned on IRC. And it can also be simply fixed directly. Thanks, Gao Xiang > > > 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; > > So if LITINO is 336 and the caller asked for 350 bytes, offset will be > negative here. However, offset is the proposed forkoff, right? It > doesn't make any sense to have a negative forkoff, so I think Eric's > (bytes > LITINO) suggestion above is correct. > > This patch was hard to review because the comment for > xfs_attr_shortform_bytesfit mentions "...the requested number of > additional bytes", but the bytes parameter represents the total number > of attr fork bytes we want, not a delta over what we have right now. > Can someone please fix that comment too? > > --D > > > > > 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. > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) 2020-11-13 2:04 ` Gao Xiang @ 2020-11-13 2:12 ` Gao Xiang 0 siblings, 0 replies; 11+ messages in thread From: Gao Xiang @ 2020-11-13 2:12 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen, Dennis Gilmore Cc: linux-xfs, Christoph Hellwig, Eric Sandeen, stable [ sorry for forgetting to Cc dgilmore on this reply, it would be much better/helpful with a "Tested-by:" for v2... Let me resend, sorry for annoying. ] Hi, On Thu, Nov 12, 2020 at 10:30:04AM -0800, Darrick J. Wong wrote: > On Thu, Nov 12, 2020 at 09:53:53AM -0600, Eric Sandeen wrote: > > 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. > > TBH I think this needs to be cleaned up -- what is "LITINO", anyway? > > I'm pretty sure it's the size of the literal area, aka everything after > the inode core, where the forks live? > > And, uh, can these things get turned into static inline helpers instead > of weird macros? Separate patches, obviously. Thanks, I've addressed all comments in these two reviews in v2: https://lore.kernel.org/r/20201113015044.844213-1-hsiangkao@redhat.com As for LITINO itself, btw, what would be the suggested name for this? I'm not good at naming, and will seek time working on cleaning up this. > > > > > > 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? :) > > Seconded. Will seek time on this later as well. > > If this is the fix for the corruption report that dgilmore reported on > IRC, this should credit him as the reporter and/or tester. Especially > because I thought this was just a "oh I found this via code review" > until someone else pointed out that this was actually a fix for > something that a user hit in the field. > > The difference is that we're headed towards -rc4 and I'm much more > willing to hold up posting the xfs-5.11-merge branch to get in fixes for > user-reported problems. Yeah, sorry for ignoring this original bugreport, since I thought the original bugzilla couldn't open publicly. https://bugzilla.redhat.com/show_bug.cgi?id=1894177 It would be better to get a "Tested-by:" tag to check the original case for v2. :) > > > > 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. > > It's still weird, because "size of literal inode area" isn't a signed > quantity because you can't have a negative size. I'm fine with either way, since my starting point was to address the regression of e9e2eae89ddb as I mentioned on IRC. And it can also be simply fixed directly. Thanks, Gao Xiang > > > 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; > > So if LITINO is 336 and the caller asked for 350 bytes, offset will be > negative here. However, offset is the proposed forkoff, right? It > doesn't make any sense to have a negative forkoff, so I think Eric's > (bytes > LITINO) suggestion above is correct. > > This patch was hard to review because the comment for > xfs_attr_shortform_bytesfit mentions "...the requested number of > additional bytes", but the bytes parameter represents the total number > of attr fork bytes we want, not a delta over what we have right now. > Can someone please fix that comment too? > > --D > > > > > 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. > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-12 6:30 [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) Gao Xiang 2020-11-12 15:53 ` Eric Sandeen @ 2020-11-13 1:50 ` Gao Xiang 2020-11-13 15:31 ` Dennis Gilmore ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Gao Xiang @ 2020-11-13 1:50 UTC (permalink / raw) To: linux-xfs Cc: Dennis Gilmore, Christoph Hellwig, Darrick J. Wong, Eric Sandeen, Gao Xiang, stable 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 */ + if (bytes > XFS_LITINO(mp)) + return 0; + /* rounded down */ offset = (XFS_LITINO(mp) - bytes) >> 3; -- 2.18.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang @ 2020-11-13 15:31 ` Dennis Gilmore 2020-11-14 10:32 ` Christoph Hellwig 2020-11-14 14:02 ` [PATCH v3] " Gao Xiang 2 siblings, 0 replies; 11+ messages in thread From: Dennis Gilmore @ 2020-11-13 15:31 UTC (permalink / raw) To: Gao Xiang Cc: linux-xfs, Christoph Hellwig, Darrick J. Wong, Eric Sandeen, stable Tested-By: Dennis Gilmore <dgilmore@redhat.com> On Thu, Nov 12, 2020 at 7:51 PM Gao Xiang <hsiangkao@redhat.com> 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 */ > + 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang 2020-11-13 15:31 ` Dennis Gilmore @ 2020-11-14 10:32 ` Christoph Hellwig 2020-11-14 13:49 ` Gao Xiang 2020-11-14 14:02 ` [PATCH v3] " Gao Xiang 2 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2020-11-14 10:32 UTC (permalink / raw) To: Gao Xiang Cc: linux-xfs, Dennis Gilmore, Christoph Hellwig, Darrick J. Wong, Eric Sandeen, stable 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: */ Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-14 10:32 ` Christoph Hellwig @ 2020-11-14 13:49 ` Gao Xiang 0 siblings, 0 replies; 11+ messages in thread From: Gao Xiang @ 2020-11-14 13:49 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-xfs, Dennis Gilmore, Darrick J. Wong, Eric Sandeen, stable 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang 2020-11-13 15:31 ` Dennis Gilmore 2020-11-14 10:32 ` Christoph Hellwig @ 2020-11-14 14:02 ` Gao Xiang 2020-11-14 19:06 ` Darrick J. Wong 2 siblings, 1 reply; 11+ messages in thread From: Gao Xiang @ 2020-11-14 14:02 UTC (permalink / raw) To: linux-xfs Cc: Dennis Gilmore, Christoph Hellwig, Darrick J. Wong, Eric Sandeen, Gao Xiang, stable 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. Fixes: e9e2eae89ddb ("xfs: only check the superblock version for dinode size calculation") Cc: <stable@vger.kernel.org> # 5.7+ Reported-and-tested-by: Dennis Gilmore <dgilmore@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- changes since v2: - collect more tags from the replies of v2; - refine a comment suggested by Christoph. fs/xfs/libxfs/xfs_attr_leaf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index bb128db220ac..d6ef69ab1c67 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,12 @@ xfs_attr_shortform_bytesfit( int maxforkoff; int offset; + /* + * Check if the new size could fit at all first: + */ + if (bytes > XFS_LITINO(mp)) + return 0; + /* rounded down */ offset = (XFS_LITINO(mp) - bytes) >> 3; -- 2.18.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xfs: fix forkoff miscalculation related to XFS_LITINO(mp) 2020-11-14 14:02 ` [PATCH v3] " Gao Xiang @ 2020-11-14 19:06 ` Darrick J. Wong 0 siblings, 0 replies; 11+ messages in thread From: Darrick J. Wong @ 2020-11-14 19:06 UTC (permalink / raw) To: Gao Xiang Cc: linux-xfs, Dennis Gilmore, Christoph Hellwig, Eric Sandeen, stable On Sat, Nov 14, 2020 at 10:02:34PM +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. > > Fixes: e9e2eae89ddb ("xfs: only check the superblock version for dinode size calculation") > Cc: <stable@vger.kernel.org> # 5.7+ > Reported-and-tested-by: Dennis Gilmore <dgilmore@redhat.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> Looks good to me, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > changes since v2: > - collect more tags from the replies of v2; > - refine a comment suggested by Christoph. > > fs/xfs/libxfs/xfs_attr_leaf.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index bb128db220ac..d6ef69ab1c67 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,12 @@ xfs_attr_shortform_bytesfit( > int maxforkoff; > int offset; > > + /* > + * Check if the new size could fit at all first: > + */ > + if (bytes > XFS_LITINO(mp)) > + return 0; > + > /* rounded down */ > offset = (XFS_LITINO(mp) - bytes) >> 3; > > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-14 19:08 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-12 6:30 [PATCH] xfs: fix signed calculation related to XFS_LITINO(mp) Gao Xiang 2020-11-12 15:53 ` Eric Sandeen 2020-11-12 18:30 ` Darrick J. Wong 2020-11-13 2:04 ` Gao Xiang 2020-11-13 2:12 ` Gao Xiang 2020-11-13 1:50 ` [PATCH v2] xfs: fix forkoff miscalculation " Gao Xiang 2020-11-13 15:31 ` Dennis Gilmore 2020-11-14 10:32 ` Christoph Hellwig 2020-11-14 13:49 ` Gao Xiang 2020-11-14 14:02 ` [PATCH v3] " Gao Xiang 2020-11-14 19:06 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.