Page MenuHomeFreeBSD

pf: Handle unmapped mbufs when computing checksums
ClosedPublic

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

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Mon, Mar 22, 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.

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.

markj marked 2 inline comments as done.

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 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.Mon, Mar 22, 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.