On Thu, 20 Jun 2024 at 00:53, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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));
> }
Oh, I'd be ok with that.
That "access_ok()" thing was actually the first thing I did, before
doing all the asm goto fixes and making the arm64 "unsafe" user access
functions work. I may have gone a bit overboard when compensating for
all the other crap the generated code had.
That said, the size check really is of dubious value, and the bit
games did make the code nice and efficient.
But yeah, maybe I made it a bit *too* subtle in the process.
> 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().").
Hmm. That _is_ ugly. But I do think that the untagging is very much a
per-thread state (well, it *should* be per-VM, but that is a whole
other discussion), and so the rationale for _why_ that code doesn't do
untagging is still very very true.
Yes, the x86 code no longer has a WARN for that case, but the arm64
code really *would* be horrible broken if the code just untagged based
on random thread data.
Of course, in the end that's just one more reason to consider the
current arm64 tagging model completely broken.
But my point is: copy_from_user_nofault() can be called from random
contexts, and as long as that is the case - and as long as we still
make the untagging some per-thread thing - that code must not do
untagging because it's in the wrong context to actually do that
correctly.
And no, the way the arm64 hardware setup works, none of this matters.
The arm64 untagging really *is* unconditional on a hw setup level,
which took me by surprise.
Linus
