Re: [PATCH 1/5] include/linux: Add instrumented.h infrastructure

[Marco Elver <elver@google.com>]

On Mon, 20 Jan 2020 at 17:06, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Jan 20, 2020 at 4:40 PM Marco Elver <elver@google.com> wrote:
> > > > > > This adds instrumented.h, which provides generic wrappers for memory
> > > > > > access instrumentation that the compiler cannot emit for various
> > > > > > sanitizers. Currently this unifies KASAN and KCSAN instrumentation. In
> > > > > > future this will also include KMSAN instrumentation.
> > > > > >
> > > > > > Note that, copy_{to,from}_user require special instrumentation,
> > > > > > providing hooks before and after the access, since we may need to know
> > > > > > the actual bytes accessed (currently this is relevant for KCSAN, and is
> > > > > > also relevant in future for KMSAN).
> > > > > >
> > > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > > > > Signed-off-by: Marco Elver <elver@google.com>
> > > > > > ---
> > > > > >  include/linux/instrumented.h | 153 +++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 153 insertions(+)
> > > > > >  create mode 100644 include/linux/instrumented.h
> > > > > >
> > > > > > diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..9f83c8520223
> > > > > > --- /dev/null
> > > > > > +++ b/include/linux/instrumented.h
> > > > > > @@ -0,0 +1,153 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +
> > > > > > +/*
> > > > > > + * This header provides generic wrappers for memory access instrumentation that
> > > > > > + * the compiler cannot emit for: KASAN, KCSAN.
> > > > > > + */
> > > > > > +#ifndef _LINUX_INSTRUMENTED_H
> > > > > > +#define _LINUX_INSTRUMENTED_H
> > > > > > +
> > > > > > +#include <linux/compiler.h>
> > > > > > +#include <linux/kasan-checks.h>
> > > > > > +#include <linux/kcsan-checks.h>
> > > > > > +#include <linux/types.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * instrument_read - instrument regular read access
> > > > > > + *
> > > > > > + * Instrument a regular read access. The instrumentation should be inserted
> > > > > > + * before the actual read happens.
> > > > > > + *
> > > > > > + * @ptr address of access
> > > > > > + * @size size of access
> > > > > > + */
> > > > >
> > > > > Based on offline discussion, that's what we add for KMSAN:
> > > > >
> > > > > > +static __always_inline void instrument_read(const volatile void *v, size_t size)
> > > > > > +{
> > > > > > +       kasan_check_read(v, size);
> > > > > > +       kcsan_check_read(v, size);
> > > > >
> > > > > KMSAN: nothing
> > > >
> > > > KMSAN also has instrumentation in
> > > > copy_to_user_page/copy_from_user_page. Do we need to do anything for
> > > > KASAN/KCSAN for these functions?
> >
> > copy_to_user_page/copy_from_user_page can be instrumented with
> > instrument_copy_{to,from}_user_. I prefer keeping this series with no
> > functional change intended for KASAN at least.
> >
> > > There is also copy_user_highpage.
> > >
> > > And ioread/write8/16/32_rep: do we need any instrumentation there. It
> > > seems we want both KSAN and KCSAN too. One may argue that KCSAN
> > > instrumentation there is to super critical at this point, but KASAN
> > > instrumentation is important, if anything to prevent silent memory
> > > corruptions. How do we instrument there? I don't see how it maps to
> > > any of the existing instrumentation functions.
> >
> > These should be able to use the regular instrument_{read,write}. I
> > prefer keeping this series with no functional change intended for
> > KASAN at least.
>
> instrument_{read,write} will not contain any KMSAN instrumentation,
> which means we will effectively remove KMSAN instrumentation, which is
> weird because we instrumented these functions because of KMSAN in the
> first place...
>
> > > There is also kmsan_check_skb/kmsan_handle_dma/kmsan_handle_urb that
> > > does not seem to map to any of the instrumentation functions.
> >
> > For now, I would rather that there are some one-off special
> > instrumentation, like for KMSAN. Coming up with a unified interface
> > here that, without the use-cases even settled, seems hard to justify.
> > Once instrumentation for these have settled, unifying the interface
> > would have better justification.
>
> I would assume they may also require an annotation that checks the
> memory region under all 3 tools and we don't have such annotation
> (same as the previous case and effectively copy_to_user). I would
> expect such annotation will be used in more places once we start
> looking for more opportunities.

Agreed, I'm certainly not against adding these. We may need to
introduce 'instrument_dma_' etc. However, would it be reasonable to do
this in a separate follow-up patch-series, to avoid stalling bitops
instrumentation?  Assuming that the 8 hooks in instrumented.h right
now are reasonable, and such future changes add new hooks, I think
that would be the more pragmatic approach.

Thanks,
-- Marco

>
> > This patch series is merely supposed to introduce instrumented.h and
> > replace the kasan_checks (also implicitly introducing kcsan_checks
> > there), however, with no further functional change intended.
> >
> > I propose that adding entirely new instrumentation for both KASAN and
> > KCSAN, we should send a separate patch-series.
> >
> > Thanks,
> > -- Marco