I think you also need to remove M_EXTPG from M_WRITABLE(). Attached a trivial, 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 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. > > Patch available below. Compile tested but not run-tested: > > https://github.com/freebsd/freebsd-src/compare/main...bsdjhb:freebsd:m_extpg_rdonly > > > 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 <k...@freebsd.org> > >>>>> AuthorDate: 2024-09-10 20:15:31 +0000 > >>>>> Commit: Kristof Provost <k...@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 > >
t.diff
Description: Binary data
