On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>
> > On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> > <[email protected]> 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).
> >
[snip]
> >
> > 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);
> > + }
No need for curly braces here --^
> > +
> > + 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.
>
Even the typedef? (-:
But seriously I hate the _x_ things that Sasha is introducing
as it's certainly not the style we're using and not the style
pfvar.h is using. There's a bit of a mix of _x and just x,
but I'd prefer the former, e.g. _k instead of _kif_. Double
parenthesis are also not needed, e.g. pfi_kif_take((_kif_))
should be pfi_kif_take(_k).
PFI_KIF_TAKE and PFI_KIF_RELE macros add another level of
complexity for almost zero gain. PFI_KIF_TAKE is not even
used. I'd say scrap them.
I would also say that the PF_REF_* should stay under _KERNEL
as they simply cannot be used in the userland and moved some-
where before "extern struct pf_status pf_status" (after the
prototypes chunk) or after the "extern struct pf_pool_limit"
right before the "#endif /* _KERNEL */".
