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?
Looking at this again: it seems easy to get rid of locking
kcov_remote_lock when doing kmalloc, but a bit harder to get rid of
kcov->lock. Andrew, would it be OK to just change the max number of
GFP_ATOMIC allocations to 256?
>
> > +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?
>
>
