Hello,
While reviewing patch, which introduces MP-friendly reference counting for
pfi_kif objects (PF stuff), we've found some 'interesting detail' in
if_pfsync.c,
which I think should get addressed by extra patch.
It looks like if_pfsync.c module does not realize, the pfi_kif_get() actually
always attempt to create a pfi_kif instance, if it does not exist already.
This particular module rather needs a pfi_kif_check() function, which returns 0
if pfi_kif does not exist for name, non-zero otherwise. To support my
statement I'm going to bring two snippets from if_pfsync.c:
snippet (1)
470 pfsync_state_import(struct pfsync_state *sp, int flags)
471 {
472 struct pf_state *st = NULL;
473 struct pf_state_key *skw = NULL, *sks = NULL;
474 struct pf_rule *r = NULL;
475 struct pfi_kif *kif;
476 int pool_flags;
477 int error;
478
479 if (sp->creatorid == 0) {
480 DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
481 "invalid creator id: %08x", ntohl(sp->creatorid));
482 return (EINVAL);
483 }
484
485 if ((kif = pfi_kif_get(sp->ifname)) == NULL) {
486 DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
487 "unknown interface: %s", sp->ifname);
488 if (flags & PFSYNC_SI_IOCTL)
489 return (EINVAL);
490 return (0); /* skip this state */
491 }
snippet (2)
756 pfsyn_in_clr(caddr_t buf, int len, int count, int flags)
755 {
756 struct pfsync_clr *clr;
757 int i;
...
764 for (i = 0; i < count; i++) {
... }
777 } else {
778 if (pfi_kif_get(clr->ifname) == NULL)
779 continue;
...
795 }
in both cases we actually want to use pfi_kif_get() to verify if interface with
desired '->ifname' exists in PF. Consider we are trying to import state, which
is bound to interface (if-bound). Let's further assume imported state is bound
to `eexist0` interface. There is no such `eexist0` network interface on PF
node, where we attempt to import state to. In current if_pfsync.c so we
actually use pfi_kif_get() to create such interface for us in PF at least, we
create pfi_kif with no real HW behind. This will keep imported state consistent,
however it wastes resources, because the state is doomed to time out anyway, as
there will be no matching packets for it. No one can expect there will be
packet bound to `eexist0` interface, if such interface does not exist.
So I really think the if_pfsync.c should rather be doing pfi_kif_check()
instead of pfi_kif_get(). However I must admit I don't know all details
here.
opinions/OKs much appreciated here...
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 14 Oct 2015 12:18:57 -0000
@@ -482,13 +482,15 @@ pfsync_state_import(struct pfsync_state
return (EINVAL);
}
- if ((kif = pfi_kif_get(sp->ifname)) == NULL) {
+ if (pfi_kif_check(sp->ifname) == 0) {
DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
"unknown interface: %s", sp->ifname);
if (flags & PFSYNC_SI_IOCTL)
return (EINVAL);
return (0); /* skip this state */
}
+ kif = pfi_kif_get(sp->ifname);
+ KASSERT(kif != NULL);
if (sp->af == 0)
return (0); /* skip this state */
@@ -775,7 +777,7 @@ pfsync_in_clr(caddr_t buf, int len, int
}
}
} else {
- if (pfi_kif_get(clr->ifname) == NULL)
+ if (pfi_kif_check(clr->ifname) == 0)
continue;
/* XXX correct? */
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 14 Oct 2015 12:18:58 -0000
@@ -127,6 +127,16 @@ pfi_kif_get(const char *kif_name)
return (kif);
}
+int
+pfi_kif_check(const char *kif_name)
+{
+ struct pfi_kif_cmp s;
+
+ 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) != NULL);
+}
+
void
pfi_kif_ref(struct pfi_kif *kif, enum pfi_kif_refs what)
{
Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.421
diff -u -p -r1.421 pfvar.h
--- pfvar.h 13 Oct 2015 19:32:32 -0000 1.421
+++ pfvar.h 14 Oct 2015 12:18:59 -0000
@@ -1811,6 +1811,7 @@ extern struct pfi_kif *pfi_all;
void pfi_initialize(void);
struct pfi_kif *pfi_kif_get(const char *);
+int pfi_kif_check(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 *);
