linux-kernel - Re: [RFC PATCH 1/7] mm/gup: introduce pin_user

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:   Tue, 1 Mar 2022 00:40:47 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     David Hildenbrand <david@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Jan Kara <jack@...e.cz>, Christoph Hellwig <hch@...radead.org>,
        Dave Chinner <dchinner@...hat.com>,
        "Darrick J . Wong" <djwong@...nel.org>,
        Theodore Ts'o <tytso@....edu>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Miklos Szeredi <miklos@...redi.hu>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Chaitanya Kulkarni <kch@...dia.com>
Cc:     linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-mm@...ck.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/7] mm/gup: introduce pin_user_page()

On 3/1/22 00:11, David Hildenbrand wrote:
>> ...
>>>> +EXPORT_SYMBOL(pin_user_page);
>>>> +
>>>>    /*
>>>>     * pin_user_pages_unlocked() is the FOLL_PIN variant of
>>>>     * get_user_pages_unlocked(). Behavior is the same, except that this one sets
>>>
>>> I assume that function will only get called on a page that has been
>>> obtained by a previous pin_user_pages_fast(), correct?
>>>
>>
>> Well, no. This is meant to be used in place of get_page(), for code that
>> knows that the pages will be released via unpin_user_page(). So there is
>> no special prerequisite there.
> 
> That might be problematic and possibly the wrong approach, depending on
> *what* we're actually pinning and what we're intending to do with that.
> 
> My assumption would have been that this interface is to duplicate a pin

I see that I need to put more documentation here, so people don't have
to assume things... :)

> on a page, which would be perfectly fine, because the page actually saw
> a FOLL_PIN previously.
> 
> We're taking a pin on a page that we haven't obtained via FOLL_PIN if I
> understand correctly. Which raises the questions, how do we end up with
> the pages here, and what are we doing to do with them (use them like we
> obtained them via FOLL_PIN?)?
> 
> 
> If it's converting FOLL_GET -> FOLL_PIN manually, then we're bypassing
> FOLL_PIN special handling in GUP code:
> 
> page = get_user_pages(FOLL_GET)
> pin_user_page(page)
> put_page(page)

No, that's not where this is going at all. The idea, which  I now see
needs better documentation, is to handle file-backed pages. Only.

We're not converting from one type to another, nor are we doubling up.
We're just keeping the pin type consistent so that the vast block-
processing machinery can take pages in and handle them, then release
them at the end with bio_release_pages(), which will call
unpin_user_pages().

> 
> 
> For anonymous pages, we'll bail out for example once we have
> 
> https://lkml.kernel.org/r/20220224122614.94921-14-david@redhat.com
> 
> Because the conditions for pinned anonymous pages might no longer hold.
> 
> If we won't call pin_user_page() on anonymous pages, it would be fine.

We won't, and in fact, I should add WARN_ON_ONCE(PageAnon(page)) to
this function.

> But then, I still wonder how we come up the "struct page" here.
> 

 From the file system. For example, the NFS-direct and fuse conversions
in the last patches show how that works.

Thanks for this feedback, this is very helpful.

thanks,
-- 
John Hubbard
NVIDIA