linux-kernel - Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

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: <CAFqt6zZoFBkvkZKOc9mbGVO6_Gst+0k=wpjmXYSKvFru=HgczA@mail.gmail.com>
Date:   Tue, 19 May 2020 02:36:45 +0530
From:   Souptick Joarder <jrdr.linux@...il.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing
 write flag

On Tue, May 19, 2020 at 2:15 AM John Hubbard <jhubbard@...dia.com> wrote:
>
> On 2020-05-18 13:44, Souptick Joarder wrote:
> > On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@...radead.org> wrote:
> >>
> >> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> >>> The idea is to get rid of write parameter. Instead caller will pass
> >>> FOLL_WRITE to __get_user_pages_fast(). This will not change any
> >>> functionality of the API. Once it is upstream all the callers will
> >>> be changed to pass FOLL_WRITE.
> >>
> >> Uhh ... until you change all the callers, haven't you just broken all
> >> the callers?
> >
> > All the callers have called the API with either 1 or 0.  I think, it's
> > not going to break
> > any of the callers.
>
> Maybe so, but it's still "wrong" to do that, because it only works more
> or less accidentally. That is, it works in spite of a type safety
> violation. So we don't want to do that sort of thing unless there is
> a compelling reason.
>
> In addition to that, I am at the exact moment putting together a minor
> refactoring of this function, because I need a FOLL_PIN variant:
> __pin_user_pages_fast(), as part of my work to change over a few dozen
> gup call sites to pin_user_pages*().
>
> (In fact, I was wondering whether to stick with the "write" parameter, for
> the new __pin_user_pages_fast(), or go with gup_flags. That could go either
> way: gup_flags provides a nicer API, but "write" matches the existing
> callers.)
>
> So in other words, if you do go out and change all the call sites (there only
> seem to be about 7, outside of gup.c, actually), that's going to conflict
> a little bit with what I'm doing here.
>
> So, how would you like proceed? If you want to do the full conversion
> (which really should include the call sites), it would be easier for me
> if you based it on my upcoming small patchset, which I expect to post
> shortly (later today).

Sure, I will wait for your patchset :)

As there are only 7 callers of the __get_user_pages_fast(), I prefer to do
full conversion in a single commit. But if it is not preferred way, I would go
as per feedback.

> >>
> >>> -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>> -                       struct page **pages)
> >>> +int __get_user_pages_fast(unsigned long start, int nr_pages,
> >>> +                     unsigned int gup_flags, struct page **pages)
> >>>   {
> >>>        unsigned long len, end;
> >>>        unsigned long flags;
> >>> @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>>         * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> >>>         * because gup fast is always a "pin with a +1 page refcount" request.
> >>>         */
> >>> -     unsigned int gup_flags = FOLL_GET;
> >>> -
> >>> -     if (write)
> >>> -             gup_flags |= FOLL_WRITE;
> >>> +     gup_flags |= FOLL_GET;
> >
>