Page MenuHomeFreeBSD

New netgraph node: macfilter
ClosedPublic

Authored by nick.hibma_gmail.com on Nov 18 2020, 2:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 11:00 AM
Unknown Object (File)
Feb 6 2024, 7:18 AM
Unknown Object (File)
Feb 6 2024, 3:51 AM
Unknown Object (File)
Feb 2 2024, 6:53 PM
Unknown Object (File)
Jan 31 2024, 6:23 AM
Unknown Object (File)
Dec 23 2023, 3:36 AM
Unknown Object (File)
Dec 9 2023, 8:07 PM
Unknown Object (File)
Nov 28 2023, 5:18 AM

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35118
Build 32079: arc lint + arc unit

Event Timeline

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

share/man/man4/ng_macfilter.4
51

Line break is needed here after the sentence stop.

62

Another line break needed here.

75

One more line break here at the sentence stop.

76

And another one here.

Remove unrelated change from this changeset.

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.)

sys/netgraph/ng_macfilter.h
6

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

20

I think this is a useless comment.

23

Ditto.

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

Comment and copyright fixes (afedorov).

sys/netgraph/ng_macfilter.c
64–67

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

72

Ditto.

89

These two variables are not used.

92

These define also unused.

sys/netgraph/ng_macfilter.h
89

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.

91

Same.

sys/netgraph/ng_macfilter.c
640

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

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

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

Nick Hibma
nick@van-laarhoven.org

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

Thanks. Removed all other checks as well as they are not being triggered either.
Nick Hibma
nick@van-laarhoven.org

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

Implement suggested changes:

  • make number of outbound hooks dynamic (afedorov).
  • remove unused includes, unused variables, redundant checks.
  • cleanup testing script a little
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.

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

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

Reviewers: lwhsu

share/man/man4/ng_macfilter.4
112

u_int32_t -> u_int64_t

114

Ditto.

sys/netgraph/ng_macfilter.h
93

ng_parse_uint32_type -> g_parse_uint64_type

95

Ditto.

nick.hibma_gmail.com marked 18 inline comments as done.

Final update incorporating all the changes.

Also, make buildworld and buildkernel tested.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 8 2020, 3:10 PM
This revision was automatically updated to reflect the committed changes.