[05/26] xfs: use an empty transaction to protect xfs_attr_get from deadlocks

Commit Message

Darrick J. Wong April 30, 2024, 3:25 a.m. UTC

From: Darrick J. Wong <[e-mail]>

Wrap the xfs_attr_get_ilocked call in xfs_attr_get with an empty
transaction so that we cannot livelock the kernel if someone injects a
loop into the attr structure or the attr fork bmbt.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/libxfs/xfs_attr.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Christoph Hellwig May 1, 2024, 6:57 a.m. UTC | #1

> +	if (error)
> +		return error;
> +
>  	lock_mode = xfs_ilock_attr_map_shared(args->dp);
> +
> +        /*
> +	 * Make sure the attr fork iext tree is loaded.  Use the empty
> +	 * transaction to load the bmbt so that we avoid livelocking on loops.
> +	 */
> +        if (xfs_inode_hasattr(args->dp)) {
> +                error = xfs_iread_extents(args->trans, args->dp, XFS_ATTR_FORK);

Overly long line here.  But I'd expect the xfs_iread_extents to be in
xfs_attr_get_ilocked anyway instead of duplicated in the callers.

Darrick J. Wong May 1, 2024, 10:42 p.m. UTC | #2

On Tue, Apr 30, 2024 at 11:57:13PM -0700, Christoph Hellwig wrote:
> > +	if (error)
> > +		return error;
> > +
> >  	lock_mode = xfs_ilock_attr_map_shared(args->dp);
> > +
> > +        /*
> > +	 * Make sure the attr fork iext tree is loaded.  Use the empty
> > +	 * transaction to load the bmbt so that we avoid livelocking on loops.
> > +	 */
> > +        if (xfs_inode_hasattr(args->dp)) {
> > +                error = xfs_iread_extents(args->trans, args->dp, XFS_ATTR_FORK);
> 
> Overly long line here.  But I'd expect the xfs_iread_extents to be in
> xfs_attr_get_ilocked anyway instead of duplicated in the callers.

Hmm, I think that's the result of xfs_iread_extents whackamole in
djwong-dev.  You are correct that we don't need this whitespace damaged
blob.

--D

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b841096947acb..e0be8d0c1ffdc 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -274,6 +274,8 @@  xfs_attr_get(
 
 	XFS_STATS_INC(args->dp->i_mount, xs_attr_get);
 
+	ASSERT(!args->trans);
+
 	if (xfs_is_shutdown(args->dp->i_mount))
 		return -EIO;
 
@@ -286,8 +288,27 @@  xfs_attr_get(
 	/* Entirely possible to look up a name which doesn't exist */
 	args->op_flags = XFS_DA_OP_OKNOENT;
 
+	error = xfs_trans_alloc_empty(args->dp->i_mount, &args->trans);
+	if (error)
+		return error;
+
 	lock_mode = xfs_ilock_attr_map_shared(args->dp);
+
+        /*
+	 * Make sure the attr fork iext tree is loaded.  Use the empty
+	 * transaction to load the bmbt so that we avoid livelocking on loops.
+	 */
+        if (xfs_inode_hasattr(args->dp)) {
+                error = xfs_iread_extents(args->trans, args->dp, XFS_ATTR_FORK);
+                if (error)
+                        goto out_cancel;
+        }
+
 	error = xfs_attr_get_ilocked(args);
+
+out_cancel:
+	xfs_trans_cancel(args->trans);
+	args->trans = NULL;
 	xfs_iunlock(args->dp, lock_mode);
 
 	return error;