Date:   Mon, 13 Dec 2021 23:07:42 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Peter Collingbourne' <pcc@...gle.com>
CC:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Juri Lelli" <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        "Thomas Gleixner" <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Masahiro Yamada" <masahiroy@...nel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        YiFei Zhu <yifeifz2@...inois.edu>,
        Mark Rutland <mark.rutland@....com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Andrey Konovalov <andreyknvl@...il.com>,
        "Gabriel Krisman Bertazi" <krisman@...labora.com>,
        Chris Hyser <chris.hyser@...cle.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        "Chris Wilson" <chris@...is-wilson.co.uk>,
        Arnd Bergmann <arnd@...db.de>,
        "Dmitry Vyukov" <dvyukov@...gle.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Alexey Gladkov <legion@...nel.org>,
        Ran Xiaokai <ran.xiaokai@....com.cn>,
        David Hildenbrand <david@...hat.com>,
        Xiaofeng Cao <caoxiaofeng@...ong.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Thomas Cedeno <thomascedeno@...gle.com>,
        Marco Elver <elver@...gle.com>,
        "Alexander Potapenko" <glider@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Evgenii Stepanov <eugenis@...gle.com>
Subject: RE: [PATCH v4 0/7] kernel: introduce uaccess logging

From: Peter Collingbourne
> Sent: 13 December 2021 19:49
> 
> On Sat, Dec 11, 2021 at 9:23 AM David Laight <David.Laight@...lab.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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists