Hello Richard,
thanks for taking a look at this. I'm not sure. ioctl() basically handles the
insertion/load of rules. Rule might be actually pre-loaded before desired
interface will come alive. So in general ioctl() always want to make
sure the pfi_kif interface object referenced by rule exists.
especially in cases where we do:
...->kif = pfi_kif_get(...)
> pf_ioctl.c
> 1031 qs->kif = pfi_kif_get(qs->ifname);
> 1032 if (qs->kif == NULL) {
> 1033 pool_put(&pf_queue_pl, qs);
> 1034 error = ESRCH;
> 1035 break;
> 1036 }
I took a quick look at your patch. I have one comment:
> - if ((kif = pfi_kif_get(sp->ifname)) == NULL) {
> + if ((kif = pfi_kif_find(sp->ifname)) == NULL) {
> DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
> "unknown interface: %s", sp->ifname);
> if (flags & PFSYNC_SI_IOCTL)
> @@ -493,6 +493,9 @@ pfsync_state_import(struct pfsync_state
> if (sp->af == 0)
> return (0); /* skip this state */
>
> + kif = pfi_kif_get(sp->ifname);
> + KASSERT(kif != NULL);
introducing pfi_kif_find() followed by pfi_kif_get() is overkill. I think
pfi_kif_find() should return a reference to existing kif and NULL if desired
interface does not exist. This is more friendly with respect to upcoming
MP changes.
thanks and
regards
sasha
On Thu, Oct 15, 2015 at 11:57:26AM +1300, Richard Procter wrote:
> Hi Sasha,
>
> I notice the calls in pf_ioctl.c below may not intend to create a new kif,
> either.
>
> In constrast all but one of the calls in pf_if.c treat pfi_kfi_get() == NULL
> as a panic, suggesting they test for out-of-memory conditions. The exception
> may also intend to find, not create.
>
> This confusion suggests that pfi_kif_get() is poorly named or does too much.
>
> I include a slight tweak on your patch below as another possibility, renaming
> check to find and using find in pfi_kif_get(). I find it hard to think of a
> better name for pfi_kif_get(), though. Maybe pfi_kif_realize(const char
> *kif_name)?
>
> best,
> Richard.
>
> These following may intend a find but I don't know for sure.
>
> pf_ioctl.c
> 1031 qs->kif = pfi_kif_get(qs->ifname);
> 1032 if (qs->kif == NULL) {
> 1033 pool_put(&pf_queue_pl, qs);
> 1034 error = ESRCH;
> 1035 break;
> 1036 }
>
> pf_ioctl.c
> 0785 pf_kif_setup(char *ifname, struct pfi_kif **kif)
> 0786 {
> 0787 if (ifname[0]) {
> 0788 *kif = pfi_kif_get(ifname);
> 0789 if (*kif == NULL)
> 0790 return (EINVAL);
> 0791
> 0792 pfi_kif_ref(*kif, PFI_KIF_REF_RULE);
> 0793 } else
> 0794 *kif = NULL;
> 0795
> 0796 return (0);
> 0797 }
> 0798
>
> pf_if.c
> 0365 s = splsoftnet();
> 0366 if (!strcmp(aw->v.ifname, "self"))
> 0367 dyn->pfid_kif = pfi_kif_get(IFG_ALL);
> 0368 else
> 0369 dyn->pfid_kif = pfi_kif_get(aw->v.ifname);
> 0370 if (dyn->pfid_kif == NULL) {
> 0371 rv = 1;
> 0372 goto _bad;
> 0373 }
>
> Index: net/if_pfsync.c
> ===================================================================
> --- net.orig/if_pfsync.c
> +++ net/if_pfsync.c
> @@ -482,7 +482,7 @@ pfsync_state_import(struct pfsync_state
> return (EINVAL);
> }
>
> - if ((kif = pfi_kif_get(sp->ifname)) == NULL) {
> + if ((kif = pfi_kif_find(sp->ifname)) == NULL) {
> DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
> "unknown interface: %s", sp->ifname);
> if (flags & PFSYNC_SI_IOCTL)
> @@ -493,6 +493,9 @@ pfsync_state_import(struct pfsync_state
> if (sp->af == 0)
> return (0); /* skip this state */
>
> + kif = pfi_kif_get(sp->ifname);
> + KASSERT(kif != NULL);
> +
> /*
> * If the ruleset checksums match or the state is coming from the ioctl,
> * it's safe to associate the state with the rule of that number.
> @@ -775,7 +778,7 @@ pfsync_in_clr(caddr_t buf, int len, int
> }
> }
> } else {
> - if (pfi_kif_get(clr->ifname) == NULL)
> + if (pfi_kif_find(clr->ifname) == NULL)
> continue;
>
> /* XXX correct? */
> Index: net/pf_if.c
> ===================================================================
> --- net.orig/pf_if.c
> +++ net/pf_if.c
> @@ -99,14 +99,22 @@ pfi_initialize(void)
> }
>
> 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;
> + 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)) != NULL)
> return (kif);
>
> /* create new one */
> Index: net/pfvar.h
> ===================================================================
> --- net.orig/pfvar.h
> +++ net/pfvar.h
> @@ -1810,6 +1810,7 @@ int pfr_ina_define(struct pfr_table *, s
> 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);
>
>
> On 15/10/2015, at 2:11 AM, Alexandr Nedvedicky wrote:
>
> > 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 *);
> >
>
