From owner-dev-commits-src-all@freebsd.org Thu Aug 5 17:41:58 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 424B766793A for ; Thu, 5 Aug 2021 17:41:58 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound2o.ore.mailhop.org (outbound2o.ore.mailhop.org [54.187.213.119]) (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 4GgbX56Vspz3h90 for ; Thu, 5 Aug 2021 17:41:57 +0000 (UTC) (envelope-from ian@freebsd.org) ARC-Seal: i=1; a=rsa-sha256; t=1628185311; cv=none; d=outbound.mailhop.org; s=arc-outbound20181012; b=dCziVoFTMSWKLoe06vSNsSXvvEB6RPRGu5/n90bMfpGKIDss1tub8+PFybLSnFycF4U0slte7rPB3 BuDCLYrHbk1ITFgMXiq28Pgg12hBxo0e6EzD9FnnwW89ZEo9aRH13BPBaHL1hhjsqmfCboCBEEOmke SJvdLY+PW5plLxXGcxdv5QFkz931HYTl59vG18YFn/p1Z5KfNrBCpBZBdfGCFa0fFq0TD2w6oNBQ5n jqrDZ7ypSTG734hySndWMBnOmrPUn3HW77hniMW/g+3avsnwHOu/JsDI6SVpBF9dFpBboNCePCwDM2 0/5ITt/X3SM1uqftjprwyrjcF/m2XdA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=arc-outbound20181012; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:dkim-signature:from; bh=8xad6Q1P38YW/K6tdXzN+cLrcAf+M8sm5hlFeoJKZXo=; b=D7fkIHzxGgjFqJs1Pzmpcc959b8rTjVW/pzyyJOaVpoKO4rG8QxGdX4pgM/bDRFnBe94c0ILnE/JF fXZ2PXgbsx+Lh2BrqYg9fkLJS6OhRmfjfLPN2JjmMcnCYrLxtCtyOlrgyp5cD3sweGUXx7pVmYCRKz qE1rPwAixKxBPXwvRmCvk6sUQU09gkiCtNy0ThJluR/Nc9mIh7oBRL8uJq8Vnk4QwHYULk7BvlDxQg e5/pSu3sByYMAgzYPd7sjXUczDgm6OrJ2Xn5hRamZyDXcF4ZbsyXJLvTSnNWNoT8vYrr6JYH3DX9F0 yGIHGVOmJXoRVx/voqia1kT9jDYIK/A== ARC-Authentication-Results: i=1; outbound4.ore.mailhop.org; spf=softfail smtp.mailfrom=freebsd.org smtp.remote-ip=67.177.211.60; dmarc=none header.from=freebsd.org; arc=none header.oldest-pass=0; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outbound.mailhop.org; s=dkim-high; h=content-transfer-encoding:mime-version:content-type:references:in-reply-to: date:to:from:subject:message-id:from; bh=8xad6Q1P38YW/K6tdXzN+cLrcAf+M8sm5hlFeoJKZXo=; b=D89lPYv9jHyedehI8iEhXvM12q5Z6NkLpcWkcucqsyolYHxbDMvDNaDTzeIVeLrUT/2ZFHz2bQuV0 D1V8S6tNehZbcwNx2j99o2f56Kn05t39zt4s3SeDwCxWFGHKbE7YazvyRrFu7WePyLfwzkwSN12R5R NAxiYs0++bFRz9iFsHGA93gFE5Zi5dOFTj+xO/xgXKSEOYeFM3v0bhnPrAauBlZv1Bv5HX9WuQ1RWK i5F6X4GFFv6SJuEf3bv2MzoOzB6C7oiLfIpASnoawGk6NSxfT/AhQFAVcXnK+bOiOmCQa8M/acT01N Vljr7WzDgNS3p1GxQu+ew+eTFniqN7Q== X-Originating-IP: 67.177.211.60 X-MHO-RoutePath: aGlwcGll X-MHO-User: 692b7e57-f614-11eb-a658-89389772cfc7 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (c-67-177-211-60.hsd1.co.comcast.net [67.177.211.60]) by outbound4.ore.mailhop.org (Halon) with ESMTPSA id 692b7e57-f614-11eb-a658-89389772cfc7; Thu, 05 Aug 2021 17:41:49 +0000 (UTC) Received: from [172.22.42.84] (rev2.hippie.lan [172.22.42.84]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id 175HfmhW085229; Thu, 5 Aug 2021 11:41:48 -0600 (MDT) (envelope-from ian@freebsd.org) X-Authentication-Warning: paranoia.hippie.lan: Host rev2.hippie.lan [172.22.42.84] claimed to be [172.22.42.84] Message-ID: <1013cb35c9a2c3cc55b4f3bd10d14b1540c9462f.camel@freebsd.org> Subject: Re: git: 2694c869ff9f - main - ktls: fix a panic with INVARIANTS From: Ian Lepore To: Andrew Gallatin , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Date: Thu, 05 Aug 2021 11:41:48 -0600 In-Reply-To: <202108051710.175HAP1D031061@gitrepo.freebsd.org> References: <202108051710.175HAP1D031061@gitrepo.freebsd.org> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.40.3 FreeBSD GNOME Team MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 4GgbX56Vspz3h90 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Aug 2021 17:41:58 -0000 On Thu, 2021-08-05 at 17:10 +0000, Andrew Gallatin wrote: > The branch main has been updated by gallatin: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=2694c869ff9ff60fd8e3d4d7936b7dc61763c18a > > commit 2694c869ff9ff60fd8e3d4d7936b7dc61763c18a > Author:     Andrew Gallatin > AuthorDate: 2021-08-05 17:05:00 +0000 > Commit:     Andrew Gallatin > CommitDate: 2021-08-05 17:09:06 +0000 > >     ktls: fix a panic with INVARIANTS >     >     98215005b747fef67f44794ca64abd473b98bade introduced a new >     thread that uses tsleep(..0) to sleep forever.  This hit >     an assert due to sleeping with a 0 timeout. >     >     So spell "forever" using SBT_MAX instead, which does not >     trigger the assert. >     >     Pointy hat to: gallatin >     Pointed out by: emaste >     Sponsored by: Netflix > --- >  sys/kern/uipc_ktls.c | 2 +- >  1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sys/kern/uipc_ktls.c b/sys/kern/uipc_ktls.c > index 17b87195fc50..47815c266667 100644 > --- a/sys/kern/uipc_ktls.c > +++ b/sys/kern/uipc_ktls.c > @@ -2240,7 +2240,7 @@ ktls_alloc_thread(void *ctx) >         nbufs = 0; >         for (;;) { >                 atomic_store_int(&sc->running, 0); > -               tsleep(sc, PZERO, "waiting for work", 0); > +               tsleep_sbt(sc, PZERO, "waiting for work", SBT_MAX, > SBT_1S, 0); >                 atomic_store_int(&sc->running, 1); >                 sc->wakeups++; >                 if (nbufs != ktls_max_alloc) { Finding a different way to spell "forever" is not a valid way to fix a problem where you're being warned that it is not safe to sleep forever. The assert was warning you that the code was vulnerable to hanging forever due to a missed wakeup. The code is still vulnerable to that, but now the problem is hidden and will be very difficult to find (more so because the wait message also violates the convention of using a short name that can be displayed by tools such as ps(1) and SIGINFO, where the wait-channel display is currently likely to show as "waitin"). I haven't looked at the code outside of the few lines shown in the commit diff, but based on the names involved, I suspect the right fix is to protect sc->running with a mutex and use msleep() instead of trying to roll-your-own with atomics. That would certainly be true if the wakeup code is some form of "if (!sc->running) wakeup(sc);" -- Ian