pf: fix a crash on sendfile()

[takahiro.kurosawa_gmail.com]

Summary

The network layer must not pass unmapped (M_EXTPG) mbufs to if_output()
of network interfaces without IFCAP_MEXTPG. pf should convert these
mbufs by mb_unmapped_to_ext() for such interfaces but it didn't.
The problem had occurred on sendfile because sendfile system call
uses unmapped mbufs for the file data.

Reported by: feld

Test Plan

testcase included (tests/sys/netpfil/pf/unmapped_mbuf)

[kp]

Thanks!

That looks pretty good to me, but I'd like to give Gleb some time to look as well. He's a lot more familiar with unmapped buffers than I am.

sys/netpfil/pf/pf.c

11650

It's fairly trivial to iterate an mbuf chain and discover that none of it is M_EXT, so I wonder if it's worth tracking that we've done the conversion.
If we don't, at worst we walk the mbuf chain a second time and do no actual work.

11650

*M_EXTPG, rather.

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

if_epair also doesn't announce support for unmapped mbufs, although possibly it doesn't crash.
I wonder if we should just add an assertion to it to check (thus making it crash).
There's precedent for making if_epair fail in more cases (e.g. a9bfd080d09a915055af51103defb5c38b94a236 added a explicit MTU check) already, and that makes this test case simpler too.

[glebius]

I understand the change to pf_route(), but I do not understand the change to pf_test(). Why the second one is required? pf(4) definitely can test (read IP/TCP level headers) of mbuf chains that have M_EXTPG later in paylod.

tests/sys/netpfil/pf/Makefile

75–77

Note that we already have sendfile_helper.c in tests/sys/kern. Would be really good to use that one instead of adding a second one. Feel free to modify the existing one so that it fits both tests.

tests/sys/netpfil/pf/sendfile_helper.c

122–123 ↗

(On Diff #181250)

Why this sockopt is needed for the test?

[takahiro.kurosawa_gmail.com]

Address review comments

• reuse the existing sendfile_helper (D58040, D58041)
• remove the second mb_unmapped_to_ext() call in pf_test()

[takahiro.kurosawa_gmail.com]

In D58021#1330660, @glebius wrote:

I understand the change to pf_route(), but I do not understand the change to pf_test(). Why the second one is required? pf(4) definitely can test (read IP/TCP level headers) of mbuf chains that have M_EXTPG later in paylod.

The second mb_unmapped_to_ext() call is actually unneeded because the packet had been processed in pf_route() and dummynet re-inject it. Thanks for pointing out that.

The first mb_unmapped_to_ext() just before m_unshare() is required because m_unshare() crashes in memcpy() if unmapped mbuf is passed. The original report in freebsd-net shows a crash in the m_unshare() call of if_ovpn's ovpn_output().
It might be better also fix m_unshare() so that it allows unmapped mbufs. I will add another review if m_unshare() should be fixed too.

sys/netpfil/pf/pf.c

11650

I noticed that the second mb_unmapped_to_ext() call was unneeded. I've removed it.

tests/sys/netpfil/pf/Makefile

75–77

I have submitted the reviews (D58040, D58041) that reuse the existing sendfile_helper.c in sys/kern.

[takahiro.kurosawa_gmail.com]

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

I'm wondering if I should add an assert in if_epair.
Probably if_epair can process unmapped mbufs, so adding IFCAP_MEXTPG for if_epair would be better. It might improve the performance of sendfile for host-jail and jail-jail communication.

[takahiro.kurosawa_gmail.com]

tests/sys/netpfil/pf/sendfile_helper.c

122–123 ↗

(On Diff #181250)

It is not required. I missed to cleanup the test code. Thanks for pointing out.
The socket option is not added for reused version of sendfile_helper.

[kp]

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

That's a good point.

Happily I think there's a way to do both. We can default if_epair to announcing the MEXTP option, and only assert (if we see an EXTPG mbuf) if it's disabled. The tests can disable it because we don't care about performance there, but do want the verification that we don't offer MEXTPG mbufs to interfaces which don't announce the option.

I'll see if I can come up with a patch.

[kp]

sys/netpfil/pf/pf.c

11674

Let's M_ASSERTMAPPED(*m0); here.

[takahiro.kurosawa_gmail.com]

Add M_ASSERTMAPPED() in pf_test()

[takahiro.kurosawa_gmail.com]

sys/netpfil/pf/pf.c

11674

I've added the assertion. Thanks for the review!

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

It would be excellent to be able to use if_epair for the test instead of if_ovpn. I will change the test script if if_epair is available for the test.

[kp]

tests/sys/netpfil/pf/unmapped_mbuf.sh

136

sendfile_helper doesn't know about '-c', but even with that removed the arguments don't make sense to me. Is there meant to be a wrapper script that's not in this patch?

136

Ah. Yes, there's meant to be https://reviews.freebsd.org/D58041 , which I missed.

[kp]

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

See https://reviews.freebsd.org/D58054

[takahiro.kurosawa_gmail.com]

Use epair(4) with D58054 instead of ovpn(4) for the test script

[takahiro.kurosawa_gmail.com]

tests/sys/netpfil/pf/unmapped_mbuf.sh

53

I've updated the test with if_epair that includes your change. It did simplify the test very much and make it independent of external tools, thanks!

[kp]

I'll give Gleb a few more days to take another look and then I'll commit the entire series.
Thanks for the fix!