Bug 210924 - 10.3-STABLE - PF - possible regression in pf.conf set timeout interval
Summary: 10.3-STABLE - PF - possible regression in pf.conf set timeout interval
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: conf (show other bugs)
Version: 10.3-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-pf (Nobody)
URL:
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2016-07-08 14:20 UTC by Oliver Peter
Modified: 2016-08-09 03:48 UTC (History)
3 users (show)

See Also:


Attachments
proposed fix for parser.y (1.36 KB, patch)
2016-07-12 13:13 UTC, Oliver Peter
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Peter 2016-07-08 14:20:05 UTC
# pfctl -f /etc/pf.conf
/etc/pf.conf:24: syntax error
pfctl: Syntax error in config file: pf rules not loaded

The problematic line is :
set timeout interval 10

I think that was broken by the commit which added ALTQ support for CoDel

Probably came in by git commit 498601242d037970fd571c0aa7a61a9253e174d3

https://github.com/freebsd/freebsd/commit/498601242d037970fd571c0aa7a61a9253e174d3#diff-e3e24f6a57bb8bee378eabff3cf2eed0R5391
Comment 1 Kristof Provost freebsd_committer freebsd_triage 2016-07-08 15:08:03 UTC
You're quite right.
I believe loos@ was going to look at this.
Comment 2 Oliver Peter 2016-07-12 13:13:43 UTC
Created attachment 172410 [details]
proposed fix for parser.y

INTERVAL is a keyword defined in struct pf_timeout in pfctl_parser.c.
We should stick with documentation and upstream and leave it as it is.

Proposed change will rename INTERVAL to ALTQINTERVAL in latest version.

Is there any documentation that needs to be updated to reflect this change?

Tested on 11-CURRENT/amd64
Comment 3 Kristof Provost freebsd_committer freebsd_triage 2016-07-14 14:26:18 UTC
It's probably a little too late to get away with changing the altq keywords. This has hit 10.3 (and soon 11.0).

It should be possible to teach pfctl that both 'set timeout interval 10' and the new interval option are valid.

I'm not really good with yacc, but this seems to work, even if it looks a little ugly:
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index e0cfa3d..9457a5b 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -4460,6 +4460,19 @@ timeout_spec     : STRING NUMBER
                        }
                        free($1);
                }
+               | INTERVAL NUMBER
+               {
+                       if (check_rulestate(PFCTL_STATE_OPTION)) {
+                               YYERROR;
+                       }
+                       if ($2 < 0 || $2 > UINT_MAX) {
+                               yyerror("only positive values permitted");
+                               YYERROR;
+                       }
+                       if (pfctl_set_timeout(pf, "interval", $2, 0) != 0) {
+                               YYERROR;
+                       }
+               }
                ;

 timeout_list   : timeout_list comma timeout_spec optnl
Comment 4 Oliver Peter 2016-07-19 10:41:07 UTC
(In reply to Kristof Provost from comment #3)
Thanks, of course this is the better approach.
Looks good so far for me:

oliver@wayne pfctl % cat /etc/pf.conf
set timeout interval 5

altq on em0 hfsc bandwidth 1Mb queue { ftp, ssh, icmp, other }
queue ftp bandwidth 30% priority 0 hfsc (upperlimit 99%)
queue ssh bandwidth 30% priority 2 hfsc (upperlimit 99%)
queue icmp bandwidth 10% priority 2 hfsc (upperlimit 99%)
queue other bandwidth 30% priority 1 hfsc (default upperlimit 99%)
pass out quick on em0 inet proto tcp from any port 21 to any queue ftp
pass out quick on em0 inet proto tcp from any port 22 to any queue ssh
pass out quick on em0 inet proto icmp from any to any queue icmp
pass out quick on em0 all

pass keep state

oliver@wayne pfctl % sudo pfctl -f /etc/pf.conf
oliver@wayne pfctl % uname -a
FreeBSD wayne.lab.home.gfuzz.de 12.0-CURRENT FreeBSD 12.0-CURRENT #2 575d5bb(master)-dirty: Mon Jul 18 15:40:49 CEST 2016     root@wayne.lab.home.gfuzz.de:/usr/obj/usr/src/sys/WAYNE  i386



However, since I'm not familiar with the new CODELQ syntax, I'm not able to do extended tests.  Perhaps you could point out some documentation as you mentioned it has been imported into 10.3 already.
Comment 5 Kristof Provost freebsd_committer freebsd_triage 2016-07-19 10:57:58 UTC
Unfortunately the patch to add CODEL doesn't seem to have included any documentation changes. I'm not familiar with it myself.
Comment 6 commit-hook freebsd_committer freebsd_triage 2016-08-05 02:19:30 UTC
A commit references this bug:

Author: loos
Date: Fri Aug  5 02:19:03 UTC 2016
New revision: 303760
URL: https://svnweb.freebsd.org/changeset/base/303760

Log:
  Fix a regression in pf.conf while parsing the 'interval' keyword.

  The bug was introduced by r287009.

  PR:		210924
  Submitted by:	kp@
  Sponsored by:	Rubicon Communications (Netgate)
  Pointy hat to:	loos

Changes:
  head/sbin/pfctl/parse.y
Comment 7 Oliver Peter 2016-08-08 12:40:34 UTC
Thanks guys!
Comment 8 commit-hook freebsd_committer freebsd_triage 2016-08-09 03:40:19 UTC
A commit references this bug:

Author: loos
Date: Tue Aug  9 03:39:21 UTC 2016
New revision: 303864
URL: https://svnweb.freebsd.org/changeset/base/303864

Log:
  MFC r303760:

  Fix a regression in pf.conf while parsing the 'interval' keyword.

  The bug was introduced by r287009.

  PR:		210924
  Submitted by:	kp@
  Sponsored by:	Rubicon Communications (Netgate)
  Pointy hat to:	loos
  Approved by:	re (gjb)

Changes:
_U  stable/11/
  stable/11/sbin/pfctl/parse.y
Comment 9 commit-hook freebsd_committer freebsd_triage 2016-08-09 03:48:21 UTC
A commit references this bug:

Author: loos
Date: Tue Aug  9 03:47:38 UTC 2016
New revision: 303865
URL: https://svnweb.freebsd.org/changeset/base/303865

Log:
  MFC r303760:

  Fix a regression in pf.conf while parsing the 'interval' keyword.

  The bug was introduced by r287009.

  PR:		210924
  Submitted by:	kp@
  Sponsored by:	Rubicon Communications (Netgate)

Changes:
_U  stable/10/
  stable/10/sbin/pfctl/parse.y