linux-kernel - Re: [PATCH] userfaultfd/selftests: clean up hugetlb allocation code

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:   Wed, 5 Jan 2022 15:56:13 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Axel Rasmussen <axelrasmussen@...gle.com>
Cc:     Mike Kravetz <mike.kravetz@...cle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>, Peter Xu <peterx@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH] userfaultfd/selftests: clean up hugetlb allocation code

On Tue, 4 Jan 2022 14:35:34 -0800 Axel Rasmussen <axelrasmussen@...gle.com> wrote:

> On Mon, Jan 3, 2022 at 6:17 PM Mike Kravetz <mike.kravetz@...cle.com> wrote:
> >
> > The message for commit f5c73297181c ("userfaultfd/selftests: fix hugetlb
> > area allocations") says there is no need to create a hugetlb file in the
> > non-shared testing case.  However, the commit did not actually change
> > the code to prevent creation of the file.
> >
> > While it is technically true that there is no need to create and use a
> > hugetlb file in the case of non-shared-testing, it is useful.  This is
> > because 'hole punching' of a hugetlb file has the potentially incorrect
> > side effect of also removing pages from private mappings.  The
> > userfaultfd test relies on this side effect for removing pages from the
> > destination buffer during rounds of stress testing.
> >
> > Remove the incomplete code that was added to deal with no hugetlb file.
> > Just keep the code that prevents reserves from being created for the
> > destination area.
> >
> >         *alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> > -                          map_shared ? MAP_SHARED :
> > -                          MAP_PRIVATE | MAP_HUGETLB |
> > +                          (map_shared ? MAP_SHARED : MAP_PRIVATE) |
> > +                          MAP_HUGETLB |
> >                            (*alloc_area == area_src ? 0 : MAP_NORESERVE),
> > -                          huge_fd,
> > -                          *alloc_area == area_src ? 0 : nr_pages * page_size);
> > +                          huge_fd, *alloc_area == area_src ? 0 :
> > +                          nr_pages * page_size);
> 
> Sorry to nitpick, but I think it was slightly more readable when the
> ternary was all on one line.

When you have that many arguments I think it's clearer to put one per
line, viz.

	*alloc_area = mmap(NULL,
			   nr_pages * page_size,
			   PROT_READ | PROT_WRITE,
			   (map_shared ? MAP_SHARED : MAP_PRIVATE) |
			   	MAP_HUGETLB |
			   	(*alloc_area == area_src ? 0 : MAP_NORESERVE),
			   huge_fd,
			   *alloc_area == area_src ? 0 : nr_pages * page_size);


But whatever...