On Fri, Nov 19, 2021 at 12:59:38AM +0100, Alexandr Nedvedicky wrote: > Hello, > > it has turned out things are bit more complicated when it comes to interface > groups. diff below makes following scenario work for me. > > we start with etc/pf.conf as follows: > > # cat /etc/pf.conf > set skip on lo > set skip on test1 > set skip on test2 > set skip on agroup > block return > > Let's see what interfaces are known to pf(4) > > # pfctl -sI > agroup > all > carp > egress > enc > enc0 > lo > lo0 > pflog > pflog0 > test1 > test2 > vio0 > > Using the same rules on current we get different output: > > # pfctl -sI > all > carp > egress > enc > enc0 > lo > lo0 > pflog > pflog0 > vio0 > > as you can see test1, test2 and agroup are missing on current. > it's because the current does not create a pfi_kif entry for > interfaces/groups referred by 'set skip on' keyword in pf.conf. > > the rules found in /etc/pf.conf block all network traffic: > > # ping -c 1 192.168.2.1 > PING 192.168.2.1 (192.168.2.1): 56 data bytes > ping: sendmsg: Permission denied > ping: wrote 192.168.2.1 64 chars, ret=-1 > > --- 192.168.2.1 ping statistics --- > 1 packets transmitted, 0 packets received, 100.0% packet loss > > to demonstrate 'set skip on agroup' works I'll add vio0 interface into > agroup and repeat test: > > # ifconfig vio0 group agroup > # ping -c 1 192.168.2.1 > PING 192.168.2.1 (192.168.2.1): 56 data bytes > 64 bytes from 192.168.2.1: icmp_seq=0 ttl=254 time=1.058 ms > > --- 192.168.2.1 ping statistics --- > 1 packets transmitted, 1 packets received, 0.0% packet loss > round-trip min/avg/max/std-dev = 1.058/1.058/1.058/0.000 ms > > removing vio0 from agroup stops network again: > > # ifconfig vio0 -group agroup > # ping -c 1 192.168.2.1 > PING 192.168.2.1 (192.168.2.1): 56 data bytes > ping: sendmsg: Permission denied > ping: wrote 192.168.2.1 64 chars, ret=-1 > > --- 192.168.2.1 ping statistics --- > 1 packets transmitted, 0 packets received, 100.0% packet loss > > removing 'set skip on agroup' from /etc/pf.conf also removes > the matching pfi_kif object from interface table kept by pf(4): > > # cat /etc/pf.conf > set skip on lo > set skip on test1 > set skip on test2 > block return > # pfctl -f /etc/pf.conf > # pfctl -sI > all > carp > egress > enc > enc0 > lo > lo0 > pflog > pflog0 > test1 > test2 > vio0 > > > The main trick to get things to work is to let 'set skip on ...' create > entries > (pfi_kif objects)) in pf's table for interfaces and groups which are not known > to IP stack yet. The same thing happens when firewall rule refers interface, > which does not exist in system yet. Diff below modifies pfi_set_flags() > function to create pfi_kif object if it does not already. We also obtain > a reference PFI_KIF_REF_FLAG if and only if the flag is being changed. > > We treat a pfik_reflag reference counter as an indication whether the pfi_kif > object get created on behalf of 'set skip on ...' keyword, so we can call > pfi_kif_unref() in pfi_clear_flags() to destroy it. The pfik_reflag also > prevents other callers to pfi_kif_unref() from destroying pfi_kif, which > got created by pfi_set_flags(). > > Changes described so far are sufficient to get 'set skip on...' working for > regular interfaces. To make it working for interface group we need to > introduce > pfi_group_delmember() and slightly adjust pfi_group_addmember(). > Both functions update interface flag in the same fashion like pfi_set_flags() > and pfi_clear_flags(). The caller of > pfi_group_delmember()/pfi_group_addmember() > must call pfi_xcommit() to update interfaces. > > OK?
Some inline comments. > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 > index bff448aa8dc..4afe841651f 100644 > --- a/share/man/man5/pf.conf.5 > +++ b/share/man/man5/pf.conf.5 > @@ -1383,9 +1383,6 @@ Packets passing in or out on such interfaces are passed > as if pf was > disabled, i.e. pf does not process them in any way. > This can be useful on loopback and other virtual interfaces, when > packet filtering is not desired and can have unexpected effects. > -.Ar ifspec > -is only evaluated when the ruleset is loaded; interfaces created > -later will not be skipped. > PF filters traffic on all interfaces by default. > .It Ic set Cm state-defaults Ar state-option , ... > The > diff --git a/sys/net/if.c b/sys/net/if.c > index 2e9a968d7cc..a6d3cb4f4ac 100644 > --- a/sys/net/if.c > +++ b/sys/net/if.c > @@ -2725,6 +2725,7 @@ if_addgroup(struct ifnet *ifp, const char *groupname) > > #if NPF > 0 > pfi_group_addmember(groupname, ifp); > + pfi_xcommit(); > #endif > > return (0); > @@ -2757,7 +2758,7 @@ if_delgroup(struct ifnet *ifp, const char *groupname) > } > > #if NPF > 0 > - pfi_group_change(groupname); > + pfi_group_delmember(groupname, ifp); > #endif > > KASSERT(ifgl->ifgl_group->ifg_refcnt != 0); > @@ -2769,6 +2770,10 @@ if_delgroup(struct ifnet *ifp, const char *groupname) > free(ifgl->ifgl_group, M_TEMP, sizeof(*ifgl->ifgl_group)); > } > > +#if NPF > 0 > + pfi_xcommit(); > +#endif > + > free(ifgl, M_TEMP, sizeof(*ifgl)); > > return (0); > diff --git a/sys/net/pf_if.c b/sys/net/pf_if.c > index 8de37375ab4..4fed66d9bf3 100644 > --- a/sys/net/pf_if.c > +++ b/sys/net/pf_if.c > @@ -75,6 +75,7 @@ void pfi_address_add(struct sockaddr *, > sa_family_t, u_int8_t); > int pfi_if_compare(struct pfi_kif *, struct pfi_kif *); > int pfi_skip_if(const char *, struct pfi_kif *); > int pfi_unmask(void *); > +void pfi_group_change(const char *); > > RB_PROTOTYPE(pfi_ifhead, pfi_kif, pfik_tree, pfi_if_compare); > RB_GENERATE(pfi_ifhead, pfi_kif, pfik_tree, pfi_if_compare); > @@ -187,6 +188,9 @@ pfi_kif_ref(struct pfi_kif *kif, enum pfi_kif_refs what) > case PFI_KIF_REF_SRCNODE: > kif->pfik_srcnodes++; > break; > + case PFI_KIF_REF_FLAG: > + kif->pfik_flagrefs++; > + break; > default: > panic("pfi_kif_ref with unknown type"); > } > @@ -204,7 +208,8 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what) > case PFI_KIF_REF_RULE: > if (kif->pfik_rules <= 0) { > DPFPRINTF(LOG_ERR, > - "pfi_kif_unref: rules refcount <= 0"); > + "pfi_kif_unref (%s): rules refcount <= 0", > + kif->pfik_name); > return; > } > kif->pfik_rules--; > @@ -212,7 +217,8 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what) > case PFI_KIF_REF_STATE: > if (kif->pfik_states <= 0) { > DPFPRINTF(LOG_ERR, > - "pfi_kif_unref: state refcount <= 0"); > + "pfi_kif_unref (%s): state refcount <= 0", > + kif->pfik_name); > return; > } > kif->pfik_states--; > @@ -220,7 +226,8 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what) > case PFI_KIF_REF_ROUTE: > if (kif->pfik_routes <= 0) { > DPFPRINTF(LOG_ERR, > - "pfi_kif_unref: route refcount <= 0"); > + "pfi_kif_unref (%s): route refcount <= 0", > + kif->pfik_name); > return; > } > kif->pfik_routes--; > @@ -228,20 +235,30 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs > what) > case PFI_KIF_REF_SRCNODE: > if (kif->pfik_srcnodes <= 0) { > DPFPRINTF(LOG_ERR, > - "pfi_kif_unref: src-node refcount <= 0"); > + "pfi_kif_unref (%s): src-node refcount <= 0", > + kif->pfik_name); > return; > } > kif->pfik_srcnodes--; > break; > + case PFI_KIF_REF_FLAG: > + if (kif->pfik_flagrefs <= 0) { > + DPFPRINTF(LOG_ERR, > + "pfi_kif_unref (%s): flags refcount <= 0", > + kif->pfik_name); > + return; > + } > + kif->pfik_flagrefs--; > + break; > default: > - panic("pfi_kif_unref with unknown type"); > + panic("pfi_kif_unref (%s) with unknown type", kif->pfik_name); > } > > - if (kif->pfik_ifp != NULL || kif->pfik_group != NULL || kif == pfi_all) > + if (kif->pfik_ifp != NULL || kif->pfik_group != NULL ||kif == pfi_all) Missing space over ^^^ here > return; > > if (kif->pfik_rules || kif->pfik_states || kif->pfik_routes || > - kif->pfik_srcnodes) > + kif->pfik_srcnodes || kif->pfik_flagrefs) > return; > > RB_REMOVE(pfi_ifhead, &pfi_ifs, kif); > @@ -353,6 +370,19 @@ pfi_group_change(const char *group) > pfi_kif_update(kif); > } > > +void > +pfi_group_delmember(const char *group, struct ifnet *ifp) > +{ > + struct pfi_kif *gkif, *ikif; > + > + if ((gkif = pfi_kif_get(group, NULL)) == NULL || > + (ikif = pfi_kif_get(ifp->if_xname, NULL)) == NULL) > + panic("%s: pfi_kif_get failed", __func__); > + ikif->pfik_flags_new = ikif->pfik_flags & ~gkif->pfik_flags; > + > + pfi_group_change(group); > +} > + > void > pfi_group_addmember(const char *group, struct ifnet *ifp) > { > @@ -361,7 +391,7 @@ pfi_group_addmember(const char *group, struct ifnet *ifp) > if ((gkif = pfi_kif_get(group, NULL)) == NULL || > (ikif = pfi_kif_get(ifp->if_xname, NULL)) == NULL) > panic("%s: pfi_kif_get failed", __func__); > - ikif->pfik_flags |= gkif->pfik_flags; > + ikif->pfik_flags_new = ikif->pfik_flags | gkif->pfik_flags; > > pfi_group_change(group); > } > @@ -786,25 +816,65 @@ int > pfi_set_flags(const char *name, int flags) > { > struct pfi_kif *p; > + int found = 0; > + size_t n; > > RB_FOREACH(p, pfi_ifhead, &pfi_ifs) { > if (pfi_skip_if(name, p)) > continue; > p->pfik_flags_new = p->pfik_flags | flags; > + found = 1; > } > + > + if (found == 0) { > + if (name == NULL) > + return (0); > + > + n = strlen(name); > + if ((n < 1) || (n >= IFNAMSIZ)) I would just use: if (n < 1 || n >= IFNAMSIZ) like in other places. > + return (0); > + > + if (((name[0] < 'a') || (name[0] > 'z')) || > + ((name[0] < 'A') && (name[0] > 'Z'))) > + return (0); Not sure what you're after here. The logic of this construct is incorrect. I would steal the defines from libsa/stand.h and then us !isalpha: #define isupper(c) ((c) >= 'A' && (c) <= 'Z') #define islower(c) ((c) >= 'a' && (c) <= 'z') #define isalpha(c) (isupper(c)||islower(c)) if (!isalpha(name[0])) return (0); You can also expand the defines if you want. I feel the intention here is to check if this is a interface group. So maybe using the check in pfi_skip_if() would be better: if (name[n-1] >= '0' && name[n-1] <= '9') return (0); /* group names may not end in a digit */ > + > + p = pfi_kif_get(name, NULL); > + if (p != NULL) { > + p->pfik_flags_new = p->pfik_flags | flags; > + > + /* > + * We use pfik_flagrefs counter as an indication > + * whether the kif has been created on behalf of > + * 'pfi_set_flags()' or not. > + */ > + KASSERT((p->pfik_flagrefs == 0) || > + (p->pfik_flagrefs == 1)); > + > + if ((p->pfik_flags_new != p->pfik_flags) && > + (p->pfik_flagrefs == 0)) > + pfi_kif_ref(p, PFI_KIF_REF_FLAG); > + } > + } > + > return (0); > } > > int > pfi_clear_flags(const char *name, int flags) > { > - struct pfi_kif *p; > + struct pfi_kif *p, *w; > > - RB_FOREACH(p, pfi_ifhead, &pfi_ifs) { > + RB_FOREACH_SAFE(p, pfi_ifhead, &pfi_ifs, w) { > if (pfi_skip_if(name, p)) > continue; > + > + KASSERT((p->pfik_flagrefs == 0) || (p->pfik_flagrefs == 1)); > p->pfik_flags_new = p->pfik_flags & ~flags; > + if ((p->pfik_flags_new != p->pfik_flags) && > + (p->pfik_flagrefs == 1)) > + pfi_kif_unref(p, PFI_KIF_REF_FLAG); > } > + > return (0); > } > > diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h > index 514b9e156f3..61fb6b9436f 100644 > --- a/sys/net/pfvar.h > +++ b/sys/net/pfvar.h > @@ -1171,6 +1171,7 @@ struct pfi_kif { > int pfik_rules; > int pfik_routes; > int pfik_srcnodes; > + int pfik_flagrefs; > TAILQ_HEAD(, pfi_dynaddr) pfik_dynaddrs; > }; > > @@ -1179,7 +1180,8 @@ enum pfi_kif_refs { > PFI_KIF_REF_STATE, > PFI_KIF_REF_RULE, > PFI_KIF_REF_ROUTE, > - PFI_KIF_REF_SRCNODE > + PFI_KIF_REF_SRCNODE, > + PFI_KIF_REF_FLAG > }; > > #define PFI_IFLAG_SKIP 0x0100 /* skip filtering on interface > */ > @@ -1867,8 +1869,8 @@ void pfi_attach_ifnet(struct ifnet *); > void pfi_detach_ifnet(struct ifnet *); > void pfi_attach_ifgroup(struct ifg_group *); > void pfi_detach_ifgroup(struct ifg_group *); > -void pfi_group_change(const char *); > void pfi_group_addmember(const char *, struct ifnet *); > +void pfi_group_delmember(const char *, struct ifnet *); > int pfi_match_addr(struct pfi_dynaddr *, struct pf_addr *, > sa_family_t); > int pfi_dynaddr_setup(struct pf_addr_wrap *, sa_family_t); > > to > -- :wq Claudio