linux-kernel - Re: [PATCH] kcov, usb: Fix invalid context sleep in softirq path on PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025072614-molehill-sequel-3aff@gregkh>
Date: Sat, 26 Jul 2025 09:59:35 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Yunseong Kim <ysk@...lloc.com>, Dmitry Vyukov <dvyukov@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Byungchul Park <byungchul@...com>, max.byungchul.park@...il.com,
	Yeoreum Yun <yeoreum.yun@....com>,
	Michelle Jin <shjy180909@...il.com>, linux-kernel@...r.kernel.org,
	Alan Stern <stern@...land.harvard.edu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	stable@...r.kernel.org, kasan-dev@...glegroups.com,
	syzkaller@...glegroups.com, linux-usb@...r.kernel.org,
	linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH] kcov, usb: Fix invalid context sleep in softirq path on
 PREEMPT_RT

On Sat, Jul 26, 2025 at 04:44:42PM +0900, Tetsuo Handa wrote:
> On 2025/07/26 15:36, Greg Kroah-Hartman wrote:
> > Why is this only a USB thing?  What is unique about it to trigger this
> > issue?
> 
> I couldn't catch your question. But the answer could be that
> 
>   __usb_hcd_giveback_urb() is a function which is a USB thing
> 
> and
> 
>   kcov_remote_start_usb_softirq() is calling local_irq_save() despite CONFIG_PREEMPT_RT=y
> 
> as shown below.
> 
> 
> 
> static void __usb_hcd_giveback_urb(struct urb *urb)
> {
>   (...snipped...)
>   kcov_remote_start_usb_softirq((u64)urb->dev->bus->busnum) {
>     if (in_serving_softirq()) {
>       local_irq_save(flags); // calling local_irq_save() is wrong if CONFIG_PREEMPT_RT=y
>       kcov_remote_start_usb(id) {
>         kcov_remote_start(id) {
>           kcov_remote_start(kcov_remote_handle(KCOV_SUBSYSTEM_USB, id)) {
>             (...snipped...)
>             local_lock_irqsave(&kcov_percpu_data.lock, flags) {
>               __local_lock_irqsave(lock, flags) {
>                 #ifndef CONFIG_PREEMPT_RT
>                   https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/local_lock_internal.h#L125
>                 #else
>                   https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/local_lock_internal.h#L235 // not calling local_irq_save(flags)
>                 #endif
>               }
>             }
>             (...snipped...)
>             spin_lock(&kcov_remote_lock) {
>               #ifndef CONFIG_PREEMPT_RT
>                 https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/spinlock.h#L351
>               #else
>                 https://elixir.bootlin.com/linux/v6.16-rc7/source/include/linux/spinlock_rt.h#L42 // mapped to rt_mutex which might sleep
>               #endif
>             }
>             (...snipped...)
>           }
>         }
>       }
>     }
>   }
>   (...snipped...)
> }
> 

Ok, but then how does the big comment section for
kcov_remote_start_usb_softirq() work, where it explicitly states:

 * 2. Disables interrupts for the duration of the coverage collection section.
 *    This allows avoiding nested remote coverage collection sections in the
 *    softirq context (a softirq might occur during the execution of a work in
 *    the BH workqueue, which runs with in_serving_softirq() > 0).
 *    For example, usb_giveback_urb_bh() runs in the BH workqueue with
 *    interrupts enabled, so __usb_hcd_giveback_urb() might be interrupted in
 *    the middle of its remote coverage collection section, and the interrupt
 *    handler might invoke __usb_hcd_giveback_urb() again.


You are removing half of this function entirely, which feels very wrong
to me as any sort of solution, as you have just said that all of that
documentation entry is now not needed.

Are you sure this is ok?

thanks,

greg k-h