Page MenuHomeFreeBSD

ipfw: Support radix tables and table lookup for MAC addresses
ClosedPublic

Authored by smalukav_gmail.com on Apr 30 2022, 5:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 1, 9:31 PM
Unknown Object (File)
Wed, Feb 1, 8:47 PM
Unknown Object (File)
Sat, Jan 28, 1:23 PM
Unknown Object (File)
Dec 15 2022, 7:14 AM
Unknown Object (File)
Dec 14 2022, 3:55 AM
Unknown Object (File)
Dec 5 2022, 6:19 AM

Details

Summary

At the moment, there is only one way to check a packet's src or dst MAC addresses in ipfw - mac dst-mac src-mac command.
If you want to check a packet against many MAC addresses, you have to add a rule for every of them.
It can be quite slow, e.g. https://lists.freebsd.org/archives/freebsd-ipfw/2021-August/000078.html

By analogy with IP address matching, I've implemented a way to use ipfw radix tables for MAC matching.
I've added a new ipfw table with mac:radix type, and added src-mac and dst-mac lookup commands.

Usage example:

  1. ipfw table 1 create type mac
  2. ipfw table 1 add 11:22:33:44:55:66/48
  3. ipfw add skipto tablearg src-mac 'table(1)' or ipfw add deny src-mac 'table(1, 100)'. ipfw add deny lookup dst-mac 1 syntax is also supported.

Notice that you need to set sysctl net.link.ether.ipfw=1 to enable ipfw filtering on L2 level.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zlei retitled this revision from Support radix tables and table lookup for MAC addresses to ipfw: Support radix tables and table lookup for MAC addresses.May 5 2022, 1:54 AM

Thank you for the submission! That's indeed a really good & needed addition for ipfw tables!
Could you please upload a version w/context? (shouldn't be hard to install archanist).
Generally looks good to me, one question though - what was the driver for using radix? In my mental model there's nearly no room for the mac lookups other than direct match. Like, "mark all Apple devices"?

Please see some minor comments inline

sys/netpfil/ipfw/ip_fw2.c
2048

Looks like there's plenty of options here.
Probably worth rethinking this part of code & consider using switch() instead of many if's for each vidx?

sys/netpfil/ipfw/ip_fw_table_algo.c
383

Could you please explain why those renamings are required?

4039

Are there other key lengths? There's a plenty of other places (such as KEY_LEN_MAC) where the length is already hardcoded.
Also: if you want it configurable, probably worth just storing key length on per-table basis?

what was the driver for using radix?

Only because I thought radix was easier to implement as a POC. I don't know if any mask other than /48 is practical. Perhaps I should implement both radix and hash.

sys/netpfil/ipfw/ip_fw2.c
2048

Yeah, switch sounds good

sys/netpfil/ipfw/ip_fw_table_algo.c
383

addr_radix functions are used in addr:radix tables, mac_radix functions are used in mac:radix tables, just radix are used in both.

4039

There can be in the future, for example, for InfiniBand address. I used the approach from IPv4 and IPv6 addresses.
If you want to add InfiniBand address support, you should leave KEY_LEN_MAC as if and add KEY_LEN_IB = 20.

And enum and use switch for lookup instruction

smalukav_gmail.com marked an inline comment as done.

Remove duplicates

LGTM, added some minor comments.

sys/netinet/ip_fw.h
795

That's a public header, please add IPFW_ prefix to the name (IPFW_MAX_L2_ADDR_LEN or IPFW_MAXL2SIZE or similar)? Otherwise it may classify with other similar definitions in other headers/programs.

sys/netpfil/ipfw/ip_fw_table_algo.c
359

Worth naming the fields according to other SA-like structures (mac_len, mac_addr).
Also - I'd rather add an explicit spare field between len and addr, so we get 2-byte aligned address & 8 byte-total structure (for ethernet).
You can also store prefixlen in this field (as it's before the radix offset).

4025

Worth putting masklen into sa.

This revision is now accepted and ready to land.May 16 2022, 7:15 PM
This revision now requires review to proceed.May 17 2022, 5:48 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2022, 4:20 PM
This revision was automatically updated to reflect the committed changes.