From nobody Mon Jul 15 20:13:17 2024 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 4WND0Z57h0z5RZ4j; Mon, 15 Jul 2024 20:13:18 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (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 "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4WND0Z410tz4cjL; Mon, 15 Jul 2024 20:13:18 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1721074398; 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=U1wox1LMz6GN2mnC/H1991KjuCABxi23L7xTfPwshLY=; b=jv9PFZUaFXK0wzbjb39OkuHlyiEZbwIP2vEadNGwEJBLL3kWgiCOskOmdgnCm7TpOI3YPI pm7ImG0qO23HtyVMUxREYHX0dFzJDuoaDsCRvfe20qWSol3dTeKeuIdmfi46VffhAu3IbQ u0vU5Qnc/dRXiN8x3AQHE3Rq//H3fddWot889JSZCh+87a1pUF7IPhQbtjwHD+nXNF/6SP LwU6TrduDL9r10hngAlVc8YcQf81judUKvohs7bND5U/harKRGRNB6AEfr228GVPSavq0Y g0v15NvMi5C/9rNUNbZA3hKtvL9tOZir36/svPWPUGn2scRYk1IbZTQx5kFMcw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1721074398; a=rsa-sha256; cv=none; b=afADPdE70DXWmjP4Jj4R8+lO8DDifv81aiNBJahIA/Fzd5sf2UVYnApn8LnoOkDNTE0Yon 6KJ4Gv7Z7Sv5bANEzryqoVYDXyot7Iv0ZZsVM932xda8k1aa/b/3D4ru0PO5911Y7cmkue HCElLx2+CVQHeDZjRHR5zV2IOnzQYgKeeF7+7bsFx4FYg7gSSIILd3GO7vqig9xrXlbYMN I6kMqxElMKGUtCAdov0naJ0EtIVGvmbyfIkvOuVswX7r05Tk14bHE32aBIxa8cHV5galI3 odXO9wUOoyKVKA+mPabL6kUhX9y2yRSSejhv+m55GlHAh8s9oE+auT4XHRP3Ig== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1721074398; 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=U1wox1LMz6GN2mnC/H1991KjuCABxi23L7xTfPwshLY=; b=eMofur4F4DERStJ7RDGMW3UdLdA2SUR1YJ2gOTDqTa0mN8jC6/6CyobablIt8kgdPUekX+ VI1PpUiBeEQKmkmFCcx8UVtl5u7vIy0AE/gIVfJd8dkbo/ANlUKg5nA5yCc1t3ajdrB8af j+F5sFO7GhxEOMb22wZ4mbQnThAGSiddDSaNr58q5is/F1q4Ig0fxXWYkqYb5D1gmVuiA4 L32/xEb5Xjjngz1L6zzL5HfNKPbQeqldZvmiTmsaIeU7Wj4OVb5sXVehKqZQBAYUaDK5qo 2dV0Ww5QmWrMijZT14Mr70VwGdShfIr9GLnN/t43wGktvjLrh/7TSIydsSZZXQ== Received: from [IPV6:2601:5c0:4200:b830:28d3:efa9:9f5c:a97f] (unknown [IPv6:2601:5c0:4200:b830:28d3:efa9:9f5c:a97f]) (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 4WND0Z1qqQz1RLl; Mon, 15 Jul 2024 20:13:18 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <752a3f0a-438e-4465-a8ae-47700e79306d@FreeBSD.org> Date: Mon, 15 Jul 2024 16:13:17 -0400 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: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: 87ee63bac69d - main - locks: add a runtime check for missing turnstile Content-Language: en-US To: Mateusz Guzik Cc: Mateusz Guzik , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202407111107.46BB7uSb007077@gitrepo.freebsd.org> <81cfe7ad-cbee-4122-abef-e47ce2b34f05@FreeBSD.org> From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7/15/24 14:40, Mateusz Guzik wrote: > On Mon, Jul 15, 2024 at 8:33 PM Mateusz Guzik wrote: >> >> On Mon, Jul 15, 2024 at 8:21 PM John Baldwin wrote: >>> >>> On 7/15/24 13:59, Mateusz Guzik wrote: >>>> On Mon, Jul 15, 2024 at 6:22 PM John Baldwin wrote: >>>>> >>>>> On 7/11/24 07:07, Mateusz Guzik wrote: >>>>>> The branch main has been updated by mjg: >>>>>> >>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=87ee63bac69dc49291f55590b8baa57cad6c7d85 >>>>>> >>>>>> commit 87ee63bac69dc49291f55590b8baa57cad6c7d85 >>>>>> Author: Mateusz Guzik >>>>>> AuthorDate: 2024-07-11 00:17:27 +0000 >>>>>> Commit: Mateusz Guzik >>>>>> CommitDate: 2024-07-11 11:06:52 +0000 >>>>>> >>>>>> locks: add a runtime check for missing turnstile >>>>>> >>>>>> There are sometimes bugs which result in the unlock fast path failing, >>>>>> which in turns causes a not-helpful crash report when dereferencing a >>>>>> NULL turnstile. Help debugging such cases by pointing out what happened >>>>>> along with some debug. >>>>>> >>>>>> Sponsored by: Rubicon Communications, LLC ("Netgate") >>>>>> --- >>>>>> sys/kern/kern_mutex.c | 4 +++- >>>>>> sys/kern/kern_rwlock.c | 16 ++++++++++++---- >>>>>> 2 files changed, 15 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/sys/kern/kern_mutex.c b/sys/kern/kern_mutex.c >>>>>> index 90361b23c09a..0fa624cc4bb1 100644 >>>>>> --- a/sys/kern/kern_mutex.c >>>>>> +++ b/sys/kern/kern_mutex.c >>>>>> @@ -1053,7 +1053,9 @@ __mtx_unlock_sleep(volatile uintptr_t *c, uintptr_t v) >>>>>> turnstile_chain_lock(&m->lock_object); >>>>>> _mtx_release_lock_quick(m); >>>>>> ts = turnstile_lookup(&m->lock_object); >>>>>> - MPASS(ts != NULL); >>>>>> + if (__predict_false(ts == NULL)) { >>>>>> + panic("got NULL turnstile on mutex %p v %zx", m, v); >>>>>> + } >>>>> >>>>> Hmm, this is just an expanded KASSERT() but always on rather than conditional on INVARIANTS? >>>>> >>>>> Do you have examples of the type of bugs that cause this? (Is it unlocking a freed mutex >>>>> or the like?) We generally hide all these types of checks under INVARIANTS rather than >>>>> shipping them in release kernels. >>>>> >>>> >>>> Use-after-free, overflow, underflow, bitflip or what have you all can >>>> fail the fast path. >>>> >>>> Once that happens and the kernel crashes with a null pointer deref, >>>> here is a crash at netgate which prodded this: >>>> calltrap() at calltrap+0x8/frame 0xfffffe0106720920 >>>> --- trap 0xc, rip = 0xffffffff80d5ab70, rsp = 0xfffffe01067209f0, rbp >>>> = 0xfffffe0106720a00 --- >>>> turnstile_broadcast() at turnstile_broadcast+0x40/frame 0xfffffe0106720a00 >>>> __rw_wunlock_hard() at __rw_wunlock_hard+0x9e/frame 0xfffffe0106720a30 >>>> nd6_resolve_slow() at nd6_resolve_slow+0x2d7/frame 0xfffffe0106720aa0 >>>> nd6_resolve() at nd6_resolve+0x125/frame 0xfffffe0106720b10 >>>> ether_output() at ether_output+0x4e7/frame 0xfffffe0106720ba0 >>>> ip_output_send() at ip_output_send+0xdc/frame 0xfffffe0106720be0 >>>> ip_output() at ip_output+0x1295/frame 0xfffffe0106720ce0 >>>> ip_forward() at ip_forward+0x3c2/frame 0xfffffe0106720d90 >>>> ip_input() at ip_input+0x705/frame 0xfffffe0106720df0 >>>> swi_net() at swi_net+0x138/frame 0xfffffe0106720e60 >>>> ithread_loop() at ithread_loop+0x257/frame 0xfffffe0106720ef0 >>>> fork_exit() at fork_exit+0x7f/frame 0xfffffe0106720f30 >>>> fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe0106720f30 >>>> --- trap 0, rip = 0, rsp = 0, rbp = 0 --- >>>> >>>> Neither the register dump nor anything in the backtrace indicate what happened. >>>> >>>> Since the kernel is going down anyway, one may as well get some debug from it. >>> >>> If you don't mind the extra branches for sanity checks, why not just run with >>> INVARIANTS? That is, what makes these particular assertions different from >>> other assertions such that they should be on unconditionally? The last line below >>> applies to pretty much every other assertion in the tree. >>> >> >> This adds a branch in the slowpath, a spot which should relatively >> rarely execute compared to the fast path. On top of that the branch at >> hand does not do any extra memory accesses or complex arithmetic. >> >> So no, I don't think I may as well run with INVARIANTS. > > How about this: if you strongly about this branch, feel free to revert > the commit, I'm just going to keep the change at Netgate. > > But should you do it, make sure to not add avoidable branches in your stuff. It's more that I think this is an unusual case (unconditional assertions rather than conditional on INVARIANTS) such that it's probably worth being a bit more explicit about that in the log with the rationale, etc. That is, I don't think "runtime check" quite communicates that you are intentionally doing an unconditional assertion, and a bit more detail about the specific types of bugs might have been useful in the log as well. I think it's fine if we want to have some checks that are always on, but it's currently quite rare so needs a bit more rationale in the log than other changes is all. -- John Baldwin