On Sun, Apr 05, 2015 at 12:06 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> while testing PBR on Solaris we found out the pfi_kif instances
> are not removed from pfi_ifs table. We took a look at crashdump
> and have seen pfik_route counter at those object is still
> non-zero, while all rules were gone.
>
> looking at sources we can see the 'pfik_route' (PFI_KIF_REF_ROUTE)
> reference is being grabbed in pfr_create_kentry():
>
> 840 case PFRKE_ROUTE:
> 841 if (ad->pfra_ifname[0])
> 842 ke->pfrke_rkif = pfi_kif_get(ad->pfra_ifname);
> 843 if (ke->pfrke_rkif)
> 844 pfi_kif_ref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE);
> 845 break;
> 846 default:
> 847 panic("unknown pfrke_type %d", ke->pfrke_type);
> 848 break;
>
> however we have not found any matching pfi_kif_ref() command, which
> would remove the reference created by pfr_create_kentry(). It seems
> to us the call to
>
> pfi_kif_unref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE)
>
> is missing at pfr_destroy_kentry(). We created patch against OpenBSD CURRENT.
> We have no OpenBSD boxes around, where we could verify our fix.
>
> also for your info: IPF in Solaris is on its death row. PF in 11.3
> release will be available as optional firewall. We hope to make PF
> default (and only firewall) in Solaris 12. You've made excellent job,
> your PF is crystal-clear design.
>
> kind regards
> sasha
>
> ----------- cut here to get a patch ---------------
>
> Index: pf_table.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.106
> diff -u -r1.106 pf_table.c
> --- pf_table.c 14 Mar 2015 03:38:51 -0000 1.106
> +++ pf_table.c 5 Apr 2015 09:59:58 -0000
> @@ -877,6 +877,17 @@
> {
> if (ke->pfrke_counters)
> pool_put(&pfr_kcounters_pl, ke->pfrke_counters);
> +
> + switch (ke->pfrke_type) {
> + case PFRKE_COST:
> + /* FALLTHROUGH */
> + case PFRKE_ROUTE:
> + if (ke->pfrke_rkif != NULL) {
> + pfi_kif_unref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE);
> + }
> + break;
> + default:
> + }
> pool_put(&pfr_kentry_pl[ke->pfrke_type], ke);
> }
>
>
Hi,
Your analysis is correct. I'd drop the switch statement here for
simplicity though.
Anyone's willing to OK?
diff --git sys/net/pf_table.c sys/net/pf_table.c
index 10e6e4f..6c32c68 100644
--- sys/net/pf_table.c
+++ sys/net/pf_table.c
@@ -875,10 +875,13 @@ pfr_destroy_kentries(struct pfr_kentryworkq *workq)
void
pfr_destroy_kentry(struct pfr_kentry *ke)
{
if (ke->pfrke_counters)
pool_put(&pfr_kcounters_pl, ke->pfrke_counters);
+ if (ke->pfrke_type == PFRKE_COST || ke->pfrke_type == PFRKE_ROUTE)
+ pfi_kif_unref(((struct pfr_kentry_all *)ke)->pfrke_rkif,
+ PFI_KIF_REF_ROUTE);
pool_put(&pfr_kentry_pl[ke->pfrke_type], ke);
}
void
pfr_insert_kentries(struct pfr_ktable *kt,
