> On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> <alexandr.nedvedi...@oracle.com> wrote:
>
> Hello,
>
> patch below introduces struct refcnt to pfi_kif structure. Patch also changes
> pfi_kif_get() function to always return a reference to pfi_kif instance.
>
> Furthermore existing functions pfi_kif_ref()/pfi_kif_unref() are thrown away
> in favor of pfi_kif_take()/pfi_kif_rele(), which follow naming convention
> set by refcnt_init(9). Patch also removes kif reference types (enum
> pfi_kif_refs).
>
> Patch also updates if_pfsync.c file. I'm bit puzzled with test as follows
> in pfsync_in_clr():
>
> 770 for (i = 0; i < count; i++) {
> 771 clr = (struct pfsync_clr *)buf + len * i;
> 772 creatorid = clr->creatorid;
> 773
> 774 if (clr->ifname[0] == '\0') {
> ...
> 783 } else {
> 784 if (pfi_kif_get(clr->ifname) == NULL)
> 789 continue;
> 790
>
> I could not make any sense of line 784. Are we just making sure particular kif
> object exists for clr->ifname? Note pfi_kif_get() creates kif for us if it
> does not exist. If we want to query for kif existence only patch offers
> pfi_kif_check() function. I need some advice here if we should keep, what's
> currently in patch or switch to pfi_kif_check().
my name is probably against that code.
i didn't realise that pfi_kif_get creates a kif on demand. however, it also
uses malloc(, , M_NOWAIT) to try and allocate it, which can fail and cause
pfi_kif_get to return NULL.
>
> thanks and
> regards
> sasha
>
> ---------8<----------------8<----------------8<---------------8<--------
> Index: if_pfsync.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.220
> diff -u -p -r1.220 if_pfsync.c
> --- if_pfsync.c 11 Sep 2015 08:17:06 -0000 1.220
> +++ if_pfsync.c 11 Oct 2015 13:46:11 -0000
> @@ -631,6 +631,8 @@ pfsync_state_import(struct pfsync_state
> }
> CLR(st->state_flags, PFSTATE_ACK);
>
> + pfi_kif_rele(kif);
> +
> return (0);
>
> cleanup:
> @@ -650,6 +652,9 @@ pfsync_state_import(struct pfsync_state
> pool_put(&pf_state_scrub_pl, st->src.scrub);
> pool_put(&pf_state_pl, st);
> }
> +
> + pfi_kif_rele(kif);
> +
> return (error);
> }
>
> @@ -759,6 +764,7 @@ pfsync_in_clr(caddr_t buf, int len, int
> struct pf_state *st, *nexts;
> struct pf_state_key *sk, *nextsk;
> struct pf_state_item *si;
> + struct pfi_kif *kif;
> u_int32_t creatorid;
>
> for (i = 0; i < count; i++) {
> @@ -775,7 +781,11 @@ pfsync_in_clr(caddr_t buf, int len, int
> }
> }
> } else {
> - if (pfi_kif_get(clr->ifname) == NULL)
> + /*
> + * Do we realize here the pfi_kif_get() actually
> + * creates kif for name if it does not exist?
> + */
> + if ((kif = pfi_kif_get(clr->ifname)) == NULL)
> continue;
>
> /* XXX correct? */
> @@ -791,6 +801,8 @@ pfsync_in_clr(caddr_t buf, int len, int
> }
> }
> }
> +
> + pfi_kif_rele(kif);
> }
> }
>
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.946
> diff -u -p -r1.946 pf.c
> --- pf.c 8 Oct 2015 11:36:51 -0000 1.946
> +++ pf.c 11 Oct 2015 13:46:14 -0000
> @@ -944,7 +944,7 @@ pf_state_insert(struct pfi_kif *kif, str
> TAILQ_INSERT_TAIL(&state_list, s, entry_list);
> pf_status.fcounters[FCNT_STATE_INSERT]++;
> pf_status.states++;
> - pfi_kif_ref(kif, PFI_KIF_REF_STATE);
> + pfi_kif_take(kif);
> #if NPFSYNC > 0
> pfsync_insert_state(s);
> #endif /* NPFSYNC > 0 */
> @@ -1315,7 +1315,7 @@ pf_free_state(struct pf_state *cur)
> pool_put(&pf_rule_item_pl, ri);
> }
> pf_normalize_tcp_cleanup(cur);
> - pfi_kif_unref(cur->kif, PFI_KIF_REF_STATE);
> + pfi_kif_rele(cur->kif);
> TAILQ_REMOVE(&state_list, cur, entry_list);
> if (cur->tag)
> pf_tag_unref(cur->tag);
> Index: pf_if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_if.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 pf_if.c
> --- pf_if.c 4 Sep 2015 21:40:25 -0000 1.80
> +++ pf_if.c 11 Oct 2015 13:46:14 -0000
> @@ -107,7 +107,7 @@ pfi_kif_get(const char *kif_name)
> bzero(&s, sizeof(s));
> strlcpy(s.pfik_name, kif_name, sizeof(s.pfik_name));
> if ((kif = RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s)) != NULL)
> - return (kif);
> + return (pfi_kif_take(kif));
>
> /* create new one */
> if ((kif = malloc(sizeof(*kif), PFI_MTYPE, M_NOWAIT|M_ZERO)) == NULL)
> @@ -116,6 +116,7 @@ pfi_kif_get(const char *kif_name)
> strlcpy(kif->pfik_name, kif_name, sizeof(kif->pfik_name));
> kif->pfik_tzero = time_second;
> TAILQ_INIT(&kif->pfik_dynaddrs);
> + PF_REF_INIT(kif->pfik_refcnt);
>
> if (!strcmp(kif->pfik_name, "any")) {
> /* both so it works in the ioctl and the regular case */
> @@ -124,72 +125,21 @@ pfi_kif_get(const char *kif_name)
> }
>
> RB_INSERT(pfi_ifhead, &pfi_ifs, kif);
> + /*
> + * there is no pfi_kif_remove(), hence caller shares reference with
> + * pfi_ifs look up table.
> + */
> return (kif);
> }
>
> -void
> -pfi_kif_ref(struct pfi_kif *kif, enum pfi_kif_refs what)
> -{
> - switch (what) {
> - case PFI_KIF_REF_RULE:
> - kif->pfik_rules++;
> - break;
> - case PFI_KIF_REF_STATE:
> - kif->pfik_states++;
> - break;
> - case PFI_KIF_REF_ROUTE:
> - kif->pfik_routes++;
> - break;
> - default:
> - panic("pfi_kif_ref with unknown type");
> - }
> -}
> -
> -void
> -pfi_kif_unref(struct pfi_kif *kif, enum pfi_kif_refs what)
> +int
> +pfi_kif_check(const char *kif_name)
> {
> - if (kif == NULL)
> - return;
> -
> - switch (what) {
> - case PFI_KIF_REF_NONE:
> - break;
> - case PFI_KIF_REF_RULE:
> - if (kif->pfik_rules <= 0) {
> - DPFPRINTF(LOG_ERR,
> - "pfi_kif_unref: rules refcount <= 0");
> - return;
> - }
> - kif->pfik_rules--;
> - break;
> - case PFI_KIF_REF_STATE:
> - if (kif->pfik_states <= 0) {
> - DPFPRINTF(LOG_ERR,
> - "pfi_kif_unref: state refcount <= 0");
> - return;
> - }
> - kif->pfik_states--;
> - break;
> - case PFI_KIF_REF_ROUTE:
> - if (kif->pfik_routes <= 0) {
> - DPFPRINTF(LOG_ERR,
> - "pfi_kif_unref: state refcount <= 0");
> - return;
> - }
> - kif->pfik_routes--;
> - break;
> - default:
> - panic("pfi_kif_unref with unknown type");
> - }
> -
> - if (kif->pfik_ifp != NULL || kif->pfik_group != NULL || kif == pfi_all)
> - return;
> -
> - if (kif->pfik_rules || kif->pfik_states || kif->pfik_routes)
> - return;
> + struct pfi_kif_cmp s;
>
> - RB_REMOVE(pfi_ifhead, &pfi_ifs, kif);
> - free(kif, PFI_MTYPE, 0);
> + bzero(&s, sizeof(s));
> + strlcpy(s.pfik_name, kif_name, sizeof (s.pfik_name));
> + return (RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s));
> }
>
> int
> @@ -258,7 +208,7 @@ pfi_detach_ifnet(struct ifnet *ifp)
>
> kif->pfik_ifp = NULL;
> ifp->if_pf_kif = NULL;
> - pfi_kif_unref(kif, PFI_KIF_REF_NONE);
> + pfi_kif_rele(kif);
>
> splx(s);
> }
> @@ -275,6 +225,7 @@ pfi_attach_ifgroup(struct ifg_group *ifg
> if ((kif = pfi_kif_get(ifg->ifg_group)) == NULL)
> panic("pfi_kif_get failed");
>
> + KASSERT(kif->pfik_group == NULL);
> kif->pfik_group = ifg;
> ifg->ifg_pf_kif = (caddr_t)kif;
>
> @@ -295,7 +246,7 @@ pfi_detach_ifgroup(struct ifg_group *ifg
>
> kif->pfik_group = NULL;
> ifg->ifg_pf_kif = NULL;
> - pfi_kif_unref(kif, PFI_KIF_REF_NONE);
> + pfi_kif_rele(kif);
> splx(s);
> }
>
> @@ -312,6 +263,8 @@ pfi_group_change(const char *group)
>
> pfi_kif_update(kif);
>
> + pfi_kif_rele(kif);
> +
> splx(s);
> }
>
> @@ -371,7 +324,6 @@ pfi_dynaddr_setup(struct pf_addr_wrap *a
> rv = 1;
> goto _bad;
> }
> - pfi_kif_ref(dyn->pfid_kif, PFI_KIF_REF_RULE);
>
> dyn->pfid_net = pfi_unmask(&aw->v.a.mask);
> if (af == AF_INET && dyn->pfid_net == 32)
> @@ -414,7 +366,7 @@ _bad:
> if (ruleset != NULL)
> pf_remove_if_empty_ruleset(ruleset);
> if (dyn->pfid_kif != NULL)
> - pfi_kif_unref(dyn->pfid_kif, PFI_KIF_REF_RULE);
> + pfi_kif_rele(dyn->pfid_kif);
> pool_put(&pfi_addr_pl, dyn);
> splx(s);
> return (rv);
> @@ -595,7 +547,7 @@ pfi_dynaddr_remove(struct pf_addr_wrap *
>
> s = splsoftnet();
> TAILQ_REMOVE(&aw->p.dyn->pfid_kif->pfik_dynaddrs, aw->p.dyn, entry);
> - pfi_kif_unref(aw->p.dyn->pfid_kif, PFI_KIF_REF_RULE);
> + pfi_kif_rele(aw->p.dyn->pfid_kif);
> aw->p.dyn->pfid_kif = NULL;
> pfr_detach_table(aw->p.dyn->pfid_kt);
> aw->p.dyn->pfid_kt = NULL;
> @@ -709,14 +661,14 @@ pfi_get_ifaces(const char *name, struct
> if (*size > n++) {
> if (!p->pfik_tzero)
> p->pfik_tzero = time_second;
> - pfi_kif_ref(p, PFI_KIF_REF_RULE);
> + pfi_kif_take(p);
> if (copyout(p, buf++, sizeof(*buf))) {
> - pfi_kif_unref(p, PFI_KIF_REF_RULE);
> + pfi_kif_rele(p);
> splx(s);
> return (EFAULT);
> }
> nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
> - pfi_kif_unref(p, PFI_KIF_REF_RULE);
> + pfi_kif_rele(p);
> }
> }
> splx(s);
> @@ -810,3 +762,28 @@ pfi_unmask(void *addr)
> return (b);
> }
>
> +struct pfi_kif *
> +pfi_kif_take(struct pfi_kif *kif)
> +{
> + if (kif != pfi_all) {
> + PF_REF_TAKE(kif->pfik_refcnt);
> + }
> +
> + return (kif);
> +}
> +
> +void
> +pfi_kif_rele(struct pfi_kif *kif)
> +{
> + if (kif == pfi_all)
> + return;
> +
> + if (PF_REF_RELE(kif->pfik_refcnt)) {
> + KASSERT(kif->pfik_ifp == NULL);
> + KASSERT(kif->pfik_group == NULL);
> + KASSERT(TAILQ_EMPTY(&kif->pfik_dynaddrs));
> +
> + RB_REMOVE(pfi_ifhead, &pfi_ifs, kif);
> + free(kif, PFI_MTYPE, 0);
> + }
> +}
> Index: pf_ioctl.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.290
> diff -u -p -r1.290 pf_ioctl.c
> --- pf_ioctl.c 4 Sep 2015 21:40:25 -0000 1.290
> +++ pf_ioctl.c 11 Oct 2015 13:46:15 -0000
> @@ -289,11 +289,11 @@ pf_rm_rule(struct pf_rulequeue *rulequeu
> if (rule->overload_tbl)
> pfr_detach_table(rule->overload_tbl);
> }
> - pfi_kif_unref(rule->rcv_kif, PFI_KIF_REF_RULE);
> - pfi_kif_unref(rule->kif, PFI_KIF_REF_RULE);
> - pfi_kif_unref(rule->rdr.kif, PFI_KIF_REF_RULE);
> - pfi_kif_unref(rule->nat.kif, PFI_KIF_REF_RULE);
> - pfi_kif_unref(rule->route.kif, PFI_KIF_REF_RULE);
> + PFI_KIF_RELE(rule->rcv_kif);
> + PFI_KIF_RELE(rule->kif);
> + PFI_KIF_RELE(rule->rdr.kif);
> + PFI_KIF_RELE(rule->nat.kif);
> + PFI_KIF_RELE(rule->route.kif);
> pf_anchor_remove(rule);
> pool_put(&pf_rule_pl, rule);
> }
> @@ -529,7 +529,7 @@ pf_free_queues(struct pf_queuehead *wher
> if (ifp && q->kif->pfik_ifp != ifp)
> continue;
> TAILQ_REMOVE(where, q, entries);
> - pfi_kif_unref(q->kif, PFI_KIF_REF_RULE);
> + PFI_KIF_RELE(q->kif);
> pool_put(&pf_queue_pl, q);
> }
> return (0);
> @@ -788,8 +788,6 @@ pf_kif_setup(char *ifname, struct pfi_ki
> *kif = pfi_kif_get(ifname);
> if (*kif == NULL)
> return (EINVAL);
> -
> - pfi_kif_ref(*kif, PFI_KIF_REF_RULE);
> } else
> *kif = NULL;
>
> @@ -1035,7 +1033,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
> break;
> }
> /* XXX resolve bw percentage specs */
> - pfi_kif_ref(qs->kif, PFI_KIF_REF_RULE);
> if (qs->qlimit == 0)
> qs->qlimit = HFSC_DEFAULT_QLIMIT;
> TAILQ_INSERT_TAIL(pf_queues_inactive, qs, entries);
> Index: pf_table.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 pf_table.c
> --- pf_table.c 7 Oct 2015 11:57:44 -0000 1.115
> +++ pf_table.c 11 Oct 2015 13:46:17 -0000
> @@ -847,8 +847,6 @@ pfr_create_kentry(struct pfr_addr *ad)
> case PFRKE_ROUTE:
> if (ad->pfra_ifname[0])
> ke->pfrke_rkif = pfi_kif_get(ad->pfra_ifname);
> - if (ke->pfrke_rkif)
> - pfi_kif_ref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE);
> break;
> default:
> panic("unknown pfrke_type %d", ke->pfrke_type);
> @@ -893,8 +891,7 @@ pfr_destroy_kentry(struct pfr_kentry *ke
> if (ke->pfrke_counters)
> pool_put(&pfr_kcounters_pl, ke->pfrke_counters);
> if (ke->pfrke_type == PFRKE_COST || ke->pfrke_type == PFRKE_ROUTE)
> - pfi_kif_unref(((struct pfr_kentry_all *)ke)->pfrke_rkif,
> - PFI_KIF_REF_ROUTE);
> + pfi_kif_rele(((struct pfr_kentry_all *)ke)->pfrke_rkif);
> pool_put(&pfr_kentry_pl[ke->pfrke_type], ke);
> }
>
> Index: pfvar.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pfvar.h,v
> retrieving revision 1.420
> diff -u -p -r1.420 pfvar.h
> --- pfvar.h 19 Aug 2015 21:22:41 -0000 1.420
> +++ pfvar.h 11 Oct 2015 13:46:17 -0000
> @@ -38,6 +38,7 @@
> #include <sys/tree.h>
> #include <sys/rwlock.h>
> #include <sys/syslimits.h>
> +#include <sys/refcnt.h>
>
> #include <net/radix.h>
> #include <net/route.h>
> @@ -55,6 +56,8 @@ struct ip6_hdr;
> #endif
> #endif
>
> +typedef struct refcnt pf_refcnt_t;
> +
> enum { PF_INOUT, PF_IN, PF_OUT, PF_FWD };
> enum { PF_PASS, PF_DROP, PF_SCRUB, PF_NOSCRUB, PF_NAT, PF_NONAT,
> PF_BINAT, PF_NOBINAT, PF_RDR, PF_NORDR, PF_SYNPROXY_DROP, PF_DEFER,
> @@ -1125,17 +1128,8 @@ struct pfi_kif {
> void *pfik_ah_cookie;
> struct ifnet *pfik_ifp;
> struct ifg_group *pfik_group;
> - int pfik_states;
> - int pfik_rules;
> - int pfik_routes;
> TAILQ_HEAD(, pfi_dynaddr) pfik_dynaddrs;
> -};
> -
> -enum pfi_kif_refs {
> - PFI_KIF_REF_NONE,
> - PFI_KIF_REF_STATE,
> - PFI_KIF_REF_RULE,
> - PFI_KIF_REF_ROUTE
> + pf_refcnt_t pfik_refcnt;
> };
>
> #define PFI_IFLAG_SKIP 0x0100 /* skip filtering on interface
> */
> @@ -1811,8 +1805,9 @@ extern struct pfi_kif *pfi_all;
>
> void pfi_initialize(void);
> struct pfi_kif *pfi_kif_get(const char *);
> -void pfi_kif_ref(struct pfi_kif *, enum pfi_kif_refs);
> -void pfi_kif_unref(struct pfi_kif *, enum pfi_kif_refs);
> +struct pfi_kif *pfi_kif_take(struct pfi_kif *);
> +void pfi_kif_rele(struct pfi_kif *);
> +int pfi_kif_check(const char *);
> int pfi_kif_match(struct pfi_kif *, struct pfi_kif *);
> void pfi_attach_ifnet(struct ifnet *);
> void pfi_detach_ifnet(struct ifnet *);
> @@ -1909,5 +1904,16 @@ void pf_cksum(struct pf_pdesc *,
> stru
>
> #endif /* _KERNEL */
>
> +#define PF_REF_INIT(_x_) refcnt_init(&(_x_))
> +
> +#define PF_REF_TAKE(_x_) refcnt_take(&(_x_))
> +
> +#define PF_REF_RELE(_x_) refcnt_rele(&(_x_))
> +
> +#define PFI_KIF_TAKE(_kif_) \
> + (((_kif_) != NULL) ? pfi_kif_take((_kif_)) : NULL)
> +
> +#define PFI_KIF_RELE(_kif_) \
> + (((_kif_) != NULL) ? pfi_kif_rele((_kif_)) : ((void)(0)))
>
> #endif /* _NET_PFVAR_H_ */
>
it all looks good to me. ok.
dlg
