linux-ext4 - Re: [PATCH 4/6] xfs: use memalloc_nofs_{save,restore} instead of memalloc

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: <20170206195111.GH3580@birch.djwong.org>
Date:   Mon, 6 Feb 2017 11:51:11 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        Dave Chinner <david@...morbit.com>, djwong@...nel.org,
        "Theodore Ts'o" <tytso@....edu>, Chris Mason <clm@...com>,
        David Sterba <dsterba@...e.cz>, Jan Kara <jack@...e.cz>,
        ceph-devel@...r.kernel.org, cluster-devel@...hat.com,
        linux-nfs@...r.kernel.org, logfs@...fs.org,
        linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-btrfs@...r.kernel.org, linux-mtd@...ts.infradead.org,
        reiserfs-devel@...r.kernel.org,
        linux-ntfs-dev@...ts.sourceforge.net,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-afs@...ts.infradead.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/6] xfs: use memalloc_nofs_{save,restore} instead of
 memalloc_noio*

On Mon, Feb 06, 2017 at 07:47:43PM +0100, Michal Hocko wrote:
> On Mon 06-02-17 10:32:37, Darrick J. Wong wrote:
> > On Mon, Feb 06, 2017 at 06:44:15PM +0100, Michal Hocko wrote:
> > > On Mon 06-02-17 07:39:23, Matthew Wilcox wrote:
> > > > On Mon, Feb 06, 2017 at 03:07:16PM +0100, Michal Hocko wrote:
> > > > > +++ b/fs/xfs/xfs_buf.c
> > > > > @@ -442,17 +442,17 @@ _xfs_buf_map_pages(
> > > > >  		bp->b_addr = NULL;
> > > > >  	} else {
> > > > >  		int retried = 0;
> > > > > -		unsigned noio_flag;
> > > > > +		unsigned nofs_flag;
> > > > >  
> > > > >  		/*
> > > > >  		 * vm_map_ram() will allocate auxillary structures (e.g.
> > > > >  		 * pagetables) with GFP_KERNEL, yet we are likely to be under
> > > > >  		 * GFP_NOFS context here. Hence we need to tell memory reclaim
> > > > > -		 * that we are in such a context via PF_MEMALLOC_NOIO to prevent
> > > > > +		 * that we are in such a context via PF_MEMALLOC_NOFS to prevent
> > > > >  		 * memory reclaim re-entering the filesystem here and
> > > > >  		 * potentially deadlocking.
> > > > >  		 */
> > > > 
> > > > This comment feels out of date ... how about:
> > > 
> > > which part is out of date?
> > > 
> > > > 
> > > > 		/*
> > > > 		 * vm_map_ram will allocate auxiliary structures (eg page
> > > > 		 * tables) with GFP_KERNEL.  If that tries to reclaim memory
> > > > 		 * by calling back into this filesystem, we may deadlock.
> > > > 		 * Prevent that by setting the NOFS flag.
> > > > 		 */
> > > 
> > > dunno, the previous wording seems clear enough to me. Maybe little bit
> > > more chatty than yours but I am not sure this is worth changing.
> > 
> > I prefer to keep the "...yet we are likely to be under GFP_NOFS..."
> > wording of the old comment because it captures the uncertainty of
> > whether or not we actually are already under NOFS.  If someone actually
> > has audited this code well enough to know for sure then yes let's change
> > the comment, but I haven't gone that far.

Ugh, /me hands himself another cup of coffee...

Somehow I mixed up _xfs_buf_map_pages and kmem_zalloc_large in this
discussion.  Probably because they have similar code snippets with very
similar comments to two totally different parts of xfs.

The _xfs_buf_map_pages can be called inside or outside of
transaction context, so I think we still have to memalloc_nofs_save for
that to avoid the lockdep complaints and deadlocks referenced in the
commit that added all that (to _xfs_buf_map_pages) in the first place.
ae687e58b3 ("xfs: use NOIO contexts for vm_map_ram")

My comments about kmem_zalloc_large still stand, even though the part
of the patch you two were discussing was the _xfs_buf_map_pages.  I
probably should have clarified that I think both functions actually
/are/ doing the right thing wrt calling (or not calling)
memalloc_nofs_save().

> I believe we can drop the memalloc_nofs_save then as well because either
> we are called from a potentially dangerous context and thus we are in
> the nofs scope we we do not need the protection at all.

Uh, now that I've muddied up the waters, which part are you referring to?

--D

> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html