Date: Wed, 11 Sep 2024 16:56:17 -0400 From: "Drew Gallatin" <gallatin@fastmail.com> To: "Kristof Provost" <kp@freebsd.org>, "Mark Johnston" <markj@freebsd.org>, "John Baldwin" <jhb@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: <a0eeed9d-d2ff-49c2-af3f-328e0c0c28b1@app.fastmail.com> In-Reply-To: <ED0A74EE-FB1B-44E6-8DEE-E34D22953825@FreeBSD.org> References: <202409111118.48BBIQ2h065089@gitrepo.freebsd.org> <ZuGtGHm-u4QiJGUz@nuc> <ED0A74EE-FB1B-44E6-8DEE-E34D22953825@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--c716c615bd2f40de8ba52bcfbdeae1f0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable M_EXTPGS mbufs are marked read-only because they refer to external data.= The 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. I'm not sure what the best thing to do is, as they should be read only, = except this one specific case.... I'd be tempted to just nerf the KASSER= T 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=3Df08247fd888e6f7db0ec= f2aaa39377144ac40b4c > >> > >> 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 mbuf. > > > > 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 su= re > > 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. >=20 > 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 spe= cific case. >=20 > I=E2=80=99m not familiar enough with ktls to easily tell which. >=20 > 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. >=20 > Best regards, > Kristof >=20 --c716c615bd2f40de8ba52bcfbdeae1f0 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>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 they 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><div>On Wed, Sep 11, 2024, a= t 11:02 AM, Kristof Provost wrote:<br></div><blockquote type=3D"cite" id= =3D"qt" style=3D""><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 Prov= ost wrote:<br></div><div>>> The branch main has been updated by kp= :<br></div><div>>><br></div><div>>> URL: <a href=3D"htt= ps://cgit.FreeBSD.org/src/commit/?id=3Df08247fd888e6f7db0ecf2aaa39377144= ac40b4c">https://cgit.FreeBSD.org/src/commit/?id=3Df08247fd888e6f7db0ecf= 2aaa39377144ac40b4c</a><br></div><div>>><br></div><div>>> co= mmit f08247fd888e6f7db0ecf2aaa39377144ac40b4c<br></div><div>>> Aut= hor: Kristof Provost <<a href=3D"mailto:kp@Fr= eeBSD.org">kp@FreeBSD.org</a>><br></div><div>>> AuthorDate: 202= 4-09-10 20:15:31 +0000<br></div><div>>> Commit: &= nbsp; Kristof Provost <<a href=3D"mailto:kp@FreeBSD.org">kp@FreeBSD.o= rg</a>><br></div><div>>> CommitDate: 2024-09-11 11:17:48 +0000<= br></div><div>>><br></div><div>>> As= sert that mbufs are writable if we write to them<br></div><div>>><= br></div><div>>> m_copyback() modifies the= mbuf, so it must be a writable 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 mb= ufs need special handling with respect to the new<br></div><div>> ass= ertion.<br></div><div>><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><div>Yeah, I s= aw 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 o= r the M_WRITABLE() macro isn=E2=80=99t quite smart enough to spot this s= pecific case.<br></div><div><br></div><div>I=E2=80=99m not familiar enou= gh with ktls to easily tell which.<br></div><div><br></div><div>I=E2=80=99= ll back this assertion change out for now, so we=E2=80=99re not panicing= test machines while we figure this out.<br></div><div><br></div><div>Be= st regards,<br></div><div>Kristof<br></div><div><br></div></blockquote><= div><br></div></body></html> --c716c615bd2f40de8ba52bcfbdeae1f0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?a0eeed9d-d2ff-49c2-af3f-328e0c0c28b1>