pfctl: further improve rule load time with thousands of interfaces

[kp]

Summary

In expand_rule(), be more careful about calling if_indextoname() on an ifindex of zero, which happens when a rule does not specify an interface name as a source or destination specification. if_indextoname() will still call getifaddrs(), even when there is no interface to lookup. When loading the primary ruleset or an anchor, this results in two queries to the routetable sysctl and subsequent iteration over all interfaces, all of which happens twice per rule. This gets very expensive when there's hundreds or thousands of interfaces and many rules being loaded.

Note that although the PF parser seems to have blindly called if_indextoname() on a missing ifname for quite some time, there appears to be some regression over time in getifaddrs() or the underlying kernel mechanics that are beyond my comprehension at the moment. This used to be far less noticeable.

Test Plan

Configure a system with N interfaces and X filter rules. truss -d pfctl -f /etc/pf.conf. The special "net.routetable..." sysctl via getifaddrs() will be loaded something like (4 * N * X) times.

[ncrogers_gmail.com]

This change also removes an orphaned get_query_socket() and prioritizes loading the isgroup_map via ifa_load() before doing ifa_grouplookup().

[kp]

sbin/pfctl/parse.y

5304

Wouldn't it make more sense to do this:

diff --git a/lib/libc/net/if_indextoname.c b/lib/libc/net/if_indextoname.c
index 908e771c4a4..5a61ead2b8e 100644
--- a/lib/libc/net/if_indextoname.c
+++ b/lib/libc/net/if_indextoname.c
@@ -66,6 +66,9 @@ if_indextoname(unsigned int ifindex, char *ifname)
        struct ifaddrs *ifaddrs, *ifa;
        int error = 0;

+       if (ifindex == 0)
+               return(NULL);
+
        if (getifaddrs(&ifaddrs) < 0)
                return(NULL);   /* getifaddrs properly set errno */

We'd achieve the same, but help out every other use of if_indextoname() as well.

sbin/pfctl/pfctl_parser.c

1379

Good catch.

1442

This seems like it fixes an actual bug. I'd expect ifa_grouplookup() to return incorrect results if ifa_load() hasn't been called yet.

Did you run into that bug? If so, a reproduction scenario would be nice, because more tests, more better.

[ncrogers_gmail.com]

sbin/pfctl/parse.y

5304

That did make more sense to me as well, but I wasn’t sure what the convention is for ignoring a null argument in a shared lib, or if it should be up to the caller?

sbin/pfctl/pfctl_parser.c

1442

I didn’t run into a bug, just noticed that whilst tracking down the performance issue and thought it might be related. It ended up not making any difference in my case since I’m not using interface groups. But it does seem like an error.

[kp]

This was committed as three separate changes: r360231, r360097 and r360096.

Thanks for the patch!