Re: [PATCH RFC 2/3] usb, kcov: collect coverage from hub_event

[Andrey Konovalov]

On Thu, Oct 17, 2019 at 8:19 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 17, 2019 at 07:44:14PM +0200, Andrey Konovalov wrote:
> > This patch adds kcov_remote_start/kcov_remote_stop annotations to the
> > hub_event function, which is responsible for processing events on USB
> > buses, in particular events that happen during USB device enumeration.
> > Each USB bus gets a unique id, which can be used to attach a kcov device
> > to a particular USB bus for coverage collection.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > ---
> >  drivers/usb/core/hub.c    | 4 ++++
> >  include/linux/kcov.h      | 1 +
> >  include/uapi/linux/kcov.h | 7 +++++++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 236313f41f4a..03a40e41b099 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -5374,6 +5374,8 @@ static void hub_event(struct work_struct *work)
> >       hub_dev = hub->intfdev;
> >       intf = to_usb_interface(hub_dev);
> >
> > +     kcov_remote_start(kcov_remote_handle_usb(hdev->bus->busnum));
> > +
> >       dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
> >                       hdev->state, hdev->maxchild,
> >                       /* NOTE: expects max 15 ports... */
> > @@ -5480,6 +5482,8 @@ static void hub_event(struct work_struct *work)
> >       /* Balance the stuff in kick_hub_wq() and allow autosuspend */
> >       usb_autopm_put_interface(intf);
> >       kref_put(&hub->kref, hub_release);
> > +
> > +     kcov_remote_stop();
> >  }
> >
> >  static const struct usb_device_id hub_id_table[] = {
> > diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> > index 702672d98d35..38a47e0b67c2 100644
> > --- a/include/linux/kcov.h
> > +++ b/include/linux/kcov.h
> > @@ -30,6 +30,7 @@ void kcov_task_exit(struct task_struct *t);
> >  /*
> >   * Reserved handle ranges:
> >   * 0000000000000000 - 0000ffffffffffff : common handles
> > + * 0001000000000000 - 0001ffffffffffff : USB subsystem handles
>
> So how many bits are you going to have for any in-kernel tasks?  Aren't
> you going to run out quickly?

With these patches we only collect coverage from hub_event threads,
and we need one ID per USB bus, the number of which is quite limited.
But then we might want to collect coverage from other parts of the USB
subsystem, so we might need more IDs. I don't expect the number of
different subsystem from which we want to collect coverage to be
large, so the idea here is to use 2 bytes of an ID to denote the
subsystem, and the other 6 to denote different coverage collection
sections within it.

But overall, which encoding scheme to use here is a good question.
Ideas are welcome.

> >   */
> >  void kcov_remote_start(u64 handle);
> >  void kcov_remote_stop(void);
> > diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> > index 46f78f716ca9..45c9ae59cebc 100644
> > --- a/include/uapi/linux/kcov.h
> > +++ b/include/uapi/linux/kcov.h
> > @@ -43,4 +43,11 @@ enum {
> >  #define KCOV_CMP_SIZE(n)        ((n) << 1)
> >  #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
> >
> > +#define KCOV_REMOTE_HANDLE_USB  0x0001000000000000ull
> > +
> > +static inline __u64 kcov_remote_handle_usb(unsigned int bus)
> > +{
> > +     return KCOV_REMOTE_HANDLE_USB + (__u64)bus;
> > +}
>
> Why is this function in a uapi .h file?  What userspace code would call
> this?

A userspace process that wants to collect coverage from USB bus # N
needs to pass kcov_remote_handle_usb(N) into KCOV_REMOTE_ENABLE ioctl.