mmap shared stack design was: [PATCH v4 1/3] Strict thread-stack isolation

[Gedare Bloom]

On Sat, Aug 22, 2020 at 11:27 PM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
>
> On Sat, Aug 22, 2020 at 11:32 PM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> I have some comments below. I'm not that happy with the lack of design
>> discussion during the iteration of this code. While it is a little
>> easier to critique the code, it is also a bit wasteful because I have
>> to also comment on stylistic problems, and when some decisions you
>> make while coding end up not being correct or aren't easily understood
>> during code review then you might have to spend more time recoding. It
>> can be hard to find the balance between design and code reviews,
>>
>> On Sat, Aug 22, 2020 at 9:19 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>> > +
>> > +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
>> > +
>> > +/**
>> > + * The following defines the control block  of a shared stack. Each stack can have
>> > + * different sharing attributes.
>> > + */
>> > +typedef struct
>> > +{
>> > +  /** Access flags of the shared stack */
>> > +  uint32_t access_flags;
>> > +  /** This is the stack address of the sharing thread*/
>> > +  void* shared_stack_area;
>> > +  /** Stack size of the sharing thread*/
>> > +  size_t shared_stack_size;
>> > +  /** This is the stack address of the target stack. Maybe this area is not
>> > +   * needed but this helps in selecting the target thread during stack sharing.
>> > +   */
>> > +  void* target_stack_area;
>> I'm confused about the difference between shared_stack_area and
>> target_stack_area
>>
>> > + /** Error checking for valid target stack address. This is also used to
>> > +  * distinguish between a normal mmap operation and a stack sharing operation.
>> > +  */
>> I don't really know what this means.
>
>
> This is related to the mmap() call. When we share stack using mmap,  two of the important parameters that reference to the target thread-stack
> and sharing thread-stack are -'void* addr' which is the address of the target thread stack and 'int fd' that is the file descriptor to the shared memory
> object corresponding to the thread stack being shared. Now, stack sharing is different from a normal mmap operation in the sense that we need to add
> the sharing stack attributes to the stack control structure of the thread that is specified by 'addr' as compared to adding it to the chain of mappings. To select the
> target thread we need to iterate over all the threads, in the case we do not find any thread, we can proceed with our normal mmap operation otherwise we add the
> sharing thread stack attribute to the stack control structure of the target thread. We have the _Thread_Iterate() function but it only has two parameters the pointer to the
> function that performs operation during each iteration and a parameter related to this operation. My decision to specify the 'target_stack_area',  the 'shared_stack_area' and 'stack_shared' in the structure
> was so that I could squeeze in as much information to select the thread,  set the sharing stack parameters in the thread,  and validate(through stack_shared attribute) in the mmap() call that this is
> in fact a stack-sharing operation, in a single iteration.
>

Maybe I should ask this on the mmap patch itself, but I'm more
interested in the design than the code right now. I think I'll just
ask a few questions:

The 'sharing' stack is the stack that is getting r/w permission added
to another thread, and the 'target' thread is the thread that is
getting the r/w permission to access the shared stack. We should not
really care about the 'sharing thread' or the 'target stack'?

If mmap() assumes the calling context then the executing thread is the
"target thread" and you shouldn't have to search for it? In typical
POSIX implementations, mmap() assumes the executing process context
called it, but can we make the same assumption?

For mmap() the 'addr' should be the 'sharing' stack's (base) address
not the 'target' stack, because you want to be accessing the 'sharing'
stack memory? We don't have any virtual memory mapping, so the address
of the sharing stack needs to be used. Or maybe it can be NULL, or
even ignored, since the mmap() call should return the base address of
the 'sharing' stack? The fd should be able to find all the information
needed about the 'sharing' stack.

-Gedare