Re: [External] : Re: make 'set skip on ...' dynamic

2021-12-25 Thread Alexandr Nedvedicky 
Hello,

> > updated diff is attached.
> 
> One comment below but this diff is OK claudio@
>  
> > thanks and
> > regards
> > sashan
> > 
> > 8<---8<---8<--8<
> >  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, _ifs)
> > +   RB_FOREACH(p, pfi_ifhead, _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)
> 
> No other code in pf_if.c is checking both pfik_name and ifp != NULL in
> similar situations.  I think you should skip the isdigit() check here.
> If there is a real interface connected to a pfi_kif than it should be updated.
> Lets not introduce more complexity here.
> 
> > +   TAILQ_FOREACH(g, >if_groups, ifgl_next) {
> > +   gkif =
> > +   (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
> > +   KASSERT(gkif != NULL);
> > +   p->pfik_flags |= gkif->pfik_flags_new;
> > +   }
> > +   }

yes, there is no reason to keep isdigit() check. if `p` happens to
represent interface group, then ifp is NULL (p->pfik_ifp == NULL
for interface groups).

thanks and
regards
sashan

Re: [External] : Re: make 'set skip on ...' dynamic

2021-12-22 Thread Claudio Jeker 
On Sat, Dec 04, 2021 at 07:01:23PM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> 
> 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 - < > 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.

One comment below but this diff is OK claudio@
 
> thanks and
> regards
> sashan
> 
> 8<---8<---8<--8<
>  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, _ifs)
> + RB_FOREACH(p, pfi_ifhead, _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)

No other code in pf_if.c is checking both pfik_name and ifp != NULL in
similar situations.  I think you should skip the isdigit() check here.
If there is a real interface connected to a pfi_kif than it should be updated.
Lets not introduce more complexity here.

> + TAILQ_FOREACH(g, >if_groups, ifgl_next) {
> + gkif =
> + (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
> + KASSERT(gkif != NULL);
> + p->pfik_flags |= gkif->pfik_flags_new;
> + }
> + }
>  }

-- 
:wq Claudio

Re: [External] : Re: make 'set skip on ...' dynamic

2021-12-04 Thread Alexandr Nedvedicky 
Hello,


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 - < 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(>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 
 #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_ifheadpfi_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 *);
+voidpfi_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;
}

Re: [External] : Re: make 'set skip on ...' dynamic

2021-12-03 Thread Claudio Jeker 
On Fri, Nov 26, 2021 at 11:37:37PM +0100, Alexandr Nedvedicky wrote:
> 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.

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 - < --- 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);

I would extend this to also make sure that io->pfiio_flags only sets
PFI_IFLAG_SKIP.
if (io == NULL || io->pfiio_flags & ~PFI_IFLAG_SKIP)
return (EINVAL);

Userland is not allowed to touch PFI_IFLAG_ANY (at least that is my
understanding of the code).

> +
>   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);

Same here.

> +
>   NET_LOCK();
>   PF_LOCK();
>   error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags);

-- 
:wq Claudio

Re: [External] : Re: make 'set skip on ...' dynamic

2021-11-26 Thread Alexandr Nedvedicky 
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 
 #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_ifheadpfi_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 *);
+voidpfi_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,
-

Re: [External] : Re: make 'set skip on ...' dynamic

2021-11-26 Thread Claudio Jeker 
On Thu, Nov 25, 2021 at 02:56:02PM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> thank you for taking a look at my diff.
> 
> 
> 
> > >   }
> > >  
> > > - 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.

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))

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?

 
> 8<---8<---8<--8<
> index 8de37375ab4..383b8c38f6a 100644
> --- a/sys/net/pf_if.c
> +++ b/sys/net/pf_if.c
> +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;
>

Re: [External] : Re: make 'set skip on ...' dynamic

2021-11-25 Thread Alexandr Nedvedicky 
Hello,

thank you for taking a look at my diff.



> > }
> >  
> > -   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 
 #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_ifheadpfi_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 *);
+voidpfi_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 @@

Re: make 'set skip on ...' dynamic

2021-11-25 Thread Claudio Jeker 
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

Re: make 'set skip on ...' dynamic

2021-11-18 Thread Alexandr Nedvedicky 
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

Re: make 'set skip on ...' dynamic

2021-11-16 Thread Alexandr Nedvedicky 
Hello,

I've sent older version of diff. The new version contains
a minor tweak to pfi_set_flags() to make it more defensive:

+   if (name[n-1] >= '0' && name[n-1] <= '9') {
+   p = pfi_kif_get(name, NULL);
+   if (p != NULL) {
+   p->pfik_flags_new = p->pfik_flags | flags;
+   pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+   if (p->pfik_flags_new == p->pfik_flags)
+   pfi_kif_unref(p, PFI_KIF_REF_FLAG);

older version did not dropped/destroyed pfi_kif obtained from pfi_kif_get()
when flags were actually same.

thanks and
regards
sashan

On Wed, Nov 17, 2021 at 03:07:04AM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> back in Gouveia claudio@ complained 'set skip on ...' requires reloading
> pf.conf, when new interface appears. Diff below should fix that.
> 
> the idea is to introduce yet another reference type (PFI_KIF_REF_FLAG) pfi_kif
> objects. PFI_KIF_REF_FLAG type keeps pfi_kif object in pf's interface table,
> when there is no other reference to interface/group than 'set skip ...'
> statement itself.  The PFI_KIF_REF_FLAG is grabbed/dropped if and only if the
> interface flag is being changed.
> 
> 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/pf_if.c b/sys/net/pf_if.c
index 8de37375ab4..d241717ee7e 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -187,6 +187,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");
}
@@ -233,15 +236,23 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what)
}
kif->pfik_srcnodes--;
break;
+   case PFI_KIF_REF_FLAG:
+   if (kif->pfik_flagrefs <= 0) {
+   DPFPRINTF(LOG_ERR,
+   "pfi_kif_unref: flags refcount <= 0");
+   return;
+   }
+   kif->pfik_flagrefs--;
+   break;
default:
panic("pfi_kif_unref with unknown type");
}
 
-   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, _ifs, kif);
@@ -786,24 +797,53 @@ int
 pfi_set_flags(const char *name, int flags)
 {
struct pfi_kif  *p;
+   size_t n;
 
-   RB_FOREACH(p, pfi_ifhead, _ifs) {
-   if (pfi_skip_if(name, p))
-   continue;
-   p->pfik_flags_new = p->pfik_flags | flags;
+   if (name == NULL)
+   return (0);
+
+   n = strlen(name);
+   if ((n < 1) || (n >= IFNAMSIZ))
+   return (0);
+
+   if (name[n-1] >= '0' && name[n-1] <= '9') {
+   p = pfi_kif_get(name, NULL);
+   if (p != NULL) {
+   p->pfik_flags_new = p->pfik_flags | flags;
+   pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+   if (p->pfik_flags_new == p->pfik_flags)
+   pfi_kif_unref(p, PFI_KIF_REF_FLAG);
+   }
+   } else {
+   p = pfi_kif_get(name, NULL);
+   if (p != NULL) {
+   p->pfik_flags_new = p->pfik_flags | flags;
+   pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+   if (p->pfik_flags_new == p->pfik_flags)
+   pfi_kif_unref(p, PFI_KIF_REF_FLAG);
+   }
+
+   RB_FOREACH(p, pfi_ifhead, _ifs) {
+   if (pfi_skip_if(name, p))
+   continue;
+   p->pfik_flags_new = p->pfik_flags | flags;
+   }
}
+
return (0);
 }
 
 int
 pfi_clear_flags(const char *name, int flags)
 {
-   struct

make 'set skip on ...' dynamic

2021-11-16 Thread Alexandr Nedvedicky 
Hello,

back in Gouveia claudio@ complained 'set skip on ...' requires reloading
pf.conf, when new interface appears. Diff below should fix that.

the idea is to introduce yet another reference type (PFI_KIF_REF_FLAG) pfi_kif
objects. PFI_KIF_REF_FLAG type keeps pfi_kif object in pf's interface table,
when there is no other reference to interface/group than 'set skip ...'
statement itself.  The PFI_KIF_REF_FLAG is grabbed/dropped if and only if the
interface flag is being changed.

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/pf_if.c b/sys/net/pf_if.c
index 8de37375ab4..e91ba692a57 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -187,6 +187,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");
}
@@ -233,15 +236,23 @@ pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what)
}
kif->pfik_srcnodes--;
break;
+   case PFI_KIF_REF_FLAG:
+   if (kif->pfik_flagrefs <= 0) {
+   DPFPRINTF(LOG_ERR,
+   "pfi_kif_unref: flags refcount <= 0");
+   return;
+   }
+   kif->pfik_flagrefs--;
+   break;
default:
panic("pfi_kif_unref with unknown type");
}
 
-   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, _ifs, kif);
@@ -786,24 +797,51 @@ int
 pfi_set_flags(const char *name, int flags)
 {
struct pfi_kif  *p;
+   size_t n;
 
-   RB_FOREACH(p, pfi_ifhead, _ifs) {
-   if (pfi_skip_if(name, p))
-   continue;
-   p->pfik_flags_new = p->pfik_flags | flags;
+   if (name == NULL)
+   return (0);
+
+   n = strlen(name);
+   if ((n < 1) || (n >= IFNAMSIZ))
+   return (0);
+
+   if (name[n-1] >= '0' && name[n-1] <= '9') {
+   p = pfi_kif_get(name, NULL);
+   if (p != NULL) {
+   p->pfik_flags_new = p->pfik_flags | flags;
+   if (p->pfik_flags_new != p->pfik_flags)
+   pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+   }
+   } else {
+   p = pfi_kif_get(name, NULL);
+   if (p != NULL) {
+   p->pfik_flags_new = p->pfik_flags | flags;
+   if (p->pfik_flags_new != p->pfik_flags)
+   pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+   }
+
+   RB_FOREACH(p, pfi_ifhead, _ifs) {
+   if (pfi_skip_if(name, p))
+   continue;
+   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;
 
-   RB_FOREACH(p, pfi_ifhead, _ifs) {
+   RB_FOREACH_SAFE(p, pfi_ifhead, _ifs, w) {
if (pfi_skip_if(name, p))
continue;
p->pfik_flags_new = p->pfik_flags & ~flags;
+   if (p->pfik_flags_new != p->pfik_flags)
+   pfi_kif_unref(p, PFI_KIF_REF_FLAG);
}
return (0);
 }
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 514b9e156f3..53ced8ab97d 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;
 };
 
@@