On Thu, Oct 24, 2019 at 12:22 AM Andrew Morton
[off-list ref] wrote:

On Wed, 23 Oct 2019 17:24:29 +0200 Andrey Konovalov [off-list ref] wrote:

quoted

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?).

quoted

+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!