linux-kernel - Re: [PATCH 5/7] kcov: add ioctl(KCOV_UNIQUE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG_fn=WD3ZuJCQ4TiVKXLhn5-=tsaW0d=zrM-TuEokP5zEvOSw@mail.gmail.com>
Date: Wed, 23 Apr 2025 15:08:36 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Marco Elver <elver@...gle.com>
Cc: quic_jiangenj@...cinc.com, linux-kernel@...r.kernel.org, 
	kasan-dev@...glegroups.com, Aleksandr Nogikh <nogikh@...gle.com>, 
	Andrey Konovalov <andreyknvl@...il.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Ingo Molnar <mingo@...hat.com>, Josh Poimboeuf <jpoimboe@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 5/7] kcov: add ioctl(KCOV_UNIQUE_ENABLE)

On Tue, Apr 22, 2025 at 11:29 AM Marco Elver <elver@...gle.com> wrote:
>
> On Wed, 16 Apr 2025 at 10:55, Alexander Potapenko <glider@...gle.com> wrote:
> >
> > ioctl(KCOV_UNIQUE_ENABLE) enables collection of deduplicated coverage
> > in the presence of CONFIG_KCOV_ENABLE_GUARDS.
> >
> > The buffer shared with the userspace is divided in two parts, one holding
> > a bitmap, and the other one being the trace. The single parameter of
> > ioctl(KCOV_UNIQUE_ENABLE) determines the number of words used for the
> > bitmap.
> >
> > Each __sanitizer_cov_trace_pc_guard() instrumentation hook receives a
> > pointer to a unique guard variable. Upon the first call of each hook,
> > the guard variable is initialized with a unique integer, which is used to
> > map those hooks to bits in the bitmap. In the new coverage collection mode,
> > the kernel first checks whether the bit corresponding to a particular hook
> > is set, and then, if it is not, the PC is written into the trace buffer,
> > and the bit is set.
> >
> > Note: when CONFIG_KCOV_ENABLE_GUARDS is disabled, ioctl(KCOV_UNIQUE_ENABLE)
> > returns -ENOTSUPP, which is consistent with the existing kcov code.
> >
> > Also update the documentation.
>
> Do you have performance measurements (old vs. new mode) that can be
> included in this commit description?

That's hard to measure.
According to the latest measurements (50 instances x 24h with and
without deduplication), if we normalize by pure fuzzing time, exec
total goes down by 2.1% with p=0.01.
On the other hand, if we normalize by fuzzer uptime, the reduction is
statistically insignificant (-1.0% with p=0.20)
In both cases, we observe a statistically significant (p<0.01)
increase in corpus size (+0.6%) and coverage (+0.6) and -99.8%
reduction in coverage overflows.

So while there might be a slight slowdown introduced by this patch
series, it still positively impacts fuzzing.
I can add something along these lines to the commit description.


> > +.. code-block:: c
> > +
> > +       /* Same includes and defines as above. */
> > +       #define KCOV_UNIQUE_ENABLE              _IOW('c', 103, unsigned long)
>
> Here it's _IOW.
>
...
> > diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> > index ed95dba9fa37e..fe1695ddf8a06 100644
> > --- a/include/uapi/linux/kcov.h
> > +++ b/include/uapi/linux/kcov.h
> > @@ -22,6 +22,7 @@ struct kcov_remote_arg {
> >  #define KCOV_ENABLE                    _IO('c', 100)
> >  #define KCOV_DISABLE                   _IO('c', 101)
> >  #define KCOV_REMOTE_ENABLE             _IOW('c', 102, struct kcov_remote_arg)
> > +#define KCOV_UNIQUE_ENABLE             _IOR('c', 103, unsigned long)
>
> _IOR? The unsigned long arg is copied to the kernel, so this should be
> _IOW, right?

Right, thanks for spotting!
This also suggests our declaration of KCOV_INIT_TRACE is incorrect
(should also be _IOW), but I don't think we can do much about that
now.

> >  void notrace __sanitizer_cov_trace_pc_guard(u32 *guard)
> >  {
> > -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, current))
> > -               return;
> > +       u32 pc_index;
> > +       enum kcov_mode mode = get_kcov_mode(current);
> >
> > -       sanitizer_cov_write_subsequent(current->kcov_state.s.trace,
> > -                                      current->kcov_state.s.trace_size,
> > -                                      canonicalize_ip(_RET_IP_));
> > +       switch (mode) {
> > +       case KCOV_MODE_TRACE_UNIQUE_PC:
> > +               pc_index = READ_ONCE(*guard);
> > +               if (unlikely(!pc_index))
> > +                       pc_index = init_pc_guard(guard);
>
> This is an unlikely branch, yet init_pc_guard is __always_inline. Can
> we somehow make it noinline? I know objtool will complain, but besides
> the cosmetic issues, doing noinline and just giving it a better name
> ("kcov_init_pc_guard") and adding that to objtool whilelist will be
> better for codegen.

I don't expect it to have a big impact on the performance, but let's
check it out.