From nobody Tue Jan 31 18:30:49 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 4P5tsS274mz3bxM7; Tue, 31 Jan 2023 18:30:52 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (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-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4P5tsS1gbBz4Gj3; Tue, 31 Jan 2023 18:30:52 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1675189852; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SXu9kHZ9Jy5KXPzoeEwHKZqdP+qrvz0bfVvMZ2gsYko=; b=taKy/o5XV6y7ZQ49U5umoqwkPyyui2EE4Vl6tK/Mb0H0UAC2eoYy+m9D/8+RRKgu9amYs9 kmAP+f/UpR8UxEIcdGBNuFSR4EYFS/d03tVg9aANQwtM9be5dRZ9NgRf1CDjkU2IkcYXuD +bujozLE4sBFW3t8Gexzjqtas0sO0gHQHASrfsPi3mrbVWL/n/Puq0ORxWz9L2W5hzDE/1 0RV5Wp/O0Zo9VxYLnzrJwmSj+DsI5aAANW/fQI/Ii5h/c6E1FgDb/FDVPakM18sRSf9ZNB Ua24l7/w2qidRnL8zYsHi+M6PJAf1o73lxSYOUgPw38z9cnocfMz/Mjhd2vweA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1675189852; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SXu9kHZ9Jy5KXPzoeEwHKZqdP+qrvz0bfVvMZ2gsYko=; b=pzdkAsmfOe3grORovvwh3yshkhcoaoDhbPc/Dy60VrHkJx4kTNT/oR6MvU1S8MNDxzgJol 0nIs31Y9Idadzr2xbQbZqNdWpQS3oD8B4+Z0pP3GW2ntETpLc3rH6KMaPMe9IGfuyg6F77 PB8Gq4XlyPHJY79zHUuPtidr7MwP9Are5QqSAMYOeMfl9Zp0YRWQK8X0+uJV7Xvlg80poE N7onwnMkYjbOPEc3uKC/Fk5tIYf3x1+5c1WmAKAF2jlHZoZU4pK/gPxFxB0WCQzL2aUZLs rqoTnrYvREiICz56ZmEY6ndE1HVLFdZAjwhYb/rjNTsG2mY4CLA0pP/9dj60pQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1675189852; a=rsa-sha256; cv=none; b=bb0gsa7fty/X7gGThFYGUjh/gEUhrOERNS7RHH1diFgY8RK+o+ur9O2BrrneYAJcITR6Zl 1PvIYb8l3/cZ6QMOBaeA9iWnzeBAjXu5LegbdIukKg9GxXhB79fIj88ufIj98JZW+T+fdX gpcST9AqcxldSoYIp03VhR+btpxdTx54ncmRhTOLNjDukyJ62zh4jE1Fdtg80HBpOivAwU P7Lsoc8qC1HDD3G4oVxCPIoElDqUnwBpyQHdsctq7c5dnXau1wgQzmMDKhrRrKWcohC/2r xSXoXliqcInLVCAkCKiwc2ov5CpgEY6S30NP5bQXoEH+5y6mSmFPOYpMpb2CMg== Received: from [IPV6:2601:642:4c00:139c:bc83:d7b0:5032:707b] (unknown [IPv6:2601:642:4c00:139c:bc83:d7b0:5032:707b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4P5tsR3QJ9zhHv; Tue, 31 Jan 2023 18:30:51 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Tue, 31 Jan 2023 10:30:49 -0800 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 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: git: fa1d803c0f65 - main - epoch: replace hand coded assertion Content-Language: en-US To: Konstantin Belousov , Gleb Smirnoff Cc: Brooks Davis , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202301201805.30KI5Ht0099187@gitrepo.freebsd.org> From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-ThisMailContainsUnwantedMimeParts: N 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. -- John Baldwin