linux-kernel - Re: [PATCH 4/6] mm: introduce page->dma_pinned_flags,

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: <20181013230124.GB18822@dastard>
Date:   Sun, 14 Oct 2018 10:01:24 +1100
From:   Dave Chinner <david@...morbit.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...nel.org>,
        Christopher Lameter <cl@...ux.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Dan Williams <dan.j.williams@...el.com>,
        Jan Kara <jack@...e.cz>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 4/6] mm: introduce page->dma_pinned_flags, _count

On Sat, Oct 13, 2018 at 12:34:12AM -0700, John Hubbard wrote:
> On 10/12/18 8:55 PM, Dave Chinner wrote:
> > On Thu, Oct 11, 2018 at 11:00:12PM -0700, john.hubbard@...il.com wrote:
> >> From: John Hubbard <jhubbard@...dia.com>
> [...]
> >> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> >> index 5ed8f6292a53..017ab82e36ca 100644
> >> --- a/include/linux/mm_types.h
> >> +++ b/include/linux/mm_types.h
> >> @@ -78,12 +78,22 @@ struct page {
> >>  	 */
> >>  	union {
> >>  		struct {	/* Page cache and anonymous pages */
> >> -			/**
> >> -			 * @lru: Pageout list, eg. active_list protected by
> >> -			 * zone_lru_lock.  Sometimes used as a generic list
> >> -			 * by the page owner.
> >> -			 */
> >> -			struct list_head lru;
> >> +			union {
> >> +				/**
> >> +				 * @lru: Pageout list, eg. active_list protected
> >> +				 * by zone_lru_lock.  Sometimes used as a
> >> +				 * generic list by the page owner.
> >> +				 */
> >> +				struct list_head lru;
> >> +				/* Used by get_user_pages*(). Pages may not be
> >> +				 * on an LRU while these dma_pinned_* fields
> >> +				 * are in use.
> >> +				 */
> >> +				struct {
> >> +					unsigned long dma_pinned_flags;
> >> +					atomic_t      dma_pinned_count;
> >> +				};
> >> +			};
> > 
> > Isn't this broken for mapped file-backed pages? i.e. they may be
> > passed as the user buffer to read/write direct IO and so the pages
> > passed to gup will be on the active/inactive LRUs. hence I can't see
> > how you can have dual use of the LRU list head like this....
> > 
> > What am I missing here?
> 
> Hi Dave,
> 
> In patch 6/6, pin_page_for_dma(), which is called at the end of get_user_pages(),
> unceremoniously rips the pages out of the LRU, as a prerequisite to using
> either of the page->dma_pinned_* fields. 

How is that safe? If you've ripped the page out of the LRU, it's no
longer being tracked by the page cache aging and reclaim algorithms.
Patch 6 doesn't appear to put these pages back in the LRU, either,
so it looks to me like this just dumps them on the ground after the
gup reference is dropped.  How do we reclaim these page cache pages
when there is memory pressure if they aren't in the LRU?

> The idea is that LRU is not especially useful for this situation anyway,
> so we'll just make it one or the other: either a page is dma-pinned, and
> just hanging out doing RDMA most likely (and LRU is less meaningful during that
> time), or it's possibly on an LRU list.

gup isn't just used for RDMA. It's used by direct IO in far, far
more situations and machines than RDMA is. Please explain why
ripping pages out of the LRU and not putting them back is safe, has
no side effects, doesn't adversely impact page cache reclaim, etc.
Indeed, I'd love to see a description of all the page references and
where they come and go so we know the changes aren't just leaking
these pages until the filesystem invalidates them at unmount.

Maybe I'm not seeing why this is safe yet, but seeing as you haven't
explained why it is safe then, at minimum, the patch descriptions
are incomplete.

Cheers,

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