| Message ID | 26e088ae3ebcaa30afe957aeabaa9f0c653df7d0.1571762488.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kcov: collect coverage from usb and vhost | expand |
On Tue, Oct 22, 2019 at 6:46 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > This patch adds kcov_remote_start()/kcov_remote_stop() annotations to the > vhost_worker() function, which is responsible for processing vhost works. > Since vhost_worker() threads are spawned per vhost device instance > the common kcov handle is used for kcov_remote_start()/stop() annotations > (see Documentation/dev-tools/kcov.rst for details). As the result kcov can > now be used to collect coverage from vhost worker threads. > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > drivers/vhost/vhost.c | 6 ++++++ > drivers/vhost/vhost.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 36ca2cf419bf..a5a557c4b67f 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -30,6 +30,7 @@ > #include <linux/sched/signal.h> > #include <linux/interval_tree_generic.h> > #include <linux/nospec.h> > +#include <linux/kcov.h> > > #include "vhost.h" > > @@ -357,7 +358,9 @@ static int vhost_worker(void *data) > llist_for_each_entry_safe(work, work_next, node, node) { > clear_bit(VHOST_WORK_QUEUED, &work->flags); > __set_current_state(TASK_RUNNING); > + kcov_remote_start(dev->kcov_handle); > work->fn(work); > + kcov_remote_stop(); > if (need_resched()) > schedule(); > } > @@ -546,6 +549,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > /* No owner, become one */ > dev->mm = get_task_mm(current); > + dev->kcov_handle = current->kcov_handle; kcov_handle is not present in task_struct if !CONFIG_KCOV Also this does not use KCOV_SUBSYSTEM_COMMON. We discussed something along the following lines: u64 kcov_remote_handle(u64 subsys, u64 id) { WARN_ON(subsys or id has wrong bits set). return ...; } kcov_remote_handle(KCOV_SUBSYSTEM_USB, bus); kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, current->kcov_handle); > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > if (IS_ERR(worker)) { > err = PTR_ERR(worker); > @@ -571,6 +575,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > if (dev->mm) > mmput(dev->mm); > dev->mm = NULL; > + dev->kcov_handle = 0; > err_mm: > return err; > } > @@ -682,6 +687,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > if (dev->worker) { > kthread_stop(dev->worker); > dev->worker = NULL; > + dev->kcov_handle = 0; > } > if (dev->mm) > mmput(dev->mm); > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index e9ed2722b633..a123fd70847e 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -173,6 +173,7 @@ struct vhost_dev { > int iov_limit; > int weight; > int byte_weight; > + u64 kcov_handle; > }; > > bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len); > -- > 2.23.0.866.gb869b98d4c-goog >
On Wed, Oct 23, 2019 at 10:36 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Tue, Oct 22, 2019 at 6:46 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > This patch adds kcov_remote_start()/kcov_remote_stop() annotations to the > > vhost_worker() function, which is responsible for processing vhost works. > > Since vhost_worker() threads are spawned per vhost device instance > > the common kcov handle is used for kcov_remote_start()/stop() annotations > > (see Documentation/dev-tools/kcov.rst for details). As the result kcov can > > now be used to collect coverage from vhost worker threads. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > --- > > drivers/vhost/vhost.c | 6 ++++++ > > drivers/vhost/vhost.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index 36ca2cf419bf..a5a557c4b67f 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -30,6 +30,7 @@ > > #include <linux/sched/signal.h> > > #include <linux/interval_tree_generic.h> > > #include <linux/nospec.h> > > +#include <linux/kcov.h> > > > > #include "vhost.h" > > > > @@ -357,7 +358,9 @@ static int vhost_worker(void *data) > > llist_for_each_entry_safe(work, work_next, node, node) { > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > __set_current_state(TASK_RUNNING); > > + kcov_remote_start(dev->kcov_handle); > > work->fn(work); > > + kcov_remote_stop(); > > if (need_resched()) > > schedule(); > > } > > @@ -546,6 +549,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > > /* No owner, become one */ > > dev->mm = get_task_mm(current); > > + dev->kcov_handle = current->kcov_handle; > > kcov_handle is not present in task_struct if !CONFIG_KCOV > > Also this does not use KCOV_SUBSYSTEM_COMMON. > We discussed something along the following lines: > > u64 kcov_remote_handle(u64 subsys, u64 id) > { > WARN_ON(subsys or id has wrong bits set). Hm, we can't have warnings in kcov_remote_handle() that is exposed in uapi headers. What we can do is return 0 (invalid handle) if subsys/id have incorrect bits set. And then we can either have another kcov_remote_handle() internally (with a different name though) that has a warning, or have warning in kcov_remote_start(). WDYT? > return ...; > } > > kcov_remote_handle(KCOV_SUBSYSTEM_USB, bus); > kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, current->kcov_handle); > > > > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > > if (IS_ERR(worker)) { > > err = PTR_ERR(worker); > > @@ -571,6 +575,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > if (dev->mm) > > mmput(dev->mm); > > dev->mm = NULL; > > + dev->kcov_handle = 0; > > err_mm: > > return err; > > } > > @@ -682,6 +687,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > > if (dev->worker) { > > kthread_stop(dev->worker); > > dev->worker = NULL; > > + dev->kcov_handle = 0; > > } > > if (dev->mm) > > mmput(dev->mm); > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > > index e9ed2722b633..a123fd70847e 100644 > > --- a/drivers/vhost/vhost.h > > +++ b/drivers/vhost/vhost.h > > @@ -173,6 +173,7 @@ struct vhost_dev { > > int iov_limit; > > int weight; > > int byte_weight; > > + u64 kcov_handle; > > }; > > > > bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len); > > -- > > 2.23.0.866.gb869b98d4c-goog > >
On Wed, Oct 23, 2019 at 3:35 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Wed, Oct 23, 2019 at 10:36 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Tue, Oct 22, 2019 at 6:46 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > This patch adds kcov_remote_start()/kcov_remote_stop() annotations to the > > > vhost_worker() function, which is responsible for processing vhost works. > > > Since vhost_worker() threads are spawned per vhost device instance > > > the common kcov handle is used for kcov_remote_start()/stop() annotations > > > (see Documentation/dev-tools/kcov.rst for details). As the result kcov can > > > now be used to collect coverage from vhost worker threads. > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > --- > > > drivers/vhost/vhost.c | 6 ++++++ > > > drivers/vhost/vhost.h | 1 + > > > 2 files changed, 7 insertions(+) > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > index 36ca2cf419bf..a5a557c4b67f 100644 > > > --- a/drivers/vhost/vhost.c > > > +++ b/drivers/vhost/vhost.c > > > @@ -30,6 +30,7 @@ > > > #include <linux/sched/signal.h> > > > #include <linux/interval_tree_generic.h> > > > #include <linux/nospec.h> > > > +#include <linux/kcov.h> > > > > > > #include "vhost.h" > > > > > > @@ -357,7 +358,9 @@ static int vhost_worker(void *data) > > > llist_for_each_entry_safe(work, work_next, node, node) { > > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > > __set_current_state(TASK_RUNNING); > > > + kcov_remote_start(dev->kcov_handle); > > > work->fn(work); > > > + kcov_remote_stop(); > > > if (need_resched()) > > > schedule(); > > > } > > > @@ -546,6 +549,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > > > > /* No owner, become one */ > > > dev->mm = get_task_mm(current); > > > + dev->kcov_handle = current->kcov_handle; > > > > kcov_handle is not present in task_struct if !CONFIG_KCOV > > > > Also this does not use KCOV_SUBSYSTEM_COMMON. > > We discussed something along the following lines: > > > > u64 kcov_remote_handle(u64 subsys, u64 id) > > { > > WARN_ON(subsys or id has wrong bits set). > > Hm, we can't have warnings in kcov_remote_handle() that is exposed in > uapi headers. What we can do is return 0 (invalid handle) if subsys/id > have incorrect bits set. And then we can either have another > kcov_remote_handle() internally (with a different name though) that > has a warning, or have warning in kcov_remote_start(). WDYT? I would probably add the warning to kcov_remote_start(). This avoids the need for another function and will catch a wrong ID if caller generated it by some other means. And then ioctls should also detect bad handles passed in and return EINVAL. Then we will cover errors for both kernel and user programs. > > > return ...; > > } > > > > kcov_remote_handle(KCOV_SUBSYSTEM_USB, bus); > > kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, current->kcov_handle); > > > > > > > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > > > if (IS_ERR(worker)) { > > > err = PTR_ERR(worker); > > > @@ -571,6 +575,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > if (dev->mm) > > > mmput(dev->mm); > > > dev->mm = NULL; > > > + dev->kcov_handle = 0; > > > err_mm: > > > return err; > > > } > > > @@ -682,6 +687,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > > > if (dev->worker) { > > > kthread_stop(dev->worker); > > > dev->worker = NULL; > > > + dev->kcov_handle = 0; > > > } > > > if (dev->mm) > > > mmput(dev->mm); > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > > > index e9ed2722b633..a123fd70847e 100644 > > > --- a/drivers/vhost/vhost.h > > > +++ b/drivers/vhost/vhost.h > > > @@ -173,6 +173,7 @@ struct vhost_dev { > > > int iov_limit; > > > int weight; > > > int byte_weight; > > > + u64 kcov_handle; > > > }; > > > > > > bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len); > > > -- > > > 2.23.0.866.gb869b98d4c-goog > > >
On Wed, Oct 23, 2019 at 3:50 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Wed, Oct 23, 2019 at 3:35 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > On Wed, Oct 23, 2019 at 10:36 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Tue, Oct 22, 2019 at 6:46 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > > > > > This patch adds kcov_remote_start()/kcov_remote_stop() annotations to the > > > > vhost_worker() function, which is responsible for processing vhost works. > > > > Since vhost_worker() threads are spawned per vhost device instance > > > > the common kcov handle is used for kcov_remote_start()/stop() annotations > > > > (see Documentation/dev-tools/kcov.rst for details). As the result kcov can > > > > now be used to collect coverage from vhost worker threads. > > > > > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > > > --- > > > > drivers/vhost/vhost.c | 6 ++++++ > > > > drivers/vhost/vhost.h | 1 + > > > > 2 files changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > index 36ca2cf419bf..a5a557c4b67f 100644 > > > > --- a/drivers/vhost/vhost.c > > > > +++ b/drivers/vhost/vhost.c > > > > @@ -30,6 +30,7 @@ > > > > #include <linux/sched/signal.h> > > > > #include <linux/interval_tree_generic.h> > > > > #include <linux/nospec.h> > > > > +#include <linux/kcov.h> > > > > > > > > #include "vhost.h" > > > > > > > > @@ -357,7 +358,9 @@ static int vhost_worker(void *data) > > > > llist_for_each_entry_safe(work, work_next, node, node) { > > > > clear_bit(VHOST_WORK_QUEUED, &work->flags); > > > > __set_current_state(TASK_RUNNING); > > > > + kcov_remote_start(dev->kcov_handle); > > > > work->fn(work); > > > > + kcov_remote_stop(); > > > > if (need_resched()) > > > > schedule(); > > > > } > > > > @@ -546,6 +549,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > > > > > > /* No owner, become one */ > > > > dev->mm = get_task_mm(current); > > > > + dev->kcov_handle = current->kcov_handle; > > > > > > kcov_handle is not present in task_struct if !CONFIG_KCOV > > > > > > Also this does not use KCOV_SUBSYSTEM_COMMON. > > > We discussed something along the following lines: > > > > > > u64 kcov_remote_handle(u64 subsys, u64 id) > > > { > > > WARN_ON(subsys or id has wrong bits set). > > > > Hm, we can't have warnings in kcov_remote_handle() that is exposed in > > uapi headers. What we can do is return 0 (invalid handle) if subsys/id > > have incorrect bits set. And then we can either have another > > kcov_remote_handle() internally (with a different name though) that > > has a warning, or have warning in kcov_remote_start(). WDYT? > > I would probably add the warning to kcov_remote_start(). This avoids > the need for another function and will catch a wrong ID if caller > generated it by some other means. > And then ioctls should also detect bad handles passed in and return > EINVAL. Then we will cover errors for both kernel and user programs. OK, will do in v2. > > > > > > return ...; > > > } > > > > > > kcov_remote_handle(KCOV_SUBSYSTEM_USB, bus); > > > kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, current->kcov_handle); I'll add internal kcov_remote_handle_common() and kcov_remote_handle_usb() helpers to simplify kcov hooks in usb/vhost code though. > > > > > > > > > > worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); > > > > if (IS_ERR(worker)) { > > > > err = PTR_ERR(worker); > > > > @@ -571,6 +575,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) > > > > if (dev->mm) > > > > mmput(dev->mm); > > > > dev->mm = NULL; > > > > + dev->kcov_handle = 0; > > > > err_mm: > > > > return err; > > > > } > > > > @@ -682,6 +687,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) > > > > if (dev->worker) { > > > > kthread_stop(dev->worker); > > > > dev->worker = NULL; > > > > + dev->kcov_handle = 0; > > > > } > > > > if (dev->mm) > > > > mmput(dev->mm); > > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > > > > index e9ed2722b633..a123fd70847e 100644 > > > > --- a/drivers/vhost/vhost.h > > > > +++ b/drivers/vhost/vhost.h > > > > @@ -173,6 +173,7 @@ struct vhost_dev { > > > > int iov_limit; > > > > int weight; > > > > int byte_weight; > > > > + u64 kcov_handle; > > > > }; > > > > > > > > bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len); > > > > -- > > > > 2.23.0.866.gb869b98d4c-goog > > > >
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 36ca2cf419bf..a5a557c4b67f 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -30,6 +30,7 @@ #include <linux/sched/signal.h> #include <linux/interval_tree_generic.h> #include <linux/nospec.h> +#include <linux/kcov.h> #include "vhost.h" @@ -357,7 +358,9 @@ static int vhost_worker(void *data) llist_for_each_entry_safe(work, work_next, node, node) { clear_bit(VHOST_WORK_QUEUED, &work->flags); __set_current_state(TASK_RUNNING); + kcov_remote_start(dev->kcov_handle); work->fn(work); + kcov_remote_stop(); if (need_resched()) schedule(); } @@ -546,6 +549,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) /* No owner, become one */ dev->mm = get_task_mm(current); + dev->kcov_handle = current->kcov_handle; worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid); if (IS_ERR(worker)) { err = PTR_ERR(worker); @@ -571,6 +575,7 @@ long vhost_dev_set_owner(struct vhost_dev *dev) if (dev->mm) mmput(dev->mm); dev->mm = NULL; + dev->kcov_handle = 0; err_mm: return err; } @@ -682,6 +687,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) if (dev->worker) { kthread_stop(dev->worker); dev->worker = NULL; + dev->kcov_handle = 0; } if (dev->mm) mmput(dev->mm); diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index e9ed2722b633..a123fd70847e 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -173,6 +173,7 @@ struct vhost_dev { int iov_limit; int weight; int byte_weight; + u64 kcov_handle; }; bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len);
This patch adds kcov_remote_start()/kcov_remote_stop() annotations to the vhost_worker() function, which is responsible for processing vhost works. Since vhost_worker() threads are spawned per vhost device instance the common kcov handle is used for kcov_remote_start()/stop() annotations (see Documentation/dev-tools/kcov.rst for details). As the result kcov can now be used to collect coverage from vhost worker threads. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- drivers/vhost/vhost.c | 6 ++++++ drivers/vhost/vhost.h | 1 + 2 files changed, 7 insertions(+)