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
