linux-kernel - Re: [PATCH] x86/vdso: Change return type to vm_fault

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

Message-ID: <CALCETrUTeVfj7n29AHK6Pa=caqmZOwqtZrBNawXKa_GBvCw8XQ@mail.gmail.com>
Date:   Mon, 25 Jun 2018 09:39:47 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     jrdr.linux@...il.com
Cc:     Matthew Wilcox <willy@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>, brajeswar.linux@...il.com,
        sabyasachi.linux@...il.com
Subject: Re: [PATCH] x86/vdso: Change return type to vm_fault_t for fault handlers

On Mon, Jun 25, 2018 at 9:15 AM Souptick Joarder <jrdr.linux@...il.com> wrote:
>
> On Mon, Jun 25, 2018 at 9:41 PM, Andy Lutomirski <luto@...capital.net> wrote:
> > On Mon, Jun 25, 2018 at 8:55 AM Souptick Joarder <jrdr.linux@...il.com> wrote:
> >>
> >> Use new return type vm_fault_t for fault handler. For
> >> now, this is just documenting that the function returns
> >> a VM_FAULT value rather than an errno. Once all instances
> >> are converted, vm_fault_t will become a distinct type.
> >
> > Whoa there..  Your commit message makes it sound like you're just
> > changing the return type, but:
> >
> >>                 if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
> >> -                       ret = vm_insert_pfn(vma, vmf->address,
> >> +                       ret = vmf_insert_pfn(vma, vmf->address,
> >>                                             vmalloc_to_pfn(tsc_pg));
> >>         }
> >>
> >> -       if (ret == 0 || ret == -EBUSY)
> >> -               return VM_FAULT_NOPAGE;
> >> -
> >> -       return VM_FAULT_SIGBUS;
> >> +       return ret;
> >
> > You're refactoring the code, too.
> >
> > Please fix your changelog.
>
>
> I have mentioned it.
>
> ********************
> Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>
> Previously vm_insert_pfn() returns err which has to
> mapped into VM_FAULT_* type. The new function
> vmf_insert_pfn() will replace this inefficiency by
> returning VM_FAULT_* type.
> *********************

The second line of your changelog says "For now, this is just
documenting...".  Please don't make future readers read all the way
through the whole changelog and try to reconcile conflicting sentences
to figure out what the patch does.

I read your changelog, then I read through the diff to make sure that
you were really just changing the return type (as your changelog
seemed to claim), and I decided that your patch was incorrect.  As the
maintainer of this code, this means that you've made my work harder
than it should be, so NAK.  Please improve your changelog and
resubmit.

--Andy