On 28 October 2015 at 18:41, Alexandr Nedvedicky
<alexandr.nedvedi...@oracle.com> 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;
>
> void pfi_initialize(void);
> +struct pfi_kif *pfi_kif_find(const char *);
> 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);
> int pfi_kif_match(struct pfi_kif *, struct pfi_kif *);
> void pfi_attach_ifnet(struct ifnet *);
