ipfw: refactor macros around m_pullup()
ClosedPublic
Actions

Authored by glebius on Fri, Sep 4, 9:24 PM.

Details

Reviewers

netchild
gallatin
ae
lytboris_gmail.com

Group Reviewers

network

Commits

rG5f81426831a6: ipfw: refactor macros around m_pullup()

Summary

In the prologue, where we check if the argument is a memory or an mbuf
chain, do not set 'struct ip *ip' pointer. However, set the 'struct
ether_header *eh' pointer there and set Etherner header length in 'ehlen'.
Side effect of this refactor is that now Layer 2 hooks may send mbufs with
ETHERTYPE_VLAN frames. However, current network stack doesn't do that.

Write a new PULLUP() macro that would take type of the argument to
determine how much to pull. Unlike PULLUP_TO() this macro can take typed
pointer. This will allow to get rid of 'void *ulp' and bunch of casting
macros in the next change. Use local bool variable to see if we need to
unlock upon jump to pullup_failed. Embed pointer update into the branch
of the macro, where pointers indeed need an update.

Use new PULLUP() macro to pullup initial 'struct ip *ip' and 'struct
ip6_hdr *ip6'. This removes max_protohdr sized pullup, that previously
tried to pull more than an unmapped mbuf could yield, fixing a bug covered
by the testcase sys/netpfil/ipfw/unmapped.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76538
Build 73421: arc lint + arc unit

Event Timeline

glebius created this revision.Fri, Sep 4, 9:24 PM

Herald added subscribers: vegeta_tuxpowered.net, donner, melifaro, imp. · View Herald TranscriptFri, Sep 4, 9:24 PM

glebius requested review of this revision.Fri, Sep 4, 9:24 PM

Harbormaster completed remote builds in B76538: Diff 185872.Fri, Sep 4, 9:24 PM

glebius added a parent revision: D59390: ipfw tests: cover layer-2 filtering of unmapped mbufs.Fri, Sep 4, 9:24 PM

glebius mentioned this in D59389: ipfw: pull up only the headers ipfw_chk() inspects.Fri, Sep 4, 9:25 PM

Otherwise I like this.

sys/netpfil/ipfw/ip_fw2.c
1556	Is _x not really _end or _maxlen or something like that? Would read better?
1813	This is a mechanical change? PULLUP_TO?

glebius added a child revision: D59435: ipfw: use typed pointers to access network protocols headers where possible.Fri, Sep 4, 11:10 PM

glebius added inline comments.Fri, Sep 4, 11:14 PM

sys/netpfil/ipfw/ip_fw2.c
1556	Maybe `_max`?
1813	I have next change that changes this line to `PULLUP(sctp)`.

bz added inline comments.Sat, Sep 5, 1:21 AM

sys/netpfil/ipfw/ip_fw2.c
1556	Seem good! Thanks!
1813	ACK

glebius marked 2 inline comments as done.Sat, Sep 5, 6:13 PM

gallatin accepted this revision.Mon, Sep 7, 9:33 PM

gallatin added inline comments.

sys/netpfil/ipfw/ip_fw2.c
1548	I'd remove the word "global", as these variables are not really global; they just need to be in the current scope.
1553	I'd almost want to call this PULLUP_THRU. Eg, this is the first time i'm reading this, and I'd expect that PULLUP_TO(foo, struct tcphdr) would pull up to the START of the tcp header, not though the end of it. Yes, I realize its a per-existing name, but its confusing to me..

This revision is now accepted and ready to land.Mon, Sep 7, 9:33 PM

glebius added inline comments.Tue, Sep 8, 5:34 AM

sys/netpfil/ipfw/ip_fw2.c
1548	Right, will do.
1553	I agree with that. However, this macro has been here for a long time. I'm in favor of renaming it, but I don't want to slow down this review. If more people will say "do rename in the current review", I will do. If no, I will leave that for later. if that happens feel free to open a review that will just rename. I will vote "yes".