Re: preparing pfi_kif to MP world
Hello, On Fri, Oct 16, 2015 at 01:51:31PM +0200, Alexandr Nedvedicky wrote: > On Fri, Oct 16, 2015 at 01:41:50PM +0200, Mike Belopuhov wrote: > > On 16 October 2015 at 13:28, Alexandr Nedvedicky > > wrote: > > > > > > may be it's kind of bike shading... > > > How about make kifs to stick to convention we see for other objects > > > such as rulesets/anchors: > > > > > > pfi_kif_find() > > > pfi_kif_find_or_create() > > > > > > > Personally I don't like "_or_create" style of function naming and > > I would rather see those renamed to something else > > > just blink of idea before I'll fall asleep, so using email to remember it... we should eventually rename all pf_*_find_or_create() functions to pf_*_create() and that's it, no change to current behavior. regards sasha
Re: preparing pfi_kif to MP world
On 29/10/15(Thu) 13:32, Mike Belopuhov wrote:
> On Thu, Oct 29, 2015 at 11:58 +0100, Martin Pieuchot wrote:
> > On 29/10/15(Thu) 02:49, Mike Belopuhov wrote:
> > > On 28 October 2015 at 18:41, Alexandr Nedvedicky
> > > wrote:
> > > > Hello Mike,
> > > >
> > > > just a quick question:
> > > >
> > > > are you going to commit your pfi_kif_find() et. al.?
> > > > or more work is needed there?
> > > >
> > >
> > > I need OKs
> > > [...]
> > > > pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> > > > {
> > > > struct pfsync_clr *clr;
> > > > - int i;
> > > > -
> > > > struct pf_state *st, *nexts;
> > > > - struct pf_state_key *sk, *nextsk;
> > > > - struct pf_state_item *si;
> > > > + struct pfi_kif *kif = NULL;
> > > > u_int32_t creatorid;
> > > > + int i;
> > > >
> > > > for (i = 0; i < count; i++) {
> > > > clr = (struct pfsync_clr *)buf + len * i;
> > > > creatorid = clr->creatorid;
> > > > + if (strlen(clr->ifname) &&
> > > > + (kif = pfi_kif_find(clr->ifname)) == NULL)
> > > > + continue;
> > > > -[...]
> > > > + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st =
> > > > nexts) {
> > > > + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> > > > + if (st->creatorid == creatorid &&
> > > > + ((kif && st->kif == kif) || !kif)) {
> >
> > I find this check confusing.
> >
> > Since you're continuing when "kif == NULL" can't you change this check
> > into "st->kif == kif"?
> >
>
> No.
>
> > Or is it possible for strlen(clr->ifname) to return 0
>
> Yes.
>
> > in which case you
> > might end up comparing a different ``kif''?
> >
>
> To garbage even...
>
> ...and I need to move the "kif = NULL" after creatorid actually.
> It's rather unlikely to get a message with several entries having
> clr->ifname set, but the code must be correct nevertheless.
ok mpi@
>
> diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
> index 7d633db..5296642 100644
> --- sys/net/if_pfsync.c
> +++ sys/net/if_pfsync.c
> @@ -752,46 +752,29 @@ done:
>
> int
> pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> {
> struct pfsync_clr *clr;
> - int i;
> -
> struct pf_state *st, *nexts;
> - struct pf_state_key *sk, *nextsk;
> - struct pf_state_item *si;
> + struct pfi_kif *kif;
> u_int32_t creatorid;
> + int i;
>
> for (i = 0; i < count; i++) {
> clr = (struct pfsync_clr *)buf + len * i;
> + kif = NULL;
> creatorid = clr->creatorid;
> + if (strlen(clr->ifname) &&
> + (kif = pfi_kif_find(clr->ifname)) == NULL)
> + continue;
>
> - if (clr->ifname[0] == '\0') {
> - for (st = RB_MIN(pf_state_tree_id, &tree_id);
> - st; st = nexts) {
> - nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> - if (st->creatorid == creatorid) {
> - SET(st->state_flags, PFSTATE_NOSYNC);
> - pf_unlink_state(st);
> - }
> - }
> - } else {
> - if (pfi_kif_get(clr->ifname) == NULL)
> - continue;
> -
> - /* XXX correct? */
> - for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
> - sk; sk = nextsk) {
> - nextsk = RB_NEXT(pf_state_tree,
> - &pf_statetbl, sk);
> - TAILQ_FOREACH(si, &sk->states, entry) {
> - if (si->s->creatorid == creatorid) {
> - SET(si->s->state_flags,
> - PFSTATE_NOSYNC);
> - pf_unlink_state(si->s);
> - }
> - }
> + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts) {
> + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> + if (st->creatorid == creatorid &&
> + ((kif && st->kif == kif) || !kif)) {
> + SET(st->state_flags, PFSTATE_NOSYNC);
> + pf_unlink_state(st);
> }
> }
> }
>
> return (0);
> diff --git sys/net/pf_if.c sys/net/pf_if.c
> index caaf9f9..bf77184 100644
> --- sys/net/pf_if.c
> +++ sys/net/pf_if.c
> @@ -97,18 +97,25 @@ pfi_initialize(void)
> if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL)
> panic("pfi_kif_get for pfi_all failed");
> }
>
> struct pfi_kif *
> -pfi
Re: preparing pfi_kif to MP world
On Thu, Oct 29, 2015 at 11:58 +0100, Martin Pieuchot wrote:
> On 29/10/15(Thu) 02:49, Mike Belopuhov wrote:
> > On 28 October 2015 at 18:41, Alexandr Nedvedicky
> > wrote:
> > > Hello Mike,
> > >
> > > just a quick question:
> > >
> > > are you going to commit your pfi_kif_find() et. al.?
> > > or more work is needed there?
> > >
> >
> > I need OKs
> > [...]
> > > pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> > > {
> > > struct pfsync_clr *clr;
> > > - int i;
> > > -
> > > struct pf_state *st, *nexts;
> > > - struct pf_state_key *sk, *nextsk;
> > > - struct pf_state_item *si;
> > > + struct pfi_kif *kif = NULL;
> > > u_int32_t creatorid;
> > > + int i;
> > >
> > > for (i = 0; i < count; i++) {
> > > clr = (struct pfsync_clr *)buf + len * i;
> > > creatorid = clr->creatorid;
> > > + if (strlen(clr->ifname) &&
> > > + (kif = pfi_kif_find(clr->ifname)) == NULL)
> > > + continue;
> > > -[...]
> > > + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st =
> > > nexts) {
> > > + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> > > + if (st->creatorid == creatorid &&
> > > + ((kif && st->kif == kif) || !kif)) {
>
> I find this check confusing.
>
> Since you're continuing when "kif == NULL" can't you change this check
> into "st->kif == kif"?
>
No.
> Or is it possible for strlen(clr->ifname) to return 0
Yes.
> in which case you
> might end up comparing a different ``kif''?
>
To garbage even...
...and I need to move the "kif = NULL" after creatorid actually.
It's rather unlikely to get a message with several entries having
clr->ifname set, but the code must be correct nevertheless.
diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
index 7d633db..5296642 100644
--- sys/net/if_pfsync.c
+++ sys/net/if_pfsync.c
@@ -752,46 +752,29 @@ done:
int
pfsync_in_clr(caddr_t buf, int len, int count, int flags)
{
struct pfsync_clr *clr;
- int i;
-
struct pf_state *st, *nexts;
- struct pf_state_key *sk, *nextsk;
- struct pf_state_item *si;
+ struct pfi_kif *kif;
u_int32_t creatorid;
+ int i;
for (i = 0; i < count; i++) {
clr = (struct pfsync_clr *)buf + len * i;
+ kif = NULL;
creatorid = clr->creatorid;
+ if (strlen(clr->ifname) &&
+ (kif = pfi_kif_find(clr->ifname)) == NULL)
+ continue;
- if (clr->ifname[0] == '\0') {
- for (st = RB_MIN(pf_state_tree_id, &tree_id);
- st; st = nexts) {
- nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
- if (st->creatorid == creatorid) {
- SET(st->state_flags, PFSTATE_NOSYNC);
- pf_unlink_state(st);
- }
- }
- } else {
- if (pfi_kif_get(clr->ifname) == NULL)
- continue;
-
- /* XXX correct? */
- for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
- sk; sk = nextsk) {
- nextsk = RB_NEXT(pf_state_tree,
- &pf_statetbl, sk);
- TAILQ_FOREACH(si, &sk->states, entry) {
- if (si->s->creatorid == creatorid) {
- SET(si->s->state_flags,
- PFSTATE_NOSYNC);
- pf_unlink_state(si->s);
- }
- }
+ for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts) {
+ nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
+ if (st->creatorid == creatorid &&
+ ((kif && st->kif == kif) || !kif)) {
+ SET(st->state_flags, PFSTATE_NOSYNC);
+ pf_unlink_state(st);
}
}
}
return (0);
diff --git sys/net/pf_if.c sys/net/pf_if.c
index caaf9f9..bf77184 100644
--- sys/net/pf_if.c
+++ sys/net/pf_if.c
@@ -97,18 +97,25 @@ pfi_initialize(void)
if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL)
panic("pfi_kif_get for pfi_all failed");
}
struct pfi_kif *
-pfi_kif_get(const char *kif_name)
+pfi_kif_find(const char *kif_name)
{
- struct pfi_kif *kif;
struct pfi_kif_cmp s;
bzero(&s, sizeof(s));
strlcpy(s.pfik_name, kif_name, sizeof(s.
Re: preparing pfi_kif to MP world
On 29/10/15(Thu) 02:49, Mike Belopuhov wrote:
> On 28 October 2015 at 18:41, Alexandr Nedvedicky
> wrote:
> > Hello Mike,
> >
> > just a quick question:
> >
> > are you going to commit your pfi_kif_find() et. al.?
> > or more work is needed there?
> >
>
> I need OKs
> [...]
> > pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> > {
> > struct pfsync_clr *clr;
> > - int i;
> > -
> > struct pf_state *st, *nexts;
> > - struct pf_state_key *sk, *nextsk;
> > - struct pf_state_item *si;
> > + struct pfi_kif *kif = NULL;
> > u_int32_t creatorid;
> > + int i;
> >
> > for (i = 0; i < count; i++) {
> > clr = (struct pfsync_clr *)buf + len * i;
> > creatorid = clr->creatorid;
> > + if (strlen(clr->ifname) &&
> > + (kif = pfi_kif_find(clr->ifname)) == NULL)
> > + continue;
> > -[...]
> > + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st =
> > nexts) {
> > + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> > + if (st->creatorid == creatorid &&
> > + ((kif && st->kif == kif) || !kif)) {
I find this check confusing.
Since you're continuing when "kif == NULL" can't you change this check
into "st->kif == kif"?
Or is it possible for strlen(clr->ifname) to return 0 in which case you
might end up comparing a different ``kif''?
> > + SET(st->state_flags, PFSTATE_NOSYNC);
> > + pf_unlink_state(st);
> > }
> > }
> > }
Re: preparing pfi_kif to MP world
On Thu, Oct 29, 2015 at 02:49:40AM +0100, Mike Belopuhov wrote: > On 28 October 2015 at 18:41, Alexandr Nedvedicky > wrote: > > Hello Mike, > > > > just a quick question: > > > > are you going to commit your pfi_kif_find() et. al.? > > or more work is needed there? > > > > I need OKs > OK for pf_if.c, I'm still not sure, what's going on in if_pfsync.c thanks and regards sasha
Re: preparing pfi_kif to MP world
On 28 October 2015 at 18:41, Alexandr Nedvedicky
wrote:
> Hello Mike,
>
> just a quick question:
>
> are you going to commit your pfi_kif_find() et. al.?
> or more work is needed there?
>
I need OKs
> thanks a lot
> regards
> sasha
>
>>
>> Turns out this is a rather simple issue that got slightly
>> complicated by the code diverging quite a bit since the
>> inception. Essentially the clr->ifname comes from the
>> interface specification in the "pfctl -i foo0 -Fs" for
>> if-bound states (floating states use fake interface "any").
>>
>> Previously states have been hanging off of kif nodes but it's
>> long gone and we can simply iterate over the state table tree
>> (or even a state list like it's done in the DIOCGETSTATES in
>> pf_ioctl).
>>
>> Calling pf_kif_get here wouldn't be prudent because spawning
>> new objects while disposing of the other ones seems somewhat
>> counterproductive.
>>
> diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
> index 7d633db..fcaf5f5 100644
> --- sys/net/if_pfsync.c
> +++ sys/net/if_pfsync.c
> @@ -752,46 +752,28 @@ done:
>
> int
> pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> {
> struct pfsync_clr *clr;
> - int i;
> -
> struct pf_state *st, *nexts;
> - struct pf_state_key *sk, *nextsk;
> - struct pf_state_item *si;
> + struct pfi_kif *kif = NULL;
> u_int32_t creatorid;
> + int i;
>
> for (i = 0; i < count; i++) {
> clr = (struct pfsync_clr *)buf + len * i;
> creatorid = clr->creatorid;
> + if (strlen(clr->ifname) &&
> + (kif = pfi_kif_find(clr->ifname)) == NULL)
> + continue;
>
> - if (clr->ifname[0] == '\0') {
> - for (st = RB_MIN(pf_state_tree_id, &tree_id);
> - st; st = nexts) {
> - nexts = RB_NEXT(pf_state_tree_id, &tree_id,
> st);
> - if (st->creatorid == creatorid) {
> - SET(st->state_flags, PFSTATE_NOSYNC);
> - pf_unlink_state(st);
> - }
> - }
> - } else {
> - if (pfi_kif_get(clr->ifname) == NULL)
> - continue;
> -
> - /* XXX correct? */
> - for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
> - sk; sk = nextsk) {
> - nextsk = RB_NEXT(pf_state_tree,
> - &pf_statetbl, sk);
> - TAILQ_FOREACH(si, &sk->states, entry) {
> - if (si->s->creatorid == creatorid) {
> - SET(si->s->state_flags,
> - PFSTATE_NOSYNC);
> - pf_unlink_state(si->s);
> - }
> - }
> + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts)
> {
> + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> + if (st->creatorid == creatorid &&
> + ((kif && st->kif == kif) || !kif)) {
> + SET(st->state_flags, PFSTATE_NOSYNC);
> + pf_unlink_state(st);
> }
> }
> }
>
> return (0);
> diff --git sys/net/pf_if.c sys/net/pf_if.c
> index caaf9f9..bf77184 100644
> --- sys/net/pf_if.c
> +++ sys/net/pf_if.c
> @@ -97,18 +97,25 @@ pfi_initialize(void)
> if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL)
> panic("pfi_kif_get for pfi_all failed");
> }
>
> struct pfi_kif *
> -pfi_kif_get(const char *kif_name)
> +pfi_kif_find(const char *kif_name)
> {
> - struct pfi_kif *kif;
> struct pfi_kif_cmp s;
>
> 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 (RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s));
> +}
> +
> +struct pfi_kif *
> +pfi_kif_get(const char *kif_name)
> +{
> + struct pfi_kif *kif;
> +
> + if ((kif = pfi_kif_find(kif_name)))
> return (kif);
>
> /* create new one */
> if ((kif = malloc(sizeof(*kif), PFI_MTYPE, M_NOWAIT|M_ZERO)) == NULL)
> return (NULL);
> diff --git sys/net/pfvar.h sys/net/pfvar.h
> index cdb2f7f..76a98a9 100644
> --- sys/net/pfvar.h
> +++ sys/net/pfvar.h
> @@ -1808,10 +1808,11 @@ int pfr_ina_define(struct pfr_table *, struct
> pfr_addr *, int, int *,
> int *, u_int32_t, int);
>
> extern struct pfi_kif *pfi_all;
>
>
Re: preparing pfi_kif to MP world
Hello Mike,
just a quick question:
are you going to commit your pfi_kif_find() et. al.?
or more work is needed there?
thanks a lot
regards
sasha
>
> Turns out this is a rather simple issue that got slightly
> complicated by the code diverging quite a bit since the
> inception. Essentially the clr->ifname comes from the
> interface specification in the "pfctl -i foo0 -Fs" for
> if-bound states (floating states use fake interface "any").
>
> Previously states have been hanging off of kif nodes but it's
> long gone and we can simply iterate over the state table tree
> (or even a state list like it's done in the DIOCGETSTATES in
> pf_ioctl).
>
> Calling pf_kif_get here wouldn't be prudent because spawning
> new objects while disposing of the other ones seems somewhat
> counterproductive.
>
diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
index 7d633db..fcaf5f5 100644
--- sys/net/if_pfsync.c
+++ sys/net/if_pfsync.c
@@ -752,46 +752,28 @@ done:
int
pfsync_in_clr(caddr_t buf, int len, int count, int flags)
{
struct pfsync_clr *clr;
- int i;
-
struct pf_state *st, *nexts;
- struct pf_state_key *sk, *nextsk;
- struct pf_state_item *si;
+ struct pfi_kif *kif = NULL;
u_int32_t creatorid;
+ int i;
for (i = 0; i < count; i++) {
clr = (struct pfsync_clr *)buf + len * i;
creatorid = clr->creatorid;
+ if (strlen(clr->ifname) &&
+ (kif = pfi_kif_find(clr->ifname)) == NULL)
+ continue;
- if (clr->ifname[0] == '\0') {
- for (st = RB_MIN(pf_state_tree_id, &tree_id);
- st; st = nexts) {
- nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
- if (st->creatorid == creatorid) {
- SET(st->state_flags, PFSTATE_NOSYNC);
- pf_unlink_state(st);
- }
- }
- } else {
- if (pfi_kif_get(clr->ifname) == NULL)
- continue;
-
- /* XXX correct? */
- for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
- sk; sk = nextsk) {
- nextsk = RB_NEXT(pf_state_tree,
- &pf_statetbl, sk);
- TAILQ_FOREACH(si, &sk->states, entry) {
- if (si->s->creatorid == creatorid) {
- SET(si->s->state_flags,
- PFSTATE_NOSYNC);
- pf_unlink_state(si->s);
- }
- }
+ for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts) {
+ nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
+ if (st->creatorid == creatorid &&
+ ((kif && st->kif == kif) || !kif)) {
+ SET(st->state_flags, PFSTATE_NOSYNC);
+ pf_unlink_state(st);
}
}
}
return (0);
diff --git sys/net/pf_if.c sys/net/pf_if.c
index caaf9f9..bf77184 100644
--- sys/net/pf_if.c
+++ sys/net/pf_if.c
@@ -97,18 +97,25 @@ pfi_initialize(void)
if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL)
panic("pfi_kif_get for pfi_all failed");
}
struct pfi_kif *
-pfi_kif_get(const char *kif_name)
+pfi_kif_find(const char *kif_name)
{
- struct pfi_kif *kif;
struct pfi_kif_cmp s;
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 (RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s));
+}
+
+struct pfi_kif *
+pfi_kif_get(const char *kif_name)
+{
+ struct pfi_kif *kif;
+
+ if ((kif = pfi_kif_find(kif_name)))
return (kif);
/* create new one */
if ((kif = malloc(sizeof(*kif), PFI_MTYPE, M_NOWAIT|M_ZERO)) == NULL)
return (NULL);
diff --git sys/net/pfvar.h sys/net/pfvar.h
index cdb2f7f..76a98a9 100644
--- sys/net/pfvar.h
+++ sys/net/pfvar.h
@@ -1808,10 +1808,11 @@ int pfr_ina_define(struct pfr_table *, struct
pfr_addr *, int, int *,
int *, u_int32_t, int);
extern struct pfi_kif *pfi_all;
voidpfi_initialize(void);
+struct pfi_kif *pfi_kif_find(const char *);
struct pfi_kif *pfi_kif_get(const char *);
voidpfi_kif_ref(struct pfi_kif *, enum pfi_kif_refs);
voidpfi_kif_unref(struct pfi_kif *, enum pfi_kif_refs);
int pfi_kif_match(struct pf
Re: preparing pfi_kif to MP world
On Fri, Oct 16, 2015 at 01:41:50PM +0200, Mike Belopuhov wrote: > On 16 October 2015 at 13:28, Alexandr Nedvedicky > wrote: > > > > may be it's kind of bike shading... > > How about make kifs to stick to convention we see for other objects > > such as rulesets/anchors: > > > > pfi_kif_find() > > pfi_kif_find_or_create() > > > > Personally I don't like "_or_create" style of function naming and > I would rather see those renamed to something else > yes, '... naming is hard'. another option would be to keep _find() around and add an argument - a create flag, which orders create if it does not exist. Then all _or_create() can get traded for create flag. I really don't care that much for now... I agree we should stick to your patch. I'm OK with your changes. sasha
Re: preparing pfi_kif to MP world
On 16 October 2015 at 13:28, Alexandr Nedvedicky
wrote:
>
> may be it's kind of bike shading...
> How about make kifs to stick to convention we see for other objects
> such as rulesets/anchors:
>
> pfi_kif_find()
> pfi_kif_find_or_create()
>
Personally I don't like "_or_create" style of function naming and
I would rather see those renamed to something else
> and kill pfi_kif_get() completely, just to avoid confusion/surprise.
>
I kinda like pfi_kif_get interface and apart from this particular
case where I can also use clenching my teeth, it manages to get the
point across: kif objects might not represent any actual interface
but act as a placeholder and you always have to have one ("any").
It might be worth elaborating here as well that removing pfi_kif_get
in other places appears to be wrong: for instance when you're loading
states from a file you want to create placeholder objects for non-
existent pseudo or hotpluggable interfaces.
So yeah, I'm a bit on a fence about this one.
> anything else makes a sense to me.
>
> regards
> sasha
>
Re: preparing pfi_kif to MP world
>
> Turns out this is a rather simple issue that got slightly
> complicated by the code diverging quite a bit since the
> inception. Essentially the clr->ifname comes from the
> interface specification in the "pfctl -i foo0 -Fs" for
> if-bound states (floating states use fake interface "any").
>
> Previously states have been hanging off of kif nodes but it's
> long gone and we can simply iterate over the state table tree
> (or even a state list like it's done in the DIOCGETSTATES in
> pf_ioctl).
>
> Calling pf_kif_get here wouldn't be prudent because spawning
> new objects while disposing of the other ones seems somewhat
> counterproductive.
>
> diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
> index 7d633db..fcaf5f5 100644
> --- sys/net/if_pfsync.c
> +++ sys/net/if_pfsync.c
> @@ -752,46 +752,28 @@ done:
>
> int
> pfsync_in_clr(caddr_t buf, int len, int count, int flags)
> {
> struct pfsync_clr *clr;
> - int i;
> -
> struct pf_state *st, *nexts;
> - struct pf_state_key *sk, *nextsk;
> - struct pf_state_item *si;
> + struct pfi_kif *kif = NULL;
> u_int32_t creatorid;
> + int i;
>
> for (i = 0; i < count; i++) {
> clr = (struct pfsync_clr *)buf + len * i;
> creatorid = clr->creatorid;
> + if (strlen(clr->ifname) &&
> + (kif = pfi_kif_find(clr->ifname)) == NULL)
> + continue;
>
> - if (clr->ifname[0] == '\0') {
> - for (st = RB_MIN(pf_state_tree_id, &tree_id);
> - st; st = nexts) {
> - nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> - if (st->creatorid == creatorid) {
> - SET(st->state_flags, PFSTATE_NOSYNC);
> - pf_unlink_state(st);
> - }
> - }
> - } else {
> - if (pfi_kif_get(clr->ifname) == NULL)
> - continue;
> -
> - /* XXX correct? */
> - for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
> - sk; sk = nextsk) {
> - nextsk = RB_NEXT(pf_state_tree,
> - &pf_statetbl, sk);
> - TAILQ_FOREACH(si, &sk->states, entry) {
> - if (si->s->creatorid == creatorid) {
> - SET(si->s->state_flags,
> - PFSTATE_NOSYNC);
> - pf_unlink_state(si->s);
> - }
> - }
> + for (st = RB_MIN(pf_state_tree_id, &tree_id); st; st = nexts) {
> + nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
> + if (st->creatorid == creatorid &&
> + ((kif && st->kif == kif) || !kif)) {
> + SET(st->state_flags, PFSTATE_NOSYNC);
> + pf_unlink_state(st);
> }
> }
> }
>
> return (0);
> diff --git sys/net/pf_if.c sys/net/pf_if.c
> index caaf9f9..bf77184 100644
> --- sys/net/pf_if.c
> +++ sys/net/pf_if.c
> @@ -97,18 +97,25 @@ pfi_initialize(void)
> if ((pfi_all = pfi_kif_get(IFG_ALL)) == NULL)
> panic("pfi_kif_get for pfi_all failed");
> }
>
> struct pfi_kif *
> -pfi_kif_get(const char *kif_name)
> +pfi_kif_find(const char *kif_name)
> {
> - struct pfi_kif *kif;
> struct pfi_kif_cmp s;
>
> 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 (RB_FIND(pfi_ifhead, &pfi_ifs, (struct pfi_kif *)&s));
> +}
> +
> +struct pfi_kif *
> +pfi_kif_get(const char *kif_name)
may be it's kind of bike shading...
How about make kifs to stick to convention we see for other objects
such as rulesets/anchors:
pfi_kif_find()
pfi_kif_find_or_create()
and kill pfi_kif_get() completely, just to avoid confusion/surprise.
anything else makes a sense to me.
regards
sasha
Re: preparing pfi_kif to MP world
On Wed, Oct 14, 2015 at 10:03 +1000, David Gwynne wrote:
>
> > On 13 Oct 2015, at 21:12, Mike Belopuhov wrote:
> >
> > On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
> >>
> >>> On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> >>> 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.
> >>
> >
> > Revision 1.102 used to get the kif and check the state against it:
> >
> > if (si->s->creatorid == creatorid &&
> > si->s->kif == kif) {
> >
> > Then in the rev 1.103 you have removed the check. The question is
> > why did you do that? (-:
>
> oh. i have no idea. it was a long time ago :(
>
> putting it back makes sense to me, but id like the opinions of
> people who are more aware of pf internals to agree with me too.
>
> dlg
Turns out this is a rather simple issue that got slightly
complicated by the code diverging quite a bit since the
inception. Essentially the clr->ifname comes from the
interface specification in the "pfctl -i foo0 -Fs" for
if-bound states (floating states use fake interface "any").
Previously states have been hanging off of kif nodes but it's
long gone and we can simply iterate over the state table tree
(or even a state list like it's done in the DIOCGETSTATES in
pf_ioctl).
Calling pf_kif_get here wouldn't be prudent because spawning
new objects while disposing of the other ones seems somewhat
counterproductive.
diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
index 7d633db..fcaf5f5 100644
--- sys/net/if_pfsync.c
+++ sys/net/if_pfsync.c
@@ -752,46 +752,28 @@ done:
int
pfsync_in_clr(caddr_t buf, int len, int count, int flags)
{
struct pfsync_clr *clr;
- int i;
-
struct pf_state *st, *nexts;
- struct pf_state_key *sk, *nextsk;
- struct pf_state_item *si;
+ struct pfi_kif *kif = NULL;
u_int32_t creatorid;
+ int i;
for (i = 0; i < count; i++) {
clr = (struct pfsync_clr *)buf + len * i;
creatorid = clr->creatorid;
+ if (strlen(clr->ifname) &&
+ (kif = pfi_kif_find(clr->ifname)) == NULL)
+ continue;
- if (clr->ifname[0] == '\0') {
- for (st = RB_MIN(pf_state_tree_id, &tree_id);
- st; st = nexts) {
- nexts = RB_NEXT(pf_state_tree_id, &tree_id, st);
- if (st->creatorid == creatorid) {
- SET(st->state_flags, PFSTATE_NOSYNC);
- pf_unlink_state(st);
- }
- }
- } else {
- if (pfi_kif_get(clr->ifname) == NULL)
- continue;
-
- /* XXX correct? */
- for (sk = RB_MIN(pf_state_tree, &pf_statetbl);
- sk; sk = nextsk) {
- nextsk = RB_NEXT(pf_state_tree,
- &pf_statetbl, sk);
- TAILQ_FOREACH(si, &sk->states, entry) {
- if (si->s->creatorid == creatorid) {
- SET(si->s->state_flags,
- PFSTATE_NOSYNC);
-
Re: preparing pfi_kif to MP world
> On 13 Oct 2015, at 21:12, Mike Belopuhov wrote:
>
> On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>>
>>> On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
>>> 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.
>>
>
> Revision 1.102 used to get the kif and check the state against it:
>
> if (si->s->creatorid == creatorid &&
> si->s->kif == kif) {
>
> Then in the rev 1.103 you have removed the check. The question is
> why did you do that? (-:
oh. i have no idea. it was a long time ago :(
putting it back makes sense to me, but id like the opinions of people who are
more aware of pf internals to agree with me too.
dlg
Re: preparing pfi_kif to MP world
Hello,
> > > 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]
> > > @@ -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 --^
>
yes, that's true, will fix that...
>
> Even the typedef? (-:
>
sorry I forgot about OpenBSD hates typedefs ;-)
I'll keep in mind to kill all typdefs in my patches next time.
> but I'd prefer the former, e.g. _k instead of _kif_. Double
O.K. I'll stick to _k style.
>
> 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.
it's true in case of pfi_kif objects we can scrap them.
on the other hand the macros illustrate the way I'd like to reference counting
for other objects. I'll stick to the pfi_kif just as an illustration.
if I use lowercase function such as:
r->kif = pfi_kif_take(kif);
then I'm sure the kif instance exists, if it does not exist the thing
will blow up with NULL pointer dereference and I know I'm wrong. On the
other hand if I do:
r->kif = PFI_KIF_TAKE(kif);
then it means kif is optional here, but if it exists, then I must have
a reference for it here.
The way we have it in PF on Solaris these days looks as follows:
_take(x)
{
if (x != NULL)
PF_INC_REF(x->refcnt);
return (x);
}
This way we assume 'x' is always optional. And this is sometimes quite
wrong, as we might set mine, we trip over later.
The uppercase macros allows us to easily annotate cases, where the
object is optional and vice versa: the lower case functions annotate
cases, where we assume the object must exist. This could save us few
days
of debugging.
So I'd like to make sure if you hate that approach in general or in the
scope of this particular patch. I admit the pfi_kif is not the right
guy for the uppercase version as the pfi_kif instance always must exist.
I'll remove those macros from proposed patch, but I'd like to know
how do you feel about them for other objects such as tables, anchors,
states,
> 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 */".
>
yes that's very good point. (How I could miss it?)
thanks and
regards
sasha
Re: preparing pfi_kif to MP world
On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>
> > On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> > 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 - 1.220
> > +++ if_pfsync.c 11 Oct 2015 13:46:11 -
> > @@ -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.c8 Oct 2015 11:36:51 - 1.946
> > +++ pf.c11 Oct 2015 13:46:14 -
> > @@ -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 - 1.80
> > +++ pf_if.c 11 Oct 2015 13:46:14 -
> > @@ -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 wha
Re: preparing pfi_kif to MP world
On Tue, Oct 13, 2015 at 20:36 +1000, David Gwynne wrote:
>
> > On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> > 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.
>
Revision 1.102 used to get the kif and check the state against it:
if (si->s->creatorid == creatorid &&
si->s->kif == kif) {
Then in the rev 1.103 you have removed the check. The question is
why did you do that? (-:
Re: preparing pfi_kif to MP world
> On 12 Oct 2015, at 12:00 AM, Alexandr Nedvedicky
> 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 - 1.220
> +++ if_pfsync.c 11 Oct 2015 13:46:11 -
> @@ -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 - 1.946
> +++ pf.c 11 Oct 2015 13:46:14 -
> @@ -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 - 1.80
> +++ pf_if.c 11 Oct 2015 13:46:14 -
> @@ -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 (ki
