You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/03/24 16:26:49 UTC
[GitHub] [incubator-nuttx] v01d opened a new issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
v01d opened a new issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168
As per recent discussion, some APIs such as getopt() use global variables internally. This means getopt() calls between different tasks (or even calls done in subsequent runs of the task) will see changes of these globals. The idea is to move these variables to TLS and provide safety at the thread level.
Note that even when reentrant versions of a function exist, code expecting a non-reentrant function to work will still break due to this interference so it is not a solution to just switch to a reentrant version of a non-reentrant POSIX interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808200522
> I meant to copy this here yesterday but I accientally put this in PR #3182:
>
> > > From PR #3170
> > > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
> >
> >
> > Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
>
> I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
>
The non-standard OS interface already exist, we don't need new one.
> I think would need:
>
> * A correctly working getpid(), and
But it isn't really required. If we reserve -1 as the current main thread(just like 0 mean the current thread).
> * A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
>
You have defined one in errno patch:
```
int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
```
And do the correct security check:
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
/* It is an application thread. It is permitted to query
* only threads within the same task group. It is not permitted
* to peek into the stacks of either kernel threads or other
* applications tasks.
*/
if (rtcb->group != qtcb->group)
{
return -EACCES;
}
}
```
> So I think we are dead in the water for now:
>
> * getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
> * We don't have agreement on how to do process-specific data
>
> I will do something else today. This is not ready to implement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809779539
> To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
pthread keys would not be used in either case. I used thread specific data in the version that we declined. But neither what Xiang or I propose would use it. Instead then would implement task-specific data: In one case by adding the data to main thread TLS (yech) or by adding allocated memory to the task_group_s as you mention.
I would personally only agree with the latter. I believe making the TLS storage different on the main thread is a bad decision. The stack initialization and checking logic in every architecture would have to be changed, for example. I definitely will not do that and do not recommend that anyone does that. It is just a bad idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808200522
> I meant to copy this here yesterday but I accientally put this in PR #3182:
>
> > > From PR #3170
> > > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
> >
> >
> > Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
>
> I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
>
The non-standard OS interface already exist, we don't need new one.
> I think would need:
>
> * A correctly working getpid(), and
But it isn't really required. If we reserve -1 as the current main thread(just like 0 mean the current thread).
> * A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
>
You have defined one in errno patch:
```
int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
```
And do the correct security check:
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
/* It is an application thread. It is permitted to query
* only threads within the same task group. It is not permitted
* to peek into the stacks of either kernel threads or other
* applications tasks.
*/
if (rtcb->group != qtcb->group)
{
return -EACCES;
}
}
```
> So I think we are dead in the water for now:
>
> * getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
> * We don't have agreement on how to do process-specific data
>
> I will do something else today. This is not ready to implement.
So, all primitive funtions are there, the major work is to reorginize the data struture and source file. Yes, the reorg self is a huge change. I propose to do the mirgration step by step:
1. Implement something like tls_info_s but for task
2. Move the simple(global variables e.g. g_mask, h_errno or g_saveptr) to per-task or per-thread storage
3. Implment pthread/task destruct callback to handle the dynamical allocaiton(e.g. g_hostbuffer)
4. Migrate the rest global varible to the new location
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810231189
> I have a small but important improvement to this proposal: Use up_stack_frame to allocate memory for globals. up_stack_frame() is currently used to allocate memory at the beginning (highest address of the stack). This would have the advantage that it would put the global data in the main thread's stack as you desire.
>
I like this ideal, because tls_info_s could also call up_stack_frame to reserve memory, it will decouple thread local storage from arch code too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809675860
>
>
> @xiaoxiang781216 do you have plans to move forward with this issue? would be good to reach a solution.
My objection to implementing this is that I do not believe that combining this task-specific data into the main thread's TLS is the correct technical decision. I prefer that it were implement in the same fashion as is other task-specific data (file descriptors, streams). Moving this as a special case into the main thread TLS makes the design inconsistent and complicates things like stack initialization, stack coloration, and stack overflow detection. So I cannot implement that design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809706113
Environment variables (setenv(), getenv()) is another good example of the model for OS managment of task-specific user data. This is my preferred technical approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808200522
> I meant to copy this here yesterday but I accientally put this in PR #3182:
>
> > > From PR #3170
> > > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
> >
> >
> > Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
>
> I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
>
The non-standard OS interface already exist, we don't need new one.
> I think would need:
>
> * A correctly working getpid(), and
But it isn't really required. If we reserve -1 as the current main thread(just like 0 mean the current thread).
> * A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
>
You have defined one in errno patch:
```
int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
```
And do the correct security check:
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
/* It is an application thread. It is permitted to query
* only threads within the same task group. It is not permitted
* to peek into the stacks of either kernel threads or other
* applications tasks.
*/
if (rtcb->group != qtcb->group)
{
return -EACCES;
}
}
```
> So I think we are dead in the water for now:
>
> * getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
> * We don't have agreement on how to do process-specific data
>
> I will do something else today. This is not ready to implement.
So, all primitive funtions are there, the major work is to reorginize the data struture and source file. Yes, the reorg self is a huge change. I propose to do the mirgration step by step:
1. Implement something like tls_info_s but for struct
2. Move the simple(global variables e.g. g_mask, h_errno or g_saveptr) to per-task or per-thread storage
3. Implment pthread/task destruct callback to handle the dynamical allocaiton(e.g. g_hostbuffer)
4. Migrate the rest global varible to the new location
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809779539
> To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
pthread keys would not be used in either case. I used thread specific data in the version that we declined. But neither what Xiang or I propose would use it. Instead then would implement task-specific data: In one case by adding the data to main thread TLS (yech) or by adding allocated memory to the task_group_s as you mention.
I would personally only agree with the latter. I believe making the TLS storage different on the main thread is a bad decision. The stack initialization and checking logic in every architecture would have to be changed, for example. I definitely will not do that and do not recommend that anyone does that. It is just a bad idea.
The checking is not effected, but ALL of the following would have to be modified:
```
arch/arm/src/common/arm_createstack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/arm/src/common/arm_usestack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/avr/src/avr/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr32/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/include/tls.h:static inline FAR struct tls_info_s *up_tls_info(void)
arch/hc/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/mips/src/common/mips_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/lm32/lm32_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/minerva/minerva_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/renesas/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_usestack.c: sizeof(struct tls_info_s)),
arch/risc-v/src/common/riscv_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/sim/src/sim/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: memset(stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_createstack.c: sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size = tcb->adj_stack_size - sizeof(struct tls_info_s);
arch/sim/src/sim/up_usestack.c: memset(stack, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_usestack.c: sizeof(struct tls_info_s)),
arch/sim/src/sim/up_usestack.c: adj_stack_size - sizeof(struct tls_info_s));
arch/x86/src/i486/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86/src/i486/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86/src/i486/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86_64/src/intel64/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_checkstack.c: start = STACK_ALIGN_UP(alloc + sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/xtensa/src/common/xtensa_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: sizeof(struct tls_info_s),
arch/xtensa/src/common/xtensa_createstack.c: tcb->adj_stack_size - sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: sizeof(struct tls_info_s)),
arch/xtensa/src/common/xtensa_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/z16/src/common/z16_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z16/src/common/z16_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z16/src/common/z16_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z80/src/common/z80_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
```
Basically, you would need to change all occurrences of sizeof(struct tls_info_s) with conditional logic: If this is a pthread, use one size; if it is the main thread, use another size. This is just pointless collateral damage.
Keeping TLS data consistent for all threads is important to the integrity of the system for a number of reasons. I would not take part in any effort that ruins that nice property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809779539
> To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
pthread keys would not be used in either case. I used thread specific data in the version that we declined. But neither what Xiang or I propose would use it. Instead then would implement task-specific data: In one case by adding the data to main thread TLS (yech) or by adding allocated memory to the task_group_s as you mention.
I would personally only agree with the latter. I believe making the TLS storage different on the main thread is a bad decision. The stack initialization and checking logic in every architecture would have to be changed, for example. I definitely will not do that and do not recommend that anyone does that. It is just a bad idea.
The checking is not effected, but ALL of the following would have to be modified:
```
arch/arm/src/common/arm_createstack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/arm/src/common/arm_usestack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/avr/src/avr/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr32/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/include/tls.h:static inline FAR struct tls_info_s *up_tls_info(void)
arch/hc/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/mips/src/common/mips_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/lm32/lm32_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/minerva/minerva_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/renesas/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_usestack.c: sizeof(struct tls_info_s)),
arch/risc-v/src/common/riscv_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/sim/src/sim/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: memset(stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_createstack.c: sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size = tcb->adj_stack_size - sizeof(struct tls_info_s);
arch/sim/src/sim/up_usestack.c: memset(stack, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_usestack.c: sizeof(struct tls_info_s)),
arch/sim/src/sim/up_usestack.c: adj_stack_size - sizeof(struct tls_info_s));
arch/x86/src/i486/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86/src/i486/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86/src/i486/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86_64/src/intel64/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_checkstack.c: start = STACK_ALIGN_UP(alloc + sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/xtensa/src/common/xtensa_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: sizeof(struct tls_info_s),
arch/xtensa/src/common/xtensa_createstack.c: tcb->adj_stack_size - sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: sizeof(struct tls_info_s)),
arch/xtensa/src/common/xtensa_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/z16/src/common/z16_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z16/src/common/z16_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z16/src/common/z16_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z80/src/common/z80_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
```
Basically, you would need to change all occurrences of sizeof(struct tls_info_s) with conditional logic: If this is a pthread, use one size; if it is the main thread, use another size.
Keeping TLS data consistent for all threads is important to the integrity of the system for a number of reasons. I would not take part in any effort that ruins that nice property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809675860
>
>
> @xiaoxiang781216 do you have plans to move forward with this issue? would be good to reach a solution.
My objection to implementing this is that I do not believe that combining this task-specific data into the main thread's TLS is the correct technical decision. I prefer that it were implement in the same fashion as is other task-specific data (file descriptors, streams). Moving this as a special case into the main thread TLS makes the design inconsistent and complicates things like stack initialization, stack coloration, and stack overflow detection. It is not a good design and I cannot implement it that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809049180
Yes, this is an important feature to support the multiple global instances. We will assign a dedicated resource to implement the infrastucture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809898417
> It is possible to implement task-specific data without introducing the asymmetries. Consider the following:
>
> 1. Add a pointer like FAR struct libvars_s *libvars to tls_info_s. This would appear in all thread TLS data.
@patacongo this approach is a good compromise. Another question, how to extend the functionality out side the core os and libc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810147142
> @patacongo this approach is a good compromise.
Good. Thank you. I will start looking at this again today. I think it will not take long to implement.. probably only a couple of hours.
> Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
Other than pthread_setspecific(), I am not aware of any standard OS interfaces that could be used.
Also, this would require modification of the third party libraries to access the global data. This is not a fully transparent replacement for global variables.
I would think that most 3rd party libraries are very application specific and would not be used widely in the many tasks. But, of course, some would probably have broad usage such as libc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809706113
Environment variables (setenv(), getenv()) is another good example of the model for OS engagement of task-specific user data. This is my preferred technical approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810202152
> > @patacongo this approach is a good compromise.
>
> Good. Thank you. I will start look at this again today. I think it will not take long to implement.. probably only a couple of hours.
>
> > Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
>
> Other than pthread_setspecific(), I am not aware of any standard OS interfaces that could be used.
>
> Also, this would require modification of the third party libraries to access the global data. This is not a fully transparent replacement for global variables.
>
Yes, it isn't fully transparent, but this may only the possible solution for the flat mode. We plan to split the change into two steps:
1. Move all global variables into a structure and access them through one fucntion(e.g. get_instance)
2. Call the per task storage API to access the global variable in NuttX
The first change is general enough and should be acceptable by the maintainer. The second change is specific to NuttX and can't upstream, but the change is very small on top of the first one.
> I would think that most 3rd party libraries are very application specific and would not be used widely in the many tasks. But, of course, some would probably have broad usage such as libc.
But, some system level library(e.g. libuv) has to support the multiple task usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-811219766
Many times in the past, I have implemented limited scope data by declaring the data on the stack. For example:
```
int foo(int argc, char **argv, char **envp)
{
struct state_info_s info;
memset(&info, 0, sizeof(struct state_info_s));
func1(&info);
func2(&info);
...
}
```
This is easy, fast, and efficient. But it has the downside that you often requires a substantial change to the existing design to pass the state data as a parameter. But it is a better solution that using an inefficient OS-based solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809706113
Environment variables (setenv(), getenv()) is another good example of the model for OS managment of user data
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809779539
> To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
pthread keys would not be used in either case. I used thread specific data in the version that we declined. But neither what Xiang or I propose would use it. Instead then would implement task-specific data: In one case by adding the data to main thread TLS (yech) or by adding allocated memory to the task_group_s as you mention.
I would personally only agree with the latter. I believe making the TLS storage different on the main thread is a bad decision. The stack initialization and checking logic in every architecture would have to be changed, for example. I definitely will not do that and do not recommend that anyone does that. It is just a bad idea.
The checking is not effected, but ALL of the following would have to be modified:
```
arch/arm/src/common/arm_createstack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/arm/src/common/arm_usestack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/avr/src/avr/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr32/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/include/tls.h:static inline FAR struct tls_info_s *up_tls_info(void)
arch/hc/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/mips/src/common/mips_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/lm32/lm32_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/minerva/minerva_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/renesas/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_usestack.c: sizeof(struct tls_info_s)),
arch/risc-v/src/common/riscv_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/sim/src/sim/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: memset(stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_createstack.c: sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size = tcb->adj_stack_size - sizeof(struct tls_info_s);
arch/sim/src/sim/up_usestack.c: memset(stack, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_usestack.c: sizeof(struct tls_info_s)),
arch/sim/src/sim/up_usestack.c: adj_stack_size - sizeof(struct tls_info_s));
arch/x86/src/i486/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86/src/i486/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86/src/i486/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86_64/src/intel64/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_checkstack.c: start = STACK_ALIGN_UP(alloc + sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/xtensa/src/common/xtensa_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: sizeof(struct tls_info_s),
arch/xtensa/src/common/xtensa_createstack.c: tcb->adj_stack_size - sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: sizeof(struct tls_info_s)),
arch/xtensa/src/common/xtensa_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/z16/src/common/z16_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z16/src/common/z16_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z16/src/common/z16_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z80/src/common/z80_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
```
Keeping TLS data consistent for all threads is important to the integrity of the system for a number of reasons. I would not take part in any effort that ruins that nice property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808200522
> I meant to copy this here yesterday but I accientally put this in PR #3182:
>
> > > From PR #3170
> > > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
> >
> >
> > Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
>
> I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
>
The non-standard OS interface already exist, we don't need new one.
> I think would need:
>
> * A correctly working getpid(), and
But it isn't really required. If we reserve -1 as the current main thread(just like 0 mean the current thread).
> * A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
>
You have defined one in errno patch:
```
int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
```
And do the correct security check:
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
/* It is an application thread. It is permitted to query
* only threads within the same task group. It is not permitted
* to peek into the stacks of either kernel threads or other
* applications tasks.
*/
if (rtcb->group != qtcb->group)
{
return -EACCES;
}
}
```
> So I think we are dead in the water for now:
>
> * getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
> * We don't have agreement on how to do process-specific data
>
> I will do something else today. This is not ready to implement.
So, all primitive funtions are there, the major work is to reorginize the data struture and source file. Yes, the reorg self is a huge change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-811219766
Many times in the past, I have implemented limited scope data by declaring the data on the stack. For example:
```
int foo(int argc, char **argv, char **envp)
{
struct state_info_s info;
memset(&info, 0, sizeof(struct state_info_s));
func1(&info);
func2(&info);
...
}
```
This is easy, fast, and efficient. But it has the downside that you often requires a substantial change to the existing design to pass the state data as a parameter. But it is a better solution that using a inefficient OS-based solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809675860
>
>
> @xiaoxiang781216 do you have plans to move forward with this issue? would be good to reach a solution.
My objection to implementing this is that I do not believe that combining this task-specific data into the main thread's TLS is the correct technical decision. I prefer that it were implement in the same fashion as is other task-specific data (file descriptors, streams). Moving this as a special case into the main thread TLS introduces asymmetries, makes the design inconsistent, and complicates things like stack initialization, stack coloration, and stack overflow detection. It is not a good design and I cannot implement it that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810146464
I have a small but important improvement to this proposal: Use up_stack_frame to allocate memory for globals. up_stack_frame() is currently used to allocate memory at the beginning (highest address of the stack). This would have the advantage that it would put the global data in the main thread's stack as you desire.
> It is possible to implement task-specific data without introducing the asymmetries. Consider the following:
>
> 1. Add a pointer like FAR struct libvars_s *libvars to tls_info_s. This would appear in all thread TLS data.
>
> 2. When the main thread is created, it would allocate memory for the library variables and assign them to tls->libvars.
It would allocate the memory using up_stack_frame()
> 3. When a new thread is created in the task group, it would inherit tls->libvars from its parent. Then all threads in the group would have the same pointer to the same data, making it common to all threads in the task.
>
> 4. When the task terminates, the allocated library variables would be freed.
Since the stack is automatically freed, nothing special need to be done.
> Disadvantages: Adds a pointer to the size of TLS. Advantages: Not very much but might be a path to compromise. The only improvement I can think of is with CONFIG_BUILD_KERNEL=y. In that case, TLS can be accessed without a system call so it would be a performance enhancer in that configuration. But this is not really an advantage because we should be using normal globals in CONFIG_BUILD_KERNEL=y because global data is naturally process-specific in that case
>
> Accessing the main thread would require a system call in all cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810147142
> @patacongo this approach is a good compromise. Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
Other than pthread_setspecific(), I am not aware of any standard OS interfaces that could be used.
And also, this would require modification of the third party libraries to access the global data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810147142
> @patacongo this approach is a good compromise.
Good. Thank you. I will start look at this again today. I think it will not take long to implement.. probably only a couple of hours.
> Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
Other than pthread_setspecific(), I am not aware of any standard OS interfaces that could be used.
Also, this would require modification of the third party libraries to access the global data. This is not a fully transparent replacement for global variables.
I would think that most 3rd party libraries are very application specific and would not be used widely in the many tasks. But, of course, some would probably have broad usage such as libc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
v01d commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808963851
@xiaoxiang781216 do you have plans to move forward with this issue? would be good to reach a solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809699196
It is possible to implement task-specific data without introducing the asymmetries. Consider the following:
1. Add a pointer like FAR struct libvars_s *libvars to tls_info_s. This would appear in all thread TLS data.
2. When the main thread is created, it would allocate memory for the library variables and assign them to tls->libvars.
3. When a new thread is created in the task group, it would inherit tls->libvars from its parent. Then all threads in the group would have the same pointer to the same data, making it common to all threads in the task.
4. When the task terminates, the allocated library variables would be freed.
Disadvantages: Adds a pointer to the size of TLS. Advantages: Not very much but might be a path to compromise. The only improvement I can think of is with CONFIG_BUILD_KERNEL=y. In that case, TLS can be accessed without a system call so it would be a performance enhancer in that configuration. But this is not really an advantage because we should be using normal globals in CONFIG_BUILD_KERNEL=y because global data is naturally process-specific in that case
Accessing the main thread would require a system call in all cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-810202152
> > @patacongo this approach is a good compromise.
>
> Good. Thank you. I will start look at this again today. I think it will not take long to implement.. probably only a couple of hours.
>
> > Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
>
> Other than pthread_setspecific(), I am not aware of any standard OS interfaces that could be used.
>
> Also, this would require modification of the third party libraries to access the global data. This is not a fully transparent replacement for global variables.
>
Yes, it isn't fully transparent, but this may only the possible solution for the flat mode. We plan to split the change into two steps:
1. Move all global variables into a structure and access them through one fucntion(e.g. get_instance)
2. Call the per task storage API to access the global variable in NuttX
The first change is general enough and should be acceptable by the maintainer. The second change is specific to NuttX and then can't upstream, but the change is very small on top of the first one.
> I would think that most 3rd party libraries are very application specific and would not be used widely in the many tasks. But, of course, some would probably have broad usage such as libc.
But, some system level library(e.g. libuv) has to support the multiple task usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808200522
> I meant to copy this here yesterday but I accientally put this in PR #3182:
>
> > > From PR #3170
> > > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
> >
> >
> > Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
>
> I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
>
The non-standard OS interface already exist, we don't need new one.
> I think would need:
>
> * A correctly working getpid(), and
But it isn't really required. If we reserve -1 as the current main thread(just like 0 mean the current thread).
> * A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
>
You have defined one in errno patch:
```
int nxsched_get_stackinfo(pid_t pid, FAR struct stackinfo_s *stackinfo)
```
And do the correct security check:
```
if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) != TCB_FLAG_TTYPE_KERNEL)
{
/* It is an application thread. It is permitted to query
* only threads within the same task group. It is not permitted
* to peek into the stacks of either kernel threads or other
* applications tasks.
*/
if (rtcb->group != qtcb->group)
{
return -EACCES;
}
}
```
> So I think we are dead in the water for now:
>
> * getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
> * We don't have agreement on how to do process-specific data
>
> I will do something else today. This is not ready to implement.
So, all primitive funtions are there, the major work is to reorginize the data struture and source file. Yes, the reorg self is a huge change. I propose to do the mirgration step by step:
1. Implement something like tls_info_s but for task
2. Move the simple(global variables e.g. g_mask, h_errno or g_saveptr) to per-task or per-thread storage
3. Implment pthread/task destruct callback to handle the dynamical allocaiton(e.g. g_hostbuffer)
4. Migrate the rest global variables to the new location
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-808191946
I meant to copy this here yesterday but I accientally put this in PR #3182:
> > From PR #3170
> > I am beginning to think that the best way to implement per-process globals might be the same way that we already implement other per-process, application data like file descriptor, streams, etc. Allocate then with umm_malloc(), but retain them in the group structure with the system call to access them.
>
> Yes, it's another approach, but why not move all pure userspace stuff to the main stack? So we don't have to put some special FILE function inside the kernel space.
I don't know how to do that and moving ALL of the userspace stuff to the main stack is more than I can offer to do now. There is no interface that will support a thread's access to data stored in the main thread's stack. How would you do that? A new non-standard OS interface?
I think would need:
- A correctly working getpid(), and
- A new TLS interface that will get the TLS data from the main thread (or perhaps any thread given the pid? No, that would be a security problem).
So I think we are dead in the water for now:
- getopt() needs process-specific data (It does not need TLS or a pthread data destructor).
- We don't have agreement on how to do process-specific data
I will do something else today. This is not ready to implement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
v01d commented on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809759508
> Environment variables (setenv(), getenv()) is another good example of the model for OS managment of task-specific user data. This is my preferred technical approach.
So this case would be implemented by adding a pointer to task_group_s?
To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809898417
> It is possible to implement task-specific data without introducing the asymmetries. Consider the following:
>
> 1. Add a pointer like FAR struct libvars_s *libvars to tls_info_s. This would appear in all thread TLS data.
@patacongo this approach is a good compromise. Another question, how to extend the functionality out side the core os and libc? It's very important to allow 3rd party library allocate and manage the per-task variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809779539
> To me both approaches sound reasonable, the second one being decoupled from pthread (which can be good, unless the functionality to support this ends up being the same as the whole pthread key handling).
pthread keys would not be used in either case. I used thread specific data in the version that we declined. But neither what Xiang or I propose would use it. Instead then would implement task-specific data: In one case by adding the data to main thread TLS (yech) or by adding allocated memory to the task_group_s as you mention.
I would personally only agree with the latter. I believe making the TLS storage different on the main thread is a bad decision. The stack initialization and checking logic in every architecture would have to be changed, for example. I definitely will not do that and do not recommend that anyone does that. It is just a bad idea.
The checking is not effected, but ALL of the following would have to be modified:
```
arch/arm/src/common/arm_createstack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/arm/src/common/arm_usestack.c: tls_size = INT32_ALIGN_UP(sizeof(struct tls_info_s));
arch/avr/src/avr/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/avr/src/avr32/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/avr/src/avr32/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/include/tls.h:static inline FAR struct tls_info_s *up_tls_info(void)
arch/hc/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/hc/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/mips/src/common/mips_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/mips/src/common/mips_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/lm32/lm32_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/lm32/lm32_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/misoc/src/minerva/minerva_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/misoc/src/minerva/minerva_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_createstack.c: sizeof(struct tls_info_s);
arch/or1k/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/renesas/src/common/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/renesas/src/common/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_createstack.c: sizeof(struct tls_info_s);
arch/risc-v/src/common/riscv_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/risc-v/src/common/riscv_usestack.c: sizeof(struct tls_info_s)),
arch/risc-v/src/common/riscv_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/sim/src/sim/up_checkstack.c: start = alloc + sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: memset(stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_createstack.c: sizeof(struct tls_info_s);
arch/sim/src/sim/up_createstack.c: stack_size = tcb->adj_stack_size - sizeof(struct tls_info_s);
arch/sim/src/sim/up_usestack.c: memset(stack, 0, sizeof(struct tls_info_s));
arch/sim/src/sim/up_usestack.c: sizeof(struct tls_info_s)),
arch/sim/src/sim/up_usestack.c: adj_stack_size - sizeof(struct tls_info_s));
arch/x86/src/i486/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86/src/i486/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86/src/i486/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/x86_64/src/intel64/up_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/x86_64/src/intel64/up_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_checkstack.c: start = STACK_ALIGN_UP(alloc + sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/xtensa/src/common/xtensa_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_createstack.c: sizeof(struct tls_info_s),
arch/xtensa/src/common/xtensa_createstack.c: tcb->adj_stack_size - sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/xtensa/src/common/xtensa_usestack.c: sizeof(struct tls_info_s)),
arch/xtensa/src/common/xtensa_usestack.c: size_of_stack - sizeof(struct tls_info_s));
arch/z16/src/common/z16_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z16/src/common/z16_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z16/src/common/z16_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_createstack.c: stack_size += sizeof(struct tls_info_s);
arch/z80/src/common/z80_createstack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
arch/z80/src/common/z80_usestack.c: memset(tcb->stack_alloc_ptr, 0, sizeof(struct tls_info_s));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809049180
Yes, this is an important feature to support the multiple global instances per task. We will assign a dedicated resource to implement the infrastucture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809706113
Environment variables (setenv(), getenv()) is another good example of the model for OS engagement of task-specific user data
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #3168: Move internal globals to TLS to protect mutual access on FLAT mode
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #3168:
URL: https://github.com/apache/incubator-nuttx/issues/3168#issuecomment-809049180
Yes, this is an important feature to support the multiple global instances per task, not only for libc internal use, but also for userspace application. We will assign a dedicated resource to implement the infrastucture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org