Re: git: fa1d803c0f65 - main - epoch: replace hand coded assertion

On Tue, Jan 31, 2023 at 10:30:49AM -0800, John Baldwin wrote:
> On 1/30/23 2:31 PM, Konstantin Belousov wrote:
> > On Mon, Jan 30, 2023 at 11:22:05AM -0800, Gleb Smirnoff wrote:
> > >    Brooks, John, Kostik,
> > > 
> > > On Fri, Jan 20, 2023 at 06:05:17PM +0000, Brooks Davis wrote:
> > > B> The branch main has been updated by brooks:
> > > B>
> > > B> URL: https://cgit.FreeBSD.org/src/commit/?id=fa1d803c0f652d72840a3c59139baf9d30792860
> > > B>
> > > B> commit fa1d803c0f652d72840a3c59139baf9d30792860
> > > B> Author:     Brooks Davis <brooks@FreeBSD.org>
> > > B> AuthorDate: 2023-01-20 18:03:39 +0000
> > > B> Commit:     Brooks Davis <brooks@FreeBSD.org>
> > > B> CommitDate: 2023-01-20 18:04:40 +0000
> > > B>
> > > B>     epoch: replace hand coded assertion
> > > B>
> > > B>     The assertion is equivalent to kstack_contains() so use that rather
> > > B>     than spelling it out.
> > > B>
> > > B>     Suggested by:   jhb
> > > B>     Reviewed by:    jhb
> > > B>     MFC after:      1 week
> > > B>     Sponsored by:   DARPA, AFRL
> > > B>     Differential Revision:  https://reviews.freebsd.org/D38107
> > > B> ---
> > > B>  sys/kern/subr_epoch.c | 4 +---
> > > B>  1 file changed, 1 insertion(+), 3 deletions(-)
> > > B>
> > > B> diff --git a/sys/kern/subr_epoch.c b/sys/kern/subr_epoch.c
> > > B> index 100221cd62f9..98a560e44c9d 100644
> > > B> --- a/sys/kern/subr_epoch.c
> > > B> +++ b/sys/kern/subr_epoch.c
> > > B> @@ -468,9 +468,7 @@ _epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE)
> > > B>
> > > B>  	MPASS(cold || epoch != NULL);
> > > B>  	td = curthread;
> > > B> -	MPASS((vm_offset_t)et >= td->td_kstack &&
> > > B> -	    (vm_offset_t)et + sizeof(struct epoch_tracker) <=
> > > B> -	    td->td_kstack + td->td_kstack_pages * PAGE_SIZE);
> > > B> +	MPASS(kstack_contains(td, (vm_offset_t)et, sizeof(*et)));
> > > B>
> > > B>  	INIT_CHECK(epoch);
> > > B>  	MPASS(epoch->e_flags & EPOCH_PREEMPT);
> > > 
> > > At Netflix we are currently using more strict assertion:
> > > 
> > >      Check the epoch tracker not against the whole stack size, but against
> > >      the legitimate part of the stack.  While here add the same check for
> > >      the epoch exit.  The bug we are going to catch most likely is associated
> > >      with incorrect exit.
> > > diff --git a/FreeBSD/sys/kern/subr_epoch.c b/FreeBSD/sys/kern/subr_epoch.c
> > > index b457b070cb65..b345719eb406 100644
> > > --- a/FreeBSD/sys/kern/subr_epoch.c
> > > +++ b/FreeBSD/sys/kern/subr_epoch.c
> > > @@ -420,7 +420,8 @@ _epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE)
> > >          MPASS(cold || epoch != NULL);
> > >          MPASS(epoch->e_flags & EPOCH_PREEMPT);
> > >          td = curthread;
> > > -       MPASS((vm_offset_t)et >= td->td_kstack &&
> > > +       MPASS((vm_offset_t)et + sizeof(struct epoch_tracker) >=
> > > +           (vm_offset_t)__builtin_frame_address(0) &&
> > >              (vm_offset_t)et + sizeof(struct epoch_tracker) <=
> > >              td->td_kstack + td->td_kstack_pages * PAGE_SIZE);
> > > @@ -457,8 +458,13 @@ _epoch_exit_preempt(epoch_t epoch, epoch_tracker_t et EPOCH_FILE_LINE)
> > >          struct epoch_record *er;
> > >          struct thread *td;
> > > -       INIT_CHECK(epoch);
> > >          td = curthread;
> > > +       MPASS((vm_offset_t)et + sizeof(struct epoch_tracker) >=
> > > +           (vm_offset_t)__builtin_frame_address(0) &&
> > > +           (vm_offset_t)et + sizeof(struct epoch_tracker) <=
> > > +           td->td_kstack + td->td_kstack_pages * PAGE_SIZE);
> > > +
> > > +       INIT_CHECK(epoch);
> > >          critical_enter();
> > >          sched_unpin();
> > >          THREAD_SLEEPING_OK();
> > > 
> > > 
> > > What do you guys think on legitimacy of the improved assertion that uses
> > > compiler built to get the current top of stack and makes the assertion even
> > > stricter?
> > > 
> > > I think in FreeBSD we should at least add the MPASS(kstack_contains(td, ...)
> > > assertion to the epoch exit.
> > 
> > The __builtin_frame_address() is somewhat weird thing, e.g. it assumes
> > that the local function frame is contiguous, and static. Also I do not
> > see a guarantee that any local variable address is higher that the value
> > returned from the builtin.
> 
> Though in these cases the pointer value (et) is not a local variable on
> this frame, but should be in an earlier frame (usually the callers).
> 
> > I would be happier if you added a per-arch primitive that reads the stack
> > pointer.  This helper can be used by GET_STACK_USAGE() as well.
> > 
> > Also typical arch puts at least the thread' pcb on top of the stack, so
> > the assertion could be improved by not allowing the address to fall into
> > pcb, or FPU save area on i386.
> 
> kstack_contains arguably should exclude the pcb as well as it is typically
> used in stack unwinders to validate frame pointers.  I think the only reason
> for pcbs to be in the kstack at all is to simplify swapping, and if we
> ever decided to drop kstack swapping it would be nicer to allocate the pcb
> separate from the kstack IMO.
Apparently pcb is not on kstack for amd64, I completely forgot that.

I did the fixes both for kstack_contains() and GET_STACK_USAGE()
in D38320.