[PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[Yunseong Kim]

The function ppl_io_unit_finished() uses a local_irq_save()/spin_lock()
sequence. On a PREEMPT_RT enabled kernel, spin_lock() can sleep. Calling it
with interrupts disabled creates an atomic context where sleeping is
forbidden.

Ensuring that the interrupt state is managed atomically with the lock
itself. The change is applied to both the 'log->io_list_lock' and
'ppl_conf->no_mem_stripes_lock' critical sections within the function.

Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
---
 drivers/md/raid5-ppl.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
index 56b234683ee6..650bd59ead72 100644
--- a/drivers/md/raid5-ppl.c
+++ b/drivers/md/raid5-ppl.c
@@ -553,15 +553,13 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
 
 	pr_debug("%s: seq: %llu\n", __func__, io->seq);
 
-	local_irq_save(flags);
-
-	spin_lock(&log->io_list_lock);
+	spin_lock_irqsave(&log->io_list_lock, flags);
 	list_del(&io->log_sibling);
-	spin_unlock(&log->io_list_lock);
+	spin_unlock_irqrestore(&log->io_list_lock, flags);
 
 	mempool_free(io, &ppl_conf->io_pool);
 
-	spin_lock(&ppl_conf->no_mem_stripes_lock);
+	spin_lock_irqsave(&ppl_conf->no_mem_stripes_lock, flags);
 	if (!list_empty(&ppl_conf->no_mem_stripes)) {
 		struct stripe_head *sh;
 
@@ -571,9 +569,7 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
 		set_bit(STRIPE_HANDLE, &sh->state);
 		raid5_release_stripe(sh);
 	}
-	spin_unlock(&ppl_conf->no_mem_stripes_lock);
-
-	local_irq_restore(flags);
+	spin_unlock_irqrestore(&ppl_conf->no_mem_stripes_lock, flags);
 
 	wake_up(&conf->wait_for_quiescent);
 }
-- 
2.50.0

Re: [PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[Yu Kuai]

Hi,

在 2025/08/17 19:31, Yunseong Kim 写道:
> The function ppl_io_unit_finished() uses a local_irq_save()/spin_lock()
> sequence. On a PREEMPT_RT enabled kernel, spin_lock() can sleep. Calling it
> with interrupts disabled creates an atomic context where sleeping is
> forbidden.
> 
What? I believe spin_lock can never sleep.

> Ensuring that the interrupt state is managed atomically with the lock
> itself. The change is applied to both the 'log->io_list_lock' and
> 'ppl_conf->no_mem_stripes_lock' critical sections within the function.
> 
> Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
> ---
>   drivers/md/raid5-ppl.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
> index 56b234683ee6..650bd59ead72 100644
> --- a/drivers/md/raid5-ppl.c
> +++ b/drivers/md/raid5-ppl.c
> @@ -553,15 +553,13 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
>   
>   	pr_debug("%s: seq: %llu\n", __func__, io->seq);
>   
> -	local_irq_save(flags);
> -
> -	spin_lock(&log->io_list_lock);
> +	spin_lock_irqsave(&log->io_list_lock, flags);
>   	list_del(&io->log_sibling);
> -	spin_unlock(&log->io_list_lock);
> +	spin_unlock_irqrestore(&log->io_list_lock, flags);
>   
>   	mempool_free(io, &ppl_conf->io_pool);
>   
> -	spin_lock(&ppl_conf->no_mem_stripes_lock);
> +	spin_lock_irqsave(&ppl_conf->no_mem_stripes_lock, flags);

Please notice, local_irq_save + spin_lock is the same as
spin_lock_irqsave, I don't think your changes have any functonal
chagnes.

Thanks,
Kuai

>   	if (!list_empty(&ppl_conf->no_mem_stripes)) {
>   		struct stripe_head *sh;
>   
> @@ -571,9 +569,7 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
>   		set_bit(STRIPE_HANDLE, &sh->state);
>   		raid5_release_stripe(sh);
>   	}
> -	spin_unlock(&ppl_conf->no_mem_stripes_lock);
> -
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&ppl_conf->no_mem_stripes_lock, flags);
>   
>   	wake_up(&conf->wait_for_quiescent);
>   }
> 

Re: [PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[Yunseong Kim]

Hi Yu,

On 8/18/25 9:56 AM, Yu Kuai wrote:
> Hi,
> 
> 在 2025/08/17 19:31, Yunseong Kim 写道:
>> The function ppl_io_unit_finished() uses a local_irq_save()/spin_lock()
>> sequence. On a PREEMPT_RT enabled kernel, spin_lock() can sleep. Calling it
>> with interrupts disabled creates an atomic context where sleeping is
>> forbidden.
>>
> What? I believe spin_lock can never sleep.

I think you might have been a bit surprised by me sending a patch out of
the blue. It would be helpful to refer to the references below:

 On PREEMPT_RT kernels, these lock types are converted to sleeping locks:
  local_lock
  spinlock_t
  rwlock_t

Link: https://docs.kernel.org/locking/locktypes.html#sleeping-locks

>> Ensuring that the interrupt state is managed atomically with the lock
>> itself. The change is applied to both the 'log->io_list_lock' and
>> 'ppl_conf->no_mem_stripes_lock' critical sections within the function.
>>
>> Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
>> ---
>>   drivers/md/raid5-ppl.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
>> index 56b234683ee6..650bd59ead72 100644
>> --- a/drivers/md/raid5-ppl.c
>> +++ b/drivers/md/raid5-ppl.c
>> @@ -553,15 +553,13 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
>>         pr_debug("%s: seq: %llu\n", __func__, io->seq);
>>   -    local_irq_save(flags);
>> -
>> -    spin_lock(&log->io_list_lock);
>> +    spin_lock_irqsave(&log->io_list_lock, flags);

 The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
 have a few implications. For example, on a non-PREEMPT_RT kernel the
 following code sequence works as expected:

 local_irq_disable();
 spin_lock(&lock);
 
 and is fully equivalent to:
 
 spin_lock_irq(&lock);
 
 Same applies to rwlock_t and the _irqsave() suffix variants.

Link: https://docs.kernel.org/locking/locktypes.html#spinlock-t-and-rwlock-t


>>       list_del(&io->log_sibling);
>> -    spin_unlock(&log->io_list_lock);
>> +    spin_unlock_irqrestore(&log->io_list_lock, flags);
>>         mempool_free(io, &ppl_conf->io_pool);
>>   -    spin_lock(&ppl_conf->no_mem_stripes_lock);
>> +    spin_lock_irqsave(&ppl_conf->no_mem_stripes_lock, flags);
> 
> Please notice, local_irq_save + spin_lock is the same as
> spin_lock_irqsave, I don't think your changes have any functonal
> chagnes.

This issue has also been a problem in other subsystems, such as USB:

[BUG] usb: gadget: dummy_hcd: Sleeping function called from invalid
context in dummy_dequeue on PREEMPT_RT

Link: https://lore.kernel.org/lkml/20250816065933.EPwBJ0Sd@linutronix.de/t/#u

I am currently contributing to the kernel to address the areas that need to
adapt to this paradigm shift so that the PREEMPT_RT Linux kernel can be
well supported. I have CC’d other RT people so they can also review
this part.

> Thanks,
> Kuai

Thank you!

Yunseong

Re: [PATCH] md/raid5-ppl: Fix invalid context sleep in ppl_io_unit_finished() on PREEMPT_RT

[Yu Kuai]

Hi,

在 2025/8/18 19:54, Yunseong Kim 写道:
> Hi Yu,
>
> On 8/18/25 9:56 AM, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/08/17 19:31, Yunseong Kim 写道:
>>> The function ppl_io_unit_finished() uses a local_irq_save()/spin_lock()
>>> sequence. On a PREEMPT_RT enabled kernel, spin_lock() can sleep. Calling it
>>> with interrupts disabled creates an atomic context where sleeping is
>>> forbidden.
>>>
>> What? I believe spin_lock can never sleep.
> I think you might have been a bit surprised by me sending a patch out of
> the blue. It would be helpful to refer to the references below:
>
>   On PREEMPT_RT kernels, these lock types are converted to sleeping locks:
>    local_lock
>    spinlock_t
>    rwlock_t
>
> Link: https://docs.kernel.org/locking/locktypes.html#sleeping-locks
>
>>> Ensuring that the interrupt state is managed atomically with the lock
>>> itself. The change is applied to both the 'log->io_list_lock' and
>>> 'ppl_conf->no_mem_stripes_lock' critical sections within the function.
>>>
>>> Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
>>> ---
>>>    drivers/md/raid5-ppl.c | 12 ++++--------
>>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
>>> index 56b234683ee6..650bd59ead72 100644
>>> --- a/drivers/md/raid5-ppl.c
>>> +++ b/drivers/md/raid5-ppl.c
>>> @@ -553,15 +553,13 @@ static void ppl_io_unit_finished(struct ppl_io_unit *io)
>>>          pr_debug("%s: seq: %llu\n", __func__, io->seq);
>>>    -    local_irq_save(flags);
>>> -
>>> -    spin_lock(&log->io_list_lock);
>>> +    spin_lock_irqsave(&log->io_list_lock, flags);
>   The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
>   have a few implications. For example, on a non-PREEMPT_RT kernel the
>   following code sequence works as expected:
>
>   local_irq_disable();
>   spin_lock(&lock);
>   
>   and is fully equivalent to:
>   
>   spin_lock_irq(&lock);
>   
>   Same applies to rwlock_t and the _irqsave() suffix variants.
>
> Link: https://docs.kernel.org/locking/locktypes.html#spinlock-t-and-rwlock-t

Yes, lessons are learned. Perhaps add a link tag in the commit message
just in case someone else will be confused?

Thanks,
Kuai

>
>>>        list_del(&io->log_sibling);
>>> -    spin_unlock(&log->io_list_lock);
>>> +    spin_unlock_irqrestore(&log->io_list_lock, flags);
>>>          mempool_free(io, &ppl_conf->io_pool);
>>>    -    spin_lock(&ppl_conf->no_mem_stripes_lock);
>>> +    spin_lock_irqsave(&ppl_conf->no_mem_stripes_lock, flags);
>> Please notice, local_irq_save + spin_lock is the same as
>> spin_lock_irqsave, I don't think your changes have any functonal
>> chagnes.
> This issue has also been a problem in other subsystems, such as USB:
>
> [BUG] usb: gadget: dummy_hcd: Sleeping function called from invalid
> context in dummy_dequeue on PREEMPT_RT
>
> Link: https://lore.kernel.org/lkml/20250816065933.EPwBJ0Sd@linutronix.de/t/#u
>
> I am currently contributing to the kernel to address the areas that need to
> adapt to this paradigm shift so that the PREEMPT_RT Linux kernel can be
> well supported. I have CC’d other RT people so they can also review
> this part.
>
>> Thanks,
>> Kuai
> Thank you!
>
> Yunseong
>