Re: git: f08247fd888e - main - Assert that mbufs are writable if we write to them

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 <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
> 
>