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