* [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-28 3:47 ` Wei Yongjun 0 siblings, 0 replies; 12+ messages in thread From: Wei Yongjun @ 2020-04-28 3:47 UTC (permalink / raw) To: Pankaj Bharadiya, Matthew Wilcox (Oracle), Andrew Morton, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan Cc: Wei Yongjun, linux-kernel, kernel-janitors The function ipc_id_alloc() is called from ipc_addid(), in which a spin lock is held, so we should use GFP_ATOMIC instead. Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- ipc/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipc/util.c b/ipc/util.c index 723dc4b05208..093b31993d39 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -241,7 +241,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) xas.xa_index; xas_store(&xas, new); xas_clear_mark(&xas, XA_FREE_MARK); - } while (__xas_nomem(&xas, GFP_KERNEL)); + } while (__xas_nomem(&xas, GFP_ATOMIC)); xas_unlock(&xas); err = xas_error(&xas); @@ -250,7 +250,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) new->id = get_restore_id(ids); new->seq = ipcid_to_seqx(new->id); idx = ipcid_to_idx(new->id); - err = xa_insert(&ids->ipcs, idx, new, GFP_KERNEL); + err = xa_insert(&ids->ipcs, idx, new, GFP_ATOMIC); if (err == -EBUSY) err = -ENOSPC; set_restore_id(ids, -1); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-28 3:47 ` Wei Yongjun 0 siblings, 0 replies; 12+ messages in thread From: Wei Yongjun @ 2020-04-28 3:47 UTC (permalink / raw) To: Pankaj Bharadiya, Matthew Wilcox (Oracle), Andrew Morton, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan Cc: Wei Yongjun, linux-kernel, kernel-janitors The function ipc_id_alloc() is called from ipc_addid(), in which a spin lock is held, so we should use GFP_ATOMIC instead. Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- ipc/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipc/util.c b/ipc/util.c index 723dc4b05208..093b31993d39 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -241,7 +241,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) xas.xa_index; xas_store(&xas, new); xas_clear_mark(&xas, XA_FREE_MARK); - } while (__xas_nomem(&xas, GFP_KERNEL)); + } while (__xas_nomem(&xas, GFP_ATOMIC)); xas_unlock(&xas); err = xas_error(&xas); @@ -250,7 +250,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) new->id = get_restore_id(ids); new->seq = ipcid_to_seqx(new->id); idx = ipcid_to_idx(new->id); - err = xa_insert(&ids->ipcs, idx, new, GFP_KERNEL); + err = xa_insert(&ids->ipcs, idx, new, GFP_ATOMIC); if (err = -EBUSY) err = -ENOSPC; set_restore_id(ids, -1); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock 2020-04-28 3:47 ` Wei Yongjun @ 2020-04-28 11:14 ` Matthew Wilcox -1 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-28 11:14 UTC (permalink / raw) To: Wei Yongjun Cc: Pankaj Bharadiya, Andrew Morton, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > The function ipc_id_alloc() is called from ipc_addid(), in which > a spin lock is held, so we should use GFP_ATOMIC instead. > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> I see why you think that, but it's not true. Yes, we hold a spinlock, but the spinlock is in an object which is not reachable from any other CPU. So it's not possible to deadlock. This probably confuses all kinds of automated checkers, and I should probably rewrite the code to not acquire the new spinlock until we're already holding the xa_lock. Converting to GFP_ATOMIC is completely wrong. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-28 11:14 ` Matthew Wilcox 0 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-28 11:14 UTC (permalink / raw) To: Wei Yongjun Cc: Pankaj Bharadiya, Andrew Morton, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > The function ipc_id_alloc() is called from ipc_addid(), in which > a spin lock is held, so we should use GFP_ATOMIC instead. > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> I see why you think that, but it's not true. Yes, we hold a spinlock, but the spinlock is in an object which is not reachable from any other CPU. So it's not possible to deadlock. This probably confuses all kinds of automated checkers, and I should probably rewrite the code to not acquire the new spinlock until we're already holding the xa_lock. Converting to GFP_ATOMIC is completely wrong. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock 2020-04-28 11:14 ` Matthew Wilcox @ 2020-04-29 0:14 ` Andrew Morton -1 siblings, 0 replies; 12+ messages in thread From: Andrew Morton @ 2020-04-29 0:14 UTC (permalink / raw) To: Matthew Wilcox Cc: Wei Yongjun, Pankaj Bharadiya, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, 28 Apr 2020 04:14:03 -0700 Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > The function ipc_id_alloc() is called from ipc_addid(), in which > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > the spinlock is in an object which is not reachable from any other CPU. > So it's not possible to deadlock. um, then why are we taking it? > This probably confuses all kinds > of automated checkers, A big fat code comment would reduce the email traffic? > and I should probably rewrite the code to not > acquire the new spinlock until we're already holding the xa_lock. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-29 0:14 ` Andrew Morton 0 siblings, 0 replies; 12+ messages in thread From: Andrew Morton @ 2020-04-29 0:14 UTC (permalink / raw) To: Matthew Wilcox Cc: Wei Yongjun, Pankaj Bharadiya, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, 28 Apr 2020 04:14:03 -0700 Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > The function ipc_id_alloc() is called from ipc_addid(), in which > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > the spinlock is in an object which is not reachable from any other CPU. > So it's not possible to deadlock. um, then why are we taking it? > This probably confuses all kinds > of automated checkers, A big fat code comment would reduce the email traffic? > and I should probably rewrite the code to not > acquire the new spinlock until we're already holding the xa_lock. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock 2020-04-29 0:14 ` Andrew Morton @ 2020-04-29 1:48 ` Matthew Wilcox -1 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-29 1:48 UTC (permalink / raw) To: Andrew Morton Cc: Wei Yongjun, Pankaj Bharadiya, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, Apr 28, 2020 at 05:14:20PM -0700, Andrew Morton wrote: > On Tue, 28 Apr 2020 04:14:03 -0700 Matthew Wilcox <willy@infradead.org> wrote: > > > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > > The function ipc_id_alloc() is called from ipc_addid(), in which > > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > > the spinlock is in an object which is not reachable from any other CPU. > > So it's not possible to deadlock. > > um, then why are we taking it? The lock has to be held by the time 'new' is findable because 'new' is not completely constructed at that point. The finder will try to acquire the spinlock before looking at the uninitialised fields, so it's safe. But it's not a common idiom we use at all. > > This probably confuses all kinds > > of automated checkers, > > A big fat code comment would reduce the email traffic? I think I can rewrite this to take the spinlock after doing the allocation. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-29 1:48 ` Matthew Wilcox 0 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-29 1:48 UTC (permalink / raw) To: Andrew Morton Cc: Wei Yongjun, Pankaj Bharadiya, Waiman Long, Manfred Spraul, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Tue, Apr 28, 2020 at 05:14:20PM -0700, Andrew Morton wrote: > On Tue, 28 Apr 2020 04:14:03 -0700 Matthew Wilcox <willy@infradead.org> wrote: > > > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > > The function ipc_id_alloc() is called from ipc_addid(), in which > > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > > the spinlock is in an object which is not reachable from any other CPU. > > So it's not possible to deadlock. > > um, then why are we taking it? The lock has to be held by the time 'new' is findable because 'new' is not completely constructed at that point. The finder will try to acquire the spinlock before looking at the uninitialised fields, so it's safe. But it's not a common idiom we use at all. > > This probably confuses all kinds > > of automated checkers, > > A big fat code comment would reduce the email traffic? I think I can rewrite this to take the spinlock after doing the allocation. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock 2020-04-28 11:14 ` Matthew Wilcox @ 2020-04-29 5:22 ` Manfred Spraul -1 siblings, 0 replies; 12+ messages in thread From: Manfred Spraul @ 2020-04-29 5:22 UTC (permalink / raw) To: Matthew Wilcox, Wei Yongjun Cc: Pankaj Bharadiya, Andrew Morton, Waiman Long, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors Hello together, On 4/28/20 1:14 PM, Matthew Wilcox wrote: > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: >> The function ipc_id_alloc() is called from ipc_addid(), in which >> a spin lock is held, so we should use GFP_ATOMIC instead. >> >> Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > I see why you think that, but it's not true. Yes, we hold a spinlock, but > the spinlock is in an object which is not reachable from any other CPU. Is it really allowed that spin_lock()/spin_unlock may happen on different cpus? CPU1: spin_lock() CPU1: schedule() -> sleeps CPU2: -> schedule() returns CPU2: spin_unlock(). > Converting to GFP_ATOMIC is completely wrong. What is your solution proposal? xa_store() also gets a gfp_ flag. Thus even splitting _alloc() and _store() won't help xa_alloc(,entry=NULL,) new->seq = ... spin_lock(); xa_store(,entry=new,GFP_KERNEL); -- Manfred ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-29 5:22 ` Manfred Spraul 0 siblings, 0 replies; 12+ messages in thread From: Manfred Spraul @ 2020-04-29 5:22 UTC (permalink / raw) To: Matthew Wilcox, Wei Yongjun Cc: Pankaj Bharadiya, Andrew Morton, Waiman Long, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors Hello together, On 4/28/20 1:14 PM, Matthew Wilcox wrote: > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: >> The function ipc_id_alloc() is called from ipc_addid(), in which >> a spin lock is held, so we should use GFP_ATOMIC instead. >> >> Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > I see why you think that, but it's not true. Yes, we hold a spinlock, but > the spinlock is in an object which is not reachable from any other CPU. Is it really allowed that spin_lock()/spin_unlock may happen on different cpus? CPU1: spin_lock() CPU1: schedule() -> sleeps CPU2: -> schedule() returns CPU2: spin_unlock(). > Converting to GFP_ATOMIC is completely wrong. What is your solution proposal? xa_store() also gets a gfp_ flag. Thus even splitting _alloc() and _store() won't help xa_alloc(,entry=NULL,) new->seq = ... spin_lock(); xa_store(,entry=new,GFP_KERNEL); -- Manfred ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock 2020-04-29 5:22 ` Manfred Spraul @ 2020-04-29 13:08 ` Matthew Wilcox -1 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-29 13:08 UTC (permalink / raw) To: Manfred Spraul Cc: Wei Yongjun, Pankaj Bharadiya, Andrew Morton, Waiman Long, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Wed, Apr 29, 2020 at 07:22:13AM +0200, Manfred Spraul wrote: > Hello together, > > On 4/28/20 1:14 PM, Matthew Wilcox wrote: > > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > > The function ipc_id_alloc() is called from ipc_addid(), in which > > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > > the spinlock is in an object which is not reachable from any other CPU. > > Is it really allowed that spin_lock()/spin_unlock may happen on different > cpus? > > CPU1: spin_lock() > > CPU1: schedule() -> sleeps > > CPU2: -> schedule() returns > > CPU2: spin_unlock(). I think that is allowed, but I'm not an expert in the implementations. > > Converting to GFP_ATOMIC is completely wrong. > > What is your solution proposal? > > xa_store() also gets a gfp_ flag. Thus even splitting _alloc() and _store() > won't help > > xa_alloc(,entry=NULL,) > new->seq = ... > spin_lock(); > xa_store(,entry=new,GFP_KERNEL); While it takes GFP flags, it won't do any allocation if it's overwriting an allocated entry. diff --git a/ipc/util.c b/ipc/util.c index 0f6b0e0aa17e..b929ab0072a5 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -19,8 +19,8 @@ * * General sysv ipc locking scheme: * rcu_read_lock() - * obtain the ipc object (kern_ipc_perm) by looking up the id in an idr - * tree. + * obtain the ipc object (kern_ipc_perm) by looking up the id in an + * xarray. * - perform initial checks (capabilities, auditing and permission, * etc). * - perform read-only operations, such as INFO command, that @@ -209,14 +209,14 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) u32 idx; int err; + xa_lock(&ids->ipcs); + if (get_restore_id(ids) < 0) { int max_idx; max_idx = max(ids->in_use*3/2, ipc_min_cycle); max_idx = min(max_idx, ipc_mni) - 1; - xa_lock(&ids->ipcs); - err = __xa_alloc_cyclic(&ids->ipcs, &idx, NULL, XA_LIMIT(0, max_idx), &ids->next_idx, GFP_KERNEL); @@ -224,24 +224,31 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) ids->seq++; if (ids->seq >= ipcid_seq_max()) ids->seq = 0; + err = 0; } - if (err >= 0) { + if (!err) { new->seq = ids->seq; new->id = (new->seq << ipcmni_seq_shift()) + idx; - /* xa_store contains a write barrier */ - __xa_store(&ids->ipcs, idx, new, GFP_KERNEL); } - - xa_unlock(&ids->ipcs); } else { new->id = get_restore_id(ids); new->seq = ipcid_to_seqx(new->id); idx = ipcid_to_idx(new->id); - err = xa_insert(&ids->ipcs, idx, new, GFP_KERNEL); + err = __xa_insert(&ids->ipcs, idx, NULL, GFP_KERNEL); set_restore_id(ids, -1); } + /* + * Hold the spinlock so that nobody else can access the object + * once they can find it. xa_store contains a write barrier so + * all previous stores to 'new' will be visible. + */ + spin_lock(&new->lock); + if (!err) + __xa_store(&ids->ipcs, idx, new, GFP_NOWAIT); + xa_unlock(&ids->ipcs); + if (err == -EBUSY) return -ENOSPC; if (err < 0) @@ -255,7 +262,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) * @new: new ipc permission set * @limit: limit for the number of used ids * - * Add an entry 'new' to the ipc ids idr. The permissions object is + * Add an entry 'new' to the ipc ids. The permissions object is * initialised and the first free entry is set up and the index assigned * is returned. The 'new' entry is returned in a locked state on success. * @@ -270,7 +277,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) kgid_t egid; int idx; - /* 1) Initialize the refcount so that ipc_rcu_putref works */ + /* Initialize the refcount so that ipc_rcu_putref works */ refcount_set(&new->refcount, 1); if (limit > ipc_mni) @@ -279,12 +286,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) if (ids->in_use >= limit) return -ENOSPC; - /* - * 2) Hold the spinlock so that nobody else can access the object - * once they can find it - */ spin_lock_init(&new->lock); - spin_lock(&new->lock); current_euid_egid(&euid, &egid); new->cuid = new->uid = euid; new->gid = new->cgid = egid; @@ -588,7 +590,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out) * @ids: ipc identifier set * @id: ipc id to look for * - * Look for an id in the ipc ids idr and return associated ipc object. + * Look for an id in the ipc ids and return associated ipc object. * * Call inside the RCU critical section. * The ipc object is *not* locked on exit. ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH -next] ipc: use GFP_ATOMIC under spin lock @ 2020-04-29 13:08 ` Matthew Wilcox 0 siblings, 0 replies; 12+ messages in thread From: Matthew Wilcox @ 2020-04-29 13:08 UTC (permalink / raw) To: Manfred Spraul Cc: Wei Yongjun, Pankaj Bharadiya, Andrew Morton, Waiman Long, Stephen Rothwell, Alexey Dobriyan, linux-kernel, kernel-janitors On Wed, Apr 29, 2020 at 07:22:13AM +0200, Manfred Spraul wrote: > Hello together, > > On 4/28/20 1:14 PM, Matthew Wilcox wrote: > > On Tue, Apr 28, 2020 at 03:47:36AM +0000, Wei Yongjun wrote: > > > The function ipc_id_alloc() is called from ipc_addid(), in which > > > a spin lock is held, so we should use GFP_ATOMIC instead. > > > > > > Fixes: de5738d1c364 ("ipc: convert ipcs_idr to XArray") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > I see why you think that, but it's not true. Yes, we hold a spinlock, but > > the spinlock is in an object which is not reachable from any other CPU. > > Is it really allowed that spin_lock()/spin_unlock may happen on different > cpus? > > CPU1: spin_lock() > > CPU1: schedule() -> sleeps > > CPU2: -> schedule() returns > > CPU2: spin_unlock(). I think that is allowed, but I'm not an expert in the implementations. > > Converting to GFP_ATOMIC is completely wrong. > > What is your solution proposal? > > xa_store() also gets a gfp_ flag. Thus even splitting _alloc() and _store() > won't help > > xa_alloc(,entry=NULL,) > new->seq = ... > spin_lock(); > xa_store(,entry=new,GFP_KERNEL); While it takes GFP flags, it won't do any allocation if it's overwriting an allocated entry. diff --git a/ipc/util.c b/ipc/util.c index 0f6b0e0aa17e..b929ab0072a5 100644 --- a/ipc/util.c +++ b/ipc/util.c @@ -19,8 +19,8 @@ * * General sysv ipc locking scheme: * rcu_read_lock() - * obtain the ipc object (kern_ipc_perm) by looking up the id in an idr - * tree. + * obtain the ipc object (kern_ipc_perm) by looking up the id in an + * xarray. * - perform initial checks (capabilities, auditing and permission, * etc). * - perform read-only operations, such as INFO command, that @@ -209,14 +209,14 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) u32 idx; int err; + xa_lock(&ids->ipcs); + if (get_restore_id(ids) < 0) { int max_idx; max_idx = max(ids->in_use*3/2, ipc_min_cycle); max_idx = min(max_idx, ipc_mni) - 1; - xa_lock(&ids->ipcs); - err = __xa_alloc_cyclic(&ids->ipcs, &idx, NULL, XA_LIMIT(0, max_idx), &ids->next_idx, GFP_KERNEL); @@ -224,24 +224,31 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) ids->seq++; if (ids->seq >= ipcid_seq_max()) ids->seq = 0; + err = 0; } - if (err >= 0) { + if (!err) { new->seq = ids->seq; new->id = (new->seq << ipcmni_seq_shift()) + idx; - /* xa_store contains a write barrier */ - __xa_store(&ids->ipcs, idx, new, GFP_KERNEL); } - - xa_unlock(&ids->ipcs); } else { new->id = get_restore_id(ids); new->seq = ipcid_to_seqx(new->id); idx = ipcid_to_idx(new->id); - err = xa_insert(&ids->ipcs, idx, new, GFP_KERNEL); + err = __xa_insert(&ids->ipcs, idx, NULL, GFP_KERNEL); set_restore_id(ids, -1); } + /* + * Hold the spinlock so that nobody else can access the object + * once they can find it. xa_store contains a write barrier so + * all previous stores to 'new' will be visible. + */ + spin_lock(&new->lock); + if (!err) + __xa_store(&ids->ipcs, idx, new, GFP_NOWAIT); + xa_unlock(&ids->ipcs); + if (err = -EBUSY) return -ENOSPC; if (err < 0) @@ -255,7 +262,7 @@ static inline int ipc_id_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new) * @new: new ipc permission set * @limit: limit for the number of used ids * - * Add an entry 'new' to the ipc ids idr. The permissions object is + * Add an entry 'new' to the ipc ids. The permissions object is * initialised and the first free entry is set up and the index assigned * is returned. The 'new' entry is returned in a locked state on success. * @@ -270,7 +277,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) kgid_t egid; int idx; - /* 1) Initialize the refcount so that ipc_rcu_putref works */ + /* Initialize the refcount so that ipc_rcu_putref works */ refcount_set(&new->refcount, 1); if (limit > ipc_mni) @@ -279,12 +286,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit) if (ids->in_use >= limit) return -ENOSPC; - /* - * 2) Hold the spinlock so that nobody else can access the object - * once they can find it - */ spin_lock_init(&new->lock); - spin_lock(&new->lock); current_euid_egid(&euid, &egid); new->cuid = new->uid = euid; new->gid = new->cgid = egid; @@ -588,7 +590,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out) * @ids: ipc identifier set * @id: ipc id to look for * - * Look for an id in the ipc ids idr and return associated ipc object. + * Look for an id in the ipc ids and return associated ipc object. * * Call inside the RCU critical section. * The ipc object is *not* locked on exit. ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-29 13:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 3:47 [PATCH -next] ipc: use GFP_ATOMIC under spin lock Wei Yongjun 2020-04-28 3:47 ` Wei Yongjun 2020-04-28 11:14 ` Matthew Wilcox 2020-04-28 11:14 ` Matthew Wilcox 2020-04-29 0:14 ` Andrew Morton 2020-04-29 0:14 ` Andrew Morton 2020-04-29 1:48 ` Matthew Wilcox 2020-04-29 1:48 ` Matthew Wilcox 2020-04-29 5:22 ` Manfred Spraul 2020-04-29 5:22 ` Manfred Spraul 2020-04-29 13:08 ` Matthew Wilcox 2020-04-29 13:08 ` Matthew Wilcox
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.