On Wed, Oct 14, 2015 at 10:03 +1000, David Gwynne wrote:
>
> > On 13 Oct 2015, at 21:12, Mike Belopuhov <[email protected]> wrote:
> >
> > 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).
> >>>
> >>> 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);
- 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 *);
