New netgraph node: macfilter

[nick.hibma_gmail.com]

Summary

Macfilter to route packets through different hooks based on sender MAC address.

Test Plan

Test script included in /usr/src/tools/test/ng_macfilter. Also actual use in our Hotspot.

[bcr]

A few minor things in the man page. You can check it with "mandoc -Tlint" and textproc/igor.

share/man/man4/ng_macfilter.4

50 ↗

(On Diff #79702)

Line break is needed here after the sentence stop.

61 ↗

(On Diff #79702)

Another line break needed here.

74 ↗

(On Diff #79702)

One more line break here at the sentence stop.

75 ↗

(On Diff #79702)

And another one here.

[nick.hibma_gmail.com]

Fixed all mandoc lint errors (bcr)

[nick.hibma_gmail.com]

Remove unrelated change from this changeset.

[donner]

Thank you for the work. In principle the functionality can be emulated by ng_bpf as well, but this kind of node is easier to use.

I'm curios about two things:

• The only selecting criteria is the source mac, isn't it? Would it be difficult to match either by src or dst?
• There is a static array of NG_MACFILTER_UPPER_NUM entries to remember the hooks for the node. Usually such limitations cause trouble in the medium time frame, so I'd ask you to think about getting rid of this array and use the hook traversal functions from the netgraph framework instead.

For the fine details, I'll need some more time. But at the first glance it looks very good. (Especially the test case.)

[afedorov]

sys/netgraph/ng_macfilter.h

5 ↗

(On Diff #79715)

Hmm, there is no COPYRIGHT file. May you add standard BSD license here?

19 ↗

(On Diff #79715)

I think this is a useless comment.

22 ↗

(On Diff #79715)

Ditto.

[bcr]

OK, thanks for the manpage fixes. That part looks good now.

[nick.hibma_gmail.com]

Comment and copyright fixes (afedorov).

[afedorov]

sys/netgraph/ng_macfilter.c

63–66 ↗

(On Diff #79720)

These 'include's are unnecessary:
#include <net/if.h>
#include <net/if_var.h>
#include <net/if_dl.h>
#include <net/if_types.h>

71 ↗

(On Diff #79720)

Ditto.

88 ↗

(On Diff #79720)

These two variables are not used.

91 ↗

(On Diff #79720)

These define also unused.

[afedorov]

sys/netgraph/ng_macfilter.h

88 ↗

(On Diff #79720)

I see no reason why this field should be of type uint32_t. Today we have millions of packets per second, so it overflows in a few hours.

90 ↗

(On Diff #79720)

Same.

[afedorov]

sys/netgraph/ng_macfilter.c

639 ↗

(On Diff #79720)

It seems to me that check is not needed here. Because the netgraph(4) has already checked that there is no hook with the same name.
See https://github.com/freebsd/freebsd/blob/master/sys/netgraph/ng_base.c#L1074

[lwhsu]

It will be great if the test can be converted to a test case and put to src/tests/sys/netgraph/ .

[nick.hibma_gmail.com]

Who would be able to help me get that sorted out? The script as-is is starting to get convoluted.

Nick Hibma
[e-mail]

• Open Source: We stand on the shoulders of giants.

[nick.hibma_gmail.com]

Thanks. Removed all other checks as well as they are not being triggered either.
Nick Hibma
[e-mail]

• Open Source: We stand on the shoulders of giants.

[nick.hibma_gmail.com]

Implement suggested changes:

• make number of outbound hooks dynamic (afedorov).
• remove unused includes, unused variables, redundant checks.
• cleanup testing script a little

[nick.hibma_gmail.com]

In D27268#608971, @lutz_donnerhacke.de wrote:

Thank you for the work. In principle the functionality can be emulated by ng_bpf as well, but this kind of node is easier to use.

I'm curios about two things:

• The only selecting criteria is the source mac, isn't it? Would it be difficult to match either by src or dst?
• There is a static array of NG_MACFILTER_UPPER_NUM entries to remember the hooks for the node. Usually such limitations cause trouble in the medium time frame, so I'd ask you to think about getting rid of this array and use the hook traversal functions from the netgraph framework instead.

For the fine details, I'll need some more time. But at the first glance it looks very good. (Especially the test case.)

I had a look at ng_bpf, and it looks like it should be able to do it, but it's complicated. Also: I need to keep track of packets and bytes. ng_macfilter is specifically geared towards hotspot usage.

Selecting on destination MAC is not a problem per se: add a netgraph message to select either mode and add an if statement in macfilter_ether_input. What would be the use case? In many cases an ipfw firewall table would work there as far as I can see.

NG_MACFILTER_UPPER_NUM I replaced, and added a test adding 42 hooks and removing them in random order.

[lwhsu]

In D27268#609092, @nick.hibma_gmail.com wrote:

Who would be able to help me get that sorted out? The script as-is is starting to get convoluted.

I'm happy to, and please note that I don't mean this would be a blocking issue, although having the test connected to the test suite and be run periodically gives some advantages like ensure this work in the future.

While it may take me some time for understand the background and the code, there are also some other channels would help:

• The codes and author of src/tests/sys/net*
• freebsd-testing mailing list

[nick.hibma_gmail.com]

Change test to TAP and add to system tests (lwhsu).

Reviewers: lwhsu

[afedorov]

share/man/man4/ng_macfilter.4

111 ↗

(On Diff #79760)

u_int32_t -> u_int64_t

113 ↗

(On Diff #79760)

Ditto.

sys/netgraph/ng_macfilter.h

92 ↗

(On Diff #79760)

ng_parse_uint32_type -> g_parse_uint64_type

94 ↗

(On Diff #79760)

Ditto.

[nick.hibma_gmail.com]

Final update incorporating all the changes.

Also, make buildworld and buildkernel tested.