Hello,
On Fri, Nov 26, 2021 at 01:01:40PM +0100, Claudio Jeker wrote:
>
> One more thing to consider, I think the following test in pfi_set_flags():
>
> > + if ((p->pfik_flags_new != p->pfik_flags) &&
> > + (p->pfik_flagrefs == 0))
> > + pfi_kif_ref(p, PFI_KIF_REF_FLAG);
>
> should actually check for the PFI_IFLAG_SKIP flag and not any flag.
>
> if (ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
> p->pfik_flagrefs == 0)
> pfi_kif_ref(p, PFI_KIF_REF_FLAG);
>
> Same goes for pfi_clear_flags() just in reverse:
>
> > + if ((p->pfik_flags_new != p->pfik_flags) &&
> > + (p->pfik_flagrefs == 1))
> > + pfi_kif_unref(p, PFI_KIF_REF_FLAG);
>
> Should be changed to:
> if (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
> p->pfik_flagrefs == 1)
> pfi_kif_unref(p, PFI_KIF_REF_FLAG);
>
> We only want to track the PFI_IFLAG_SKIP flag but not any other flag like
> PFI_IFLAG_ANY. At least I think we want to do that, but then I guess
> pfi_set_flags() should only add a kif
> if (found == 0 && ISSET(flags, PFI_IFLAG_SKIP))
>
yes it makes sense.
> I don't really like pfi_set_flags() and pfi_clear_flags()
> and their ioctls DIOCSETIFFLAG and DIOCCLRIFFLAG. There are no checks for
> valid flag combinations. So anything goes for these functions.
> Also should the name check not happen in the ioctl handler and return
> EINVAL for bad input?
>
the thing is that empty string acts as a kind of wildcard pfi_skip_if()
matches anything if 'name' is NULL or empty string. So I'm keeping sanity
check in pfi_set_flags().
But it it still worth to add test for io == NULL to DIOCSETIFFLAG
and to DIOCCLRIFFLAG to avoid NULL pointer dereference (NULL->pfiio_name)
updated diff is below.
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..22d5937ec1d 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -57,6 +57,10 @@
#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))
+
struct pfi_kif *pfi_all = NULL;
struct pool pfi_addr_pl;
struct pfi_ifhead pfi_ifs;
@@ -75,6 +79,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 +192,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 +212,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 +221,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 +230,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 +239,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);
@@ -353,6 +374,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 +395,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 +820,64 @@ 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 (EINVAL);
+
+ 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) ||
+ (p->pfik_flagrefs == 1));
+
+ if (ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
+ (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 (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
+ (p->pfik_flagrefs == 1))
+ pfi_kif_unref(p, PFI_KIF_REF_FLAG);
}
+
return (0);
}
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..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);
