[patch] Reloading pf rules breaks connections on lo0

[Ermal Luçi]

On Thu, Mar 28, 2013 at 3:03 PM, Andreas Longwitz <longwitz at incore.de>wrote:

> 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;
>

any reason this is not static?


> +
>  const char     *clearopt;
>  char           *rulesopt;
>  const char     *showopt;
> @@ -296,6 +302,53 @@
>      return (0);
>  }
>
> +void
> +radix_perror(void)
> +{
>

Why do you need the extra function?
If any reason can you redo the patch with a pfctl_ prepended and a better
naming?

> +    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
>
>
Looks ok.
Can you make the changes so i can push it?

-- 
Ermal