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

[Eric W. Biederman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Fri, Dec 4, 2020 at 11:35 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> 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.

Oh.  I see what you are saying.  I knew the writer had to be involved
and block, but if the reader comes first all that has to be added to the
situation is that an exec happens and attempts to take the lock.  Any
reader will block the writer.

For some reason I was blind to the fact that a reader could block the
writer, and I was looking for anything else that might block the writer.

> 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 don't think so.

It does look like my change makes it one step more difficult to cause
the deadlock, but the deadlock can still happen. :-(

Making it a read/writer lock both makes it more difficult to cause
a deadlock and makes a lot of sense from a documentation standpoint
so I still plan on merging the changes after the weekend.


It looks like we do need to have a close look at perf_event_open and
everything associated with it.

Eric