Re: [RFC PATCH] shm: extend forced shm destroy to support objects from several IPC nses

Manfred Spraul
2021-07-22T18:46:22Z

[-- Attachment #1: Type: text/plain, Size: 7806 bytes --]

Hi Alexander,

A few more remarks.

On 7/14/21 7:30 PM, Alexander Mikhalitsyn wrote:
> This is total rework of fix.
> Thanks to Eric Biederman for suggestions (but may be I've misunderstood some of them :))
>
> I've tested it with reproducer of the original problem. But of course it needs
> detailed testing. I hope that I get some general comments about design and implementation.
>
> ToDo: remove unneeded "ns" argument from shm_destroy, shm_rmid and other functions.

What ensures the that shp->ns is not destroyed prematurely?

I did some tests, and it seems that shmat() acquires a namespace
refcount, and shm_release() puts it again, and the shm_release is late
enough to ensure that the ns cannot go out of scope.

But I haven't checked all combinations (with/without shm_rmid_forced,
delete via exit(), shmctl(), shmdt(), mmap()).

And: This should be documented somewhere.

> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -65,6 +65,7 @@ struct shmid_kernel /* private to the kernel */
> /* The task created the shm object. NULL if the task is dead. */
> struct task_struct *shm_creator;
> struct list_head shm_clist; /* list by creator */

I think the comments are wrong/outdated.

Some parts of the new code checks with list_empty(shm_clist), not by
looking at shm_creator.

> + struct ipc_namespace *ns;
> } __randomize_layout;
>
> /* shm_mode upper byte flags */
> @@ -115,6 +116,7 @@ static void do_shm_rmid(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
> struct shmid_kernel *shp;
>
> shp = container_of(ipcp, struct shmid_kernel, shm_perm);
> + BUG_ON(shp->ns && ns != shp->ns);

Is shp->ns == NULL allowed/possible? From what I see, it is impossible.

I think we should not have  NULL check in a few codepaths, but not in
other codepaths. Either everywhere, or nowhere.

>
> if (shp->shm_nattch) {
> shp->shm_perm.mode |= SHM_DEST;
> @@ -225,9 +227,32 @@ static void shm_rcu_free(struct rcu_head *head)
> kfree(shp);
> }
>
> +static inline void task_shm_clist_lock(struct task_struct *task)
> +{
> + spin_lock(&task->sysvshm.shm_clist_lock);
> +}
> +
> +static inline void task_shm_clist_unlock(struct task_struct *task)
> +{
> + spin_unlock(&task->sysvshm.shm_clist_lock);
> +}
> +
> +void shm_clist_rm(struct shmid_kernel *shp)
> +{
> + if (!shp->shm_creator)
> + return;
> +
> + task_shm_clist_lock(shp->shm_creator);
> + list_del_init(&shp->shm_clist);
> + task_shm_clist_unlock(shp->shm_creator);
> + shp->shm_creator = NULL;
> +}
> +
> static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
> {
> - list_del(&s->shm_clist);
> + WARN_ON(s->ns && ns != s->ns);
> + //list_del_init(&s->shm_clist);
> + shm_clist_rm(s);
> ipc_rmid(&shm_ids(ns), &s->shm_perm);
> }
>
> @@ -306,10 +331,10 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
> *
> * 2) sysctl kernel.shm_rmid_forced is set to 1.
> */
> -static bool shm_may_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
> +static bool shm_may_destroy(struct shmid_kernel *shp)
> {
> return (shp->shm_nattch == 0) &&
> - (ns->shm_rmid_forced ||
> + (shp->ns->shm_rmid_forced ||
> (shp->shm_perm.mode & SHM_DEST));
> }
>
As written before: what ensures that shp->ns->shm_rmid_forced was not
released already?
> @@ -340,7 +365,7 @@ static void shm_close(struct vm_area_struct *vma)
> ipc_update_pid(&shp->shm_lprid, task_tgid(current));
> shp->shm_dtim = ktime_get_real_seconds();
> shp->shm_nattch--;
> - if (shm_may_destroy(ns, shp))
> + if (shm_may_destroy(shp))
> shm_destroy(ns, shp);
> else
> shm_unlock(shp);
> @@ -361,10 +386,10 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
> *
> * As shp->* are changed under rwsem, it's safe to skip shp locking.
> */
> - if (shp->shm_creator != NULL)
> + if (!list_empty(&shp->shm_clist))
> return 0;
>
This collides with the comment above: here, list_empty() is used.
> - if (shm_may_destroy(ns, shp)) {
> + if (shm_may_destroy(shp)) {
> shm_lock_by_ptr(shp);
> shm_destroy(ns, shp);
> }
> @@ -379,51 +404,77 @@ void shm_destroy_orphaned(struct ipc_namespace *ns)
> up_write(&shm_ids(ns).rwsem);
> }
>
> +void shm_init_task(struct task_struct *task)
> +{
> + INIT_LIST_HEAD(&(task)->sysvshm.shm_clist);
> + spin_lock_init(&task->sysvshm.shm_clist_lock);
> +}
> +
> /* Locking assumes this will only be called with task == current */
> void exit_shm(struct task_struct *task)
> {
> - struct ipc_namespace *ns = task->nsproxy->ipc_ns;
> + LIST_HEAD(tmp);
> struct shmid_kernel *shp, *n;
>
> if (list_empty(&task->sysvshm.shm_clist))
> return;
>
> - /*
> - * If kernel.shm_rmid_forced is not set then only keep track of
> - * which shmids are orphaned, so that a later set of the sysctl
> - * can clean them up.
> - */
> - if (!ns->shm_rmid_forced) {
> - down_read(&shm_ids(ns).rwsem);
> - list_for_each_entry(shp, &task->sysvshm.shm_clist, shm_clist)
> - shp->shm_creator = NULL;
> - /*
> - * Only under read lock but we are only called on current
> - * so no entry on the list will be shared.
> - */
> - list_del(&task->sysvshm.shm_clist);
> - up_read(&shm_ids(ns).rwsem);
> - return;
> - }
> + rcu_read_lock(); /* for refcount_inc_not_zero */
> + task_shm_clist_lock(task);
>
> - /*
> - * Destroy all already created segments, that were not yet mapped,
> - * and mark any mapped as orphan to cover the sysctl toggling.
> - * Destroy is skipped if shm_may_destroy() returns false.
> - */
> - down_write(&shm_ids(ns).rwsem);
> list_for_each_entry_safe(shp, n, &task->sysvshm.shm_clist, shm_clist) {
> + struct ipc_namespace *ns = shp->ns;
> +
> + /*
> + * Remove shm from task list and nullify shm_creator which
> + * marks object as orphaned.
> + *
> + * If kernel.shm_rmid_forced is not set then only keep track of
> + * which shmids are orphaned, so that a later set of the sysctl
> + * can clean them up.
> + */
> + list_del_init(&shp->shm_clist);
> shp->shm_creator = NULL;
>
> - if (shm_may_destroy(ns, shp)) {
> - shm_lock_by_ptr(shp);
> - shm_destroy(ns, shp);
> + printk("exit_shm() %px refcnt=%u, id=%d,key=%x\n", shp,
> + refcount_read(&shp->shm_perm.refcount),
> + shp->shm_perm.id, shp->shm_perm.key
> + );
> +
> + /*
> + * Will destroy all already created segments, that were not yet mapped,
> + * and mark any mapped as orphan to cover the sysctl toggling.
> + * Destroy is skipped if shm_may_destroy() returns false.
> + */
> + if (shp->ns->shm_rmid_forced && shm_may_destroy(shp)) {
> + /*
> + * We may race with shm_exit_ns() if refcounter
> + * already zero. Let's skip shm_destroy() of such
> + * shm object as it will be destroyed during shm_exit_ns()
> + */
> + if (!refcount_inc_not_zero(&ns->ns.count)) /* get_ipc_ns */
> + continue;
> +
> + list_add(&shp->shm_clist, &tmp);
> }
> }
>
> - /* Remove the list head from any segments still attached. */
> list_del(&task->sysvshm.shm_clist);
> - up_write(&shm_ids(ns).rwsem);
> + task_shm_clist_unlock(task);
> + rcu_read_unlock();
> +
> + list_for_each_entry_safe(shp, n, &tmp, shm_clist) {
> + struct ipc_namespace *ns = shp->ns;
> +
> + list_del_init(&shp->shm_clist);
> +
> + down_write(&shm_ids(ns).rwsem);
> + shm_lock_by_ptr(shp);
> + /* will do put_ipc_ns(shp->ns) */
> + shm_destroy(ns, shp);
> + up_write(&shm_ids(ns).rwsem);
> + put_ipc_ns(ns); /* see refcount_inc_not_zero */
> + }
> }
>

I do not see the advantage of first collecting everything in a local
list, and then destroying the elements.

Attached is my current test case. Feel free to merge whatever you
consider as useful into your change.

--

Manfred

[-- Attachment #2: 0003-ipc-shm-locking-questions.patch --]
[-- Type: text/x-patch, Size: 10628 bytes --]

From 7ac17dcf2615f40cef789b203d6f6876038627b6 Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
Date: Thu, 22 Jul 2021 20:19:47 +0200
Subject: [PATCH 3/3] ipc/shm: locking / questions

for discussion, not for merging:
- make locking a bit more explicit
- there is no advantage of a temporary list in exit_shm(),
thus delete segments one by one.
- lots of debug pr_info.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
include/linux/ipc_namespace.h | 16 ++++
ipc/namespace.c | 4 +
ipc/shm.c | 160 +++++++++++++++++++++-------------
3 files changed, 121 insertions(+), 59 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 05e22770af51..f3a09604d0d5 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -126,11 +126,22 @@ extern struct ipc_namespace *copy_ipcs(unsigned long flags,

static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
{
+pr_info("get_ipc_ns: inc for ns %px.\n", ns);
if (ns)
refcount_inc(&ns->ns.count);
return ns;
}

+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+pr_info("get_ipc_ns_not_zero: inc for ns %px.\n", ns);
+ if (ns) {
+ if (refcount_inc_not_zero(&ns->ns.count))
+ return ns;
+ }
+ return NULL;
+}
+
extern void put_ipc_ns(struct ipc_namespace *ns);
#else
static inline struct ipc_namespace *copy_ipcs(unsigned long flags,
@@ -147,6 +158,11 @@ static inline struct ipc_namespace *get_ipc_ns(struct ipc_namespace *ns)
return ns;
}

+static inline struct ipc_namespace *get_ipc_ns_not_zero(struct ipc_namespace *ns)
+{
+ return ns;
+}
+
static inline void put_ipc_ns(struct ipc_namespace *ns)
{
}
diff --git a/ipc/namespace.c b/ipc/namespace.c
index 7bd0766ddc3b..4160ea18dcd2 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -117,6 +117,7 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,

static void free_ipc_ns(struct ipc_namespace *ns)
{
+pr_info("free_ipc_ns: worker task for namespace %px.\n", ns);
/* mq_put_mnt() waits for a grace period as kern_unmount()
* uses synchronize_rcu().
*/
@@ -129,6 +130,7 @@ static void free_ipc_ns(struct ipc_namespace *ns)
put_user_ns(ns->user_ns);
ns_free_inum(&ns->ns);
kfree(ns);
+pr_info("free_ipc_ns: worker task for namespace %px done.\n", ns);
}

static LLIST_HEAD(free_ipc_list);
@@ -164,9 +166,11 @@ static DECLARE_WORK(free_ipc_work, free_ipc);
*/
void put_ipc_ns(struct ipc_namespace *ns)
{
+pr_info("put_ipc_ns: got called for %px.\n", ns);
if (refcount_dec_and_lock(&ns->ns.count, &mq_lock)) {
mq_clear_sbinfo(ns);
spin_unlock(&mq_lock);
+pr_info("put_ipc_ns: destroying namespace %px.\n", ns);

if (llist_add(&ns->mnt_llist, &free_ipc_list))
schedule_work(&free_ipc_work);
diff --git a/ipc/shm.c b/ipc/shm.c
index a746886a0e00..f55118d0a425 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -62,9 +62,14 @@ struct shmid_kernel /* private to the kernel */
struct pid *shm_lprid;
struct ucounts *mlock_ucounts;

- /* The task created the shm object. NULL if the task is dead. */
+ /* The task created the shm object, for looking up
+ * task->sysvshm.shm_clist_lock */
struct task_struct *shm_creator;
- struct list_head shm_clist; /* list by creator */
+
+ /* list by creator. shm_clist_lock required for read/write
+ * if list_empty(), then the creator is dead already
+ */
+ struct list_head shm_clist;
struct ipc_namespace *ns;
} __randomize_layout;

@@ -130,6 +135,7 @@ static void do_shm_rmid(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
#ifdef CONFIG_IPC_NS
void shm_exit_ns(struct ipc_namespace *ns)
{
+pr_info("shm_exit_ns(), %px.\n", ns);
free_ipcs(ns, &shm_ids(ns), do_shm_rmid);
idr_destroy(&ns->ids[IPC_SHM_IDS].ipcs_idr);
rhashtable_destroy(&ns->ids[IPC_SHM_IDS].key_ht);
@@ -237,15 +243,30 @@ static inline void task_shm_clist_unlock(struct task_struct *task)
spin_unlock(&task->sysvshm.shm_clist_lock);
}

-void shm_clist_rm(struct shmid_kernel *shp)
+/* It has to be called with shp locked.
+ * It must be called before ipc_rmid() */
+static inline void shm_clist_rm(struct shmid_kernel *shp)
{
- if (!shp->shm_creator)
+ struct task_struct *creator;
+
+ creator = READ_ONCE(shp->shm_creator);
+ if (!creator) {
+pr_info("shm_clist_rm: creator already NULL for %px.\n", shp);
return;
+ }
+
+ task_shm_clist_lock(creator);
+
+pr_info("shm_clist_rm: creator %px locked for shp %px.\n", creator, shp);

- task_shm_clist_lock(shp->shm_creator);
- list_del_init(&shp->shm_clist);
- task_shm_clist_unlock(shp->shm_creator);
- shp->shm_creator = NULL;
+ /* A concurrent exit_shm may do a list_del_init() as well.
+ * Just do nothing if exit_shm already did the work
+ */
+ if (!list_empty(&shp->shm_clist)) {
+ list_del_init(&shp->shm_clist);
+ WRITE_ONCE(shp->shm_creator, NULL);
+ }
+ task_shm_clist_unlock(creator);
}

static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
@@ -305,6 +326,8 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
{
struct file *shm_file;

+pr_info("shm_destroy: current %px, namespace %px, shp %px.\n", current, ns, shp);
+
shm_file = shp->shm_file;
shp->shm_file = NULL;
ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -333,6 +356,11 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
*/
static bool shm_may_destroy(struct shmid_kernel *shp)
{
+pr_info("shm_may_destroy for current %px, ns %px, shp %px.\n", current, shp->ns, shp);
+
+/* TODO:
+ * understand refcounting for shp->ns: What guarantees that ns cannot go out of scope?
+ */
return (shp->shm_nattch == 0) &&
(shp->ns->shm_rmid_forced ||
(shp->shm_perm.mode & SHM_DEST));
@@ -365,6 +393,7 @@ static void shm_close(struct vm_area_struct *vma)
ipc_update_pid(&shp->shm_lprid, task_tgid(current));
shp->shm_dtim = ktime_get_real_seconds();
shp->shm_nattch--;
+pr_info("shm_close for current %px, ns %px, shp %px.\n", current, shp->ns, shp);
if (shm_may_destroy(shp))
shm_destroy(ns, shp);
else
@@ -413,67 +442,66 @@ void shm_init_task(struct task_struct *task)
/* Locking assumes this will only be called with task == current */
void exit_shm(struct task_struct *task)
{
- LIST_HEAD(tmp);
- struct shmid_kernel *shp, *n;

- if (list_empty(&task->sysvshm.shm_clist))
- return;
+pr_info("exit_shm: called for task %px, current %px\n", task, current);

- rcu_read_lock(); /* for refcount_inc_not_zero */
- task_shm_clist_lock(task);
+ for (;;) {
+ struct shmid_kernel *shp;
+ struct ipc_namespace *ns;

- list_for_each_entry_safe(shp, n, &task->sysvshm.shm_clist, shm_clist) {
- struct ipc_namespace *ns = shp->ns;

- /*
- * Remove shm from task list and nullify shm_creator which
- * marks object as orphaned.
- *
- * If kernel.shm_rmid_forced is not set then only keep track of
- * which shmids are orphaned, so that a later set of the sysctl
- * can clean them up.
- */
- list_del_init(&shp->shm_clist);
- shp->shm_creator = NULL;
+ task_shm_clist_lock(task);
+ if (list_empty(&task->sysvshm.shm_clist)) {
+ task_shm_clist_unlock(task);
+ break;
+ }

- printk("exit_shm() %px refcnt=%u, id=%d,key=%x\n", shp,
- refcount_read(&shp->shm_perm.refcount),
- shp->shm_perm.id, shp->shm_perm.key
- );
+ shp = list_first_entry(&task->sysvshm.shm_clist, struct shmid_kernel,
+ shm_clist);
+
+ /* 1) unlink */
+ list_del_init(&shp->shm_clist);
+ WRITE_ONCE(shp->shm_creator, NULL);

/*
- * Will destroy all already created segments, that were not yet mapped,
- * and mark any mapped as orphan to cover the sysctl toggling.
- * Destroy is skipped if shm_may_destroy() returns false.
+ * 2) get a reference to the namespace.
+ * The refcount could be already 0. If it is 0, then
+ * the shm objects will be free by free_ipc_work().
*/
- if (shp->ns->shm_rmid_forced && shm_may_destroy(shp)) {
+ ns = shp->ns;
+pr_info("exit_shm: called for task %px, current %px, processing shp %px, ns %px\n", task, current, shp, ns);
+
+ ns = get_ipc_ns_not_zero(ns);
+ if (ns) {
/*
- * We may race with shm_exit_ns() if refcounter
- * already zero. Let's skip shm_destroy() of such
- * shm object as it will be destroyed during shm_exit_ns()
+ * 3) get a reference to the shp itself.
+ * This cannot fail: shm_clist_rm() is called before
+ * ipc_rmid(), thus the refcount cannot be 0.
*/
- if (!refcount_inc_not_zero(&ns->ns.count)) /* get_ipc_ns */
- continue;
-
- list_add(&shp->shm_clist, &tmp);
+ ipc_rcu_getref(&shp->shm_perm);
+ }
+ task_shm_clist_unlock(task);
+
+ if (ns) {
+ down_write(&shm_ids(ns).rwsem);
+ shm_lock_by_ptr(shp);
+ ipc_rcu_putref(&shp->shm_perm, shm_rcu_free);
+
+ if (ipc_valid_object(&shp->shm_perm)) {
+ if (shm_may_destroy(shp))
+ shm_destroy(ns, shp);
+ else
+ shm_unlock(shp);
+ } else {
+ /*
+ * Someone else deleted the shp while we have waited.
+ * Just unlock and continue.
+ */
+ shm_unlock(shp);
+ }
+ up_write(&shm_ids(ns).rwsem);
+ put_ipc_ns(ns);
}
- }
-
- list_del(&task->sysvshm.shm_clist);
- task_shm_clist_unlock(task);
- rcu_read_unlock();
-
- list_for_each_entry_safe(shp, n, &tmp, shm_clist) {
- struct ipc_namespace *ns = shp->ns;
-
- list_del_init(&shp->shm_clist);
-
- down_write(&shm_ids(ns).rwsem);
- shm_lock_by_ptr(shp);
- /* will do put_ipc_ns(shp->ns) */
- shm_destroy(ns, shp);
- up_write(&shm_ids(ns).rwsem);
- put_ipc_ns(ns); /* see refcount_inc_not_zero */
}
}

@@ -566,6 +594,8 @@ static int shm_release(struct inode *ino, struct file *file)
{
struct shm_file_data *sfd = shm_file_data(file);

+pr_info("shm_release for current %px.\n", current);
+
put_ipc_ns(sfd->ns);
fput(sfd->file);
shm_file_data(file) = NULL;
@@ -731,9 +761,21 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
if (error < 0)
goto no_id;

- list_add(&shp->shm_clist, &current->sysvshm.shm_clist);
shp->ns = ns;

+ task_shm_clist_lock(current);
+ list_add(&shp->shm_clist, &current->sysvshm.shm_clist);
+
+pr_info("newseg: current %px, namespace %px, shp %px.\n", current, ns, shp);
+ {
+ struct shmid_kernel *shp;
+
+ list_for_each_entry(shp, &current->sysvshm.shm_clist, shm_clist) {
+pr_info("newseg: current %px, list entry %px.\n", current, shp);
+ }
+ }
+ task_shm_clist_unlock(current);
+
/*
* shmid gets reported as "inode#" in /proc/pid/maps.
* proc-ps tools use this. Changing this will break them.
--
2.31.1