Hello,
thank you for taking a look at my diff.
</snip>
> > }
> >
> > - 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
fixed, looks like unintended change
> > + 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.
sure.
>
> > + 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 */
>
it is just sanity check we deal with either interface or group,
so isalpha(name[0]) is exactly what I want. And thanks for tip
to steal it from libsa/stand.h. Perhaps one day some day there
will be ctype.h for for kernel as well.
the reason to add this extra check is that I want to be sure we eventually
don't create interface, which starts with space (white char).
Remember former code did not create interface (kif object), it just walked
through list of existing interfaces in table. Now the DIOCSETIFFLAG ioctl
needs to be more cautious.
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..383b8c38f6a 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 (0);
+
+ n = strlen(name);
+ if (n < 1 || n >= IFNAMSIZ)
+ return (0);
+
+ if (!isalpha(name[0]))
+ 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);
