Re: git: 5d45f99a1ed8 - stable/13 - i386 doreti: Fix calculation of stack frame size

On Wed, Sep 07, 2022 at 11:56:04AM +0200, Tijl Coosemans wrote:
> On Tue, 6 Sep 2022 20:18:33 +0300 Konstantin Belousov
> <kostikbel@gmail.com> wrote:
> > On Tue, Sep 06, 2022 at 05:22:25PM +0200, Tijl Coosemans wrote:
> >> On Fri, 2 Sep 2022 15:58:14 +0000 Colin Percival <cperciva@tarsnap.com>
> >> wrote:  
> >>> On 9/2/22 08:06, Tijl Coosemans wrote:  
> >>>> The branch stable/13 has been updated by tijl:
> >>>> 
> >>>> URL: https://cgit.FreeBSD.org/src/commit/?id=5d45f99a1ed861618d5c4d5b4252d5eba001ad35
> >>>> 
> >>>> commit 5d45f99a1ed861618d5c4d5b4252d5eba001ad35
> >>>> Author:     Tijl Coosemans <tijl@FreeBSD.org>
> >>>> AuthorDate: 2022-09-02 14:16:35 +0000
> >>>> Commit:     Tijl Coosemans <tijl@FreeBSD.org>
> >>>> CommitDate: 2022-09-02 15:02:00 +0000
> >>>> 
> >>>>      i386 doreti: Fix calculation of stack frame size
> >>>>      
> >>>>      Reviewed by:    kib
> >>>>      Fixes:          e8b2980e4a12 - i386 doreti: stop saving/restoring %ecx
> >>>>      
> >>>>      (cherry picked from commit cfdc649e455bc0d37d42c46b59360462e93b4300)    
> >>> I assume this MFC took place 23 minutes after the original commit because it
> >>> was an urgent fix; to avoid the need for assumptions, please indicate the
> >>> reason for an accelerated MFC in the commit message when you skip the normal
> >>> 3 day waiting period.  
> >> 
> >> Will do.  In this case it was a trivial fixup in a critical path.  At the
> >> time I also thought it fixed a panic I was seeing but this turned out to
> >> be caused by another commit.  
> > 
> > It is not on critical path, at worst it affected vm86 mode only.
> > That said, 'the other bug' was there before the series.
> 
> Looking into this again I now believe your change was actually correct.
> trampstk = (uintptr_t)tramp_stack_base + TRAMP_STACK_SZ - VM86_STACK_SPACE
> So it already has VM86_STACK_SPACE subtracted.  Can you double-check
> this?

The trampstk pointer has the space for additional segment registers values
that are pushed by hardware (in C it is at the end of the struct trapframe).
But the calculation for %ecx is the size we are going to copy from kstack
to trampoline.  So when we are returning to vm86 mode, we must account for
additional segment registers by increasing the copy size.

The trampstk definition you cited means that we do not overflow the
destination by the copy.

So I think that your fix was correct.