Re: Dynamic & static address grouping in NPF

> On 31 Jul 2025, at 10:41 PM, Christoph Badura <[email protected]> wrote:
> 
> On Thu, Jul 24, 2025 at 09:25:30AM +0000, Emmanuel Nyarko wrote:
>> In an attempt to fix this PR reported by Josh Mayor: https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=59507
>> 
>> the crash ( from the PR )  was here    "$clientgroup={ $self, $otherip }”
>> $self =. Dynamic, $otherip = raw IP
>> 
>> and this is because he was listing a dynamic address first and a raw IP second on the same rule.
>> 
>> the basic issue I found was 
>> if you have a rule
>> pass from { ifaddrs(wm0), 192.164.64.7 } on the same rule,
>> 
>> an assertion failure.
>> 
>> /*
>>  * L3 block cannot be inserted in the middle of a group.
>>  * In fact, it never is.  Check and start the group after.
>>  */
>> if (ingroup) {
>> assert(ctx->nblocks == ctx->gblock);
>> npfctl_bpf_group_exit(ctx);
>> }
>> 
>> 
>> This is a comment rmind@ left in a function that fetches the address family.
>> 
>> Group here means : a group of addresses ( not the group keyword )
>> { 192.168.65.7, 192.168.64.8, 192.168.64.9 } => a group 
> 
> No, group here means a grouping on filter conditions as explained here:
> https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_bpf_comp.c?r=1.19#70
> 
>> What he means:  always load the address family first.
> 
> Sort of.  What this actually means is that he wants to load the address
> family *once* and before the first static address is filtered.

Then unless i change how groupings are handled in NPF. because 

Because it cramps all filtering elements into one group and then this intent of his is violated.
> 
>> and when you list a static address, NPF extracts the address family and uses first when evaluating a group.
>> 
>> (000) ld M[0]     // 4 bytes offset the packet header to get the version
>> (001) jeq #0x04 // the address family fetched on the rule ( version 4 here )
>> 
>> /// before you load and compare the addresses.
> 
> Yes.
> 
>> But NPF doesn’t extract and check the version to use in filtering when you enter a dynamic address,
>> 
>> when you put { Ifaddrs(wm0), 192.168.64.7 } in the same group, 
> 
> Pretty much.  That is because "ifaddrs(wm0)" is internally implemented as
> an IFADDR table and it does a BPF_COP call to do the table lookup.  And the
> table lookup doesn't care about the BPF code having loaded the address
> family into the BPF memory.
> 
>> now when it is now about to evaluate the static IP on rule
>> 
>> doesn’t allow fetching the address family because NPF thinks it should be the first thing checked.
> 
> Correct.
> 
>> So it tries to exit the group,
> 
> Where does it try to exit the group?  I don't see that.

> 
>> With this behaviour , NPF tries to tie each rule to one address family. 
> 
>> based on this comment by rmind, 
>>  /*
>> * L3 block cannot be inserted in the middle of a group.
>> * In fact, it never is.  Check and start the group after.
>> */
>> if this is a statement we agree with, then I make it a compile time error
>> when you try to insert a raw IP in the middle of a group that begins with
>> a dynamic address.
> 
> You misunderstand the statement.  It's also a bit misleading.  What it says
> is that the L3 block of statements should be the first thing done in such a
> group, if it is necessary.  And certainly it shouldn't be done multiple
> times in the group.

> But that comment was added in r1.1 of npf_bpf_comp.c and the filt_addr_list
> stuff was added in r1.55 of npf_parse.y, i.e. much, much later.  So this
> comment doesn't reflect current reality.
> 
>> you are free to list only dynamic addresses in one group.(that doesn’t
>> seg fault because no address family check )
> 
> That's also not accurate.

What do you mean here by “not accurate” ?

Pass from { ifaddrs(wm0), ifaddrs(lo0), ifaddrs(en0) }

this never seg faults. and if you check the BPF code, it doesn’t load
the address family from the memory store. it just loads the table id and 
does a BPF_COP call to do a table lookup.

Maybe I don't understand what a dynamic address on a firewall rule is
> 
> The following example asserts:
> ----------------------------------------------------------------------------
> group default {
>        pass from { ifaddrs(wm0), 192.164.64.7 } 
> }
> ----------------------------------------------------------------------------


But according to the NPF manual, 

ifaddrs ==> a dynamic list, which i believe to be an addresses whose value isn’t resolved
At the time the rules are compiled.

the statement: “only dynamic addresses in one group doesn’t hit a seg fault"
i meant 
Pass from { ifaddrs(wm0), ifaddrs(lo0), ifaddrs(en0) }
and this doesn’t segfault.

my statement said “only dynamic addresses “ and i’m confused why you are including 192.164.64.7
in your example. 
> 
> This is because, leaving out unimported details, it gets translated into the
> following group
> 
>    ifnet_table(wm0) or bpf_cidr(192.164.64.8)
> 
> and bpf_cidr() tries to add fetch_l3(AF_INET).  Which, btw doesn't do any harm.
> 
> But the following doesn't.
> 
> ----------------------------------------------------------------------------
> group default {
> pass from { 192.164.64.7, ifaddrs(wm0) } 
> }
> ----------------------------------------------------------------------------
> 
> Because it gets translated into:
> 
>    bpf_cidr(192.164.64.8/32) or ifnet_table(wm0)
> 
> Here bpf_cidr() can add the fetch_l3(AF_INET) but if we're dealing with a
> IPv6 address, the code generated by fetch_l3() will jump passt the
> ifnet_table(wm0) too, I think, which is wrong.

Alright noted. so you think we should separate cidr groupings and table groupings.
> 
> Please add test cases. ;-)
> 
> But that seems wrong to me.  It should get translated into
> 
>    ( bpf_cidr(192.164.64.8/32) ) or ifnet_table(wm0)
> 
> That is, the bpf_cidr() should be put into it's own group.
> 
> Consider this:
> 
> ----------------------------------------------------------------------------
> group default {
> pass from { 192.164.64.7, 2001:db8::1, ifaddrs(wm0),
> 3fff:1334::/64, 192.0.2.0/24, ifaddrs(wm1) } 
> }


> ----------------------------------------------------------------------------
> 
> This could be translated into
> 
>   ( bpf_cidr(192.164.64.7/32) or bpf_cidr(192.0.2.0/24) ) or
>   ( bpf_cidr(2001:db8::1/128) or bpf_cidr(3fff:1334::/64) or
>   ( ifnet_table(wm0) or ifnet_table(wm1) )
> 
> And then bpf_cidr() could add the fetch_l3() at the beginning of each group
> and it would work.
> 
> Note that the order of the terms doesn't matter.

so in conclusion you think NPF should allow different family addresses
on one rule and the order shouldn’t matter ?

>  Except for performance, of
> course.
> 
> --chris


Emmanuel