On Wed, Jun 19, 2024, at 22:25, Linus Torvalds wrote:
> The arm64-uaccess branch is just what it says, and makes a big
> difference in strncpy_from_user(). The "access_ok()" change is
> certainly debatable, but I think needs to be done for sanity. I think
> it's one of those "let's do it, and if it causes problems we'll have
> to fix things up" things.
I'm a bit worried about the access_ok() being so different from
the other architectures, after I previously saw all the ways
it could go wrong because of subtle differences.
I don't mind making the bit that makes the untagging
unconditional, and I can see how this improves code
generation. I've tried comparing your version against
the more conventional
static inline int access_ok(const void __user *p, unsigned long size)
{
return likely(__access_ok(untagged_addr(p), size));
}
Using gcc-13.2, I see your version is likely better in all
cases, but not by much: for the constant-length case, it
saves only one instruction (combining the untagging with the
limit), while for a variable length it avoids a branch.
On a 24MB kernel image, I see this add up to a size difference
of 12KB, while the total savings from avoiding the conditional
untagging are 76KB.
Do you see a measurable performance difference between your
version and the one above?
On a related note, I see that there is one caller of
__access_ok() in common code, and this was added in
d319f344561d ("mm: Fix copy_from_user_nofault().").
I think that one should just go back to using access_ok()
after your 6ccdc91d6af9 ("x86: mm: remove
architecture-specific 'access_ok()' define"). In the
current version, it otherwise misses the untagging
on arm64.
Arnd
