From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA4ADCA9EA0 for ; Tue, 22 Oct 2019 16:44:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7A56B20679 for ; Tue, 22 Oct 2019 16:44:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="g8plYifc" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732091AbfJVQoW (ORCPT ); Tue, 22 Oct 2019 12:44:22 -0400 Received: from mail-pf1-f195.google.com ([209.85.210.195]:44173 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731806AbfJVQoV (ORCPT ); Tue, 22 Oct 2019 12:44:21 -0400 Received: by mail-pf1-f195.google.com with SMTP id q21so10977396pfn.11 for ; Tue, 22 Oct 2019 09:44:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Cju3Trgb9ARWn7SLQVB88Vdykd80DHdd2zXGQcX8d5Q=; b=g8plYifcL3YE+sT+5oQgQzZgiGUmQdHMYB6BxLgTfRy8FTSJlZnsFOoExopbnVqL8W qiUeeG9xiHQ1yGKpNBbnspkOKZ/Cdh+pr94gw1j9DAnyd26w/wz1n1t0cjDvOHMw2XH4 7GuZNs9fYK/tKYdyIfjPM1Z0SAowl2OpjKr6SfKSo5GtsAkg45CcwqsWafJwsdv9er64 HukZgZSDspgY+2w8T0J2LSvuMoRko++kQ0LRpwUMD2pRGq1kEslf02UmVbxQzpQmW0ln QwvXv5ENCrwknJ8bbZnM2dIxKX1Sa3r8qFBWWvWFd6iX7KasOc673SblMrH5PCPkBvS6 SimQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Cju3Trgb9ARWn7SLQVB88Vdykd80DHdd2zXGQcX8d5Q=; b=XTPNW2/wLIKV0cu41I42FbUbS51sVlXT1qBSO51ipKppD/VKGQWnaJyl7FsY5a46C+ FTmqLt3CeC7/JWdQOHb2V+lKetxgSLBbyFeqhuuOS2L/WJ9ueF6lzraZqy/YGrGdDXE5 3WDCr6q34ZPkKGcBFbrMJC+/YEW51pQWeStixtMmW/tUMCSa29rWy7lSHXZYSdtkpMgo shwBgak11W5xcoudeEnBWpOtW33SEeTVqwwvNzHKWxOiaLmh/G2V+dDngvCNqF7SraG6 X5LP5mpDsLgPg4QU39y6kaQ6LLhSXPVEH2jkJIv5vKuPw19WqKuEc5pzjZDEHxkHqaKU vgVA== X-Gm-Message-State: APjAAAVI65GcxhqIdX+ec1CZZtGTeIc+G4XMrV16kD41CltCY3NC/auI m7qv+7kGXUL0RZzWBpWIryMDqkbJ13eGXC0cZxO5qg== X-Google-Smtp-Source: APXvYqx56qByJQyRoqWZVWY3mX0KRR3T/hjOHreFtXheGN0o34Hd15hAggTBr4Meu/YNngU/SLj6qdO9Iyf8GnBbeyM= X-Received: by 2002:a63:541e:: with SMTP id i30mr4854951pgb.130.1571762659310; Tue, 22 Oct 2019 09:44:19 -0700 (PDT) MIME-Version: 1.0 References: <65fed562d5e6571ece5e3f507ee3701b3f0dd8c9.1571333592.git.andreyknvl@google.com> In-Reply-To: From: Andrey Konovalov Date: Tue, 22 Oct 2019 18:44:07 +0200 Message-ID: Subject: Re: [PATCH RFC 1/3] kcov: remote coverage support To: Dmitry Vyukov Cc: USB list , KVM list , virtualization@lists.linux-foundation.org, netdev , LKML , Greg Kroah-Hartman , Alan Stern , "Michael S . Tsirkin" , Jason Wang , Andrew Morton , Arnd Bergmann , Steven Rostedt , David Windsor , Elena Reshetova , Anders Roxell Content-Type: text/plain; charset="UTF-8" Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On Mon, Oct 21, 2019 at 3:53 PM Dmitry Vyukov wrote: > > On Thu, Oct 17, 2019 at 7:44 PM Andrey Konovalov wrote: > > > > Currently kcov can only collect coverage for syscalls that are issued > > from the current process. This patch adds support for KCOV_REMOTE_ENABLE, > > that makes it possible to collect coverage for arbitrary parts of the > > kernel code, provided that this part is annotated with kcov_remote_start > > and kcov_remote_stop. > > Nitpick: () after function names is the common C style. > > > 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 interfaces (e.g. vhost). > > > > To enable collecting coverage from a global background thread, a unique > > global id must be assigned and passed to the corresponding > > kcov_remote_start annotation call. Then a userspace process can pass this > > id to the KCOV_REMOTE_ENABLE ioctl in the handles array field of the > > kcov_remote_arg struct. This will attach kcov device to the code section, > > that is referenced by this id. Multiple ids can be targeted with the same > > kcov device simultaneously. > > > > Since there might be many local background threads spawned from different > > userspace processes, we can't use a single global id per annotation. > > Instead, the userspace process passes an id through the common_handle > > field of the kcov_remote_arg struct. This id 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 be in turn > > annotated with kcov_remote_start/kcov_remote_stop. > > > > Signed-off-by: Andrey Konovalov > > --- > > Documentation/dev-tools/kcov.rst | 99 +++++++ > > include/linux/kcov.h | 9 + > > include/linux/sched.h | 6 + > > include/uapi/linux/kcov.h | 11 + > > kernel/kcov.c | 434 ++++++++++++++++++++++++++++--- > > 5 files changed, 524 insertions(+), 35 deletions(-) > > > > diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst > > index 42b612677799..1cf0af95a801 100644 > > --- a/Documentation/dev-tools/kcov.rst > > +++ b/Documentation/dev-tools/kcov.rst > > @@ -34,6 +34,7 @@ Profiling data will only become accessible once debugfs has been mounted:: > > > > Coverage collection > > ------------------- > > + > > The following program demonstrates coverage collection from within a test > > program using kcov: > > > > @@ -128,6 +129,7 @@ only need to enable coverage (disable happens automatically on thread end). > > > > Comparison operands collection > > ------------------------------ > > + > > Comparison operands collection is similar to coverage collection: > > > > .. code-block:: c > > @@ -202,3 +204,100 @@ Comparison operands collection is similar to coverage collection: > > > > Note that the kcov modes (coverage collection or comparison operands) are > > mutually exclusive. > > + > > +Remote coverage collection > > +-------------------------- > > + > > +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 this part is annotated > > +with kcov_remote_start/kcov_remote_stop. > > Same here. > > > +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 interfaces (e.g. vhost). > > + > > +To enable collecting coverage from a global background thread, a unique global > > +id must be assigned and passed to the corresponding kcov_remote_start annotation > > +call. Then a userspace process can pass this id to the KCOV_REMOTE_ENABLE ioctl > > +in the handles array field of the kcov_remote_arg struct. This will attach kcov > > +device to the code section, that is referenced by this id. Multiple ids can be > > +targeted with the same kcov device simultaneously. > > + > > +Since there might be many local background threads spawned from different > > +userspace processes, we can't use a single global id per annotation. Instead, > > +the userspace process passes an id through the common_handle field of the > > +kcov_remote_arg struct. This id 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 be in turn annotated with > > +kcov_remote_start/kcov_remote_stop. > > + > > +.. code-block:: c > > + > > + struct kcov_remote_arg { > > + unsigned trace_mode; > > + unsigned area_size; > > + unsigned num_handles; > > + uint64_t common_handle; > > + uint64_t handles[0]; > > + }; > > + > > + #define KCOV_REMOTE_MAX_HANDLES 0x10000 > > This is unused. I would remove it. I assume you just copied all of the > current declarations. But they are extended over time, so later it > will be just some random subset of used and unused declarations. > > > + > > + #define KCOV_INIT_TRACE _IOR('c', 1, unsigned long) > > + #define KCOV_ENABLE _IO('c', 100) > > Same here and for few others below. > > > + #define KCOV_DISABLE _IO('c', 101) > > + #define KCOV_REMOTE_ENABLE _IOW('c', 102, struct kcov_remote_arg) > > + > > + #define COVER_SIZE (64 << 10) > > + > > + #define KCOV_TRACE_PC 0 > > + #define KCOV_TRACE_CMP 1 > > + > > + #define KCOV_REMOTE_ID 0x42 > > + > > + int main(int argc, char **argv) > > + { > > + int fd; > > + unsigned long *cover, n, i; > > + uint64_t handle; > > + > > + fd = open("/sys/kernel/debug/kcov", O_RDWR); > > + if (fd == -1) > > + perror("open"), exit(1); > > + if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE)) > > + perror("ioctl"), exit(1); > > + cover = (unsigned long*)mmap(NULL, COVER_SIZE * sizeof(unsigned long), > > + PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > + if ((void*)cover == MAP_FAILED) > > + perror("mmap"), exit(1); > > + /* Enable coverage collection from the USB bus #1. */ > > This is a bit confusing because KCOV_REMOTE_ID name is not related to > USB in any way. And the real USB id is actually 1. > I think we also need to show an example for the common id functionality. > > > > + arg = calloc(1, sizeof(*arg) + sizeof(uint64_t)); > > + if (!arg) > > + perror("calloc"), exit(1); > > + arg->trace_mode = KCOV_TRACE_PC; > > + arg->area_size = COVER_SIZE; > > + arg->num_handles = 1; > > + arg->handles[0] = KCOV_REMOTE_ID; > > + if (ioctl(fd, KCOV_REMOTE_ENABLE, arg)) > > + perror("ioctl"), free(arg), exit(1); > > + free(arg); > > + > > + /* > > + * The user needs to trigger execution of kernel code section that is > > + * annotated with KCOV_REMOTE_ID. > > + */ > > + sleep(2); > > + > > + n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED); > > + for (i = 0; i < n; i++) > > + printf("0x%lx\n", cover[i + 1]); > > + if (ioctl(fd, KCOV_DISABLE, 0)) > > + perror("ioctl"), exit(1); > > + if (munmap(cover, COVER_SIZE * sizeof(unsigned long))) > > + perror("munmap"), exit(1); > > + if (close(fd)) > > + perror("close"), exit(1); > > + return 0; > > + } > > diff --git a/include/linux/kcov.h b/include/linux/kcov.h > > index b76a1807028d..702672d98d35 100644 > > --- a/include/linux/kcov.h > > +++ b/include/linux/kcov.h > > @@ -27,6 +27,13 @@ enum kcov_mode { > > void kcov_task_init(struct task_struct *t); > > void kcov_task_exit(struct task_struct *t); > > > > +/* > > + * Reserved handle ranges: > > + * 0000000000000000 - 0000ffffffffffff : common handles > > + */ > > It can make sense to make this scheme slightly more generic and > officially reserve high bits for "subsystem id" and low bits for "id > within the subsystem". We probably won't use different subsystems for > common handles for syzkaller, but still it may be reasonable to be > able to separate coverage from different subsystems. We could reserve > 0x00 as a "generic" subsystem, but allow introductions of other > subsystems later. > > > > +void kcov_remote_start(u64 handle); > > +void kcov_remote_stop(void); > > + > > #define kcov_prepare_switch(t) \ > > do { \ > > (t)->kcov_mode |= KCOV_IN_CTXSW; \ > > @@ -41,6 +48,8 @@ do { \ > > > > static inline void kcov_task_init(struct task_struct *t) {} > > static inline void kcov_task_exit(struct task_struct *t) {} > > +static inline void kcov_remote_start(u64 handle) {} > > +static inline void kcov_remote_stop(void) {} > > static inline void kcov_prepare_switch(struct task_struct *t) {} > > static inline void kcov_finish_switch(struct task_struct *t) {} > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index 2c2e56bd8913..7417e0207b48 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -1211,8 +1211,14 @@ struct task_struct { > > /* Buffer for coverage collection: */ > > void *kcov_area; > > > > + /* KCOV sequence number: */ > > This comment does not add much. Extend it to explain why we need it > and how it is used. > > > + int kcov_sequence; > > + > > /* KCOV descriptor wired with this task or NULL: */ > > struct kcov *kcov; > > + > > + /* KCOV handle for remote coverage collection: */ > > + u64 kcov_handle; > > #endif > > > > #ifdef CONFIG_MEMCG > > diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h > > index 9529867717a8..46f78f716ca9 100644 > > --- a/include/uapi/linux/kcov.h > > +++ b/include/uapi/linux/kcov.h > > @@ -4,9 +4,20 @@ > > > > #include > > > > +struct kcov_remote_arg { > > + unsigned int trace_mode; > > + unsigned int area_size; > > + unsigned int num_handles; > > + __u64 common_handle; > > We need some docs here. > In particular we never spell out that common_handle == 0 means no > handle (can't be used for real handles). > > > + __u64 handles[0]; > > +}; > > + > > +#define KCOV_REMOTE_MAX_HANDLES 0x10000 > > + > > #define KCOV_INIT_TRACE _IOR('c', 1, unsigned long) > > #define KCOV_ENABLE _IO('c', 100) > > #define KCOV_DISABLE _IO('c', 101) > > +#define KCOV_REMOTE_ENABLE _IOW('c', 102, struct kcov_remote_arg) > > > > enum { > > /* > > diff --git a/kernel/kcov.c b/kernel/kcov.c > > index 2ee38727844a..49d1668a4570 100644 > > --- a/kernel/kcov.c > > +++ b/kernel/kcov.c > > @@ -9,6 +9,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -23,6 +24,8 @@ > > #include > > #include > > > > +#define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__) > > + > > /* Number of 64-bit words written per one comparison: */ > > #define KCOV_WORDS_PER_CMP 4 > > > > @@ -44,19 +47,94 @@ struct kcov { > > * Reference counter. We keep one for: > > * - opened file descriptor > > * - task with enabled coverage (we can't unwire it from another task) > > + * - each code section for remote coverage collection > > */ > > refcount_t refcount; > > /* The lock protects mode, size, area and t. */ > > spinlock_t lock; > > enum kcov_mode mode; > > - /* Size of arena (in long's for KCOV_MODE_TRACE). */ > > - unsigned size; > > + /* Size of arena (in long's). */ > > + unsigned int size; > > /* Coverage buffer shared with user space. */ > > void *area; > > /* Task for which we collect coverage, or NULL. */ > > struct task_struct *t; > > + /* Collecting coverage from remote threads. */ > > + bool remote; > > + /* Size of remote arena (in long's). */ > > + unsigned int remote_size; > > + /* Sequence is incremented each time kcov is reenabled. */ > > + int sequence; > > +}; > > + > > +struct kcov_remote_area { > > + struct list_head list; > > + unsigned int size; > > +}; > > + > > +struct kcov_remote { > > + u64 handle; > > + struct kcov *kcov; > > + struct hlist_node hnode; > > }; > > > > +static DEFINE_SPINLOCK(kcov_remote_lock); > > +static DEFINE_HASHTABLE(kcov_remote_map, 4); > > +static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas); > > + > > +static struct kcov_remote *kcov_remote_find(u64 handle) > > +{ > > + struct kcov_remote *remote; > > + > > + hash_for_each_possible(kcov_remote_map, remote, hnode, handle) { > > + if (remote->handle == handle) > > + return remote; > > + } > > + return NULL; > > +} > > + > > +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; > > +} > > + > > +static struct kcov_remote_area *kcov_remote_area_get(unsigned int size) > > +{ > > + struct kcov_remote_area *area; > > + struct list_head *pos; > > + > > + kcov_debug("size = %u\n", size); > > + list_for_each(pos, &kcov_remote_areas) { > > + area = list_entry(pos, struct kcov_remote_area, list); > > + if (area->size == size) { > > + list_del(&area->list); > > + kcov_debug("rv = %px\n", area); > > + return area; > > + } > > + } > > + kcov_debug("rv = NULL\n"); > > + return NULL; > > +} > > + > > +static void kcov_remote_area_put(struct kcov_remote_area *area, > > + unsigned int size) > > +{ > > + kcov_debug("area = %px, size = %u\n", area, size); > > + INIT_LIST_HEAD(&area->list); > > + area->size = size; > > + list_add(&area->list, &kcov_remote_areas); > > +} > > + > > static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t) > > { > > unsigned int mode; > > @@ -73,7 +151,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru > > * in_interrupt() returns false (e.g. preempt_schedule_irq()). > > * READ_ONCE()/barrier() effectively provides load-acquire wrt > > * interrupts, there are paired barrier()/WRITE_ONCE() in > > - * kcov_ioctl_locked(). > > + * kcov_start(). > > */ > > barrier(); > > return mode == needed_mode; > > @@ -227,6 +305,69 @@ void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases) > > EXPORT_SYMBOL(__sanitizer_cov_trace_switch); > > #endif /* ifdef CONFIG_KCOV_ENABLE_COMPARISONS */ > > > > +static void kcov_start(struct task_struct *t, unsigned int size, > > + void *area, enum kcov_mode mode, int sequence) > > +{ > > + kcov_debug("t = %px, size = %u, area = %px\n", t, size, area); > > + /* Cache in task struct for performance. */ > > + t->kcov_size = size; > > + t->kcov_area = area; > > + /* See comment in check_kcov_mode(). */ > > + barrier(); > > + WRITE_ONCE(t->kcov_mode, mode); > > + t->kcov_sequence = sequence; > > +} > > + > > +static void kcov_stop(struct task_struct *t) > > +{ > > + WRITE_ONCE(t->kcov_mode, KCOV_MODE_DISABLED); > > + barrier(); > > + t->kcov_size = 0; > > + t->kcov_area = NULL; > > +} > > + > > +static void kcov_task_reset(struct task_struct *t) > > +{ > > + kcov_stop(t); > > + t->kcov = NULL; > > + t->kcov_sequence = 0; > > + t->kcov_handle = 0; > > +} > > + > > +void kcov_task_init(struct task_struct *t) > > +{ > > + kcov_task_reset(t); > > + t->kcov_handle = current->kcov_handle; > > +} > > + > > +static void kcov_reset(struct kcov *kcov) > > +{ > > + kcov->t = NULL; > > + kcov->mode = KCOV_MODE_INIT; > > + kcov->remote = false; > > + kcov->remote_size = 0; > > + kcov->sequence++; > > +} > > + > > +static void kcov_remote_reset(struct kcov *kcov) > > +{ > > + int bkt; > > + struct kcov_remote *remote; > > + struct hlist_node *tmp; > > + > > + spin_lock(&kcov_remote_lock); > > + hash_for_each_safe(kcov_remote_map, bkt, tmp, remote, hnode) { > > + if (remote->kcov != kcov) > > + continue; > > + kcov_debug("removing handle %llx\n", remote->handle); > > + hash_del(&remote->hnode); > > + kfree(remote); > > + } > > + /* Do reset before unlock to prevent races with kcov_remote_start(). */ > > + kcov_reset(kcov); > > + spin_unlock(&kcov_remote_lock); > > +} > > + > > static void kcov_get(struct kcov *kcov) > > { > > refcount_inc(&kcov->refcount); > > @@ -235,20 +376,12 @@ static void kcov_get(struct kcov *kcov) > > static void kcov_put(struct kcov *kcov) > > { > > if (refcount_dec_and_test(&kcov->refcount)) { > > + kcov_remote_reset(kcov); > > vfree(kcov->area); > > kfree(kcov); > > } > > } > > > > -void kcov_task_init(strucKCOV_TRACE_PCt task_struct *t) > > -{ > > - WRITE_ONCE(t->kcov_mode, KCOV_MODE_DISABLED); > > - barrier(); > > - t->kcov_size = 0; > > - t->kcov_area = NULL; > > - t->kcov = NULL; > > -} > > - > > void kcov_task_exit(struct task_struct *t) > > { > > struct kcov *kcov; > > @@ -256,15 +389,27 @@ void kcov_task_exit(struct task_struct *t) > > kcov = t->kcov; > > if (kcov == NULL) > > return; > > + > > spin_lock(&kcov->lock); > > + kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t); > > + /* > > + * If !kcov->remote, this checks that t->kcov->t == t. > > + * If kcov->remote == true then the exiting task is either: > > + * 1. a remote task between kcov_remote_start() and kcov_remote_stop(), > > + * in this case t != kcov->t and we'll print a warning; or > > + * 2. the task that created kcov exiting without calling KCOV_DISABLE, > > + * in this case t == kcov->t and no warning is printed. > > + */ > > if (WARN_ON(kcov->t != t)) { > > spin_unlock(&kcov->lock); > > return; > > } > > /* Just to not leave dangling references behind. */ > > - kcov_task_init(t); > > - kcov->t = NULL; > > - kcov->mode = KCOV_MODE_INIT; > > + kcov_task_reset(t); > > + if (kcov->remote) > > + kcov_remote_reset(kcov); > > + else > > + kcov_reset(kcov); > > These 5 lines are also repeated in KCOV_DISABLE. Looks like a good > candidate for a helper function. > > > spin_unlock(&kcov->lock); > > kcov_put(kcov); > > } > > @@ -313,6 +458,7 @@ static int kcov_open(struct inode *inode, struct file *filep) > > if (!kcov) > > return -ENOMEM; > > kcov->mode = KCOV_MODE_DISABLED; > > + kcov->sequence = 1; > > refcount_set(&kcov->refcount, 1); > > spin_lock_init(&kcov->lock); > > filep->private_data = kcov; > > @@ -325,6 +471,20 @@ static int kcov_close(struct inode *inode, struct file *filep) > > return 0; > > } > > > > +static int kcov_get_mode(unsigned long arg) > > +{ > > + if (arg == KCOV_TRACE_PC) > > + return KCOV_MODE_TRACE_PC; > > + else if (arg == KCOV_TRACE_CMP) > > +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS > > + return KCOV_MODE_TRACE_CMP; > > +#else > > + return -ENOTSUPP; > > +#endif > > + else > > + return -EINVAL; > > +} > > + > > /* > > * Fault in a lazily-faulted vmalloc area before it can be used by > > * __santizer_cov_trace_pc(), to avoid recursion issues if any code on the > > @@ -345,9 +505,13 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > > { > > struct task_struct *t; > > unsigned long size, unused; > > + int mode, i; > > + struct kcov_remote_arg *remote_arg; > > + struct kcov_remote *remote; > > > > switch (cmd) { > > case KCOV_INIT_TRACE: > > + kcov_debug("KCOV_REMOTE_ENABLE\n"); > > /* > > * Enable kcov in trace mode and setup buffer size. > > * Must happen before anything else. > > @@ -366,6 +530,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > > kcov->mode = KCOV_MODE_INIT; > > return 0; > > case KCOV_ENABLE: > > + kcov_debug("KCOV_ENABLE\n"); > > /* > > * Enable coverage for the current task. > > * At this point user must have been enabled trace mode, > > @@ -378,29 +543,20 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > > t = current; > > if (kcov->t != NULL || t->kcov != NULL) > > return -EBUSY; > > - if (arg == KCOV_TRACE_PC) > > - kcov->mode = KCOV_MODE_TRACE_PC; > > - else if (arg == KCOV_TRACE_CMP) > > -#ifdef CONFIG_KCOV_ENABLE_COMPARISONS > > - kcov->mode = KCOV_MODE_TRACE_CMP; > > -#else > > - return -ENOTSUPP; > > -#endif > > - else > > - return -EINVAL; > > + mode = kcov_get_mode(arg); > > + if (mode < 0) > > + return mode; > > kcov_fault_in_area(kcov); > > - /* Cache in task struct for performance. */ > > - t->kcov_size = kcov->size; > > - t->kcov_area = kcov->area; > > - /* See comment in check_kcov_mode(). */ > > - barrier(); > > - WRITE_ONCE(t->kcov_mode, kcov->mode); > > + kcov->mode = mode; > > + kcov_start(t, kcov->size, kcov->area, kcov->mode, > > + kcov->sequence); > > t->kcov = kcov; > > kcov->t = t; > > - /* This is put either in kcov_task_exit() or in KCOV_DISABLE. */ > > + /* Put either in kcov_task_exit() or in KCOV_DISABLE. */ > > kcov_get(kcov); > > return 0; > > case KCOV_DISABLE: > > + kcov_debug("KCOV_DISABLE\n"); > > /* Disable coverage for the current task. */ > > unused = arg; > > if (unused != 0 || current->kcov != kcov) > > @@ -408,11 +564,52 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > > t = current; > > if (WARN_ON(kcov->t != t)) > > return -EINVAL; > > - kcov_task_init(t); > > - kcov->t = NULL; > > - kcov->mode = KCOV_MODE_INIT; > > + kcov_task_reset(t); > > + if (kcov->remote) > > + kcov_remote_reset(kcov); > > + else > > + kcov_reset(kcov); > > kcov_put(kcov); > > return 0; > > + case KCOV_REMOTE_ENABLE: > > + kcov_debug("KCOV_REMOTE_ENABLE\n"); > > + if (kcov->mode != KCOV_MODE_INIT || !kcov->area) > > + return -EINVAL; > > + t = current; > > + if (kcov->t != NULL || t->kcov != NULL) > > + return -EBUSY; > > + remote_arg = (struct kcov_remote_arg *)arg; > > + mode = kcov_get_mode(remote_arg->trace_mode); > > + if (mode < 0) > > + return mode; > > + kcov->mode = mode; > > + t->kcov = kcov; > > + kcov->t = t; > > + kcov->remote = true; > > + kcov->remote_size = remote_arg->area_size; > > I think we need some sanity check on area_size, or it can lead to > overflows later. > > > + spin_lock(&kcov_remote_lock); > > + for (i = 0; i < remote_arg->num_handles; i++) { > > + remote = kcov_remote_add(kcov, remote_arg->handles[i]); > > + if (IS_ERR(remote)) { > > + spin_unlock(&kcov_remote_lock); > > + kcov_remote_reset(kcov); > > + return PTR_ERR(remote); > > + } > > + } > > + if (remote_arg->common_handle) { > > + remote = kcov_remote_add(kcov, > > + remote_arg->common_handle); > > + if (IS_ERR(remote)) { > > + spin_unlock(&kcov_remote_lock); > > + kcov_remote_reset(kcov); > > + return PTR_ERR(remote); > > + } > > + t->kcov_handle = remote_arg->common_handle; > > + } > > + spin_unlock(&kcov_remote_lock); > > + /* Put either in kcov_task_exit() or in KCOV_DISABLE. */ > > + kcov_get(kcov); > > + return 0; > > default: > > return -ENOTTY; > > } > > @@ -422,11 +619,35 @@ static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > > { > > struct kcov *kcov; > > int res; > > + struct kcov_remote_arg *remote_arg = NULL; > > + unsigned int remote_num_handles; > > + unsigned long remote_arg_size; > > + > > + if (cmd == KCOV_REMOTE_ENABLE) { > > + if (get_user(remote_num_handles, (unsigned __user *)(arg + > > + offsetof(struct kcov_remote_arg, num_handles)))) > > + return -EFAULT; > > + if (remote_num_handles > KCOV_REMOTE_MAX_HANDLES) > > + return -EINVAL; > > + remote_arg_size = sizeof(*remote_arg) + > > + remote_num_handles * sizeof(u64); > > Looks like a good candidate for struct_size(). > This can't overflow due to KCOV_REMOTE_MAX_HANDLES, but still more > readable and bulletproof for future. > > > + remote_arg = memdup_user((void __user *)arg, remote_arg_size); > > + if (IS_ERR(remote_arg)) > > + return PTR_ERR(remote_arg); > > + if (remote_arg->num_handles != remote_num_handles) { > > + kfree(remote_arg); > > + return -EINVAL; > > + } > > + arg = (unsigned long)remote_arg; > > + } > > > > kcov = filep->private_data; > > spin_lock(&kcov->lock); > > res = kcov_ioctl_locked(kcov, cmd, arg); > > spin_unlock(&kcov->lock); > > + > > + kfree(remote_arg); > > + > > return res; > > } > > > > @@ -438,6 +659,149 @@ static const struct file_operations kcov_fops = { > > .release = kcov_close, > > }; > > > > +void kcov_remote_start(u64 handle) > > +{ > > + struct kcov_remote *remote; > > + void *area; > > + struct task_struct *t; > > + unsigned int size; > > + enum kcov_mode mode; > > + int sequence; > > + > > + if (WARN_ON(!in_task())) > > + return; > > + t = current; > > + /* > > + * Check that kcov_remote_start is not called twice > > + * nor called by user tasks (with enabled kcov). > > + */ > > + if (WARN_ON(t->kcov)) > > + return; > > + > > + kcov_debug("handle = %llx\n", handle); > > + > > + spin_lock(&kcov_remote_lock); > > + remote = kcov_remote_find(handle); > > + if (!remote) { > > + kcov_debug("no remote found"); > > + spin_unlock(&kcov_remote_lock); > > + return; > > + } > > + /* Put in kcov_remote_stop(). */ > > + kcov_get(remote->kcov); > > + t->kcov = remote->kcov; > > + /* > > + * Read kcov fields before unlock to prevent races with > > + * KCOV_DISABLE / kcov_remote_reset(). > > + */ > > + size = remote->kcov->remote_size; > > + mode = remote->kcov->mode; > > + sequence = remote->kcov->sequence; > > + area = kcov_remote_area_get(size); > > + spin_unlock(&kcov_remote_lock); > > + > > + if (!area) { > > + area = vmalloc(size * sizeof(unsigned long)); > > + if (!area) { > > Reset t->kcov? > > > + kcov_put(remote->kcov); > > + return; > > + } > > + } > > + /* Reset coverage size. */ > > + *(u64 *)area = 0; > > + > > + kcov_debug("area = %px, size = %u", area, size); > > + > > + kcov_start(t, size, area, mode, sequence); > > + > > +} > > + > > +static void kcov_move_area(enum kcov_mode mode, void *dst_area, > > + unsigned int dst_area_size, void *src_area) > > +{ > > + u64 word_size = sizeof(unsigned long); > > + u64 count_size, entry_size; > > + u64 dst_len, src_len; > > + void *dst_entries, *src_entries; > > + u64 dst_occupied, dst_free, bytes_to_move, entries_moved; > > + > > + kcov_debug("%px %u <= %px %lu\n", > > + dst_area, dst_area_size, src_area, *(unsigned long *)src_area); > > + > > + switch (mode) { > > + case KCOV_MODE_TRACE_PC: > > + dst_len = READ_ONCE(*(unsigned long *)dst_area); > > + src_len = *(unsigned long *)src_area; > > + count_size = sizeof(unsigned long); > > + entry_size = sizeof(unsigned long); > > + break; > > + case KCOV_MODE_TRACE_CMP: > > + dst_len = READ_ONCE(*(u64 *)dst_area); > > + src_len = *(u64 *)src_area; > > + count_size = sizeof(u64); > > + entry_size = sizeof(u64) * KCOV_WORDS_PER_CMP; > > + break; > > + default: > > + WARN_ON(1); > > + return; > > + } > > + > > + if (dst_len > (dst_area_size * word_size - count_size) / entry_size) > > + return; > > + dst_occupied = count_size + dst_len * entry_size; > > + dst_free = dst_area_size * word_size - dst_occupied; > > + bytes_to_move = min(dst_free, src_len * entry_size); > > + dst_entries = dst_area + dst_occupied; > > + src_entries = src_area + count_size; > > + memcpy(dst_entries, src_entries, bytes_to_move); > > + entries_moved = bytes_to_move / entry_size; > > + > > + switch (mode) { > > + case KCOV_MODE_TRACE_PC: > > + WRITE_ONCE(*(unsigned long *)dst_area, dst_len + entries_moved); > > + break; > > + case KCOV_MODE_TRACE_CMP: > > + WRITE_ONCE(*(u64 *)dst_area, dst_len + entries_moved); > > + break; > > + default: > > + break; > > + } > > +} > > + > > +void kcov_remote_stop(void) > > +{ > > + struct task_struct *t = current; > > + struct kcov *kcov = t->kcov; > > + void *area = t->kcov_area; > > + unsigned int size = t->kcov_size; > > + int sequence = t->kcov_sequence; > > + > > + if (!kcov) { > > + kcov_debug("no kcov found\n"); > > + return; > > + } > > + > > + kcov_stop(t); > > + t->kcov = NULL; > > + > > + spin_lock(&kcov->lock); > > + /* > > + * KCOV_DISABLE could have been called between kcov_remote_start() > > + * and kcov_remote_stop(), hence the check. > > + */ > > + kcov_debug("move if: %d == %d && %d\n", > > + sequence, kcov->sequence, (int)kcov->remote); > > + if (sequence == kcov->sequence && kcov->remote) > > + kcov_move_area(kcov->mode, kcov->area, kcov->size, area); > > + spin_unlock(&kcov->lock); > > + > > + spin_lock(&kcov_remote_lock); > > + kcov_remote_area_put(area, size); > > + spin_unlock(&kcov_remote_lock); > > + > > + kcov_put(kcov); > > +} > > + > > static int __init kcov_init(void) > > { > > /* > > -- > > 2.23.0.866.gb869b98d4c-goog > > Will fix all in the next version, thanks!