| Message ID | 223901affc7bd759b2d6995c2dbfbdd0a29bc88a.1602248029.git.andreyknvl@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] kcov, usb, vhost: specify contexts for remote coverage sections | expand |
Hi Andrey,
I love your patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on vhost/linux-next hnaz-linux-mm/master linus/master v5.9-rc8 next-20201009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andrey-Konovalov/kcov-usb-vhost-specify-contexts-for-remote-coverage-sections/20201009-205923
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-rhel-8.3 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/669e3ded7b025bc4f34c158e66974ff11f452a88
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrey-Konovalov/kcov-usb-vhost-specify-contexts-for-remote-coverage-sections/20201009-205923
git checkout 669e3ded7b025bc4f34c158e66974ff11f452a88
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/vhost/vhost.c: In function 'vhost_worker':
>> drivers/vhost/vhost.c:367:8: error: 'KCOV_CONTEXT_TASK' undeclared (first use in this function)
367 | KCOV_CONTEXT_TASK);
| ^~~~~~~~~~~~~~~~~
drivers/vhost/vhost.c:367:8: note: each undeclared identifier is reported only once for each function it appears in
--
drivers/usb/core/hub.c: In function 'hub_event':
>> drivers/usb/core/hub.c:5526:48: error: 'KCOV_CONTEXT_TASK' undeclared (first use in this function)
5526 | kcov_remote_start_usb((u64)hdev->bus->busnum, KCOV_CONTEXT_TASK);
| ^~~~~~~~~~~~~~~~~
drivers/usb/core/hub.c:5526:48: note: each undeclared identifier is reported only once for each function it appears in
--
drivers/usb/core/hcd.c: In function '__usb_hcd_giveback_urb':
>> drivers/usb/core/hcd.c:1649:52: error: 'KCOV_CONTEXT_SOFTIRQ' undeclared (first use in this function)
1649 | kcov_remote_start_usb((u64)urb->dev->bus->busnum, KCOV_CONTEXT_SOFTIRQ);
| ^~~~~~~~~~~~~~~~~~~~
drivers/usb/core/hcd.c:1649:52: note: each undeclared identifier is reported only once for each function it appears in
vim +/KCOV_CONTEXT_TASK +367 drivers/vhost/vhost.c
338
339 static int vhost_worker(void *data)
340 {
341 struct vhost_dev *dev = data;
342 struct vhost_work *work, *work_next;
343 struct llist_node *node;
344
345 kthread_use_mm(dev->mm);
346
347 for (;;) {
348 /* mb paired w/ kthread_stop */
349 set_current_state(TASK_INTERRUPTIBLE);
350
351 if (kthread_should_stop()) {
352 __set_current_state(TASK_RUNNING);
353 break;
354 }
355
356 node = llist_del_all(&dev->work_list);
357 if (!node)
358 schedule();
359
360 node = llist_reverse_order(node);
361 /* make sure flag is seen after deletion */
362 smp_wmb();
363 llist_for_each_entry_safe(work, work_next, node, node) {
364 clear_bit(VHOST_WORK_QUEUED, &work->flags);
365 __set_current_state(TASK_RUNNING);
366 kcov_remote_start_common(dev->kcov_handle,
> 367 KCOV_CONTEXT_TASK);
368 work->fn(work);
369 kcov_remote_stop(KCOV_CONTEXT_TASK);
370 if (need_resched())
371 schedule();
372 }
373 }
374 kthread_unuse_mm(dev->mm);
375 return 0;
376 }
377
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Andrey,
I love your patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on vhost/linux-next hnaz-linux-mm/master linus/master v5.9-rc8 next-20201009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andrey-Konovalov/kcov-usb-vhost-specify-contexts-for-remote-coverage-sections/20201009-205923
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/669e3ded7b025bc4f34c158e66974ff11f452a88
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrey-Konovalov/kcov-usb-vhost-specify-contexts-for-remote-coverage-sections/20201009-205923
git checkout 669e3ded7b025bc4f34c158e66974ff11f452a88
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/usb/core/hub.c: In function 'hub_event':
>> drivers/usb/core/hub.c:5526:48: error: 'KCOV_CONTEXT_TASK' undeclared (first use in this function)
5526 | kcov_remote_start_usb((u64)hdev->bus->busnum, KCOV_CONTEXT_TASK);
| ^~~~~~~~~~~~~~~~~
drivers/usb/core/hub.c:5526:48: note: each undeclared identifier is reported only once for each function it appears in
--
drivers/usb/core/hcd.c: In function '__usb_hcd_giveback_urb':
>> drivers/usb/core/hcd.c:1649:52: error: 'KCOV_CONTEXT_SOFTIRQ' undeclared (first use in this function)
1649 | kcov_remote_start_usb((u64)urb->dev->bus->busnum, KCOV_CONTEXT_SOFTIRQ);
| ^~~~~~~~~~~~~~~~~~~~
drivers/usb/core/hcd.c:1649:52: note: each undeclared identifier is reported only once for each function it appears in
--
drivers/vhost/vhost.c: In function 'vhost_worker':
>> drivers/vhost/vhost.c:367:8: error: 'KCOV_CONTEXT_TASK' undeclared (first use in this function)
367 | KCOV_CONTEXT_TASK);
| ^~~~~~~~~~~~~~~~~
drivers/vhost/vhost.c:367:8: note: each undeclared identifier is reported only once for each function it appears in
vim +/KCOV_CONTEXT_TASK +5526 drivers/usb/core/hub.c
5510
5511 static void hub_event(struct work_struct *work)
5512 {
5513 struct usb_device *hdev;
5514 struct usb_interface *intf;
5515 struct usb_hub *hub;
5516 struct device *hub_dev;
5517 u16 hubstatus;
5518 u16 hubchange;
5519 int i, ret;
5520
5521 hub = container_of(work, struct usb_hub, events);
5522 hdev = hub->hdev;
5523 hub_dev = hub->intfdev;
5524 intf = to_usb_interface(hub_dev);
5525
> 5526 kcov_remote_start_usb((u64)hdev->bus->busnum, KCOV_CONTEXT_TASK);
5527
5528 dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
5529 hdev->state, hdev->maxchild,
5530 /* NOTE: expects max 15 ports... */
5531 (u16) hub->change_bits[0],
5532 (u16) hub->event_bits[0]);
5533
5534 /* Lock the device, then check to see if we were
5535 * disconnected while waiting for the lock to succeed. */
5536 usb_lock_device(hdev);
5537 if (unlikely(hub->disconnected))
5538 goto out_hdev_lock;
5539
5540 /* If the hub has died, clean up after it */
5541 if (hdev->state == USB_STATE_NOTATTACHED) {
5542 hub->error = -ENODEV;
5543 hub_quiesce(hub, HUB_DISCONNECT);
5544 goto out_hdev_lock;
5545 }
5546
5547 /* Autoresume */
5548 ret = usb_autopm_get_interface(intf);
5549 if (ret) {
5550 dev_dbg(hub_dev, "Can't autoresume: %d\n", ret);
5551 goto out_hdev_lock;
5552 }
5553
5554 /* If this is an inactive hub, do nothing */
5555 if (hub->quiescing)
5556 goto out_autopm;
5557
5558 if (hub->error) {
5559 dev_dbg(hub_dev, "resetting for error %d\n", hub->error);
5560
5561 ret = usb_reset_device(hdev);
5562 if (ret) {
5563 dev_dbg(hub_dev, "error resetting hub: %d\n", ret);
5564 goto out_autopm;
5565 }
5566
5567 hub->nerrors = 0;
5568 hub->error = 0;
5569 }
5570
5571 /* deal with port status changes */
5572 for (i = 1; i <= hdev->maxchild; i++) {
5573 struct usb_port *port_dev = hub->ports[i - 1];
5574
5575 if (test_bit(i, hub->event_bits)
5576 || test_bit(i, hub->change_bits)
5577 || test_bit(i, hub->wakeup_bits)) {
5578 /*
5579 * The get_noresume and barrier ensure that if
5580 * the port was in the process of resuming, we
5581 * flush that work and keep the port active for
5582 * the duration of the port_event(). However,
5583 * if the port is runtime pm suspended
5584 * (powered-off), we leave it in that state, run
5585 * an abbreviated port_event(), and move on.
5586 */
5587 pm_runtime_get_noresume(&port_dev->dev);
5588 pm_runtime_barrier(&port_dev->dev);
5589 usb_lock_port(port_dev);
5590 port_event(hub, i);
5591 usb_unlock_port(port_dev);
5592 pm_runtime_put_sync(&port_dev->dev);
5593 }
5594 }
5595
5596 /* deal with hub status changes */
5597 if (test_and_clear_bit(0, hub->event_bits) == 0)
5598 ; /* do nothing */
5599 else if (hub_hub_status(hub, &hubstatus, &hubchange) < 0)
5600 dev_err(hub_dev, "get_hub_status failed\n");
5601 else {
5602 if (hubchange & HUB_CHANGE_LOCAL_POWER) {
5603 dev_dbg(hub_dev, "power change\n");
5604 clear_hub_feature(hdev, C_HUB_LOCAL_POWER);
5605 if (hubstatus & HUB_STATUS_LOCAL_POWER)
5606 /* FIXME: Is this always true? */
5607 hub->limited_power = 1;
5608 else
5609 hub->limited_power = 0;
5610 }
5611 if (hubchange & HUB_CHANGE_OVERCURRENT) {
5612 u16 status = 0;
5613 u16 unused;
5614
5615 dev_dbg(hub_dev, "over-current change\n");
5616 clear_hub_feature(hdev, C_HUB_OVER_CURRENT);
5617 msleep(500); /* Cool down */
5618 hub_power_on(hub, true);
5619 hub_hub_status(hub, &status, &unused);
5620 if (status & HUB_STATUS_OVERCURRENT)
5621 dev_err(hub_dev, "over-current condition\n");
5622 }
5623 }
5624
5625 out_autopm:
5626 /* Balance the usb_autopm_get_interface() above */
5627 usb_autopm_put_interface_no_suspend(intf);
5628 out_hdev_lock:
5629 usb_unlock_device(hdev);
5630
5631 /* Balance the stuff in kick_hub_wq() and allow autosuspend */
5632 usb_autopm_put_interface(intf);
5633 kref_put(&hub->kref, hub_release);
5634
5635 kcov_remote_stop(KCOV_CONTEXT_TASK);
5636 }
5637
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst index 8548b0b04e43..99fda94a34c5 100644 --- a/Documentation/dev-tools/kcov.rst +++ b/Documentation/dev-tools/kcov.rst @@ -235,6 +235,11 @@ 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(). +Besides a handle, kcov_remote_start()/kcov_remote_stop() annotations accept +a context mask. This mask describes the contexts in which these annotations +should be applied. E.g. specifying KCOV_CONTEXT_SOFTIRQ will result in the +corresponding annotations being ignored in any context other than softirq. + 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 the id of a thread instance within diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index a33b849e8beb..1b090e3218a8 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1646,9 +1646,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb) /* pass ownership to the completion handler */ urb->status = status; - kcov_remote_start_usb((u64)urb->dev->bus->busnum); + kcov_remote_start_usb((u64)urb->dev->bus->busnum, KCOV_CONTEXT_SOFTIRQ); urb->complete(urb); - kcov_remote_stop(); + kcov_remote_stop(KCOV_CONTEXT_SOFTIRQ); usb_anchor_resume_wakeups(anchor); atomic_dec(&urb->use_count); diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 5b768b80d1ee..d17db72dd020 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -5509,7 +5509,7 @@ static void hub_event(struct work_struct *work) hub_dev = hub->intfdev; intf = to_usb_interface(hub_dev); - kcov_remote_start_usb((u64)hdev->bus->busnum); + kcov_remote_start_usb((u64)hdev->bus->busnum, KCOV_CONTEXT_TASK); dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n", hdev->state, hdev->maxchild, @@ -5618,7 +5618,7 @@ static void hub_event(struct work_struct *work) usb_autopm_put_interface(intf); kref_put(&hub->kref, hub_release); - kcov_remote_stop(); + kcov_remote_stop(KCOV_CONTEXT_TASK); } static const struct usb_device_id hub_id_table[] = { diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index b45519ca66a7..8913de414e89 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -363,9 +363,10 @@ 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_common(dev->kcov_handle); + kcov_remote_start_common(dev->kcov_handle, + KCOV_CONTEXT_TASK); work->fn(work); - kcov_remote_stop(); + kcov_remote_stop(KCOV_CONTEXT_TASK); if (need_resched()) schedule(); } diff --git a/include/linux/kcov.h b/include/linux/kcov.h index a10e84707d82..507003038918 100644 --- a/include/linux/kcov.h +++ b/include/linux/kcov.h @@ -22,6 +22,10 @@ enum kcov_mode { KCOV_MODE_TRACE_CMP = 3, }; +#define KCOV_CONTEXT_TASK (1u << 0) +#define KCOV_CONTEXT_SOFTIRQ (1u << 1) +#define KCOV_CONTEXT_MASK (KCOV_CONTEXT_TASK | KCOV_CONTEXT_SOFTIRQ) + #define KCOV_IN_CTXSW (1 << 30) void kcov_task_init(struct task_struct *t); @@ -38,18 +42,18 @@ do { \ } while (0) /* See Documentation/dev-tools/kcov.rst for usage details. */ -void kcov_remote_start(u64 handle); -void kcov_remote_stop(void); +void kcov_remote_start(u64 handle, unsigned int context); +void kcov_remote_stop(unsigned int context); u64 kcov_common_handle(void); -static inline void kcov_remote_start_common(u64 id) +static inline void kcov_remote_start_common(u64 id, unsigned int context) { - kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, id)); + kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_COMMON, id), context); } -static inline void kcov_remote_start_usb(u64 id) +static inline void kcov_remote_start_usb(u64 id, unsigned int context) { - kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id)); + kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id), context); } #else @@ -58,14 +62,14 @@ static inline void kcov_task_init(struct task_struct *t) {} static inline void kcov_task_exit(struct task_struct *t) {} static inline void kcov_prepare_switch(struct task_struct *t) {} static inline void kcov_finish_switch(struct task_struct *t) {} -static inline void kcov_remote_start(u64 handle) {} -static inline void kcov_remote_stop(void) {} +static inline void kcov_remote_start(u64 handle, unsigned int context) {} +static inline void kcov_remote_stop(unsigned int context) {} static inline u64 kcov_common_handle(void) { return 0; } -static inline void kcov_remote_start_common(u64 id) {} -static inline void kcov_remote_start_usb(u64 id) {} +static inline void kcov_remote_start_common(u64 id, unsigned int context) {} +static inline void kcov_remote_start_usb(u64 id, unsigned int context) {} #endif /* CONFIG_KCOV */ #endif /* _LINUX_KCOV_H */ diff --git a/kernel/kcov.c b/kernel/kcov.c index 6b8368be89c8..e4475b63d6be 100644 --- a/kernel/kcov.c +++ b/kernel/kcov.c @@ -808,7 +808,7 @@ static void kcov_remote_softirq_stop(struct task_struct *t) } } -void kcov_remote_start(u64 handle) +void kcov_remote_start(u64 handle, unsigned int context) { struct task_struct *t = current; struct kcov_remote *remote; @@ -821,7 +821,11 @@ void kcov_remote_start(u64 handle) if (WARN_ON(!kcov_check_handle(handle, true, true, true))) return; - if (!in_task() && !in_serving_softirq()) + if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context)) + return; + if (in_task() && !(context & KCOV_CONTEXT_TASK)) + return; + if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ)) return; local_irq_save(flags); @@ -952,7 +956,7 @@ static void kcov_move_area(enum kcov_mode mode, void *dst_area, } /* See the comment before kcov_remote_start() for usage details. */ -void kcov_remote_stop(void) +void kcov_remote_stop(unsigned int context) { struct task_struct *t = current; struct kcov *kcov; @@ -962,7 +966,11 @@ void kcov_remote_stop(void) int sequence; unsigned long flags; - if (!in_task() && !in_serving_softirq()) + if (WARN_ON((context & ~KCOV_CONTEXT_MASK) || !context)) + return; + if (in_task() && !(context & KCOV_CONTEXT_TASK)) + return; + if (in_serving_softirq() && !(context & KCOV_CONTEXT_SOFTIRQ)) return; local_irq_save(flags);
Currently there's a KCOV remote coverage collection section in __usb_hcd_giveback_urb(). Initially that section was added based on the assumption that usb_hcd_giveback_urb() can only be called in interrupt context as indicated by a comment before it. This is what happens when syzkaller is fuzzing the USB stack via the dummy_hcd driver. As it turns out, it's actually valid to call usb_hcd_giveback_urb() in task context, provided that the caller turned off the interrupts; USB/IP does exactly that. This can lead to a nested KCOV remote coverage collection sections both trying to collect coverage in task context. This isn't supported by KCOV, and leads to a WARNING. The approach this patch takes is to annotate every call of kcov_remote_*() callbacks with the context those callbacks are supposed to be executed in. If the current context doesn't match the mask provided to a callback, that callback is ignored. KCOV currently only supports collecting remote coverage in two contexts: task and softirq. This patch constraints KCOV to only collect coverage from __usb_hcd_giveback_urb() when it's executed in softirq context. As the result, the coverage from USB/IP related usb_hcd_giveback_urb() calls won't be collected, but the WARNING is fixed. A potential future improvement would be to support nested remote coverage collection sections, but this patch doesn't address that. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- Changes v1->v2: - Fix context checks in kcov_remote_start/stop(). - Clarify commit description. --- Documentation/dev-tools/kcov.rst | 5 +++++ drivers/usb/core/hcd.c | 4 ++-- drivers/usb/core/hub.c | 4 ++-- drivers/vhost/vhost.c | 5 +++-- include/linux/kcov.h | 24 ++++++++++++++---------- kernel/kcov.c | 16 ++++++++++++---- 6 files changed, 38 insertions(+), 20 deletions(-)