linux-kernel - Re: [PATCHv7 2/3] mm/gup: fix omission of check on FOLL

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>] [day] [month] [year] [list]

Message-ID: <CAFgQCTt8KpzOTrLfiSLQhy1kOo10-fxdVD+WQky_ynQoqun+AQ@mail.gmail.com>
Date:   Fri, 20 Mar 2020 17:19:46 +0800
From:   Pingfan Liu <kernelfans@...il.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Linux-MM <linux-mm@...ck.org>, Ira Weiny <ira.weiny@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Christoph Hellwig <hch@...radead.org>,
        Shuah Khan <shuah@...nel.org>, Jason Gunthorpe <jgg@...pe.ca>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv7 2/3] mm/gup: fix omission of check on FOLL_LONGTERM in
 gup fast path

On Fri, Mar 20, 2020 at 6:17 AM John Hubbard <jhubbard@...dia.com> wrote:
>
> On 3/17/20 4:47 AM, Pingfan Liu wrote:
> > FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is
> > going to be given to hardware and can't move. It would truncate CMA
> > permanently and should be excluded.
> >
> > In gup slow path, slow path, where
>
>
> s/slow path, slow path/slow path/
Yeah.
[...]
> >
> >               /*
> > +              * Huge page's subpages have the same migrate type due to either
> > +              * allocation from a free_list[] or alloc_contig_range() with
> > +              * param MIGRATE_MOVABLE. So it is enough to check on a subpage.
> > +              */
>
> Urggh, this comment is fine in the commit description, but at this location in the
> code it is completely incomprehensible! Instead of an extremely far-removed tidbit about
> interactions between CMA and huge pages, this comment should be explaining why we bail
> out early in the specific case of FOLL_PIN + FOLL_LONGTERM. And we don't bail out for
> FOLL_GET + FOLL_LONGTERM...
>
>
> I'm expect it is something like:
>
>                 /*
>                  * We can't do FOLL_LONGTERM + FOLL_PIN with CMA in the gup fast
>                  * path, so fail and let the caller fall back to the slow path.
>                  */
>
>
> ...approximately. Right?
Yes, right. And I think it is better to drop "We".

Thanks,
Pingfan