Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore

Bernd Edlinger
2020-12-04T20:31:28Z

On 12/4/20 9:10 PM, Linus Torvalds wrote:
> On Fri, Dec 4, 2020 at 11:35 AM Eric W. Biederman <ebiederm@xmission.com> 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
>