Re: [PATCH 3/3] vhost, kcov: collect coverage from vhost_worker

[Dmitry Vyukov via Virtualization <virtualization@lists.linux-foundation.org>]

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