On 12/4/20 9:10 PM, Linus Torvalds wrote: > On Fri, Dec 4, 2020 at 11:35 AM Eric W. Biederman <[email protected]> > wrote: >> >> From a deadlock perspective the change is strictly better than what we >> have today. The readers will no longer block on each other. > > No, agreed, it's better regardless. > >> For the specific case that syzbot reported it is readers who were >> blocking on each other so that specific case if fixed. > > So the thing is, a reader can still block another reader if a writer > comes in between them. Which is why I was thinking that the same > deadlock could happen if somebody does an execve at just the right > point. > >> On the write side of exec_update_lock we have: >> >> cred_guard_mutex -> exec_update_lock >> >> Which means that to get an ABBA deadlock cred_guard_mutex would need to >> be involved > > No, see above: you can get a deadlock with > > - first reader gets exec_update_lock > > - writer on exec_update_lock blocks on first reader (this is exec) > > - second reader of exec_update_lock now blocks on the writer. > > So if that second reader holds something that the first one wants to > get (or is the same thread as the first one), you have a deadlock: the > first reader will never make any progress, will never release the > lock, and the writer will never get it, and the second reader will > forever wait for the writer that is ahead of it in the queue. > > cred_guard_mutex is immaterial, it's not involved in the deadlock. > Yes, the writer holds it, but it's not relevant for anything else. > > And that deadlock looks very much like what syzcaller detected, in > exactly that scenario: > > Chain exists of: > &sig->exec_update_mutex --> sb_writers#4 --> &p->lock > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&p->lock); > lock(sb_writers#4); > lock(&p->lock); > lock(&sig->exec_update_mutex); > > *** DEADLOCK *** > > No? The only thing that is missing is that writer that causes the > exec_update_mutex readers to block each other - exactly like they did > when it was a mutex. > > But I may be missing something entirely obvious that keeps this from > happening. >
I think you convinced me: > perf_event_open (exec_update_mutex -> ovl_i_mutex) > chown (ovl_i_mutex -> sb_writes) > sendfile (sb_writes -> p->lock) > by reading from a proc file and writing to overlayfs > proc_pid_syscall (p->lock -> exec_update_mutex) We need just 5 Philosophers (A-E): Philosopher A calls proc_pid_syscall, takes p->lock, and falls asleep for a while now. Philosphper B calls sendfile, takes sb_writes, and has to wait on p->lock. Philosopher C calls chown, takes ovl_i_mutes, and has to wait on sb_writes. Philosopher D calls perf_event_open, takes down_read(exec_mutex_lock), and has to wait on ovl_i_mutex. Philosopher E calls exec, and wants to take down_write(exec_mutex_lock), has to wait for Philosopher D. Now Philosopher A wakes up from his sleep, and wants to take down_read(exec_mutex_lock), but due to fairness reasons, he has to wait until Philosopher E gets and releases his write lock again. If Philosopher A is now blocked, we have a deadlock, right? Bernd. > Linus >
