LKML Archive on
help / color / mirror / Atom feed
From: Manfred Spraul <>
To: "Eric W. Biederman" <>,
	Alexander Mihalicyn <>
Cc: Andrew Morton <>,
	"" <>,
	Milton Miller <>,
	Jack Miller <>,
	Pavel Tikhomirov <>,
	Davidlohr Bueso <>,
	Johannes Weiner <>,
	Michal Hocko <>,
	Vladimir Davydov <>,
	Andrei Vagin <>,
	Christian Brauner <>
Subject: Re: [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed
Date: Thu, 23 Sep 2021 18:36:16 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <87y2ab9w8u.fsf@disp2133>

Hi Eric,

I'd like to restart the discussion, the issue should be fixed.

On 7/12/21 9:18 PM, Eric W. Biederman wrote:
> Given that this is a bug I think c) is the safest option.
> A couple of suggestions.
> 1) We can replace the test "shm_creator != NULL" with
>     "list_empty(&shp->shm_clist)"

Yes, good idea. list_del() already contains WRITE_ONCE() and 
list_empty() contains READ_ONCE(), i.e. we get clear memory ordering rules.

>   and remove shm_creator.

I do not see how we can remove shm_creator:

- thread A: creates new segment, doesn't map it.

- thread B: shmctl(,IPC_RMID,).

thread B must now locate the lock that protects ->shm_clist. And if the 
lock is per-thread, then I do not see how we can avoid having a pointer 
in shp to the lock.

>     Along with replacing "shm_creator = NULL" with
>     "list_del_init(&shp->shm_clist)".
Correct, list_del_init() is better than shm_create = NULL.
> 2) We can update shmat to do "list_del_init(&shp->shm_clist)"
>     upon shmat.

That would be a (tiny) user space visible change:

echo 0 > /proc/sys/kernel/shm_rmid_forced
echo 1 > /proc/sys/kernel/shm_rmid_forced

Right now: segment is destroyed

After your proposal: Segment is not destroyed.

I don't think that we should mix that with the bugfix.

>    The last unmap will still shm_destroy the
>     shm segment as ns->shm_rmid_forced is set.
But what if shm_rmid_forced is modified?
>     For a multi-threaded process I think this will nicely clean up
>     the clist, and make it clear that the clist only cares about
>     those segments that have been created but never attached.

> 3) Put a non-reference counted struct ipc_namespace in struct
>     shmid_kernel, and use it to remove the namespace parameter
>     from shm_destroy.
> I think that is enough to fix this bug with no changes in semantics,
> no additional memory consumed, and an implementation that is easier
> to read and perhaps a little faster.

I do not see how this solves the list corruption:

A thread creates 2 shm segments, then switches the namespace and creates 
another 2 segment.

- corruption 1: in each of the namespaces, one thread calls 
shmctl(,IPC_RMID,) -> both will operate in parallel on ->shm_clist.

- corruption 2: exit_shm() in parallel to one thread

- corruption 3: one shmctl(,IPC_RMID,) in parallel to a shmget().

i.e.: we can have list_add() and multiple list_del() in parallel.

I don't see how this should work without a lock.

With regards to memory usage: I would propose to use task_lock(), that 
lock exists already.



  parent reply	other threads:[~2021-09-23 16:36 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-06 13:22 Alexander Mikhalitsyn
2021-07-06 13:22 ` [PATCH 1/2] shm: skip shm_destroy " Alexander Mikhalitsyn
2021-07-06 13:22 ` [PATCH 2/2] ipc: WARN if trying to remove ipc object which is absent Alexander Mikhalitsyn
2021-07-10  1:12 ` [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed Andrew Morton
2021-07-10 10:55   ` Alexander Mihalicyn
     [not found]     ` <>
2021-07-11 10:33       ` Alexander Mihalicyn
2021-07-11 11:46         ` Manfred Spraul
2021-07-12  9:54           ` Alexander Mihalicyn
2021-07-12 19:18             ` Eric W. Biederman
2021-07-12 19:27               ` Alexander Mihalicyn
2021-07-14 17:30                 ` [RFC PATCH] shm: extend forced shm destroy to support objects from several IPC nses Alexander Mikhalitsyn
2021-07-21  6:32                   ` Manfred Spraul
2021-07-22 18:46                   ` Manfred Spraul
2021-07-30 14:52                     ` Alexander Mihalicyn
2021-07-14 17:42                 ` [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed Alexander Mihalicyn
2021-09-23 16:36               ` Manfred Spraul [this message]
2021-09-24 16:45                 ` Eric W. Biederman

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 0/2] shm: omit forced shm destroy if task IPC namespace was changed' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).