From: John Johansen <[email protected]>
To: Linus Torvalds <[email protected]>,
"Eric W. Biederman" <[email protected]>
Cc: Ammar Faizi <[email protected]>,
James Morris <[email protected]>,
LSM List <[email protected]>,
Linux Kernel Mailing List <[email protected]>,
Al Viro <[email protected]>,
Kees Cook <[email protected]>,
[email protected], Linux-MM <[email protected]>,
[email protected]
Subject: Re: Linux 5.18-rc4
Date: Mon, 6 Jun 2022 12:19:36 -0700 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHk-=wijAnOcC2qQEAvFtRD_xpPbG+aSUXkfM-nFTHuMmPbZGA@mail.gmail.com>
On 6/6/22 11:28, Linus Torvalds wrote:
> On Mon, Jun 6, 2022 at 8:19 AM Eric W. Biederman <[email protected]> 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.
>
yes
> 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.
>
yeah that is the part I got stuck on, before being pulled away from this
> I suspect that part is that both Apparmor and IPC use the idr local lock.
>
bingo,
apparmor moved its secids allocation from a custom radix tree to idr in
99cc45e48678 apparmor: Use an IDR to allocate apparmor secids
and ipc is using the idr for its id allocation as well
I can easily lift the secid() allocation out of the ctx->lock but that
would still leave it happening under the file_lock and not fix the problem.
I think the quick solution would be for apparmor to stop using idr, reverting
back at least temporarily to the custom radix tree.
next prev parent reply other threads:[~2022-06-06 19:19 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CAHk-=whmtHMzjaVUF9bS+7vE_rrRctcCTvsAeB8fuLYcyYLN-g@mail.gmail.com>
2022-04-27 17:59 ` Linux 5.18-rc4 Ammar Faizi
2022-04-27 18:31 ` Linus Torvalds
2022-06-06 15:19 ` Eric W. Biederman
2022-06-06 18:28 ` Linus Torvalds
2022-06-06 19:19 ` John Johansen [this message]
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 \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
/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