Re: asprintf failure
On Wed, Apr 26, 2023 at 9:32 AM Fotis Panagiotopoulos wrote: > > Hello, > > I fixed all critical usages of asprintf and strdup. > > These fixes are in the following PRs: > https://github.com/apache/nuttx/pull/9009 > https://github.com/apache/nuttx/pull/9010 > https://github.com/apache/nuttx-apps/pull/1713 > > > The OS itself is now fixed, there should be no other issues pending. > > However, there are some uses that are still to be fixed. > These are in the tools directory and in some apps (strdup mostly). > > They should not affect the stability of the system in any way, and I > consider these "minor" issues. > > Unfortunately, I cannot spend any more time on this. > So, if anyone has any free time, I would encourage you to improve the apps > or the tools in this regard. Thank you for doing this! If I may make a suggestion, it would be helpful to post an issue to the issue tracker about the ones that aren't fixed yet (strdup etc) so that we won't forget to eventually fix them. Thanks again for taking care of these!! Nathan
Re: Usage of mutex_t inside libc
I didn't do a careful analysis, but glancing at the ARMv7-A code, it looks to me that none of the things needed to create stack frames are available for the kernel stack. It looks like the system call entry logic just sets the SP to the top of the kernel stack and does very little more. The address stack base and size of the stack do not get set up in the TCB on a system call so creating a frame with arm_stackframe() would not work. Am I missing something? On 4/26/2023 2:14 PM, Ville Juven wrote: Hi, But, why need involve kheap here? If the security is your concern, you should enable per-thread kernel stack in the first place. It's safe to allocate waitlist/holder from this stack directly, since userspace code can't touch this area too. Not from a security perspective, and of course I have the per-thread kstack enabled (kernel mode won't work without it) but how do you keep the frame from being corrupted / freed until it is not needed any more? Do you suggest the frame is allocated for each waiting thread separately ? How does that work ? I was thinking the first one who waits in sem_wait() allocates the kernel structure, puts a reference to it into e.g. the user semaphore, so other waiters and the kernel itself can find it later (reference being ID, not pointer!). But, which scenario needs to access kernel struct through user semaphore? It is not necessary to make the access via user semaphore, but somehow the kernel side structure needs to be tied to the semaphore, so the kernel can find it. Linux does this by a hash function, if I remember correctly. I'm not sure duplicating this is a good approach, my concern is the real-time aspect. sem_waitirq() is the problem I have, and fixing it is nasty. The semaphore is accessed via tcb->waitobj and this access can happen from interrupt or signal dispatch. This means the context of sem_wait() / sem_post() is _not_ the only place the access happens. sem_waitirq() currently also modifies the semaphore counter value, so it needs access to the userspace counter. This I will do by mapping the page where the counter exists to kernel (implement something like kmap() / vmap()). If you have an alternative suggestion on how to fix sem_waitirq() I will gladly implement it. Br, Ville Juven On Wed, Apr 26, 2023 at 10:34 PM Xiang Xiao wrote: On Thu, Apr 27, 2023 at 2:18 AM Ville Juven wrote: Hi, Yes exactly, this is the basic idea. Like I said, the waitlist/holder structure is needed only when sem_wait needs to block / wait for the semaphore to become available. How to protect the integrity of the stack allocated structure is still a bit open but one option is to use kheap as well. Semantics to be figured out, the solution should be feasible. But, why need involve kheap here? If the security is your concern, you should enable per-thread kernel stack in the first place. It's safe to allocate waitlist/holder from this stack directly, since userspace code can't touch this area too. My idea was to put the handle to this data into the user semaphore, however a pointer must not be used, a handle / integer id is needed, which then holds the pointer (much like files etc). As the user can spoof / destroy the pointer it is unsafe to do that. Spoofing the id can cause the user process to crash, but the kernel integrity remains. But, which scenario needs to access kernel struct through user semaphore? Br, Ville Juven On Wed, Apr 26, 2023 at 9:09 PM Xiang Xiao wrote: But why not allocate list entry and holder temporally inside the kernel thread stack? Both data is just used when the calling thread can't hold, but wait the semphare, this trick is widely used inside Linux kernel. On Thu, Apr 27, 2023 at 1:32 AM Ville Juven wrote: Hi, Thanks for the explanation, it makes sense. I had an idea to move struct sem_s entirely to kernel and make the user use it via a handle (in KERNEL build) but due to the ubiquitous init macros, that is not a feasible solution. I will explore allocating the kernel structures on-demand next. What does this mean ? The kernel side structures (dq_waitlist and priority inheritance holders) are only needed when the semaphore is locked and someone is actually waiting for it to be released. This means that when sem_wait() results in the thread getting blocked, the kernel part can be allocated here and a reference placed into the user sem_t structure. When the semaphore unlocks, the kernel parts can be freed. This should be completely regression free for FLAT/PROTECTED, which is very important for me because I don't want to be responsible for breaking such a fundamental OS API for current users ;) Br, Ville On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt wrote: I have a question about using mutex_t / struct mutex_s inside libs/libc. The mutex_t definition is kernel internal but some modules inside the libs folder use it directly. My question is, is this intentional ? I don't think such modules
Re: Usage of mutex_t inside libc
Hi, >But, why need involve kheap here? If the security is your concern, you >should enable per-thread kernel stack in the first place. It's safe to >allocate waitlist/holder from this stack directly, since userspace code >can't touch this area too. Not from a security perspective, and of course I have the per-thread kstack enabled (kernel mode won't work without it) but how do you keep the frame from being corrupted / freed until it is not needed any more? Do you suggest the frame is allocated for each waiting thread separately ? How does that work ? I was thinking the first one who waits in sem_wait() allocates the kernel structure, puts a reference to it into e.g. the user semaphore, so other waiters and the kernel itself can find it later (reference being ID, not pointer!). > But, which scenario needs to access kernel struct through user semaphore? It is not necessary to make the access via user semaphore, but somehow the kernel side structure needs to be tied to the semaphore, so the kernel can find it. Linux does this by a hash function, if I remember correctly. I'm not sure duplicating this is a good approach, my concern is the real-time aspect. sem_waitirq() is the problem I have, and fixing it is nasty. The semaphore is accessed via tcb->waitobj and this access can happen from interrupt or signal dispatch. This means the context of sem_wait() / sem_post() is _not_ the only place the access happens. sem_waitirq() currently also modifies the semaphore counter value, so it needs access to the userspace counter. This I will do by mapping the page where the counter exists to kernel (implement something like kmap() / vmap()). If you have an alternative suggestion on how to fix sem_waitirq() I will gladly implement it. Br, Ville Juven On Wed, Apr 26, 2023 at 10:34 PM Xiang Xiao wrote: > On Thu, Apr 27, 2023 at 2:18 AM Ville Juven wrote: > > > Hi, > > > > Yes exactly, this is the basic idea. Like I said, the waitlist/holder > > structure is needed only when sem_wait needs to block / wait for the > > semaphore to become available. > > > > How to protect the integrity of the stack allocated structure is still a > > bit open but one option is to use kheap as well. Semantics to be figured > > out, the solution should be feasible. > > > > But, why need involve kheap here? If the security is your concern, you > should enable per-thread kernel stack in the first place. It's safe to > allocate waitlist/holder from this stack directly, since userspace code > can't touch this area too. > > > > My idea was to put the handle to this data into the user semaphore, > however > > a pointer must not be used, a handle / integer id is needed, which then > > holds the pointer (much like files etc). As the user can spoof / destroy > > the pointer it is unsafe to do that. Spoofing the id can cause the user > > process to crash, but the kernel integrity remains. > > > > > But, which scenario needs to access kernel struct through user semaphore? > > > > Br, > > Ville Juven > > > > On Wed, Apr 26, 2023 at 9:09 PM Xiang Xiao > > wrote: > > > > > But why not allocate list entry and holder temporally inside the kernel > > > thread stack? Both data is just used when the calling thread can't > hold, > > > but wait the semphare, this trick is widely used inside Linux kernel. > > > > > > On Thu, Apr 27, 2023 at 1:32 AM Ville Juven > > wrote: > > > > > > > Hi, > > > > > > > > Thanks for the explanation, it makes sense. I had an idea to move > > struct > > > > sem_s entirely to kernel and make the user use it via a handle (in > > KERNEL > > > > build) but due to the ubiquitous init macros, that is not a feasible > > > > solution. > > > > > > > > I will explore allocating the kernel structures on-demand next. > > > > > > > > What does this mean ? The kernel side structures (dq_waitlist and > > > priority > > > > inheritance holders) are only needed when the semaphore is locked and > > > > someone is actually waiting for it to be released. This means that > when > > > > sem_wait() results in the thread getting blocked, the kernel part can > > be > > > > allocated here and a reference placed into the user sem_t structure. > > When > > > > the semaphore unlocks, the kernel parts can be freed. > > > > > > > > This should be completely regression free for FLAT/PROTECTED, which > is > > > very > > > > important for me because I don't want to be responsible for breaking > > > such a > > > > fundamental OS API for current users ;) > > > > > > > > Br, > > > > Ville > > > > > > > > On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt > > > wrote: > > > > > > > > > > > > > > > I have a question about using mutex_t / struct mutex_s inside > > > > > > libs/libc. The mutex_t definition is kernel internal but some > > modules > > > > > > inside the libs folder use it directly. My question is, is this > > > > > > intentional ? I don't think such modules should be using mutex_t. > > > > > > > > > > > > ... > > > > > > > > > > > > My question ? Should
Re: Usage of mutex_t inside libc
On Thu, Apr 27, 2023 at 2:18 AM Ville Juven wrote: > Hi, > > Yes exactly, this is the basic idea. Like I said, the waitlist/holder > structure is needed only when sem_wait needs to block / wait for the > semaphore to become available. > > How to protect the integrity of the stack allocated structure is still a > bit open but one option is to use kheap as well. Semantics to be figured > out, the solution should be feasible. > But, why need involve kheap here? If the security is your concern, you should enable per-thread kernel stack in the first place. It's safe to allocate waitlist/holder from this stack directly, since userspace code can't touch this area too. > My idea was to put the handle to this data into the user semaphore, however > a pointer must not be used, a handle / integer id is needed, which then > holds the pointer (much like files etc). As the user can spoof / destroy > the pointer it is unsafe to do that. Spoofing the id can cause the user > process to crash, but the kernel integrity remains. > > But, which scenario needs to access kernel struct through user semaphore? > Br, > Ville Juven > > On Wed, Apr 26, 2023 at 9:09 PM Xiang Xiao > wrote: > > > But why not allocate list entry and holder temporally inside the kernel > > thread stack? Both data is just used when the calling thread can't hold, > > but wait the semphare, this trick is widely used inside Linux kernel. > > > > On Thu, Apr 27, 2023 at 1:32 AM Ville Juven > wrote: > > > > > Hi, > > > > > > Thanks for the explanation, it makes sense. I had an idea to move > struct > > > sem_s entirely to kernel and make the user use it via a handle (in > KERNEL > > > build) but due to the ubiquitous init macros, that is not a feasible > > > solution. > > > > > > I will explore allocating the kernel structures on-demand next. > > > > > > What does this mean ? The kernel side structures (dq_waitlist and > > priority > > > inheritance holders) are only needed when the semaphore is locked and > > > someone is actually waiting for it to be released. This means that when > > > sem_wait() results in the thread getting blocked, the kernel part can > be > > > allocated here and a reference placed into the user sem_t structure. > When > > > the semaphore unlocks, the kernel parts can be freed. > > > > > > This should be completely regression free for FLAT/PROTECTED, which is > > very > > > important for me because I don't want to be responsible for breaking > > such a > > > fundamental OS API for current users ;) > > > > > > Br, > > > Ville > > > > > > On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt > > wrote: > > > > > > > > > > > > I have a question about using mutex_t / struct mutex_s inside > > > > > libs/libc. The mutex_t definition is kernel internal but some > modules > > > > > inside the libs folder use it directly. My question is, is this > > > > > intentional ? I don't think such modules should be using mutex_t. > > > > > > > > > > ... > > > > > > > > > > My question ? Should these be sem_t or perhaps pthread_mutex_t ? > Both > > > > > of which are valid user APIs. > > > > > > > > It was sem_t before it was changed to the non-standard mutex_t. See > > > > commit d1d46335df6: > > > > > > > > commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d > > > > Author: anjiahao > > > > Date: Tue Sep 6 14:18:45 2022 +0800 > > > > > > > > Replace nxsem API when used as a lock with nxmutex API > > > > > > > > Signed-off-by: anjiahao > > > > Signed-off-by: Xiang Xiao > > > > > > > > Parts of the above which introduce inappropriate use of mutex_t could > > be > > > > reverted back to sem_t. Any use of any non-standard OS function or > > > > structure should be prohibited in application code. > > > > > > > > However, nuttx/libs is a special creature. It is not confined by the > > > > rules of POSIX interface because user-space libraries are a > > > > non-privileged extension of the operating system. They have intimate > > > > knowledge of OS internals and can do non-standard things to > accomplish > > > > what they need to do. So although I think it is wrong > architecturally > > > > for applications to use OS internals, I don't think it is wrong for > > > > nuttx/libs to do so. It is a friend of the OS. > > > > > > > > But it is should not break the KERNEL build either. > > > > > > > > Perhaps we will need a user-space function to allocate (and > initialize) > > > > kernel memory. Use of such functions should not performed outside of > > > > nuttx/libs, although we have no way to prohibit that use: Such an > API > > > > would be a security hole. user-space code cannot access kernel > memory, > > > > but it can use the kernel memory address like a "handle" to interact > > > > with the OS. > > > > > > > > > > > > > > > > > >
Re: Usage of mutex_t inside libc
On 4/26/2023 12:18 PM, Ville Juven wrote: How to protect the integrity of the stack allocated structure is still a bit open but one option is to use kheap as well. Semantics to be figured out, the solution should be feasible. My idea was to put the handle to this data into the user semaphore, however a pointer must not be used, a handle / integer id is needed, which then holds the pointer (much like files etc). As the user can spoof / destroy the pointer it is unsafe to do that. Spoofing the id can cause the user process to crash, but the kernel integrity remains. I think the missing piece is: https://github.com/apache/nuttx/issues/1329 also https://github.com/apache/nuttx/issues/1359 All system calls really need to verify all data passed from applications. (The current problem statement in 1329 is insufficient) You can't really prove that an address is correct, but we can at least assure that all user addresses lie in user space and all kernel address lie in kernel space. I suppose if we could protect a secret we could hash kernel addresses. There are many open issues in this regard. I don't think you should have to worry about this too much now. Perhaps you could just open an issue and link it to 1329. Someday we will get more serious about security bugs.
Re: Usage of mutex_t inside libc
Hi, Yes exactly, this is the basic idea. Like I said, the waitlist/holder structure is needed only when sem_wait needs to block / wait for the semaphore to become available. How to protect the integrity of the stack allocated structure is still a bit open but one option is to use kheap as well. Semantics to be figured out, the solution should be feasible. My idea was to put the handle to this data into the user semaphore, however a pointer must not be used, a handle / integer id is needed, which then holds the pointer (much like files etc). As the user can spoof / destroy the pointer it is unsafe to do that. Spoofing the id can cause the user process to crash, but the kernel integrity remains. Br, Ville Juven On Wed, Apr 26, 2023 at 9:09 PM Xiang Xiao wrote: > But why not allocate list entry and holder temporally inside the kernel > thread stack? Both data is just used when the calling thread can't hold, > but wait the semphare, this trick is widely used inside Linux kernel. > > On Thu, Apr 27, 2023 at 1:32 AM Ville Juven wrote: > > > Hi, > > > > Thanks for the explanation, it makes sense. I had an idea to move struct > > sem_s entirely to kernel and make the user use it via a handle (in KERNEL > > build) but due to the ubiquitous init macros, that is not a feasible > > solution. > > > > I will explore allocating the kernel structures on-demand next. > > > > What does this mean ? The kernel side structures (dq_waitlist and > priority > > inheritance holders) are only needed when the semaphore is locked and > > someone is actually waiting for it to be released. This means that when > > sem_wait() results in the thread getting blocked, the kernel part can be > > allocated here and a reference placed into the user sem_t structure. When > > the semaphore unlocks, the kernel parts can be freed. > > > > This should be completely regression free for FLAT/PROTECTED, which is > very > > important for me because I don't want to be responsible for breaking > such a > > fundamental OS API for current users ;) > > > > Br, > > Ville > > > > On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt > wrote: > > > > > > > > > I have a question about using mutex_t / struct mutex_s inside > > > > libs/libc. The mutex_t definition is kernel internal but some modules > > > > inside the libs folder use it directly. My question is, is this > > > > intentional ? I don't think such modules should be using mutex_t. > > > > > > > > ... > > > > > > > > My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both > > > > of which are valid user APIs. > > > > > > It was sem_t before it was changed to the non-standard mutex_t. See > > > commit d1d46335df6: > > > > > > commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d > > > Author: anjiahao > > > Date: Tue Sep 6 14:18:45 2022 +0800 > > > > > > Replace nxsem API when used as a lock with nxmutex API > > > > > > Signed-off-by: anjiahao > > > Signed-off-by: Xiang Xiao > > > > > > Parts of the above which introduce inappropriate use of mutex_t could > be > > > reverted back to sem_t. Any use of any non-standard OS function or > > > structure should be prohibited in application code. > > > > > > However, nuttx/libs is a special creature. It is not confined by the > > > rules of POSIX interface because user-space libraries are a > > > non-privileged extension of the operating system. They have intimate > > > knowledge of OS internals and can do non-standard things to accomplish > > > what they need to do. So although I think it is wrong architecturally > > > for applications to use OS internals, I don't think it is wrong for > > > nuttx/libs to do so. It is a friend of the OS. > > > > > > But it is should not break the KERNEL build either. > > > > > > Perhaps we will need a user-space function to allocate (and initialize) > > > kernel memory. Use of such functions should not performed outside of > > > nuttx/libs, although we have no way to prohibit that use: Such an API > > > would be a security hole. user-space code cannot access kernel memory, > > > but it can use the kernel memory address like a "handle" to interact > > > with the OS. > > > > > > > > > > > >
Re: Usage of mutex_t inside libc
But why not allocate list entry and holder temporally inside the kernel thread stack? Both data is just used when the calling thread can't hold, but wait the semphare, this trick is widely used inside Linux kernel. On Thu, Apr 27, 2023 at 1:32 AM Ville Juven wrote: > Hi, > > Thanks for the explanation, it makes sense. I had an idea to move struct > sem_s entirely to kernel and make the user use it via a handle (in KERNEL > build) but due to the ubiquitous init macros, that is not a feasible > solution. > > I will explore allocating the kernel structures on-demand next. > > What does this mean ? The kernel side structures (dq_waitlist and priority > inheritance holders) are only needed when the semaphore is locked and > someone is actually waiting for it to be released. This means that when > sem_wait() results in the thread getting blocked, the kernel part can be > allocated here and a reference placed into the user sem_t structure. When > the semaphore unlocks, the kernel parts can be freed. > > This should be completely regression free for FLAT/PROTECTED, which is very > important for me because I don't want to be responsible for breaking such a > fundamental OS API for current users ;) > > Br, > Ville > > On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt wrote: > > > > > > I have a question about using mutex_t / struct mutex_s inside > > > libs/libc. The mutex_t definition is kernel internal but some modules > > > inside the libs folder use it directly. My question is, is this > > > intentional ? I don't think such modules should be using mutex_t. > > > > > > ... > > > > > > My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both > > > of which are valid user APIs. > > > > It was sem_t before it was changed to the non-standard mutex_t. See > > commit d1d46335df6: > > > > commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d > > Author: anjiahao > > Date: Tue Sep 6 14:18:45 2022 +0800 > > > > Replace nxsem API when used as a lock with nxmutex API > > > > Signed-off-by: anjiahao > > Signed-off-by: Xiang Xiao > > > > Parts of the above which introduce inappropriate use of mutex_t could be > > reverted back to sem_t. Any use of any non-standard OS function or > > structure should be prohibited in application code. > > > > However, nuttx/libs is a special creature. It is not confined by the > > rules of POSIX interface because user-space libraries are a > > non-privileged extension of the operating system. They have intimate > > knowledge of OS internals and can do non-standard things to accomplish > > what they need to do. So although I think it is wrong architecturally > > for applications to use OS internals, I don't think it is wrong for > > nuttx/libs to do so. It is a friend of the OS. > > > > But it is should not break the KERNEL build either. > > > > Perhaps we will need a user-space function to allocate (and initialize) > > kernel memory. Use of such functions should not performed outside of > > nuttx/libs, although we have no way to prohibit that use: Such an API > > would be a security hole. user-space code cannot access kernel memory, > > but it can use the kernel memory address like a "handle" to interact > > with the OS. > > > > > > >
RE: external rtc small fix
Hi, I think that rtc should be one! If you have two rtcs which one is correct? And from which one OS will sync time? >From my point of view if you select both internal rtc and external rtc there >should be an error (in this case as a side effect) or the config options for them must be mutually exclusive. How many rtcs does have the pc? Or the smartphone? If you select only internal or only external rtc there is no problem. Best regards -Original Message- From: Petro Karashchenko Sent: Wednesday, April 26, 2023 6:15 PM To: dev@nuttx.apache.org Subject: Re: external rtc small fix Hello Krasimir, One of the contributors brought https://lists.apache.org/thread/y7zrsbf2k86n1qbjlt67hlcj10smjdfw into the discussion. Could you please take a look and reply if you agree? Best regards, Petro On Wed, Apr 26, 2023, 10:26 AM Petro Karashchenko < petro.karashche...@gmail.com> wrote: > Please take a look if this https://github.com/apache/nuttx/pull/9104 > fixes the reported issue. > > Best regards, > Petro > > ср, 26 квіт. 2023 р. о 10:10 Krasimir Cheshmedzhiev < > cheshmedzh...@gmail.com> пише: > >> Hi, >> >> 12.1, but this exists in earlier versions - 7.27 for example >> >> >>> So you propose not to call up_rtc_initialize(); as well? >> >> No, only when using external rtc. >> >> Should be: >> >> #if defined(CONFIG_RTC) && !defined(CONFIG_RTC_EXTERNAL) >> /* Initialize the internal RTC hardware. Initialization of external RTC >>* must be deferred until the system has booted. >>*/ >> >> up_rtc_initialize(); >> . >> >> Best regards >> >> -Original Message- >> From: Petro Karashchenko >> Sent: Wednesday, April 26, 2023 10:02 AM >> To: dev@nuttx.apache.org >> Subject: Re: external rtc small fix >> >> Hi, >> >> Which version are you using? In the latest master I see: >> #if defined(CONFIG_RTC) >> /* Initialize the internal RTC hardware. Initialization of external RTC >>* must be deferred until the system has booted. >>*/ >> >> up_rtc_initialize(); >> >> #if !defined(CONFIG_RTC_EXTERNAL) >> /* Initialize the time value to match the RTC */ >> >> clock_inittime(NULL); >> #endif >> #endif >> >> So you propose not to call up_rtc_initialize(); as well? >> >> Best regards, >> Petro >> >> ср, 26 квіт. 2023 р. о 09:33 Krasimir Cheshmedzhiev < >> cheshmedzh...@gmail.com> >> пише: >> >> > Hi all, >> > >> > >> > >> > When using external rtc the function up_rtc_initialize() in file >> > nuttx/sched/clock_initialize.c should not be called. >> > >> > A small fix is attached. >> > >> > >> > >> > Best regards >> > >> >>
Re: Usage of mutex_t inside libc
Hi, Thanks for the explanation, it makes sense. I had an idea to move struct sem_s entirely to kernel and make the user use it via a handle (in KERNEL build) but due to the ubiquitous init macros, that is not a feasible solution. I will explore allocating the kernel structures on-demand next. What does this mean ? The kernel side structures (dq_waitlist and priority inheritance holders) are only needed when the semaphore is locked and someone is actually waiting for it to be released. This means that when sem_wait() results in the thread getting blocked, the kernel part can be allocated here and a reference placed into the user sem_t structure. When the semaphore unlocks, the kernel parts can be freed. This should be completely regression free for FLAT/PROTECTED, which is very important for me because I don't want to be responsible for breaking such a fundamental OS API for current users ;) Br, Ville On Wed, Apr 26, 2023 at 5:26 PM Gregory Nutt wrote: > > > I have a question about using mutex_t / struct mutex_s inside > > libs/libc. The mutex_t definition is kernel internal but some modules > > inside the libs folder use it directly. My question is, is this > > intentional ? I don't think such modules should be using mutex_t. > > > > ... > > > > My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both > > of which are valid user APIs. > > It was sem_t before it was changed to the non-standard mutex_t. See > commit d1d46335df6: > > commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d > Author: anjiahao > Date: Tue Sep 6 14:18:45 2022 +0800 > > Replace nxsem API when used as a lock with nxmutex API > > Signed-off-by: anjiahao > Signed-off-by: Xiang Xiao > > Parts of the above which introduce inappropriate use of mutex_t could be > reverted back to sem_t. Any use of any non-standard OS function or > structure should be prohibited in application code. > > However, nuttx/libs is a special creature. It is not confined by the > rules of POSIX interface because user-space libraries are a > non-privileged extension of the operating system. They have intimate > knowledge of OS internals and can do non-standard things to accomplish > what they need to do. So although I think it is wrong architecturally > for applications to use OS internals, I don't think it is wrong for > nuttx/libs to do so. It is a friend of the OS. > > But it is should not break the KERNEL build either. > > Perhaps we will need a user-space function to allocate (and initialize) > kernel memory. Use of such functions should not performed outside of > nuttx/libs, although we have no way to prohibit that use: Such an API > would be a security hole. user-space code cannot access kernel memory, > but it can use the kernel memory address like a "handle" to interact > with the OS. > > >
Re: Usage of SEM_INITIALIZER in apps, userspace in general
Hi, Thanks for the explanation. I did notice the reason why the macro exists. If the macro was removed an option I thought of was to implement a lazy init for the semaphore into e.g. sem_wait(), sem_post() etc. That path will not work, because like you said, the semaphores exist in many structures as members (pthread_mutex is one good example) both inside the kernel as well as in userspace. I'm not familiar enough with the OS semantics, this has been a multi-week long learning journey into the NuttX implementation of semaphores, how they work and where/how they are used. I will not be the one to remove this legacy, as the risk is too great. I'll figure out something else for my specific problem, with a smaller chance for regression to working targets. Br, Ville Juven On Wed, Apr 26, 2023 at 6:23 PM Gregory Nutt wrote: > On 4/26/2023 4:10 AM, Ville JUven wrote: > > I know it might be too late to fix the usage of SEM_INITIALIZER macro > > from user space, as many out-of-tree targets might depend on this as > > well, but I'd still like to open a discussion about the possibility of > > removing it from user space. > > > > Why remove it? SEM_INITIALIZER is a non-standard way of initializing > > the semaphore structure. This is only possible because the full > > composition of sem_t is visible to the user, including all of the > > kernel private data (waitlist, priority inheritance info). If the > > composition is hidden / moved (IMO the user should _not_ know the > > implementation defined composition of sem_t) , then the initializer > > list will become unfeasible. > > SEM_INITIALIZER was added many years ago when NuttX was a tiny OS that > was just /mostly POSIX/. Now the OS has grown and matured and stricter > POSIX-like behavior is more mission critical. So I would be supportive > of removing it because it is a tiny step toward POSIX compliance. > > However, you will probably have to deal with the reason why > SEM_INITIALIZER was created in the first place: There are many places > where semaphores are declared and sem_init() cannot be called to > initialize them. For example, when the sem_t is defined but there is no > initialization function that runs to call sem_init() or when the sem_t > is member of a more complex type like an array of structures with unions. > > I am not sure if there are any cases like this in user-space, but if so > those cases will require some re-design. >
Re: Usage of SEM_INITIALIZER in apps, userspace in general
On 4/26/2023 4:10 AM, Ville JUven wrote: I know it might be too late to fix the usage of SEM_INITIALIZER macro from user space, as many out-of-tree targets might depend on this as well, but I'd still like to open a discussion about the possibility of removing it from user space. Why remove it? SEM_INITIALIZER is a non-standard way of initializing the semaphore structure. This is only possible because the full composition of sem_t is visible to the user, including all of the kernel private data (waitlist, priority inheritance info). If the composition is hidden / moved (IMO the user should _not_ know the implementation defined composition of sem_t) , then the initializer list will become unfeasible. SEM_INITIALIZER was added many years ago when NuttX was a tiny OS that was just /mostly POSIX/. Now the OS has grown and matured and stricter POSIX-like behavior is more mission critical. So I would be supportive of removing it because it is a tiny step toward POSIX compliance. However, you will probably have to deal with the reason why SEM_INITIALIZER was created in the first place: There are many places where semaphores are declared and sem_init() cannot be called to initialize them. For example, when the sem_t is defined but there is no initialization function that runs to call sem_init() or when the sem_t is member of a more complex type like an array of structures with unions. I am not sure if there are any cases like this in user-space, but if so those cases will require some re-design.
Re: external rtc small fix
Hello Krasimir, One of the contributors brought https://lists.apache.org/thread/y7zrsbf2k86n1qbjlt67hlcj10smjdfw into the discussion. Could you please take a look and reply if you agree? Best regards, Petro On Wed, Apr 26, 2023, 10:26 AM Petro Karashchenko < petro.karashche...@gmail.com> wrote: > Please take a look if this https://github.com/apache/nuttx/pull/9104 > fixes the reported issue. > > Best regards, > Petro > > ср, 26 квіт. 2023 р. о 10:10 Krasimir Cheshmedzhiev < > cheshmedzh...@gmail.com> пише: > >> Hi, >> >> 12.1, but this exists in earlier versions - 7.27 for example >> >> >>> So you propose not to call up_rtc_initialize(); as well? >> >> No, only when using external rtc. >> >> Should be: >> >> #if defined(CONFIG_RTC) && !defined(CONFIG_RTC_EXTERNAL) >> /* Initialize the internal RTC hardware. Initialization of external RTC >>* must be deferred until the system has booted. >>*/ >> >> up_rtc_initialize(); >> . >> >> Best regards >> >> -Original Message- >> From: Petro Karashchenko >> Sent: Wednesday, April 26, 2023 10:02 AM >> To: dev@nuttx.apache.org >> Subject: Re: external rtc small fix >> >> Hi, >> >> Which version are you using? In the latest master I see: >> #if defined(CONFIG_RTC) >> /* Initialize the internal RTC hardware. Initialization of external RTC >>* must be deferred until the system has booted. >>*/ >> >> up_rtc_initialize(); >> >> #if !defined(CONFIG_RTC_EXTERNAL) >> /* Initialize the time value to match the RTC */ >> >> clock_inittime(NULL); >> #endif >> #endif >> >> So you propose not to call up_rtc_initialize(); as well? >> >> Best regards, >> Petro >> >> ср, 26 квіт. 2023 р. о 09:33 Krasimir Cheshmedzhiev < >> cheshmedzh...@gmail.com> >> пише: >> >> > Hi all, >> > >> > >> > >> > When using external rtc the function up_rtc_initialize() in file >> > nuttx/sched/clock_initialize.c should not be called. >> > >> > A small fix is attached. >> > >> > >> > >> > Best regards >> > >> >>
Re: Usage of mutex_t inside libc
I have a question about using mutex_t / struct mutex_s inside libs/libc. The mutex_t definition is kernel internal but some modules inside the libs folder use it directly. My question is, is this intentional ? I don't think such modules should be using mutex_t. ... My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both of which are valid user APIs. It was sem_t before it was changed to the non-standard mutex_t. See commit d1d46335df6: commit d1d46335df6cc1bc63f1cd7e7bffe3735b8c275d Author: anjiahao Date: Tue Sep 6 14:18:45 2022 +0800 Replace nxsem API when used as a lock with nxmutex API Signed-off-by: anjiahao Signed-off-by: Xiang Xiao Parts of the above which introduce inappropriate use of mutex_t could be reverted back to sem_t. Any use of any non-standard OS function or structure should be prohibited in application code. However, nuttx/libs is a special creature. It is not confined by the rules of POSIX interface because user-space libraries are a non-privileged extension of the operating system. They have intimate knowledge of OS internals and can do non-standard things to accomplish what they need to do. So although I think it is wrong architecturally for applications to use OS internals, I don't think it is wrong for nuttx/libs to do so. It is a friend of the OS. But it is should not break the KERNEL build either. Perhaps we will need a user-space function to allocate (and initialize) kernel memory. Use of such functions should not performed outside of nuttx/libs, although we have no way to prohibit that use: Such an API would be a security hole. user-space code cannot access kernel memory, but it can use the kernel memory address like a "handle" to interact with the OS.
Re: asprintf failure
Hello, I fixed all critical usages of asprintf and strdup. These fixes are in the following PRs: https://github.com/apache/nuttx/pull/9009 https://github.com/apache/nuttx/pull/9010 https://github.com/apache/nuttx-apps/pull/1713 The OS itself is now fixed, there should be no other issues pending. However, there are some uses that are still to be fixed. These are in the tools directory and in some apps (strdup mostly). They should not affect the stability of the system in any way, and I consider these "minor" issues. Unfortunately, I cannot spend any more time on this. So, if anyone has any free time, I would encourage you to improve the apps or the tools in this regard. On Wed, Mar 29, 2023 at 8:07 PM Nathan Hartman wrote: > On Wed, Mar 29, 2023 at 5:02 AM Fotis Panagiotopoulos > > wrote: > > > > In my opinion asprintf should set the pointer to NULL, to be safe. > > > But the calling code should probably be changed as well, because it is > > > not a good coding example for portability. > > > > I'm sceptical about this. > > Setting the pointer to NULL seems more safe, but also it is a change in > > functionality! > > > > Consider the following example: > > > > char * msg = "Error message"; > > asprintf(, "format string", args...); > > > > Based on the current functionality, I can directly use msg without any > > error checking, as it will always be valid. > > (Either due to its initialization, or due to a successful asprintf). > > > > Indeed, this seems like a not-so-great piece of code, but I don't know > > whether this approach is used anywhere in NuttX > > (or in user code). > > > > > The above usage may work on some implementations but not others. As Bernd > Walter pointed out from some *BSD manpages, some implementations will set > your msg pointer to NULL, losing your fallback string. So code that runs > perfectly well on one OS may break when run on another. > > If you have many usages like that, you could encapsulate the > asprintf-or-default-string functionality in a function you can write for > that purpose. You'd utilize vararg and vasprintf for it. > > Cheers > Nathan >
Re: Usage of mutex_t inside libc
On Wed, Apr 26, 2023 at 8:34 AM Ville Juven wrote: > Hi, > > Thanks for pointing that out, basically the same issue I face. > > First I thought it would be simple to fix OR wrap around the usage of > SEM_INITIALIZER/NXSEM_INITIALIZER/NXMUTEX_INITIALIZER. My assumption was > that they are not used in very many places in user space. For wrapping > one option would be to do a lazy init of the semaphore when sem_wait() / > sem_post() / etc. are called and identify the non-init state by some > special flagging. > > However I'm totally wrong, PTHREAD_MUTEX_INITIALIZER exists as well and > this is used to initialize the semaphore in pthread_mutex_t and > pthread_cond_t. Trying to get rid of the initializer macro proves to be > too much re-factoring of code I'm not very familiar with (big risk > something will break) -> I'll try to figure out something else. PTHREAD_MUTEX_INITIALIZER not only exists, it's in the standard: https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_init.html Nathan
Re: Usage of mutex_t inside libc
Hi, Thanks for pointing that out, basically the same issue I face. First I thought it would be simple to fix OR wrap around the usage of SEM_INITIALIZER/NXSEM_INITIALIZER/NXMUTEX_INITIALIZER. My assumption was that they are not used in very many places in user space. For wrapping one option would be to do a lazy init of the semaphore when sem_wait() / sem_post() / etc. are called and identify the non-init state by some special flagging. However I'm totally wrong, PTHREAD_MUTEX_INITIALIZER exists as well and this is used to initialize the semaphore in pthread_mutex_t and pthread_cond_t. Trying to get rid of the initializer macro proves to be too much re-factoring of code I'm not very familiar with (big risk something will break) -> I'll try to figure out something else. Br, Ville Juven On 26.4.2023 12.53, Petro Karashchenko wrote: Hi, Some time ago I tried to fix this with https://github.com/apache/nuttx/pull/6376 but it ended-up nowhere as I didn't find time for a proper solution. Best regards, Petro ср, 26 квіт. 2023 р. о 12:50 Ville JUven пише: Hi, I'm in the process of trying to fix this issue: https://github.com/apache/nuttx/issues/8917 TL;DR semaphores are unusable when MMU is in use, because the kernel private data of struct sem_s exists in user space and is not always visible to the kernel when it needs it. So I'm trying to separate / move the kernel private data to kernel space (for CONFIG_BUILD_KERNEL only btw.). I have a question about using mutex_t / struct mutex_s inside libs/libc. The mutex_t definition is kernel internal but some modules inside the libs folder use it directly. My question is, is this intentional ? I don't think such modules should be using mutex_t. Why not ? libc is userspace code (even though we use it inside the kernel as well as I guess nothing prohibits this) What is the problem ? Using NXMUTEX_INITIALIZER uses the kernel private NXSEM_INITIALIZER macro. I'm trying to separate kernel private data from semaphores like stated above but there are some corner cases where this becomes impossible, mutex_t is one example. The locations where mutex_t is used (there might be others, using nxrmutex for example): ./libs/libnx/nxmu/nx_connect.c:55:static mutex_t g_nxliblock = NXMUTEX_INITIALIZER; ./libs/libnx/nxfonts/nxfonts_cache.c:79:static mutex_t g_cachelock = NXMUTEX_INITIALIZER; ./libs/libc/locale/lib_gettext.c:87:static mutex_t g_lock = NXMUTEX_INITIALIZER; ./libs/libc/wqueue/work_usrthread.c:65: NXMUTEX_INITIALIZER, ./libs/libc/userfs/lib_userfs.c:73:static mutex_t g_userfs_lock = NXMUTEX_INITIALIZER; ./libs/libc/stdlib/lib_mktemp.c:68:static mutex_t g_b62lock = NXMUTEX_INITIALIZER; My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both of which are valid user APIs. Anyone have any idea / suggestions on how to fix this ? Br, Ville Juven / pussuw
Usage of SEM_INITIALIZER in apps, userspace in general
Hi, I know it might be too late to fix the usage of SEM_INITIALIZER macro from user space, as many out-of-tree targets might depend on this as well, but I'd still like to open a discussion about the possibility of removing it from user space. Why remove it? SEM_INITIALIZER is a non-standard way of initializing the semaphore structure. This is only possible because the full composition of sem_t is visible to the user, including all of the kernel private data (waitlist, priority inheritance info). If the composition is hidden / moved (IMO the user should _not_ know the implementation defined composition of sem_t) , then the initializer list will become unfeasible. Inside the /apps folder I see only a handful of references which should be fixable by using the standard sem_init() function instead. I do not have a strong opinion about this, I'm fine with keeping it and I can wrap around the issue keeping any existing code functional/intact. I just think it is a bit odd and confusing to have such non-standard quirks in the otherwise well defined user API. BR, Ville Juven / pussuw To clarify, I am _not_ proposing we remove NXSEM_INITIALIZER, which is ubiquitous inside the kernel, and is completely fine to use there.
Re: Usage of mutex_t inside libc
Hi, Some time ago I tried to fix this with https://github.com/apache/nuttx/pull/6376 but it ended-up nowhere as I didn't find time for a proper solution. Best regards, Petro ср, 26 квіт. 2023 р. о 12:50 Ville JUven пише: > Hi, > > I'm in the process of trying to fix this issue: > https://github.com/apache/nuttx/issues/8917 > > TL;DR semaphores are unusable when MMU is in use, because the kernel > private data of struct sem_s exists in user space and is not always > visible to the kernel when it needs it. So I'm trying to separate / move > the kernel private data to kernel space (for CONFIG_BUILD_KERNEL only > btw.). > > > > I have a question about using mutex_t / struct mutex_s inside libs/libc. > The mutex_t definition is kernel internal but some modules inside the > libs folder use it directly. My question is, is this intentional ? I > don't think such modules should be using mutex_t. > > Why not ? libc is userspace code (even though we use it inside the > kernel as well as I guess nothing prohibits this) > > What is the problem ? Using NXMUTEX_INITIALIZER uses the kernel private > NXSEM_INITIALIZER macro. I'm trying to separate kernel private data from > semaphores like stated above but there are some corner cases where this > becomes impossible, mutex_t is one example. > > The locations where mutex_t is used (there might be others, using > nxrmutex for example): > ./libs/libnx/nxmu/nx_connect.c:55:static mutex_t g_nxliblock = > NXMUTEX_INITIALIZER; > ./libs/libnx/nxfonts/nxfonts_cache.c:79:static mutex_t g_cachelock = > NXMUTEX_INITIALIZER; > ./libs/libc/locale/lib_gettext.c:87:static mutex_t g_lock = > NXMUTEX_INITIALIZER; > ./libs/libc/wqueue/work_usrthread.c:65: NXMUTEX_INITIALIZER, > ./libs/libc/userfs/lib_userfs.c:73:static mutex_t g_userfs_lock = > NXMUTEX_INITIALIZER; > ./libs/libc/stdlib/lib_mktemp.c:68:static mutex_t g_b62lock = > NXMUTEX_INITIALIZER; > > My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both of > which are valid user APIs. > > Anyone have any idea / suggestions on how to fix this ? > > Br, > > Ville Juven / pussuw > >
Usage of mutex_t inside libc
Hi, I'm in the process of trying to fix this issue: https://github.com/apache/nuttx/issues/8917 TL;DR semaphores are unusable when MMU is in use, because the kernel private data of struct sem_s exists in user space and is not always visible to the kernel when it needs it. So I'm trying to separate / move the kernel private data to kernel space (for CONFIG_BUILD_KERNEL only btw.). I have a question about using mutex_t / struct mutex_s inside libs/libc. The mutex_t definition is kernel internal but some modules inside the libs folder use it directly. My question is, is this intentional ? I don't think such modules should be using mutex_t. Why not ? libc is userspace code (even though we use it inside the kernel as well as I guess nothing prohibits this) What is the problem ? Using NXMUTEX_INITIALIZER uses the kernel private NXSEM_INITIALIZER macro. I'm trying to separate kernel private data from semaphores like stated above but there are some corner cases where this becomes impossible, mutex_t is one example. The locations where mutex_t is used (there might be others, using nxrmutex for example): ./libs/libnx/nxmu/nx_connect.c:55:static mutex_t g_nxliblock = NXMUTEX_INITIALIZER; ./libs/libnx/nxfonts/nxfonts_cache.c:79:static mutex_t g_cachelock = NXMUTEX_INITIALIZER; ./libs/libc/locale/lib_gettext.c:87:static mutex_t g_lock = NXMUTEX_INITIALIZER; ./libs/libc/wqueue/work_usrthread.c:65: NXMUTEX_INITIALIZER, ./libs/libc/userfs/lib_userfs.c:73:static mutex_t g_userfs_lock = NXMUTEX_INITIALIZER; ./libs/libc/stdlib/lib_mktemp.c:68:static mutex_t g_b62lock = NXMUTEX_INITIALIZER; My question ? Should these be sem_t or perhaps pthread_mutex_t ? Both of which are valid user APIs. Anyone have any idea / suggestions on how to fix this ? Br, Ville Juven / pussuw
Re: external rtc small fix
Please take a look if this https://github.com/apache/nuttx/pull/9104 fixes the reported issue. Best regards, Petro ср, 26 квіт. 2023 р. о 10:10 Krasimir Cheshmedzhiev пише: > Hi, > > 12.1, but this exists in earlier versions - 7.27 for example > > >>> So you propose not to call up_rtc_initialize(); as well? > > No, only when using external rtc. > > Should be: > > #if defined(CONFIG_RTC) && !defined(CONFIG_RTC_EXTERNAL) > /* Initialize the internal RTC hardware. Initialization of external RTC >* must be deferred until the system has booted. >*/ > > up_rtc_initialize(); > . > > Best regards > > -Original Message- > From: Petro Karashchenko > Sent: Wednesday, April 26, 2023 10:02 AM > To: dev@nuttx.apache.org > Subject: Re: external rtc small fix > > Hi, > > Which version are you using? In the latest master I see: > #if defined(CONFIG_RTC) > /* Initialize the internal RTC hardware. Initialization of external RTC >* must be deferred until the system has booted. >*/ > > up_rtc_initialize(); > > #if !defined(CONFIG_RTC_EXTERNAL) > /* Initialize the time value to match the RTC */ > > clock_inittime(NULL); > #endif > #endif > > So you propose not to call up_rtc_initialize(); as well? > > Best regards, > Petro > > ср, 26 квіт. 2023 р. о 09:33 Krasimir Cheshmedzhiev < > cheshmedzh...@gmail.com> > пише: > > > Hi all, > > > > > > > > When using external rtc the function up_rtc_initialize() in file > > nuttx/sched/clock_initialize.c should not be called. > > > > A small fix is attached. > > > > > > > > Best regards > > > >
RE: external rtc small fix
Hi, 12.1, but this exists in earlier versions - 7.27 for example >>> So you propose not to call up_rtc_initialize(); as well? No, only when using external rtc. Should be: #if defined(CONFIG_RTC) && !defined(CONFIG_RTC_EXTERNAL) /* Initialize the internal RTC hardware. Initialization of external RTC * must be deferred until the system has booted. */ up_rtc_initialize(); . Best regards -Original Message- From: Petro Karashchenko Sent: Wednesday, April 26, 2023 10:02 AM To: dev@nuttx.apache.org Subject: Re: external rtc small fix Hi, Which version are you using? In the latest master I see: #if defined(CONFIG_RTC) /* Initialize the internal RTC hardware. Initialization of external RTC * must be deferred until the system has booted. */ up_rtc_initialize(); #if !defined(CONFIG_RTC_EXTERNAL) /* Initialize the time value to match the RTC */ clock_inittime(NULL); #endif #endif So you propose not to call up_rtc_initialize(); as well? Best regards, Petro ср, 26 квіт. 2023 р. о 09:33 Krasimir Cheshmedzhiev пише: > Hi all, > > > > When using external rtc the function up_rtc_initialize() in file > nuttx/sched/clock_initialize.c should not be called. > > A small fix is attached. > > > > Best regards >
Re: external rtc small fix
Hi, Which version are you using? In the latest master I see: #if defined(CONFIG_RTC) /* Initialize the internal RTC hardware. Initialization of external RTC * must be deferred until the system has booted. */ up_rtc_initialize(); #if !defined(CONFIG_RTC_EXTERNAL) /* Initialize the time value to match the RTC */ clock_inittime(NULL); #endif #endif So you propose not to call up_rtc_initialize(); as well? Best regards, Petro ср, 26 квіт. 2023 р. о 09:33 Krasimir Cheshmedzhiev пише: > Hi all, > > > > When using external rtc the function up_rtc_initialize() in file > nuttx/sched/clock_initialize.c should not be called. > > A small fix is attached. > > > > Best regards >
external rtc small fix
Hi all, When using external rtc the function up_rtc_initialize() in file nuttx/sched/clock_initialize.c should not be called. A small fix is attached. Best regards --- nx12.1-orig/nuttx/sched/clock/clock_initialize.c 2022-12-13 09:30:57.0 +0200 +++ nx12.1-new/nuttx/sched/clock/clock_initialize.c 2023-04-21 22:00:12.918932964 +0300 @@ -210,7 +210,8 @@ up_timer_initialize(); #endif -#if defined(CONFIG_RTC) +#if defined(CONFIG_RTC) && !defined(CONFIG_RTC_EXTERNAL) + /* Initialize the internal RTC hardware. Initialization of external RTC * must be deferred until the system has booted. */
