[PATCH 4/4] score: Ensure stack alignment requirement

[Gedare Bloom]

On Wed, Mar 3, 2021 at 11:20 AM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Wed, Mar 3, 2021 at 10:56 AM Sebastian Huber
> <sebastian.huber at embedded-brains.de> wrote:
> >
> >
> > On 03/03/2021 17:04, Gedare Bloom wrote:
> > >> +#if CPU_STACK_ALIGNMENT > CPU_HEAP_ALIGNMENT
> > >> +  stack_align = CPU_STACK_ALIGNMENT;
> > >> +  stack_begin = (char *) RTEMS_ALIGN_UP( (uintptr_t) stack_begin, stack_align );
> > >> +  stack_end = (char *) RTEMS_ALIGN_DOWN( (uintptr_t) stack_end, stack_align );
> > > Why align down here? Why align the end at all?
> > Most stacks grow down, so the alignment of the end address is important.
>
> OK, I think we had this discussion recently also on related patches.
> It might be good to include a comment here that this code aims to
> accommodate stacks growing either direction.
>
> > > Is there a requirement
> > > for the stack area to be a multiple of the stack alignment? I guess we
> > > can lose some bytes and return less than config->stack_size as a
> > > result of this?
> >
> > The CPU_STACK_ALIGNMENT should be set to the stack alignment required by
> > the ABI (in SMP configurations, it should be probably at least cache
> > aligned, but this is another topic). In this case all stack frames will
> > be on such a boundary and you can't use these extra bytes. Independent
> > of this, you should probably not calculate your stack size so that you
> > actually use the last byte of your stack.
> >
> OK, but it's not necessary to waste those bytes, when we can align the
> stack_begin area of the stack before we calculate the stack_end, if a
> stack size is requested that is a multiple of the CPU_STACK_ALIGNMENT
> (which is likely).  It's fine either way, pretty minor space
> optimization/waste problem.
>
> I could see someone doing something like putting an application CANARY
> value at the end (bottom/begin) of their stack.
>
> mystack[end - size] = CANARY;
> that will give some unexpected results when the stack end has been
> aligned down, and the size is larger than the end-begin due to
> alignment.
>
> Just a hunch. Maybe I'm making a big deal out of nothing.
>
> > Also the changed _Stack_Extend_size() includes the alignment overhead.
> >
> > >
> > > Shouldn't we better do...
> > > #if CPU_STACK_ALIGNMENT > CPU_HEAP_ALIGNMENT
> > >> +  stack_align = CPU_STACK_ALIGNMENT;
> > >> +  stack_begin = (char *) RTEMS_ALIGN_UP( (uintptr_t) config->stack_area, stack_align );
> > > #else
> > > stack_begin = config->stack_area;
> > No, I think the code in the patch is fine, however, I will add an assert
> > after the block to ensure that begin and end are aligned.
> ok
>
> Is there no guarantee that stack_size == config->stack_size?
>

Suppose an application requests a stack size with
config->stack_area = m*CPU_STACK_ALIGNMENT
config->stack_size = (n*CPU_STACK_ALIGNMENT - 1)

Then, you will get
   stack_begin = config->stack_area;
because it is already aligned
   stack_end = (n-1)*CPU_STACK_ALIGNMENT;
for the align_down
and the stack size you get in the end is
    (n-1)*CPU_STACK_ALIGNMENT;
instead of
    n*CPU_STACK_ALIGNMENT-1;

so you lose CPU_STACK_ALIGNMENT-1 bytes from the stack, and the user
doesn't get back the requested number of bytes, but they also don't
get any kind of error or status. They just lose some bytes.

I think this is wrong.


> > > #endif
> > > stack_end = stack_begin + config->stack_size;
> > >
> > > So we always return the requested stack size?
> > >
> > --
> > embedded brains GmbH
> > Herr Sebastian HUBER
> > Dornierstr. 4
> > 82178 Puchheim
> > Germany
> > email: sebastian.huber at embedded-brains.de
> > phone: +49-89-18 94 741 - 16
> > fax:   +49-89-18 94 741 - 08
> >
> > Registergericht: Amtsgericht München
> > Registernummer: HRB 157899
> > Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> > Unsere Datenschutzerklärung finden Sie hier:
> > https://embedded-brains.de/datenschutzerklaerung/
> >