linux-kernel - Re: [PATCH 4/4] hugetlb: Do early cow when page pinned on src mm

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=whGvKx2aDhrW4K-PZix+Yt2L=dcv1meQWBiDFrW7ZJ63A@mail.gmail.com>
Date:   Wed, 3 Feb 2021 13:15:03 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Xu <peterx@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>, Wei Zhang <wzam@...zon.com>,
        Matthew Wilcox <willy@...radead.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Gal Pressman <galpress@...zon.com>,
        Christoph Hellwig <hch@....de>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Jan Kara <jack@...e.cz>,
        Kirill Shutemov <kirill@...temov.name>,
        David Gibson <david@...son.dropbear.id.au>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Jann Horn <jannh@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 4/4] hugetlb: Do early cow when page pinned on src mm

On Wed, Feb 3, 2021 at 1:08 PM Peter Xu <peterx@...hat.com> wrote:
>
> This is the last missing piece of the COW-during-fork effort when there're
> pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
> pinned pages during fork() for ptes", 2020-09-27) for more information, since
> we do similar things here rather than pte this time, but just for hugetlb.

No issues with the code itself, but..

Comments are good, but the comments inside this block of code actually
makes the code *much* harder to read, because now the actual logic is
much more spread out and you can't see what it does so well.

> +                       if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
> +                               /* This is very possibly a pinned huge page */
> +                               if (!prealloc) {
> +                                       /*
> +                                        * Preallocate the huge page without
> +                                        * tons of locks since we could sleep.
> +                                        * Note: we can't use any reservation
> +                                        * because the page will be exclusively
> +                                        * owned by the child later.
> +                                        */
> +                                       put_page(ptepage);
> +                                       spin_unlock(src_ptl);
> +                                       spin_unlock(dst_ptl);
> +                                       prealloc = alloc_huge_page(vma, addr, 0);
> +                                       if (!prealloc) {
> +                                               /*
> +                                                * hugetlb_cow() seems to be
> +                                                * more careful here than us.
> +                                                * However for fork() we could
> +                                                * be strict not only because
> +                                                * no one should be referencing
> +                                                * the child mm yet, but also
> +                                                * if resources are rare we'd
> +                                                * better simply fail the
> +                                                * fork() even earlier.
> +                                                */
> +                                               ret = -ENOMEM;
> +                                               break;
> +                                       }
> +                                       goto again;
> +                               }
> +                               /*
> +                                * We have page preallocated so that we can do
> +                                * the copy right now.
> +                                */
> +                               hugetlb_copy_page(vma, dst_pte, addr, ptepage,
> +                                                 prealloc);
> +                               put_page(ptepage);
> +                               spin_unlock(src_ptl);
> +                               spin_unlock(dst_ptl);
> +                               prealloc = NULL;
> +                               continue;
> +                       }

Can you move the comment above the code? And I _think_ the prealloc
conditional could be split up to a helper function (which would help
more), but maybe there are too many variables for that to be
practical.

           Linus