From: sashan Date: Sun, 26 Dec 2021 01:00:32 +0000 (+0000) Subject: make 'set skip on ...' in pf.conf dynamic X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=840840b0ba8d4c6274090cce6a5f80acf621ee83;p=openbsd make 'set skip on ...' in pf.conf dynamic This is an old issue in pf(4): whenever new interface appears in IP stack, we must reload pf.conf to apply 'set skip on ...' to newly plumbed network interfaces. Time has come to fix it. The idea is to also create pfi_kif for interfaces, which are referred by 'set skip on ...'. Such pfi_kif instances are created/destroyed by pfi_set_flags()/pfi_clear_flags(). claudio@ dragged my attention to this in Gouveia. Also his feedback helped me to put change into shape. OK claudio@ --- diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 index 9fbbd0ccf7a..4b997a9067a 100644 --- a/share/man/man5/pf.conf.5 +++ b/share/man/man5/pf.conf.5 @@ -1,4 +1,4 @@ -.\" $OpenBSD: pf.conf.5,v 1.589 2021/12/21 00:23:15 jmatthew Exp $ +.\" $OpenBSD: pf.conf.5,v 1.590 2021/12/26 01:00:32 sashan Exp $ .\" .\" Copyright (c) 2002, Daniel Hartmeier .\" Copyright (c) 2003 - 2013 Henning Brauer @@ -28,7 +28,7 @@ .\" ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE .\" POSSIBILITY OF SUCH DAMAGE. .\" -.Dd $Mdocdate: December 21 2021 $ +.Dd $Mdocdate: December 26 2021 $ .Dt PF.CONF 5 .Os .Sh NAME @@ -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..91bacd06a77 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if.c,v 1.644 2021/11/11 10:03:10 claudio Exp $ */ +/* $OpenBSD: if.c,v 1.645 2021/12/26 01:00:32 sashan Exp $ */ /* $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $ */ /* @@ -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 6a83e86583e..7d12cfe934d 100644 --- a/sys/net/pf_if.c +++ b/sys/net/pf_if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_if.c,v 1.102 2021/12/06 07:41:33 sashan Exp $ */ +/* $OpenBSD: pf_if.c,v 1.103 2021/12/26 01:00:32 sashan Exp $ */ /* * Copyright 2005 Henning Brauer @@ -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_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); @@ -108,7 +113,7 @@ pfi_kif_free(struct pfi_kif *kif) return; if (kif->pfik_rules || kif->pfik_states || kif->pfik_routes || - kif->pfik_srcnodes) + kif->pfik_srcnodes || kif->pfik_flagrefs) panic("kif is still alive"); free(kif, PFI_MTYPE, sizeof(*kif)); @@ -186,6 +191,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"); } @@ -203,7 +211,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--; @@ -211,7 +220,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--; @@ -219,7 +229,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--; @@ -227,20 +238,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,16 +374,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 @@ -785,35 +807,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 (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..9293253ea2a 100644 --- a/sys/net/pf_ioctl.c +++ b/sys/net/pf_ioctl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_ioctl.c,v 1.367 2021/11/16 20:51:30 sashan Exp $ */ +/* $OpenBSD: pf_ioctl.c,v 1.368 2021/12/26 01:00:32 sashan Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -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..bd7ec1d88e7 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -1,4 +1,4 @@ -/* $OpenBSD: pfvar.h,v 1.504 2021/11/16 20:51:31 sashan Exp $ */ +/* $OpenBSD: pfvar.h,v 1.505 2021/12/26 01:00:32 sashan Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -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);