linux-kernel - Re: [PATCH v4] xfs: Fix false positive lockdep warning with sb_internal & fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Jun 2020 08:58:10 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Waiman Long <longman@...hat.com>
Cc:     "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Qian Cai <cai@....pw>, Eric Sandeen <sandeen@...hat.com>
Subject: Re: [PATCH v4] xfs: Fix false positive lockdep warning with
 sb_internal & fs_reclaim

On Thu, Jun 18, 2020 at 01:19:41PM -0400, Waiman Long wrote:
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 379cbff438bc..1b94b9bfa4d7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -913,11 +913,33 @@ xfs_fs_freeze(
>  	struct super_block	*sb)
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
> +	unsigned long		pflags;
> +	int			ret;
>  
> +	/*
> +	 * A fs_reclaim pseudo lock is added to check for potential deadlock
> +	 * condition with fs reclaim. The following lockdep splat was hit
> +	 * occasionally. This is actually a false positive as the allocation
> +	 * is being done only after the frozen filesystem is no longer dirty.
> +	 * One way to avoid this splat is to add GFP_NOFS to the affected
> +	 * allocation calls. This is what PF_MEMALLOC_NOFS is for.
> +	 *
> +	 *       CPU0                    CPU1
> +	 *       ----                    ----
> +	 *  lock(sb_internal);
> +	 *                               lock(fs_reclaim);
> +	 *                               lock(sb_internal);
> +	 *  lock(fs_reclaim);
> +	 *
> +	 *  *** DEADLOCK ***
> +	 */

The lockdep splat is detailed in the commit message - it most
definitely does not need to be repeated in full here because:

	a) it doesn't explain why the splat occurring is, and
	b) we most definitely don't care about how the lockdep check
	   that triggered it is implemented.

IOWs, the comment here needs to explain how the freeze state held at
this point requires that we avoid reclaim recursion back into the
filesystem, regardless of how lockdep detects it or whether the
lockdep splats are a false positive or not...

e.g.

/*
 * The superblock is now in the frozen state, which means we cannot
 * allow memory allocation to recurse into reclaim on this
 * filesystem as this may require running operations that the
 * current freeze state prevents. This should not occur if
 * everything is working correctly and sometimes lockdep may report
 * false positives in this path. However, to be safe and to avoid
 * unnecessary false positives in test/CI environments, put the
 * entire final freeze processing path under GFP_NOFS allocation
 * contexts to prevent reclaim recursion from occurring anywhere in
 * the path.
 */

Cheers,

Dave.

> +	current_set_flags_nested(&pflags, PF_MEMALLOC_NOFS);
>  	xfs_stop_block_reaping(mp);
>  	xfs_save_resvblks(mp);
>  	xfs_quiesce_attr(mp);
> -	return xfs_sync_sb(mp, true);
> +	ret = xfs_sync_sb(mp, true);
> +	current_restore_flags_nested(&pflags, PF_MEMALLOC_NOFS);
> +	return ret;
>  }
>  
>  STATIC int
> -- 
> 2.18.1
> 
> 

-- 
Dave Chinner
david@...morbit.com