Re: git: f08247fd888e - main - Assert that mbufs are writable if we write to them
Date: Wed, 25 Sep 2024 13:58:44 UTC
On 9/13/24 11:45, Drew Gallatin wrote: > I think you also need to remove M_EXTPG from M_WRITABLE(). Attached a trivial, untested patch. Yes, I came back to testing this yesterday and ran into that as well. However, as part of this I also tried to audit all the calls to M_WRITABLE and most of them are assuming they can use mtod() to get a pointer. I think what might be a better approach is to add a new M_WRITABLE_EXTPG() variant that doesn't check M_EXTPG and leave M_WRITABLE as-is. Places like m_copyback that are M_EXTPG aware would use the new macro. This still requires the patch to not set M_RDONLY in all M_EXTPG mbufs. The other thing we might want to do though is set M_RDONLY on encrypted data after KTLS has encrypted it as there is no good reason to modify encrypted data. > 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 >> >> > -- John Baldwin