linux-kernel - Re: [PATCH v2] 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]

Message-ID: <20200604231327.GV2040@dread.disaster.area>
Date:   Fri, 5 Jun 2020 09:13:27 +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 v2] xfs: Fix false positive lockdep warning with
 sb_internal & fs_reclaim

On Thu, Jun 04, 2020 at 05:01:30PM -0400, Waiman Long wrote:
> ---
>  fs/xfs/xfs_log.c   | 3 ++-
>  fs/xfs/xfs_trans.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 00fda2e8e738..d273d4e74ef8 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -433,7 +433,8 @@ xfs_log_reserve(
>  	XFS_STATS_INC(mp, xs_try_logspace);
>  
>  	ASSERT(*ticp == NULL);
> -	tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, 0);
> +	tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent,
> +			mp->m_super->s_writers.frozen ? KM_NOLOCKDEP : 0);
>  	*ticp = tic;

Hi Waiman,

As I originally stated when you posted this the first time 6 months
ago: we are not going to spread this sort of conditional gunk though
the XFS codebase just to shut up lockdep false positives.

I pointed you at the way to conditionally turn of lockdep for
operations where we are doing transactions when the filesystem has
already frozen the transaction subsystem. That is:

>  
>  	xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 3c94e5ff4316..3a9f394a0f02 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -261,8 +261,14 @@ xfs_trans_alloc(
>  	 * Allocate the handle before we do our freeze accounting and setting up
>  	 * GFP_NOFS allocation context so that we avoid lockdep false positives
>  	 * by doing GFP_KERNEL allocations inside sb_start_intwrite().
> +	 *
> +	 * To prevent false positive lockdep warning of circular locking
> +	 * dependency between sb_internal and fs_reclaim, disable the
> +	 * acquisition of the fs_reclaim pseudo-lock when the superblock
> +	 * has been frozen or in the process of being frozen.
>  	 */
> -	tp = kmem_zone_zalloc(xfs_trans_zone, 0);
> +	tp = kmem_zone_zalloc(xfs_trans_zone,
> +		mp->m_super->s_writers.frozen ? KM_NOLOCKDEP : 0);
>  	if (!(flags & XFS_TRANS_NO_WRITECOUNT))

We only should be setting KM_NOLOCKDEP when XFS_TRANS_NO_WRITECOUNT
is set.  That's the flag that transactions set when they run in a
fully frozen context to avoid deadlocking with the freeze in
progress, and that's the only case where we should be turning off
lockdep.

And, as I also mentioned, this should be done via a process flag -
PF_MEMALLOC_NOLOCKDEP - so that it is automatically inherited by
all subsequent memory allocations done in this path. That way we
only need this wrapping code in xfs_trans_alloc():

	if (flags & XFS_TRANS_NO_WRITECOUNT)
		memalloc_nolockdep_save()

	.....

	if (flags & XFS_TRANS_NO_WRITECOUNT)
		memalloc_nolockdep_restore()

and nothing else needs to change.

Cheers,

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