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?
thanks and
regards
sashan
--------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)
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))
+ return (0);
+
+ if (((name[0] < 'a') || (name[0] > 'z')) ||
+ ((name[0] < 'A') && (name[0] > 'Z')))
+ return (0);
+
+ 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
