Re: [PATCH v2] xfs: remove racy hasattr check from attr ops

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 09, 2017 at 10:22:58AM -0500, Brian Foster wrote:
> xfs_attr_[get|remove]() have unlocked attribute fork checks to optimize
> away a lock cycle in cases where the fork does not exist or is otherwise
> empty. This check is not safe, however, because an attribute fork short
> form to extent format conversion includes a transient state that causes
> the xfs_inode_hasattr() check to fail. Specifically,
> xfs_attr_shortform_to_leaf() creates an empty extent format attribute
> fork and then adds the existing shortform attributes to it.
> 
> This means that lookup of an existing xattr can spuriously return
> -ENOATTR when racing against a setxattr that causes the associated
> format conversion. This was originally reproduced by an untar on a
> particularly configured glusterfs volume, but can also be reproduced on
> demand with properly crafted xattr requests.
> 
> The format conversion occurs under the exclusive ilock. xfs_attr_get()
> and xfs_attr_remove() already have the proper locking and checks further
> down in the functions to handle this situation correctly. Drop the
> unlocked checks to avoid the spurious failure and rely on the existing
> logic.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 

Ping. Any chance of getting this one in soonish?

Brian

> v2:
> - Fix up xfs_attr_remove() as well.
> v1: https://patchwork.kernel.org/patch/9501675/
> 
>  fs/xfs/libxfs/xfs_attr.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index af1ecb1..6622d46 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -131,9 +131,6 @@ xfs_attr_get(
>  	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
>  		return -EIO;
>  
> -	if (!xfs_inode_hasattr(ip))
> -		return -ENOATTR;
> -
>  	error = xfs_attr_args_init(&args, ip, name, flags);
>  	if (error)
>  		return error;
> @@ -392,9 +389,6 @@ xfs_attr_remove(
>  	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
>  		return -EIO;
>  
> -	if (!xfs_inode_hasattr(dp))
> -		return -ENOATTR;
> -
>  	error = xfs_attr_args_init(&args, dp, name, flags);
>  	if (error)
>  		return error;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html