Date: Thu, 12 Sep 2024 10:03:05 +0100 From: John Baldwin <jhb@FreeBSD.org> To: Drew Gallatin <gallatin@fastmail.com>, 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: <2b1955e2-fbf1-41cb-b256-a9a257b16a83@FreeBSD.org> In-Reply-To: <a0eeed9d-d2ff-49c2-af3f-328e0c0c28b1@app.fastmail.com> References: <202409111118.48BBIQ2h065089@gitrepo.freebsd.org> <ZuGtGHm-u4QiJGUz@nuc> <ED0A74EE-FB1B-44E6-8DEE-E34D22953825@FreeBSD.org> <a0eeed9d-d2ff-49c2-af3f-328e0c0c28b1@app.fastmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I think part of the motivation for marking 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 read-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. However, M_EXTPG mbufs allocated via functions like m_uiotombuf_nomap should not be M_RDONLY. I think this originated in the original import of KTLS which doesn't push setting 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 until after Euro. 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), used to just build an iovec using PHYS_TO_DMAP() on the page array. I think 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 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 main has been updated by kp: >>>> >>>> URL: https://cgit.FreeBSD.org/src/commit/?id=f08247fd888e6f7db0ecf2aaa39377144ac40b4c >>>> >>>> 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 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=58c918369f9dc323409d >>> >> 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’t quite smart enough to spot this specific case. >> >> I’m not familiar enough with ktls to easily tell which. >> >> I’ll back this assertion change out for now, so we’re not panicing test machines while we figure this out. >> >> Best regards, >> Kristof >> > -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2b1955e2-fbf1-41cb-b256-a9a257b16a83>