On 9/4/23, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sun, 3 Sept 2023 at 23:03, Mateusz Guzik <mjguzik@xxxxxxxxx> 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>
