From nobody Fri Sep 13 15:45:00 2024 X-Original-To: dev-commits-src-all@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 4X4zCr3V7jz5V6c3; Fri, 13 Sep 2024 15:45:28 +0000 (UTC) (envelope-from gallatin@fastmail.com) Received: from fout3-smtp.messagingengine.com (fout3-smtp.messagingengine.com [103.168.172.146]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4X4zCq5sgBz4s6M; Fri, 13 Sep 2024 15:45:27 +0000 (UTC) (envelope-from gallatin@fastmail.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=fastmail.com header.s=fm1 header.b="dl3d+/nS"; dkim=pass header.d=messagingengine.com header.s=fm1 header.b=n4pIYDLl; dmarc=pass (policy=none) header.from=fastmail.com; spf=pass (mx1.freebsd.org: domain of gallatin@fastmail.com designates 103.168.172.146 as permitted sender) smtp.mailfrom=gallatin@fastmail.com Received: from phl-compute-10.internal (phl-compute-10.phl.internal [10.202.2.50]) by mailfout.phl.internal (Postfix) with ESMTP id 57FCE13802AC; Fri, 13 Sep 2024 11:45:27 -0400 (EDT) Received: from phl-imap-13 ([10.202.2.103]) by phl-compute-10.internal (MEProxy); Fri, 13 Sep 2024 11:45:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fastmail.com; h= cc:cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm1; t=1726242327; x=1726328727; bh=B6Nhf4y+S0 HS9fwp1LGLBzfBoJarB40zw41kYRzYj+w=; b=dl3d+/nS5puy1QVZi98ZLqLEqd Ov4rCJb1R8+OkWLNH8KPlElMrulqQHRv/v0ozn8yaE8pG4v0G7Nmqd2hFcqRVuPT VMxZtlSjhfFhg0GGhyCpM0KiJUK7hyhTtfGwNJg4oelWFQnjtkxRo7tiHuqZcvSv 9bIvlR906en0PXfiuqAfJF5xfgL1SNnYEMPuvJQOlxlhnAE1IVS+jKY1UI4T+qap +J2vaVeh9l19xzvuJLrXYXrvm+Fuqq2CyHFwtQTtGMoZHrlS96CX+RYPX4x53Sq+ v+7Vt504MiIs5PUK5gbk9yS7b0dOvw2nb3ia/TQc4gLaKFh5Zfgkd1XzQ5vA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; t=1726242327; x=1726328727; bh=B6Nhf4y+S0HS9fwp1LGLBzfBoJar B40zw41kYRzYj+w=; b=n4pIYDLlVlCFeVS0VU1sG75bYMN0sOoS4caj1tnFU2ys FzuxI1krdldvNSOvy8r2kUgQXAGbIelSjsf1klqZXiGl5oJA9hIHfseEVmtGwSCJ JqzG06TG/IieX8K/iLWWgBhBqcJNxYmZ2R0YD6OST1hNHLb/69V/0WWob9kGbjIN YZJmxUSqD/PegL7GeVL17lGXd4rlYFFERqIAoixvmWfO7dqjzWozucfur2xEnxHx 93PaehBbVbH3e/O9w9/6RkWtj/P6Ik/fZa6DtVHhrmhIWXtvgPOKgBlgbOOx9kyB CKwVYFGvqyhWoBhoNVMAL05RRrbgtQtpYmNB56VUKQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeeftddrudejjedgledvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhepofggfffhvfevkfgjfhfutgesmhdtreerredtjeen ucfhrhhomhepfdffrhgvficuifgrlhhlrghtihhnfdcuoehgrghllhgrthhinhesfhgrsh htmhgrihhlrdgtohhmqeenucggtffrrghtthgvrhhnpeefuefhgedvtdejvdduvdeuudeu geeuieduveethfehheeiueetgffhudfgledvjeenucffohhmrghinhepghhithhhuhgsrd gtohhmpdhfrhgvvggsshgurdhorhhgpdhshiiikhgrlhhlvghrrdgrphhpshhpohhtrdgt ohhmnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepgh grlhhlrghtihhnsehfrghsthhmrghilhdrtghomhdpnhgspghrtghpthhtohepiedpmhho uggvpehsmhhtphhouhhtpdhrtghpthhtohepuggvvhdqtghomhhmihhtshdqshhrtgdqrg hllhesfhhrvggvsghsugdrohhrghdprhgtphhtthhopeguvghvqdgtohhmmhhithhsqdhs rhgtqdhmrghinhesfhhrvggvsghsugdrohhrghdprhgtphhtthhopehjhhgssehfrhgvvg gsshgurdhorhhgpdhrtghpthhtohepkhhpsehfrhgvvggsshgurdhorhhgpdhrtghpthht ohepmhgrrhhkjhesfhhrvggvsghsugdrohhrghdprhgtphhtthhopehsrhgtqdgtohhmmh hithhtvghrshesfhhrvggvsghsugdrohhrgh X-ME-Proxy: Feedback-ID: i2f014658:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id 23AFA1F0007A; Fri, 13 Sep 2024 11:45:27 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Date: Fri, 13 Sep 2024 11:45:00 -0400 From: "Drew Gallatin" To: "John Baldwin" , "Kristof Provost" , "Mark Johnston" Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Message-Id: In-Reply-To: <1f61b6de-0fe2-4343-b4ad-f0866785a4bc@FreeBSD.org> References: <202409111118.48BBIQ2h065089@gitrepo.freebsd.org> <2b1955e2-fbf1-41cb-b256-a9a257b16a83@FreeBSD.org> <1f61b6de-0fe2-4343-b4ad-f0866785a4bc@FreeBSD.org> Subject: Re: git: f08247fd888e - main - Assert that mbufs are writable if we write to them Content-Type: multipart/mixed; boundary=f05b720e4072433382bbf437e9890aa6 X-Spamd-Bar: --- X-Spamd-Result: default: False [-3.05 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; DMARC_POLICY_ALLOW(-0.50)[fastmail.com,none]; R_DKIM_ALLOW(-0.20)[fastmail.com:s=fm1,messagingengine.com:s=fm1]; R_SPF_ALLOW(-0.20)[+ip4:103.168.172.128/27]; RCVD_IN_DNSWL_LOW(-0.10)[103.168.172.146:from]; MIME_GOOD(-0.10)[multipart/mixed,multipart/alternative,text/plain]; NEURAL_SPAM_SHORT(0.04)[0.039]; XM_UA_NO_VERSION(0.01)[]; FREEMAIL_ENVFROM(0.00)[fastmail.com]; RCVD_COUNT_THREE(0.00)[3]; TO_DN_SOME(0.00)[]; ARC_NA(0.00)[]; FREEMAIL_FROM(0.00)[fastmail.com]; MIME_TRACE(0.00)[0:+,1:+,2:+,3:~,4:~]; RCVD_TLS_LAST(0.00)[]; FROM_HAS_DN(0.00)[]; DWL_DNSWL_NONE(0.00)[messagingengine.com:dkim]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; MID_RHS_MATCH_FROMTLD(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; ASN(0.00)[asn:209242, ipnet:103.168.172.0/24, country:US]; DKIM_TRACE(0.00)[fastmail.com:+,messagingengine.com:+]; FREEFALL_USER(0.00)[gallatin]; HAS_ATTACHMENT(0.00)[]; RCPT_COUNT_FIVE(0.00)[6] X-Rspamd-Queue-Id: 4X4zCq5sgBz4s6M --f05b720e4072433382bbf437e9890aa6 Content-Type: multipart/alternative; boundary=16fb958509d44755bdf660d7a1b86bf9 --16fb958509d44755bdf660d7a1b86bf9 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable I think you also need to remove M_EXTPG from M_WRITABLE(). Attached a t= rivial, untested patch. Drew On Thu, Sep 12, 2024, at 5:40 AM, John Baldwin wrote: > On 9/12/24 05:03, John Baldwin wrote: > > I think part of the motivation for marking M_EXTPG as read-only is t= hat you can't "write" > > to m_data via mtod() or the like. That said, M_EXTPG aren't really = read-only. It > > depends on the backing store. M_EXTPG were first merged into FreeBS= D prior to KTLS to > > support sendfile, and in that case, they should be M_RDONLY because = they alias pages > > from the file's VM object. However, M_EXTPG mbufs allocated via fun= ctions like > > m_uiotombuf_nomap should not be M_RDONLY. I think this originated i= n the original > > import of KTLS which doesn't push setting M_RDONLY out to the caller= s of mb_alloc_extpgs, > > and a few other places that hardcode M_RDONLY with M_EXTPG (_mb_unma= pped_to_ext should > > preserve M_RDONLY from the original mbuf instead of forcing M_RDONLY= ). > >=20 > > I can take a stab at a patch but won't have time to really test it u= ntil after Euro. >=20 > Patch available below. Compile tested but not run-tested: >=20 > https://github.com/freebsd/freebsd-src/compare/main...bsdjhb:freebsd:m= _extpg_rdonly >=20 > > On 9/11/24 16:56, Drew Gallatin wrote: > >> M_EXTPGS mbufs are marked read-only because they refer to external = data. The original crypto code, (before kTLS was converted to OCF), us= ed to just build an iovec using PHYS_TO_DMAP() on the page array. I thi= nk this case was missed during the conversion to OCF. > >> > >> I'm not sure what the best thing to do is, as they should be read o= nly, except this one specific case.... I'd be tempted to just nerf the K= ASSERT for EXTPGS. > >> > >> On Wed, Sep 11, 2024, at 11:02 AM, Kristof Provost wrote: > >>> On 11 Sep 2024, at 16:45, Mark Johnston wrote: > >>>> On Wed, Sep 11, 2024 at 11:18:26AM +0000, Kristof Provost wrote: > >>>>> The branch main has been updated by kp: > >>>>> > >>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=3Df08247fd888e6f7db= 0ecf2aaa39377144ac40b4c > >>>>> > >>>>> commit f08247fd888e6f7db0ecf2aaa39377144ac40b4c > >>>>> Author: Kristof Provost > >>>>> AuthorDate: 2024-09-10 20:15:31 +0000 > >>>>> Commit: Kristof Provost > >>>>> CommitDate: 2024-09-11 11:17:48 +0000 > >>>>> > >>>>> Assert that mbufs are writable if we write to them > >>>>> > >>>>> m_copyback() modifies the mbuf, so it must be a writable m= buf. > >>>> > >>>> This change still triggers a panic for me when running KTLS tests= . I > >>>> note that EXTPG mbufs always have M_RDONLY set, but I'm not quite= sure > >>>> why. I suspect such mbufs need special handling with respect to = the new > >>>> assertion. > >>>> > >>>> syzbot also triggered this panic: > >>>> https://syzkaller.appspot.com/bug?extid=3D58c918369f9dc323409d > >>>> > >>> Yeah, I saw that one before I went out for a bike ride. > >>> > >>> Clearly something is wrong. Either ktls is using read-only buffers= or the M_WRITABLE() macro isn=E2=80=99t quite smart enough to spot this= specific case. > >>> > >>> I=E2=80=99m not familiar enough with ktls to easily tell which. > >>> > >>> I=E2=80=99ll back this assertion change out for now, so we=E2=80=99= re not panicing test machines while we figure this out. > >>> > >>> Best regards, > >>> Kristof > >>> > >> > >=20 >=20 > --=20 > John Baldwin >=20 >=20 --16fb958509d44755bdf660d7a1b86bf9 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
I think you als= o need to remove M_EXTPG from M_WRITABLE().  Attached a trivial, un= tested patch.

Drew

=
On Thu, Sep 12, 2024, at 5:40 AM, John Baldwin wrote:
On 9/12/24 05:03, John B= aldwin wrote:
> I think part of the motivation for mark= ing M_EXTPG as read-only is that you can't "write"
> to= m_data via mtod() or the like.  That said, M_EXTPG aren't really r= ead-only.  It
> depends on the backing store. = ; M_EXTPG were first merged into FreeBSD prior to KTLS to
= > support sendfile, and in that case, they should be M_RDONLY because= they alias pages
> from the file's VM object.  Ho= wever, M_EXTPG mbufs allocated via functions like
> m_u= iotombuf_nomap should not be M_RDONLY.  I think this originated in = the original
> import of KTLS which doesn't push settin= g M_RDONLY out to the callers of mb_alloc_extpgs,
> and= a few other places that hardcode M_RDONLY with M_EXTPG (_mb_unmapped_to= _ext should
> preserve M_RDONLY from the original mbuf = instead of forcing M_RDONLY).
> 
>= ; I can take a stab at a patch but won't have time to really test it unt= il after Euro.

Patch available below. = Compile tested but not run-tested:


> On 9/= 11/24 16:56, Drew Gallatin wrote:
>> M_EXTPGS mbufs = are marked read-only because they refer to external data.   Th= e original crypto code, (before kTLS was converted to OCF), used to just= build an iovec using PHYS_TO_DMAP() on the page array.  I think th= is case was missed during the conversion to OCF.
>><= br>
>> I'm not sure what the best thing to do is, as the= y should be read only, except this one specific case.... I'd be tempted = to just nerf the KASSERT for EXTPGS.
>>
>> On Wed, Sep 11, 2024, at 11:02 AM, Kristof Provost wrote:
=
>>> On 11 Sep 2024, at 16:45, Mark Johnston wrote:
>>>> On Wed, Sep 11, 2024 at 11:18:26AM +0000, = Kristof Provost wrote:
>>>>> The branch mai= n has been updated by kp:
>>>>>
>>>>> URL: https://cgit.Fr= eeBSD.org/src/commit/?id=3Df08247fd888e6f7db0ecf2aaa39377144ac40b4c<= br>
>>>>>
>>>>> co= mmit f08247fd888e6f7db0ecf2aaa39377144ac40b4c
>>>= >> Author:     Kristof Provost <kp@FreeBSD.org>
>>>= >> AuthorDate: 2024-09-10 20:15:31 +0000
>>>= ;>> Commit:     Kristof Provost <kp@FreeBSD.org>
>>>= ;>> CommitDate: 2024-09-11 11:17:48 +0000
>>&g= t;>>
>>>>>    &nb= sp;  Assert that mbufs are writable if we write to them
>>>>>
>>>>>  &n= bsp;    m_copyback() modifies the mbuf, so it must be a w= ritable mbuf.
>>>>
>>>&= gt; This change still triggers a panic for me when running KTLS tests.&n= bsp; I
>>>> note that EXTPG mbufs always have = M_RDONLY set, but I'm not quite sure
>>>> why.=   I suspect such mbufs need special handling with respect to the ne= w
>>>> assertion.
>>>&g= t;
>>>> syzbot also triggered this panic:
<= /div>
>>>>
>>> Yeah, I saw that one before I went out for a bike ride.<= br>
>>>
>>> Clearly something = is wrong. Either ktls is using read-only buffers or the M_WRITABLE() mac= ro isn=E2=80=99t quite smart enough to spot this specific case.
>>>
>>> I=E2=80=99m not familiar e= nough with ktls to easily tell which.
>>>
>>> I=E2=80=99ll back this assertion change out for now,= so we=E2=80=99re not panicing test machines while we figure this out.
>>>
>>> Best regards,
>>> Kristof
>>>
&= gt;>
> 

-- 
John Baldwin



--16fb958509d44755bdf660d7a1b86bf9-- --f05b720e4072433382bbf437e9890aa6 Content-Disposition: attachment; filename="t.diff" Content-Type: application/octet-stream; name="t.diff" Content-Transfer-Encoding: base64 ZGlmZiAtLWdpdCBhL3N5cy9zeXMvbWJ1Zi5oIGIvc3lzL3N5cy9tYnVmLmgKaW5kZXggYWI0 OTRhNzY4MzNlLi4zMWE3NjcwNGFlYjMgMTAwNjQ0Ci0tLSBhL3N5cy9zeXMvbWJ1Zi5oCisr KyBiL3N5cy9zeXMvbWJ1Zi5oCkBAIC0xMTM0LDcgKzExMzQsNyBAQCBtX2V4dHJlZmNudChz dHJ1Y3QgbWJ1ZiAqbSkKICAqIGJlIGJvdGggdGhlIGxvY2FsIGRhdGEgcGF5bG9hZCwgb3Ig YW4gZXh0ZXJuYWwgYnVmZmVyIGFyZWEsIGRlcGVuZGluZyBvbgogICogd2hldGhlciBNX0VY VCBpcyBzZXQpLgogICovCi0jZGVmaW5lCU1fV1JJVEFCTEUobSkJKCgobSktPm1fZmxhZ3Mg JiAoTV9SRE9OTFkgfCBNX0VYVFBHKSkgPT0gMCAmJglcCisjZGVmaW5lCU1fV1JJVEFCTEUo bSkJKCgobSktPm1fZmxhZ3MgJiBNX1JET05MWSkgPT0gMCAmJglcCiAJCQkgKCEoKChtKS0+ bV9mbGFncyAmIE1fRVhUKSkgfHwJCQlcCiAJCQkgKG1fZXh0cmVmY250KG0pID09IDEpKSkK IAo= --f05b720e4072433382bbf437e9890aa6--