linux-kernel - Re: [git pull] vfs.git get_user_pages

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:   Fri, 17 Nov 2017 12:50:47 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [git pull] vfs.git get_user_pages_fast() conversion

On Thu, Nov 16, 2017 at 7:02 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>         A bunch of places switched to get_user_pages_fast().

I really would have liked a bit of a commentary.

Looking at the individual patches, I notice this, for example:

-               down_read(&current->mm->mmap_sem);
-               page_nr = get_user_pages((unsigned long)userptr,
-                                        (int)(bo->pgnr), 1, pages, NULL);
-               up_read(&current->mm->mmap_sem);
+               page_nr = get_user_pages_fast((unsigned long)userptr,
+                                        (int)(bo->pgnr), 1, pages);

from the atomisp conversion, and it made me throw up my hands in horror.

Not because the conversion was wrong, but because the original code is
so broken.

In particular, that "1" that is unchanged in the arguments is correct
in the conversion, but it was completely wrong in the original, even
if it happened to work.

it _should_ have been a FOLL_WRITE. Yes, it happens to have that
value, but it was broken.

(I note that a bit of grepping shows we have the same issue in a stale
comment in mm/ksm.c).

It would have been nice to see things like this mentioned in the commit message.

Because I'm pretty sure you actually _realized_ that as you made the
conversion, but there's no sign of that in the logs, because the
commit message just says

    atomisp: use get_user_pages_fast()

without mentioning how broken the old case was (even it if happened to work).

                   Linus