pf: Handle unmapped mbufs when computing checksums
ClosedPublic
Actions

Authored by markj on Mar 22 2021, 2:40 PM.

Details

Reviewers

Commits

Summary

PR: 254419

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

markj requested review of this revision.Mar 22 2021, 2:40 PM

I see a call in ipfw, I guess that needs to be updated too. Seems time to introduce a new subroutine to handle this.

kp added inline comments.Mar 22 2021, 2:55 PM

sys/netpfil/pf/pf.c
5585	Presumably we'd have to do this here as well.
5598	And here?
5764	And here?
5879	And here?
5886	And here?

sys/netpfil/pf/pf.c
5598	I believe this treatment isn't needed when we're only checksumming a protocol header.

sys/netpfil/pf/pf.c
5598	And here?
5598	No, since this is just the IP header. Unmapped mbufs are a problem for checksums only on payload data.
5764	Yes.

Handle more cases pointed out by Kristof.

sys/netpfil/pf/pf.c
5879	These seem a bit tricky. mb_unmapped_to_ext() frees the chain upon failure, but this function and its callers can't really do that. We are also assuming in general that the first mbuf in the chain is preserved, otherwise mb_unmapped_to_ext() would invalidate some pointers in its callers.

kp accepted this revision.Mar 22 2021, 3:57 PM

kp added inline comments.

sys/netpfil/pf/pf.c
5879	It's tempting to 'fix' this by calling mb_unmapped_to_ext() in pf_check_in()/pf_check_out(), so that we restore the assumption that it's safe to access packet data. Probably not the best idea though. Is there a straightforward way of asserting that an mbuf is or is not unmapped? Just adding an assertion here would at least make future debugging easier.

This revision is now accepted and ready to land.Mar 22 2021, 3:57 PM

sys/netpfil/pf/pf.c
5879	You have to check the entire chain to see if any mbufs in the chain have M_EXTPG set in m_flags. However, it might be nice to have some kind of M_ASSERTMAPPED(m) wrapper for that the way we have a M_ASSERTPKTHDR() macro.