[Xenomai] __cobalt_thread_create: cleanup after failing cobalt_map_user

[Xenomai] __cobalt_thread_create: cleanup after failing cobalt_map_user

[Jan Kiszka]

Hi,

I ran into an oops (bt below) when (for yet unknown reasons)
cobalt_map_user fails. The reason is that we then call xnthread_cancel,
but on a thread that has no user space task assigned yet.

Which functions rolls only pthread_create back?

The issue should apply to cobalt_thread_shadow as well.

Jan

(gdb) bt
#0  dump_stack () at /data/linux-ipipe/lib/dump_stack.c:27
#1  0xffffffff810e75ad in ipipe_root_only () at /data/linux-ipipe/kernel/ipipe/core.c:1685
#2  0xffffffff810e86b1 in ipipe_unstall_root () at /data/linux-ipipe/kernel/ipipe/core.c:419
#3  0xffffffff8168a51a in arch_local_irq_enable () at /data/linux-ipipe/arch/x86/include/asm/irqflags.h:114
#4  __do_page_fault (regs=0xffff88003a4a7cc8, error_code=0, address=732) at /data/linux-ipipe/arch/x86/mm/fault.c:1134
#5  0xffffffff8168a7b2 in do_page_fault (regs=<optimized out>, error_code=<optimized out>) at /data/linux-ipipe/arch/x86/mm/fault.c:1277
#6  0xffffffff8102ea1a in __ipipe_handle_exception (regs=0xffff88003a4a7cc8, error_code=-131940329925848, vector=14) at /data/linux-ipipe/arch/x86/kernel/ipipe.c:438
#7  <signal handler called>
#8  0xffffffff81133902 in xnthread_host_pid (thread=<optimized out>) at /data/linux-ipipe/include/xenomai/cobalt/kernel/thread.h:249
#9  xnthread_resume (thread=0xffff88003c204040, mask=<optimized out>) at /data/linux-ipipe/kernel/xenomai/thread.c:1038
#10 0xffffffff81137a5f in xnthread_cancel (thread=0xffff88003c204040) at /data/linux-ipipe/kernel/xenomai/thread.c:1463
#11 0xffffffff81179f45 in __cobalt_thread_create (pth=140597174187776, policy=<optimized out>, param_ex=0xffff88003a4a7e80, xid=<optimized out>, u_winoff=<optimized out>) at /data/linux-ipipe/kernel/xenomai/posix/thread.c:586
#12 0xffffffff8117a164 in cobalt_thread_create (pth=140597174187776, policy=0, u_param=<optimized out>, xid=0, u_winoff=0x7fdf549fae94) at /data/linux-ipipe/kernel/xenomai/posix/thread.c:604
#13 0xffffffff8117031a in handle_root_syscall (ipd=<optimized out>, regs=<optimized out>) at /data/linux-ipipe/kernel/xenomai/posix/syscall.c:950
#14 ipipe_syscall_hook (ipd=<optimized out>, regs=0xffff88003a4a7f58) at /data/linux-ipipe/kernel/xenomai/posix/syscall.c:999
#15 0xffffffff810e97a7 in __ipipe_notify_syscall (regs=<optimized out>) at /data/linux-ipipe/kernel/ipipe/core.c:1045
#16 <signal handler called>
#17 0x00007fdf5591caff in ?? ()
#18 0xffff88003b9ef220 in ?? ()
#19 0xffffffff819a75a0 in ?? ()
#20 0x0000000000000000 in ?? ()

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] __cobalt_thread_create: cleanup after failing cobalt_map_user

[Philippe Gerum]

On 11/25/2014 08:42 PM, Jan Kiszka wrote:
> Hi,
> 
> I ran into an oops (bt below) when (for yet unknown reasons)
> cobalt_map_user fails. The reason is that we then call xnthread_cancel,
> but on a thread that has no user space task assigned yet.
>

Ack, it's terminally broken.

> Which functions rolls only pthread_create back?
> 

None, we need one which specifically applies to a dormant, unmapped thread. The existing code assumes a mapped thread.

> The issue should apply to cobalt_thread_shadow as well.
> 

Correct. In theory, this patch should fix it. In practice, it's untested code in all its glory:

diff --git a/include/cobalt/kernel/thread.h b/include/cobalt/kernel/thread.h
index 72168f4..535489b 100644
--- a/include/cobalt/kernel/thread.h
+++ b/include/cobalt/kernel/thread.h
@@ -332,6 +332,8 @@ void __xnthread_test_cancel(struct xnthread *curr);
 
 void __xnthread_cleanup(struct xnthread *curr);
 
+void __xnthread_discard(struct xnthread *thread);
+
 /**
  * @fn struct xnthread *xnthread_current(void)
  * @brief Retrieve the current Cobalt core TCB.
diff --git a/kernel/cobalt/posix/thread.c b/kernel/cobalt/posix/thread.c
index 3ab20f6..7c78405 100644
--- a/kernel/cobalt/posix/thread.c
+++ b/kernel/cobalt/posix/thread.c
@@ -345,10 +345,10 @@ unlock_and_exit:
 	return 0;
 }
 
-static inline int pthread_create(struct cobalt_thread **thread_p,
-				 int policy,
-				 const struct sched_param_ex *param_ex,
-				 struct task_struct *task)
+static int pthread_create(struct cobalt_thread **thread_p,
+			  int policy,
+			  const struct sched_param_ex *param_ex,
+			  struct task_struct *task)
 {
 	struct xnsched_class *sched_class;
 	union xnsched_policy_param param;
@@ -416,6 +416,20 @@ static inline int pthread_create(struct cobalt_thread **thread_p,
 	return 0;
 }
 
+static void pthread_discard(struct cobalt_thread *thread)
+{
+	spl_t s;
+
+	xnsynch_destroy(&thread->monitor_synch);
+	xnsynch_destroy(&thread->sigwait);
+
+	xnlock_get_irqsave(&nklock, s);
+	list_del(&thread->link);
+	xnlock_put_irqrestore(&nklock, s);
+	__xnthread_discard(&thread->threadbase);
+	xnfree(thread);
+}
+
 static inline int pthread_setmode_np(int clrmask, int setmask, int *mode_r)
 {
 	const int valid_flags = XNLOCK|XNWARN|XNTRAPLB;
@@ -566,8 +580,10 @@ int __cobalt_thread_create(unsigned long pth, int policy,
 		return ret;
 
 	ret = cobalt_map_user(&thread->threadbase, u_winoff);
-	if (ret)
-		goto fail;
+	if (ret) {
+		pthread_discard(thread);
+		return ret;
+	}
 
 	if (!thread_hash(&hkey, thread, task_pid_vnr(p))) {
 		ret = -EAGAIN;
@@ -617,11 +633,13 @@ cobalt_thread_shadow(struct task_struct *p,
 	trace_cobalt_pthread_create(hkey->u_pth, SCHED_NORMAL, &param_ex);
 	ret = pthread_create(&thread, SCHED_NORMAL, &param_ex, p);
 	if (ret)
-		return ERR_PTR(-ret);
+		return ERR_PTR(ret);
 
 	ret = cobalt_map_user(&thread->threadbase, u_winoff);
-	if (ret)
-		goto fail;
+	if (ret) {
+		pthread_discard(thread);
+		return ERR_PTR(ret);
+	}
 
 	if (!thread_hash(hkey, thread, task_pid_vnr(p))) {
 		ret = -EAGAIN;
diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index f5444d3..e7726dd 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -492,6 +492,25 @@ void __xnthread_cleanup(struct xnthread *curr)
 	wake_up(&nkjoinq);
 }
 
+/*
+ * Unwinds xnthread_init() ops for an unmapped thread.  Since the
+ * latter must be dormant, it can't be part of any runqueue.
+ */
+void __xnthread_discard(struct xnthread *thread)
+{
+	spl_t s;
+
+	xntimer_destroy(&thread->rtimer);
+	xntimer_destroy(&thread->ptimer);
+
+	xnlock_get_irqsave(&nklock, s);
+	list_del(&thread->glink);
+	nknrthreads--;
+	xnvfile_touch_tag(&nkthreadlist_tag);
+	xnthread_deregister(thread);
+	xnlock_put_irqrestore(&nklock, s);
+}
+
 /**
  * @fn void xnthread_init(struct xnthread *thread,const struct xnthread_init_attr *attr,struct xnsched_class *sched_class,const union xnsched_policy_param *sched_param)
  * @brief Initialize a new thread.

-- 
Philippe.

Re: [Xenomai] __cobalt_thread_create: cleanup after failing cobalt_map_user

[Jan Kiszka]

On 2014-11-25 21:37, Philippe Gerum wrote:
> On 11/25/2014 08:42 PM, Jan Kiszka wrote:
>> Hi,
>>
>> I ran into an oops (bt below) when (for yet unknown reasons)
>> cobalt_map_user fails. The reason is that we then call xnthread_cancel,
>> but on a thread that has no user space task assigned yet.
>>
> 
> Ack, it's terminally broken.
> 
>> Which functions rolls only pthread_create back?
>>
> 
> None, we need one which specifically applies to a dormant, unmapped thread. The existing code assumes a mapped thread.
> 
>> The issue should apply to cobalt_thread_shadow as well.
>>
> 
> Correct. In theory, this patch should fix it. In practice, it's untested code in all its glory:

Wish I could still test, but I happened to have found the trigger of
that issue - an inconsistent kernel build - and destroyed it along the way.

Will check tomorrow via an artificial failure of cobalt_map_user.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] __cobalt_thread_create: cleanup after failing cobalt_map_user

[Jan Kiszka]

On 2014-11-25 21:37, Philippe Gerum wrote:
> On 11/25/2014 08:42 PM, Jan Kiszka wrote:
>> Hi,
>>
>> I ran into an oops (bt below) when (for yet unknown reasons)
>> cobalt_map_user fails. The reason is that we then call xnthread_cancel,
>> but on a thread that has no user space task assigned yet.
>>
> 
> Ack, it's terminally broken.
> 
>> Which functions rolls only pthread_create back?
>>
> 
> None, we need one which specifically applies to a dormant, unmapped thread. The existing code assumes a mapped thread.
> 
>> The issue should apply to cobalt_thread_shadow as well.
>>
> 
> Correct. In theory, this patch should fix it. In practice, it's untested code in all its glory:

Looks good, test passed here.

> 
> diff --git a/include/cobalt/kernel/thread.h b/include/cobalt/kernel/thread.h
> index 72168f4..535489b 100644
> --- a/include/cobalt/kernel/thread.h
> +++ b/include/cobalt/kernel/thread.h
> @@ -332,6 +332,8 @@ void __xnthread_test_cancel(struct xnthread *curr);
>  
>  void __xnthread_cleanup(struct xnthread *curr);
>  
> +void __xnthread_discard(struct xnthread *thread);
> +
>  /**
>   * @fn struct xnthread *xnthread_current(void)
>   * @brief Retrieve the current Cobalt core TCB.
> diff --git a/kernel/cobalt/posix/thread.c b/kernel/cobalt/posix/thread.c
> index 3ab20f6..7c78405 100644
> --- a/kernel/cobalt/posix/thread.c
> +++ b/kernel/cobalt/posix/thread.c
> @@ -345,10 +345,10 @@ unlock_and_exit:
>  	return 0;
>  }
>  
> -static inline int pthread_create(struct cobalt_thread **thread_p,
> -				 int policy,
> -				 const struct sched_param_ex *param_ex,
> -				 struct task_struct *task)
> +static int pthread_create(struct cobalt_thread **thread_p,
> +			  int policy,
> +			  const struct sched_param_ex *param_ex,
> +			  struct task_struct *task)
>  {
>  	struct xnsched_class *sched_class;
>  	union xnsched_policy_param param;
> @@ -416,6 +416,20 @@ static inline int pthread_create(struct cobalt_thread **thread_p,
>  	return 0;
>  }
>  
> +static void pthread_discard(struct cobalt_thread *thread)
> +{
> +	spl_t s;
> +
> +	xnsynch_destroy(&thread->monitor_synch);
> +	xnsynch_destroy(&thread->sigwait);
> +
> +	xnlock_get_irqsave(&nklock, s);
> +	list_del(&thread->link);
> +	xnlock_put_irqrestore(&nklock, s);
> +	__xnthread_discard(&thread->threadbase);
> +	xnfree(thread);
> +}
> +
>  static inline int pthread_setmode_np(int clrmask, int setmask, int *mode_r)
>  {
>  	const int valid_flags = XNLOCK|XNWARN|XNTRAPLB;
> @@ -566,8 +580,10 @@ int __cobalt_thread_create(unsigned long pth, int policy,
>  		return ret;
>  
>  	ret = cobalt_map_user(&thread->threadbase, u_winoff);
> -	if (ret)
> -		goto fail;
> +	if (ret) {
> +		pthread_discard(thread);
> +		return ret;
> +	}
>  
>  	if (!thread_hash(&hkey, thread, task_pid_vnr(p))) {
>  		ret = -EAGAIN;
> @@ -617,11 +633,13 @@ cobalt_thread_shadow(struct task_struct *p,
>  	trace_cobalt_pthread_create(hkey->u_pth, SCHED_NORMAL, &param_ex);
>  	ret = pthread_create(&thread, SCHED_NORMAL, &param_ex, p);
>  	if (ret)
> -		return ERR_PTR(-ret);
> +		return ERR_PTR(ret);
>  
>  	ret = cobalt_map_user(&thread->threadbase, u_winoff);
> -	if (ret)
> -		goto fail;
> +	if (ret) {
> +		pthread_discard(thread);
> +		return ERR_PTR(ret);
> +	}

After removing this jump source of the fail label, you could fold the
latter into its last user.

Jan

>  
>  	if (!thread_hash(hkey, thread, task_pid_vnr(p))) {
>  		ret = -EAGAIN;
> diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
> index f5444d3..e7726dd 100644
> --- a/kernel/cobalt/thread.c
> +++ b/kernel/cobalt/thread.c
> @@ -492,6 +492,25 @@ void __xnthread_cleanup(struct xnthread *curr)
>  	wake_up(&nkjoinq);
>  }
>  
> +/*
> + * Unwinds xnthread_init() ops for an unmapped thread.  Since the
> + * latter must be dormant, it can't be part of any runqueue.
> + */
> +void __xnthread_discard(struct xnthread *thread)
> +{
> +	spl_t s;
> +
> +	xntimer_destroy(&thread->rtimer);
> +	xntimer_destroy(&thread->ptimer);
> +
> +	xnlock_get_irqsave(&nklock, s);
> +	list_del(&thread->glink);
> +	nknrthreads--;
> +	xnvfile_touch_tag(&nkthreadlist_tag);
> +	xnthread_deregister(thread);
> +	xnlock_put_irqrestore(&nklock, s);
> +}
> +
>  /**
>   * @fn void xnthread_init(struct xnthread *thread,const struct xnthread_init_attr *attr,struct xnsched_class *sched_class,const union xnsched_policy_param *sched_param)
>   * @brief Initialize a new thread.
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux