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

[Peter Collingbourne <pcc@google.com>]

On Mon, Dec 13, 2021 at 7:47 PM Peter Collingbourne <pcc@google.com> wrote:
>
> On Mon, Dec 13, 2021 at 3:07 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Peter Collingbourne
> > > Sent: 13 December 2021 19:49
> > >
> > > On Sat, Dec 11, 2021 at 9:23 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Peter Collingbourne
> > > > > 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