Hi Greg,
These are the two patches that you asked me [1] to defer until they are posted to 5.15.y.
Now that Leah has posted them for 5.15.y [2], please apply them also to 5.10.y.
Note that Leah has an extra patch in her 5.15.y series: "xfs: fix xfs_ifree() error handling to not leak perag ref" This fix does not apply and is not relevant to 5.10.y.
Thanks, Amir.
[1] https://lore.kernel.org/linux-xfs/YxCulVd4dESBjCUM@kroah.com/ [2] https://lore.kernel.org/linux-xfs/20220922151501.2297190-1-leah.rumancik@gma...
Dave Chinner (2): xfs: reorder iunlink remove operation in xfs_ifree xfs: validate inode fork size against fork format
fs/xfs/libxfs/xfs_inode_buf.c | 35 ++++++++++++++++++++++++++--------- fs/xfs/xfs_inode.c | 22 ++++++++++++---------- 2 files changed, 38 insertions(+), 19 deletions(-)
From: Dave Chinner dchinner@redhat.com
commit 9a5280b312e2e7898b6397b2ca3cfd03f67d7be1 upstream.
[backport for 5.10.y]
The O_TMPFILE creation implementation creates a specific order of operations for inode allocation/freeing and unlinked list modification. Currently both are serialised by the AGI, so the order doesn't strictly matter as long as the are both in the same transaction.
However, if we want to move the unlinked list insertions largely out from under the AGI lock, then we have to be concerned about the order in which we do unlinked list modification operations. O_TMPFILE creation tells us this order is inode allocation/free, then unlinked list modification.
Change xfs_ifree() to use this same ordering on unlinked list removal. This way we always guarantee that when we enter the iunlinked list removal code from this path, we already have the AGI locked and we don't have to worry about lock nesting AGI reads inside unlink list locks because it's already locked and attached to the transaction.
We can do this safely as the inode freeing and unlinked list removal are done in the same transaction and hence are atomic operations with respect to log recovery.
Reported-by: Frank Hofmann fhofmann@cloudflare.com Fixes: 298f7bec503f ("xfs: pin inode backing buffer to the inode log item") Signed-off-by: Dave Chinner dchinner@redhat.com Reviewed-by: Darrick J. Wong darrick.wong@oracle.com Signed-off-by: Dave Chinner david@fromorbit.com Signed-off-by: Amir Goldstein amir73il@gmail.com Acked-by: Darrick J. Wong djwong@kernel.org --- fs/xfs/xfs_inode.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 1f61e085676b..929ed3bc5619 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2669,14 +2669,13 @@ xfs_ifree_cluster( }
/* - * This is called to return an inode to the inode free list. - * The inode should already be truncated to 0 length and have - * no pages associated with it. This routine also assumes that - * the inode is already a part of the transaction. + * This is called to return an inode to the inode free list. The inode should + * already be truncated to 0 length and have no pages associated with it. This + * routine also assumes that the inode is already a part of the transaction. * - * The on-disk copy of the inode will have been added to the list - * of unlinked inodes in the AGI. We need to remove the inode from - * that list atomically with respect to freeing it here. + * The on-disk copy of the inode will have been added to the list of unlinked + * inodes in the AGI. We need to remove the inode from that list atomically with + * respect to freeing it here. */ int xfs_ifree( @@ -2694,13 +2693,16 @@ xfs_ifree( ASSERT(ip->i_d.di_nblocks == 0);
/* - * Pull the on-disk inode from the AGI unlinked list. + * Free the inode first so that we guarantee that the AGI lock is going + * to be taken before we remove the inode from the unlinked list. This + * makes the AGI lock -> unlinked list modification order the same as + * used in O_TMPFILE creation. */ - error = xfs_iunlink_remove(tp, ip); + error = xfs_difree(tp, ip->i_ino, &xic); if (error) return error;
- error = xfs_difree(tp, ip->i_ino, &xic); + error = xfs_iunlink_remove(tp, ip); if (error) return error;
On Thu, Sep 22, 2022 at 06:47:27PM +0300, Amir Goldstein wrote:
From: Dave Chinner dchinner@redhat.com
commit 9a5280b312e2e7898b6397b2ca3cfd03f67d7be1 upstream.
[backport for 5.10.y]
The O_TMPFILE creation implementation creates a specific order of operations for inode allocation/freeing and unlinked list modification. Currently both are serialised by the AGI, so the order doesn't strictly matter as long as the are both in the same transaction.
However, if we want to move the unlinked list insertions largely out from under the AGI lock, then we have to be concerned about the order in which we do unlinked list modification operations. O_TMPFILE creation tells us this order is inode allocation/free, then unlinked list modification.
Change xfs_ifree() to use this same ordering on unlinked list removal. This way we always guarantee that when we enter the iunlinked list removal code from this path, we already have the AGI locked and we don't have to worry about lock nesting AGI reads inside unlink list locks because it's already locked and attached to the transaction.
We can do this safely as the inode freeing and unlinked list removal are done in the same transaction and hence are atomic operations with respect to log recovery.
Reported-by: Frank Hofmann fhofmann@cloudflare.com Fixes: 298f7bec503f ("xfs: pin inode backing buffer to the inode log item") Signed-off-by: Dave Chinner dchinner@redhat.com Reviewed-by: Darrick J. Wong darrick.wong@oracle.com Signed-off-by: Dave Chinner david@fromorbit.com Signed-off-by: Amir Goldstein amir73il@gmail.com Acked-by: Darrick J. Wong djwong@kernel.org
fs/xfs/xfs_inode.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 1f61e085676b..929ed3bc5619 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2669,14 +2669,13 @@ xfs_ifree_cluster( } /*
- This is called to return an inode to the inode free list.
- The inode should already be truncated to 0 length and have
- no pages associated with it. This routine also assumes that
- the inode is already a part of the transaction.
- This is called to return an inode to the inode free list. The inode should
- already be truncated to 0 length and have no pages associated with it. This
- routine also assumes that the inode is already a part of the transaction.
- The on-disk copy of the inode will have been added to the list
- of unlinked inodes in the AGI. We need to remove the inode from
- that list atomically with respect to freeing it here.
- The on-disk copy of the inode will have been added to the list of unlinked
- inodes in the AGI. We need to remove the inode from that list atomically with
*/
- respect to freeing it here.
int xfs_ifree( @@ -2694,13 +2693,16 @@ xfs_ifree( ASSERT(ip->i_d.di_nblocks == 0); /*
* Pull the on-disk inode from the AGI unlinked list.
* Free the inode first so that we guarantee that the AGI lock is going* to be taken before we remove the inode from the unlinked list. This* makes the AGI lock -> unlinked list modification order the same as* used in O_TMPFILE creation.
- error = xfs_iunlink_remove(tp, ip);
- error = xfs_difree(tp, ip->i_ino, &xic); if (error) return error;
- error = xfs_difree(tp, ip->i_ino, &xic);
- error = xfs_iunlink_remove(tp, ip); if (error) return error;
2.25.1
Any specific reason you do not want 6f5097e3367a ("xfs: fix xfs_ifree() error handling to not leak perag ref") also applied? That commit fixes this one (or so it says.)
That is part of the 5.15 queue right now, but not 5.10, is that a problem?
thanks,
greg k-h
From: Dave Chinner dchinner@redhat.com
commit 1eb70f54c445fcbb25817841e774adb3d912f3e8 upstream.
[backport for 5.10.y]
xfs_repair catches fork size/format mismatches, but the in-kernel verifier doesn't, leading to null pointer failures when attempting to perform operations on the fork. This can occur in the xfs_dir_is_empty() where the in-memory fork format does not match the size and so the fork data pointer is accessed incorrectly.
Note: this causes new failures in xfs/348 which is testing mode vs ftype mismatches. We now detect a regular file that has been changed to a directory or symlink mode as being corrupt because the data fork is for a symlink or directory should be in local form when there are only 3 bytes of data in the data fork. Hence the inode verify for the regular file now fires w/ -EFSCORRUPTED because the inode fork format does not match the format the corrupted mode says it should be in.
Signed-off-by: Dave Chinner dchinner@redhat.com Reviewed-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Dave Chinner david@fromorbit.com Signed-off-by: Amir Goldstein amir73il@gmail.com Acked-by: Darrick J. Wong djwong@kernel.org --- fs/xfs/libxfs/xfs_inode_buf.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c index c667c63f2cb0..fa8aefe6b7ec 100644 --- a/fs/xfs/libxfs/xfs_inode_buf.c +++ b/fs/xfs/libxfs/xfs_inode_buf.c @@ -358,19 +358,36 @@ xfs_dinode_verify_fork( int whichfork) { uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); + mode_t mode = be16_to_cpu(dip->di_mode); + uint32_t fork_size = XFS_DFORK_SIZE(dip, mp, whichfork); + uint32_t fork_format = XFS_DFORK_FORMAT(dip, whichfork);
- switch (XFS_DFORK_FORMAT(dip, whichfork)) { + /* + * For fork types that can contain local data, check that the fork + * format matches the size of local data contained within the fork. + * + * For all types, check that when the size says the should be in extent + * or btree format, the inode isn't claiming it is in local format. + */ + if (whichfork == XFS_DATA_FORK) { + if (S_ISDIR(mode) || S_ISLNK(mode)) { + if (be64_to_cpu(dip->di_size) <= fork_size && + fork_format != XFS_DINODE_FMT_LOCAL) + return __this_address; + } + + if (be64_to_cpu(dip->di_size) > fork_size && + fork_format == XFS_DINODE_FMT_LOCAL) + return __this_address; + } + + switch (fork_format) { case XFS_DINODE_FMT_LOCAL: /* - * no local regular files yet + * No local regular files yet. */ - if (whichfork == XFS_DATA_FORK) { - if (S_ISREG(be16_to_cpu(dip->di_mode))) - return __this_address; - if (be64_to_cpu(dip->di_size) > - XFS_DFORK_SIZE(dip, mp, whichfork)) - return __this_address; - } + if (S_ISREG(mode) && whichfork == XFS_DATA_FORK) + return __this_address; if (di_nextents) return __this_address; break;
On Thu, Sep 22, 2022 at 06:47:26PM +0300, Amir Goldstein wrote:
Hi Greg,
These are the two patches that you asked me [1] to defer until they are posted to 5.15.y.
Now that Leah has posted them for 5.15.y [2], please apply them also to 5.10.y.
Note that Leah has an extra patch in her 5.15.y series: "xfs: fix xfs_ifree() error handling to not leak perag ref" This fix does not apply and is not relevant to 5.10.y.
Thanks, Amir.
[1] https://lore.kernel.org/linux-xfs/YxCulVd4dESBjCUM@kroah.com/ [2] https://lore.kernel.org/linux-xfs/20220922151501.2297190-1-leah.rumancik@gma...
Dave Chinner (2): xfs: reorder iunlink remove operation in xfs_ifree xfs: validate inode fork size against fork format
fs/xfs/libxfs/xfs_inode_buf.c | 35 ++++++++++++++++++++++++++--------- fs/xfs/xfs_inode.c | 22 ++++++++++++---------- 2 files changed, 38 insertions(+), 19 deletions(-)
-- 2.25.1
Now queued up, thnaks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Amir Goldstein -
Greg Kroah-Hartman