[Xenomai-git] Philippe Gerum : cobalt/thread: fix race on accessing the global thread list

[git repository hosting]

Module: xenomai-3
Branch: wip/drivers
Commit: 203f2007e2c1328ca8847562ed3edbcb81d5e38a
URL:
http://git.xenomai.org/?p=xenomai-3.git;a=commit;h=203f2007e2c1328ca8847562ed3edbcb81d5e38a

Author: Philippe Gerum 
Date:   Wed Jun 29 09:54:12 2016 +0200

cobalt/thread: fix race on accessing the global thread list

nkthreadq is only used by the /proc interface handlers these days, to
get the list of active Cobalt threads.

Adding an emerging user-space thread to that list should not be done
before it is actually mated to a Cobalt shadow, otherwise the /proc
handlers might find such thread before its host_task pointer is known,
causing invalid memory accesses.

A simple way to reproduce the issue involves running a (user) thread
creation loop while continuously reading from /proc/xenomai/sched/* in
parallel.

The fix postpones the insertion of emerging threads into the global
thread queue until their TCB is fully built.

---

 kernel/cobalt/thread.c |   84 +++-
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index e16f7a3..7daefdb 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -87,6 +87,13 @@ static void periodic_handler(struct xntimer *timer)
fixup_ptimer_affinity(thread);
 }
 
+static inline void enlist_new_thread(struct xnthread *thread)
+{  /* nklock held, irqs off */
+   list_add_tail(>glink, );
+   cobalt_nrthreads++;
+   xnvfile_touch_tag(_tag);
+}
+
 struct kthread_arg {
struct xnthread *thread;
struct completion *done;
@@ -557,10 +564,10 @@ void __xnthread_discard(struct xnthread *thread)
  * Initializes a new thread. The thread is left dormant until it is
  * actually started by xnthread_start().
  *
- * @param thread The address of a thread descriptor the nucleus will
- * use to store the thread-specific data.  This descriptor must always
- * be valid while the thread is active therefore it must be allocated
- * in permanent memory. @warning Some architectures may require the
+ * @param thread The address of a thread descriptor Cobalt will use to
+ * store the thread-specific data.  This descriptor must always be
+ * valid while the thread is active therefore it must be allocated in
+ * permanent memory. @warning Some architectures may require the
  * descriptor to be properly aligned in memory; this is an additional
  * reason for descriptors not to be laid in the program stack where
  * alignement constraints might not always be satisfied.
@@ -571,14 +578,13 @@ void __xnthread_discard(struct xnthread *thread)
  *
  * - name: An ASCII string standing for the symbolic name of the
  * thread. This name is copied to a safe place into the thread
- * descriptor. This name might be used in various situations by the
- * nucleus for issuing human-readable diagnostic messages, so it is
- * usually a good idea to provide a sensible value here.  NULL is fine
- * though and means "anonymous".
+ * descriptor. This name might be used in various situations by Cobalt
+ * for issuing human-readable diagnostic messages, so it is usually a
+ * good idea to provide a sensible value here.  NULL is fine though
+ * and means "anonymous".
  *
  * - flags: A set of creation flags affecting the operation. The
- * following flags can be part of this bitmask, each of them affecting
- * the nucleus behaviour regarding the created thread:
+ * following flags can be part of this bitmask:
  *
  *   - XNSUSP creates the thread in a suspended state. In such a case,
  * the thread shall be explicitly resumed using the xnthread_resume()
@@ -590,8 +596,8 @@ void __xnthread_discard(struct xnthread *thread)
  * user-space task. Otherwise, a new kernel host task is created, then
  * paired with the new Xenomai thread.
  *
- * - XNFPU (enable FPU) tells the nucleus that the new thread may use
- * the floating-point unit. XNFPU is implicitly assumed for user-space
+ * - XNFPU (enable FPU) tells Cobalt that the new thread may use the
+ * floating-point unit. XNFPU is implicitly assumed for user-space
  * threads even if not set in @a flags.
  *
  * - affinity: The processor affinity of this thread. Passing
@@ -620,7 +626,6 @@ int xnthread_init(struct xnthread *thread,
 {
struct xnsched *sched;
cpumask_t affinity;
-   spl_t s;
int ret;
 
if (attr->flags & ~(XNFPU | XNUSER | XNSUSP))
@@ -643,12 +648,6 @@ int xnthread_init(struct xnthread *thread,
 
trace_cobalt_thread_init(thread, attr, sched_class);
 
-   xnlock_get_irqsave(, s);
-   list_add_tail(>glink, );
-   cobalt_nrthreads++;
-   xnvfile_touch_tag(_tag);
-   xnlock_put_irqrestore(, s);
-
return 0;
 }
 EXPORT_SYMBOL_GPL(xnthread_init);
@@ -668,9 +667,8 @@ EXPORT_SYMBOL_GPL(xnthread_init);
  * execution properties of the new thread. Members of this structure
  * are defined as follows:
  *
- * - mode: The 

[Xenomai-git] Philippe Gerum : cobalt/thread: fix race on accessing the global thread list

[git repository hosting]

Module: xenomai-3
Branch: wip/dovetail
Commit: e1b0d84ce7ab5a49bd4c5553bb9060055ebd2adb
URL:
http://git.xenomai.org/?p=xenomai-3.git;a=commit;h=e1b0d84ce7ab5a49bd4c5553bb9060055ebd2adb

Author: Philippe Gerum 
Date:   Wed Jun 29 09:54:12 2016 +0200

cobalt/thread: fix race on accessing the global thread list

nkthreadq is only used by the /proc interface handlers these days, to
get the list of active Cobalt threads.

Adding an emerging user-space thread to that list should not be done
before it is actually mated to a Cobalt shadow, otherwise the /proc
handlers might find such thread before its host_task pointer is known,
causing invalid memory accesses.

A simple way to reproduce the issue involves running a (user) thread
creation loop while continuously reading from /proc/xenomai/sched/* in
parallel.

The fix postpones the insertion of emerging threads into the global
thread queue until their TCB is fully built.

---

 kernel/cobalt/thread.c |   84 +++-
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index cabc6b8..435cd4b 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -88,6 +88,13 @@ static void periodic_handler(struct xntimer *timer)
fixup_ptimer_affinity(thread);
 }
 
+static inline void enlist_new_thread(struct xnthread *thread)
+{  /* nklock held, irqs off */
+   list_add_tail(>glink, );
+   cobalt_nrthreads++;
+   xnvfile_touch_tag(_tag);
+}
+
 struct kthread_arg {
struct xnthread *thread;
struct completion *done;
@@ -573,10 +580,10 @@ void __xnthread_discard(struct xnthread *thread)
  * Initializes a new thread. The thread is left dormant until it is
  * actually started by xnthread_start().
  *
- * @param thread The address of a thread descriptor the nucleus will
- * use to store the thread-specific data.  This descriptor must always
- * be valid while the thread is active therefore it must be allocated
- * in permanent memory. @warning Some architectures may require the
+ * @param thread The address of a thread descriptor Cobalt will use to
+ * store the thread-specific data.  This descriptor must always be
+ * valid while the thread is active therefore it must be allocated in
+ * permanent memory. @warning Some architectures may require the
  * descriptor to be properly aligned in memory; this is an additional
  * reason for descriptors not to be laid in the program stack where
  * alignement constraints might not always be satisfied.
@@ -587,14 +594,13 @@ void __xnthread_discard(struct xnthread *thread)
  *
  * - name: An ASCII string standing for the symbolic name of the
  * thread. This name is copied to a safe place into the thread
- * descriptor. This name might be used in various situations by the
- * nucleus for issuing human-readable diagnostic messages, so it is
- * usually a good idea to provide a sensible value here.  NULL is fine
- * though and means "anonymous".
+ * descriptor. This name might be used in various situations by Cobalt
+ * for issuing human-readable diagnostic messages, so it is usually a
+ * good idea to provide a sensible value here.  NULL is fine though
+ * and means "anonymous".
  *
  * - flags: A set of creation flags affecting the operation. The
- * following flags can be part of this bitmask, each of them affecting
- * the nucleus behaviour regarding the created thread:
+ * following flags can be part of this bitmask:
  *
  *   - XNSUSP creates the thread in a suspended state. In such a case,
  * the thread shall be explicitly resumed using the xnthread_resume()
@@ -606,8 +612,8 @@ void __xnthread_discard(struct xnthread *thread)
  * user-space task. Otherwise, a new kernel host task is created, then
  * paired with the new Xenomai thread.
  *
- * - XNFPU (enable FPU) tells the nucleus that the new thread may use
- * the floating-point unit. XNFPU is implicitly assumed for user-space
+ * - XNFPU (enable FPU) tells Cobalt that the new thread may use the
+ * floating-point unit. XNFPU is implicitly assumed for user-space
  * threads even if not set in @a flags.
  *
  * - affinity: The processor affinity of this thread. Passing
@@ -636,7 +642,6 @@ int xnthread_init(struct xnthread *thread,
 {
struct xnsched *sched;
cpumask_t affinity;
-   spl_t s;
int ret;
 
if (attr->flags & ~(XNFPU | XNUSER | XNSUSP))
@@ -659,12 +664,6 @@ int xnthread_init(struct xnthread *thread,
 
trace_cobalt_thread_init(thread, attr, sched_class);
 
-   xnlock_get_irqsave(, s);
-   list_add_tail(>glink, );
-   cobalt_nrthreads++;
-   xnvfile_touch_tag(_tag);
-   xnlock_put_irqrestore(, s);
-
return 0;
 }
 EXPORT_SYMBOL_GPL(xnthread_init);
@@ -684,9 +683,8 @@ EXPORT_SYMBOL_GPL(xnthread_init);
  * execution properties of the new thread. Members of this structure
  * are defined as follows:
  *
- * - mode: The 

[Xenomai-git] Philippe Gerum : cobalt/thread: fix race on accessing the global thread list

[git repository hosting]

Module: xenomai-3
Branch: next
Commit: e1b0d84ce7ab5a49bd4c5553bb9060055ebd2adb
URL:
http://git.xenomai.org/?p=xenomai-3.git;a=commit;h=e1b0d84ce7ab5a49bd4c5553bb9060055ebd2adb

Author: Philippe Gerum 
Date:   Wed Jun 29 09:54:12 2016 +0200

cobalt/thread: fix race on accessing the global thread list

nkthreadq is only used by the /proc interface handlers these days, to
get the list of active Cobalt threads.

Adding an emerging user-space thread to that list should not be done
before it is actually mated to a Cobalt shadow, otherwise the /proc
handlers might find such thread before its host_task pointer is known,
causing invalid memory accesses.

A simple way to reproduce the issue involves running a (user) thread
creation loop while continuously reading from /proc/xenomai/sched/* in
parallel.

The fix postpones the insertion of emerging threads into the global
thread queue until their TCB is fully built.

---

 kernel/cobalt/thread.c |   84 +++-
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index cabc6b8..435cd4b 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -88,6 +88,13 @@ static void periodic_handler(struct xntimer *timer)
fixup_ptimer_affinity(thread);
 }
 
+static inline void enlist_new_thread(struct xnthread *thread)
+{  /* nklock held, irqs off */
+   list_add_tail(>glink, );
+   cobalt_nrthreads++;
+   xnvfile_touch_tag(_tag);
+}
+
 struct kthread_arg {
struct xnthread *thread;
struct completion *done;
@@ -573,10 +580,10 @@ void __xnthread_discard(struct xnthread *thread)
  * Initializes a new thread. The thread is left dormant until it is
  * actually started by xnthread_start().
  *
- * @param thread The address of a thread descriptor the nucleus will
- * use to store the thread-specific data.  This descriptor must always
- * be valid while the thread is active therefore it must be allocated
- * in permanent memory. @warning Some architectures may require the
+ * @param thread The address of a thread descriptor Cobalt will use to
+ * store the thread-specific data.  This descriptor must always be
+ * valid while the thread is active therefore it must be allocated in
+ * permanent memory. @warning Some architectures may require the
  * descriptor to be properly aligned in memory; this is an additional
  * reason for descriptors not to be laid in the program stack where
  * alignement constraints might not always be satisfied.
@@ -587,14 +594,13 @@ void __xnthread_discard(struct xnthread *thread)
  *
  * - name: An ASCII string standing for the symbolic name of the
  * thread. This name is copied to a safe place into the thread
- * descriptor. This name might be used in various situations by the
- * nucleus for issuing human-readable diagnostic messages, so it is
- * usually a good idea to provide a sensible value here.  NULL is fine
- * though and means "anonymous".
+ * descriptor. This name might be used in various situations by Cobalt
+ * for issuing human-readable diagnostic messages, so it is usually a
+ * good idea to provide a sensible value here.  NULL is fine though
+ * and means "anonymous".
  *
  * - flags: A set of creation flags affecting the operation. The
- * following flags can be part of this bitmask, each of them affecting
- * the nucleus behaviour regarding the created thread:
+ * following flags can be part of this bitmask:
  *
  *   - XNSUSP creates the thread in a suspended state. In such a case,
  * the thread shall be explicitly resumed using the xnthread_resume()
@@ -606,8 +612,8 @@ void __xnthread_discard(struct xnthread *thread)
  * user-space task. Otherwise, a new kernel host task is created, then
  * paired with the new Xenomai thread.
  *
- * - XNFPU (enable FPU) tells the nucleus that the new thread may use
- * the floating-point unit. XNFPU is implicitly assumed for user-space
+ * - XNFPU (enable FPU) tells Cobalt that the new thread may use the
+ * floating-point unit. XNFPU is implicitly assumed for user-space
  * threads even if not set in @a flags.
  *
  * - affinity: The processor affinity of this thread. Passing
@@ -636,7 +642,6 @@ int xnthread_init(struct xnthread *thread,
 {
struct xnsched *sched;
cpumask_t affinity;
-   spl_t s;
int ret;
 
if (attr->flags & ~(XNFPU | XNUSER | XNSUSP))
@@ -659,12 +664,6 @@ int xnthread_init(struct xnthread *thread,
 
trace_cobalt_thread_init(thread, attr, sched_class);
 
-   xnlock_get_irqsave(, s);
-   list_add_tail(>glink, );
-   cobalt_nrthreads++;
-   xnvfile_touch_tag(_tag);
-   xnlock_put_irqrestore(, s);
-
return 0;
 }
 EXPORT_SYMBOL_GPL(xnthread_init);
@@ -684,9 +683,8 @@ EXPORT_SYMBOL_GPL(xnthread_init);
  * execution properties of the new thread. Members of this structure
  * are defined as follows:
  *
- * - mode: The initial 

[Xenomai-git] Philippe Gerum : cobalt/thread: fix race on accessing the global thread list

[git repository hosting]

Module: xenomai-3
Branch: stable-3.0.x
Commit: 203f2007e2c1328ca8847562ed3edbcb81d5e38a
URL:
http://git.xenomai.org/?p=xenomai-3.git;a=commit;h=203f2007e2c1328ca8847562ed3edbcb81d5e38a

Author: Philippe Gerum 
Date:   Wed Jun 29 09:54:12 2016 +0200

cobalt/thread: fix race on accessing the global thread list

nkthreadq is only used by the /proc interface handlers these days, to
get the list of active Cobalt threads.

Adding an emerging user-space thread to that list should not be done
before it is actually mated to a Cobalt shadow, otherwise the /proc
handlers might find such thread before its host_task pointer is known,
causing invalid memory accesses.

A simple way to reproduce the issue involves running a (user) thread
creation loop while continuously reading from /proc/xenomai/sched/* in
parallel.

The fix postpones the insertion of emerging threads into the global
thread queue until their TCB is fully built.

---

 kernel/cobalt/thread.c |   84 +++-
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index e16f7a3..7daefdb 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -87,6 +87,13 @@ static void periodic_handler(struct xntimer *timer)
fixup_ptimer_affinity(thread);
 }
 
+static inline void enlist_new_thread(struct xnthread *thread)
+{  /* nklock held, irqs off */
+   list_add_tail(>glink, );
+   cobalt_nrthreads++;
+   xnvfile_touch_tag(_tag);
+}
+
 struct kthread_arg {
struct xnthread *thread;
struct completion *done;
@@ -557,10 +564,10 @@ void __xnthread_discard(struct xnthread *thread)
  * Initializes a new thread. The thread is left dormant until it is
  * actually started by xnthread_start().
  *
- * @param thread The address of a thread descriptor the nucleus will
- * use to store the thread-specific data.  This descriptor must always
- * be valid while the thread is active therefore it must be allocated
- * in permanent memory. @warning Some architectures may require the
+ * @param thread The address of a thread descriptor Cobalt will use to
+ * store the thread-specific data.  This descriptor must always be
+ * valid while the thread is active therefore it must be allocated in
+ * permanent memory. @warning Some architectures may require the
  * descriptor to be properly aligned in memory; this is an additional
  * reason for descriptors not to be laid in the program stack where
  * alignement constraints might not always be satisfied.
@@ -571,14 +578,13 @@ void __xnthread_discard(struct xnthread *thread)
  *
  * - name: An ASCII string standing for the symbolic name of the
  * thread. This name is copied to a safe place into the thread
- * descriptor. This name might be used in various situations by the
- * nucleus for issuing human-readable diagnostic messages, so it is
- * usually a good idea to provide a sensible value here.  NULL is fine
- * though and means "anonymous".
+ * descriptor. This name might be used in various situations by Cobalt
+ * for issuing human-readable diagnostic messages, so it is usually a
+ * good idea to provide a sensible value here.  NULL is fine though
+ * and means "anonymous".
  *
  * - flags: A set of creation flags affecting the operation. The
- * following flags can be part of this bitmask, each of them affecting
- * the nucleus behaviour regarding the created thread:
+ * following flags can be part of this bitmask:
  *
  *   - XNSUSP creates the thread in a suspended state. In such a case,
  * the thread shall be explicitly resumed using the xnthread_resume()
@@ -590,8 +596,8 @@ void __xnthread_discard(struct xnthread *thread)
  * user-space task. Otherwise, a new kernel host task is created, then
  * paired with the new Xenomai thread.
  *
- * - XNFPU (enable FPU) tells the nucleus that the new thread may use
- * the floating-point unit. XNFPU is implicitly assumed for user-space
+ * - XNFPU (enable FPU) tells Cobalt that the new thread may use the
+ * floating-point unit. XNFPU is implicitly assumed for user-space
  * threads even if not set in @a flags.
  *
  * - affinity: The processor affinity of this thread. Passing
@@ -620,7 +626,6 @@ int xnthread_init(struct xnthread *thread,
 {
struct xnsched *sched;
cpumask_t affinity;
-   spl_t s;
int ret;
 
if (attr->flags & ~(XNFPU | XNUSER | XNSUSP))
@@ -643,12 +648,6 @@ int xnthread_init(struct xnthread *thread,
 
trace_cobalt_thread_init(thread, attr, sched_class);
 
-   xnlock_get_irqsave(, s);
-   list_add_tail(>glink, );
-   cobalt_nrthreads++;
-   xnvfile_touch_tag(_tag);
-   xnlock_put_irqrestore(, s);
-
return 0;
 }
 EXPORT_SYMBOL_GPL(xnthread_init);
@@ -668,9 +667,8 @@ EXPORT_SYMBOL_GPL(xnthread_init);
  * execution properties of the new thread. Members of this structure
  * are defined as follows:
  *
- * - mode: The