On Fri, Jan 10, 2020 at 03:57:42AM -0800, Christoph Hellwig wrote: > > While running generic/103, I observed what looks like memory corruption > > and (with slub debugging turned on) a slub redzone warning on i386 when > > inactivating an inode with a 64k remote attr value. > > > > On a v5 filesystem, maximally sized remote attr values require one block > > more than 64k worth of space to hold both the remote attribute value > > header (64 bytes). On a 4k block filesystem this results in a 68k > > buffer; on a 64k block filesystem, this would be a 128k buffer. Note > > that even though we'll never use more than 65,600 bytes of this buffer, > > XFS_MAX_BLOCKSIZE is 64k. > > > > This is a problem because the definition of struct xfs_buf_log_format > > allows for XFS_MAX_BLOCKSIZE worth of dirty bitmap (64k). On i386 when we > > invalidate a remote attribute, xfs_trans_binval zeroes all 68k worth of > > the dirty map, writing right off the end of the log item and corrupting > > memory. We've gotten away with this on x86_64 for years because the > > compiler inserts a u32 padding on the end of struct xfs_buf_log_format. > > > > Fortunately for us, remote attribute values are written to disk with > > xfs_bwrite(), which is to say that they are not logged. Fix the problem > > by removing all places where we could end up creating a buffer log item > > for a remote attribute value and leave a note explaining why. > > I think this changelog needs an explanation why using > xfs_attr_rmtval_stale which just trylock and checks if the buffers are > in core only in xfs_attr3_leaf_freextent is fine. Hmm, maybe the *function* needs an explanation for why it's valid to use rmtval_stale here: /* * Invalidate any incore buffers associated with this remote attribute * value extent. We never log remote attribute value buffers, which * means that they won't be attached to a transaction and are therefore * safe to mark stale. The actual bunmapi will be taken care of later. */ STATIC int xfs_attr3_rmt_inactive( struct xfs_inode *dp, struct xfs_attr_inactive_list *lp) I say the function is also confusingly named and could have its arguments list trimmed. > And while the incore > part looks sane to me, I think the trylock is wrong and we need to pass > the locking flag to xfs_attr_rmtval_stale. Hmm, yeah, it's definitely wrong for the inactivation case -- the inactivation thread holds the only incore reference to this unlinked inode, so the buffer had better not be locked by another thread. I'm not even all that sure why XBF_TRYLOCK is necessary in the remove case, since we hold ILOCK_EXCL and there shouldn't be anyone else messing around inside the xattr data. --D
