Re: [PATCH v2 1/3] kcov: remote coverage support

[Andrey Konovalov]

On Thu, Oct 24, 2019 at 12:22 AM Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 23 Oct 2019 17:24:29 +0200 Andrey Konovalov <andreyknvl@xxxxxxxxxx> wrote:
>
> > This patch adds background thread coverage collection ability to kcov.
> >
> > With KCOV_ENABLE coverage is collected only for syscalls that are issued
> > from the current process. With KCOV_REMOTE_ENABLE it's possible to collect
> > coverage for arbitrary parts of the kernel code, provided that those parts
> > are annotated with kcov_remote_start()/kcov_remote_stop().
> >
> > This allows to collect coverage from two types of kernel background
> > threads: the global ones, that are spawned during kernel boot and are
> > always running (e.g. USB hub_event()); and the local ones, that are
> > spawned when a user interacts with some kernel interface (e.g. vhost
> > workers).
> >
> > To enable collecting coverage from a global background thread, a unique
> > global handle must be assigned and passed to the corresponding
> > kcov_remote_start() call. Then a userspace process can pass a list of such
> > handles to the KCOV_REMOTE_ENABLE ioctl in the handles array field of the
> > kcov_remote_arg struct. This will attach the used kcov device to the code
> > sections, that are referenced by those handles.
> >
> > Since there might be many local background threads spawned from different
> > userspace processes, we can't use a single global handle per annotation.
> > Instead, the userspace process passes a non-zero handle through the
> > common_handle field of the kcov_remote_arg struct. This common handle gets
> > saved to the kcov_handle field in the current task_struct and needs to be
> > passed to the newly spawned threads via custom annotations. Those threads
> > should in turn be annotated with kcov_remote_start()/kcov_remote_stop().
> >
> > Internally kcov stores handles as u64 integers. The top byte of a handle
> > is used to denote the id of a subsystem that this handle belongs to, and
> > the lower 4 bytes are used to denote a handle id within that subsystem.
> > A reserved value 0 is used as a subsystem id for common handles as they
> > don't belong to a particular subsystem. The bytes 4-7 are currently
> > reserved and must be zero. In the future the number of bytes used for the
> > subsystem or handle ids might be increased.
> >
> > When a particular userspace proccess collects coverage by via a common
> > handle, kcov will collect coverage for each code section that is annotated
> > to use the common handle obtained as kcov_handle from the current
> > task_struct. However non common handles allow to collect coverage
> > selectively from different subsystems.
> >
> > ...
> >
> > +static struct kcov_remote *kcov_remote_add(struct kcov *kcov, u64 handle)
> > +{
> > +     struct kcov_remote *remote;
> > +
> > +     if (kcov_remote_find(handle))
> > +             return ERR_PTR(-EEXIST);
> > +     remote = kmalloc(sizeof(*remote), GFP_ATOMIC);
> > +     if (!remote)
> > +             return ERR_PTR(-ENOMEM);
> > +     remote->handle = handle;
> > +     remote->kcov = kcov;
> > +     hash_add(kcov_remote_map, &remote->hnode, handle);
> > +     return remote;
> > +}
> > +
> >
> > ...
> >
> > +             spin_lock(&kcov_remote_lock);
> > +             for (i = 0; i < remote_arg->num_handles; i++) {
> > +                     kcov_debug("handle %llx\n", remote_arg->handles[i]);
> > +                     if (!kcov_check_handle(remote_arg->handles[i],
> > +                                             false, true, false)) {
> > +                             spin_unlock(&kcov_remote_lock);
> > +                             kcov_disable(t, kcov);
> > +                             return -EINVAL;
> > +                     }
> > +                     remote = kcov_remote_add(kcov, remote_arg->handles[i]);
> > +                     if (IS_ERR(remote)) {
> > +                             spin_unlock(&kcov_remote_lock);
> > +                             kcov_disable(t, kcov);
> > +                             return PTR_ERR(remote);
> > +                     }
> > +             }
>
> It's worrisome that this code can perform up to 65536 GFP_ATOMIC
> allocations without coming up for air.  The possibility of ENOMEM or of
> causing collateral problems is significant.  It doesn't look too hard
> to change this to use GFP_KERNEL?

Sure, I'll do that in v3. I can also change the limit on the number of
handles to something lower (0x100?).

>
> > +u64 kcov_common_handle(void)
> > +{
> > +     return current->kcov_handle;
> > +}
>
> I don't immediately understand what this "common handle" thing is all about.
> Code is rather lacking in this sort of high-level commentary?

It's described in the documentation, but I'll add comments to the
exported functions to explain how they work and how they should be
used.

Thanks!