linux-kernel - Re: [PATCH v2 14/31] selftests/mm: uffd

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: <ZDcHnEgCsoDBhFYM@x1n>
Date:   Wed, 12 Apr 2023 15:33:48 -0400
From:   Peter Xu <peterx@...hat.com>
To:     Axel Rasmussen <axelrasmussen@...gle.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        David Hildenbrand <david@...hat.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        Leonardo Bras Soares Passos <lsoaresp@...hat.com>
Subject: Re: [PATCH v2 14/31] selftests/mm: uffd_[un]register()

On Wed, Apr 12, 2023 at 11:20:35AM -0700, Axel Rasmussen wrote:
> > -       uffdio_register.range.start = (unsigned long)addr;
> > -       uffdio_register.range.len = len;
> > -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > +       if (uffd_register(uffd, addr, len, true, false, false)) {
> 
> For what it's worth, I agree with Mike that the booleans here are hard
> to read. It's not clear what "true, false, false" means without going
> to look at the header file, whereas "uffd_register(uffd, addr, len,
> MINOR)" would be immediately clear.
> 
> One solution I've seen outside the kernel is to comment the bools, like:
> 
> uffd_register(..., /*minor=*/true, /*wp=*/false, /*minor=*/false);
> 
> But, then I feel we lose most of the benefit we wanted from switching
> to bools anyway (code length). :)
> 
> I do agree the macro names are unwieldy, and lots of tests already use
> booleans so we'd have to convert from bool -> flag. If it were me, I
> would resolve that by:
> 
> - Define much shorter aliases for these macros in uffd-common.h
> - Consider refactoring callers to not use bools either.
> 
> Then again, I also agree with Mike that it's not a deal breaker, if
> you like this way much more than that alternative we can leave it.

No strong feeling here either.  I kept it just to avoid code churns and
rebases.

I'll see whether I'll need a repost, if so I can go back to use modes.

Thanks,

-- 
Peter Xu