PR: NPF Layer 2 filtering by Emmankoko · Pull Request #50 · NetBSD/src

Emmankoko · 2025-05-18T14:37:11Z

This is work covering the development of media access control filtering in NPF

has been tested on my netbsd using ssh connections and checking the statistics and everything works as expected.
work available and opened for review.

test to be written after completed review.

we ensure that layer 2 rules are inserted in layer 2 groups and layer 3 rules in layer 3 groups

here, rules are moved up to kernel and only l2 flagged bytcode are executed by layer2 handler and l3 flagged byte code are executed by l3 handler for l2, we use per interface filter hooks, we traverse all the interfaces on the system and hook our function with the ifnet pointer as key in the hook head we see a lot of duplications here: the decision to leave code in the different layer duplicated reduces a lot of branching and also personal decision to keep them as decoupled as possible.

and also add ether pass and ether block stats in npfctl stat

zoulasc · 2025-06-20T17:50:08Z

+	 * including the interface the frame is traveling on
+	 */
+	nbuf_init(npf, &nbuf, *mp, ifp);
+	memset(&npc, 0, sizeof(npf_cache_t));


sizeof(npc)

zoulasc · 2025-06-20T17:51:30Z

+
+	if (npf_ph_ether) {
+		error = pfil_add_hook(npfos_layer2_handler, npf,
+			PFIL_ALL, npf_ph_ether);


wrong indent

zoulasc · 2025-06-20T17:51:39Z

+register_etherpfil_hook(npf_t *npf, ifnet_t *ifp, int i)
+{
+	int error = 0;
+	static pfil_head_t *	npf_ph_ether;


wrong whitespace

still wrong whitespace

zoulasc · 2025-06-20T17:52:09Z

+destroy_pfilether_hook(npf_t *npf)
+{
+	int i = 0;
+	while(npf_ph_etherlist[i]) {


space after keyword

zoulasc · 2025-06-20T17:52:33Z

+{
+	int i = 0;
+	while(npf_ph_etherlist[i]) {
+		pfil_head_t *npf_ph_ether;


why not assign in the decl?

zoulasc · 2025-06-20T18:34:45Z

+		}
+
+	} else if (type && type != ctx->eth_type) {
+		errx(EXIT_FAILURE, "ether type mismatch");


print the types that did not match

zoulasc · 2025-06-20T18:36:02Z

+	const uint32_t *awords = (const uint32_t *)ether_addr;
+
+	off = (opts & MATCH_SRC) ? offsetof(struct ether_header, ether_shost) :
+					offsetof(struct ether_header, ether_dhost);


zoulasc · 2025-06-20T18:37:04Z

+{
+	unsigned off;
+	assert(((opts & MATCH_SRC) != 0) ^ ((opts & MATCH_DST) != 0));
+	const uint32_t *awords = (const uint32_t *)ether_addr;


How do you know that this is aligned properly?

zoulasc · 2025-06-20T18:37:58Z

+	bc = npfctl_bpf_create();
+	if (fopts->layer == NPF_RULE_LAYER_3) {
+		if (!build_l3_code(bc, rl, family, popts, fopts))
+		return false;


zoulasc · 2025-06-20T18:39:57Z

+static uint32_t
+npf_rule_layer_compat(nl_rule_t *cg, uint32_t layer)
+{
+	const char *str = (layer & NPF_RULE_LAYER_2) ? "layer 2" : "layer 3";


You are using 2 flags one for layer 2 and one for layer 3, then you test layer 2 here and in the code above you test layer 3 (with the else being the other one). Why do you need two flags? Are we planning to filter more layers in the future? And if we do, then if-then-else is not good enough.

zoulasc · 2025-06-20T18:40:06Z

+
+	if ((attr & layer) == 0) {
+		yyerror("cannot insert %s rules in this group"
+		" make sure to insert same layer rules in same group ", str);


zoulasc · 2025-06-20T18:41:07Z

 		memset(&imfopts, 0, sizeof(filt_opts_t));
-		memcpy(&imfopts.fo_from, ap1, sizeof(addr_port_t));
+		imfopts.layer = NPF_RULE_LAYER_3;
+		memcpy(&imfopts.filt.opt3.fo_from, ap1, sizeof(addr_port_t));


sizeof (infmopts....)

still applies

zoulasc · 2025-06-20T18:42:46Z

+
+	while (*cp) {
+		if (!isxdigit(*cp))
+			yyerror("%s", err);


Print the address that is bad too.

zoulasc · 2025-06-20T18:43:29Z

+			if (*cp == '\0')
+				return;
+			else
+				yyerror("%s", err);


be more specific in the error.

zoulasc · 2025-06-20T18:43:37Z

+		}
+
+		switch (*cp) {
+			case ':':


wrong indent.

zoulasc · 2025-06-20T18:44:23Z

+
+	_DIAGASSERT(type != NULL);
+
+	easprintf(&a, "%c%c%02x%02x", 'E', 'x', type[0], type[1]);


why %c%c instead of Ex

zoulasc · 2025-06-28T13:44:44Z

+void
+fetch_ether_type(npf_bpf_t *ctx, uint16_t type)
+{
+	if ((ctx->flags & FETCHED_L2) == 0 || (type && ctx->eth_type == 0)) {


early returns, reverse the if logic and return early so you can unindent the code.

zoulasc · 2025-06-28T13:46:55Z

+static nl_rule_t *
+set_defgroup(nl_rule_t *rl, nl_rule_t *def_group, int attr)
+{
+	const char *str = (attr & NPF_RULE_LAYER_3) ? "layer 3" : "layer 2";


not used by the other error so why always compute it?

also you have code figuring out the layer string again below, centralize it to a method.

zoulasc · 2025-06-28T13:48:02Z

+	if ((attr & layer) == 0) {
+		/* only set the layer strings when you need them */
+		const char *str;
+		if (layer & NPF_RULE_LAYER_2)


here is the other code.

zoulasc · 2025-06-28T13:48:28Z

 		memset(&imfopts, 0, sizeof(filt_opts_t));
-		memcpy(&imfopts.fo_from, ap1, sizeof(addr_port_t));
+		imfopts.layer = NPF_RULE_LAYER_3;
+		memcpy(&imfopts.filt.opt3.fo_from, ap1, sizeof(addr_port_t));


still applies

zoulasc · 2025-06-28T13:48:35Z

+
+filt_opts_t
+npfctl_parse_l3filt_opt(npfvar_t *src_addr, npfvar_t *src_port, bool tnot,
+			npfvar_t *dst_addr, npfvar_t *dst_port, bool fnot)


zoulasc · 2025-06-28T13:48:44Z

+
+filt_opts_t
+npfctl_parse_l2filt_opt(npfvar_t *src_addr, bool fnot, npfvar_t *dst_addr,
+			bool tnot, uint16_t eth_type)


zoulasc · 2025-06-28T13:50:59Z

+enum { /* book marks for L2 */
+	BM_ETHER_TYPE, BM_SRC_ETHER, BM_DST_ETHER, BM_SRC_ENEG, BM_DST_ENEG,
+
+	//BML2_COUNT // total number of l2 marks


what is this?

zoulasc · 2025-06-29T18:19:50Z

+
+	off = (opts & MATCH_SRC) ? offsetof(struct ether_header, ether_shost) :
+	    offsetof(struct ether_header, ether_dhost);
+	const uint32_t word_offset = sizeof(uint32_t);


forget about word_offset and use sizeof(mac_word) like we do in the memcpy case.

zoulasc · 2025-06-29T18:23:53Z

 static nl_rule_t *		the_rule = NULL;
 static bool			npf_conf_built = false;

+static bool			l2_group = false;;


I don't like this global, and why ;;?

zoulasc · 2025-06-29T18:24:05Z

 static bool			npf_conf_built = false;

+static bool			l2_group = false;;
 static nl_rule_t *		defgroup = NULL;


rename to defgroup_l3 for clarity?

sys/dev/usb/xhci.c: revision 1.192 Don't generate duplicate events when sending ZLP. Patch from sc_dying as posted to tech-kern. Addresses PR/59678.

Emmankoko added 7 commits May 18, 2025 14:33

accomodate space for layer 2 filtering options

687fbf5

adding ether variables now

3a963fe

set compatiblity layer between layer2 and layer3

e6d476d

we ensure that layer 2 rules are inserted in layer 2 groups and layer 3 rules in layer 3 groups

build layer 2 bpf code

b92b662

add extension for displaying layer 2 rules in npfctl show

6cb3194

and also add ether pass and ether block stats in npfctl stat

ensure all other npf facilities tests pass

b0d44a5

Emmankoko force-pushed the npflayer2 branch from abdea15 to b0d44a5 Compare May 20, 2025 13:49

testing for layer 2 rules

f424f0c

Emmankoko force-pushed the npflayer2 branch from ed1f35d to f424f0c Compare May 22, 2025 23:11