Messages in this thread | | | Date | Sat, 6 Nov 2021 08:50:39 +0100 | Subject | Re: [RFC] shm: extend forced shm destroy to support objects from several IPC nses (simplified) | From | Manfred Spraul <> |
| |
Hi Eric,
On 11/5/21 22:34, Eric W. Biederman wrote: > I have to dash so this is short.
As last time, I'll review the change and check for new/good ideas.
As first question: Is the change tested?
[...]
> > /* The task created the shm object. NULL if the task is dead. */ > - struct task_struct *shm_creator; > + struct task_struct __rcu *shm_creator; > struct list_head shm_clist; /* list by creator */ > + struct ipc_namespace *shm_ns; /* valid when shm_nattch != 0 */ > } __randomize_layout; > There is no reason to modify shm_creator:
We need _one_ indicator that the creator has died, not two.
We have both list_empty() and shm_creator. Thus we should/must define what is the relevant indicator, and every function must use the same one.
exit_sem() must walk shm_clist. list_empty() must return the correct answer.
Thus I think it is simpler that list_empty() is the indicator.
In addition, as you have correctly noticed: If we make shm_creator==NULL the indicator, then we must use at __rcu or at least READ_ONCE() accessors.
But: This would only solve a self created problem. Just leave shm_creator unmodified - and the need for READ_ONCE() goes away.
> /* shm_mode upper byte flags */ > @@ -106,29 +107,17 @@ void shm_init_ns(struct ipc_namespace *ns) > ipc_init_ids(&shm_ids(ns)); > } > > -/* > - * Called with shm_ids.rwsem (writer) and the shp structure locked. > - * Only shm_ids.rwsem remains locked on exit. > - */ > -static void do_shm_rmid(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) > +static void do_shm_destroy(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp) > { > - struct shmid_kernel *shp; > - > - shp = container_of(ipcp, struct shmid_kernel, shm_perm); > - > - if (shp->shm_nattch) { > - shp->shm_perm.mode |= SHM_DEST; > - /* Do not find it any more */ > - ipc_set_key_private(&shm_ids(ns), &shp->shm_perm); > - shm_unlock(shp); > - } else > - shm_destroy(ns, shp); > + struct shmid_kernel *shp = > + container_of(ipcp, struct shmid_kernel, shm_perm); > + shm_destroy(ns, shp); > } > > #ifdef CONFIG_IPC_NS > void shm_exit_ns(struct ipc_namespace *ns) > { > - free_ipcs(ns, &shm_ids(ns), do_shm_rmid); > + free_ipcs(ns, &shm_ids(ns), do_shm_destroy); > idr_destroy(&ns->ids[IPC_SHM_IDS].ipcs_idr); > rhashtable_destroy(&ns->ids[IPC_SHM_IDS].key_ht); > } > @@ -225,9 +214,22 @@ static void shm_rcu_free(struct rcu_head *head) > kfree(shp); > } > > +static inline void shm_clist_del(struct shmid_kernel *shp) > +{ > + struct task_struct *creator; > + > + rcu_read_lock(); > + creator = rcu_dereference(shp->shm_creator); > + if (creator) { > + task_lock(creator); > + list_del(&shp->shm_clist);
Does this work? You are using list_del() instead of list_del_init().
I fear that this might break exit_sem()
> + task_unlock(creator); > + } > + rcu_read_unlock(); > +} > + > static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s) > { > - list_del(&s->shm_clist); > ipc_rmid(&shm_ids(ns), &s->shm_perm); > } > > @@ -283,7 +285,9 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp) > shm_file = shp->shm_file; > shp->shm_file = NULL; > ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT; > + shm_clist_del(shp); > shm_rmid(ns, shp); > + shp->shm_ns = NULL; > shm_unlock(shp); > if (!is_file_hugepages(shm_file)) > shmem_lock(shm_file, 0, shp->mlock_ucounts); > @@ -361,7 +365,7 @@ 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 (rcu_access_pointer(shp->shm_creator) != NULL) > return 0; > > if (shm_may_destroy(ns, shp)) { > @@ -382,48 +386,62 @@ void shm_destroy_orphaned(struct ipc_namespace *ns) > /* 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; > - 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; > - } > + struct list_head *head = &task->sysvshm.shm_clist; > > /* > * 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) { > - shp->shm_creator = NULL; > + for (;;) { > + struct ipc_namespace *ns; > + struct shmid_kernel *shp; > > - if (shm_may_destroy(ns, shp)) { > + task_lock(task); > + if (list_empty(head)) { > + task_unlock(task); > + break; > + } > + > + shp = list_first_entry(head, struct shmid_kernel, shm_clist); > + > + list_del(&shp->shm_clist); > + rcu_assign_pointer(shp->shm_creator, NULL); > + > + /* > + * Guarantee that ns lives after task_list is dropped. > + * > + * This shm segment may not be attached and it's ipc > + * namespace may be exiting. If so ignore the shm > + * segment as it will be destroyed by shm_exit_ns. > + */ > + ns = get_ipc_ns_not_zero(shp->shm_ns); > + if (!ns) { > + task_unlock(task); > + continue; > + } > + > + /* Guarantee shp lives after task_lock is dropped */ > + ipc_getref(&shp->shm_perm); > + > + /* Drop task_lock so that shm_destroy may take it */ > + task_unlock(task); > + > + /* Can the shm segment be destroyed? */ > + down_write(&shm_ids(ns).rwsem); > + shm_lock_by_ptr(shp); > + if (ipc_valid_object(&shp->shm_perm) && > + shm_may_destroy(ns, shp)) { > shm_lock_by_ptr(shp); > shm_destroy(ns, shp); > + } else { > + shm_unlock(shp); > } > - } > > - /* Remove the list head from any segments still attached. */ > - list_del(&task->sysvshm.shm_clist); > - up_write(&shm_ids(ns).rwsem); > + ipc_rcu_putref(&shp->shm_perm, shm_rcu_free); > + up_write(&shm_ids(ns).rwsem); > + put_ipc_ns(ns); > + } > } > > static vm_fault_t shm_fault(struct vm_fault *vmf) > @@ -673,14 +691,17 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params) > shp->shm_segsz = size; > shp->shm_nattch = 0; > shp->shm_file = file; > - shp->shm_creator = current; > + RCU_INIT_POINTER(shp->shm_creator, current); > + shp->shm_ns = ns; > > /* ipc_addid() locks shp upon success. */ > error = ipc_addid(&shm_ids(ns), &shp->shm_perm, ns->shm_ctlmni); > if (error < 0) > goto no_id; > > + task_lock(current); > list_add(&shp->shm_clist, ¤t->sysvshm.shm_clist); > + task_unlock(current); > > /* > * shmid gets reported as "inode#" in /proc/pid/maps. > @@ -913,8 +934,14 @@ static int shmctl_down(struct ipc_namespace *ns, int shmid, int cmd, > switch (cmd) { > case IPC_RMID: > ipc_lock_object(&shp->shm_perm); > - /* do_shm_rmid unlocks the ipc object and rcu */ > - do_shm_rmid(ns, ipcp); > + if (shp->shm_nattch) { > + shp->shm_perm.mode |= SHM_DEST; > + /* Do not find it any more */ > + ipc_set_key_private(&shm_ids(ns), &shp->shm_perm); > + shm_unlock(shp); > + } else > + shm_destroy(ns, shp); > + /* shm_unlock unlocked the ipc object and rcu */ > goto out_up; > case IPC_SET: > ipc_lock_object(&shp->shm_perm); > diff --git a/ipc/util.c b/ipc/util.c > index fa2d86ef3fb8..58228f342397 100644 > --- a/ipc/util.c > +++ b/ipc/util.c > @@ -525,6 +525,11 @@ void ipc_set_key_private(struct ipc_ids *ids, struct kern_ipc_perm *ipcp) > ipcp->key = IPC_PRIVATE; > } > > +void ipc_getref(struct kern_ipc_perm *ptr) > +{ > + return refcount_inc(&ptr->refcount); > +} > + > bool ipc_rcu_getref(struct kern_ipc_perm *ptr) > { > return refcount_inc_not_zero(&ptr->refcount); > diff --git a/ipc/util.h b/ipc/util.h > index 2dd7ce0416d8..e13b46ff675f 100644 > --- a/ipc/util.h > +++ b/ipc/util.h > @@ -170,6 +170,7 @@ static inline int ipc_get_maxidx(struct ipc_ids *ids) > * refcount is initialized by ipc_addid(), before that point call_rcu() > * must be used. > */ > +void ipc_getref(struct kern_ipc_perm *ptr); > bool ipc_rcu_getref(struct kern_ipc_perm *ptr); > void ipc_rcu_putref(struct kern_ipc_perm *ptr, > void (*func)(struct rcu_head *head)); > diff --git a/kernel/fork.c b/kernel/fork.c > index 38681ad44c76..3e881f78bcf2 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -3095,7 +3095,6 @@ int ksys_unshare(unsigned long unshare_flags) > if (unshare_flags & CLONE_NEWIPC) { > /* Orphan segments in old ns (see sem above). */ > exit_shm(current); > - shm_init_task(current); > } > > if (new_nsproxy)
| |