Hello,
</snip>
On Fri, Dec 03, 2021 at 03:42:09PM +0100, Claudio Jeker wrote:
>
> See comments below.
>
>
> > +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);
> > +}
> > +
>
> This function is not quite right. For example:
>
> ifconfig vio0 group foo group bar
> pfctl -f - <<EOF
> set skip on { foo bar }
> block return
> EOF
> ping -qc2 100.64.1.2
>
> PING 100.64.1.2 (100.64.1.2): 56 data bytes
>
> --- 100.64.1.2 ping statistics ---
> 2 packets transmitted, 2 packets received, 0.0% packet loss
> round-trip min/avg/max/std-dev = 0.153/0.178/0.202/0.024 ms
>
>
> Now lets remove just group bar from the interface.
>
> ifconfig vio0 -group bar
> ping -qc2 100.64.1.2
>
> PING 100.64.1.2 (100.64.1.2): 56 data bytes
> ping: sendmsg: Permission denied
> ping: wrote 100.64.1.2 64 chars, ret=-1
> ping: sendmsg: Permission denied
> ping: wrote 100.64.1.2 64 chars, ret=-1
>
> pfi_group_delmember() does not take into consideration other groups
> (including the interface itself) that may still allow PFI_IFLAG_SKIP.
> It just clears the flag.
>
> I think on delete the flag needs to be recalculated after the group has
> been removed. Not sure if this is easy possible though.
>
yes, you are absolutely right. the current logic around pfi_skip_if() is
bit convoluted. in order to fix it I must rework pfi_xcommit() and related
set_flags()/clear_flags() functions. The idea is to let pfi_xcommit() to
combine skip flags from all groups, which are attached to interface.
updated diff is attached.
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..d0ec56b5329 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -2724,7 +2724,7 @@ if_addgroup(struct ifnet *ifp, const char *groupname)
TAILQ_INSERT_TAIL(&ifp->if_groups, ifgl, ifgl_next);
#if NPF > 0
- pfi_group_addmember(groupname, ifp);
+ pfi_group_addmember(groupname);
#endif
return (0);
@@ -2757,7 +2757,7 @@ if_delgroup(struct ifnet *ifp, const char *groupname)
}
#if NPF > 0
- pfi_group_change(groupname);
+ pfi_group_delmember(groupname);
#endif
KASSERT(ifgl->ifgl_group->ifg_refcnt != 0);
diff --git a/sys/net/pf_if.c b/sys/net/pf_if.c
index 8de37375ab4..20dee8f70d0 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -57,6 +57,11 @@
#include <netinet/ip6.h>
#endif /* INET6 */
+#define isupper(c) ((c) >= 'A' && (c) <= 'Z')
+#define islower(c) ((c) >= 'a' && (c) <= 'z')
+#define isalpha(c) (isupper(c)||islower(c))
+#define isdigit(c) ((c) >= '0' && (c) <= '9')
+
struct pfi_kif *pfi_all = NULL;
struct pool pfi_addr_pl;
struct pfi_ifhead pfi_ifs;
@@ -75,6 +80,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 +193,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 +213,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 +222,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 +231,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 +240,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)
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);
@@ -354,16 +376,17 @@ pfi_group_change(const char *group)
}
void
-pfi_group_addmember(const char *group, struct ifnet *ifp)
+pfi_group_delmember(const char *group)
{
- 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 |= gkif->pfik_flags;
+ pfi_group_change(group);
+ pfi_xcommit();
+}
+void
+pfi_group_addmember(const char *group)
+{
pfi_group_change(group);
+ pfi_xcommit();
}
int
@@ -786,35 +809,103 @@ int
pfi_set_flags(const char *name, int flags)
{
struct pfi_kif *p;
+ 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;
+ if (name != NULL && name[0] != '\0') {
+ p = pfi_kif_find(name);
+ if (p == NULL) {
+ n = strlen(name);
+ if (n < 1 || n >= IFNAMSIZ)
+ return (EINVAL);
+
+ if (!isalpha(name[0]))
+ return (EINVAL);
+
+ 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);
+ if (ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP))
+ pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+ } else
+ panic("%s pfi_kif_get() returned NULL\n",
+ __func__);
+ } else
+ p->pfik_flags_new = p->pfik_flags | flags;
+ } else {
+ RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
+ p->pfik_flags_new = p->pfik_flags | flags;
}
+
return (0);
}
int
pfi_clear_flags(const char *name, int flags)
{
- struct pfi_kif *p;
+ struct pfi_kif *p, *w;
+
+ if (name != NULL && name[0] != '\0') {
+ p = pfi_kif_find(name);
+ if (p != NULL) {
+ p->pfik_flags_new = p->pfik_flags & ~flags;
+
+ KASSERT((p->pfik_flagrefs == 0) ||
+ (p->pfik_flagrefs == 1));
+
+ if (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
+ (p->pfik_flagrefs == 1))
+ pfi_kif_unref(p, PFI_KIF_REF_FLAG);
+ } else
+ return (ESRCH);
+
+ } else
+ RB_FOREACH_SAFE(p, pfi_ifhead, &pfi_ifs, w) {
+ p->pfik_flags_new = p->pfik_flags & ~flags;
+
+ KASSERT((p->pfik_flagrefs == 0) ||
+ (p->pfik_flagrefs == 1));
+
+ if (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
+ (p->pfik_flagrefs == 1))
+ pfi_kif_unref(p, PFI_KIF_REF_FLAG);
+ }
- RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
- if (pfi_skip_if(name, p))
- continue;
- p->pfik_flags_new = p->pfik_flags & ~flags;
- }
return (0);
}
void
pfi_xcommit(void)
{
- struct pfi_kif *p;
+ struct pfi_kif *p, *gkif;
+ struct ifg_list *g;
+ struct ifnet *ifp;
+ size_t n;
- RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
+ RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
p->pfik_flags = p->pfik_flags_new;
+ n = strlen(p->pfik_name);
+ ifp = p->pfik_ifp;
+ /*
+ * if kif is backed by existing interface, then we must use
+ * skip flags found in groups. We use pfik_flags_new, otherwise
+ * we would need to do two RB_FOREACH() passes: the first to
+ * commit group changes the second to commit flag changes for
+ * interfaces.
+ */
+ if (isdigit(p->pfik_name[n - 1]) && ifp != NULL)
+ TAILQ_FOREACH(g, &ifp->if_groups, ifgl_next) {
+ gkif =
+ (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
+ KASSERT(gkif != NULL);
+ p->pfik_flags |= gkif->pfik_flags_new;
+ }
+ }
}
/* from pf_print_state.c */
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index d61face1558..93df571e41c 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -2892,6 +2892,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
case DIOCSETIFFLAG: {
struct pfioc_iface *io = (struct pfioc_iface *)addr;
+ if (io == NULL)
+ return (EINVAL);
+
NET_LOCK();
PF_LOCK();
error = pfi_set_flags(io->pfiio_name, io->pfiio_flags);
@@ -2903,6 +2906,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
case DIOCCLRIFFLAG: {
struct pfioc_iface *io = (struct pfioc_iface *)addr;
+ if (io == NULL)
+ return (EINVAL);
+
NET_LOCK();
PF_LOCK();
error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags);
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 514b9e156f3..fa7d3894c97 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_addmember(const char *);
+void pfi_group_delmember(const char *);
int pfi_match_addr(struct pfi_dynaddr *, struct pf_addr *,
sa_family_t);
int pfi_dynaddr_setup(struct pf_addr_wrap *, sa_family_t);
