On Mon, 29 Jul 2024 at 04:22, <andrey.konovalov@xxxxxxxxx> wrote:
>
> From: Andrey Konovalov <andreyknvl@xxxxxxxxx>
>
> When collecting coverage from softirqs, KCOV uses in_serving_softirq() to
> check whether the code is running in the softirq context. Unfortunately,
> in_serving_softirq() is > 0 even when the code is running in the hardirq
> or NMI context for hardirqs and NMIs that happened during a softirq.
>
> As a result, if a softirq handler contains a remote coverage collection
> section and a hardirq with another remote coverage collection section
> happens during handling the softirq, KCOV incorrectly detects a nested
> softirq coverate collection section and prints a WARNING, as reported
> by syzbot.
>
> This issue was exposed by commit a7f3813e589f ("usb: gadget: dummy_hcd:
> Switch to hrtimer transfer scheduler"), which switched dummy_hcd to using
> hrtimer and made the timer's callback be executed in the hardirq context.
>
> Change the related checks in KCOV to account for this behavior of
> in_serving_softirq() and make KCOV ignore remote coverage collection
> sections in the hardirq and NMI contexts.
>
> This prevents the WARNING printed by syzbot but does not fix the inability
> of KCOV to collect coverage from the __usb_hcd_giveback_urb when dummy_hcd
> is in use (caused by a7f3813e589f); a separate patch is required for that.
>
> Reported-by: syzbot+2388cdaeb6b10f0c13ac@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=2388cdaeb6b10f0c13ac
> Fixes: 5ff3b30ab57d ("kcov: collect coverage from interrupts")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> ---
> kernel/kcov.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index f0a69d402066e..274b6b7c718de 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -161,6 +161,15 @@ static void kcov_remote_area_put(struct kcov_remote_area *area,
> kmsan_unpoison_memory(&area->list, sizeof(area->list));
> }
>
> +/*
> + * Unlike in_serving_softirq(), this function returns false when called during
> + * a hardirq or an NMI that happened in the softirq context.
> + */
> +static inline bool in_softirq_really(void)
> +{
> + return in_serving_softirq() && !in_hardirq() && !in_nmi();
> +}
Not sure you need this function. Check if just this will give you what you want:
interrupt_context_level() == 1
I think the below condition could then also just become:
if (interrupt_context_level() == 1 && t->kcov_softirq)
Although the softirq_count() helper has a special PREEMPT_RT variant,
and interrupt_context_level() doesn't, so it's not immediately obvious
to me if that's also ok on PREEMPT_RT kernels.
Maybe some RT folks can help confirm that using
interrupt_context_level()==1 does what your above function does also
on RT kernels.
> static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
> {
> unsigned int mode;
> @@ -170,7 +179,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
> * so we ignore code executed in interrupts, unless we are in a remote
> * coverage collection section in a softirq.
> */
> - if (!in_task() && !(in_serving_softirq() && t->kcov_softirq))
> + if (!in_task() && !(in_softirq_really() && t->kcov_softirq))
> return false;
> mode = READ_ONCE(t->kcov_mode);
> /*
> @@ -849,7 +858,7 @@ void kcov_remote_start(u64 handle)
>
> if (WARN_ON(!kcov_check_handle(handle, true, true, true)))
> return;
> - if (!in_task() && !in_serving_softirq())
> + if (!in_task() && !in_softirq_really())
> return;
>
> local_lock_irqsave(&kcov_percpu_data.lock, flags);
> @@ -991,7 +1000,7 @@ void kcov_remote_stop(void)
> int sequence;
> unsigned long flags;
>
> - if (!in_task() && !in_serving_softirq())
> + if (!in_task() && !in_softirq_really())
> return;
>
> local_lock_irqsave(&kcov_percpu_data.lock, flags);
> --
> 2.25.1
>
