Modules, TXM_MODULE_MEMORY_PROTECTION and TX_ENABLE_STACK_CHECKING bug?

[mwolosewicz]

Hi guys, I'm playing with the modules and found some interesting things:

Abstract:

It looks that if:

TXM_MODULE_MEMORY_PROTECTION and TX_ENABLE_STACK_CHECKING are enabled then in the tx_thread_system_suspend.c the kernel stack (which is not prepared for stack checking) is checked with the macro TX_THREAD_STACK_CHECK and cause the crash.

In TX_THREAD_STACK_CHECK macro, for modules, thread_ptr->tx_thread_module_stack_end should be used instead of thread_ptr->tx_thread_stack_end.

Investigation:

Initial: TX_ENABLE_STACK_CHECKING 1 TXM_MODULE_MEMORY_PROTECTION 1

The creation of the "Module Start Thread":

In file txm_module_manager_create.c:

At the entry:

CHAR *name_ptr is "Module Start Thread" VOID *stack_start is 0x400 8800 ULONG stack_size is 512.

1. Some basic stuff is checked, and then we get to the part of creating the stack.

2. The memory stack is filled by TX_MEMSET(stack_start, ((UCHAR) TX_STACK_FILL), stack_size);

3. Because TX_ENABLE_STACK_CHECKING is enabled, then we prepare the module stack:

stack_start is aligned, in this case it stays as it was (0x0400 8800). stack_size is reduced by 4 bytes and the last 4 bytes of the stack are 0xEFEFEFEF.

4. The stack size is reduced again:

    /* Allocate the thread entry information at the top of thread's stack - Leaving one
       ULONG worth of 0xEF pattern between the actual stack and the entry info structure.  */

    stack_size =  stack_size - (sizeof(TXM_MODULE_THREAD_ENTRY_INFO) + (3*sizeof(ULONG)));

After that, the stack_size is 448 bytes.

5. Then we enter the TXM_MODULE_MEMORY_PROTECTION section (txm_module_manager_thread_create.c:312):

6. New memory is allocated using: status = _txm_module_manager_object_allocate((VOID **) &(thread_ptr -> tx_thread_module_kernel_stack_start), TXM_MODULE_KERNEL_STACK_SIZE, module_instance);

• The thread_ptr->tx_thread_module_kernel_stack_start is set to: 0x400 6788

• The TXM_MODULE_KERNEL_STACK_SIZE is 768.

7. The newly allocated memory is filled with pattern: TX_MEMSET(thread_ptr -> tx_thread_module_kernel_stack_start, ((UCHAR) TX_STACK_FILL), TXM_MODULE_KERNEL_STACK_SIZE);

• After this, in memory there is 768 bytes of 0xEF value.

8. The thread_ptr->tx_thread_module_kernel_stack_end is aligned. (But next, the stack size is set to TXM_MODULE_KERNEL_STACK_SIZE instead of (tx_thread_module_kernel_stack_end - tx_thread_module_kernel_stack_start [another potential BUG?]).

9. Then the thread_ptr->tx_thread_module_kernel_stack_end takes value: 0x0400 6A88

THE KERNEL STACK IS NOT PREPARED FOR TX_THREAD_STACK_CHECK.

10. No more surprises till the end of the function.

At the end of the functions there are the following stacks values in the thread_ptr:

tx_thread_name is "Module Start Thread"

tx_thread_stack_start is 0x0400 8800 tx_thread_stack_end is 0x0400 89BF tx_thread_stack_size is 448

tx_thread_module_stack_start is 0x0400 8800 tx_thread_module_stack_end is 0x0400 89BF tx_thread_module_stack_size is 448

tx_thread_module_kernel_stack_start is 0x0400 6788 tx_thread_module_kernel_stack_end is 0x0400 6A88 tx_thread_module_kernel_stack_size is 768

Switching of the stacks

Then in the tx_thread_schedule.S the following code is executed:

    /* Switch to the module thread's kernel stack */
    LDR     r0, [r2, #0xA8]                         // Load the module kernel stack end
#ifndef TXM_MODULE_KERNEL_STACK_MAINTENANCE_DISABLE
    LDR     r1, [r2, #0xA4]                         // Load the module kernel stack start
    LDR     r3, [r2, #0xAC]                         // Load the module kernel stack size
    STR     r1, [r2, #12]                           // Set stack start
    STR     r0, [r2, #16]                           // Set stack end
    STR     r3, [r2, #20]                           // Set stack size
#endif

And the thread stack (tx_thread_stack_start) is switched to module thread's kernel stack (tx_thread_module_kernel_stack_start).

Checking the stack before suspend

And some ticks later...

In the tx_thread_system_suspend.c, the "Module Start Thread" is going to be suspended but:

thread_ptr->tx_thread_stack_start is 0x0400 6788 (position of the kernel stack) thread_ptr->tx_thread_stack_end is 0x0400 6A88 (end position of the kernel stack) thread_ptr->tx_thread_stack_size is 768.

Then TX_THREAD_STACK_CHECK(thread_ptr) is performed and *((ULONG *) (((UCHAR *) (thread_ptr)->tx_thread_stack_end) + 1)) != TX_STACK_FILL) is not true because the kernel stack has no 0xef at the end, because it was not prepared for TX_THREAD_STACK_CHECK.

Probably the thread_ptr->tx_thread_module_stack_end should be used instead of thread_ptr->tx_thread_stack_end.

Some other interesting points:

1. The memory in the TXM_MODULE_MEMORY_PROTECTION shouldnt be filled using the calculated: thread_ptr->tx_thread_module_kernel_stack_end - thread_ptr->tx_thread_module_kernel_stack_start AFTER alligning of the end of stack instead of the const TXM_MODULE_KERNEL_STACK_SIZE?

2. Stack size shouldn't be calculated using tx_thread_module_kernel_stack_end - tx_thread_module_kernel_stack_start after alignment instead of TXM_MODULE_KERNEL_STACK_SIZE?

[ascheurer]

Did your suggestions above resolve the stack checking issue using ThreadX with Modules? It sounds like stack checking for modules needs to be different than normal stack checking for ThreadX and so introduce a specific stack checking flag for ThreadX with modules. Is that right? Does it work now? Add a flag to make the adjustments for modules?

[goldscott]

Modules has never supported the stack checking feature, so running into these issues that you are seeing is expected.

Let me address some points you made:

4. The module thread stack size is reduced by the ENTRY_INFO struct. This is mentioned in the second note here: https://docs.microsoft.com/en-us/azure/rtos/threadx-modules/chapter2. We should add this note to the txm_thread_create.c file.

5. tx_thread_module_kernel_stack_size isn't used anywhere other than in txm_module_manager_thread_reset.c and some RTOS-aware debuggers to show the stack size. This value has no effect on runtime behavior. But yes, we should do the calculation you are suggesting.

6. Because the stack checking feature was never supported by modules, we did not prepare the kernel stack.

For both of your questions: yes, we should do the calculation instead of using TXM_MODULE_KERNEL_STACK_SIZE.

[mwolosewicz]

Thanks @goldscott for the fast answer! In this case, I have a few more questions:

1. If the modules has never supported the stack checking, then why in the txm_module_manager_thread_create.c:279 (where module threads are created) there IS a section which enable the stack checking and prepare the module for this?

2. In the tx_thread_system_suspend.c if TX_ENABLE_STACK_CHECKING is defined, there is no distinction between module thread or ThreadX thread, so @ascheurer has right in his comment.

3. Is there any dedicated way to check if the current thread is a module or not? For now - it can be done by checking the module specific part of the thread_ptr, ex. if thread_ptr->tx_thread_module_instance_ptr is not NULL.

4. Because in the tx_thread_system_suspend.c if the TX_ENABLE_STACK_CHECKING is enabled there is no distinction if the current thread is module or not, then the conclusion is that if we have any module in our system, then stack checking should be disabled globally?

5. I've made a simple workaround which works (at least in tx_thread_system_suspend.c:114).

#ifdef TX_ENABLE_STACK_CHECKING
    if (thread_ptr)
    {
        if (thread_ptr->tx_thread_module_instance_ptr == NULL)
        {
            TX_THREAD_STACK_CHECK(thread_ptr)
        }
        else
        {
            TX_INTERRUPT_SAVE_AREA
                TX_DISABLE
                if (((thread_ptr)) && ((thread_ptr)->tx_thread_id == TX_THREAD_ID))
                {
                    if ((*((ULONG *) (thread_ptr)->tx_thread_module_stack_start) != TX_STACK_FILL) ||
                        (*((ULONG *) (((UCHAR *) (thread_ptr)->tx_thread_module_stack_end) + 1)) != TX_STACK_FILL))
                    {
                        TX_RESTORE
                            _tx_thread_stack_error_handler((thread_ptr));
                        TX_DISABLE
                    }
                }
                TX_RESTORE
        }
    }
#endif

instead of

#ifdef TX_ENABLE_STACK_CHECKING
TX_THREAD_STACK_CHECK(thread_ptr)
#endif

Iv'e tried to make more complete solution, but there is no such thing as thread_ptr->tx_thread_module_stack_highest, so in case of modules now we can only check if there is no stack override.

[goldscott]

Hi @mwolosewicz,

No worries!

1. txm_module_manager_thread_create was derived from tx_thread_create, so that stack checking was there to begin with.

2. Correct. tx_thread_suspend (more specifically the TX_THREAD_STACK_CHECK macro) has no knowledge whether a thread is normal or from a module.

3. That is how to check if a thread is owned by a module. There is no "dedicated way" such as an API.

4. Correct, you should not use stack checking if you are using modules. The stack checking built into ThreadX does not currently know how to handle a module thread (which actually has two stacks).

5. Your workaround is a good start. We'd have to #define a modified TX_THREAD_STACK_CHECK (probably in tx_port.h) and add that thread struct member (tx_thread_module_stack_highest_ptr).

Stack checking support for Modules has been on my to-do list for a while. If this is a feature you'd like to use, I'll bump up its priority.

[mwolosewicz]

Hi @goldscott,

thanks for great explanation. I'm not sure if the stack checking is crucial for me right now. I won't affect your priorities, but in general I believe that in a safety critical system it may be important.

I've asked the questions because I wasn't sure If there is a bug, or my understanding of the code base is too shallow.

Now everything is clear for me.

Great, thanks!

[ascheurer]

Hi @goldscott, The stack checking feature is useful and did flag an overrun in the non-module use of threadx - it would be useful and consistent to support stack checking with threadx with modules and as you mentioned as a next step. Suggest include a clearer set of calculations on how the stack is managed (see above "yes, we should do the calculation instead of using TXM_MODULE_KERNEL_STACK_SIZE). For now if modules do not support stack checking,making this more clear now would be helpful as it does look like stack checking with modules is supported - a comment or macro would be helpful to make this clear. Stack checking within modules would be useful - it's a feature that would certainly get used.

[goldscott]

Hi @ascheurer - I'll bump up the priority of adding stack checking to modules. It should be pretty easy. A couple "limitations" I see is (1) only the kernel/manager is going to do the stack check and error handling (i.e. no error handling on the module side, but that is probably a good thing) and (2) modules can manipulate their stacks at will, thus stack checking may not be accurate.

I'll keep this ticket open and close it once stack checking is supported in modules. We have a new release coming at the end of April, so I'm not sure we'll be able to get this done in time. It will likely be in the July release.