Hello, Please find attached a new patch that should take into account your latest remarks. This patch has been created using 'git diff -p' command and applies on 2.4.10 revision. I'm not sure I fully understand your remark on missing critical section. Indeed, this code is similar to __t_delete routine that doesn't seem to have critical section enforcement? Should a critical section be added to it as well ? Anyway, I tried to replicate __t_ident mechanism as you mentioned.
Please let me know your feeling about this. Fabrice > -------- Message original -------- > Sujet: Re: [Xenomai-help] How to fix t_delete call in psos skin? > Date: Fri, 06 Nov 2009 12:28:33 +0100 > De: Philippe Gerum <r...@xenomai.org> > Organisation: Xenomai > Pour :: Alexandre Coffignal <alexandre.coffig...@cenosys.com> > Copie à :: xenomai-core@gna.org > Références: <4af1474c.7000...@cenosys.com> > <1257331856.2210.85.ca...@redshift.xenomai.org> > <4af2aef9.2050...@cenosys.com> > > > > On Thu, 2009-11-05 at 11:54 +0100, Alexandre Coffignal wrote: > > >> Does this fix seem more suitable to you? >> Do you see someting else missing? >> >> > > The logic is fine, but the implementation should get closer to what is > being done in the -head branch. > > Preliminary remarks: > > - Please follow the kernel CodingStyle guidelines for anything that is > Xenomai-related, including userland code. Keeping a single code style > throughout our code makes things easier for reviewing, and keeps visual > pollution to the lowest possible level. > > - Please rebase your work on 2.4.10 for pushing those changes to Xenomai > mainline. There is no point for us to consider changes to legacy > releases which are not being actively maintained anymore; you can always > backport those changes to 2.4.7 locally if you really fancy dealing with > bugs solved between 2.4.7 and 2.4.10... > > - Please tell your CVS/SVN diff tool to use -p, when formatting patches. > > >> Please advice. >> > > >> plain text document attachment (patch_psos_t_delete) >> Index: xenomai-2.4.7_bugless/include/psos+/syscall.h >> =================================================================== >> --- xenomai-2.4.7_bugless.orig/include/psos+/syscall.h 2009-11-05 >> 09:12:29.000000000 +0100 >> +++ xenomai-2.4.7_bugless/include/psos+/syscall.h 2009-11-05 >> 09:13:14.000000000 +0100 >> @@ -73,6 +73,7 @@ >> #define __psos_as_send 45 >> /* Xenomai extension: get raw count of jiffies */ >> #define __psos_tm_getc 46 >> +#define __psos_t_get_pthread 47 /* get hidden pthread_t >> identifier. */ >> > > psos_t_getpth is enough. To be really, really pSOS-conformant when > introducing a routine, we should either use: > > 1) a preposterously long argument list > > or, at the very least: > > 2) an absolutely unpronounceable C identifier > > __psos_t_getpth happily qualifies for #2. > > >> >> #ifdef __KERNEL__ >> >> Index: xenomai-2.4.7_bugless/ksrc/skins/psos+/syscall.c >> =================================================================== >> --- xenomai-2.4.7_bugless.orig/ksrc/skins/psos+/syscall.c 2009-11-05 >> 09:16:48.000000000 +0100 >> +++ xenomai-2.4.7_bugless/ksrc/skins/psos+/syscall.c 2009-11-05 >> 09:16:57.000000000 +0100 >> @@ -67,29 +67,37 @@ >> xncompletion_t __user *u_completion; >> u_long prio, flags, tid, err; >> char name[XNOBJECT_NAME_LEN]; >> + struct arg_bulk5 bulk; >> > > Please have a look at what is being done in the -head branch, for the > very same routine (in the absence of -p passed to diff, seems to be > __t_create). An arg bulk has been introduced there, so you just need to > backport this. > > >> psostask_t *task; >> >> - if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), 4)) >> + if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), >> sizeof(bulk))) >> + return -EFAULT; >> + >> + __xn_copy_from_user(curr, &bulk, (void __user *)__xn_reg_arg1(regs), >> + sizeof(bulk)); >> + >> + if (!__xn_access_ok(curr, VERIFY_READ, bulk.a1, sizeof(name))) { >> return -EFAULT; >> + } >> >> /* Get task name. */ >> - __xn_strncpy_from_user(curr, name, (const char __user >> *)__xn_reg_arg1(regs), >> + __xn_strncpy_from_user(curr, name, (const char __user *)bulk.a1, >> sizeof(name) - 1); >> name[sizeof(name) - 1] = '\0'; >> strncpy(curr->comm, name, sizeof(curr->comm)); >> curr->comm[sizeof(curr->comm) - 1] = '\0'; >> >> if (!__xn_access_ok >> - (curr, VERIFY_WRITE, __xn_reg_arg4(regs), sizeof(tid))) >> + (curr, VERIFY_WRITE, bulk.a4, sizeof(tid))) >> return -EFAULT; >> >> /* Task priority. */ >> - prio = __xn_reg_arg2(regs); >> + prio = bulk.a2; >> /* Task flags. Force FPU support in user-space. This will lead >> to a no-op if the platform does not support it. */ >> - flags = __xn_reg_arg3(regs) | T_SHADOW | T_FPU; >> + flags = bulk.a3 | T_SHADOW | T_FPU; >> /* Completion descriptor our parent thread is pending on. */ >> - u_completion = (xncompletion_t __user *)__xn_reg_arg5(regs); >> + u_completion = (xncompletion_t __user *)__xn_reg_arg2(regs); >> >> err = t_create(name, prio, 0, 0, flags, &tid); >> >> @@ -99,7 +107,8 @@ >> * about the new thread id, so we can manipulate its >> * TCB pointer freely. */ >> tid = xnthread_handle(&task->threadbase); >> - __xn_copy_to_user(curr, (void __user *)__xn_reg_arg4(regs), >> &tid, >> + task->opaque2 = bulk.a5; /* hidden pthread_t identifier. */ >> + __xn_copy_to_user(curr, (void __user *) bulk.a4, &tid, >> sizeof(tid)); >> err = xnshadow_map(&task->threadbase, u_completion); /* May be >> NULL */ >> } else { >> @@ -1443,6 +1452,36 @@ >> return as_send((u_long)task, signals); >> } >> >> + >> +/* >> + * int __t_get_pthread(u_long tid, u_long *pthread) >> + */ >> +static int __t_get_pthread(struct task_struct *curr, struct pt_regs *regs) >> +{ >> + xnhandle_t handle; >> + psostask_t *task; >> + u_long pthread; >> + >> + handle = __xn_reg_arg1(regs); >> + >> + if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), >> sizeof(u_long))) >> + return -EFAULT; >> + >> + if (handle) >> + task = (psostask_t *)xnregistry_fetch(handle); >> + else >> + task = __psos_task_current(curr); >> + >> + if (!task) >> + return ERR_OBJID; >> > > Critical section enforcement is missing. What if the caller gets > preempted after the handle is fetched, and the non-current tid gets > killed by a concurrent thread right after? See t_ident for an > illustration. > > If the deletion happens after the critical section, well, we would just > have to hope that pthread_cancel() does a good job at validating its > handle, but at least we would not have referred to stale memory on our > end. > > >> + >> + pthread = task->opaque2; /* hidden pthread_t identifier. */ >> + >> + __xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), &pthread, >> sizeof(u_long)); >> + >> + return SUCCESS; >> +} >> + >> static void *psos_shadow_eventcb(int event, void *data) >> { >> struct psos_resource_holder *rh; >> @@ -1524,6 +1563,7 @@ >> [__psos_tm_signal] = {&__tm_signal, __xn_exec_primary}, >> [__psos_as_send] = {&__as_send, __xn_exec_conforming}, >> [__psos_tm_getc] = {&__tm_getc, __xn_exec_any}, >> + [__psos_t_get_pthread] = {&__t_get_pthread, __xn_exec_any}, >> }; >> >> extern xntbase_t *psos_tbase; >> Index: xenomai-2.4.7_bugless/src/skins/psos+/task.c >> =================================================================== >> --- xenomai-2.4.7_bugless.orig/src/skins/psos+/task.c 2009-11-05 >> 09:17:31.000000000 +0100 >> +++ xenomai-2.4.7_bugless/src/skins/psos+/task.c 2009-11-05 >> 09:17:56.000000000 +0100 >> @@ -26,6 +26,15 @@ >> #include <memory.h> >> #include <psos+/psos.h> >> >> +struct arg_bulk5{ >> + >> + u_long a1; >> + u_long a2; >> + u_long a3; >> + u_long a4; >> + u_long a5; >> +}; >> > > Rather use psos_arg_bulk like in -head tree. > > >> + >> extern int __psos_muxid; >> >> struct psos_task_iargs { >> @@ -73,6 +82,7 @@ >> struct sched_param param; >> int policy; >> long err; >> + struct arg_bulk5 bulk; >> >> policy = psos_task_set_posix_priority(iargs->prio, ¶m); >> pthread_setschedparam(pthread_self(), policy, ¶m); >> @@ -81,10 +91,14 @@ >> >> old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden); >> >> - err = XENOMAI_SKINCALL5(__psos_muxid, >> - __psos_t_create, >> - iargs->name, iargs->prio, iargs->flags, >> - iargs->tid_r, iargs->completionp); >> + bulk.a1 = (u_long)iargs->name; >> + bulk.a2 = (u_long)iargs->prio; >> + bulk.a3 = (u_long)iargs->flags; >> + bulk.a4 = (u_long)iargs->tid_r; >> + bulk.a5 = (u_long)pthread_self(); >> + >> + err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, >> iargs->completionp); >> + >> if (err) >> goto fail; >> >> @@ -172,14 +186,19 @@ >> u_long flags, >> u_long *tid_r) >> { >> + struct arg_bulk5 bulk; >> + >> pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); >> >> old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden); >> >> - return XENOMAI_SKINCALL5(__psos_muxid, >> - __psos_t_create, >> - name, prio, flags, >> - tid_r, NULL); >> + bulk.a1 = (u_long)name; >> + bulk.a2 = (u_long)prio; >> + bulk.a3 = (u_long)flags; >> + bulk.a4 = (u_long)tid_r; >> + bulk.a5 = (u_long)pthread_self(); >> + >> + return XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, NULL); >> } >> >> u_long t_start(u_long tid, >> @@ -196,6 +215,18 @@ >> >> u_long t_delete(u_long tid) >> { >> + long err; >> + u_long pthread; >> + >> + if(tid) >> + { >> + err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_get_pthread, >> tid, &pthread); >> + if (err) >> + return err; >> + err = pthread_cancel((pthread_t)pthread); >> + if (err) >> + return err; >> + } >> > > We should handle the self-deletion case specifically: > > + else { > + /* Silently migrate to avoid raising SIGXCPU. */ > + XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN); > + pthread_exit(NULL); > + } > > Actually, we should also test for the case when a non-zero tid ends up > pointing at the current thread as well, e.g. > > if (tid == 0) > goto self_delete; > > err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, > &ptid); > if (err) > return err; > > if ((pthread_t)ptid == pthread_self()) > goto self_delete; > > err = pthread_cancel((pthread_t)ptid); > if (err) > return -err; /* differentiate from pSOS codes */ > > return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid); > self_delete: > /* Silently migrate to avoid raising SIGXCPU. */ > XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN); > pthread_exit(NULL); > > return SUCCESS; /* not reached */ > > >> return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid); >> } >> >> Index: xenomai-2.4.7_bugless/include/psos+/task.h >> =================================================================== >> --- xenomai-2.4.7_bugless.orig/include/psos+/task.h 2009-11-05 >> 09:19:49.000000000 +0100 >> +++ xenomai-2.4.7_bugless/include/psos+/task.h 2009-11-05 >> 09:19:58.000000000 +0100 >> @@ -26,6 +26,16 @@ >> >> #define PSOS_TASK_MAGIC 0x81810101 >> >> +struct arg_bulk5{ >> + >> + u_long a1; >> + u_long a2; >> + u_long a3; >> + u_long a4; >> + u_long a5; >> +}; >> + >> + >> typedef struct psostask { >> >> unsigned magic; /* Magic code - must be first */ >> @@ -64,6 +74,8 @@ >> >> } waitargs; >> >> + u_long opaque2; /* hidden pthread_t identifier. */ >> > > No need to make this more opaque than required to the reviewer; besides, > we only need this field when user-space support is compiled in. > > - u_long opaque2; /* hidden pthread_t identifier. */ > > + #ifdef CONFIG_XENO_OPT_PERVASIVE > + u_long pthread; > + #endif > > >> + >> } psostask_t; >> >> static inline psostask_t *thread2psostask (xnthread_t *t) >> > > >
diff --git a/include/psos+/syscall.h b/include/psos+/syscall.h index 55ee8a0..25c0a2d 100644 --- a/include/psos+/syscall.h +++ b/include/psos+/syscall.h @@ -73,6 +73,16 @@ #define __psos_as_send 45 /* Xenomai extension: get raw count of jiffies */ #define __psos_tm_getc 46 +#define __psos_t_getpth 47 /* get hidden pthread_t identifier. */ + +struct psos_arg_bulk{ + + u_long a1; + u_long a2; + u_long a3; + u_long a4; + u_long a5; +}; #ifdef __KERNEL__ diff --git a/include/psos+/task.h b/include/psos+/task.h index ca01573..7294f0a 100644 --- a/include/psos+/task.h +++ b/include/psos+/task.h @@ -64,6 +64,10 @@ typedef struct psostask { } waitargs; + #ifdef CONFIG_XENO_OPT_PERVASIVE + u_long pthread; /* hidden pthread_t identifier. */ + #endif + } psostask_t; static inline psostask_t *thread2psostask (xnthread_t *t) diff --git a/ksrc/skins/psos+/syscall.c b/ksrc/skins/psos+/syscall.c index 1753901..a16cec5 100644 --- a/ksrc/skins/psos+/syscall.c +++ b/ksrc/skins/psos+/syscall.c @@ -67,29 +67,37 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs) xncompletion_t __user *u_completion; u_long prio, flags, tid, err; char name[XNOBJECT_NAME_LEN]; + struct psos_arg_bulk bulk; psostask_t *task; - if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), 4)) + if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), sizeof(bulk))) return -EFAULT; + __xn_copy_from_user(curr, &bulk, (void __user *)__xn_reg_arg1(regs), + sizeof(bulk)); + + if (!__xn_access_ok(curr, VERIFY_READ, bulk.a1, sizeof(name))) { + return -EFAULT; + } + /* Get task name. */ - __xn_strncpy_from_user(curr, name, (const char __user *)__xn_reg_arg1(regs), + __xn_strncpy_from_user(curr, name, (const char __user *)bulk.a1, sizeof(name) - 1); name[sizeof(name) - 1] = '\0'; strncpy(curr->comm, name, sizeof(curr->comm)); curr->comm[sizeof(curr->comm) - 1] = '\0'; if (!__xn_access_ok - (curr, VERIFY_WRITE, __xn_reg_arg4(regs), sizeof(tid))) + (curr, VERIFY_WRITE, bulk.a4, sizeof(tid))) return -EFAULT; /* Task priority. */ - prio = __xn_reg_arg2(regs); + prio = bulk.a2; /* Task flags. Force FPU support in user-space. This will lead to a no-op if the platform does not support it. */ - flags = __xn_reg_arg3(regs) | T_SHADOW | T_FPU; + flags = bulk.a3 | T_SHADOW | T_FPU; /* Completion descriptor our parent thread is pending on. */ - u_completion = (xncompletion_t __user *)__xn_reg_arg5(regs); + u_completion = (xncompletion_t __user *)__xn_reg_arg2(regs); err = t_create(name, prio, 0, 0, flags, &tid); @@ -99,7 +107,10 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs) * about the new thread id, so we can manipulate its * TCB pointer freely. */ tid = xnthread_handle(&task->threadbase); - __xn_copy_to_user(curr, (void __user *)__xn_reg_arg4(regs), &tid, + #ifdef CONFIG_XENO_OPT_PERVASIVE + task->pthread = bulk.a5; /* hidden pthread_t identifier. */ + #endif + __xn_copy_to_user(curr, (void __user *) bulk.a4, &tid, sizeof(tid)); err = xnshadow_map(&task->threadbase, u_completion); /* May be NULL */ } else { @@ -1443,6 +1454,47 @@ static int __as_send(struct task_struct *curr, struct pt_regs *regs) return as_send((u_long)task, signals); } + +/* + * int __t_getpth(u_long tid, u_long *pthread) + */ +static int __t_getpth(struct task_struct *curr, struct pt_regs *regs) +{ + xnhandle_t handle; + psostask_t *task; + spl_t s; + u_long err = SUCCESS; + u_long pthread; + + handle = __xn_reg_arg1(regs); + + if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), sizeof(u_long))) + return -EFAULT; + + xnlock_get_irqsave(&nklock, s); + + if (handle) + task = (psostask_t *)xnregistry_fetch(handle); + else + task = __psos_task_current(curr); + + if (!task) + err = ERR_OBJID; + else + { + #ifdef CONFIG_XENO_OPT_PERVASIVE + pthread = task->pthread; /* hidden pthread_t identifier. */ + #endif + } + + xnlock_put_irqrestore(&nklock, s); + + if(err == SUCCESS) + __xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), &pthread, sizeof(u_long)); + + return err; +} + static void *psos_shadow_eventcb(int event, void *data) { struct psos_resource_holder *rh; @@ -1524,6 +1576,7 @@ static xnsysent_t __systab[] = { [__psos_tm_signal] = {&__tm_signal, __xn_exec_primary}, [__psos_as_send] = {&__as_send, __xn_exec_conforming}, [__psos_tm_getc] = {&__tm_getc, __xn_exec_any}, + [__psos_t_getpth] = {&__t_getpth, __xn_exec_any}, }; extern xntbase_t *psos_tbase; diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c index 9358ea8..62e2419 100644 --- a/src/skins/psos+/task.c +++ b/src/skins/psos+/task.c @@ -73,6 +73,7 @@ static void *psos_task_trampoline(void *cookie) struct sched_param param; int policy; long err; + struct psos_arg_bulk bulk; policy = psos_task_set_posix_priority(iargs->prio, ¶m); pthread_setschedparam(pthread_self(), policy, ¶m); @@ -81,10 +82,14 @@ static void *psos_task_trampoline(void *cookie) old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden); - err = XENOMAI_SKINCALL5(__psos_muxid, - __psos_t_create, - iargs->name, iargs->prio, iargs->flags, - iargs->tid_r, iargs->completionp); + bulk.a1 = (u_long)iargs->name; + bulk.a2 = (u_long)iargs->prio; + bulk.a3 = (u_long)iargs->flags; + bulk.a4 = (u_long)iargs->tid_r; + bulk.a5 = (u_long)pthread_self(); + + err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, iargs->completionp); + if (err) goto fail; @@ -172,14 +177,19 @@ u_long t_shadow(const char *name, /* Xenomai extension. */ u_long flags, u_long *tid_r) { + struct psos_arg_bulk bulk; + pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL); old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden); - return XENOMAI_SKINCALL5(__psos_muxid, - __psos_t_create, - name, prio, flags, - tid_r, NULL); + bulk.a1 = (u_long)name; + bulk.a2 = (u_long)prio; + bulk.a3 = (u_long)flags; + bulk.a4 = (u_long)tid_r; + bulk.a5 = (u_long)pthread_self(); + + return XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, NULL); } u_long t_start(u_long tid, @@ -196,7 +206,32 @@ u_long t_start(u_long tid, u_long t_delete(u_long tid) { + long err; + u_long ptid; + + if (tid == 0) + goto self_delete; + + err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, &ptid); + if (err) + return err; + + if ((pthread_t)ptid == pthread_self()) + goto self_delete; + + err = pthread_cancel((pthread_t)ptid); + if (err) + return -err; /* differentiate from pSOS codes */ + return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid); + +self_delete: + + /* Silently migrate to avoid raising SIGXCPU. */ + XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN); + pthread_exit(NULL); + + return SUCCESS; /* not reached */ } u_long t_suspend(u_long tid)
_______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core