This makes sense. You just need an ifnet without NOMAP support. Are you seeing this over the loopback interface? I guess we don't support unmapped mbufs for receive, so the loopback needs to convert any unmapped mbufs to mapped when looping them through. Sort of sucks to have to burn another flag just for this. Or rather, this makes me think that we should just define an EXT_FLAG_ANON and eventually use it to replace EPG_FLAG_ANON.

Hmm, or maybe we really should just be using M_RDONLY for the non-ANON case? It almost makes better sense to invert the flag to be more like EXT_FLAG_NONANON as most external buffers are anon-backed, but then the flag probably just mirrors M_RDONLY.

I think adding EXT_FLAG_ANON is fine for now, especially for purposes of an EN. I do wonder if we could retire the ANON flags entirely in main though and instead move to setting M_RDONLY for the non-ANON case. That would also make the code of the original SA patch much simpler (just fail if any of the mbufs are read-only).

sys/kern/kern_mbuf.c
940	So this is used for the path that converts M_EXTPG to "plain" M_EXT via sf_bufs, e.g. when you don't have a NIC that supports NOMAP mbufs?

In D57557#1319182, @jhb wrote:

This makes sense. You just need an ifnet without NOMAP support. Are you seeing this over the loopback interface?

Yes, this is over loopback, I'm experimenting with writing some simple tests for NFS+TLS.

I did some more testing today and verified that NFS-over-TLS works ok with a NIC that supports MEXTPG, so it seems the regression only appears if you're using a non-EXTPG-capable ifnet.

I guess we don't support unmapped mbufs for receive, so the loopback needs to convert any unmapped mbufs to mapped when looping them through. Sort of sucks to have to burn another flag just for this. Or rather, this makes me think that we should just define an EXT_FLAG_ANON and eventually use it to replace EPG_FLAG_ANON.

I don't think there's much pressure on the EXT_FLAG_* space? Most of the bits are unused. Certainly it's better to avoid adding new flags if possible though.

Hmm, or maybe we really should just be using M_RDONLY for the non-ANON case? It almost makes better sense to invert the flag to be more like EXT_FLAG_NONANON as most external buffers are anon-backed, but then the flag probably just mirrors M_RDONLY.

I think adding EXT_FLAG_ANON is fine for now, especially for purposes of an EN. I do wonder if we could retire the ANON flags entirely in main though and instead move to setting M_RDONLY for the non-ANON case. That would also make the code of the original SA patch much simpler (just fail if any of the mbufs are read-only).

I will try it.

I do wonder at the motivation behind D46784 though. Is it just a seatbelt?

In D57557#1319363, @markj wrote:

I do wonder at the motivation behind D46784 though. Is it just a seatbelt?

The thought there is that once a TLS record has been software encrypted it isn't safe to modify the data as you would corrupt the record.

BTW, see the followon commit in that stack: https://reviews.freebsd.org/D46787 I never merged this, but in some ways this might be the real long term fix? That is, we could check for M_WRITABLE instead of the bespoke checks the SA patch added?

In D57557#1322156, @jhb wrote:

In D57557#1319363, @markj wrote:

I do wonder at the motivation behind D46784 though. Is it just a seatbelt?

The thought there is that once a TLS record has been software encrypted it isn't safe to modify the data as you would corrupt the record.

BTW, see the followon commit in that stack: https://reviews.freebsd.org/D46787 I never merged this, but in some ways this might be the real long term fix? That is, we could check for M_WRITABLE instead of the bespoke checks the SA patch added?

Actually go read https://reviews.freebsd.org/D46786, this is depressing as it sort of predicts the edge case. I don't actually know if this series would have mitigated the original bug, but it probably would have made the fix simpler. :(