Date: Fri, 13 Sep 2024 11:45:00 -0400 From: "Drew Gallatin" <gallatin@fastmail.com> To: "John Baldwin" <jhb@freebsd.org>, "Kristof Provost" <kp@freebsd.org>, "Mark Johnston" <markj@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: f08247fd888e - main - Assert that mbufs are writable if we write to them Message-ID: <abf56fab-d028-4aee-a75c-18b9999a0bd7@app.fastmail.com> In-Reply-To: <1f61b6de-0fe2-4343-b4ad-f0866785a4bc@FreeBSD.org> References: <202409111118.48BBIQ2h065089@gitrepo.freebsd.org> <ZuGtGHm-u4QiJGUz@nuc> <ED0A74EE-FB1B-44E6-8DEE-E34D22953825@FreeBSD.org> <a0eeed9d-d2ff-49c2-af3f-328e0c0c28b1@app.fastmail.com> <2b1955e2-fbf1-41cb-b256-a9a257b16a83@FreeBSD.org> <1f61b6de-0fe2-4343-b4ad-f0866785a4bc@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <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 > >>>>> > >>>>> 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 <!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso= Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>I think you als= o need to remove M_EXTPG from M_WRITABLE(). Attached a trivial, un= tested patch.<br></div><div><br></div><div>Drew<br></div><div><br></div>= <div>On Thu, Sep 12, 2024, at 5:40 AM, John Baldwin wrote:<br></div><blo= ckquote type=3D"cite" id=3D"qt" style=3D""><div>On 9/12/24 05:03, John B= aldwin wrote:<br></div><div>> I think part of the motivation for mark= ing M_EXTPG as read-only is that you can't "write"<br></div><div>> to= m_data via mtod() or the like. That said, M_EXTPG aren't really r= ead-only. It<br></div><div>> depends on the backing store. = ; M_EXTPG were first merged into FreeBSD prior to KTLS to<br></div><div>= > support sendfile, and in that case, they should be M_RDONLY because= they alias pages<br></div><div>> from the file's VM object. Ho= wever, M_EXTPG mbufs allocated via functions like<br></div><div>> m_u= iotombuf_nomap should not be M_RDONLY. I think this originated in = the original<br></div><div>> import of KTLS which doesn't push settin= g M_RDONLY out to the callers of mb_alloc_extpgs,<br></div><div>> and= a few other places that hardcode M_RDONLY with M_EXTPG (_mb_unmapped_to= _ext should<br></div><div>> preserve M_RDONLY from the original mbuf = instead of forcing M_RDONLY).<br></div><div>> <br></div><div>>= ; I can take a stab at a patch but won't have time to really test it unt= il after Euro.<br></div><div><br></div><div>Patch available below. = Compile tested but not run-tested:<br></div><div><br></div><div><a href= =3D"https://github.com/freebsd/freebsd-src/compare/main...bsdjhb:freebsd= :m_extpg_rdonly">https://github.com/freebsd/freebsd-src/compare/main...b= sdjhb:freebsd:m_extpg_rdonly</a><br></div><div><br></div><div>> On 9/= 11/24 16:56, Drew Gallatin wrote:<br></div><div>>> 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></div><div>>><= br></div><div>>> 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.<br></div><div>>><br></div><di= v>>> On Wed, Sep 11, 2024, at 11:02 AM, Kristof Provost wrote:<br>= </div><div>>>> On 11 Sep 2024, at 16:45, Mark Johnston wrote:<b= r></div><div>>>>> On Wed, Sep 11, 2024 at 11:18:26AM +0000, = Kristof Provost wrote:<br></div><div>>>>>> The branch mai= n has been updated by kp:<br></div><div>>>>>><br></div><d= iv>>>>>> URL: <a href=3D"https://cgit.FreeBSD.org/sr= c/commit/?id=3Df08247fd888e6f7db0ecf2aaa39377144ac40b4c">https://cgit.Fr= eeBSD.org/src/commit/?id=3Df08247fd888e6f7db0ecf2aaa39377144ac40b4c</a><= br></div><div>>>>>><br></div><div>>>>>> co= mmit f08247fd888e6f7db0ecf2aaa39377144ac40b4c<br></div><div>>>>= >> Author: Kristof Provost <<a href=3D"= mailto:kp@FreeBSD.org">kp@FreeBSD.org</a>><br></div><div>>>>= >> AuthorDate: 2024-09-10 20:15:31 +0000<br></div><div>>>>= ;>> Commit: Kristof Provost <<a href=3D= "mailto:kp@FreeBSD.org">kp@FreeBSD.org</a>><br></div><div>>>>= ;>> CommitDate: 2024-09-11 11:17:48 +0000<br></div><div>>>&g= t;>><br></div><div>>>>>> &nb= sp; Assert that mbufs are writable if we write to them<br></div><d= iv>>>>>><br></div><div>>>>>> &n= bsp; m_copyback() modifies the mbuf, so it must be a w= ritable mbuf.<br></div><div>>>>><br></div><div>>>>&= gt; This change still triggers a panic for me when running KTLS tests.&n= bsp; I<br></div><div>>>>> note that EXTPG mbufs always have = M_RDONLY set, but I'm not quite sure<br></div><div>>>>> why.= I suspect such mbufs need special handling with respect to the ne= w<br></div><div>>>>> assertion.<br></div><div>>>>&g= t;<br></div><div>>>>> syzbot also triggered this panic:<br><= /div><div>>>>> <a href=3D"https://syzkaller.appspot.com= /bug?extid=3D58c918369f9dc323409d">https://syzkaller.appspot.com/bug?ext= id=3D58c918369f9dc323409d</a><br></div><div>>>>><br></div><d= iv>>>> Yeah, I saw that one before I went out for a bike ride.<= br></div><div>>>><br></div><div>>>> 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.<br></div= ><div>>>><br></div><div>>>> I=E2=80=99m not familiar e= nough with ktls to easily tell which.<br></div><div>>>><br></di= v><div>>>> 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.<b= r></div><div>>>><br></div><div>>>> Best regards,<br></= div><div>>>> Kristof<br></div><div>>>><br></div><div>&= gt;><br></div><div>> <br></div><div><br></div><div>-- <b= r></div><div>John Baldwin<br></div><div><br></div><div><br></div></block= quote><div><br></div></body></html> --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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?abf56fab-d028-4aee-a75c-18b9999a0bd7>