linux-kernel - Re: [PATCH v2] x86: bring back rep movsq for user access on CPUs without ERMS

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: <CAGudoHEXyYSZj-7=3Xss=65jGyX4Lq=R-BdbnmGKJwSS8QznSw@mail.gmail.com>
Date:   Tue, 5 Sep 2023 22:41:33 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, bp@...en8.de
Subject: Re: [PATCH v2] x86: bring back rep movsq for user access on CPUs
 without ERMS

On 9/4/23, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Sun, 3 Sept 2023 at 23:03, Mateusz Guzik <mjguzik@...il.com> wrote:
>>
>> Worst case if the 64 bit structs differ one can settle for
>> user-accessing INIT_STRUCT_STAT_PADDING.
>
> As I said, try it. I think you'll find that you are wrong.  It's
> _hard_ to get the padding right. The "use a temporary" model of the
> current code makes the fallback easy - just clear it before copying
> the fields. Without that, you have to get every architecture padding
> right manually.
>

See below for an unrelated patch. ;)

In the worst case user-accessing INIT_STRUCT_STAT_PADDING would simply
have these padding assignments spelled out, like you had to do anyway
in your patch (among other things) and which already happens with
existing code for x86-64. The code would not be fully optimized due to
"bad" ordering, but the implementation would avoid escaping fs/stat.c
which imo justifies it.

I said my $0,03 on the matter twice, I don't think this convo is
getting anywhere or that it is particularly important.

With that out of the way:

Earlier you attached a patch to check for an empty path early, that
was a nice win but can result in duplicated smap trips for nasty
software.

Then you mentioned doing the check after the name is fully copied in,
which I incorrectly dismissed from the get go -- mentally I had a
state from $elsewhere where it would require patching namei. But in
Linux this is not the case:

        name = getname_flags(filename,
getname_statx_lookup_flags(statx_flags), NULL);
        ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
        putname(name);

So something like this:

@@ -312,7 +314,15 @@ int vfs_fstatat(int dfd, const char __user *filename,
        struct filename *name;

        name = getname_flags(filename,
getname_statx_lookup_flags(statx_flags), NULL);
-       ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
+       /*
+        * Hack: ugliness below damage controls glibc which reimplemented fstat
+        * on top of newfstatat(fd, "", buf, AT_EMPTY_PATH). We still pay for
+        * kmalloc and user access, but elide lockref.
+        */
+       if (name->name[0] == '\0' && flags == AT_EMPTY_PATH && dfd >= 0)
+               ret = vfs_fstat(dfd, stat);
+       else
+               ret = vfs_statx(dfd, name, statx_flags, stat,
STATX_BASIC_STATS);
        putname(name);

        return ret;

Previous numbers:
> stock fstat     5088199
> patched fstat   7625244 (+49%)
> real fstat      8540383 (+67% / +12%)

while moving the check lower:
patchedv2 fstat 6253694 (+22%)

So quite a bit slower than the hackiest variant, but avoids the double
smap problem for cases passing AT_EMPTY_PATH and a non-"" name.

I think this is a justifiable patch, can submit properly formatted
(this is copy pasted into gmail, sorry). Alternatively since it is
trivial and it was your idea feel free to take it or reimpement or
whatever in similar vein.

-- 
Mateusz Guzik <mjguzik gmail.com>