Messages in this thread | | | From | Alexander Mihalicyn <> | Date | Fri, 30 Jul 2021 17:52:40 +0300 | Subject | Re: [RFC PATCH] shm: extend forced shm destroy to support objects from several IPC nses |
| |
Hi, Manfred,
I'm sorry for the long delay with the answer. Bug hunting season is open, so I tried to catch another one last week :)
I will return to work on that problem next week.
Thank you very much for your review, suggestions and fixes. I will take your fixes and, if you allow, add your Co-developed-by tag ;)
I've left some comments below.
On Thu, Jul 22, 2021 at 9:46 PM Manfred Spraul <manfred@colorfullife.com> wrote: > > 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.
Yep, it's leftover from debugging.
> > 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?
As far as I understand, if we have locked struct shmid_kernel it means that someone (task) holds IPC namespace. But I will check and describe this.
> > @@ -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.
Ah, I've got your idea. You lock the list inside a loop over the list and ensure that it's not empty and it allows you to split spin_lock and rwsem taking. I've tried to do the same (split locks) but using a temporary list head. :) Thanks!
> > Attached is my current test case. Feel free to merge whatever you > consider as useful into your change. > > > -- > > Manfred >
Regards, Alex
| |