From nobody Wed Feb 01 06:51:56 2023 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4P6CJp4ZvGz3cLQh; Wed, 1 Feb 2023 06:52:10 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4P6CJp06M9z3DN3; Wed, 1 Feb 2023 06:52:09 +0000 (UTC) (envelope-from kostikbel@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 3116puAs009130 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Wed, 1 Feb 2023 08:51:59 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 3116puAs009130 Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 3116puWU009129; Wed, 1 Feb 2023 08:51:56 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 1 Feb 2023 08:51:56 +0200 From: Konstantin Belousov To: John Baldwin Cc: Gleb Smirnoff , Brooks Davis , 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: References: <202301201805.30KI5Ht0099187@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=4.0.0 X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-14) on tom.home X-Rspamd-Queue-Id: 4P6CJp06M9z3DN3 X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:6939, ipnet:2001:470::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N 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 > > > B> AuthorDate: 2023-01-20 18:03:39 +0000 > > > B> Commit: Brooks Davis > > > 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.