From owner-freebsd-pf@FreeBSD.ORG Thu Mar 28 16:31:33 2013 Return-Path: Delivered-To: freebsd-pf@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 93565DB6 for ; Thu, 28 Mar 2013 16:31:33 +0000 (UTC) (envelope-from ermal.luci@gmail.com) Received: from mail-qe0-f53.google.com (mail-qe0-f53.google.com [209.85.128.53]) by mx1.freebsd.org (Postfix) with ESMTP id 572F22F1 for ; Thu, 28 Mar 2013 16:31:33 +0000 (UTC) Received: by mail-qe0-f53.google.com with SMTP id q19so3099283qeb.26 for ; Thu, 28 Mar 2013 09:31:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=BTmvpviRGFmXOn5zBNnUh5exxwYa7L+qO/EBgLmiRYc=; b=WJ7SMBbkLhRwOnHwbpin6z8bQpXr4n3CdKO7NH5Tp+EuSExUUsjQKWFqFTCZ6tM10g 6c3wZqjDbe933eLw16xBUOnJh61zbKsOZTw/L92BmSt7MfnrFPo1U1Wq6u9a4NoIdvD2 dYvsA4T60Yn1vp722N3DpjGbrF9pvCsgOA/Nbl5eWaaw4/NB6So2q24W+avj20xTU9J3 QigPo0Ge+Ppqd5aQtEm234HgTia5xkpJSlgo3V4EseolCpsaRQ6xP3yje+Ios6z5oF45 XrBJ2NSckR51HvVW8RuqiSydGnclcfypLnyeBdpRAiCQIeYPF/wTPa3FZPmLcLIUyBl2 bM7w== MIME-Version: 1.0 X-Received: by 10.224.198.67 with SMTP id en3mr17991978qab.23.1364488292426; Thu, 28 Mar 2013 09:31:32 -0700 (PDT) Sender: ermal.luci@gmail.com Received: by 10.49.49.37 with HTTP; Thu, 28 Mar 2013 09:31:32 -0700 (PDT) In-Reply-To: <51544DAF.7000203@incore.de> References: <5134C218.6060701@incore.de> <5149BE75.3040308@incore.de> <5149E3A8.3020608@incore.de> <51544DAF.7000203@incore.de> Date: Thu, 28 Mar 2013 17:31:32 +0100 X-Google-Sender-Auth: w4WBdbw64BJNyocirTapwvHuun0 Message-ID: Subject: Re: [patch] Reloading pf rules breaks connections on lo0 From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= To: Andreas Longwitz Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.14 Cc: "freebsd-pf@freebsd.org" X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Technical discussion and general questions about packet filter \(pf\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Mar 2013 16:31:33 -0000 On Thu, Mar 28, 2013 at 3:03 PM, Andreas Longwitz wrote= : > Ermal Lu=E7i 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 =3D PFRB_IFACES; > + for (;;) { > + pfr_buf_grow(&skip_b, skip_b.pfrb_size); > + skip_b.pfrb_size =3D 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 <=3D 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 &=3D ~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 !=3D NULL) && (loadopt & PFCTL_FLAG_OPTION) && > !anchorname[0]) > - if (pfctl_clear_interface_flags(dev, opts | PF_OPT_QUIET)) > + if (pfctl_get_skip_ifaces()) > error =3D 1; > > if (rulesopt !=3D NULL && !(opts & (PF_OPT_MERGE|PF_OPT_NOACTION)) &= & > > > -- > Andreas Longwitz > > Looks ok. Can you make the changes so i can push it? --=20 Ermal