On Sat, May 03, 2025 at 10:17:34PM +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> below is a diff I'd like to commit to address this issue.
> currently we update the limit when committing transaction
> which does not work when we need to increase the limit
> in order to load the rules.
> 
> I'd like to set the limit immediately as soon as particular
> 'set limit ...' statement gets accepted by parser.
> 
> The proposed change also does best effort to reset the limits
> back to their original values when the configuration file
> can not be processed successfully. The idea here is to read
> current limits from pf(4) driver and arm atexit(3) handler which
> eventually reverts the changes if anything goes wrong. On the
> other hand if configuration gets processed successfully the main()
> functions neuters the atexit(3) handler by closing /dev/pf to
> keep any limit changes in.
> 
> OK?

passes regress

OK bluhm@

one remark: At the end of main we prefer return(exit_val) over
exit(exit_val).  Then stack protector runs again and has a chance
to find bugs.  Just seen in the context, not part of your diff.

> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index 825bd4cf04c..0fdb5269fd1 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)
> @@ -1685,8 +1720,6 @@ pfctl_rules(int dev, char *filename, int opts, int optimize,
>  		pfctl_clear_queues(&rootqs);
>  
>  		if ((opts & PF_OPT_NOACTION) == 0) {
> -			if (!anchorname[0] && pfctl_load_options(&pf))
> -				goto _error;
>  			if (pfctl_trans(dev, t, DIOCXCOMMIT, osize))
>  				ERR("DIOCXCOMMIT");
>  		}
> @@ -1878,6 +1911,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,6 +2766,8 @@ 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)
> @@ -2951,6 +2989,15 @@ main(int argc, char *argv[])
>  	if (lfile != NULL)
>  		pfctl_state_load(dev, lfile);
>  
> +	/*
> +	 * prevent pfctl_restore_limits() exit handler from restoring
> +	 * pf(4) options settings on successful exit.
> +	 */
> +	if (exit_val == 0) {
> +		close(dev);
> +		dev = -1;
> +	}
> +
>  	exit(exit_val);
>  }
>  #endif	/* REGRESS_NOMAIN */
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index e7591ab4d40..16f8719386b 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];