Re: [PATCH v4 0/7] kernel: introduce uaccess logging

On Mon, Dec 13, 2021 at 7:47 PM Peter Collingbourne [off-list ref] wrote:

On Mon, Dec 13, 2021 at 3:07 PM David Laight [off-list ref] wrote:

quoted

From: Peter Collingbourne

quoted

Sent: 13 December 2021 19:49

On Sat, Dec 11, 2021 at 9:23 AM David Laight [off-list ref] wrote:

quoted

From: Peter Collingbourne

quoted

Sent: 09 December 2021 22:16

This patch series introduces a kernel feature known as uaccess
logging, which allows userspace programs to be made aware of the
address and size of uaccesses performed by the kernel during
the servicing of a syscall. More details on the motivation
for and interface to this feature are available in the file
Documentation/admin-guide/uaccess-logging.rst added by the final
patch in the series.

How does this work when get_user() and put_user() are used to
do optimised copies?

While adding checks to copy_to/from_user() is going to have
a measurable performance impact - even if nothing is done,
adding them to get/put_user() (and friends) is going to
make some hot paths really slow.

So maybe you could add it so KASAN test kernels, but you can't
sensibly enable it on a production kernel.

Now, it might be that you could semi-sensibly log 'data' transfers.
But have you actually looked at all the transfers that happen
for something like sendmsg().
The 'user copy hardening' code already has a significant impact
on that code (in many places).

Hi David,

Yes, I realised after I sent out my patch (and while writing test
cases for it) that it didn't cover get_user()/put_user(). I have a
patch under development that will add this coverage. I used it to run
my invalid syscall and uname benchmarks and the results were basically
the same as without the coverage.

Are you aware of any benchmarks that cover sendmsg()? I can try to
look at writing my own if not. I was also planning to write a
benchmark that uses getresuid() as this was the simplest syscall that
I could find that does multiple put_user() calls.

Also look at sys_poll() I think that uses __put/get_user().

I think you'll find some of the socket option code also uses get_user().

There is also the compat code for import_iovec().
IIRC that is actually faster than the non-compat version at the moment.

I did some benchmarking of writev("/dev/null", iov, 10);
The cost of reading in the iovec is significant in that case.
Maybe I ought to find time to sort out my patches.

For sendmsg() using __copy_from_user() to avoid the user-copy
hardening checks also makes a measurable difference when sending UDP
through raw sockets - which we do a lot of.

I think you'd need to instrument user_access_begin() and also be able
to merge trace entries (for multiple get_user() calls).

You really don't have to look far to find places where copy_to/from_user()
is optimised to multiple get/put_user() or __get/put_user() (or are they
the 'nofault' variants?)
Those are all hot paths - at least for some workloads.
So adding anything there isn't likely to be accepted for production kernels.

Okay, but let's see what the benchmarks say first.

I added calls to uaccess_buffer_log_{read,write}() to
__{get,put}_user() in arch/arm64/include/asm/uaccess.h and wrote a
variant of my usual test program that does getresuid() in a loop and
measured an overhead of 5.9% on the small cores and 2.5% on the big
cores. The overhead appears to come from two sources:
1) The calling convention for the call to
__uaccess_buffer_log_read/write() transforms some leaf functions into
non-leaf functions, and as a result we end up spilling more registers.
2) We need to reload the flags from the task_struct every time to
determine the feature enabled state.
I think it should be possible to reduce the cost down to one
instruction (a conditional branch) per get/put_user() call, at least
on arm64, by using a different calling convention for the call and
maintaining the feature enabled state in one of the currently-ignored
bits 60-63 of a reserved register (x18).

I have the patch below which reduces the overhead with my test program
to 2.3% small 1.5% big (not fully functional yet because the rest of
the code never actually flips that bit in x18). For syscalls like
sendmsg() and poll() I would expect the overall overhead to be lower
because of other required work done by these syscalls (glancing
through their implementations this appears to be the case). But I'll
benchmark it of course. The preserve_most attribute is Clang-only but
I think it should be possible to use inline asm to get the same effect
with GCC. We currently only reserve x18 with CONFIG_SHADOW_CALL_STACK
enabled but I was unable to measure an overhead for getresuid() when
reserving x18 unconditionally.

I developed benchmarks for sendmsg() and poll() and, as I expected,
the performance overhead of my patch was in the noise, except that I
measured 1.0% overhead for poll(), but only on the little cores. So it
looks like getresuid() still represents the worst case. I'm not sure
it's worth spending time developing additional benchmarks because I
would expect them to show similar results.

By the way, is there any interest in maintaining my syscall latency
benchmarks in tree? They have been invaluable for measuring the
performance impact of my kernel changes. The benchmarks that I
developed are written in arm64 assembly to avoid measurement error due
to overheads introduced by the libc syscall wrappers (i.e. they make
my benchmark results look as bad as possible, by spending as little
time in userspace as possible). I looked around in the tree and found
"perf bench syscall" but that uses the libc wrappers.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel