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.