ipc: introduce shm_rmid_forced sysctl

Signed-off-by Vasiliy Kulikov <[email protected]>

---

 Documentation/sysctl/kernel.txt |   22 ++++++++++
 include/linux/ipc_namespace.h   |    3 +
 include/linux/shm.h             |    5 ++
 ipc/ipc_sysctl.c                |   33 +++++++++++++++
 ipc/shm.c                       |   87 +++++++++++++++++++++++++++++++++++++--
 kernel/exit.c                   |    1 +
 6 files changed, 147 insertions(+), 4 deletions(-)

---

Documentation/sysctl/kernel.txt +1 -0

index 5e7cb39..7a0ecfd 100644
@@ -62,6 +62,7 @@ show up in /proc/sys/kernel:
 - shmall
 - shmmax                      [ sysv ipc ]
 - shmmni
+- shm_rmid_forced
 - stop-a                      [ SPARC only ]
 - sysrq                       ==> Documentation/sysrq.txt
 - tainted

Documentation/sysctl/kernel.txt +5 -0

+process can consume, via setrlimit(2).  Unfortunately, shared memory
+segments are allowed to exist without association with any process, and
+thus might not be counted against any resource limits.  If enabled,
+shared memory segments are automatically destroyed when their attach
+count becomes zero after a detach or a process termination.  It will

This patch adds support for shm_rmid_forced sysctl. If set to 1, all shared memory objects in current ipc namespace will be automatically forced to use IPC_RMID. POSIX way of handling shmem allows to create shm objects and call shmdt() leaving shm object associated with no process, thus consuming memory not counted via rlimits. With shm_rmid_forced=1 the shared memory object is counted at least for one process, so OOM killer may effectively kill the fat process holding the shared memory.

It obviously breaks POSIX, some programs relying on the feature would stop working. So, set shm_rmid_forced=1 only if you're sure nobody uses "orphaned" memory. shm_rmid_forced=0 by default for compatability reasons.

The feature was previously impemented in -ow as a configure option.

What a horrid patch. But given the POSIX (mis?)feature I don't see a better way, and the feature seems desirable. Sigh.

What sort of users would want to turn this on, and why?

include/linux/ipc_namespace.h struct ipc_namespace { · line 49

Quoted 9 lines

include/linux/ipc_namespace.h +1 -0

@@ -44,6 +44,7 @@ struct ipc_namespace {
 	size_t		shm_ctlall;
 	int		shm_ctlmni;
 	int		shm_tot;
+	int		shm_rmid_forced;
 
 	struct notifier_block ipcns_nb;

Please send a patch which adds a nice comment to this field.

Perhaps shm_rmid_forced should have had bool type.

ipc/shm.c void shm_init_ns(struct ipc_namespace *ns) · line 80

Quoted 10 lines

ipc/shm.c +1 -0

@@ -74,6 +74,7 @@ void shm_init_ns(struct ipc_namespace *ns)
 	ns->shm_ctlmax = SHMMAX;
 	ns->shm_ctlall = SHMALL;
 	ns->shm_ctlmni = SHMMNI;
+	ns->shm_rmid_forced = 0;
 	ns->shm_tot = 0;
 	ipc_init_ids(&shm_ids(ns));
 }

The problem is that nobody will test your feature. So for testing purposes, let's enable the feature by default. I assume this:

ipc/shm.c renamed from ipc/shm.c~ipc-introduce-shm_rmid_forced-sysctl-ipc-introduce-shm_rmid_forced-sysctl-testing +1 -1

@@ -74,7 +74,7 @@ void shm_init_ns(struct ipc_namespace *n
 	ns->shm_ctlmax = SHMMAX;
 	ns->shm_ctlall = SHMALL;
 	ns->shm_ctlmni = SHMMNI;
-	ns->shm_rmid_forced = 0;
+	ns->shm_rmid_forced = 1;
 	ns->shm_tot = 0;
 	ipc_init_ids(&shm_ids(ns));
 }

will do that?

ipc/shm.c

Quoted 6 lines

ipc/shm.c +6 -0

+static bool shm_may_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
+{
+	return (shp->shm_nattch == 0) &&
+	       (ns->shm_rmid_forced ||
+		(shp->shm_perm.mode & SHM_DEST));
+}

Just because the existing code is crappily documented doesn't mean that we have to copy that ;)

Quoted 1 line

Perhaps shm_rmid_forced should have had bool type.

OK. The sysctl handler should parse it OK if feed it with sizeof(bool).

Quoted 5 lines

ipc/shm.c +0 -0

@@ -74,6 +74,7 @@ void shm_init_ns(struct ipc_namespace *ns)
 	ns->shm_ctlmax = SHMMAX;
 	ns->shm_ctlall = SHMALL;

ipc/shm.c void shm_init_ns(struct ipc_namespace *n · line 80

Quoted 5 lines

ipc/shm.c +1 -0

 	ns->shm_ctlmni = SHMMNI;
+	ns->shm_rmid_forced = 0;
 	ns->shm_tot = 0;
 	ipc_init_ids(&shm_ids(ns));
 }

The problem is that nobody will test your feature. So for testing purposes, let's enable the feature by default. I assume this:

Furthermore, if testing shows that this is not actually breaking anything in a serious way we could also in theory simplify the patch and just make this the default behavior with no runtime ability to switch it off.

Quoted 4 lines · Ingo Molnar

Furthermore, if testing shows that this is not actually breaking anything in a serious way we could also in theory simplify the patch and just make this the default behavior with no runtime ability to switch it off.

I'm afraid it's impossible. From -ow readme:

"Of course, this breaks the way things are defined, so some applications might stop working. In particular, expect most commercial databases to break. Apache and PostgreSQL are known to work, though. :-)"

http://www.openwall.com/linux/README.shtml

But as it was written in days of Linux 2.4.x, the situation could have changed. A desktop system seems to work.

As we really prefer working systems over non-working ones (and lots of unattached shm segments can clearly result in a non-working system) we can only accept the "this will break stuff" argument if it's *demonstrated* to break stuff and if the failure scenario is carefully described in the commit.

It would take a serious breakage to override a "system locks up swapping itself to death" failure scenario.

Quoted 8 lines

As we really prefer working systems over non-working ones (and lots of unattached shm segments can clearly result in a non-working system) we can only accept the "this will break stuff" argument if it's *demonstrated* to break stuff and if the failure scenario is carefully described in the commit.

It would take a serious breakage to override a "system locks up swapping itself to death" failure scenario.

Ths shared memory interface is defined to be persistent for good reason and all sorts of apps rely upon that so no you can't just ignore that. As a configurable alternative it makes sense (indeed many SYS5 admins used to run shared memory segment sweepers to clean up long idle ones)

However if it's locking the machine up and not being properly handled by resource management then

a) your resource management is broken so fix that instead
b) if your resource management is busted or you are not properly tracking resource commits then the user is going to be able to achieve the same result by other means (eg a unix domain socket bomb)

If you've got no overcommit set you shouldn't be able to swap to death, it may be the sysv shared memory objects need to be accounted for specifically somewhere but that would be the right thing to fix and the mechanisms to do it exist.

This is a simple extension of the OOM killer being able to ... kill things on OOM, ok? 'to kill' implies 'to break'.

From -ow readme:

"Of course, this breaks the way things are defined, so some applications might stop working. In particular, expect most commercial databases to break. Apache and PostgreSQL are known to work, though. :-)"

http://www.openwall.com/linux/README.shtml

But as it was written in days of Linux 2.4.x, the situation could have changed. A desktop system seems to work.

What a horrid patch. But given the POSIX (mis?)feature I don't see a better way, and the feature seems desirable. Sigh.

What sort of users would want to turn this on, and why?

ipc/shm.c static void shm_close(struct vm_area_struct *vma) · line 288

Quoted 10 lines · Vasiliy Kulikov

ipc/shm.c +9 -0

+void exit_shm(struct task_struct *task)
+{
+	struct nsproxy *nsp = task->nsproxy;
+	struct ipc_namespace *ns;
+
+	if (!nsp)
+		return;
+	ns = nsp->ipc_ns;
+	if (!ns || !ns->shm_rmid_forced)

This looks confusing, imho. How it is possible that ->nsproxy or ->ipc_ns is NULL?

ipc/shm.c static void shm_close(struct vm_area_struct *vma) · line 294

Quoted 6 lines

ipc/shm.c +5 -0

+		return;
+
+	/* Destroy all already created segments, but not mapped yet */
+	down_write(&shm_ids(ns).rw_mutex);
+	idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_current, ns);
 	up_write(&shm_ids(ns).rw_mutex);

Again, I do not pretend I understand ipc/, but it seems we can check ns->ipc_ids[].in_use != 0 before the slow path, no?

Quoted 14 lines · Oleg Nesterov

ipc/shm.c static void shm_close(struct vm_area_struct *vma) · line 288

Quoted 10 lines · Vasiliy Kulikov

ipc/shm.c +9 -0

+void exit_shm(struct task_struct *task)
+{
+	struct nsproxy *nsp = task->nsproxy;
+	struct ipc_namespace *ns;
+
+	if (!nsp)
+		return;
+	ns = nsp->ipc_ns;
+	if (!ns || !ns->shm_rmid_forced)

This looks confusing, imho. How it is possible that ->nsproxy or ->ipc_ns is NULL?

I spotted the same checking logic in other places. I don't know whether it is redundant, I guess it can happen when the namespace is dying. Probably it cannot happed inside of task do_exit(), only for extern observers.

Quoted 17 lines · Vasiliy Kulikov

Quoted 14 lines · Oleg Nesterov

ipc/shm.c static void shm_close(struct vm_area_struct *vma) · line 288

Quoted 10 lines · Vasiliy Kulikov

ipc/shm.c +9 -0

+void exit_shm(struct task_struct *task)
+{
+	struct nsproxy *nsp = task->nsproxy;
+	struct ipc_namespace *ns;
+
+	if (!nsp)
+		return;
+	ns = nsp->ipc_ns;
+	if (!ns || !ns->shm_rmid_forced)

This looks confusing, imho. How it is possible that ->nsproxy or ->ipc_ns is NULL?

Quoted 4 lines

I spotted the same checking logic in other places. I don't know whether it is redundant, I guess it can happen when the namespace is dying. Probably it cannot happed inside of task do_exit(), only for extern observers.

No, afaics it can't happen in do_exit() until we call exit_notify(). Otherwise, for example, any dying child will OOPS in do_notify_parent(). Or please look at exit_sem()->sem_lock_check(tsk->nsproxy->ipc_ns).