All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.