From nobody Mon Jan 30 19:22:05 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 4P5J332tFcz3cJPh; Mon, 30 Jan 2023 19:22:07 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from glebi.us (glebi.us [162.251.186.162]) by mx1.freebsd.org (Postfix) with ESMTP id 4P5J324XXDz3kcF; Mon, 30 Jan 2023 19:22:06 +0000 (UTC) (envelope-from glebius@freebsd.org) Authentication-Results: mx1.freebsd.org; dkim=none; spf=softfail (mx1.freebsd.org: 162.251.186.162 is neither permitted nor denied by domain of glebius@freebsd.org) smtp.mailfrom=glebius@freebsd.org; dmarc=none Received: by glebi.us (Postfix, from userid 1000) id 6652753B55; Mon, 30 Jan 2023 11:22:05 -0800 (PST) Date: Mon, 30 Jan 2023 11:22:05 -0800 From: Gleb Smirnoff To: Brooks Davis Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, jhb@freebsd.org, kib@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: <202301201805.30KI5Ht0099187@gitrepo.freebsd.org> X-Spamd-Result: default: False [0.50 / 15.00]; VIOLATED_DIRECT_SPF(3.50)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-0.999]; MIME_GOOD(-0.10)[text/plain]; RCVD_NO_TLS_LAST(0.10)[]; R_DKIM_NA(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; MIME_TRACE(0.00)[0:+]; ARC_NA(0.00)[]; R_SPF_SOFTFAIL(0.00)[~all:c]; ASN(0.00)[asn:27348, ipnet:162.251.186.0/24, country:US]; FROM_HAS_DN(0.00)[]; FREEFALL_USER(0.00)[glebius]; RCPT_COUNT_FIVE(0.00)[6]; TO_DN_SOME(0.00)[]; DMARC_NA(0.00)[freebsd.org]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Queue-Id: 4P5J324XXDz3kcF X-Spamd-Bar: / X-ThisMailContainsUnwantedMimeParts: N 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. -- Gleb Smirnoff