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] [day] [month] [year] [list]

Message-ID: <87ldobp3gu.ffs@tglx>
Date: Sat, 26 Jul 2025 13:59:45 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 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>,
 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 09:59, Greg Kroah-Hartman wrote:
> On Sat, Jul 26, 2025 at 04:44:42PM +0900, Tetsuo Handa wrote:
>> 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

Right, it does not invoke local_irq_save(flags), but it takes the
underlying lock, which means it prevents reentrance.

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

I'm not so sure because kcov_percpu_data.lock is only held within
kcov_remote_start() and kcov_remote_stop(), but the above comment
suggests that the whole section needs to be serialized.

Though I'm not a KCOV wizard and might be completely wrong here.

If the whole section is required to be serialized, then this need
another local lock in kcov_percpu_data to work correctly on RT.

Thanks,

        tglx