All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
To: linux-kernel@vger.kernel.org
Cc: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Greg KH <gregkh@linuxfoundation.org>,
	Andrei Vagin <avagin@gmail.com>,
	Pavel Tikhomirov <ptikhomirov@virtuozzo.com>,
	Vasily Averin <vvs@virtuozzo.com>,
	Manfred Spraul <manfred@colorfullife.com>,
	Alexander Mikhalitsyn <alexander@mihalicyn.com>,
	stable@vger.kernel.org
Subject: [PATCH 0/2] shm: shm_rmid_forced feature fixes
Date: Thu, 28 Oct 2021 01:43:46 +0300	[thread overview]
Message-ID: <20211027224348.611025-1-alexander.mikhalitsyn@virtuozzo.com> (raw)

A long story behind all of that...

Some time ago I met kernel crash after CRIU restore procedure,
fortunately, it was CRIU restore, so, I had dump files and could
do restore many times and crash reproduced easily. After some
investigation I've constructed the minimal reproducer. It was
found that it's use-after-free and it happens only if
sysctl kernel.shm_rmid_forced = 1.

The key of the problem is that the exit_shm() function
not handles shp's object destroy when task->sysvshm.shm_clist
contains items from different IPC namespaces. In most cases
this list will contain only items from one IPC namespace.

Why this list may contain object from different namespaces?
Function exit_shm() designed to clean up this list always when
process leaves IPC namespace. But we made a mistake a long time ago
and not add exit_shm() call into setns() syscall procedures.
1st second idea was just to add this call to setns() syscall but
it's obviously changes semantics of setns() syscall and that's
userspace-visible change. So, I gave up this idea.

First real attempt to address the issue was just to omit forced destroy
if we meet shp object not from current task IPC namespace [1]. But
that was not the best idea because task->sysvshm.shm_clist was
protected by rwsem which belongs to current task IPC namespace.
It means that list corruption may occur.

Second approach is just extend exit_shm() to properly handle
shp's from different IPC namespaces [2]. This is really
non-trivial thing, I've put a lot of effort into that but
not believed that it's possible to make it fully safe, clean
and clear.

Thanks to the efforts of Manfred Spraul working and elegant
solution was designed. Thanks a lot, Manfred!

Eric also suggested the way to address the issue in
("[RFC][PATCH] shm: In shm_exit destroy all created and never attached segments")
Eric's idea was to maintain a list of shm_clists one per IPC namespace,
use lock-less lists. But there is some extra memory consumption-related concerns.

Alternative solution which was suggested by me was implemented in
("shm: reset shm_clist on setns but omit forced shm destroy")
Idea is pretty simple, we add exit_shm() syscall to setns() but DO NOT
destroy shm segments even if sysctl kernel.shm_rmid_forced = 1, we just
clean up the task->sysvshm.shm_clist list. This chages semantics of
setns() syscall a little bit but in comparision to "naive" solution
when we just add exit_shm() without any special exclusions this looks
like a safer option.

[1] https://lkml.org/lkml/2021/7/6/1108
[2] https://lkml.org/lkml/2021/7/14/736

Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Andrei Vagin <avagin@gmail.com>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Vasily Averin <vvs@virtuozzo.com>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Alexander Mikhalitsyn <alexander@mihalicyn.com>
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>

Alexander Mikhalitsyn (2):
  ipc: WARN if trying to remove ipc object which is absent
  shm: extend forced shm destroy to support objects from several IPC
    nses

 include/linux/ipc_namespace.h |  15 +++
 include/linux/sched/task.h    |   2 +-
 include/linux/shm.h           |   2 +-
 ipc/shm.c                     | 170 +++++++++++++++++++++++++---------
 ipc/util.c                    |   6 +-
 5 files changed, 145 insertions(+), 50 deletions(-)

-- 
2.31.1


             reply	other threads:[~2021-10-27 22:44 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-27 22:43 Alexander Mikhalitsyn [this message]
2021-10-27 22:43 ` [PATCH 1/2] ipc: WARN if trying to remove ipc object which is absent Alexander Mikhalitsyn
2021-10-27 22:43 ` [PATCH 2/2] shm: extend forced shm destroy to support objects from several IPC nses Alexander Mikhalitsyn
2021-10-30  4:26   ` Eric W. Biederman
2021-10-30 13:11     ` Manfred Spraul
2021-11-05 17:46   ` Eric W. Biederman
2021-11-05 19:03     ` Manfred Spraul
2021-11-05 21:34       ` [RFC] shm: extend forced shm destroy to support objects from several IPC nses (simplified) Eric W. Biederman
2021-11-06  7:50         ` Manfred Spraul
2021-11-06 14:42         ` Manfred Spraul
2021-11-07 19:51           ` Eric W. Biederman
2021-11-08 18:34             ` Manfred Spraul

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=20211027224348.611025-1-alexander.mikhalitsyn@virtuozzo.com \
    --to=alexander.mikhalitsyn@virtuozzo.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander@mihalicyn.com \
    --cc=avagin@gmail.com \
    --cc=dave@stgolabs.net \
    --cc=ebiederm@xmission.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=manfred@colorfullife.com \
    --cc=ptikhomirov@virtuozzo.com \
    --cc=stable@vger.kernel.org \
    --cc=vvs@virtuozzo.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.