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

[Gedare Bloom]

On Mon, Aug 24, 2020 at 10:41 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
>
> On Mon, Aug 24, 2020 at 9:34 PM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> 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 the mmap() call for sharing thread stack assumes that it is being called from the context of the 'target thread', then no, we should not.
>
>>
>>
>> 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.
>
>
> My confusion was, as you asked above, whether we can assume that the mmap() was called by the executing process
> context. I was making the mmap() call from the 'POSIX_Init' thread and passing the address of the target thread and then
> selecting the thread inside mmap(). Can we do something like this in the application to ensure that the mmap() is called
> from the desired thread?
>

I think it is a reasonable assumption, but it may not be entirely
within the standard. We may need to get a "ruling" on it from Joel,
but I think you should proceed in this direction.

Also, I'm wondering if the stack mmap should also go in the mmap_mappings list?


>>
>> -Gedare