On 9/25/24 09:58, John Baldwin wrote:
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.
I've uploaded a series of reviews starting with
https://reviews.freebsd.org/D46783
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
--
John Baldwin
