Date: Wed, 1 Feb 2023 08:51:56 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Gleb Smirnoff <glebius@freebsd.org>, Brooks Davis <brooks@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: fa1d803c0f65 - main - epoch: replace hand coded assertion Message-ID: <Y9oMDJo%2BlD28XElJ@kib.kiev.ua> In-Reply-To: <e6cbc000-4bd8-f1cd-f600-4393dc371371@FreeBSD.org> References: <202301201805.30KI5Ht0099187@gitrepo.freebsd.org> <Y9gY3ZN5da3RxnvW@cell.glebi.us> <Y9hFJzrDePKSesCY@kib.kiev.ua> <e6cbc000-4bd8-f1cd-f600-4393dc371371@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Y9oMDJo%2BlD28XElJ>