Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: Error when at startup more than 512 anchors are loaded from pf.conf
To:
Alexandr Nedvedicky <sashan@fastmail.net>
Cc:
Rafa?? Ramocki <rafal.ramocki@eo.pl>, tech <tech@openbsd.org>
Date:
Thu, 22 May 2025 12:56:04 +0900

Download raw body.

Thread
  • Alexandr Nedvedicky:

    Error when at startup more than 512 anchors are loaded from pf.conf

  • On Mon, May 12, 2025 at 10:26:10AM +0200, Alexandr Nedvedicky wrote:
    > Hello,
    > 
    > On Wed, May 07, 2025 at 10:34:49AM +0200, Rafa?? Ramocki wrote:
    > > Hello,
    > > 
    > > I've tested a bit more. I have the problem but only if i load configuration second time.
    > > 
    > > (16:31:08):~
    > > # pfctl -f /etc/pf.conf                                                                                                                                                                                                                                                                 
    > 
    >     I think I've figured out what's going on there. Currently the pfctl(8)
    >     always initializes defaults by calling pfctl_init_options(). This is where
    >     we set anchor limit to 512 although the limit used by kernel might be different.
    >     I've just updated the diff to let  pfctl_init_options() use values we found
    >     when calling pfctl_read_limits() when we open /dev/pf. The pfctl_init_options()
    >     is being called from pfctl_rules() which is entered well after /dev/pf gets
    >     opened.
    > 
    >     the other tweak is to replace exit() at the end of main() by return (suggested
    >     by bluhm@)
    > 
    >     and there is one more tweak to what's been OKed by bluhm@: in earlier diff I've
    >     removed call to pfctl_load_options() in pfctl_rules(). Now I think this must stay,
    >     because we must make sure we call pfctl_load_options() at least one time when
    >     loading rules even if the pf.conf itself has no 'set limit ...'
    > 
    >     The diff below should be good enough for now. OK?
    
    Note that pool_sethardlimit() has lost two of its parameters recently.
    You have to merge your diff.
    
    OK bluhm@
    
    > --------8<---------------8<---------------8<------------------8<--------
    > diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
    > index 825bd4cf04c..04f2af43770 100644
    > --- a/sbin/pfctl/pfctl.c
    > +++ b/sbin/pfctl/pfctl.c
    > @@ -93,6 +93,8 @@ int	 pfctl_show_states(int, const char *, int, long);
    >  int	 pfctl_show_status(int, int);
    >  int	 pfctl_show_timeouts(int, int);
    >  int	 pfctl_show_limits(int, int);
    > +void	 pfctl_read_limits(int);
    > +void	 pfctl_restore_limits(void);
    >  void	 pfctl_debug(int, u_int32_t, int);
    >  int	 pfctl_show_anchors(int, int, char *);
    >  int	 pfctl_ruleset_trans(struct pfctl *, char *, struct pf_anchor *);
    > @@ -163,6 +165,8 @@ static const struct {
    >  	{ NULL,			0 }
    >  };
    >  
    > +static unsigned int	limit_curr[PF_LIMIT_MAX];
    > +
    >  struct pf_hint {
    >  	const char	*name;
    >  	int		timeout;
    > @@ -1162,6 +1166,37 @@ pfctl_show_limits(int dev, int opts)
    >  	return (0);
    >  }
    >  
    > +void
    > +pfctl_read_limits(int dev)
    > +{
    > +	struct pfioc_limit pl;
    > +	int i;
    > +
    > +	for (i = 0; pf_limits[i].name; i++) {
    > +		pl.index = pf_limits[i].index;
    > +		if (ioctl(dev, DIOCGETLIMIT, &pl) == -1)
    > +			err(1, "DIOCGETLIMIT");
    > +		limit_curr[i] = pl.limit;
    > +	}
    > +}
    > +
    > +void
    > +pfctl_restore_limits(void)
    > +{
    > +	struct pfioc_limit pl;
    > +	int i;
    > +
    > +	if (dev == -1)
    > +		return;
    > +
    > +	for (i = 0; pf_limits[i].name; i++) {
    > +		pl.index = pf_limits[i].index;
    > +		pl.limit = limit_curr[i];
    > +		if (ioctl(dev, DIOCSETLIMIT, &pl) == -1)
    > +			warn("DIOCSETLIMIT (%s)", pf_limits[i].name);
    > +	}
    > +}
    > +
    >  /* callbacks for rule/nat/rdr/addr */
    >  void
    >  pfctl_add_rule(struct pfctl *pf, struct pf_rule *r)
    > @@ -1762,18 +1797,30 @@ pfctl_init_options(struct pfctl *pf)
    >  	pf->syncookieswat[0] = PF_SYNCOOKIES_LOWATPCT;
    >  	pf->syncookieswat[1] = PF_SYNCOOKIES_HIWATPCT;
    >  
    > +	/*
    > +	 * limit_curr is populated by pfctl_read_limits() after main() opens
    > +	 * /dev/pf.
    > +	 */
    >  	mib[0] = CTL_KERN;
    >  	mib[1] = KERN_MAXCLUSTERS;
    >  	size = sizeof(mcl);
    >  	if (sysctl(mib, 2, &mcl, &size, NULL, 0) == -1)
    >  		err(1, "sysctl");
    > -	pf->limit[PF_LIMIT_FRAGS] = mcl / 4;
    > -
    > -	pf->limit[PF_LIMIT_SRC_NODES] = PFSNODE_HIWAT;
    > -	pf->limit[PF_LIMIT_TABLES] = PFR_KTABLE_HIWAT;
    > -	pf->limit[PF_LIMIT_TABLE_ENTRIES] = PFR_KENTRY_HIWAT;
    > -	pf->limit[PF_LIMIT_PKTDELAY_PKTS] = PF_PKTDELAY_MAXPKTS;
    > -	pf->limit[PF_LIMIT_ANCHORS] = PF_ANCHOR_HIWAT;
    > +	pf->limit[PF_LIMIT_FRAGS] = (limit_curr[PF_LIMIT_FRAGS] == 0) ?
    > +	    mcl / 4 : limit_curr[PF_LIMIT_FRAGS];
    > +
    > +	pf->limit[PF_LIMIT_SRC_NODES] = (limit_curr[PF_LIMIT_SRC_NODES] == 0) ?
    > +	    PFSNODE_HIWAT : limit_curr[PF_LIMIT_SRC_NODES];
    > +	pf->limit[PF_LIMIT_TABLES] = (limit_curr[PF_LIMIT_TABLES] == 0) ?
    > +	    PFR_KTABLE_HIWAT : limit_curr[PF_LIMIT_TABLES];
    > +	pf->limit[PF_LIMIT_TABLE_ENTRIES] =
    > +	    (limit_curr[PF_LIMIT_TABLE_ENTRIES] == 0) ?
    > +		PFR_KENTRY_HIWAT : limit_curr[PF_LIMIT_TABLE_ENTRIES];
    > +	pf->limit[PF_LIMIT_PKTDELAY_PKTS] =
    > +	    (limit_curr[PF_LIMIT_PKTDELAY_PKTS] == 0) ?
    > +		PF_PKTDELAY_MAXPKTS : limit_curr[PF_LIMIT_PKTDELAY_PKTS];
    > +	pf->limit[PF_LIMIT_ANCHORS] = (limit_curr[PF_LIMIT_ANCHORS] == 0) ?
    > +	    PF_ANCHOR_HIWAT : limit_curr[PF_LIMIT_ANCHORS];
    >  
    >  	mib[0] = CTL_HW;
    >  	mib[1] = HW_PHYSMEM64;
    > @@ -1878,6 +1925,9 @@ pfctl_set_limit(struct pfctl *pf, const char *opt, unsigned int limit)
    >  	if (pf->opts & PF_OPT_VERBOSE)
    >  		printf("set limit %s %d\n", opt, limit);
    >  
    > +	if ((pf->opts & PF_OPT_NOACTION) == 0)
    > +		pfctl_load_options(pf);
    > +
    >  	return (0);
    >  }
    >  
    > @@ -2730,10 +2780,13 @@ main(int argc, char *argv[])
    >  		dev = open(pf_device, mode);
    >  		if (dev == -1)
    >  			err(1, "%s", pf_device);
    > +		pfctl_read_limits(dev);
    > +		atexit(pfctl_restore_limits);
    >  	} else {
    >  		dev = open(pf_device, O_RDONLY);
    >  		if (dev >= 0)
    >  			opts |= PF_OPT_DUMMYACTION;
    > +		pfctl_read_limits(dev);
    >  		/* turn off options */
    >  		opts &= ~ (PF_OPT_DISABLE | PF_OPT_ENABLE);
    >  		clearopt = showopt = debugopt = NULL;
    > @@ -2951,7 +3004,16 @@ main(int argc, char *argv[])
    >  	if (lfile != NULL)
    >  		pfctl_state_load(dev, lfile);
    >  
    > -	exit(exit_val);
    > +	/*
    > +	 * prevent pfctl_restore_limits() exit handler from restoring
    > +	 * pf(4) options settings on successful exit.
    > +	 */
    > +	if (exit_val == 0) {
    > +		close(dev);
    > +		dev = -1;
    > +	}
    > +
    > +	return exit_val;
    >  }
    >  #endif	/* REGRESS_NOMAIN */
    >  
    > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
    > index 1e9a51e357b..6478aedc7b2 100644
    > --- a/sys/net/pf_ioctl.c
    > +++ b/sys/net/pf_ioctl.c
    > @@ -2140,8 +2140,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
    >  			goto fail;
    >  		}
    >  
    > -		pf_pool_limits[pl->index].limit_new = pl->limit;
    > -		pl->limit = pf_pool_limits[pl->index].limit;
    > +		error = pool_sethardlimit(pf_pool_limits[pl->index].pp,
    > +		    pl->limit, NULL, 0);
    > +		if (error == 0) {
    > +			pf_pool_limits[pl->index].limit_new = pl->limit;
    > +			pf_pool_limits[pl->index].limit = pl->limit;
    > +		}
    >  		PF_UNLOCK();
    >  		break;
    >  	}
    > @@ -2702,21 +2706,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
    >  		NET_LOCK();
    >  		PF_LOCK();
    >  
    > -		/*
    > -		 * Checked already in DIOCSETLIMIT, but check again as the
    > -		 * situation might have changed.
    > -		 */
    > -		for (i = 0; i < PF_LIMIT_MAX; i++) {
    > -			if (((struct pool *)pf_pool_limits[i].pp)->pr_nout >
    > -			    pf_pool_limits[i].limit_new) {
    > -				PF_UNLOCK();
    > -				NET_UNLOCK();
    > -				free(table, M_PF, sizeof(*table));
    > -				free(ioe, M_PF, sizeof(*ioe));
    > -				error = EBUSY;
    > -				goto fail;
    > -			}
    > -		}
    >  		/* now do the commit - no errors should happen here */
    >  		for (i = 0; i < io->size; i++) {
    >  			PF_UNLOCK();
    > @@ -2769,20 +2758,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
    >  				goto fail; /* really bad */
    >  			}
    >  		}
    > -		for (i = 0; i < PF_LIMIT_MAX; i++) {
    > -			if (pf_pool_limits[i].limit_new !=
    > -			    pf_pool_limits[i].limit &&
    > -			    pool_sethardlimit(pf_pool_limits[i].pp,
    > -			    pf_pool_limits[i].limit_new, NULL, 0) != 0) {
    > -				PF_UNLOCK();
    > -				NET_UNLOCK();
    > -				free(table, M_PF, sizeof(*table));
    > -				free(ioe, M_PF, sizeof(*ioe));
    > -				error = EBUSY;
    > -				goto fail; /* really bad */
    > -			}
    > -			pf_pool_limits[i].limit = pf_pool_limits[i].limit_new;
    > -		}
    >  		for (i = 0; i < PFTM_MAX; i++) {
    >  			int old = pf_default_rule.timeout[i];
    >  
    
    
  • Alexandr Nedvedicky:

    Error when at startup more than 512 anchors are loaded from pf.conf