[patch] Reloading pf rules breaks connections on lo0

[Andreas Longwitz]

Ermal Luçi wrote:
> 
> I say intended because so it behaves on the upstream. 
> By introducing another not needed option you introduce another hack on
> top of the already hackish 'set skip' one.
> 
> The correct 'fix' for it to behave correctly is to fetch the interface
> list from pf(4) and verify if something needs to be cleared or not.
> You can call pfi_get_ifaces and compare it with the defined 'set skip'
> rules.
> 
> That is easier than adding a new option.
> 
I agree with your statements completely. The following patch for pfctl.c
   solves for me the lo0 breaking problem without introducing a new
option. The patched pfctl clears the skip flag exactly for those actual
skip interfaces not longer included in the new pf.conf anymore.

--- pfctl.c.orig        2013-01-14 15:17:48.000000000 +0100
+++ pfctl.c     2013-03-27 22:01:37.000000000 +0100
@@ -67,6 +67,9 @@
 int     pfctl_enable(int, int);
 int     pfctl_disable(int, int);
 int     pfctl_clear_stats(int, int);
+int     pfctl_get_skip_ifaces(void);
+int     pfctl_check_skip_ifaces(char *);
+int     pfctl_clear_skip_ifaces(struct pfctl *);
 int     pfctl_clear_interface_flags(int, int);
 int     pfctl_clear_rules(int, int, char *);
 int     pfctl_clear_nat(int, int, char *);
@@ -101,10 +104,13 @@
                struct pf_ruleset *, int, int);
 int     pfctl_load_rule(struct pfctl *, char *, struct pf_rule *, int);
 const char     *pfctl_lookup_option(char *, const char **);
+static void    radix_perror(void);

 struct pf_anchor_global         pf_anchors;
 struct pf_anchor        pf_main_anchor;

+struct pfr_buffer       skip_b;
+
 const char     *clearopt;
 char           *rulesopt;
 const char     *showopt;
@@ -296,6 +302,53 @@
     return (0);
 }

+void
+radix_perror(void)
+{
+    extern char *__progname;
+    fprintf(stderr, "%s: %s.\n", __progname, pfr_strerror(errno));
+}
+
+int
+pfctl_get_skip_ifaces(void)
+{
+    bzero(&skip_b, sizeof(skip_b));
+    skip_b.pfrb_type = PFRB_IFACES;
+    for (;;) {
+      pfr_buf_grow(&skip_b, skip_b.pfrb_size);
+      skip_b.pfrb_size = skip_b.pfrb_msize;
+      if (pfi_get_ifaces(NULL, skip_b.pfrb_caddr, &skip_b.pfrb_size)) {
+              radix_perror();
+              return (1);
+      }
+      if (skip_b.pfrb_size <= skip_b.pfrb_msize)
+              break;
+    }
+    return (0);
+}
+
+int
+pfctl_check_skip_ifaces(char *ifname)
+{
+    struct pfi_kif          *p;
+
+    PFRB_FOREACH(p, &skip_b)
+         if ((p->pfik_flags & PFI_IFLAG_SKIP) && !strcmp(ifname,
p->pfik_name))
+                 p->pfik_flags &= ~PFI_IFLAG_SKIP;
+    return (0);
+}
+
+int
+pfctl_clear_skip_ifaces(struct pfctl *pf)
+{
+    struct pfi_kif          *p;
+
+    PFRB_FOREACH(p, &skip_b)
+      if (p->pfik_flags & PFI_IFLAG_SKIP)
+        pfctl_set_interface_flags(pf, p->pfik_name, PFI_IFLAG_SKIP, 0);
+    return (0);
+}
+
 int
 pfctl_clear_interface_flags(int dev, int opts)
 {
@@ -1437,6 +1490,8 @@
             else
                     goto _error;
     }
+    if (loadopt & PFCTL_FLAG_OPTION)
+            pfctl_clear_skip_ifaces(&pf);

     if ((pf.loadopt & PFCTL_FLAG_FILTER &&
         (pfctl_load_ruleset(&pf, path, rs, PF_RULESET_SCRUB, 0))) ||
@@ -1861,6 +1916,7 @@
             } else {
                     if (ioctl(pf->dev, DIOCSETIFFLAG, &pi))
                             err(1, "DIOCSETIFFLAG");
+                    pfctl_check_skip_ifaces(ifname);
             }
     }
     return (0);
@@ -2340,7 +2396,7 @@
     }
     if ((rulesopt != NULL) && (loadopt & PFCTL_FLAG_OPTION) &&
         !anchorname[0])
-            if (pfctl_clear_interface_flags(dev, opts | PF_OPT_QUIET))
+            if (pfctl_get_skip_ifaces())
                     error = 1;

     if (rulesopt != NULL && !(opts & (PF_OPT_MERGE|PF_OPT_NOACTION)) &&


-- 
Andreas Longwitz