Rework HFSC vs FQ-CoDel checks

The selection mechanism introduced in pf_ioctl.c -r1.316 suffers
from being too ambiguous and lacks robustness. Instead of relying
on composition of multiple flags in the queue specification, it's
easier to identify the root class (if it exists) and derive all
further checks from it.

5 files changed, 31 insertions, 19 deletions

diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index f45c0980e8b..5ab84c9fa8e 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.660 2017/05/28 15:15:21 akfaew Exp $	*/
+/*	$OpenBSD: parse.y,v 1.661 2017/07/19 12:51:30 mikeb Exp $	*/
 
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
@@ -395,7 +395,7 @@ typedef struct {
 		int			 i;
 		char			*string;
 		u_int			 rtableid;
-		u_int16_t		 weight;		
+		u_int16_t		 weight;
 		struct {
 			u_int8_t	 b1;
 			u_int8_t	 b2;
@@ -1304,10 +1304,6 @@ queuespec	: QUEUE STRING interface queue_opts		{
 				yyerror("root queue without interface");
 				YYERROR;
 			}
-			if ($2[0] == '_') {
-				yyerror("queue names must not start with _");
-				YYERROR;
-			}
 			expand_queue($2, $3, &$4);
 		}
 		;
@@ -4343,8 +4339,11 @@ expand_queue(char *qname, struct node_if *interfaces, struct queue_opts *opts)
 
 	LOOP_THROUGH(struct node_if, interface, interfaces,
 		bzero(&qspec, sizeof(qspec));
-		if ((opts->flags & PFQS_FLOWQUEUE) && opts->parent) {
-			yyerror("discipline doesn't support hierarchy");
+		if (!opts->parent && (opts->marker & QOM_BWSPEC))
+			opts->flags |= PFQS_ROOTCLASS;
+		if (!(opts->marker & QOM_BWSPEC) &&
+		    !(opts->marker & QOM_FLOWS)) {
+			yyerror("no bandwidth or flow specification");
 			return (1);
 		}
 		if (strlcpy(qspec.qname, qname, sizeof(qspec.qname)) >=
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index a98604fd41f..e97a7f3f213 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl.c,v 1.345 2017/06/16 19:59:13 awolk Exp $ */
+/*	$OpenBSD: pfctl.c,v 1.346 2017/07/19 12:51:30 mikeb Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1406,7 +1406,17 @@ pfctl_check_qassignments(struct pf_ruleset *rs)
 	if (rs->anchor->path[0] == 0) {
 		TAILQ_FOREACH(qi, &rootqs, entries) {
 			flags = pfctl_find_childqs(qi);
-			if (!(qi->qs.flags & PFQS_FLOWQUEUE) &&
+			if (!(qi->qs.flags & PFQS_ROOTCLASS) &&
+			    !TAILQ_EMPTY(&qi->children)) {
+				if (qi->qs.flags & PFQS_FLOWQUEUE)
+					errx(1, "root queue %s doesn't "
+					    "support hierarchy",
+					    qi->qs.qname);
+				else
+					errx(1, "no bandwidth was specified "
+					    "for root queue %s", qi->qs.qname);
+			}
+			if ((qi->qs.flags & PFQS_ROOTCLASS) &&
 			    !(flags & PFQS_DEFAULT))
 				errx(1, "no default queue specified");
 		}
diff --git a/sbin/pfctl/pfctl_queue.c b/sbin/pfctl/pfctl_queue.c
index feeeba33f8d..2b686defce7 100644
--- a/sbin/pfctl/pfctl_queue.c
+++ b/sbin/pfctl/pfctl_queue.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_queue.c,v 1.5 2017/05/15 16:24:44 mikeb Exp $ */
+/*	$OpenBSD: pfctl_queue.c,v 1.6 2017/07/19 12:51:30 mikeb Exp $ */
 
 /*
  * Copyright (c) 2003 - 2013 Henning Brauer <henning@openbsd.org>
@@ -212,7 +212,8 @@ pfctl_print_queue_nodestat(int dev, const struct pfctl_queue_node *node)
 	    (unsigned long long)stats->xmit_cnt.bytes,
 	    (unsigned long long)stats->drop_cnt.packets,
 	    (unsigned long long)stats->drop_cnt.bytes);
-	if (node->qs.flags & PFQS_FLOWQUEUE) {
+	if (node->qs.parent_qid == 0 && (node->qs.flags & PFQS_FLOWQUEUE) &&
+	    !(node->qs.flags & PFQS_ROOTCLASS)) {
 		double avg = 0, dev = 0;
 
 		if (fqstats->flows > 0) {
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 7c9df05a53b..4661c897487 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf_ioctl.c,v 1.318 2017/07/05 11:40:17 bluhm Exp $ */
+/*	$OpenBSD: pf_ioctl.c,v 1.319 2017/07/19 12:51:30 mikeb Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -606,12 +606,12 @@ pf_create_queues(void)
 		qif = malloc(sizeof(*qif), M_TEMP, M_WAITOK);
 		qif->ifp = ifp;
 
-		if ((q->flags & PFQS_FLOWQUEUE) && !(q->flags & PFQS_DEFAULT)) {
-			qif->ifqops = ifq_fqcodel_ops;
-			qif->pfqops = pfq_fqcodel_ops;
-		} else {
+		if (q->flags & PFQS_ROOTCLASS) {
 			qif->ifqops = ifq_hfsc_ops;
 			qif->pfqops = pfq_hfsc_ops;
+		} else {
+			qif->ifqops = ifq_fqcodel_ops;
+			qif->pfqops = pfq_fqcodel_ops;
 		}
 
 		qif->disc = qif->pfqops->pfq_alloc(ifp);
@@ -1104,8 +1104,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			break;
 		}
 		bcopy(qs, &pq->queue, sizeof(pq->queue));
+		/* It's a root flow queue but is not an HFSC root class */
 		if ((qs->flags & PFQS_FLOWQUEUE) && qs->parent_qid == 0 &&
-		    !(qs->flags & PFQS_DEFAULT))
+		    !(qs->flags & PFQS_ROOTCLASS))
 			error = pfq_fqcodel_ops->pfq_qstats(qs, pq->buf,
 			    &nbytes);
 		else
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 5adcff5042e..4f43739687d 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfvar.h,v 1.460 2017/06/28 19:30:24 mikeb Exp $ */
+/*	$OpenBSD: pfvar.h,v 1.461 2017/07/19 12:51:31 mikeb Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1343,6 +1343,7 @@ struct pf_queuespec {
 };
 
 #define PFQS_FLOWQUEUE			0x0001
+#define PFQS_ROOTCLASS			0x0002
 #define PFQS_DEFAULT			0x1000 /* maps to HFSC_DEFAULTCLASS */
 
 struct priq_opts {