Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Sun, 23 Nov 2014 19:23:53 +0100 Manfred Spraul wrote: > Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it > visible > > ipc_addid() makes a new ipc identifier visible to everyone. > New objects start as locked, so that the caller can complete > the initialization after the call. > Within struct sem_array, at least sma->sem_base and sma->sem_nsems > are accessed without any locks, therefore this approach doesn't work. > > Thus: Move the ipc_addid() to the end of the initialization. Any thoughts on which kernel version(s) need the patch? I'm still rather fuzzy on the end-user impact of this bug. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Sun, Nov 23, 2014 at 01:36:51PM -0800, Davidlohr Bueso wrote: > On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote: > > On 11/23/2014 01:23 PM, Manfred Spraul wrote: > > > Hi Rik, > > > > > > On 11/21/2014 08:52 PM, Rik van Riel wrote: > > >> When manipulating just one semaphore with semop, sem_lock only > > >> takes that single semaphore's lock. This creates a problem during > > >> initialization of the semaphore array, when the data structures > > >> used by sem_lock have not been set up yet. The sma->lock is > > >> already held by newary, and we just have to make sure everything > > >> else waits on that lock during initialization. > > >> > > >> Luckily it is easy to make sem_lock wait on the sma->lock, by > > >> pretending there is a complex operation in progress while the sma > > >> is being initialized. > > > That's not sufficient, as sma->sem_nsems is accessed before > > > calling sem_lock(), both within find_alloc_undo() and within > > > semtimedop(). > > > > > > The root problem is that sma->sem_nsems and sma->sem_base are > > > accessed without any locks, this conflicts with the approach that > > > sma starts to exist as not yet initialized but locked and is > > > unlocked after the initialization is completed. > > > > > > Attached is an idea. It did pass a few short tests. What do you > > > think? > > > > This was my other idea for fixing the issue; unfortunately > > I didn't think of it until after I sent the first patch :) > > Yep, this is what I was mentioning as well. > > > You are right that without that change, we can return the > > wrong error codes to userspace. > > > > I will give the patch a try, though I have so far been unable > > to reproduce the bug that the customer reported, so I am unlikely > > to give much in the way of useful testing results... > > > > Andrew, feel free to give Manfred's patch my > > > > Acked-by: Rik van Riel > > Acked-by: Davidlohr Bueso > Acked-by: Rafael Aquini -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Sun, Nov 23, 2014 at 01:36:51PM -0800, Davidlohr Bueso wrote: On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote: On 11/23/2014 01:23 PM, Manfred Spraul wrote: Hi Rik, On 11/21/2014 08:52 PM, Rik van Riel wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. That's not sufficient, as sma-sem_nsems is accessed before calling sem_lock(), both within find_alloc_undo() and within semtimedop(). The root problem is that sma-sem_nsems and sma-sem_base are accessed without any locks, this conflicts with the approach that sma starts to exist as not yet initialized but locked and is unlocked after the initialization is completed. Attached is an idea. It did pass a few short tests. What do you think? This was my other idea for fixing the issue; unfortunately I didn't think of it until after I sent the first patch :) Yep, this is what I was mentioning as well. You are right that without that change, we can return the wrong error codes to userspace. I will give the patch a try, though I have so far been unable to reproduce the bug that the customer reported, so I am unlikely to give much in the way of useful testing results... Andrew, feel free to give Manfred's patch my Acked-by: Rik van Riel r...@redhat.com Acked-by: Davidlohr Bueso d...@stgolabs.net Acked-by: Rafael Aquini aqu...@redhat.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Sun, 23 Nov 2014 19:23:53 +0100 Manfred Spraul manf...@colorfullife.com wrote: Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it visible ipc_addid() makes a new ipc identifier visible to everyone. New objects start as locked, so that the caller can complete the initialization after the call. Within struct sem_array, at least sma-sem_base and sma-sem_nsems are accessed without any locks, therefore this approach doesn't work. Thus: Move the ipc_addid() to the end of the initialization. Any thoughts on which kernel version(s) need the patch? I'm still rather fuzzy on the end-user impact of this bug. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote: > On 11/23/2014 01:23 PM, Manfred Spraul wrote: > > Hi Rik, > > > > On 11/21/2014 08:52 PM, Rik van Riel wrote: > >> When manipulating just one semaphore with semop, sem_lock only > >> takes that single semaphore's lock. This creates a problem during > >> initialization of the semaphore array, when the data structures > >> used by sem_lock have not been set up yet. The sma->lock is > >> already held by newary, and we just have to make sure everything > >> else waits on that lock during initialization. > >> > >> Luckily it is easy to make sem_lock wait on the sma->lock, by > >> pretending there is a complex operation in progress while the sma > >> is being initialized. > > That's not sufficient, as sma->sem_nsems is accessed before > > calling sem_lock(), both within find_alloc_undo() and within > > semtimedop(). > > > > The root problem is that sma->sem_nsems and sma->sem_base are > > accessed without any locks, this conflicts with the approach that > > sma starts to exist as not yet initialized but locked and is > > unlocked after the initialization is completed. > > > > Attached is an idea. It did pass a few short tests. What do you > > think? > > This was my other idea for fixing the issue; unfortunately > I didn't think of it until after I sent the first patch :) Yep, this is what I was mentioning as well. > You are right that without that change, we can return the > wrong error codes to userspace. > > I will give the patch a try, though I have so far been unable > to reproduce the bug that the customer reported, so I am unlikely > to give much in the way of useful testing results... > > Andrew, feel free to give Manfred's patch my > > Acked-by: Rik van Riel Acked-by: Davidlohr Bueso -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/23/2014 01:23 PM, Manfred Spraul wrote: > Hi Rik, > > On 11/21/2014 08:52 PM, Rik van Riel wrote: >> When manipulating just one semaphore with semop, sem_lock only >> takes that single semaphore's lock. This creates a problem during >> initialization of the semaphore array, when the data structures >> used by sem_lock have not been set up yet. The sma->lock is >> already held by newary, and we just have to make sure everything >> else waits on that lock during initialization. >> >> Luckily it is easy to make sem_lock wait on the sma->lock, by >> pretending there is a complex operation in progress while the sma >> is being initialized. > That's not sufficient, as sma->sem_nsems is accessed before > calling sem_lock(), both within find_alloc_undo() and within > semtimedop(). > > The root problem is that sma->sem_nsems and sma->sem_base are > accessed without any locks, this conflicts with the approach that > sma starts to exist as not yet initialized but locked and is > unlocked after the initialization is completed. > > Attached is an idea. It did pass a few short tests. What do you > think? This was my other idea for fixing the issue; unfortunately I didn't think of it until after I sent the first patch :) You are right that without that change, we can return the wrong error codes to userspace. I will give the patch a try, though I have so far been unable to reproduce the bug that the customer reported, so I am unlikely to give much in the way of useful testing results... Andrew, feel free to give Manfred's patch my Acked-by: Rik van Riel It closes off more things than my patch did, in a similar way (doing something before ipc_addid) - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUckuKAAoJEM553pKExN6DfRoH/jjcBpLwKzrId8aFtuSWYgv3 I7LNzoGozb6Lvn1D1lt/sREdl74KIQptaVClQzphIubiAkQPJVgzqe43f01mqaPu mSjGKarjfKXwUNfa+Q3pW7b2hnbdfTfyQDsTr6JhSpy0ynsmmNb1J2S7E2E2B40r KbOETKeKRTq4vYCk4Em5S4OJg31AoXNoXMVwciOB0M/QDFCSB+4JdKrRsiz6Hm1o +JlRtgwUY6m047Ur65MKeN1bkx1cgqK8tzayMpXW+PTI9cVs6153tlWXmQoXP+Co lHrODmKkhOhJfOr3q0YvrwnaZOVKliKLVV0YkQ+6pi4SAqSjH7pV0jWr/FsaLIA= =u6vO -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
Hi Rik,
On 11/21/2014 08:52 PM, Rik van Riel wrote:
When manipulating just one semaphore with semop, sem_lock only takes that
single semaphore's lock. This creates a problem during initialization of
the semaphore array, when the data structures used by sem_lock have not
been set up yet. The sma->lock is already held by newary, and we just
have to make sure everything else waits on that lock during initialization.
Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
there is a complex operation in progress while the sma is being initialized.
That's not sufficient, as sma->sem_nsems is accessed before calling
sem_lock(), both within find_alloc_undo() and within semtimedop().
The root problem is that sma->sem_nsems and sma->sem_base are accessed
without any locks, this conflicts with the approach that sma starts to
exist as not yet initialized but locked and is unlocked after the
initialization is completed.
Attached is an idea. It did pass a few short tests.
What do you think?
With regards to affected kernels:
- wrong -EFBIG are possible since 3.10 (test for sma->sem_nsems moved
before taking the lock)
- kernel memory corruptions with 0-sized undo buffer allocation is
possible since 3.10, too.
(sem_lock before accessing sma->sem_nsems replaced with
sem_obtain_object_check).
--
Manfred
>From fa928cdd6b5e032006f100f9689a5a4167c086e8 Mon Sep 17 00:00:00 2001
From: Manfred Spraul
Date: Sun, 23 Nov 2014 19:08:57 +0100
Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it
visible
ipc_addid() makes a new ipc identifier visible to everyone.
New objects start as locked, so that the caller can complete
the initialization after the call.
Within struct sem_array, at least sma->sem_base and sma->sem_nsems
are accessed without any locks, therefore this approach doesn't work.
Thus: Move the ipc_addid() to the end of the initialization.
Reported-by: Rik van Riel
Signed-off-by: Manfred Spraul
---
ipc/sem.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..53c3310 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -507,13 +507,6 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
return retval;
}
- id = ipc_addid(_ids(ns), >sem_perm, ns->sc_semmni);
- if (id < 0) {
- ipc_rcu_putref(sma, sem_rcu_free);
- return id;
- }
- ns->used_sems += nsems;
-
sma->sem_base = (struct sem *) [1];
for (i = 0; i < nsems; i++) {
@@ -528,6 +521,14 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
INIT_LIST_HEAD(>list_id);
sma->sem_nsems = nsems;
sma->sem_ctime = get_seconds();
+
+ id = ipc_addid(_ids(ns), >sem_perm, ns->sc_semmni);
+ if (id < 0) {
+ ipc_rcu_putref(sma, sem_rcu_free);
+ return id;
+ }
+ ns->used_sems += nsems;
+
sem_unlock(sma, -1);
rcu_read_unlock();
--
1.9.3
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
Hi Rik,
On 11/21/2014 08:52 PM, Rik van Riel wrote:
When manipulating just one semaphore with semop, sem_lock only takes that
single semaphore's lock. This creates a problem during initialization of
the semaphore array, when the data structures used by sem_lock have not
been set up yet. The sma-lock is already held by newary, and we just
have to make sure everything else waits on that lock during initialization.
Luckily it is easy to make sem_lock wait on the sma-lock, by pretending
there is a complex operation in progress while the sma is being initialized.
That's not sufficient, as sma-sem_nsems is accessed before calling
sem_lock(), both within find_alloc_undo() and within semtimedop().
The root problem is that sma-sem_nsems and sma-sem_base are accessed
without any locks, this conflicts with the approach that sma starts to
exist as not yet initialized but locked and is unlocked after the
initialization is completed.
Attached is an idea. It did pass a few short tests.
What do you think?
With regards to affected kernels:
- wrong -EFBIG are possible since 3.10 (test for sma-sem_nsems moved
before taking the lock)
- kernel memory corruptions with 0-sized undo buffer allocation is
possible since 3.10, too.
(sem_lock before accessing sma-sem_nsems replaced with
sem_obtain_object_check).
--
Manfred
From fa928cdd6b5e032006f100f9689a5a4167c086e8 Mon Sep 17 00:00:00 2001
From: Manfred Spraul manf...@colorfullife.com
Date: Sun, 23 Nov 2014 19:08:57 +0100
Subject: [PATCH] ipc/sem.c: Fully initialize sem_array before making it
visible
ipc_addid() makes a new ipc identifier visible to everyone.
New objects start as locked, so that the caller can complete
the initialization after the call.
Within struct sem_array, at least sma-sem_base and sma-sem_nsems
are accessed without any locks, therefore this approach doesn't work.
Thus: Move the ipc_addid() to the end of the initialization.
Reported-by: Rik van Riel r...@redhat.com
Signed-off-by: Manfred Spraul manf...@colorfullife.com
---
ipc/sem.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..53c3310 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -507,13 +507,6 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
return retval;
}
- id = ipc_addid(sem_ids(ns), sma-sem_perm, ns-sc_semmni);
- if (id 0) {
- ipc_rcu_putref(sma, sem_rcu_free);
- return id;
- }
- ns-used_sems += nsems;
-
sma-sem_base = (struct sem *) sma[1];
for (i = 0; i nsems; i++) {
@@ -528,6 +521,14 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
INIT_LIST_HEAD(sma-list_id);
sma-sem_nsems = nsems;
sma-sem_ctime = get_seconds();
+
+ id = ipc_addid(sem_ids(ns), sma-sem_perm, ns-sc_semmni);
+ if (id 0) {
+ ipc_rcu_putref(sma, sem_rcu_free);
+ return id;
+ }
+ ns-used_sems += nsems;
+
sem_unlock(sma, -1);
rcu_read_unlock();
--
1.9.3
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/23/2014 01:23 PM, Manfred Spraul wrote: Hi Rik, On 11/21/2014 08:52 PM, Rik van Riel wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. That's not sufficient, as sma-sem_nsems is accessed before calling sem_lock(), both within find_alloc_undo() and within semtimedop(). The root problem is that sma-sem_nsems and sma-sem_base are accessed without any locks, this conflicts with the approach that sma starts to exist as not yet initialized but locked and is unlocked after the initialization is completed. Attached is an idea. It did pass a few short tests. What do you think? This was my other idea for fixing the issue; unfortunately I didn't think of it until after I sent the first patch :) You are right that without that change, we can return the wrong error codes to userspace. I will give the patch a try, though I have so far been unable to reproduce the bug that the customer reported, so I am unlikely to give much in the way of useful testing results... Andrew, feel free to give Manfred's patch my Acked-by: Rik van Riel r...@redhat.com It closes off more things than my patch did, in a similar way (doing something before ipc_addid) - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUckuKAAoJEM553pKExN6DfRoH/jjcBpLwKzrId8aFtuSWYgv3 I7LNzoGozb6Lvn1D1lt/sREdl74KIQptaVClQzphIubiAkQPJVgzqe43f01mqaPu mSjGKarjfKXwUNfa+Q3pW7b2hnbdfTfyQDsTr6JhSpy0ynsmmNb1J2S7E2E2B40r KbOETKeKRTq4vYCk4Em5S4OJg31AoXNoXMVwciOB0M/QDFCSB+4JdKrRsiz6Hm1o +JlRtgwUY6m047Ur65MKeN1bkx1cgqK8tzayMpXW+PTI9cVs6153tlWXmQoXP+Co lHrODmKkhOhJfOr3q0YvrwnaZOVKliKLVV0YkQ+6pi4SAqSjH7pV0jWr/FsaLIA= =u6vO -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Sun, 2014-11-23 at 16:03 -0500, Rik van Riel wrote: On 11/23/2014 01:23 PM, Manfred Spraul wrote: Hi Rik, On 11/21/2014 08:52 PM, Rik van Riel wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. That's not sufficient, as sma-sem_nsems is accessed before calling sem_lock(), both within find_alloc_undo() and within semtimedop(). The root problem is that sma-sem_nsems and sma-sem_base are accessed without any locks, this conflicts with the approach that sma starts to exist as not yet initialized but locked and is unlocked after the initialization is completed. Attached is an idea. It did pass a few short tests. What do you think? This was my other idea for fixing the issue; unfortunately I didn't think of it until after I sent the first patch :) Yep, this is what I was mentioning as well. You are right that without that change, we can return the wrong error codes to userspace. I will give the patch a try, though I have so far been unable to reproduce the bug that the customer reported, so I am unlikely to give much in the way of useful testing results... Andrew, feel free to give Manfred's patch my Acked-by: Rik van Riel r...@redhat.com Acked-by: Davidlohr Bueso d...@stgolabs.net -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/22/2014 02:14 PM, Manfred Spraul wrote: > On 11/21/2014 09:29 PM, Rik van Riel wrote: >> -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 >> >> On 11/21/2014 03:09 PM, Andrew Morton wrote: >>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel >>> wrote: >>> When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma->lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma->lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma->complex_count before unlocking the sma->lock. >>> What are the runtime effects of the bug? >>> >> NULL pointer dereference in spin_lock from sem_lock, if it is >> called before sma->sem_base has been pointed somewhere valid. > No, this can't happen: - sma is initialized to 0 with memset() - > sma->sem_nsems is set last. - semtimedop() contains a "max >= > sma->sem_nsems". > > with sma->sem_nsems==0, this will always fail and therefore > sem_lock() can't be reached. You're right. The reported race must have been semop vs RMID. The kernel tree in question was missing this changeset: commit 6e224f94597842c5eb17f1fc2208d20b6f7f7d49 Author: Manfred Spraul Date: Wed Oct 16 13:46:45 2013 -0700 ipc/sem.c: synchronize semop and semctl with IPC_RMID - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEbBAEBAgAGBQJUcO6rAAoJEM553pKExN6DPXkH+Ot5H94no3DJ6b1WdhOhDMUM sQaWErEcSJ2dxzVES4WUMzqnnEZPokG2uK4z2PVUWjE+YA1U7hGfctLg/Eabr5tV tD+uZhrbSbJVT7HiS5wyqmyzCV5eUV+2Am19pqwa6gyfB30cAYA/GtYfnMhKRGR0 l9hcvyzhci59d/2V2/Y5cGrxvQaWued33JZYfjp2TCl1GDpPD1bocptc3BO0DbwO iHMZBcWfjR5t/EJ2Pg9gwu8X4C7amHsaNM58yTU6o93dE4bpS//A7WtwlLHJ/WEE tD9zoOMnv7o8B5AHl3UDUJJ+JjieQU498AC3IganXQE8WrsZMJWZXo1OZtQP7A== =vZEa -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On 11/21/2014 09:29 PM, Rik van Riel wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma->lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma->lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma->complex_count before unlocking the sma->lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma->sem_base has been pointed somewhere valid. No, this can't happen: - sma is initialized to 0 with memset() - sma->sem_nsems is set last. - semtimedop() contains a "max >= sma->sem_nsems". with sma->sem_nsems==0, this will always fail and therefore sem_lock() can't be reached. The only misbehavior (apart from returning -EFBIG) is that find_alloc_undo() could allocate a wrong-sized undo structure. Would cause random memory corruptions - but not NULL pointer dereference. Which which kernel version have you seen the NULL pointer dereference? -- Manfred -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/22/2014 08:56 AM, Manfred Spraul wrote: > Hi Rik, > > good catch - I completely forgot to check the initialization > > On 11/22/2014 04:40 AM, Rik van Riel wrote: >> >> newary initializes a bunch of things after the call to ipc_addid, >> however some things are initialized inside ipc_addid as well >> >> Looking closer at newary, I suppose that it should be possible to >> move those other initializations before the call to ipc_addid. >> That would likely get rid of the problem, too. >> >> However, I also see this line in newary, and I have no idea what >> protects that data: >> >> ns->used_sems += nsems; > It should be sem_ids.rwsem, and at least according to the > documentation both freeary() and newary() hold it. You're right, that is properly protected already. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUcLFwAAoJEM553pKExN6DoGwIAJVr0SJWq1sxkRr6quw03OlU 8GUNm8Mz4tVpt1PYSe5r3SIGeT0btEhXjRrqp3IOw7z85Iw5Ed2CwzuLS3aunmpg hZag9qyArxKBQkbnhtqiM/0AEHbRiju3+sFNC1cQxsdRvLV6QkRveoP5kKvv4v6d PCMJBHuxL01tPXe5eQC2WjLmI2YPgWWh3fXhDcLt2XTVBI0vwyBiVFc/CtSPERo0 sBF01l/l5KRfOAHduW2kj7KrSF004IgLU7KN5fLG0BzvwlowcHjXgzX7mMCbZrm2 8d8bdEtRJxDZqs4Z9XjDrjXfSdWH/H/5CnfAmvgXA4pswb1lLDLPFbgmt7NUtxk= =vUfd -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
Hi Rik, good catch - I completely forgot to check the initialization On 11/22/2014 04:40 AM, Rik van Riel wrote: newary initializes a bunch of things after the call to ipc_addid, however some things are initialized inside ipc_addid as well Looking closer at newary, I suppose that it should be possible to move those other initializations before the call to ipc_addid. That would likely get rid of the problem, too. However, I also see this line in newary, and I have no idea what protects that data: ns->used_sems += nsems; It should be sem_ids.rwsem, and at least according to the documentation both freeary() and newary() hold it. -- Manfred -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
Hi Rik, good catch - I completely forgot to check the initialization On 11/22/2014 04:40 AM, Rik van Riel wrote: newary initializes a bunch of things after the call to ipc_addid, however some things are initialized inside ipc_addid as well Looking closer at newary, I suppose that it should be possible to move those other initializations before the call to ipc_addid. That would likely get rid of the problem, too. However, I also see this line in newary, and I have no idea what protects that data: ns-used_sems += nsems; It should be sem_ids.rwsem, and at least according to the documentation both freeary() and newary() hold it. -- Manfred -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/22/2014 08:56 AM, Manfred Spraul wrote: Hi Rik, good catch - I completely forgot to check the initialization On 11/22/2014 04:40 AM, Rik van Riel wrote: newary initializes a bunch of things after the call to ipc_addid, however some things are initialized inside ipc_addid as well Looking closer at newary, I suppose that it should be possible to move those other initializations before the call to ipc_addid. That would likely get rid of the problem, too. However, I also see this line in newary, and I have no idea what protects that data: ns-used_sems += nsems; It should be sem_ids.rwsem, and at least according to the documentation both freeary() and newary() hold it. You're right, that is properly protected already. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUcLFwAAoJEM553pKExN6DoGwIAJVr0SJWq1sxkRr6quw03OlU 8GUNm8Mz4tVpt1PYSe5r3SIGeT0btEhXjRrqp3IOw7z85Iw5Ed2CwzuLS3aunmpg hZag9qyArxKBQkbnhtqiM/0AEHbRiju3+sFNC1cQxsdRvLV6QkRveoP5kKvv4v6d PCMJBHuxL01tPXe5eQC2WjLmI2YPgWWh3fXhDcLt2XTVBI0vwyBiVFc/CtSPERo0 sBF01l/l5KRfOAHduW2kj7KrSF004IgLU7KN5fLG0BzvwlowcHjXgzX7mMCbZrm2 8d8bdEtRJxDZqs4Z9XjDrjXfSdWH/H/5CnfAmvgXA4pswb1lLDLPFbgmt7NUtxk= =vUfd -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On 11/21/2014 09:29 PM, Rik van Riel wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. No, this can't happen: - sma is initialized to 0 with memset() - sma-sem_nsems is set last. - semtimedop() contains a max = sma-sem_nsems. with sma-sem_nsems==0, this will always fail and therefore sem_lock() can't be reached. The only misbehavior (apart from returning -EFBIG) is that find_alloc_undo() could allocate a wrong-sized undo structure. Would cause random memory corruptions - but not NULL pointer dereference. Which which kernel version have you seen the NULL pointer dereference? -- Manfred -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/22/2014 02:14 PM, Manfred Spraul wrote: On 11/21/2014 09:29 PM, Rik van Riel wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. No, this can't happen: - sma is initialized to 0 with memset() - sma-sem_nsems is set last. - semtimedop() contains a max = sma-sem_nsems. with sma-sem_nsems==0, this will always fail and therefore sem_lock() can't be reached. You're right. The reported race must have been semop vs RMID. The kernel tree in question was missing this changeset: commit 6e224f94597842c5eb17f1fc2208d20b6f7f7d49 Author: Manfred Spraul manf...@colorfullife.com Date: Wed Oct 16 13:46:45 2013 -0700 ipc/sem.c: synchronize semop and semctl with IPC_RMID - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEbBAEBAgAGBQJUcO6rAAoJEM553pKExN6DPXkH+Ot5H94no3DJ6b1WdhOhDMUM sQaWErEcSJ2dxzVES4WUMzqnnEZPokG2uK4z2PVUWjE+YA1U7hGfctLg/Eabr5tV tD+uZhrbSbJVT7HiS5wyqmyzCV5eUV+2Am19pqwa6gyfB30cAYA/GtYfnMhKRGR0 l9hcvyzhci59d/2V2/Y5cGrxvQaWued33JZYfjp2TCl1GDpPD1bocptc3BO0DbwO iHMZBcWfjR5t/EJ2Pg9gwu8X4C7amHsaNM58yTU6o93dE4bpS//A7WtwlLHJ/WEE tD9zoOMnv7o8B5AHl3UDUJJ+JjieQU498AC3IganXQE8WrsZMJWZXo1OZtQP7A== =vZEa -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 07:56 PM, Davidlohr Bueso wrote: > On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote: >> In other words, if you try to use a semaphore array before getsem >> returns, you can oops the task that calls semop. > > This seems bogus from an application level: how can you call semop > if you don't have the semid yet returned from semget? And the fact > that the race is with newary, means that the call is in fact > creating a *new* set, as opposed to plugging into an already > existing set. Agreed, this is bogus from userspace. However, userspace doing bogus things should not lead to a kernel crash. > The fix in newary() being before the actual creation of the id > seems even stranger: > > sma->complex_count = 1; id = ipc_addid(_ids(ns), > >sem_perm, ns->sc_semmni); > > As for semtimedop() before even getting to sem_lock(), we first > call: > > sma = sem_obtain_object_check(ns, semid); > > So shouldn't that fail anyway before we even consider acquiring the > lock? newary initializes a bunch of things after the call to ipc_addid, however some things are initialized inside ipc_addid as well Looking closer at newary, I suppose that it should be possible to move those other initializations before the call to ipc_addid. That would likely get rid of the problem, too. However, I also see this line in newary, and I have no idea what protects that data: ns->used_sems += nsems; I don't see any locking around ns->used_sems for simultaneous getsem & RMID... - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUcAWfAAoJEM553pKExN6D4n4H/jogtT4f/cWvMI4be3MlfE2x sAIuC0Z6Fqqzm60XB2OB4/yIAZU1JDmsUrmUVqwh3R/G2mQygpkrM9ZKW4dkxtyd MZ0IWtx74OSb376mDcmhk8vI8xh5/j/bWTx2oxP7IFZf4imVFGeZmlG/YLKGSnLS lO9ehr9wkyzoyo1wgpuWhKdxDTEaeZd8C6Ij6bVylWybuWVripN9eX13vWyDmKJ8 P754efTIDu+PWCaEdNA7eKTMlydkXqjPwUpSnSE/bs2ngFhlAkZqkWmTEu54Wc32 yoyEqFNdMvAV8QCHLeR8Uqf53PNhncz7S7RfX58wgdQ5bKO3ATuJ8jbTT5ZXVZ8= =xg+y -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote: > On 11/21/2014 03:42 PM, Andrew Morton wrote: > > On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel > > wrote: > > > >> On 11/21/2014 03:09 PM, Andrew Morton wrote: > >>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel > >>> wrote: > >>> > When manipulating just one semaphore with semop, sem_lock > only takes that single semaphore's lock. This creates a > problem during initialization of the semaphore array, when > the data structures used by sem_lock have not been set up > yet. The sma->lock is already held by newary, and we just > have to make sure everything else waits on that lock during > initialization. > > Luckily it is easy to make sem_lock wait on the sma->lock, > by pretending there is a complex operation in progress while > the sma is being initialized. > > The newary function already zeroes sma->complex_count before > unlocking the sma->lock. > >>> > >>> What are the runtime effects of the bug? > >>> > >> > >> NULL pointer dereference in spin_lock from sem_lock, if it is > >> called before sma->sem_base has been pointed somewhere valid. > > > > Help us out here. People need to use this description to work out > > which kernel versions need the patch and whether to backport the > > fix into their various kernels. Other people will be starting at > > this changelog wondering "will this fix the bug my customer has > > reported". > > > > Is there some bug report people can look at? This would be nice for the changelog... > > > > What userspace actions trigger this bug? > > The reason the bug took almost two years to get noticed is that > it takes one task doing a semop on a semaphore in an array that > is still getting instantiated by newary (getsem) from another > task. > > In other words, if you try to use a semaphore array before > getsem returns, you can oops the task that calls semop. This seems bogus from an application level: how can you call semop if you don't have the semid yet returned from semget? And the fact that the race is with newary, means that the call is in fact creating a *new* set, as opposed to plugging into an already existing set. The fix in newary() being before the actual creation of the id seems even stranger: sma->complex_count = 1; id = ipc_addid(_ids(ns), >sem_perm, ns->sc_semmni); As for semtimedop() before even getting to sem_lock(), we first call: sma = sem_obtain_object_check(ns, semid); So shouldn't that fail anyway before we even consider acquiring the lock? Thanks, Davidlohr -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:42 PM, Andrew Morton wrote: > On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel > wrote: > >> On 11/21/2014 03:09 PM, Andrew Morton wrote: >>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel >>> wrote: >>> When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma->lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma->lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma->complex_count before unlocking the sma->lock. >>> >>> What are the runtime effects of the bug? >>> >> >> NULL pointer dereference in spin_lock from sem_lock, if it is >> called before sma->sem_base has been pointed somewhere valid. > > Help us out here. People need to use this description to work out > which kernel versions need the patch and whether to backport the > fix into their various kernels. Other people will be starting at > this changelog wondering "will this fix the bug my customer has > reported". > > Is there some bug report people can look at? > > What userspace actions trigger this bug? The reason the bug took almost two years to get noticed is that it takes one task doing a semop on a semaphore in an array that is still getting instantiated by newary (getsem) from another task. In other words, if you try to use a semaphore array before getsem returns, you can oops the task that calls semop. It should not cause any damage to long-living kernel data structures. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUb8TZAAoJEM553pKExN6DzJUH/RYSovikk+36KH0uFQN44txj ZkEM6BsT7I6W9zBiK4OCPpwYCr5gy2xsXH7bLzCgzRV/YmjLFdw20DhDfSo14GO/ 1ByYcsUcsZ+lPJZ+g4IKi57VW4T+NLa1T4CoJ84+1QVGKYlpc7mlwc8suTGBhKvQ 5Eq1o1KOE9ZtAG5Go8OYH7frwalkrYE0YJbGN9PW0pUvZ7FilEiMJIkznIetRS6K WK05dK52DMKeXFxzuxVhSRcCZb2+bHZn3qFOmon6kHbMqgzRZCKMcdydtoIvcFq7 cA5eTt6V6je3XVhc4lsSfP9cHraLDZZIjkaJ856fBpgJ30ypsHcpVY6UKTbFSHo= =u1Vg -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel wrote: > On 11/21/2014 03:09 PM, Andrew Morton wrote: > > On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel > > wrote: > > > >> When manipulating just one semaphore with semop, sem_lock only > >> takes that single semaphore's lock. This creates a problem during > >> initialization of the semaphore array, when the data structures > >> used by sem_lock have not been set up yet. The sma->lock is > >> already held by newary, and we just have to make sure everything > >> else waits on that lock during initialization. > >> > >> Luckily it is easy to make sem_lock wait on the sma->lock, by > >> pretending there is a complex operation in progress while the sma > >> is being initialized. > >> > >> The newary function already zeroes sma->complex_count before > >> unlocking the sma->lock. > > > > What are the runtime effects of the bug? > > > > NULL pointer dereference in spin_lock from sem_lock, > if it is called before sma->sem_base has been pointed > somewhere valid. Help us out here. People need to use this description to work out which kernel versions need the patch and whether to backport the fix into their various kernels. Other people will be starting at this changelog wondering "will this fix the bug my customer has reported". Is there some bug report people can look at? What userspace actions trigger this bug? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:09 PM, Andrew Morton wrote: > On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel > wrote: > >> When manipulating just one semaphore with semop, sem_lock only >> takes that single semaphore's lock. This creates a problem during >> initialization of the semaphore array, when the data structures >> used by sem_lock have not been set up yet. The sma->lock is >> already held by newary, and we just have to make sure everything >> else waits on that lock during initialization. >> >> Luckily it is easy to make sem_lock wait on the sma->lock, by >> pretending there is a complex operation in progress while the sma >> is being initialized. >> >> The newary function already zeroes sma->complex_count before >> unlocking the sma->lock. > > What are the runtime effects of the bug? > NULL pointer dereference in spin_lock from sem_lock, if it is called before sma->sem_base has been pointed somewhere valid. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUb6CnAAoJEM553pKExN6Dh8oH/iFVqwrukMkZp7ViFTC84DVK m8rw6CWk76kEvi6BWx977nT26e7ZfiOCxrhyy/gETOnUDJMgQrn7cFMFd6Ja/2yG uGCq5WcvVLDiLw7ij9Rqu4C6aHcICserzgfXwWV0XAa5TZOEqvg6FKZgCUHN6sxM ek0TV0oq/VQvRwAQk/MFDOv38PydH2LH1Z3wXk7JVlhEMX062a4EoxTAe8Teed2p X5+mTOl4jezog2rFJxFe0Cp8PxpqAi4f1kDugQKohZ3TpUFqH4VKZYmTtvHvpNDH oeHjnRv632N8KuU2lvIi7EGJGu0Y+ReyOr+NQtozlRYCTPuY/rezkbBmgVwu4iY= =CDvy -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel wrote: > When manipulating just one semaphore with semop, sem_lock only takes that > single semaphore's lock. This creates a problem during initialization of > the semaphore array, when the data structures used by sem_lock have not > been set up yet. The sma->lock is already held by newary, and we just > have to make sure everything else waits on that lock during initialization. > > Luckily it is easy to make sem_lock wait on the sma->lock, by pretending > there is a complex operation in progress while the sma is being initialized. > > The newary function already zeroes sma->complex_count before unlocking > the sma->lock. What are the runtime effects of the bug? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization
On Fri, Nov 21, 2014 at 02:52:26PM -0500, Rik van Riel wrote:
> When manipulating just one semaphore with semop, sem_lock only takes that
> single semaphore's lock. This creates a problem during initialization of
> the semaphore array, when the data structures used by sem_lock have not
> been set up yet. The sma->lock is already held by newary, and we just
> have to make sure everything else waits on that lock during initialization.
>
> Luckily it is easy to make sem_lock wait on the sma->lock, by pretending
> there is a complex operation in progress while the sma is being initialized.
>
> The newary function already zeroes sma->complex_count before unlocking
> the sma->lock.
>
> Signed-off-by: Rik van Riel
> ---
> ipc/sem.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 454f6c6..1823160 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -507,6 +507,9 @@ static int newary(struct ipc_namespace *ns, struct
> ipc_params *params)
> return retval;
> }
>
> + /* Ensures sem_lock waits on >lock until sma is ready. */
> + sma->complex_count = 1;
> +
> id = ipc_addid(_ids(ns), >sem_perm, ns->sc_semmni);
> if (id < 0) {
> ipc_rcu_putref(sma, sem_rcu_free);
Acked-by: Rafael Aquini
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Fri, Nov 21, 2014 at 02:52:26PM -0500, Rik van Riel wrote:
When manipulating just one semaphore with semop, sem_lock only takes that
single semaphore's lock. This creates a problem during initialization of
the semaphore array, when the data structures used by sem_lock have not
been set up yet. The sma-lock is already held by newary, and we just
have to make sure everything else waits on that lock during initialization.
Luckily it is easy to make sem_lock wait on the sma-lock, by pretending
there is a complex operation in progress while the sma is being initialized.
The newary function already zeroes sma-complex_count before unlocking
the sma-lock.
Signed-off-by: Rik van Riel r...@redhat.com
---
ipc/sem.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ipc/sem.c b/ipc/sem.c
index 454f6c6..1823160 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -507,6 +507,9 @@ static int newary(struct ipc_namespace *ns, struct
ipc_params *params)
return retval;
}
+ /* Ensures sem_lock waits on sma-lock until sma is ready. */
+ sma-complex_count = 1;
+
id = ipc_addid(sem_ids(ns), sma-sem_perm, ns-sc_semmni);
if (id 0) {
ipc_rcu_putref(sma, sem_rcu_free);
Acked-by: Rafael Aquini aqu...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUb6CnAAoJEM553pKExN6Dh8oH/iFVqwrukMkZp7ViFTC84DVK m8rw6CWk76kEvi6BWx977nT26e7ZfiOCxrhyy/gETOnUDJMgQrn7cFMFd6Ja/2yG uGCq5WcvVLDiLw7ij9Rqu4C6aHcICserzgfXwWV0XAa5TZOEqvg6FKZgCUHN6sxM ek0TV0oq/VQvRwAQk/MFDOv38PydH2LH1Z3wXk7JVlhEMX062a4EoxTAe8Teed2p X5+mTOl4jezog2rFJxFe0Cp8PxpqAi4f1kDugQKohZ3TpUFqH4VKZYmTtvHvpNDH oeHjnRv632N8KuU2lvIi7EGJGu0Y+ReyOr+NQtozlRYCTPuY/rezkbBmgVwu4iY= =CDvy -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel r...@redhat.com wrote: On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. Help us out here. People need to use this description to work out which kernel versions need the patch and whether to backport the fix into their various kernels. Other people will be starting at this changelog wondering will this fix the bug my customer has reported. Is there some bug report people can look at? What userspace actions trigger this bug? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 03:42 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel r...@redhat.com wrote: On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. Help us out here. People need to use this description to work out which kernel versions need the patch and whether to backport the fix into their various kernels. Other people will be starting at this changelog wondering will this fix the bug my customer has reported. Is there some bug report people can look at? What userspace actions trigger this bug? The reason the bug took almost two years to get noticed is that it takes one task doing a semop on a semaphore in an array that is still getting instantiated by newary (getsem) from another task. In other words, if you try to use a semaphore array before getsem returns, you can oops the task that calls semop. It should not cause any damage to long-living kernel data structures. - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUb8TZAAoJEM553pKExN6DzJUH/RYSovikk+36KH0uFQN44txj ZkEM6BsT7I6W9zBiK4OCPpwYCr5gy2xsXH7bLzCgzRV/YmjLFdw20DhDfSo14GO/ 1ByYcsUcsZ+lPJZ+g4IKi57VW4T+NLa1T4CoJ84+1QVGKYlpc7mlwc8suTGBhKvQ 5Eq1o1KOE9ZtAG5Go8OYH7frwalkrYE0YJbGN9PW0pUvZ7FilEiMJIkznIetRS6K WK05dK52DMKeXFxzuxVhSRcCZb2+bHZn3qFOmon6kHbMqgzRZCKMcdydtoIvcFq7 cA5eTt6V6je3XVhc4lsSfP9cHraLDZZIjkaJ856fBpgJ30ypsHcpVY6UKTbFSHo= =u1Vg -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote: On 11/21/2014 03:42 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel r...@redhat.com wrote: On 11/21/2014 03:09 PM, Andrew Morton wrote: On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel r...@redhat.com wrote: When manipulating just one semaphore with semop, sem_lock only takes that single semaphore's lock. This creates a problem during initialization of the semaphore array, when the data structures used by sem_lock have not been set up yet. The sma-lock is already held by newary, and we just have to make sure everything else waits on that lock during initialization. Luckily it is easy to make sem_lock wait on the sma-lock, by pretending there is a complex operation in progress while the sma is being initialized. The newary function already zeroes sma-complex_count before unlocking the sma-lock. What are the runtime effects of the bug? NULL pointer dereference in spin_lock from sem_lock, if it is called before sma-sem_base has been pointed somewhere valid. Help us out here. People need to use this description to work out which kernel versions need the patch and whether to backport the fix into their various kernels. Other people will be starting at this changelog wondering will this fix the bug my customer has reported. Is there some bug report people can look at? This would be nice for the changelog... What userspace actions trigger this bug? The reason the bug took almost two years to get noticed is that it takes one task doing a semop on a semaphore in an array that is still getting instantiated by newary (getsem) from another task. In other words, if you try to use a semaphore array before getsem returns, you can oops the task that calls semop. This seems bogus from an application level: how can you call semop if you don't have the semid yet returned from semget? And the fact that the race is with newary, means that the call is in fact creating a *new* set, as opposed to plugging into an already existing set. The fix in newary() being before the actual creation of the id seems even stranger: sma-complex_count = 1; id = ipc_addid(sem_ids(ns), sma-sem_perm, ns-sc_semmni); As for semtimedop() before even getting to sem_lock(), we first call: sma = sem_obtain_object_check(ns, semid); So shouldn't that fail anyway before we even consider acquiring the lock? Thanks, Davidlohr -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc,sem block sem_lock on sma-lock during sma initialization
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/21/2014 07:56 PM, Davidlohr Bueso wrote: On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote: In other words, if you try to use a semaphore array before getsem returns, you can oops the task that calls semop. This seems bogus from an application level: how can you call semop if you don't have the semid yet returned from semget? And the fact that the race is with newary, means that the call is in fact creating a *new* set, as opposed to plugging into an already existing set. Agreed, this is bogus from userspace. However, userspace doing bogus things should not lead to a kernel crash. The fix in newary() being before the actual creation of the id seems even stranger: sma-complex_count = 1; id = ipc_addid(sem_ids(ns), sma-sem_perm, ns-sc_semmni); As for semtimedop() before even getting to sem_lock(), we first call: sma = sem_obtain_object_check(ns, semid); So shouldn't that fail anyway before we even consider acquiring the lock? newary initializes a bunch of things after the call to ipc_addid, however some things are initialized inside ipc_addid as well Looking closer at newary, I suppose that it should be possible to move those other initializations before the call to ipc_addid. That would likely get rid of the problem, too. However, I also see this line in newary, and I have no idea what protects that data: ns-used_sems += nsems; I don't see any locking around ns-used_sems for simultaneous getsem RMID... - -- All rights reversed -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJUcAWfAAoJEM553pKExN6D4n4H/jogtT4f/cWvMI4be3MlfE2x sAIuC0Z6Fqqzm60XB2OB4/yIAZU1JDmsUrmUVqwh3R/G2mQygpkrM9ZKW4dkxtyd MZ0IWtx74OSb376mDcmhk8vI8xh5/j/bWTx2oxP7IFZf4imVFGeZmlG/YLKGSnLS lO9ehr9wkyzoyo1wgpuWhKdxDTEaeZd8C6Ij6bVylWybuWVripN9eX13vWyDmKJ8 P754efTIDu+PWCaEdNA7eKTMlydkXqjPwUpSnSE/bs2ngFhlAkZqkWmTEu54Wc32 yoyEqFNdMvAV8QCHLeR8Uqf53PNhncz7S7RfX58wgdQ5bKO3ATuJ8jbTT5ZXVZ8= =xg+y -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
