Re: [PATCH v17] exec: Fix dead-lock in de_thread with ptrace_attach

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 11/23/25 19:32, Oleg Nesterov wrote:
> Hi Bernd,
> 
> sorry for delay, I am on PTO, didn't read emails this week...
> 
> On 11/17, Bernd Edlinger wrote:
>>
>> On 11/17/25 16:01, Oleg Nesterov wrote:
>>> On 11/17, Bernd Edlinger wrote:
>>>>
>>>> On 11/11/25 10:21, Christian Brauner wrote:
>>>>> On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:
>>>>
>>>>>> But this is minor. Why do we need "bool unsafe_execve_in_progress" ?
>>>>>> If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex
>>>>>> unconditionally.
>>>>>>
>>>>
>>>> I would not like to drop the mutex when no absolutely necessary for performance reasons.
>>>
>>> OK, I won't insist... But I don't really understand how this can help to
>>> improve the performance. If nothing else, this adds another for_other_threads()
>>> loop.
>>>
>>
>> If no dead-lock is possible it is better to complete the de_thread without
>> releasing the mutex.  For the debugger it is also the better experience,
>> no matter when the ptrace_attack happens it will succeed rather quickly either
>> before the execve or after the execve.
> 
> I still disagree, I still don't understand the "performance reasons", but since I can't
> convince you I won't really argue.
> 
>>>>>>> +	if (unlikely(unsafe_execve_in_progress)) {
>>>>>>> +		spin_unlock_irq(lock);
>>>>>>> +		sig->exec_bprm = bprm;
>>>>>>> +		mutex_unlock(&sig->cred_guard_mutex);
>>>>>>> +		spin_lock_irq(lock);
>>>>>>
>>>>>> I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...
>>>>>>
>>>>
>>>> Since the spin lock was acquired while holding the mutex, both should be
>>>> unlocked in reverse sequence and the spin lock re-acquired after releasing
>>>> the mutex.
>>>
>>> Why?
>>>
>>
>> It is generally more safe when each thread acquires its mutexes in order and
>> releases them in reverse order.
>> Consider this:
>> Thread A:
>> holds spin_lock_irq(siglock);
>> does mutes_unlock(cred_guard_mutex); with irq disabled.
>> task switch happens to Thread B which has irq enabled.
>> and is waiting for cred_guard_mutex.
>> Thrad B:
>> does mutex_lock(cred_guard_mutex);
>> but is interrupted this point and the interrupt handler I executes
>> now iterrupt handler I wants to take siglock and is blocked,
>> because the system one single CPU core.
> 
> I don't follow. Do you mean PREEMPT_RT ?
> 
> If yes. In this case spin_lock_irq() is rt_spin_lock() which doesn't disable irqs,
> it does rt_lock_lock() (takes rt_mutex) + migrate_disable().
> 
> I do think that spin/mutex/whatever_unlock() is always safe. In any order, and
> regardless of RT.
> 

Well, based on my experience with other embedded real-time O/S-es, I would
expect that something named spin_lock_irq locks the task-specific IRQ, and
prevents task switches due to time-slicing, while something called
mutes_unlock may cause an explicit task switch, when another task is waiting
for the mutex.

It is hard to follow how linux implements that spin_lock_irq exactly, but
to me it looks like it is done this way:

include/linux/spinlock_api_smp.h:static inline void __raw_spin_lock_irq(raw_spinlock_t *lock)
include/linux/spinlock_api_smp.h-{
include/linux/spinlock_api_smp.h-       local_irq_disable();
include/linux/spinlock_api_smp.h-       preempt_disable();
include/linux/spinlock_api_smp.h-       spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
include/linux/spinlock_api_smp.h-       LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
include/linux/spinlock_api_smp.h-}

so an explicit task switch while locka_irq_disable looks
very dangerous to me.  Do you know other places where such
a code pattern is used?

I do just ask, because a close look at those might reveal
some serious bugs, WDYT?


Thanks
Bernd.