Page MenuHomeFreeBSD

PFIL_MEMPTR support for ipfw link level hook
ClosedPublic

Authored by glebius on Feb 25 2019, 9:46 PM.

Details

Reviewers
ae
Group Reviewers
network
Commits
rS345166: PFIL_MEMPTR for ipfw link level hook
Summary

This commit brings support PFIL_MEMPTR support to ipfw link level hook.

This hook is expected to be used on NIC bound pfil heads, which provide
memory pointers rather than mbufs. As for now pfil_fake_mbuf() is used
to affiliate that. Skipping pfil_fake_mbuf() and working with memory
pointer directly inside ipfw gives measureable performance boost.

The patch in this review is rather large, so I'd suggest to look
at history of this branch:

https://github.com/glebius/FreeBSD/commits/pfil

The most important commit is the topmost:

0512e8e3de490b3ab2497c46373acf723dd3f6b9

This is the most crucial code that I want to be reviewed.

The following commits are preparatory steps:

1ce76e69e0219c31b3deccd4b2f9751987917763
f3941316b74a275292d8125c77dd114449481d77
45ab1e7e3aaaae323864ab56e4bbae4d43e0e4d5
72029a8fbfce40159a7006ed10eddb4400230ef8
b24f3ee15e55643aa2c6871b450f468ec4766a7f
17692f9c2cf07275fd680bc67d87cd5b099b9236

Commit bfb6a4ff73f1b4dd99ca5165080a63cc6e558a44 is a bugfix and isn't
directly related to this review. Commit isn't directly related as well a014887dd7c60ec96956bcc5cb7fec95a5a6f621.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23016
Build 22088: arc lint + arc unit

Event Timeline

glebius created this revision.Feb 25 2019, 9:46 PM
ae added inline comments.Mar 11 2019, 9:50 AM
sys/dev/virtio/network/if_vtnet.c
1837

Is it safe to call vtnet_rxq_discard_buf() just after if_input()?

sys/netpfil/ipfw/ip_fw2.c
1396

Is it expected that ipfw_chk() will always get from pfil ethernet frame without VLAN tags?

1757

This probably can break something. mbuf can have initialized oif and m_pkthdr.rcvif in the same time, this happens when we are processing forwarded packet.

glebius marked an inline comment as done.Mar 11 2019, 9:58 PM
glebius added inline comments.
sys/dev/virtio/network/if_vtnet.c
1837

This part of patch has changed. I will update pull request soon.

sys/netpfil/ipfw/ip_fw2.c
1396

That's a great question! Is that responsibility of a driver or of a packet filter. We will ponder on that.

1757

Is it possible to construct a rule that would match on oif and iif at the same time?

rgrimes added inline comments.
sys/netpfil/ipfw/ip_fw2.c
1757

Not only possible, in use. You can make decisions about a packet exiting based on if it arrived via a specific if.
ipfw add 1234 allow ip from any to any in via em0 out via em1

glebius marked an inline comment as done.Mar 11 2019, 10:27 PM
glebius updated this revision to Diff 54945.

Rebase since some changes are already in head.

glebius marked an inline comment as not done.Mar 11 2019, 10:43 PM
glebius added inline comments.
sys/netpfil/ipfw/ip_fw2.c
1757

This particular syntax won't match anything, since "via" always uses either of two interfaces. Also, a rule having "in" and "out" at the same time shoudn't match as well.

However, this rule correctly matches on two interfaces indeed:

ipfw add allow ip from any to any recv em0 out xmit em1

"out" is optional here. So, something needs to be fixed with my patch.

glebius updated this revision to Diff 54946.Mar 11 2019, 10:48 PM

Optionally use m->m_pkthdr.rcvif for iif on output packets.

glebius updated this revision to Diff 54948.Mar 11 2019, 11:28 PM

Parse VLAN headers.

glebius updated this revision to Diff 54949.Mar 11 2019, 11:47 PM

Style - no functional change.

ae added inline comments.Mar 12 2019, 8:38 AM
sys/netpfil/ipfw/ip_fw2.c
1435

It seems you forgot about ETHER_VLAN_ENCAP_LEN :-)
I think you can use difference between eh and ip pointers.

glebius updated this revision to Diff 55001.Mar 12 2019, 8:28 PM
  • Use existing difference between ip and eh pointers to calculate
glebius marked an inline comment as done.Mar 12 2019, 8:29 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 14 2019, 10:52 PM
This revision was automatically updated to reflect the committed changes.