From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Ammar Faizi <ammarfaizi2@gnuweeb.org>,
John Johansen <john.johansen@canonical.com>,
James Morris <jmorris@namei.org>,
LSM List <linux-security-module@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>,
Kees Cook <keescook@chromium.org>,
"<linux-fsdevel@vger.kernel.org>" <linux-fsdevel@vger.kernel.org>,
Linux-MM <linux-mm@kvack.org>,
gwml@vger.gnuweeb.org
Subject: Re: Linux 5.18-rc4
Date: Mon, 6 Jun 2022 11:28:23 -0700 [thread overview]
Message-ID: <CAHk-=wijAnOcC2qQEAvFtRD_xpPbG+aSUXkfM-nFTHuMmPbZGA@mail.gmail.com> (raw)
In-Reply-To: <87r1414y5v.fsf@email.froward.int.ebiederm.org>
On Mon, Jun 6, 2022 at 8:19 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Has anyone looked into this lock ordering issues?
The deadlock is
> >> [78140.503821] CPU0 CPU1
> >> [78140.503823] ---- ----
> >> [78140.503824] lock(&newf->file_lock);
> >> [78140.503826] lock(&p->alloc_lock);
> >> [78140.503828] lock(&newf->file_lock);
> >> [78140.503830] lock(&ctx->lock);
and the alloc_lock -> file_lock on CPU1 is trivial - it's seq_show()
in fs/proc/fd.c:
task_lock(task);
files = task->files;
if (files) {
unsigned int fd = proc_fd(m->private);
spin_lock(&files->file_lock);
and that looks all normal.
But the other chains look painful.
I do see the IPC code doing ugly things, in particular I detest this code:
task_lock(current);
list_add(&shp->shm_clist, ¤t->sysvshm.shm_clist);
task_unlock(current);
where it is using the task lock to protect the shm_clist list. Nasty.
And it's doing that inside the shm_ids.rwsem lock _and_ inside the
shp->shm_perm.lock.
So the IPC code has newseg() doing
shmget ->
ipcget():
down_write(ids->rwsem) ->
newseg():
ipc_addid gets perm->lock
task_lock(current)
so you have
ids->rwsem -> perm->lock -> alloc_lock
there.
So now we have that
ids->rwsem -> ipcperm->lock -> alloc_lock -> file_lock
when you put those sequences together.
But I didn't figure out what the security subsystem angle is and how
that then apparently mixes things up with execve.
Yes, newseg() is doing that
error = security_shm_alloc(&shp->shm_perm);
while holding rwsem, but I can't see how that matters. From the
lockdep output, rwsem doesn't actually seem to be part of the whole
sequence.
It *looks* like we have
apparmour ctx->lock -->
radix_tree_preloads.lock -->
ipcperm->lock
and apparently that's called under the file_lock somewhere, completing
the circle.
I guess the execve component is that
begin_new_exec ->
security_bprm_committing_creds ->
apparmor_bprm_committing_creds ->
aa_inherit_files ->
iterate_fd -> *takes file_lock*
match_file ->
aa_file_perm ->
update_file_ctx *takes ctx->lock*
so that's how you get file_lock -> ctx->lock.
So you have:
SHMGET:
ipcperm->lock -> alloc_lock
/proc:
alloc_lock -> file_lock
apparmor_bprm_committing_creds:
file_lock -> ctx->lock
and then all you need is ctx->lock -> ipcperm->lock but I didn't find that part.
I suspect that part is that both Apparmor and IPC use the idr local lock.
Linus
next prev parent reply other threads:[~2022-06-06 18:28 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-24 22:22 Linux 5.18-rc4 Linus Torvalds
2022-04-25 7:32 ` Build regressions/improvements in v5.18-rc4 Geert Uytterhoeven
2022-04-25 7:42 ` Geert Uytterhoeven
2022-04-25 9:50 ` Linux 5.18-rc4 Sudip Mukherjee
2022-04-25 21:27 ` Andrew Morton
2022-04-25 21:42 ` Linus Torvalds
2022-04-25 15:03 ` Guenter Roeck
2022-04-27 17:59 ` Ammar Faizi
2022-04-27 18:31 ` Linus Torvalds
[not found] ` <87r1414y5v.fsf@email.froward.int.ebiederm.org>
2022-06-06 18:28 ` Linus Torvalds [this message]
2022-06-06 19:19 ` John Johansen
2022-06-06 19:47 ` Linus Torvalds
2022-06-06 20:23 ` Matthew Wilcox
2022-06-06 21:00 ` John Johansen
2022-06-13 22:48 ` Matthew Wilcox
2022-06-21 20:27 ` John Johansen
2022-07-13 9:37 ` Ammar Faizi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk-=wijAnOcC2qQEAvFtRD_xpPbG+aSUXkfM-nFTHuMmPbZGA@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=ammarfaizi2@gnuweeb.org \
--cc=ebiederm@xmission.com \
--cc=gwml@vger.gnuweeb.org \
--cc=jmorris@namei.org \
--cc=john.johansen@canonical.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-security-module@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).