On Mon, Jan 20, 2020 at 4:40 PM Marco Elver <elver@xxxxxxxxxx> 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@xxxxxxxx>
> > > > > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > > > > ---
> > > > > 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.
> 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
