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 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)
Missing space over ^^^ here
> 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))
I would just use:
if (n < 1 || n >= IFNAMSIZ)
like in other places.
> + 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 */
> +
> + 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
>
--
:wq Claudio
