Re: pf: honor quick on anchor rules
On Fri, Oct 05, 2018 at 11:53:08PM +0200, Klemens Nanni wrote:
> On Fri, Oct 05, 2018 at 10:38:48PM +0200, Fabian Mueller-Knapp wrote:
> > If i read man correctly it means "evaluate the rules inside and stop if
> > any rule within matched".
> While it's own description is quite clear and unambigious:
>
> quick If a packet matches a rule which has the quick option set, this
> rule is considered the last matching rule, and evaluation of
> subsequent rules is skipped.
>
> the anchor specific addition can be improved indeed:
>
> If the anchor itself is marked with the quick option, ruleset
> evaluation will terminate when the anchor is exited if the packet is
> matched by any rule within the anchor.
>
> Does an empty ruleset match a packet? If so, and our code technically
> does so, -current including my latest fix behaves correctly according
> to the documentation.
Admittedly, I should have read the second paragraph more carefully.
> > With the current fix, rule evaluation stops after the first block,
> > regardless of matching. Therefore the following currently always passes:
> >
> > snap# uname -a
> > OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> > snap# pfctl -sr
> >
> > pass all flags S/SA
> > anchor quick all {
> > }
> > block drop all
> >
> > 6.1 behaves as i would expect, it drops all. Instead of the empty block,
> > any non-matching rule has the same effect.
> While rules within the anchor do not match, the anchor itself does.
> After all, it is a rule itself just like `pass' or `block', hence your
> ruleset's second rule `anchor quick' matches every packet and therefore
> stops evaluation.
When mcbride introduced `anchor quick' back in 2006 it was indeed bound
to matches within the ruleset:
commit a81eb4e4f7ac27afb4708cf434aee361f1ef8b19
Author: mcbride
Date: Wed Oct 11 08:42:31 2006 +
Allow the 'quick' keyword on an anchor. IFF there is a matching
rule inside
the anchor, terminate ruleset evaluation when stepping out of the
anchor.
This means that if you absolutely want the anchor to be terminal,
you
probably want to use a 'block all' or 'pass all' rule at the start
of the
anchor.
ok dhartmei@ henning@ deraadt@
Digging through commits and the history of PF already takes more of my
time than I anticipated. Some time after 2006, or more specifically
after 6.1 being released, this behaviour got broken^Wchanged - most
likely unintionally.
Since our code now treats `anchor quick' like any other normal rule
(see my other reply to benno in this thread), I suggest we adjust our
documentation so users will not fall for it again.
Fixing^Wchaning this behaviour is debatable but should probably not
happen before 6.4 comes out anyway, so I suggest we better ship with
a correct manual.
Feedback? OK?
We might as well remove the entire sentence to not special case anchors
at all anymore.
Index: pf.conf.5
===
RCS file: /cvs/src/share/man/man5/pf.conf.5,v
retrieving revision 1.577
diff -u -p -r1.577 pf.conf.5
--- pf.conf.5 12 Jul 2018 05:54:49 - 1.577
+++ pf.conf.5 7 Oct 2018 17:40:07 -
@@ -1865,8 +1865,7 @@ anchors and the main ruleset.
If the anchor itself is marked with the
.Cm quick
option,
-ruleset evaluation will terminate when the anchor is exited if the packet is
-matched by any rule within the anchor.
+ruleset evaluation will terminate when the anchor is exited.
.Pp
An anchor references other anchor attachment points
using the following syntax:
Re: unveil(2) getent(1)
On Mon, Sep 24, 2018 at 09:33:50AM -0600, Todd C. Miller wrote: > I wonder if we can do unveil(NULL, NULL) for getent databases without > an explicit file. A quick test seems to work for dns. Same thought here at first, but we're pledging without "unveil" promise after unveiling files so no need to for that.
Re: unveil(2) getent(1)
On Mon, Sep 24, 2018 at 10:49:42AM +0100, Ricardo Mestre wrote:
> Comments? OK? The initial pledge(2) is so short lived that I was tempted to
> remove it, but I'm open to suggestions :)
Is there any compelling reason to keep the initial superset pledge?
Without it, the only code paths without pledge/unveil are either
main()->usage()->exit() or main()->{all failing strcmp() loop}->return.
getent: adjust alignment in hostsprint()
hostsprint() reserves only 16 columns for IPs and prints one whitespace too many afterwards: $ getent hosts 1.1.1.1 long ::::::: 1.1.1.1 one.one.one.one ::::::: long ::::::: long This diff cranks it up to 39 as per hostsaddrinfo() and aligns nicely: $ ./obj/getent hosts 1.1.1.1 long ::::::: 1.1.1.1 one.one.one.one ::::::: long ::::::: long OK? Index: getent.c === RCS file: /cvs/src/usr.bin/getent/getent.c,v retrieving revision 1.18 diff -u -p -r1.18 getent.c --- getent.c25 Sep 2018 19:51:39 - 1.18 +++ getent.c26 Sep 2018 13:48:49 - @@ -225,7 +225,7 @@ hostsprint(const struct hostent *he) if (inet_ntop(he->h_addrtype, he->h_addr, buf, sizeof(buf)) == NULL) strlcpy(buf, "# unknown", sizeof(buf)); - printfmtstrings(he->h_aliases, " ", " ", "%-16s %s", buf, he->h_name); + printfmtstrings(he->h_aliases, " ", " ", "%-39s %s", buf, he->h_name); } static int hostsaddrinfo(const char *name)
Re: getent: use more appropiate types/limits around strtonum()
On Wed, Sep 26, 2018 at 06:48:07AM -0600, Todd C. Miller wrote:
> One comment inline, otherwise OK millert@
> > @@ -397,6 +397,9 @@ static int
> > services(int argc, char *argv[])
> > {
> > struct servent *se;
> > + const char *err;
> > + char*proto;
> > + int serv;
>
> This should be in_port_t, not int. I would also call the variable
> port instead of serv but the type is what is important.
Missed that one, thanks.
I'll commit it with `in_port_t port' later this evening unless someone
objects.
getent: usage() is void
OK?
Index: getent.c
===
RCS file: /cvs/src/usr.bin/getent/getent.c,v
retrieving revision 1.16
diff -u -p -r1.16 getent.c
--- getent.c25 Sep 2018 06:48:48 - 1.16
+++ getent.c25 Sep 2018 19:41:04 -
@@ -55,7 +55,7 @@
#include
-static int usage(void);
+static voidusage(void);
static int ethers(int, char *[]);
static int group(int, char *[]);
static int hosts(int, char *[]);
@@ -115,12 +115,11 @@ main(int argc, char *argv[])
return RV_USAGE;
}
-static int
+static void
usage(void)
{
fprintf(stderr, "usage: %s database [key ...]\n", __progname);
exit(RV_USAGE);
- /* NOTREACHED */
}
/*
getent: hostsaddrinfo(): use getnameinfo(2)
hostsaddrinfo() is called from hosts() for non-IP keys, e.g.
`getent hosts foo openbsd.org'.
Using getnameinfo(2) simplifies the code, makes it less address family
specific and plays nicely with previously used getaddrinfo(2).
While here, make function paramter `const', sort stack variables by size
nit-pick PF_UNSPEC.
No observable change in behaviour/output.
OK?
Index: getent.c
===
RCS file: /cvs/src/usr.bin/getent/getent.c,v
retrieving revision 1.14
diff -u -p -r1.14 getent.c
--- getent.c1 Feb 2016 19:57:28 - 1.14
+++ getent.c24 Sep 2018 23:00:14 -
@@ -227,42 +227,30 @@ hostsprint(const struct hostent *he)
printfmtstrings(he->h_aliases, " ", " ", "%-16s %s", buf, he->h_name);
}
static int
-hostsaddrinfo(char* name)
+hostsaddrinfo(const char *name)
{
struct addrinfo hints, *res, *res0;
- void*src;
- int rv;
char buf[INET6_ADDRSTRLEN];
+ int rv;
rv = RV_NOTFOUND;
memset(buf, 0, sizeof(buf));
memset(, 0, sizeof(hints));
- hints.ai_family = PF_UNSPEC;
+ hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_DGRAM;
- if (getaddrinfo(name, NULL, , ) == 0) {
- for (res = res0; res; res = res->ai_next) {
- switch (res->ai_family) {
- case AF_INET:
- src = &((struct sockaddr_in*)
- res->ai_addr)->sin_addr;
- break;
- case AF_INET6:
- src = &((struct sockaddr_in6*)
- res->ai_addr)->sin6_addr;
- break;
- default: /* not reached */
- src = NULL;
- }
- if (src==NULL || inet_ntop(res->ai_family, src, buf,
- sizeof(buf)) == NULL)
- strlcpy(buf, "# unknown", sizeof(buf));
- else
- rv = RV_OK;
- printf("%-39s %s\n", buf, name);
- }
- freeaddrinfo(res0);
+ if (getaddrinfo(name, NULL, , ) != 0)
+ return (rv);
+ for (res = res0; res; res = res->ai_next) {
+ if ((res->ai_family != AF_INET6 && res->ai_family != AF_INET) ||
+ getnameinfo(res->ai_addr, res->ai_addrlen, buf, sizeof(buf),
+ NULL, 0, NI_NUMERICHOST) != 0)
+ strlcpy(buf, "# unknown", sizeof(buf));
+ else
+ rv = RV_OK;
+ printf("%-39s %s\n", buf, name);
}
+ freeaddrinfo(res0);
return (rv);
}
getent: use more appropiate types/limits around strtonum()
Replace `long long id' with appropiate types and names, use smaller
limits where applicable and move variable declarations up out of loops.
This makes the code clearer and a tad simpler while staying consistent
across databases.
Feedback? OK?
Index: getent.c
===
RCS file: /cvs/src/usr.bin/getent/getent.c,v
retrieving revision 1.18
diff -u -p -r1.18 getent.c
--- getent.c25 Sep 2018 19:51:39 - 1.18
+++ getent.c25 Sep 2018 22:21:22 -
@@ -190,8 +190,10 @@ ethers(int argc, char *argv[])
static int
group(int argc, char *argv[])
{
- int i, rv = RV_OK;
struct group*gr;
+ const char *err;
+ gid_t gid;
+ int i, rv = RV_OK;
setgroupent(1);
if (argc == 2) {
@@ -199,11 +201,9 @@ group(int argc, char *argv[])
GROUPPRINT;
} else {
for (i = 2; i < argc; i++) {
- const char *err;
- long long id = strtonum(argv[i], 0, UINT_MAX, );
-
+ gid = strtonum(argv[i], 0, GID_MAX, );
if (!err)
- gr = getgrgid((gid_t)id);
+ gr = getgrgid(gid);
else
gr = getgrnam(argv[i]);
if (gr != NULL)
@@ -291,8 +291,10 @@ hosts(int argc, char *argv[])
static int
passwd(int argc, char *argv[])
{
- int i, rv = RV_OK;
struct passwd *pw;
+ const char *err;
+ uid_t uid;
+ int i, rv = RV_OK;
setpassent(1);
if (argc == 2) {
@@ -300,11 +302,9 @@ passwd(int argc, char *argv[])
PASSWDPRINT;
} else {
for (i = 2; i < argc; i++) {
- const char *err;
- long long id = strtonum(argv[i], 0, UINT_MAX, );
-
+ uid = strtonum(argv[i], 0, UID_MAX, );
if (!err)
- pw = getpwuid((uid_t)id);
+ pw = getpwuid(uid);
else
pw = getpwnam(argv[i]);
if (pw != NULL)
@@ -327,6 +327,8 @@ static int
protocols(int argc, char *argv[])
{
struct protoent *pe;
+ const char *err;
+ int proto;
int i, rv = RV_OK;
setprotoent(1);
@@ -335,11 +337,9 @@ protocols(int argc, char *argv[])
PROTOCOLSPRINT;
} else {
for (i = 2; i < argc; i++) {
- const char *err;
- long long id = strtonum(argv[i], 0, UINT_MAX, );
-
+ proto = strtonum(argv[i], 0, INT_MAX, );
if (!err)
- pe = getprotobynumber((int)id);
+ pe = getprotobynumber(proto);
else
pe = getprotobyname(argv[i]);
if (pe != NULL)
@@ -362,6 +362,8 @@ static int
rpc(int argc, char *argv[])
{
struct rpcent *re;
+ const char *err;
+ int rpc;
int i, rv = RV_OK;
setrpcent(1);
@@ -370,11 +372,9 @@ rpc(int argc, char *argv[])
RPCPRINT;
} else {
for (i = 2; i < argc; i++) {
- const char *err;
- long long id = strtonum(argv[i], 0, UINT_MAX, );
-
+ rpc = strtonum(argv[i], 0, INT_MAX, );
if (!err)
- re = getrpcbynumber((int)id);
+ re = getrpcbynumber(rpc);
else
re = getrpcbyname(argv[i]);
if (re != NULL)
@@ -397,6 +397,9 @@ static int
services(int argc, char *argv[])
{
struct servent *se;
+ const char *err;
+ char*proto;
+ int serv;
int i, rv = RV_OK;
setservent(1);
@@ -405,15 +408,11 @@ services(int argc, char *argv[])
SERVICESPRINT;
} else {
for (i = 2; i < argc; i++) {
- const char *err;
- long long id;
- char *proto = strchr(argv[i], '/');
-
- if (proto != NULL)
+ if ((proto = strchr(argv[i], '/')) != NULL)
*proto++ = '\0';
- id = strtonum(argv[i], 0, UINT_MAX, );
+ serv = strtonum(argv[i], 0, IPPORT_HILASTAUTO, );
if (!err)
- se =
Re: unveil(2) getent(1)
On Mon, Sep 24, 2018 at 08:56:14PM +0100, Ricardo Mestre wrote: > I actually prefer to see it go away since it doesn't protect us much and the > real meat is actually on the pledge(2) inside the loop. Nevertheless this > still > should on a separate commit. OK kn
pfctl: tables: improve namespace collision warnings
Tables under different anchors may have the same name, but pfctl warns
about such scenarios upon table creation to avoid mixups. Unique and
descriptive names are highly recommended (for sanity).
# pfctl -T replace -t t1
1 table created.
no changes.
# pfctl -T replace -t t1 -a a1
pfctl: warning: namespace collision with global table.
1 table created.
no changes.
Adding a table to yet another anchor will only ever warn about the one
in the main anchor:
# pfctl -T replace -t t1 -a a2
pfctl: warning: namespace collision with global table.
1 table created.
no changes.
Adding equally named tables in non-main anchors only will never produce
warnings:
# pfctl -T add -t t2 -a a1
1 table created.
0/0 addresses added.
# pfctl -T add -t t2 -a a2
1 table created.
0/0 addresses added.
Things to improve:
- spot all collisions
- warn on dry runs (`-n') as well
- name offending anchors
For this the anchor name must be passed in, but this is impossible to
do from pfctl's main() upon processing `-f file', where it's done now
since the file may contain table definitions.
This diff moves the call to process_tabledefs() inside the parser where
it's only called when tables actually occur. This way both table and
anchor name is known and warn_namespace_collision()'s `filter == NULL'
semantic can be dropped to further simplify the function.
Besides new warnings on standard error, there's no functional change as
ruleset production/manipulation is not effected; regress passes.
# ./obj/pfctl -T replace -t t3
1 table created.
no changes.
# ./obj/pfctl -T replace -t t3 -a a1
pfctl: warning: table already defined in anchor "/"
1 table created.
no changes.
# ./obj/pfctl -T replace -t t3 -a a2 -n
pfctl: warning: table already defined in anchor "/"
pfctl: warning: table already defined in anchor "a1"
1 table created (dummy).
The main anchor is denoted as "/" as it keeps the logic around warnx(3)
simple while still allowing to copy/paste it as is.
Since it's not just about collisions with the "global" tables (those in
the main anchor), I renamed the function to warn_duplicate_tables() at
no cost/blame churn since signature and callers are touched anyway.
Feedback? Objections? OK?
Index: pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.57
diff -u -p -r1.57 pfctl.h
--- pfctl.h 6 Sep 2018 15:07:33 - 1.57
+++ pfctl.h 29 Dec 2018 15:59:11 -
@@ -79,7 +79,7 @@ void pfctl_clear_tables(const char *, i
voidpfctl_show_tables(const char *, int);
int pfctl_command_tables(int, char *[], char *, const char *, char *,
const char *, int);
-voidwarn_namespace_collision(const char *);
+voidwarn_duplicate_tables(const char *, const char *);
voidpfctl_show_ifaces(const char *, int);
FILE *pfctl_fopen(const char *, const char *);
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.361
diff -u -p -r1.361 pfctl.c
--- pfctl.c 27 Dec 2018 16:33:44 - 1.361
+++ pfctl.c 29 Dec 2018 15:59:11 -
@@ -2690,8 +2690,6 @@ main(int argc, char *argv[])
if (pfctl_rules(dev, rulesopt, opts, optimize,
anchorname, NULL))
error = 1;
- else if (!(opts & PF_OPT_NOACTION))
- warn_namespace_collision(NULL);
}
if (opts & PF_OPT_ENABLE)
Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.688
diff -u -p -r1.688 parse.y
--- parse.y 15 Nov 2018 03:22:01 - 1.688
+++ parse.y 29 Dec 2018 15:59:28 -
@@ -4075,6 +4075,7 @@ process_tabledef(char *name, struct tabl
if (pf->opts & PF_OPT_VERBOSE)
print_tabledef(name, opts->flags, opts->init_addr,
>init_nodes);
+ warn_duplicate_tables(name, pf->anchor->path);
if (!(pf->opts & PF_OPT_NOACTION) &&
pfctl_define_table(name, opts->flags, opts->init_addr,
pf->anchor->path, , pf->anchor->ruleset.tticket)) {
Index: pfctl_table.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_table.c,v
retrieving revision 1.78
diff -u -p -r1.78 pfctl_table.c
--- pfctl_table.c 15 Oct 2018 21:15:35 - 1.78
+++ pfctl_table.c 29 Dec 2018 15:59:21 -
@@ -94,7 +94,8 @@ static const char *istats_text[2][2][2]
goto _error;\
} \
if (nadd) {
Re: iSerialNumber -> serial
On Thu, Dec 27, 2018 at 02:36:36PM -0200, Martin Pieuchot wrote: > New version using 'iSerial'. This is coherent w/ what lsusb(8) displays > and isn't ambiguous with regard to the device descriptor name as pointed > out by deraadt@. Even better, sure.
pfctl: zap unused struct segment
There since import and last used by ALTQ which henning removed in 2004.
OK?
Index: sbin/pfctl//pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.57
diff -u -p -r1.57 pfctl.h
--- sbin/pfctl//pfctl.h 6 Sep 2018 15:07:33 - 1.57
+++ sbin/pfctl//pfctl.h 1 Jan 2019 21:23:02 -
@@ -83,14 +83,6 @@ void warn_namespace_collision(const cha
voidpfctl_show_ifaces(const char *, int);
FILE *pfctl_fopen(const char *, const char *);
-/*
- * generalized service curve used for admission control
- */
-struct segment {
- LIST_ENTRY(segment) _next;
- double x, y, d, m;
-};
-
voidprint_addr(struct pf_addr_wrap *, sa_family_t, int);
voidprint_addr_str(sa_family_t, struct pf_addr *);
voidprint_host(struct pf_addr *, u_int16_t p, sa_family_t, u_int16_t,
const char *, int);
Re: uudecode error message
On Sun, Dec 30, 2018 at 12:19:54PM +0100, Alexander Bluhm wrote: > uudecode: in: out: character value (159) out of range [33-96] OK
uu{de,en}code: zap
and are required for b64_ntop()/b64_pton(), but there's nothing socket related here as far as I'm concerned. Included since millert's sync in 2004 with FreeBSD which still has it. No object change on amd64 and sparc64. Feedback? OK? Index: usr.bin/uudecode/uudecode.c === RCS file: /cvs/src/usr.bin/uudecode/uudecode.c,v retrieving revision 1.23 diff -u -p -r1.23 uudecode.c --- usr.bin/uudecode/uudecode.c 3 Jan 2016 14:43:20 - 1.23 +++ usr.bin/uudecode/uudecode.c 30 Dec 2018 02:11:51 - @@ -35,7 +35,6 @@ * Used with uuencode. */ -#include #include #include Index: usr.bin/uuencode/uuencode.c === RCS file: /cvs/src/usr.bin/uuencode/uuencode.c,v retrieving revision 1.13 diff -u -p -r1.13 uuencode.c --- usr.bin/uuencode/uuencode.c 9 Oct 2015 01:37:09 - 1.13 +++ usr.bin/uuencode/uuencode.c 30 Dec 2018 02:11:49 - @@ -34,7 +34,6 @@ * Encode a file so it can be mailed to a remote system. */ -#include #include #include
pfctl: bail out early on missing table command, zap wrapper
Synopsis is `[-t table -T command [address ...]]', yet tables without
commands are silently ignored:
$ pfctl -t t
pfctl: /dev/pf: Permission denied
# pfctl -t t ; echo $?
0
Commands without tables are catched, but only after opening pf(4):
$ pfctl -T show
pfctl: /dev/pf: Permission denied
# pfctl -T show
pfctl [-deghNnPqrvz] [-a anchor] [-D macro=value] [-F modifier] [-f
file]
[-i interface] [-K key] [-k key] [-L statefile] [-o level] [-p
device]
[-S statefile] [-s modifier [-R id]] [-t table -T command
[address ...]]
[-V rdomain] [-x level]
1
By moving the inter-dependence check right after option parsing is done,
we can bail out much earlier and drop the internal wrapper
pfctl_command_tables() as unneeded indirection with now duplicate checks.
With this diff both examples will show usage without doing anything as
expected. There's no other user of this wrapper.
Feedback? OK?
Index: sbin/pfctl//pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.361
diff -u -p -r1.361 pfctl.c
--- sbin/pfctl//pfctl.c 27 Dec 2018 16:33:44 - 1.361
+++ sbin/pfctl//pfctl.c 1 Jan 2019 19:01:23 -
@@ -2482,6 +2482,9 @@ main(int argc, char *argv[])
}
}
+ if (tblcmdopt == NULL ^ tableopt == NULL)
+ usage();
+
if (tblcmdopt != NULL) {
argc -= optind;
argv += optind;
@@ -2661,7 +2664,7 @@ main(int argc, char *argv[])
pfctl_kill_src_nodes(dev, ifaceopt, opts);
if (tblcmdopt != NULL) {
- error = pfctl_command_tables(argc, argv, tableopt,
+ error = pfctl_table(argc, argv, tableopt,
tblcmdopt, rulesopt, anchorname, opts);
rulesopt = NULL;
}
Index: sbin/pfctl//pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.57
diff -u -p -r1.57 pfctl.h
--- sbin/pfctl//pfctl.h 6 Sep 2018 15:07:33 - 1.57
+++ sbin/pfctl//pfctl.h 1 Jan 2019 19:14:36 -
@@ -77,7 +77,7 @@ intpfi_clr_istats(const char *, int *,
voidpfctl_print_title(char *);
voidpfctl_clear_tables(const char *, int);
voidpfctl_show_tables(const char *, int);
-int pfctl_command_tables(int, char *[], char *, const char *, char *,
+int pfctl_table(int, char *[], char *, const char *, char *,
const char *, int);
voidwarn_namespace_collision(const char *);
voidpfctl_show_ifaces(const char *, int);
Index: sbin/pfctl//pfctl_table.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_table.c,v
retrieving revision 1.78
diff -u -p -r1.78 pfctl_table.c
--- sbin/pfctl//pfctl_table.c 15 Oct 2018 21:15:35 - 1.78
+++ sbin/pfctl//pfctl_table.c 1 Jan 2019 18:59:55 -
@@ -54,8 +54,6 @@
#include "pfctl.h"
extern voidusage(void);
-static int pfctl_table(int, char *[], char *, const char *, char *,
- const char *, int);
static voidprint_table(struct pfr_table *, int, int);
static voidprint_tstats(struct pfr_tstats *, int);
static int load_addr(struct pfr_buffer *, int, char *[], char *, int, int);
@@ -114,15 +112,6 @@ pfctl_show_tables(const char *anchor, in
{
if (pfctl_table(0, NULL, NULL, "-s", NULL, anchor, opts) == -1)
exit(1);
-}
-
-int
-pfctl_command_tables(int argc, char *argv[], char *tname,
-const char *command, char *file, const char *anchor, int opts)
-{
- if (tname == NULL || command == NULL)
- usage();
- return pfctl_table(argc, argv, tname, command, file, anchor, opts);
}
int
Re: pfctl: tables: improve namespace collision warnings
On Wed, Jan 02, 2019 at 11:27:18PM +0100, Alexandr Nedvedicky wrote: > I don't object your change. However I hesitate to give OK too. I hope PF > users, who have non-trivial rulesets will speak up here. Feedback is welcome. > IMO opinion we are hitting limitations of pfctl(8) here. Making warnings > more useful requires to introduce some additional hints to pfctl, to > better express, which table should be bound to rule. This does not seem like a limitation to me. > Currently pfctl(8) tries to use table, which is attached to anchor. > If there is no table found, it implicitly fall backs to main anchor > and uses table found in main anchor (ruleset). This implicit fallback > is source of our doubts: > is it intentional the table at anchor is not defined? Pretty sure that's by design. All tables used to be global, until cedric reworked lookups at 30.04.2003 to be hierarchically per anchor. > I would prefer pfctl(8) to always complain if particular table is not defined > in anchor. e.g. if rule refers particular table as: > > pass in from > > then parser should always expect `t1` to be defined in the same anchor > as the rule itself. If no table is found anchor, then parser should > exit with error. Yes, but referring/accepting non-existing tables is a different issue. Now I want to (naively) take care of better collision warnings only. > If user wants rule above to use `t1` from main > anchor then the rule should look like: > > pass in from > > I agree going that way just puts more pain to users for kind of > little gain. What *is* the gain? I don't see it. > > # ./obj/pfctl -T replace -t t3 -a a2 -n > > pfctl: warning: table already defined in anchor "/" > > pfctl: warning: table already defined in anchor "a1" > > 1 table created (dummy). > > > > I just see an use case above from different perspective: > > it's not a problem where particular table is defined, > the tricky question is how do we refer them in rules. Just by name as it's done now, leaving scope to anchors alone. How would the user interface look like? pfctl -a foo -t /t pfctl -a / -t /t pfctl -t t pfctl a foo -t ../bar/t pfctl -t bar/t How should these behave? Thinking about hierarchies in two places makes for a big mess. > As I've said I don't object your change. I agree it does, > what you intend, however I'm not sure how much it buys. My intention is to make warnings clear and unambiguous, such that referred table and anchor names can be copied and pasted into successive pfctl invocations to fix things right away.
pfctl: defuse `-F all -i ...', catch empty argument values
Limiting the "flush all" operation to a specific interface does not
make sense, and the intention was clear as well:
pfctl.c revision 1.298
date: 2010/06/28 23:21:41; author: mcbride; state: Exp; lines: +27
-11;
Clean up iterface stats handling:
- 'make -Fi' reset ALL the interface statistics
can be restricted with -i ifname
- 'make -Fa -i ifname' fail (it's meaningless)
- get rid of a silly little struct that's only used for one thing
ok henning
Yet, tables and rules are always cleared:
# pfctl -F all -i foo
0 tables deleted.
rules cleared
pfctl: don't specify an interface with -Fall
usage: pfctl ...
It even clears statistics for the empty interface:
# pfctl -F all -i ''
0 tables deleted.
rules cleared
8 states cleared
source tracking entries cleared
pf: statistics cleared for interface
pf: interface flags reset
That's unacceptable.
Currently, the idiom `opt && *opt' is used to test for table, key and
interface arguments, but is inherently unable to detect empy values
since the NULL initialised `opt' is unconditionally set to `optarg'
which may be empty.
Diff below bails out immediately when `-i ...' is passed and hoists
empty option argument checks into the getopt(3) routine. All code using
these arguments can now rely on them not being empty if given.
$ ./obj/pfctl -F all -i ''
pfctl: empty interface name
# ./obj/pfctl -F all -i foo
pfctl: don't specify an interface with -Fall
usage: pfctl ...
Feedback? OK?
Regress passes.
NB: parse.y does accept empty strings for anchors, tables, interfaces
and tables and handles them poorly, but that's stuff for another diff.
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.362
diff -u -p -r1.362 pfctl.c
--- pfctl.c 2 Jan 2019 23:08:00 - 1.362
+++ pfctl.c 5 Jan 2019 19:03:07 -
@@ -2342,6 +2342,8 @@ main(int argc, char *argv[])
"a:dD:eqf:F:ghi:k:K:L:Nno:Pp:R:rS:s:t:T:vV:x:z")) != -1) {
switch (ch) {
case 'a':
+ if (!*optarg)
+ errx(1, "emtpy anchor name");
anchoropt = optarg;
break;
case 'd':
@@ -2369,6 +2371,8 @@ main(int argc, char *argv[])
mode = O_RDWR;
break;
case 'i':
+ if (!*optarg)
+ errx(1, "empty interface name");
ifaceopt = optarg;
break;
case 'k':
@@ -2377,6 +2381,8 @@ main(int argc, char *argv[])
usage();
/* NOTREACHED */
}
+ if (!*optarg)
+ errx(1, "empty state key");
state_kill[state_killers++] = optarg;
mode = O_RDWR;
break;
@@ -2386,6 +2392,8 @@ main(int argc, char *argv[])
usage();
/* NOTREACHED */
}
+ if (!*optarg)
+ errx(1, "empty source tracking key");
src_node_kill[src_node_killers++] = optarg;
mode = O_RDWR;
break;
@@ -2434,6 +2442,8 @@ main(int argc, char *argv[])
}
break;
case 't':
+ if (!*optarg)
+ errx(1, "empty table name");
tableopt = optarg;
break;
case 'T':
@@ -2626,13 +2636,13 @@ main(int argc, char *argv[])
pfctl_clear_stats(dev, ifaceopt, opts);
break;
case 'a':
- pfctl_clear_tables(anchorname, opts);
- pfctl_clear_rules(dev, opts, anchorname);
- if (ifaceopt && *ifaceopt) {
+ if (ifaceopt) {
warnx("don't specify an interface with -Fall");
usage();
/* NOTREACHED */
}
+ pfctl_clear_tables(anchorname, opts);
+ pfctl_clear_rules(dev, opts, anchorname);
if (!*anchorname) {
pfctl_clear_states(dev, ifaceopt, opts);
pfctl_clear_src_nodes(dev, opts);
Re: pfctl: defuse `-F all -i ...', catch empty argument values
On Sat, Jan 05, 2019 at 12:07:59PM -0700, Theo de Raadt wrote:
> + if (!*optarg)
>
> I despise this idiom. You are checking for a zero-length string.
> But you are hiding what is going on.
Because the value is used in many places. Some check for nullity, some
check for emptyness, others just strlcpy() it and leave error checks to
the ioctls.
> But why do you do it at every step??? Why not do it at the end
> after getopt?
How so?
This won't work:
while (ch = getopt(...)) {
switch (ch) {
...
}
/* breaks `-v' et al. unless I check `ch' again */
if (!*optarg)
errx(1, "empty -%c argument", ch);
}
And this is the same check but just scattered and deferred really:
while (...) {
...
}
...
if (ifaceopt && !*ifaceopt)
errx(1, "empty interface name");
...
memset(anchorname, 0, sizeof(anchorname));
if (anchoropt != NULL) {
if (!*anchoropt)
errx(1. "empty anchor name");
...
}
...
> Your diff feels wrong.
Well, it's an effective solution to multiple deficiencies that does not
require major code reshuffling.
I can work on further improvements but checking these tings as early as
possible is one step in the right direction, I think.
Re: pfctl: defuse `-F all -i ...'
On Sat, Jan 05, 2019 at 08:04:07PM +0100, Klemens Nanni wrote:
> Diff below bails out immediately when `-i ...' is passed
Just that now.
Ignore the option argument if the option was passed since that already
fulfills our error condition of passing `-i ...' with `-F all'.
`ifaceopt' is global and therefore NULL initialised.
Still clearing tables and rules when this error is hit seems wrong to
me, so don't do anything unless the pfctl command is actually valid.
mcbride introduced this code with r1.298 in 2010 but used
if (*ifaceopt) {
only to have stsp fix a segfault in r1.299 by changing it to the current
form.
One might as well assume that my proposed condition was the originally
intended behaviour after all and stsp's fix should've just removed the
derefence.
OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.362
diff -u -p -r1.362 pfctl.c
--- pfctl.c 2 Jan 2019 23:08:00 - 1.362
+++ pfctl.c 5 Jan 2019 22:01:56 -
@@ -2626,13 +2626,13 @@ main(int argc, char *argv[])
pfctl_clear_stats(dev, ifaceopt, opts);
break;
case 'a':
- pfctl_clear_tables(anchorname, opts);
- pfctl_clear_rules(dev, opts, anchorname);
- if (ifaceopt && *ifaceopt) {
+ if (ifaceopt) {
warnx("don't specify an interface with -Fall");
usage();
/* NOTREACHED */
}
+ pfctl_clear_tables(anchorname, opts);
+ pfctl_clear_rules(dev, opts, anchorname);
if (!*anchorname) {
pfctl_clear_states(dev, ifaceopt, opts);
pfctl_clear_src_nodes(dev, opts);
pfctl: zap unused function parameter
Never used, probably just copy/pasta since introduction in 2006.
`-i' and other flags are completely ignored with `-K' anyway.
OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.362
diff -u -p -r1.362 pfctl.c
--- pfctl.c 2 Jan 2019 23:08:00 - 1.362
+++ pfctl.c 5 Jan 2019 20:55:11 -
@@ -67,7 +67,7 @@ void pfctl_clear_rules(int, int, char *
voidpfctl_clear_src_nodes(int, int);
voidpfctl_clear_states(int, const char *, int);
voidpfctl_addrprefix(char *, struct pf_addr *);
-voidpfctl_kill_src_nodes(int, const char *, int);
+voidpfctl_kill_src_nodes(int, int);
voidpfctl_net_kill_states(int, const char *, int, int);
voidpfctl_label_kill_states(int, const char *, int, int);
voidpfctl_id_kill_states(int, int);
@@ -405,7 +405,7 @@ pfctl_addrprefix(char *addr, struct pf_a
}
void
-pfctl_kill_src_nodes(int dev, const char *iface, int opts)
+pfctl_kill_src_nodes(int dev, int opts)
{
struct pfioc_src_node_kill psnk;
struct addrinfo *res[2], *resp[2];
@@ -2661,7 +2661,7 @@ main(int argc, char *argv[])
}
if (src_node_killers)
- pfctl_kill_src_nodes(dev, ifaceopt, opts);
+ pfctl_kill_src_nodes(dev, opts);
if (tblcmdopt != NULL) {
error = pfctl_table(argc, argv, tableopt,
pfctl: use mnemonic macros, terminate string with null char
A few assorted nits for consistency and proper format, no object change.
OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.365
diff -u -p -r1.365 pfctl.c
--- pfctl.c 11 Jan 2019 03:09:24 - 1.365
+++ pfctl.c 19 Jan 2019 01:29:20 -
@@ -1485,7 +1485,6 @@ pfctl_load_ruleset(struct pfctl *pf, cha
}
} else if (pf->opts & PF_OPT_VERBOSE)
printf("\n");
-
}
if (pf->optimize)
@@ -1851,7 +1850,6 @@ pfctl_set_limit(struct pfctl *pf, const
{
int i;
-
for (i = 0; pf_limits[i].name; i++) {
if (strcasecmp(opt, pf_limits[i].name) == 0) {
pf->limit[pf_limits[i].index] = limit;
@@ -2217,7 +2215,7 @@ pfctl_show_anchors(int dev, int opts, ch
err(1, "DIOCGETRULESET");
if (!strcmp(pr.name, PF_RESERVED_ANCHOR))
continue;
- sub[0] = 0;
+ sub[0] = '\0';
if (pr.path[0]) {
strlcat(sub, pr.path, sizeof(sub));
strlcat(sub, "/", sizeof(sub));
@@ -2235,6 +2233,7 @@ const char *
pfctl_lookup_option(char *cmd, const char **list)
{
const char *item = NULL;
+
if (cmd != NULL && *cmd)
for (; *list; list++)
if (!strncmp(cmd, *list, strlen(cmd))) {
@@ -2580,15 +2579,15 @@ main(int argc, char *argv[])
opts |= PF_OPT_SHOWALL;
pfctl_load_fingerprints(dev, opts);
- pfctl_show_rules(dev, path, opts, 0, anchorname,
- 0, 0, -1);
+ pfctl_show_rules(dev, path, opts, PFCTL_SHOW_RULES,
+ anchorname, 0, 0, -1);
pfctl_show_queues(dev, ifaceopt, opts,
opts & PF_OPT_VERBOSE2);
pfctl_show_states(dev, ifaceopt, opts, -1);
pfctl_show_src_nodes(dev, opts);
pfctl_show_status(dev, opts);
- pfctl_show_rules(dev, path, opts, 1, anchorname,
- 0, 0, -1);
+ pfctl_show_rules(dev, path, opts, PFCTL_SHOW_LABELS,
+ anchorname, 0, 0, -1);
pfctl_show_timeouts(dev, opts);
pfctl_show_limits(dev, opts);
pfctl_show_tables(anchorname, opts);
@@ -2671,7 +2670,7 @@ main(int argc, char *argv[])
if (optiopt != NULL) {
switch (*optiopt) {
case 'n':
- optimize = 0;
+ optimize = PF_OPTIMIZE_NONE;
break;
case 'b':
optimize |= PF_OPTIMIZE_BASIC;
Index: pfctl_parser.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.h,v
retrieving revision 1.112
diff -u -p -r1.112 pfctl_parser.h
--- pfctl_parser.h 6 Sep 2018 15:07:34 - 1.112
+++ pfctl_parser.h 19 Jan 2019 01:13:13 -
@@ -57,6 +57,7 @@
#define PF_NAT_PROXY_PORT_LOW 50001
#define PF_NAT_PROXY_PORT_HIGH 65535
+#define PF_OPTIMIZE_NONE 0x
#define PF_OPTIMIZE_BASIC 0x0001
#define PF_OPTIMIZE_PROFILE0x0002
Re: pfctl: use mnemonic macros, terminate string with null char
On Sat, Jan 19, 2019 at 05:14:56PM +1300, Richard Procter wrote: > > +#define PF_OPTIMIZE_NONE 0x > > these PF_OPTIMIZE_* are bit-field definitions, > see e.g. pfctl_optimize.c:299. While I'm aware of this, > But PF_OPTIMIZE_NONE is not, as pf->optimize & PF_OPTIMIZE_NONE > is never true, and pf->optimize |= PF_OPTIMIZE_NONE has no effect. I did not really consider such usage, which is indeed a bit dangerous. > so I would leave this as optimize = 0; and drop PF_OPTIMIZE_NONE. Agreed, thanks.
ndp: zap unused ntop_buf
Last usage got removed in revision 1.9 date: 2001/02/08 08:35:17; author: itojun; state: Exp; lines: +109 -27; pull latest kame tree. ndp -n -a printing is now prettier with long IPv6 addresses. -l is deprecated (ignored). OK? Index: ndp.c === RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v retrieving revision 1.90 diff -u -p -r1.90 ndp.c --- ndp.c 13 Jul 2018 09:03:44 - 1.90 +++ ndp.c 20 Jan 2019 22:41:24 - @@ -116,7 +116,6 @@ static int32_t thiszone;/* time differe static int rtsock = -1; static int repeat = 0; -char ntop_buf[INET6_ADDRSTRLEN]; /* inet_ntop() */ char host_buf[NI_MAXHOST]; /* getnameinfo() */ char ifix_buf[IFNAMSIZ]; /* if_indextoname() */
Re: Down bridge(4) & span ports
On Tue, Jan 22, 2019 at 11:09:10PM +0100, Claudio Jeker wrote: > On Tue, Jan 22, 2019 at 12:57:34PM -0200, Martin Pieuchot wrote: > > If a bridge(4) is down packets don't flow through it, so be coherent and > > do not copy them for span ports. I hesitated to OK this since possible implications were not directly obvious to me. > This makes span ports consistent with the bpf listener so this is indeed > the right way for span ports to work on bridges. Good point, I didn't look at bpf with this in mind. OK kn.
Re: kdump -f -
On Tue, Dec 11, 2018 at 10:31:37PM -0500, Ted Unangst wrote: > I have some trace files that are gzipped to save space. (They compress really > well.) It would be convenient if I could simply zcat them into kdump for > inspection. FWIW I've always used `kdump -f/dev/stdin' for that. > This patch allows -f - to read from stdin. (Curiously, kdump always reads from > stdin, but uses freopen on the trace file.) OK kn > Unsure about man page. I think many utilities just generally assume the user > knows - means stdin/out? Without explicitly mentioning it, one might assume that ktrace(1) behaves the same, which is not the case as `ktrace -f - echo foo' will write to the file called "-". I think that should be either clarified or implemented in ktrace, too, but no objections for now.
pf.4 pfctl.8: s/drivers/groups/
All interface drivers have their interface group, but users can create
extra, driver independent groups as well.
# ifconfig lo0 group foo
# pfctl -sI -ifoo
foo
lo0
Feedback? OK?
Index: share/man/man4/pf.4
===
RCS file: /cvs/src/share/man/man4/pf.4,v
retrieving revision 1.89
diff -u -p -r1.89 pf.4
--- share/man/man4/pf.4 12 Oct 2017 14:39:24 - 1.89
+++ share/man/man4/pf.4 20 Dec 2018 20:34:10 -
@@ -916,7 +916,7 @@ will be set to the length of the buffer
.It Dv DIOCCLRSRCNODES
Clear the tree of source tracking nodes.
.It Dv DIOCIGETIFACES Fa "struct pfioc_iface *io"
-Get the list of interfaces and interface drivers known to
+Get the list of interfaces and interface groups known to
.Nm .
All the ioctls that manipulate interfaces
use the same structure described below:
@@ -933,7 +933,7 @@ struct pfioc_iface {
.Pp
If not empty,
.Va pfiio_name
-can be used to restrict the search to a specific interface or driver.
+can be used to restrict the search to a specific interface or group.
.Va pfiio_buffer[pfiio_size]
is the user-supplied buffer for returning the data.
On entry,
Index: sbin/pfctl/pfctl.8
===
RCS file: /cvs/src/sbin/pfctl/pfctl.8,v
retrieving revision 1.172
diff -u -p -r1.172 pfctl.8
--- sbin/pfctl/pfctl.8 18 Sep 2018 12:55:19 - 1.172
+++ sbin/pfctl/pfctl.8 20 Dec 2018 20:23:51 -
@@ -392,7 +392,7 @@ Show the list of tables.
.It Fl s Cm osfp
Show the list of operating system fingerprints.
.It Fl s Cm Interfaces
-Show the list of interfaces and interface drivers available to PF.
+Show the list of interfaces and interface groups available to PF.
When used together with
.Fl v ,
it additionally lists which interfaces have skip rules activated.
Re: iSerialNumber -> serial
OK
Re: convert some timeout_add to timeout_add_*
On Sun, Dec 16, 2018 at 06:45:39PM +0100, Claudio Jeker wrote: > Simple conversion of timeout_add(X, Y * hz) to timeout_add_sec(X, Y) > and timeout_add(X, tvtohz()) to timeout_add_tv(X, ). OK
Re: change nc(1) port range delimiter
On Sat, Dec 22, 2018 at 06:53:24PM -0500, Ted Unangst wrote: > @@ -393,7 +393,7 @@ option is given). > .Ar port > can be a specified as a numeric port number, or as a service name. stray ^ article that can be zapped with this diff while here. > Ports may be specified in a range of the form > -.Ar nn Ns - Ns Ar mm . > +.Ar nn Ns : Ns Ar mm . If you implement both, you should say so. > -The port range was specified to limit the search to ports 20 \- 30. > +The port range was specified to limit the search to ports 20 : 30. Maybe use "to" here? Reads nicer imho as it's a proper sentence already. > @@ -584,3 +584,9 @@ the > combination could be useful for communications diagnostics. > Note that the amount of UDP traffic generated may be limited either > due to hardware resources and/or configuration settings. > +.Pp > +Previous versions of the > +.Nm nc > +utility used a hyphen to separate port ranges. > +This was changed to a colon to avoid ambiguity with hyphenated service names, > +though the hyphen separator remains supported for backwards compatibility. Do you want to throw out hyphen support eventually?
pfctl/pf.conf: remove "load anchor" support
>From pf.conf(5):
The anchor can also be populated by adding a load anchor rule after the
anchor rule. When pfctl(8) loads pf.conf, it will also load all the
rules from the file /etc/pf-spam.conf into the anchor.
anchor spam
load anchor spam from "/etc/pf-spam.conf"
This is too much verbiage for nothing since we have `include'.
parse.y history shows
revision 1.650
date: 2016/06/16 15:46:20; author: henning; state: Exp; lines: +1 -0;
allow include in inline anchors
with this,
anchor foo {
include "/path/to/rules"
}
works and "load anchor" is obsolete, to be removed somewhen later after
release.
co-production with reky at bsdcan, ok reyk mikeb benno sasha
Like this:
anchor spam {
include /etc/pf-spam.conf
}
OK to remove these duplicate semantics? Below is a diff for pfctl and
pf.conf(5).
pfctl regress still passes when I remove the `load anchor' tests and
adjust test 103 accordingly.
I'll send a separate regress diff after consense and OKs but before
committing. Same for our pf FAQ.
current.html with instructions to switch to the simpler syntax will
follow, as well.
Feedback? OK?
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.688
diff -u -p -r1.688 parse.y
--- sbin/pfctl/parse.y 15 Nov 2018 03:22:01 - 1.688
+++ sbin/pfctl/parse.y 25 Dec 2018 15:37:37 -
@@ -394,15 +394,6 @@ int map_tos(char *string, int *);
int rdomain_exists(u_int);
int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
-TAILQ_HEAD(loadanchorshead, loadanchors)
-loadanchorshead = TAILQ_HEAD_INITIALIZER(loadanchorshead);
-
-struct loadanchors {
- TAILQ_ENTRY(loadanchors) entries;
- char*anchorname;
- char*filename;
-};
-
typedef struct {
union {
int64_t number;
@@ -547,7 +538,6 @@ ruleset : /* empty */
| ruleset option '\n'
| ruleset pfrule '\n'
| ruleset anchorrule '\n'
- | ruleset loadrule '\n'
| ruleset queuespec '\n'
| ruleset varset '\n'
| ruleset antispoof '\n'
@@ -949,37 +939,6 @@ anchorrule : ANCHOR anchorname dir quick
}
;
-loadrule : LOAD ANCHOR string FROM string{
- struct loadanchors *loadanchor;
-
- if (strlen(pf->anchor->path) + 1 +
- strlen($3) >= PATH_MAX) {
- yyerror("anchorname %s too long, max %u\n",
- $3, PATH_MAX - 1);
- free($3);
- YYERROR;
- }
- loadanchor = calloc(1, sizeof(struct loadanchors));
- if (loadanchor == NULL)
- err(1, "loadrule: calloc");
- if ((loadanchor->anchorname = malloc(PATH_MAX)) ==
- NULL)
- err(1, "loadrule: malloc");
- if (pf->anchor->name[0])
- snprintf(loadanchor->anchorname, PATH_MAX,
- "%s/%s", pf->anchor->path, $3);
- else
- strlcpy(loadanchor->anchorname, $3, PATH_MAX);
- if ((loadanchor->filename = strdup($5)) == NULL)
- err(1, "loadrule: strdup");
-
- TAILQ_INSERT_TAIL(, loadanchor,
- entries);
-
- free($3);
- free($5);
- };
-
scrub_opts : {
bzero(_opts, sizeof scrub_opts);
}
@@ -5755,23 +5714,6 @@ parseport(char *port, struct range *r, i
return (0);
}
return (-1);
-}
-
-int
-pfctl_load_anchors(int dev, struct pfctl *pf, struct pfr_buffer *trans)
-{
- struct loadanchors *la;
-
- TAILQ_FOREACH(la, , entries) {
- if (pf->opts & PF_OPT_VERBOSE)
- fprintf(stderr, "\nLoading anchor %s from %s\n",
- la->anchorname, la->filename);
- if (pfctl_rules(dev, la->filename, pf->opts, pf->optimize,
- la->anchorname, trans) == -1)
- return (-1);
- }
-
- return (0);
}
int
Index: sbin/pfctl/pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.360
diff -u -p -r1.360
Re: change nc(1) port range delimiter
On Sat, Dec 22, 2018 at 04:44:14PM -0500, Daniel Jakots wrote: > Here's a diff that change the delimiter to ":". This breaks existing > scripts but it would make the syntax like pf.conf instead of using > another symbol for a port range. Changing it to double-colon for sanity and consistency seems good, but maybe that's too much of a break at once? Or are ranges not considered critical enough to worry about this? I never use them. > If you have a better idea how to solve this problem, please share! Perhaps phasing this out by first accepting both as delimiter, then dropping the dash later on? One alternative would be to try the given word as is first, only interpreting it as a range when it's not a valid service name. Also, how do other netcat implementations handle ranges? Do we care about consistency with these?
Re: pfctl/pf.conf: remove "load anchor" support
On Tue, Dec 25, 2018 at 10:19:35AM -0700, Theo de Raadt wrote:
> I have always disliked the reliance on include, because errors detected
> during parse are poorly handled. Garbage format in the file will adjust
> the global scope and the parser is clueless to cope well.
Can you elaborate on this?
$ cat a.in
pass
# invalid inside anchors
set optimization aggressive
# error
garbage
With `include', errors in included files are detected after the line is
finished and *all* errors are reported with their correct line numbers:
$ cat include.conf
anchor a {
include a.in
}
block
$ pfctl -vnf include.conf
a.in:3: syntax error
a.in:5: syntax error
include.conf:3: syntax error
While with `load anchor' it parses everything, prints the offending
line with an off-by-one but stops after the first error, which means
I need to reparse again to find the other:
$ cat load.conf
anchor a
load anchor a from a.in
block
$ pfctl -vnf load.conf
anchor "a" all
block drop all
Loading anchor a from a.in
set optimization aggressive
a.in:4: syntax error
pfctl: load anchors
Personally, I prefer the terse format of the former. We can still
improve error messages to distinguish between invalid syntax like
setting global options inside anchors and, actual synta errors, etc.
> An implicit load directive of a specifically formatted file can be made
> far more robust.
How is the file specifically formatted? Or would you prefer more
specific formats for different type of load/include directives?
Re: pfctl/pf.conf: remove "load anchor" support
On Tue, Dec 25, 2018 at 07:19:21PM +0100, Sebastian Benoit wrote:
> that said, if we want this, we might want to have pfctl print a warning for
> a release cycle because it can impact the reachability of a machine. Like we
> do with ifconfig vlanid/parent changes.
Keep the behaviour in 6.4 -CURRENT but print a warning and drop it right
after the 6.5 release?
This diff removes `load anchor' from pf.conf(5) and adds a warning where
the "Loading anchor from ..." is printed on standard error. This way
the validated configuration is not affected on standard out and regress
passes without changes.
The queue check is needed since pfctl_rules() always calls
pfctl_load_anchors() when loading the new ruleset into the kernel, even
when there was no `load anchor' statement.
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.688
diff -u -p -r1.688 parse.y
--- sbin/pfctl/parse.y 15 Nov 2018 03:22:01 - 1.688
+++ sbin/pfctl/parse.y 25 Dec 2018 20:39:05 -
@@ -5762,6 +5762,10 @@ pfctl_load_anchors(int dev, struct pfctl
{
struct loadanchors *la;
+ if (!TAILQ_EMPTY())
+ fprintf(stderr, "\n'load anchor' is deprecated, "
+ "use 'include' inside braced anchors\n");
+
TAILQ_FOREACH(la, , entries) {
if (pf->opts & PF_OPT_VERBOSE)
fprintf(stderr, "\nLoading anchor %s from %s\n",
Index: share/man/man5/pf.conf.5
===
RCS file: /cvs/src/share/man/man5/pf.conf.5,v
retrieving revision 1.577
diff -u -p -r1.577 pf.conf.5
--- share/man/man5/pf.conf.512 Jul 2018 05:54:49 - 1.577
+++ share/man/man5/pf.conf.525 Dec 2018 16:20:56 -
@@ -1803,21 +1803,6 @@ which blocks all packets from a specific
# echo "block in quick from 1.2.3.4 to any" | pfctl -a spam -f -
.Ed
.Pp
-The anchor can also be populated by adding a
-.Ic load anchor
-rule after the anchor rule.
-When
-.Xr pfctl 8
-loads
-.Nm ,
-it will also load all the rules from the file
-.Pa /etc/pf-spam.conf
-into the anchor.
-.Bd -literal -offset indent
-anchor spam
-load anchor spam from "/etc/pf-spam.conf"
-.Ed
-.Pp
An anchor rule can also contain a filter ruleset
in a brace-delimited block.
In that case, no separate loading of rules into the anchor
@@ -1888,10 +1873,7 @@ translation rules, for example, may also
Anchor rules are evaluated relative to the anchor in which they are contained.
For example,
all anchor rules specified in the main ruleset will reference
-anchor attachment points underneath the main ruleset,
-and anchor rules specified in a file loaded from a
-.Ic load anchor
-rule will be attached under that anchor point.
+anchor attachment points underneath the main ruleset.
.Pp
Anchors may end with the asterisk
.Pq Sq *
@@ -2778,8 +2760,6 @@ anchor-rule= "anchor" [ string ] [ (
[ af ] [ protospec ] [ hosts ] [ filteropt-list ] [ "{" ]
anchor-close = "}"
-
-load-anchor= "load anchor" string "from" filename
queueopts-list = queueopts-list queueopts | queueopts
queueopts = ([ "bandwidth" bandwidth ] | [ "min" bandwidth ] |
Re: apmd status at suspend/resume
On Thu, Nov 29, 2018 at 02:50:58PM -0500, Ted Unangst wrote: > I would find it useful to know battery percentage at the time of suspend and > resume. This makes it possible to see how much battery was consumed while > sleeping. I don't think this is much noisier than things already are. Sounds reasonable, but none of this will show up when running `apmd -d' it seems.
Re: apmd debug
On Fri, Nov 30, 2018 at 01:24:27PM -0500, Ted Unangst wrote: > Developers who shall remain anonymous were confused by the behavior of apmd -d > because the behavior of apmd -d is confusing. It doesn't do anything like any > other daemon in the system when running with -d. :-) > This introduces a logmsg function to replace syslog, which will either syslog > or printf depending on debug. I think this makes debugging much easier. Yes it does. Code looks good but the manual bits are missing.
apmd: -t: use strtonum()
Base 10 suffices, negative numbers should be invalid (not converted) and zero not treated specially: # apmd -dt -1 apmd: kevent loop: Invalid argument # apmd -dt 0 usage: apmd [-AadHLs] [-f devname] [-S sockname] [-t seconds] [-Z percent] [-z percent] # apmd -dt 1 ^C # ./obj/apmd -dt -1 apmd: too small seconds: -1 # ./obj/apmd -dt 0 apmd: too small seconds: 0 # ./obj/apmd -dt 1 ^C errstr is precise enough, so drop the "Invalid argument" by using just err() while here. Feedback? OK? Index: apmd.c === RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v retrieving revision 1.82 diff -u -p -r1.82 apmd.c --- apmd.c 30 Nov 2018 18:05:31 - 1.82 +++ apmd.c 1 Dec 2018 00:33:24 - @@ -392,9 +392,9 @@ main(int argc, char *argv[]) sockname = optarg; break; case 't': - ts.tv_sec = strtoul(optarg, NULL, 0); - if (ts.tv_sec == 0) - usage(); + ts.tv_sec = strtonum(optarg, 1, LLONG_MAX, ); + if (errstr != NULL) + err(1, "%s seconds: %s", errstr, optarg); break; case 's': /* status only */ statonly = 1; @@ -422,15 +422,13 @@ main(int argc, char *argv[]) autoaction = AUTO_HIBERNATE; autolimit = strtonum(optarg, 1, 100, ); if (errstr != NULL) - errc(1, EINVAL, "%s percentage: %s", errstr, - optarg); + err(1, "%s percentage: %s", errstr, optarg); break; case 'z': autoaction = AUTO_SUSPEND; autolimit = strtonum(optarg, 1, 100, ); if (errstr != NULL) - errc(1, EINVAL, "%s percentage: %s", errstr, - optarg); + err(1, "%s percentage: %s", errstr, optarg); break; case '?': default:
sys/net/pf*.[ch]: remove useless macros
All they do is case conversion^Wconfusion, so remove them.
Relevant pfvar.h diff at the top, all other hunks were done with sed(1).
Feedback? Objections? OK?
Index: net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.486
diff -u -p -r1.486 pfvar.h
--- net/pfvar.h 13 Sep 2018 19:53:58 - 1.486
+++ net/pfvar.h 8 Dec 2018 08:12:30 -
@@ -278,19 +278,6 @@ struct pfi_dynaddr {
!(a)->addr32[0] && !(a)->addr32[1] && \
!(a)->addr32[2] && !(a)->addr32[3] )) \
-#define PF_MATCHA(n, a, m, b, f) \
- pf_match_addr(n, a, m, b, f)
-
-#define PF_ACPY(a, b, f) \
- pf_addrcpy(a, b, f)
-
-#define PF_AINC(a, f) \
- pf_addr_inc(a, f)
-
-#define PF_POOLMASK(a, b, c, d, f) \
- pf_poolmask(a, b, c, d, f)
-
-
#definePF_MISMATCHAW(aw, x, af, neg, ifp, rtid)
\
( \
(((aw)->type == PF_ADDR_NOROUTE && \
@@ -308,7 +295,7 @@ struct pfi_dynaddr {
&(aw)->v.a.mask, (x), (af))) || \
((aw)->type == PF_ADDR_ADDRMASK && \
!PF_AZERO(&(aw)->v.a.mask, (af)) && \
- !PF_MATCHA(0, &(aw)->v.a.addr, \
+ !pf_match_addr(0, &(aw)->v.a.addr, \
&(aw)->v.a.mask, (x), (af) != \
(neg) \
)
Index: net/if_pflog.c
===
RCS file: /cvs/src/sys/net/if_pflog.c,v
retrieving revision 1.81
diff -u -p -r1.81 if_pflog.c
--- net/if_pflog.c 9 Jan 2018 15:24:24 - 1.81
+++ net/if_pflog.c 8 Dec 2018 08:04:34 -
@@ -267,8 +267,8 @@ pflog_packet(struct pf_pdesc *pd, u_int8
hdr.rule_pid = rm->cpid;
hdr.dir = pd->dir;
- PF_ACPY(, >nsaddr, pd->naf);
- PF_ACPY(, >ndaddr, pd->naf);
+ pf_addrcpy(, >nsaddr, pd->naf);
+ pf_addrcpy(, >ndaddr, pd->naf);
hdr.af = pd->af;
hdr.naf = pd->naf;
hdr.sport = pd->nsport;
@@ -402,8 +402,8 @@ pflog_bpfcopy(const void *src_arg, void
goto copy;
pd.naf = pfloghdr->naf;
- PF_ACPY(, pd.src, pd.af);
- PF_ACPY(, pd.dst, pd.af);
+ pf_addrcpy(, pd.src, pd.af);
+ pf_addrcpy(, pd.dst, pd.af);
if (pd.sport)
osport = *pd.sport;
if (pd.dport)
@@ -417,12 +417,12 @@ pflog_bpfcopy(const void *src_arg, void
, M_NOWAIT);
#ifdef INET6
if (afto) {
- PF_ACPY(, >saddr, pd.naf);
- PF_ACPY(, >daddr, pd.naf);
+ pf_addrcpy(, >saddr, pd.naf);
+ pf_addrcpy(, >daddr, pd.naf);
}
#endif /* INET6 */
- PF_ACPY(>saddr, , pd.af);
- PF_ACPY(>daddr, , pd.af);
+ pf_addrcpy(>saddr, , pd.af);
+ pf_addrcpy(>daddr, , pd.af);
pfloghdr->sport = osport;
pfloghdr->dport = odport;
}
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1078
diff -u -p -r1.1078 pf.c
--- net/pf.c15 Nov 2018 13:16:37 - 1.1078
+++ net/pf.c8 Dec 2018 08:04:34 -
@@ -549,7 +549,7 @@ pf_insert_src_node(struct pf_src_node **
if (*sn == NULL) {
k.af = af;
k.type = type;
- PF_ACPY(, src, af);
+ pf_addrcpy(, src, af);
k.rule.ptr = rule;
pf_status.scounters[SCNT_SRC_NODE_SEARCH]++;
*sn = RB_FIND(pf_src_tree, _src_tracking, );
@@ -570,9 +570,9 @@ pf_insert_src_node(struct pf_src_node **
(*sn)->type = type;
(*sn)->af = af;
(*sn)->rule.ptr = rule;
- PF_ACPY(&(*sn)->addr, src, af);
+ pf_addrcpy(&(*sn)->addr, src, af);
if (raddr)
- PF_ACPY(&(*sn)->raddr, raddr, af);
+ pf_addrcpy(&(*sn)->raddr, raddr, af);
if (RB_INSERT(pf_src_tree,
_src_tracking, *sn) != NULL) {
if (pf_status.debug >= LOG_NOTICE) {
@@ -855,9 +855,9 @@ pf_state_key_addr_setup(struct pf_pdesc
copy:
#endif /* INET6 */
if (saddr)
- PF_ACPY(>addr[sidx], saddr, af);
+ pf_addrcpy(>addr[sidx], saddr, af);
if (daddr)
- PF_ACPY(>addr[didx], daddr, af);
+ pf_addrcpy(>addr[didx], daddr, af);
return (0);
}
@@ -2250,7 +2250,7 @@ pf_translate_icmp(struct pf_pdesc *pd, s
/* change quoted ip address */
Re: ktrace buglet
On Thu, Dec 06, 2018 at 03:33:06PM -0500, Ted Unangst wrote: > ktrace -C will return an error if you don't have a ktrace.out file because > sys_ktrace tries to open it whenever it has a filename, even if it won't be > used. I think it is more consistent to require it be null, so that we aren't > opening files we won't be using. > > man page and utility diff below. `ktrace -C' is shown twice as example in ktrace(1), once in DESCRIPTION and at the end of EXAMPLES again. OK kn
Re: Remove no longer used M_ALIGN and MH_ALIGN
OK
sys/net/pfvar.h: zap duplicate signatures
These are identical (see increased context) since introduction: revision 1.240 date: 2006/10/27 13:56:51; author: mcbride; state: Exp; lines: +26 -8; Split ruleset manipulation functions out into pf_ruleset.c to allow them to be imported into pfctl. This is a precursor to separating ruleset parsing from loading in pfctl, and tons of good things will come from it. 2 minor changes aside from cut-n-paste and #define portability magic: - instead of defining the global pf_main_ruleset, define pf_main_anchor (which contains the pf_main_ruleset) - allow pf_find_or_create_ruleset() to return the pf_main_ruleset if it's passed an empty anchor name. ok henning dhartmei OK to remove the duplicate signatures under _KERNEL for clarity? Index: pfvar.h === RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.486 diff -u -p -U11 -r1.486 pfvar.h --- pfvar.h 13 Sep 2018 19:53:58 - 1.486 +++ pfvar.h 8 Dec 2018 21:42:31 - @@ -1917,28 +1917,22 @@ int pf_anchor_setup(struct pf_rule *, int pf_anchor_copyout(const struct pf_ruleset *, const struct pf_rule *, struct pfioc_rule *); voidpf_anchor_remove(struct pf_rule *); voidpf_remove_if_empty_ruleset(struct pf_ruleset *); struct pf_anchor *pf_find_anchor(const char *); struct pf_ruleset *pf_find_ruleset(const char *); struct pf_ruleset *pf_get_leaf_ruleset(char *, char **); struct pf_anchor *pf_create_anchor(struct pf_anchor *, const char *); struct pf_ruleset *pf_find_or_create_ruleset(const char *); voidpf_rs_initialize(void); -#ifdef _KERNEL -int pf_anchor_copyout(const struct pf_ruleset *, - const struct pf_rule *, struct pfioc_rule *); -voidpf_anchor_remove(struct pf_rule *); -#endif /* _KERNEL */ - /* The fingerprint functions can be linked into userland programs (tcpdump) */ intpf_osfp_add(struct pf_osfp_ioctl *); #ifdef _KERNEL struct pf_osfp_enlist * pf_osfp_fingerprint(struct pf_pdesc *); #endif /* _KERNEL */ struct pf_osfp_enlist * pf_osfp_fingerprint_hdr(const struct ip *, const struct ip6_hdr *, const struct tcphdr *); void pf_osfp_flush(void); intpf_osfp_get(struct pf_osfp_ioctl *);
Re: apmd: -t: use strtonum()
On Sat, Dec 01, 2018 at 08:58:31AM +0100, Martijn van Duren wrote:
> > I'm not sure the EINVAL error string adds valuable information. I would
> > prefer if all these used variants of the idiom suggested in the strtonum
> > manual, something like:
> >
> > errx("number of seconds is %s: %s", errstr, optarg);
> > errx("battery percentage is %s: %s", errstr, optarg);
> >
> That might be even better.
I agree, thanks for the input.
OK?
Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.82
diff -u -p -r1.82 apmd.c
--- apmd.c 30 Nov 2018 18:05:31 - 1.82
+++ apmd.c 1 Dec 2018 13:19:55 -
@@ -392,9 +392,10 @@ main(int argc, char *argv[])
sockname = optarg;
break;
case 't':
- ts.tv_sec = strtoul(optarg, NULL, 0);
- if (ts.tv_sec == 0)
- usage();
+ ts.tv_sec = strtonum(optarg, 1, LLONG_MAX, );
+ if (errstr != NULL)
+ errx(1, "number of seconds is %s: %s", errstr,
+ optarg);
break;
case 's': /* status only */
statonly = 1;
@@ -422,14 +423,14 @@ main(int argc, char *argv[])
autoaction = AUTO_HIBERNATE;
autolimit = strtonum(optarg, 1, 100, );
if (errstr != NULL)
- errc(1, EINVAL, "%s percentage: %s", errstr,
+ errx(1, "battery percentage is %s: %s", errstr,
optarg);
break;
case 'z':
autoaction = AUTO_SUSPEND;
autolimit = strtonum(optarg, 1, 100, );
if (errstr != NULL)
- errc(1, EINVAL, "%s percentage: %s", errstr,
+ errx(1, "battery percentage is %s: %s", errstr,
optarg);
break;
case '?':
top: allow reverse sort order
Sometimes I want to see certain programs with least amount of memory,
so this diff implements `o -field' to sort in reverse order.
The logic is straight forward:
1. merge common code from argument and command loops into new setorder()
2. introduce global state `rev_order' (set in the helper)
3. move identical code to set up process objects from compare_*()
functions into SETORDER macro using global boolean
compare_*() are used by qsort(3). To sort in reverse, the macro simply
swaps the objects used by the ORDERKEY_* macros. That is it inverts the
comparison from `p1 > p2' into `p2 > p1' respectively `p1 < p2'.
Works fine for all available fields on amd64, no behaviour change for
"normal" order.
Feedback? Objections?
Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.57
diff -u -p -r1.57 display.c
--- display.c 17 Nov 2018 23:10:08 - 1.57
+++ display.c 24 Nov 2018 14:09:38 -
@@ -817,7 +817,8 @@ show_help(void)
"I | i- toggle the display of idle processes\n"
"k [-sig] pid - send signal `-sig' to process `pid'\n"
"n|# count- show `count' processes\n"
- "o field - specify sort order (size, res, cpu, time, pri, pid,
command)\n"
+ "o [-]field - specify sort order (size, res, cpu, time, pri, pid,
command)\n"
+ " (o -field sorts in reverse)\n"
"P pid- highlight process `pid' (P+ switches highlighting
off)\n"
"p pid- display process by `pid' (p+ selects all
processes)\n"
"q- quit\n"
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.95
diff -u -p -r1.95 machine.c
--- machine.c 17 Nov 2018 23:10:08 - 1.95
+++ machine.c 24 Nov 2018 14:47:32 -
@@ -602,6 +602,8 @@ static unsigned char sorted_state[] =
1 /* zombie*/
};
+extern int rev_order;
+
/*
* proc_compares - comparison functions for "qsort"
*/
@@ -631,6 +633,17 @@ static unsigned char sorted_state[] =
#define ORDERKEY_CMD \
if ((result = strcmp(p1->p_comm, p2->p_comm)) == 0)
+/* remove one level of indirection and set sort order */
+#define SETORDER do { \
+ if (rev_order) { \
+ p1 = *(struct kinfo_proc **) pp2; \
+ p2 = *(struct kinfo_proc **) pp1; \
+ } else { \
+ p1 = *(struct kinfo_proc **) pp1; \
+ p2 = *(struct kinfo_proc **) pp2; \
+ } \
+ } while (0)
+
/* compare_cpu - the comparison function for sorting by cpu percentage */
static int
compare_cpu(const void *v1, const void *v2)
@@ -640,9 +653,7 @@ compare_cpu(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PCTCPU
ORDERKEY_CPUTIME
@@ -663,9 +674,7 @@ compare_size(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_MEM
ORDERKEY_RSSIZE
@@ -686,9 +695,7 @@ compare_res(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_RSSIZE
ORDERKEY_MEM
@@ -709,9 +716,7 @@ compare_time(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_CPUTIME
ORDERKEY_PCTCPU
@@ -732,9 +737,7 @@ compare_prio(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PRIO
ORDERKEY_PCTCPU
@@ -754,9 +757,7 @@ compare_pid(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PID
ORDERKEY_PCTCPU
@@ -777,9 +778,7 @@ compare_cmd(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 =
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 11:28:31AM -0600, Scott Cheloha wrote:
> No objections here to the feature in general. We already support reversing
> select orderings in systat(1), which I've found useful in practice, so this
> is not without precedent and is consistent with at least one other monitoring
> tool in userland.
>
> I think it's a bit unfortunate that 'r' in interactive mode is already
> assigned
> to renice the process. If we did that here it'd be consistent with
> systat(1)'s
> interactive keybindings and a boon for overall UX. But 'r' has mean "renice"
> since before top(1) was even imported, so at this late date I don't think we
> can
> change the binding for 'r'. However, adding an 'R' shortcut to reverse the
> ordering
> for the current primary key might be a reasonable addition in a later diff.
I thought about a toggle as well but went with the "-" prefix semantics
since the existing toggles we currently have (`C', `h', `H') are
distinct features not related to filters like `g' or `o'. That is,
currently you cannot toggle something like "only user kn/all but kn".
> > Index: display.c
> > ===
> > RCS file: /cvs/src/usr.bin/top/display.c,v
> > retrieving revision 1.57
> > diff -u -p -r1.57 display.c
> > --- display.c 17 Nov 2018 23:10:08 - 1.57
> > +++ display.c 24 Nov 2018 14:09:38 -
> > @@ -817,7 +817,8 @@ show_help(void)
> > "I | i- toggle the display of idle processes\n"
> > "k [-sig] pid - send signal `-sig' to process `pid'\n"
> > "n|# count- show `count' processes\n"
> > - "o field - specify sort order (size, res, cpu, time, pri, pid,
> > command)\n"
> > + "o [-]field - specify sort order (size, res, cpu, time, pri, pid,
> > command)\n"
> > + " (o -field sorts in reverse)\n"
>
> "o" isn't needed, as contextually you're talking about the 'o' shortcut.
Keeping "o" is consistent with the rest: "g+ selects all commands" and
"u -user hides user" for example.
> > @@ -879,10 +877,10 @@ rundisplay(void)
> > new_message(MT_standout,
> > "Order to sort: ");
> > if (readline(tempbuf, sizeof(tempbuf)) > 0) {
> > - if ((i = string_index(tempbuf,
> > - statics.order_names)) == -1) {
> > + if ((i = getorder(tempbuf)) == -1) {
> > new_message(MT_standout,
> > " %s: unrecognized sorting order",
> > + tempbuf[0] == '-' ? tempbuf + 1 :
> > tempbuf);
>
> You aren't ducking the '-' during init so I don't think you should hide it
> here.
> You could even refactor the two into something like
We're also not "ducking" it for `u' in the option parsing. Admittedly,
this is inconsistent but I'd like to keep both in the command loop. The
command line options are parsed once so with an erroneous `-U --kn'
users might see "--kn: unknown user" only once on startup when in fact
"-kn" is the unknown user.
Stripping the dash in error messages from the command line which is
likely used repeatedly at runtime ensures that the correct username
is printed.
> void setorder(const char *);
>
> to keep the messages in sync, but that should be done in a subsequent diff.
So I if at all, I suggest stripping the dash in the option parsing
routine as well for consistency in separate diff.
Re: top: allow reverse sort order
On Tue, Nov 27, 2018 at 03:52:52PM -0600, Scott Cheloha wrote:
> > > static int
> > > +getorder(char *field)
> > > +{
> > > + rev_order = field[0] == '-';
> > > +
> > > + return string_index(rev_order ? field + 1 : field, statics.order_names);
> > > +}
> > > +
>
> You need to check that the field has an ordering before potentially
> modifying rev_order, otherwise a bogus input will have a side effect.
>
> For example, with the current code the input "o -notafield" reverses
> the ordering.
Good catch.
Updated diff to fix that, rest unchanged.
Note how an empty field is silently treated as the default field
"state", but that's an independent issue I'd like to address in a
separate diff for string_index().
OK?
Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.57
diff -u -p -r1.57 display.c
--- display.c 17 Nov 2018 23:10:08 - 1.57
+++ display.c 27 Nov 2018 21:09:56 -
@@ -817,7 +817,8 @@ show_help(void)
"I | i- toggle the display of idle processes\n"
"k [-sig] pid - send signal `-sig' to process `pid'\n"
"n|# count- show `count' processes\n"
- "o field - specify sort order (size, res, cpu, time, pri, pid,
command)\n"
+ "o [-]field - specify sort order (size, res, cpu, time, pri, pid,
command)\n"
+ " (o -field sorts in reverse)\n"
"P pid- highlight process `pid' (P+ switches highlighting
off)\n"
"p pid- display process by `pid' (p+ selects all
processes)\n"
"q- quit\n"
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.95
diff -u -p -r1.95 machine.c
--- machine.c 17 Nov 2018 23:10:08 - 1.95
+++ machine.c 27 Nov 2018 21:09:56 -
@@ -602,6 +602,8 @@ static unsigned char sorted_state[] =
1 /* zombie*/
};
+extern int rev_order;
+
/*
* proc_compares - comparison functions for "qsort"
*/
@@ -631,6 +633,17 @@ static unsigned char sorted_state[] =
#define ORDERKEY_CMD \
if ((result = strcmp(p1->p_comm, p2->p_comm)) == 0)
+/* remove one level of indirection and set sort order */
+#define SETORDER do { \
+ if (rev_order) { \
+ p1 = *(struct kinfo_proc **) pp2; \
+ p2 = *(struct kinfo_proc **) pp1; \
+ } else { \
+ p1 = *(struct kinfo_proc **) pp1; \
+ p2 = *(struct kinfo_proc **) pp2; \
+ } \
+ } while (0)
+
/* compare_cpu - the comparison function for sorting by cpu percentage */
static int
compare_cpu(const void *v1, const void *v2)
@@ -640,9 +653,7 @@ compare_cpu(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PCTCPU
ORDERKEY_CPUTIME
@@ -663,9 +674,7 @@ compare_size(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_MEM
ORDERKEY_RSSIZE
@@ -686,9 +695,7 @@ compare_res(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_RSSIZE
ORDERKEY_MEM
@@ -709,9 +716,7 @@ compare_time(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_CPUTIME
ORDERKEY_PCTCPU
@@ -732,9 +737,7 @@ compare_prio(const void *v1, const void
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PRIO
ORDERKEY_PCTCPU
@@ -754,9 +757,7 @@ compare_pid(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct kinfo_proc **) pp2;
+ SETORDER;
ORDERKEY_PID
ORDERKEY_PCTCPU
@@ -777,9 +778,7 @@ compare_cmd(const void *v1, const void *
struct kinfo_proc *p1, *p2;
int result;
- /* remove one level of indirection */
- p1 = *(struct kinfo_proc **) pp1;
- p2 = *(struct
Re: top: allow reverse sort order
On Wed, Nov 28, 2018 at 12:07:37AM +0100, Klemens Nanni wrote: > Note how an empty field is silently treated as the default field > "state", but that's an independent issue I'd like to address in a > separate diff for string_index(). Not a problem of string_index() actually. We consistently handle empty input for all commands silently as no-op.
Re: xidle: do not close stdout/err, error on failure, execvp(3)
On Sun, Nov 11, 2018 at 05:39:52PM +0100, Klemens Nanni wrote:
> On Sat, Nov 03, 2018 at 09:01:33PM +0100, Klemens Nanni wrote:
> > Closing stdin makes sense, but I still want to see error messages from
> > the program I'm running. Since arbitrary progams can be run, keep both
> > stdout and stderr open so users get a chance to actually notice program
> > failure or maybe even use output for good.
> >
> > In a common setup where xidle(1) is started from ~/.xsession, I'd expect
> > errors to pop up in ~/.xsession-errors.
> This, plus closely related changes:
>
> We should never execute the program unless a new session was
> created so that the child process does not share the same controlling
> terminal.
>
> Also, use execvp(3) to search PATH so users don't have to provide full
> paths any longer. Not sure why this wasn't done in the first place.
>
> Termination information from wait(2) is not used and irrelevant at this
> point, so zap `status'.
New diff with complete "full path" -> "program" changes in the manual
this time. Shuffling/splitting diffs got a bit messy here, sorry.
I'm running with this on my daily work machines without any problems.
Feedback? OK?
Index: xidle.1
===
RCS file: /cvs/xenocara/app/xidle/xidle.1,v
retrieving revision 1.5
diff -u -p -r1.5 xidle.1
--- xidle.1 6 Sep 2018 07:21:34 - 1.5
+++ xidle.1 17 Nov 2018 12:11:29 -
@@ -23,7 +23,7 @@
.\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
.\" OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
.\"
-.Dd June 20, 2005
+.Dd $Mdocdate: November 17 2018 $
.Dt XIDLE 1
.Os
.Sh NAME
@@ -36,7 +36,7 @@
.Op Fl delay Ar secs
.Op Fl display Ar display
.Op Fl no | nw | ne | sw | se
-.Op Fl program Ar path
+.Op Fl program Ar program
.Op Fl timeout Ar secs
.Ek
.Sh DESCRIPTION
@@ -71,9 +71,8 @@ Set the position to one of none, northwe
respectively.
If no position is specified,
the default is northwest.
-.It Fl program Ar path
-Specify the full pathname of the program to run on any of the
-aforementioned events.
+.It Fl program Ar program
+Specify the program to run on any of the aforementioned events.
Arguments to the program may also be specified, separated by whitespace.
If
.Fl program
@@ -110,7 +109,7 @@ and
.Fl se
options.
.It Sy program No (class Sy Program )
-Specify the full pathname of the program to run; see the
+Specify the program to run; see the
.Fl program
option.
.It Sy timeout No (class Sy Timeout )
@@ -129,8 +128,7 @@ Run
using the flying bats mode if no activity is detected in 300 seconds or the
pointer sits in the southwest corner for more than 5 seconds:
.Bd -literal -offset indent
-$ xidle -delay 5 -sw -program "/usr/X11R6/bin/xlock -mode bat" \e
- -timeout 300
+$ xidle -delay 5 -sw -program "xlock -mode bat" -timeout 300
.Ed
.Sh SEE ALSO
.Xr xlock 1 ,
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.8
diff -u -p -r1.8 xidle.c
--- xidle.c 11 Nov 2018 16:10:37 - 1.8
+++ xidle.c 17 Nov 2018 12:17:55 -
@@ -94,7 +94,7 @@ static XrmOptionDescRec opts[] = {
extern char *__progname;
-void action(struct xinfo *, char **);
+void action(struct xinfo *, char *const []);
void close_x(struct xinfo *);
Bool getres(XrmValue *, const XrmDatabase, const char *, const char *);
voidinit_x(struct xinfo *, int, int, int);
@@ -180,19 +180,18 @@ close_x(struct xinfo *xi)
void
-action(struct xinfo *xi, char **args)
+action(struct xinfo *xi, char *const args[])
{
- int dumb;
-
switch (fork()) {
case -1:
err(1, "fork");
case 0:
- setsid();
- execv(*args, args);
- exit(1);
+ if (setsid() == -1)
+ err(1, "setsid");
+ execvp(args[0], args);
+ err(1, "execvp: %s", args[0]);
default:
- wait();
+ wait(NULL);
XSync(xi->dpy, True);
break;
}
@@ -356,8 +355,6 @@ main(int argc, char **argv)
if (fd < 0)
err(1, _PATH_DEVNULL);
dup2(fd, STDIN_FILENO);
- dup2(fd, STDOUT_FILENO);
- dup2(fd, STDERR_FILENO);
if (fd > 2)
close(fd);
Re: rad: add support for listening on interface groups
On Fri, Nov 16, 2018 at 08:56:52PM +0100, Reyk Floeter wrote:
> > the following diff allows rad(8) to watch interface groups. This
> > allows to automatically add/remove interfaces in a given group.
> >
> > For example, I put "interface tap" into rad.conf and it automatically
> > serves my VM interfaces. You could also configure a custom group in
> > vm.conf and rad.conf. I'm working on IPv6 for vmd that needs it.
Nice idea/work, I like it.
> > For reasons that I don't remember, I always missed this feature in
> > rtadvd(8)[RIP]. It was amazingly simple to add it to rad(8) as it
> > already reinitializes itself on interface changes.
> >
> > This diff includes the previous ENXIO fix to prevent it from crashing
> > when a cloner interface such as tap is destroyed.
> >
>
> Updated diff after committing the ENXIO fix.
>
> Two explanations:
>
> - merge_ra_interfaces() calls merge_ra_interface() for each configured
> interface (explicit config) and for each member that is found in an
> interface group. It is basically just some code shuffling.
>
> - ra_iface->name is split into ra_iface->name and ra_iface->conf as
> "name" indicates and actual (found) interface name ("tap0") and "conf"
> is used to lookup the config where it was derived from ("tap" or
> "tap0", depending on the configuration).
This begs the question about priority in rad.conf(5). It's always been
possible to define duplicate `interface' blocks although it didn't make
any sense.
Now it does since you could define a default config for the entire group
and add further interface specific blocks.
As it currently is, the first matching block wins, so vether0 would
always get RDNS ::11 whether a more specific block exists or not:
interface vether { dns { nameserver ::11 } }
interface vether0 { dns { nameserver ::22 } }
Maybe we should clarify behaviour in rad.conf(5) regardless of your
changes?
> void
> +merge_ra_interface(struct ra_iface_conf *ra_iface_conf, char *name)
> +{
> + struct ra_iface *ra_iface;
> + uint32_t if_index;
> +
> + ra_iface = find_ra_iface_by_name(name);
> + if (ra_iface == NULL) {
> + log_debug("new interface %s", name);
> + if ((if_index = if_nametoindex(name)) == 0)
> + return;
> + log_debug("adding interface %s", name);
> + if ((ra_iface = calloc(1, sizeof(*ra_iface))) == NULL)
> + fatal("%s", __func__);
> +
> + strlcpy(ra_iface->name, name, sizeof(ra_iface->name));
> + strlcpy(ra_iface->conf, ra_iface_conf->name,
> + sizeof(ra_iface->conf));
> +
> + ra_iface->if_index = if_index;
> + SIMPLEQ_INIT(_iface->prefixes);
> + TAILQ_INSERT_TAIL(_interfaces, ra_iface, entry);
> + join_all_routers_mcast_group(ra_iface);
> + } else {
> + log_debug("keeping interface %s", name);
> + ra_iface->removed = 0;
> + }
> +}
How about flipping the check to reduce indentation?
if (ra_iface) {
log_debug("keeping interface %s", name);
...
return;
}
log_debug("new interface %s", name);
...
Re: xidle: parse options once, simplify code
On Sun, Nov 11, 2018 at 06:07:10PM +0100, Klemens Nanni wrote:
> There's no point in parsing `-display' separately, just do it once and
> simplify the code while here.
>
> This addresses two of cheloha's comments from my strtonum diff.
Ping.
Feedback? OK?
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.8
diff -u -p -r1.8 xidle.c
--- xidle.c 11 Nov 2018 16:10:37 - 1.8
+++ xidle.c 26 Nov 2018 17:37:30 -
@@ -75,13 +75,10 @@ struct xinfo {
struct xinfo x;
-static XrmOptionDescRec fopts[] = {
- { "-display", ".display", XrmoptionSepArg,(caddr_t)NULL },
-};
-
static XrmOptionDescRec opts[] = {
{ "-area", ".area",XrmoptionSepArg,(caddr_t)NULL },
{ "-delay", ".delay", XrmoptionSepArg,(caddr_t)NULL },
+ { "-display", ".display", XrmoptionSepArg,(caddr_t)NULL },
{ "-program", ".program", XrmoptionSepArg,(caddr_t)NULL },
{ "-timeout", ".timeout", XrmoptionSepArg,(caddr_t)NULL },
@@ -263,37 +260,25 @@ parse_opts(int argc, char **argv, Displa
XrmInitialize();
- /* Get display to open. */
- XrmParseCommand(, fopts, sizeof(fopts) / sizeof(fopts[0]),
+ /* Get command line values. */
+ XrmParseCommand(, opts, sizeof(opts) / sizeof(opts[0]),
__progname, , argv);
+ if (argc > 1)
+ usage();
- display = (getres(, rdb, "display", "Display") == True) ?
- (char *)value.addr : NULL;
+ display = getres(, rdb, "display", "Display") ? value.addr : NULL;
- *dpy = XOpenDisplay(display);
- if (!*dpy) {
+ if (!(*dpy = XOpenDisplay(display)))
errx(1, "Unable to open display %s", XDisplayName(display));
- }
- /* Get server resources database. */
- p = XResourceManagerString(*dpy);
- if (!p) {
- /* Get screen resources database. */
- p = XScreenResourceString(ScreenOfDisplay(*dpy,
- DefaultScreen(*dpy)));
- }
-
- if (p) {
+ /* Get resources database. */
+ if ((p = XResourceManagerString(*dpy)) ||
+ (p = XScreenResourceString(ScreenOfDisplay(*dpy,
+ DefaultScreen(*dpy) {
tdb = XrmGetStringDatabase(p);
XrmMergeDatabases(tdb, );
}
- /* Get remaining command line values. */
- XrmParseCommand(, opts, sizeof(opts) / sizeof(opts[0]),
- __progname, , argv);
- if (argc > 1) {
- usage();
- }
if (getres(, rdb, "area", "Area")) {
*area = strtonum(value.addr, 1, INT_MAX, );
if (errstr)
===
Stats: --- 26 lines 719 chars
Stats: +++ 11 lines 451 chars
Stats: -15 lines
Stats: -268 chars
pfctl: unbreak build under OPT_DEBUG
In pfctl_optimize.c r1.39 I removed the `af' parameter from `unmask()'
but accidently zapped the macro's closing paranthese.
Since DEBUG() is needlessly under an OPT_DEBUG guard here, this was not
effecting normal builds.
Add the missing ')' and remove the ifdef.
Relevant defines includede here for your convenience:
/* #define OPT_DEBUG› 1 */
#ifdef OPT_DEBUG
# define DEBUG(str, v...) \
printf("%s: " str "\n", __FUNCTION__ , ## v)
#else
# define DEBUG(str, v...) ((void)0)
#endif
OK?
Index: sbin/pfctl/pfctl_optimize.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_optimize.c,v
retrieving revision 1.39
diff -u -p -r1.39 pfctl_optimize.c
--- sbin/pfctl/pfctl_optimize.c 6 Sep 2018 15:07:33 - 1.39
+++ sbin/pfctl/pfctl_optimize.c 3 Jan 2019 19:07:46 -
@@ -1232,11 +1232,9 @@ add_opt_table(struct pfctl *pf, struct p
node_host.ifname = ifname;
node_host.weight = addr->weight;
-#ifdef OPT_DEBUG
DEBUG("<%s> adding %s/%d", (*tbl)->pt_name, inet_ntop(af,
_host.addr.v.a.addr, buf, sizeof(buf)),
- unmask(_host.addr.v.a.mask);
-#endif /* OPT_DEBUG */
+ unmask(_host.addr.v.a.mask));
if (append_addr_host((*tbl)->pt_buf, _host, 0, 0)) {
warn("failed to add host");
Re: microtime.9: update CODE REFERENCES
On Sun, Jan 13, 2019 at 01:30:09PM -0600, Scott Cheloha wrote: > The microtime(9) functions are in kern_tc.c, not kern_clock.c. OK. The rest looks fine as is: $ man -k pa=kern_tc tc_init(9) - machine-independent binary timescale $ man -k pa=kern_clock hardclock(9) - real-time system clock microtime, bintime, binuptime, getmicrotime, getmicrouptime, getnanotime, getnanouptime, microuptime, nanotime, nanouptime(9) - system clock tvtohz, tstohz(9) - translate time period to timeout delay
Re: MPLSv6 2/2 : bgpd diff
On Fri, Dec 28, 2018 at 05:21:02PM +0100, Denis Fondras wrote:
> int
> +krVPN6_change(struct ktable *kt, struct kroute_full *kl, u_int8_t fib_prio)
> +{
> + struct kroute6_node *kr6;
> + struct in6_addr lo6 = IN6ADDR_LOOPBACK_INIT;
> + int action = RTM_ADD;
> + u_int32_tmplslabel = 0;
> + u_int16_tlabelid;
> +
> + if ((kr6 = kroute6_find(kt, >prefix.vpn6.addr, kl->prefixlen,
> + fib_prio)) != NULL)
> + action = RTM_CHANGE;
Can this be moved below the conditional returns or does kroute6_find()
have side effects? `actions' is not used until much later.
> + /* nexthop to loopback -> ignore silently */
> + if (IN6_IS_ADDR_LOOPBACK(>nexthop.v6))
> + return (0);
> +
> + /* only single MPLS label are supported for now */
> + if (kl->prefix.vpn6.labellen != 3) {
> + log_warnx("%s: %s/%u has not a single label", __func__,
> + log_addr(>prefix), kl->prefixlen);
> + return (0);
> + }
Here.
> + mplslabel = (kl->prefix.vpn6.labelstack[0] << 24) |
> + (kl->prefix.vpn6.labelstack[1] << 16) |
> + (kl->prefix.vpn6.labelstack[2] << 8);
> + mplslabel = htonl(mplslabel);
> +
> + /* for blackhole and reject routes nexthop needs to be ::1 */
> + if (kl->flags & (F_BLACKHOLE|F_REJECT))
> + bcopy(, >nexthop.v6, sizeof(kl->nexthop.v6));
> +
> + labelid = rtlabel_name2id(kl->label);
Or even here right before it's used. `kr6' is not used above.
> + if (action == RTM_ADD) {
pfctl.8: mention that -T{add,replace} creates persistent tables
If the given table "t" does not exist, `pfctl -t t -T replace' and `pfctl -t t -T add ::1' will create it persistently: # pfctl -sT # pfctl -tt -Tr 1 table created. no changes. # pfctl -vsT -pa t pf.conf(5) provides this information: Tables can be defined with any of the following pfctl(8) mechanisms. As with macros, reserved words may not be used as table names. manually Persistent tables can be manually created with the add or replace option of pfctl(8), before or after the ruleset has been loaded. [...] Tables may be defined with the following attributes: [...] persist The persist flag forces the kernel to keep the table even when no rules refer to it. If the flag is not set, the kernel will automatically remove the table when the last rule referring to it is flushed. But I miss it in pfctl(8)'s actual description of these table commands where users are probably referring to first when looking into manual table management. Being clear about it does not hurt and barely adds redundant information. Feedback? Suggestions for better wording? OK? Index: pfctl.8 === RCS file: /cvs/src/sbin/pfctl/pfctl.8,v retrieving revision 1.173 diff -u -p -r1.173 pfctl.8 --- pfctl.8 21 Dec 2018 11:16:04 - 1.173 +++ pfctl.8 30 Dec 2018 15:09:52 - @@ -459,7 +459,7 @@ Kill a table. Flush all addresses of a table. .It Fl T Cm add Add one or more addresses in a table. -Automatically create a nonexisting table. +Automatically create a persistent table if it does not exist. .It Fl T Cm delete Delete one or more addresses from a table. .It Fl T Cm expire Ar number @@ -471,7 +471,7 @@ For entries which have never had their s refers to the time they were added to the table. .It Fl T Cm replace Replace the addresses of the table. -Automatically create a nonexisting table. +Automatically create a persistent table if it does not exist. .It Fl T Cm show Show the content (addresses) of a table. .It Fl T Cm test
pfctl: brace anchors must not be empty
There's a (subtle) bug in anchor creation/handling I haven't quite
pinned down yet:
Nested brace anchors with names end up being loaded under a different
name if their ruleset is empty:
$ pfctl -aa1 -vnf-
anchor a2 {
}
match
^D
anchor "a1/a2" all
match all
$ pfctl -vnf-
anchor a1 {
anchor a2 {
}
match
}
^D
anchor "a1" all {
anchor "_1/a2" all
match all
}
But it works fine without nesting or anonymous brace anchors:
$ pfctl -vnf-
$ pfctl -vnf-
anchor a1 {
}
anchor a2 {
anchor {
}
}
anchor {
}
^D
anchor "a1" all
anchor "a2" all {
anchor all
}
anchor all
Due to how the parser works, an anchor's ruleset `{ ... }' is fully
parsed before the anchor rule `anchor a' itself, so a temporary anchor
gets created to load the ruleset, continue parsing and eventually move
it place the anchor rule completes (see sbin/pfctl/parse.y:836).
That's where these "_1/a2" come from; anchor names starting with an
underscore are reservered for internal use and therefore rejected on
user-input. Any attempt to fill them will fail, hence one must at least
completely refill the parent anchor to get rid of them.
Still looking into the underlying problem, I'd like to generally prevent
such usage of empty brace-delimited blocks as they're of no use and will
only cause trouble.
* `anchor a {}' is equivalent to `anchor a'
* `anchor {}' cannot be filled with `pfctl -a' as it has no name and
`anchor' is invalid, so that is even more useless
If one ever used brace anchors with empty rulesets, above things would
happen. If not, anchors are either filled right away or braces are left
out in the first place, so I'd argue throwing a syntax error here is a
breaking syntax change without actual breakage - quite the opposite.
Thanks to benno for going into yacc details with me.
pfctl regress passes.
Feedback? Objections? OK?
Index: share/man/man5/pf.conf.5
===
RCS file: /cvs/src/share/man/man5/pf.conf.5,v
retrieving revision 1.577
diff -u -p -r1.577 pf.conf.5
--- share/man/man5/pf.conf.512 Jul 2018 05:54:49 - 1.577
+++ share/man/man5/pf.conf.530 Dec 2018 19:54:52 -
@@ -1822,7 +1822,7 @@ An anchor rule can also contain a filter
in a brace-delimited block.
In that case, no separate loading of rules into the anchor
is required.
-Brace delimited blocks may contain rules or other brace-delimited blocks.
+Brace delimited blocks must contain rules or other brace-delimited blocks.
When an anchor is populated this way the anchor name becomes optional.
Since the parser specification for anchor names is a string,
double quote characters
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.688
diff -u -p -r1.688 parse.y
--- sbin/pfctl/parse.y 15 Nov 2018 03:22:01 - 1.688
+++ sbin/pfctl/parse.y 30 Dec 2018 19:58:16 -
@@ -501,6 +501,7 @@ int parseport(char *, struct range *r, i
%typeoptweight
%type dir af optimizer syncookie_val
%type sourcetrack flush unaryop statelock
+%type pfa_anchorlist
%type action
%type flags flag blockspec prio
%type portplain portstar portrange
@@ -813,11 +814,11 @@ anchorname: STRING{ $$ =
$1; }
| /* empty */ { $$ = NULL; }
;
-pfa_anchorlist : /* empty */
- | pfa_anchorlist '\n'
- | pfa_anchorlist pfrule '\n'
- | pfa_anchorlist anchorrule '\n'
- | pfa_anchorlist include '\n'
+pfa_anchorlist : /* empty */ { $$ = 0; }
+ | pfa_anchorlist '\n' { $$ = 1; }
+ | pfa_anchorlist pfrule '\n'{ $$ = 1; }
+ | pfa_anchorlist anchorrule '\n'{ $$ = 1; }
+ | pfa_anchorlist include '\n' { $$ = 1; }
;
pfa_anchor : '{'
@@ -844,6 +845,10 @@ pfa_anchor : '{'
pf->anchor = rs->anchor;
} '\n' pfa_anchorlist '}'
{
+ if (!$4) {
+ yyerror("brace anchors must not be empty");
+ YYERROR;
+ }
pf->alast = pf->anchor;
pf->asd--;
pf->anchor = pf->astack[pf->asd];
Re: trunk shouldnt care if it's stacked
On Wed, Jan 09, 2019 at 01:12:31PM +1000, David Gwynne wrote: > -#define TRUNK_MAX_STACKING 4 /* maximum number of stacked trunks */ Is this an arbitrary limit or does it conceal other limitiations? The commit that added it lacks this information: revision 1.2 date: 2005/05/24 07:51:53; author: reyk; state: Exp; lines: +44 -10; support trunk stacking (trunks as trunk ports) and some fixes ok brad@ Since the flag TRUNK_PORT_STACK in if_trunk.h:33 is now unused, maybe add a comment to it for clarity?
Re: pfctl.8: mention that -T{add,replace} creates persistent tables
On Mon, Dec 31, 2018 at 07:05:06AM +, Jason McIntyre wrote: > i'm not a huge fan of how it reads now anyway. but your proposal makes > sense. so, without wanting to pick the text apart right now, i say just > go for it. The change is kept small deliberately, as I did not want to rewrite this section now. We can improve a few other things around here, but this should be a separate diff.
Re: pfctl: prevent modifying internal anchors through their tables
On Wed, Sep 12, 2018 at 02:05:25PM +0200, Alexander Bluhm wrote: > On Tue, Sep 11, 2018 at 12:17:05PM +0200, Klemens Nanni wrote: > > Now `t' under the anonymous anchors (internally named "_1") must not be > > modified through pfctl: > > > > # pfctl -a _1 -t t -T flush > > 0 addresses deleted. > > Why do you think that this semantic is wrong? Why should tables > within an anonoumus anchor be constant? Because that's what I count as modifying reserved anchors from the command line, similar to how adding/removing rules or further anchors below it. Thinking about it after your question made me realise that I'm not checking whether the table is used exclusively within the reserved anchor. Contrary to rules, the same table may be used in multiple places and my diff would rather naively prevent write access to them. Given the case of changing reserved anchors on the command line is already a corner case, trying to prevent users from editing automatically generated tables within them is even more so. Thanks for your feedback.
Re: mount.8: clarify -a description
On Fri, Sep 14, 2018 at 01:23:05PM +0100, Jason McIntyre wrote: > hi. i'm not so keen - we use this syntax in a lot of pages. it is not > vague. i don;t see how changing it makes anything clearer. So is "Sames as" used in a lot of places. It seemed worth suggesting, but I won't push it as there's nothing wrong per se with the current wording.
mount.8: clarify -a description
"Similar" can be a bit vague; I thought `mount -a -t nfs' would mount all NFS shares except those already mounted. This might be just be, but a little emphasis makes it even clearer that this is not the case. Feedback? OK? Index: mount.8 === RCS file: /cvs/src/sbin/mount/mount.8,v retrieving revision 1.89 diff -u -p -r1.89 mount.8 --- mount.8 18 Jan 2018 08:57:12 - 1.89 +++ mount.8 14 Sep 2018 12:04:19 - @@ -109,7 +109,7 @@ or .Dq net options are specified. .It Fl a -Similar to the +Same as the .Fl A flag, except that if a file system (other than the root file system) appears to be already mounted,
bgpd: sync host*() changes from pfctl
This simplifies host() and merges host_v{4,6}() into host_ip() as
recently done for pfctl and ntpd.
config regress still passes but I don't have a real BGP setup to tinker
with so proper testing is highly appreciated.
Feedback? OK?
Index: config.c
===
RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
retrieving revision 1.73
diff -u -p -r1.73 config.c
--- config.c9 Sep 2018 11:00:51 - 1.73
+++ config.c18 Sep 2018 21:22:22 -
@@ -37,8 +37,7 @@
#include "log.h"
u_int32_t get_bgpid(void);
-inthost_v4(const char *, struct bgpd_addr *, u_int8_t *);
-inthost_v6(const char *, struct bgpd_addr *);
+inthost_ip(const char *, struct bgpd_addr *, u_int8_t *);
void free_networks(struct network_head *);
void free_rdomains(struct rdomain_head *);
@@ -317,80 +316,58 @@ get_bgpid(void)
int
host(const char *s, struct bgpd_addr *h, u_int8_t *len)
{
- int done = 0;
- int mask;
+ int mask = 128;
char*p, *ps;
const char *errstr;
- if ((p = strrchr(s, '/')) != NULL) {
- mask = strtonum(p + 1, 0, 128, );
+ if ((ps = strdup(s)) == NULL)
+ fatal("%s: strdup", __func__);
+
+ if ((p = strrchr(ps, '/')) != NULL) {
+ mask = strtonum(p+1, 0, 128, );
if (errstr) {
- log_warnx("prefixlen is %s: %s", errstr, p + 1);
+ log_warnx("prefixlen is %s: %s", errstr, p+1);
return (0);
}
- if ((ps = malloc(strlen(s) - strlen(p) + 1)) == NULL)
- fatal("host: malloc");
- strlcpy(ps, s, strlen(s) - strlen(p) + 1);
- } else {
- if ((ps = strdup(s)) == NULL)
- fatal("host: strdup");
- mask = 128;
- }
-
- bzero(h, sizeof(struct bgpd_addr));
-
- /* IPv4 address? */
- if (!done)
- done = host_v4(s, h, len);
-
- /* IPv6 address? */
- if (!done) {
- done = host_v6(ps, h);
- *len = mask;
+ p[0] = '\0';
}
- free(ps);
-
- return (done);
-}
+ bzero(h, sizeof(*h));
-int
-host_v4(const char *s, struct bgpd_addr *h, u_int8_t *len)
-{
- struct in_addr ina = { 0 };
- int bits = 32;
-
- if (strrchr(s, '/') != NULL) {
- if ((bits = inet_net_pton(AF_INET, s, , sizeof(ina))) == -1)
- return (0);
- } else {
- if (inet_pton(AF_INET, s, ) != 1)
- return (0);
+ if (host_ip(ps, h, len) == 0) {
+ free(ps);
+ return (0);
}
- h->aid = AID_INET;
- h->v4.s_addr = ina.s_addr;
- *len = bits;
+ if (p != NULL)
+ *len = mask;
+ free(ps);
return (1);
}
int
-host_v6(const char *s, struct bgpd_addr *h)
+host_ip(const char *s, struct bgpd_addr *h, u_int8_t *len)
{
struct addrinfo hints, *res;
+ int bits;
bzero(, sizeof(hints));
- hints.ai_family = AF_INET6;
+ hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_DGRAM; /*dummy*/
hints.ai_flags = AI_NUMERICHOST;
- if (getaddrinfo(s, "0", , ) == 0) {
+ if (getaddrinfo(s, NULL, , ) == 0) {
+ *len = res->ai_family == AF_INET6 ? 128 : 32;
sa2addr(res->ai_addr, h);
freeaddrinfo(res);
- return (1);
+ } else {/* ie. for 10/8 parsing */
+ if ((bits = inet_net_pton(AF_INET, s, >v4, sizeof(h->v4)))
== -1)
+ return (0);
+ *len = bits;
+ h->aid = AID_INET;
}
- return (0);
+ return (1);
}
void
===
Stats: --- 50 lines 1196 chars
Stats: +++ 27 lines 785 chars
Stats: -23 lines
Stats: -411 chars
Re: regress/bgpd: allow specifying daemon binary
On Tue, Sep 18, 2018 at 03:44:27PM -0600, Theo de Raadt wrote: > I honestly think this is a foolishly complicated. > > Just install the program, then run regress. Install an older version > without the broken changes if it doesn't work. > > I tire of these interactions between environment variables, > base build methods, fork+exec paths in privsep programs, and now > getting tied into regress tests. > > In a word, YUCK. > > I think this isn't "convenience". Rather it comes off as artifically > complicated, trying to solve a problem which doesn't need to be exist > at all. Perhaps even perceiving there to be a problem which needs > solving via such abstration is the true problem. For me it clearly is convenient and actually less overhead. Why elevating to root and writing to /usr for each test instead of just pointing it at my local build?
regress/bgpd: allow specifying daemon binary
Same as in pfctl or route so I can easily test my changes with
$ make BGPD=/usr/obj/usr.sbin/bgpd/bgpd config
OK?
Index: config/Makefile
===
RCS file: /cvs/src/regress/usr.sbin/bgpd/config/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- config/Makefile 10 Sep 2018 14:20:25 - 1.5
+++ config/Makefile 18 Sep 2018 20:53:24 -
@@ -1,5 +1,7 @@
# $OpenBSD: Makefile,v 1.5 2018/09/10 14:20:25 benno Exp $
+BGPD ?= /usr/sbin/bgpd
+
BGPDTESTS=1 2 3 4 5 6 7 8
REGRESS_TARGETS = config
@@ -9,12 +11,12 @@ BGPD_TARGETS+=bgpd${n}
BGPD_UPDATES+=bgpd${n}-update
bgpd${n}:
- bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
+ ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
sed 's/router-id .*/router-id 127.0.0.1/' | \
diff -u ${.CURDIR}/bgpd.conf.${n}.ok /dev/stdin
bgpd${n}-update:
- bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
+ ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
sed 's/router-id .*/router-id 127.0.0.1/' > \
${.CURDIR}/bgpd.conf.${n}.ok
.endfor
@@ -24,11 +26,11 @@ bgpd-update: ${BGPD_UPDATES}
# check that the example configuration file we ship is ok
bgpd-example:
- bgpd -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
+ ${BGPD} -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
# check that the output of bgpd -nvv is parseable
bgpd-printconf:
- bgpd -nvf ${.CURDIR}/bgpd.conf.printconf | \
- bgpd -nf /dev/stdin
+ ${BGPD} -nvf ${.CURDIR}/bgpd.conf.printconf | \
+ ${BGPD} -nf /dev/stdin
.include
Re: Convert a few more timeout_add() calls
On Wed, Dec 19, 2018 at 07:57:11PM +0100, Claudio Jeker wrote: > This is mostly replacing timeout_add calls that use some sort of HZ > dependent value to timeout_add_(m)sec(). IFNET_SLOWHZ is only used in one > place and could be moved there. > In general I think the result is easier to understand. Yes, OK.
Re: bridge_ourether() tweak
On Tue, Jan 22, 2019 at 12:58:56PM -0200, Martin Pieuchot wrote: > Directly pass a pointer, which implies we have a valid reference for > the given interface. This will matters when we'll start using ifp > indexes. Yes, OK kn.
Re: introduce 'pfctl -FR' to reset settings to defaults
On Tue, Apr 02, 2019 at 11:28:43AM +0200, Petr Hoffmann wrote: > would make me believe everything mentioned as OPTIONS in pf.conf(5) is about > to be reset. I see e.g. the debug level is reset, but what about the other > stuff like fingerprints, 'skip on' and other options set via the 'set' > command? Maybe the manpage should be more precise here? Seems fine to me, given that a) some options are not persisted in the driver but only effective during ruleset parsing and b) stuff like fingerprints is already flushed separately, see pfctl(8) `-F osfp'.
Re: ksh "clear-screen" editing command
On Tue, Apr 02, 2019 at 05:20:19PM +0200, Theo Buehler wrote: > Yes, ^L is printed in vi insert mode. The text you quoted is about vi > command mode which does indeed redraw the current line on ^L. I agree > with jca, no need for a change there. I'm confused. Without jca's diff, I did the following: xterm -e /bin/sh $ set -o vi $ ^L^L # literal escape, nothing happens $ set -o emacs $ true^L# redraws the line $ true So where does ^L redraw the current line on ^L for you?
Re: introduce 'pfctl -FR' to reset settings to defaults
On Tue, Apr 02, 2019 at 02:01:05PM +0200, Alexandr Nedvedicky wrote:
> I think Petr is right here. my patch requires yet another finishing touch:
Fair enough, but it should be noted that this somewhat changes behaviour
of the existing interface:
> 8<---8<---8<--8<
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index 40929d90530..032fdd08b57 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -2267,6 +2267,8 @@ pfctl_reset(int dev, int opts)
>
> if (pfctl_trans(dev, , DIOCXCOMMIT, 0))
> warn("%s, DIOCXCOMMIT", __func__);
> +
> + pfctl_clear_interface_flags(dev, opts);
Now this is done with `-F reset' and therefore `-F all'...
> }
>
> int
> @@ -2594,7 +2596,6 @@ main(int argc, char *argv[])
> pfctl_clear_src_nodes(dev, opts);
> pfctl_clear_stats(dev, ifaceopt, opts);
> pfctl_clear_fingerprints(dev, opts);
> - pfctl_clear_interface_flags(dev, opts);
Where previously, without being documented, only `-F all' would do so.
> pfctl_reset(dev, opts);
> }
I think that is fine in this particular case, but clearing things in
specific flush commands that were previously only touched by the `all'
hammer can be dangerous.
Re: ksh: quote empties
On Sun, Dec 30, 2018 at 02:43:37PM -0800, Philip Guenther wrote: > This thread was never resolved/committed. Looking again at the diffs, I > still think I prefer that we _not_ touch print_value_quoted(), as the > other callers all use the 'key=value' format and don't need special > handling of empty values, leaving a diff like the one below. I think this is a sensible approach. OK kn
Re: ksh "clear-screen" editing command
On Tue, Apr 02, 2019 at 10:52:34AM +0200, Jeremie Courreges-Anglas wrote: > So here's a diff. oks/nays? OK with the one mention in sh(1) adjusted as well: There are two modes, interactive and command. The shell starts in interactive mode. In this mode text is entered normally. A ⟨newline⟩ executes the current command line. The command line, unless empty, is entered into command history. The ⟨ESC⟩ key is used to enter command mode, where commands similar to those used by vi(1) are available. A Ctrl-L sequence (^L) can be used in this mode to redraw the current command line.
Re: ksh "clear-screen" editing command
On Tue, Apr 02, 2019 at 11:39:05AM -0400, Andras Farkas wrote: > $ set -o vi > $ true^[^L #redraws the line > $ true > > vi uses the escape or ^[ character to go into command mode from insert mode Ooooh... I blatantly tried ^L without ESC in vi mode, of course that won't work. Yup, sorry for the noise. OK kn (again)
Re: ksh "clear-screen" editing command
On Tue, Apr 02, 2019 at 04:56:58PM +0200, Jeremie Courreges-Anglas wrote: > The diff changes only the emacs mode. I don't think sh.1 needs to be > adjusted given that the paragraph you quote is about vi mode. Sure it's just emacs mode. But for sh(1), ^L does print a literal "^L" in vi mode; in emacs mode, it currently refreshes the line as this paragraph states.
Re: route.4: Recommend ROUTE_TABLEFILTER
On Wed, Apr 03, 2019 at 09:53:46AM +0200, Klemens Nanni wrote: > While here, document RTABLE_ANY and mention rtable(4). Improved versiono that uses 5 as example and marks up RTABLE_ANY inline instead, making it searchable with `man -k .=RTABLE_ANY'. OK? Index: route.4 === RCS file: /cvs/src/share/man/man4/route.4,v retrieving revision 1.49 diff -u -p -r1.49 route.4 --- route.4 2 Apr 2019 14:12:09 - 1.49 +++ route.4 3 Apr 2019 08:18:43 - @@ -194,8 +194,19 @@ by doing a system call for further input. .Pp A process can specify an alternate routing table by using the -.Dv SO_RTABLE +.Dv ROUTE_TABLEFILTER .Xr setsockopt 2 . +A value of +.Dv RTABLE_ANY +specifies all routing tables. +For example, to receive messsags for routing table 5: +.Bd -literal -offset indent +unsigned int rdomain = 5; + +if (setsockopt(routefd, AF_ROUTE, ROUTE_TABLEFILTER, +, sizeof(rdomain)) == -1) + err(1, "setsockopt(ROUTE_TABLEFILTER)"); +.Ed .Pp A process can specify which route message types it's interested in by using @@ -469,6 +480,7 @@ Specifiers for which addresses are prese .Xr netstat 1 , .Xr socket 2 , .Xr sysctl 2 , +.Xr rtable 4 , .Xr mygate 5 , .Xr route 8 , .Xr route 9
route.4: Recommend ROUTE_TABLEFILTER
After claudio helped me with some details, here's the first round of improvements. SO_TABLE is not applicable to AF_ROUTE, ROUTE_TABLEFILTER works across all families and goes well in line with the other ROUTE_* macros. While here, document RTABLE_ANY and mention rtable(4). OK? Index: route.4 === RCS file: /cvs/src/share/man/man4/route.4,v retrieving revision 1.49 diff -u -p -r1.49 route.4 --- route.4 2 Apr 2019 14:12:09 - 1.49 +++ route.4 3 Apr 2019 07:52:39 - @@ -194,8 +194,16 @@ by doing a system call for further input. .Pp A process can specify an alternate routing table by using the -.Dv SO_RTABLE +.Dv ROUTE_TABLEFILTER .Xr setsockopt 2 . +For example, to receive messsags for all routing tables: +.Bd -literal -offset indent +unsigned int rdomain = RTABLE_ANY; + +if (setsockopt(routefd, AF_ROUTE, ROUTE_TABLEFILTER, +, sizeof(rdomain)) == -1) + err(1, "setsockopt(ROUTE_TABLEFILTER)"); +.Ed .Pp A process can specify which route message types it's interested in by using @@ -469,6 +477,7 @@ Specifiers for which addresses are prese .Xr netstat 1 , .Xr socket 2 , .Xr sysctl 2 , +.Xr rtable 4 , .Xr mygate 5 , .Xr route 8 , .Xr route 9
Re: [patch] Re: Possible sasyncd memory leak ?
On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote:
> > I also fixed a case of parsing IPv6 addresses.
> >
> > Anyone willing to ok?
See comments inline.
> And now also with a lexer bug fixed. Earlier I thougt it was an order
> dependency in the clauses. But is was an order dependency in comment
> lines and empty lines.
> +check_peer_addr(const char *peer_addr)
> +{
> + int valid = 1;
> + short peer_family = AF_UNSPEC;
> + struct ifaddrs *ifap = NULL, *ifa;
> + struct syncpeer *peer;
> + struct sockaddr_in sa;
> + struct sockaddr_in6 sa6;
> +
> + if (inet_pton(AF_INET, peer_addr, _addr) == 1)
> + peer_family = AF_INET;
> +
> + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr,
> + _addr) == 1)
> + peer_family = AF_INET6;
`peer_addr' must not be a CIDR network, so I'd go with the more AF
agnostic getaddrinfo(3) and check for res->ai_family in any case...
> + if (peer_family == AF_UNSPEC) {
> + log_msg(2, "config: skip unparseable peer %s", peer_addr);
> + valid = 0;
> + }
...but `peer_addr' may also be a hostname, so that is not handled by
your diff:
net.h: char*name; /* FQDN or an IP, from conf */
getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used
here.
Either way, this is a job for host_ip() as found in pfctl or bgpd.
Other daemons like iked still have host_v4() and host_v6(). Parsers use
these functions to check for valid addresses, so I'd say sasyncd should
be no exception and adopt the same approach.
> @@ -325,7 +386,7 @@ yylex(void)
> /* Numerical token? */
> if (isdigit(*confptr)) {
> for (p = confptr; *p; p++)
> - if (*p == '.') /* IP address, or bad input */
> + if (*p == '.' || *p == ':') /* IP address, or bad input
> */
This fixes the parser as in
# echo peer 2001:db8::1 > conf
# sasyncd -dnv -c conf
config: syntax error
# ./obj/sasyncd -dnv -c conf
configuration OK
But I have not actually tested this with a proper IPv6 setup since I do
not use sasyncd; did you?
> @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile)
> if (*s == '#') {
> while (*s != '\n' && s < buf + conflen)
> s++;
> + while (*s == '\n' && s < buf + conflen)
> + s++;
> + s--;
OK kn for this fix alone.
Re: [patch] Re: Possible sasyncd memory leak ?
On Thu, Mar 21, 2019 at 11:52:56AM +0100, Otto Moerbeek wrote: > Meanwhile, I tested a IPv6 setup, it works ok. > So I'm going to commit the diff below, Thanks! OK kn
Re: introduce 'pfctl -FR' to reset settings to defaults
On Wed, Apr 03, 2019 at 11:10:21AM +0200, Alexandr Nedvedicky wrote: > I did look at pf.conf(5) manpage yesterday. It requires more updates, > which > I would like to leave for another diff. For example pf.conf(5) does not > mention default values for limits and time outs. I expect some discussion > on how far we want to get with details. So let's exclude pf.conf(5) this > time. Yes, this bothered me as well; I also agree on tackling this with separate diffs. > below is the diff I'd like to commit. OK kn
Re: invalid netmasks should be reported
On Wed, Mar 27, 2019 at 12:34:52PM +0100, Petr Hoffmann wrote:
> I noticed it is possible to specify an invalid netmask,
> e.g. 1.1.1.1/10/20 and still get the address loaded into a table. I
> conjecture this was introduced by the following change:
>
> a7ede25358dad545e0342d2a9f8ef6ce68c6df66
> Zap bits in host_v4(), use mask parameter
That was me:
revision 1.326
date: 2018/07/31 22:48:04; author: kn; state: Exp; lines: +10 -11
Zap v4mask and v6mask in host()
Simply defer checks whether a mask has been specified to where it's set
in
host_*(); this is to reduce address family specific code.
OK sashan
> It looks like the author missed the path addresses are loaded by pfctl's '-T
> add' command. I guess the '/20' is dropped in host() and then '/10' is
> processed within host_ip() by inet_net_pton() so no error is reported.
Good find, thanks.
> OLD:
> # pfctl -t tableta -T add 1.1.1.1/10/20
> 1 table created.
> 1/1 addresses added.
>
> NEW:
> # $PFCTL -t tableta -T add
> 1.1.1.1/10/20
> netmask is invalid: /10/20
Yup; this only affects tables, though. For the ruleset parser, strings
with more than one "/" already fail the `xhost' production in parse.y:
$ pfctl -vnf-
pass to 1/2/3
stdin:1: syntax error
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index ee3c2926f1a..5737846123d 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -1627,7 +1627,7 @@ host(const char *s, int opts)
> if_name++;
> }
>
> - if ((p = strrchr(ps, '/')) != NULL) {
> + if ((p = strchr(ps, '/')) != NULL) {
> mask = strtonum(p+1, 0, 128, );
> if (errstr) {
> fprintf(stderr, "netmask is %s: %s\n", errstr, p);
OK kn; anyone else?
Re: introduce 'pfctl -FR' to reset settings to defaults
On Wed, Mar 27, 2019 at 02:17:03AM +0100, Alexandr Nedvedicky wrote: > tedu@ has planted idea for diff below here [1]. That particular email is part > of thread [2], where various cleanup/unconfigure options for PF are discussed. > To keep progressing in small steps I've decided to factor out the first diff > here, which introduces '-FR' (a.k.a. reset settings) for pfctl(8). A bit late, but I generally agree on "reset" and reusing `-F'. Diff looks sane to me, two comments: > -Flush all of the above. > +Flush all of the above (+ reset settings). This is fine as is, I think. > +void pfctl_restore_defaults(int, int); Why not simply pfctl_reset()?
iwm: fix RF_KILL interrupt handling
Coming from an UP and RUNNING interface, turning off the hardware kill
switch removes the RUNNING flag and powers down the device.
Iff still UP, switching it back on should set RUNNING again to ensure
seemless operation at runtime.
We can do this by fixing the interrupt handler which currently only
cares about the on->off edge, effectively leaving the device powered
down and unitialized after the off->on edge.
Diff below schedules iwm_init() unconditionally on receiving the RF_KILL
interrupt; it'll take care of everything else. Jumping to the `out'
label is omitted for simplicity here, it is crucial to restore
restore interrupts in this case. See the end of iwm_intr() inlined here
for ease of review:
out_ena:
iwm_restore_interrupts(sc);
out:
return rv;
}
With this, I can toggle my X230's hardware switch to kill the wifi and
toggle it back again to get an uplink again without having to pull it
up manually.
See if_iwn.c revision 1.204 from 11.01.2019 for a pure iwn(4) equivalent
(note how the commit's message last sentence
"iwm(4) already behaves this way." is an obvious lie; I got confused
during development of that commit.)
Tested with
iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless AC 7260" rev 0xbb, msi
iwm0: hw rev 0x140, fw ver 16.242414.0, address xx:xx:xx:xx:xx:xx
OK?
NB: This does not fix the resume path yet. Toggling the switch
off, suspending, toggling it back on and resuming will currently still
leave the device powered down. I have a separate diff for that in the
queue (equivalent to if_iwn.c revision 1.207 from 26.02.2019).
Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.237
diff -u -p -r1.237 if_iwm.c
--- if_iwm.c27 Feb 2019 07:47:57 - 1.237
+++ if_iwm.c1 Apr 2019 07:06:20 -
@@ -7339,11 +7339,8 @@ iwm_intr(void *arg)
if (r1 & IWM_CSR_INT_BIT_RF_KILL) {
handled |= IWM_CSR_INT_BIT_RF_KILL;
- if (iwm_check_rfkill(sc)) {
- task_add(systq, >init_task);
- rv = 1;
- goto out;
- }
+ iwm_check_rfkill(sc);
+ task_add(systq, >init_task);
}
if (r1 & IWM_CSR_INT_BIT_SW_ERR) {
iwm: enable RF_KILL interrupts on resume
As promised in my earlier mail, here's a diff that fixes seemless operation with the hardware kill switch on resume after suspend. Like the interrupt handler, the resume path needs to check the register to update flags in order to propagate the hardware kill switch state, otherwise the driver would still consider the switch to be off after resume even though it may have changed while in S3. The same idiom can be found in iwm_start_hw() already; it's pretty obvious by now. With that, the following works just fine: UP and RUNNING RF switch off suspend RF switch -> on resume UP and RUNNING again OK? Index: if_iwm.c === RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v retrieving revision 1.237 diff -u -p -r1.237 if_iwm.c --- if_iwm.c27 Feb 2019 07:47:57 - 1.237 +++ if_iwm.c1 Apr 2019 09:20:05 - @@ -7892,6 +7889,9 @@ iwm_resume(struct iwm_softc *sc) /* Clear device-specific "PCI retry timeout" register (41h). */ reg = pci_conf_read(sc->sc_pct, sc->sc_pcitag, 0x40); pci_conf_write(sc->sc_pct, sc->sc_pcitag, 0x40, reg & ~0xff00); + + iwm_enable_rfkill_int(sc); + iwm_check_rfkill(sc); return iwm_prepare_card_hw(sc); }
vmctl: report reliable VM state
As of now, `vmctl status test' will tell you whether the VM is running
or not; except that "STATE" actually denotes whether the VCPU is
currently running or haltet, not whether the VM is started/running or
stopped.
I tripped over this when trying to use
vmctl status test | fgrep 'STATE: RUNNING'
as reliable check, where it therefore only worked when the VM was busy
CPU wise. According to mlarkin, this was only ever intended as internal
debugging aid anyway.
The following diff makes vmctl(8) switch it's state indicator from the
actual VCPU state to whether the VM has a valid process ID, that is
simply whether the respective host process runs.
Since there's only ever one VCPU per VM for now, drop the constant
"VCPU: 0" all together and stick with "STATE: RUNNING" or
"STATE: STOPPED" on it's own line. That makes it easily scriptable.
Given that the current behaviour is only vaguely documented in vmctl(8),
I do not expect users to already depend on it; if so: now would be the
time to change that into something consistent. I have not come up with
a nice update to the manual yet, suggestions welcome.
Feedback? OK?
Index: usr.sbin/vmctl//vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.65
diff -u -p -r1.65 vmctl.c
--- usr.sbin/vmctl//vmctl.c 6 Dec 2018 09:23:15 - 1.65
+++ usr.sbin/vmctl//vmctl.c 1 Apr 2019 15:01:34 -
@@ -716,12 +716,13 @@ print_vm_info(struct vmop_info_result *l
{
struct vm_info_result *vir;
struct vmop_info_result *vmi;
- size_t i, j;
- char *vcpu_state, *tty;
+ size_t i;
+ char *tty;
char curmem[FMT_SCALED_STRSIZE];
char maxmem[FMT_SCALED_STRSIZE];
char user[16], group[16];
const char *name;
+ int running;
printf("%5s %5s %5s %7s %7s %7s %12s %s\n", "ID", "PID", "VCPUS",
"MAXMEM", "CURMEM", "TTY", "OWNER", "NAME");
@@ -729,6 +730,7 @@ print_vm_info(struct vmop_info_result *l
for (i = 0; i < ct; i++) {
vmi = [i];
vir = >vir_info;
+ running = (vir->vir_creator_pid != 0 && vir->vir_id != 0);
if (check_info_id(vir->vir_name, vir->vir_id)) {
/* get user name */
name = user_from_uid(vmi->vir_uid, 1);
@@ -757,7 +759,7 @@ print_vm_info(struct vmop_info_result *l
(void)fmt_scaled(vir->vir_memory_size * 1024 * 1024,
maxmem);
- if (vir->vir_creator_pid != 0 && vir->vir_id != 0) {
+ if (running) {
if (*vmi->vir_ttyname == '\0')
tty = "-";
/* get tty - skip /dev/ path */
@@ -781,19 +783,8 @@ print_vm_info(struct vmop_info_result *l
}
}
if (check_info_id(vir->vir_name, vir->vir_id) > 0) {
- for (j = 0; j < vir->vir_ncpus; j++) {
- if (vir->vir_vcpu_state[j] ==
- VCPU_STATE_STOPPED)
- vcpu_state = "STOPPED";
- else if (vir->vir_vcpu_state[j] ==
- VCPU_STATE_RUNNING)
- vcpu_state = "RUNNING";
- else
- vcpu_state = "UNKNOWN";
-
- printf(" VCPU: %2zd STATE: %s\n",
- j, vcpu_state);
- }
+ printf("STATE: %s\n",
+ running ? "RUNNING" : "STOPPED");
}
}
}
===
Stats: --- 16 lines 430 chars
Stats: +++ 7 lines 184 chars
Stats: -9 lines
Stats: -246 chars
Re: ksh "clear-screen" editing command
On Mon, Apr 01, 2019 at 09:53:31AM -0600, Todd C. Miller wrote: > AT ksh doesn't clear the screen by default on ^L. Other shells > like bash, zsh, and tcsh do. I don't object to making it the default > but as I'm not a ksh user I'll defer to those who are. Although I'm mostly using ksh in Vi mode, having ^L default to the same behaviour as bash and zsh would contribute to a slightly more consistent feeling across different systems/shells which I do appreciate.
Re: xidle: parse options once, simplify code
On Mon, Nov 26, 2018 at 06:40:05PM +0100, Klemens Nanni wrote:
> On Sun, Nov 11, 2018 at 06:07:10PM +0100, Klemens Nanni wrote:
> > There's no point in parsing `-display' separately, just do it once and
> > simplify the code while here.
> >
> > This addresses two of cheloha's comments from my strtonum diff.
> Ping.
>
> Feedback? OK?
Found this X diff I still want to get rid of; one last try but with all
the churn removed this time.
Any takers?
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.8
diff -u -p -r1.8 xidle.c
--- xidle.c 11 Nov 2018 16:10:37 - 1.8
+++ xidle.c 1 Apr 2019 11:20:07 -
@@ -76,12 +76,9 @@ struct xinfo {
struct xinfo x;
static XrmOptionDescRec fopts[] = {
- { "-display", ".display", XrmoptionSepArg,(caddr_t)NULL },
-};
-
-static XrmOptionDescRec opts[] = {
{ "-area", ".area",XrmoptionSepArg,(caddr_t)NULL },
{ "-delay", ".delay", XrmoptionSepArg,(caddr_t)NULL },
+ { "-display", ".display", XrmoptionSepArg,(caddr_t)NULL },
{ "-program", ".program", XrmoptionSepArg,(caddr_t)NULL },
{ "-timeout", ".timeout", XrmoptionSepArg,(caddr_t)NULL },
@@ -263,9 +260,11 @@ parse_opts(int argc, char **argv, Displa
XrmInitialize();
- /* Get display to open. */
+ /* Get command line values. */
XrmParseCommand(, fopts, sizeof(fopts) / sizeof(fopts[0]),
__progname, , argv);
+ if (argc > 1)
+ usage();
display = (getres(, rdb, "display", "Display") == True) ?
(char *)value.addr : NULL;
@@ -288,12 +287,6 @@ parse_opts(int argc, char **argv, Displa
XrmMergeDatabases(tdb, );
}
- /* Get remaining command line values. */
- XrmParseCommand(, opts, sizeof(opts) / sizeof(opts[0]),
- __progname, , argv);
- if (argc > 1) {
- usage();
- }
if (getres(, rdb, "area", "Area")) {
*area = strtonum(value.addr, 1, INT_MAX, );
if (errstr)
===
Stats: --- 11 lines 293 chars
Stats: +++ 4 lines 119 chars
Stats: -7 lines
Stats: -174 chars
Re: sys/dev/pci/if_wb.c: repair "} if"
OK
Re: introduce 'pfctl -FR' to reset settings to defaults
On Sat, Apr 06, 2019 at 02:37:05AM +0200, Alexandr Nedvedicky wrote:
> updated diff is attached. I'll commit the change after unlock.
OK kn with comments inline.
> + pf.ifname = strdup("none");
> + if (pf.ifname == NULL)
> + warn("%s: Warning: can't reset loginterface\n", __func__);
> + else
> + pf.ifname_set = 1;
We should fail hard as in almost all other strdup(3) use cases.
Failure means the system ran out of memory, there's no point in going
any further.
So just something like
pf.ifname = strdup("none");
if (pf.ifname == NULL)
err(1, "%s: strdup", __func__);
> + if (pfctl_trans(dev, , DIOCXBEGIN, 0))
> + warn("%s, DIOCXBEGIN", __func__);
Turn this comma into a double colon.
> + if (pfctl_trans(dev, , DIOCXCOMMIT, 0))
> + warn("%s, DIOCXCOMMIT", __func__);
Same here.
Re: apm doesn't take arguments
OK
vmctl: usage on extra arguments
tedu's apm(8) diff reminded me that certain vmctl(8) commands are too
relaxed:
$ vmctl start a b
vmctl: start vm command failed: Operation not permitted
$ vmctl stop a b
stopping vm a: vm not found
$ vmctl create a b
could not create a: missing size argument
usage: vmctl [-v] create disk [-b base | -i disk] [-s size]
$ vmctl create a b -s 1G
could not create a: missing size argument
usage: vmctl [-v] create disk [-b base | -i disk] [-s size]
$ vmctl create a -s 1G b
vmctl: raw imagefile created
`start', `stop' and `create' are the only commands with a variable
number of options; other commands have strict checks already.
With diff below, all examples above print usage directly without doing
anything else.
OK?
Index: usr.sbin/vmctl/main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.52
diff -u -p -r1.52 main.c
--- usr.sbin/vmctl/main.c 14 Dec 2018 07:56:17 - 1.52
+++ usr.sbin/vmctl/main.c 1 Mar 2019 00:29:12 -
@@ -599,6 +599,9 @@ ctl_create(struct parse_result *res, int
}
}
+ if (argc > 0)
+ ctl_usage(res->ctl);
+
if (input) {
if (base && input)
errx(1, "conflicting -b and -i arguments");
@@ -913,6 +916,9 @@ ctl_start(struct parse_result *res, int
}
}
+ if (argc > 0)
+ ctl_usage(res->ctl);
+
for (i = res->nnets; i < res->nifs; i++) {
/* Add interface that is not attached to a switch */
if (parse_network(res, "") == -1)
@@ -953,6 +959,9 @@ ctl_stop(struct parse_result *res, int a
/* NOTREACHED */
}
}
+
+ if (argc > 0)
+ ctl_usage(res->ctl);
/* VM id is only expected without the -a flag */
if ((res->action != CMD_STOPALL && ret == -1) ||
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote: > so far so good. Now let's flush the rules from kernel: > > lumpy# ./pfctl -Fr > rules cleared > lumpy# ./pfctl -sr > lumpy# > > However the underscore anchors are still there: Any unreferenced anchor will remain, not just internal ones. We have no code/logic that clears them automatically. > lumpy# ./pfctl -vsA > _1 > _1/_2 > lumpy# That's a known issue, easily visible if you run the pfctl regress tests. I've been pondering this with different attempts but haven't come up with a good/safe one yet. Perhaps we can teach pfctl something like `-F Anchors`, such that it looks at the ruleset as well as all anchors and removes those which are not referenced. The benefits are that we do not require any further starts-with-underscore quirks and it remains a wilful action done by root. > I could not figure out any existing way to remove them, hence I'm proposing > small patch, which allows me to remove those 'underscore' anchors by doing > this: > > lumpy# ./pfctl -a _1/_2 -Fr > rules cleared > lumpy# ./pfctl -a _1 -Fr > rules cleared Did you look at happens with _1/_2 when you flush _1 directly? > Does patch below make sense? Or are there some pitfalls I'm not aware of? Given your example, using named anchors seems like the right approach. > + /* > + * we want enable administrator to flush anonymous anchors, > + * thus '_' should be allowed for '-Fr' only. Also make sure > + * we fail in case of option combination as follows: > + * pfctl -a _1 -Fr -f /some/rules.conf > + */ > if (mode == O_RDWR && tblcmdopt == NULL && > + (clearopt == NULL || *clearopt != 'r' || > + rulesopt != NULL) && I'm not fond of adding an exception to what already is an exception. The anchor handling code is tricky already and has big potential for pitfalls and subtle bugs (as seen in the past). > (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL)) > errx(1, "anchor names beginning with '_' cannot " > "be modified from the command line"); That said, I think the internal semantics should stay purely internal to avoid another can of worms.
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 12:42:02PM +0100, Alexandr Nedvedicky wrote: > yes, that's what I thought. We have a kind 'service' on Solaris, which > wraps pfctl to manage firewall. If firewall is being enabled, the service > cleans up all rules (anchors). We basically dump the rulesets (pfctl -vsA) > and then traverse from leaves to root to clean it up. That's probaly how I'd approach `-F Anchors'. > so if I understand you right, your scenario for ruleset from my > first email works as follows: > pfctl -Fr # makes the anchors _1 and _1/_2 unreferenced > # (they are not reachable from root any more) > > pfctl -FAnchors # purge all unreferenced anchors. > > the 'unreferenced' means the anchor is not reachable by any packet. > like there is no path for packet between main ruleset and that particular > anchor (and all its descendants). Yes. With the regress suite for example, the following should leave no trace of regress anchors or rules: make pfctl -f /etc/pf.conf pfctl -F Anchors > sure, I agree, adding -FAnchors options i more systemic approach, though > such change is more complex. I think I can give it a try to prototype it. Cool! I'm happy to help here.
Re: bgpctl mrt parser refactor
Diff reads good, although I'm not a BGP user. One nit inline: > @@ -689,31 +690,32 @@ mrt_parse_dump_mp(struct mrt_hdr *hdr, v > - case AF_VPNv4: > + case AID_VPN_IPv4: > if (len < MRT_PREFIX_LEN(r->prefixlen)) > goto fail; > - errx(1, "AF_VPNv4 handling not yet implemented"); > + errx(1, "AID_VPN_IPv4 handling not yet implemented"); > + goto fail; > + case AID_VPN_IPv6: > + if (len < MRT_PREFIX_LEN(r->prefixlen)) > + goto fail; > + errx(1, "AID_VPN_IPv6 handling not yet implemented"); > goto fail; These labels are unreachable after errx(3), so why add them?
Re: bsd.{prog,lib}.mk: drop -S for install
On Thu, Feb 21, 2019 at 02:53:55PM +0200, Lauri Tirkkonen wrote: > Updated diff to remove -S from all files mentioned above. OK kn if anyone wants to commit, otherwise I'll do so on sunday unless I hear objections.
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 03:02:07PM +0100, Alexandr Nedvedicky wrote: > so the option '-F Anchors' will also perform a '-Fr' on main ruleset, is > that correct? No, my `-f /etc/pf.conf' is the equivalent to your `-F rules' here. > And also one more thing, which comes to my mind. How '-F Anchors' should > treat tables attached to anchors? If an unreferenced anchor contains a table, either destroy that table as well since it cannot be referenced or abort garbage collecting anchor completely, possibly prompting the user to clean tables in besaid anchors as well. I tend towards the first option since anchors may contain tables that were automatically created due to ruleset optimization. That is to say, while enforcing manual removal of tables as well, this may become annoying for big optimized rulesets where administrators would have to wield `-F Tables' and or `-a aname -T kill -t tname' before having `-F Anchors' run cleanly. Does that make sense? > the firewall service (which is a kind of rc-script in fact) on Solaris, > kills (removes) all tables first, then it removes anchors. > > What shall we do in case of '-F Anchors'? do we want '-F Anchors' to > kill attached tables too? Or should it just report "anchor can't be > removed, because table is still attached?" See above. What this "service" roughly seems like what I have in mind. > It looks like '-F Anchors' shifts pfctl(8) from simple tool, which does > exactly what it's told to do, to advanced tool, which does more things at > one step. But isn't that a perfect job for pfctl? It manipulates everything, it's the single interface for firewall related and that's a good. Cleaning things up after work is done is a necessity; this is about adding the missing peaces, imho. > The simple tools just seem to be more friendly for scripts, while advanced > tool is easier to use by human. pfctl is well suited for both, and it always should be.
Re: ssh man pages: PKCS#11 no longer limited to RSA
On Tue, Mar 05, 2019 at 04:27:22PM +0100, Christian Weisgerber wrote: > Minor man page tweaks to reflect the fact that PKCS#11 support is > no longer limited to RSA. OK, I've been using ECDSA on a PIV smartcard just fine.
Re: once rules fix
Thanks! Diff makes sense, see comments inline. I confirm that this
restores intended behaviour and regress is fine as well.
With those addressed OK kn; or I take care of it after getting an OK.
sashan?
On Tue, Mar 05, 2019 at 04:31:40AM -0800, petr.hoffm...@oracle.com wrote:
> @@ -913,7 +913,33 @@ anchorrule : ANCHOR anchorname dir quick interface
> af proto fromto
> "rules must specify a name");
> YYERROR;
> }
> +
> + /*
> + * Don't make non-brace anchors part of the
> main anchor pool.
> + */
> + if ((r.anchor = calloc(1, sizeof(*r.anchor)))
> == NULL) {
> + err(1, "anchorrule: calloc");
> + }
> + pf_init_ruleset(>ruleset);
> + r.anchor->ruleset.anchor = r.anchor;
> + if (strlcpy(r.anchor->path, $2,
> + sizeof(r.anchor->path)) >=
> sizeof(r.anchor->path)) {
> + errx(1, "anchorrule: strlcpy");
> + }
> + if ((p = strrchr($2, '/')) != NULL) {
> + if (strlen(p) == 1) {
> + yyerror("anchorrule: bad anchor
> name %s",
> + $2);
> + YYERROR;
> + }
> + } else
> + p = (char *)$2;
This cast is not needed.
> @@ -5875,7 +5900,7 @@ int
> filteropts_to_rule(struct pf_rule *r, struct filter_opts *opts)
> {
> if (opts->marker & FOM_ONCE) {
> - if (r->action != PF_PASS && r->action != PF_MATCH) {
> + if ((r->action != PF_PASS && r->action != PF_DROP) ||
> r->anchor) {
`PF_MATCH' -> `PF_DROP' is obviously correct here; I made this
copy/pasta mistake in parse.y revision 1.682:
date: 2018/07/16 08:29:08; author: kn; state: Exp; lines: +11 -29;
reduce duplicate code, fix typo/free correct buffer
In filteropts_to_rule():
* Merge `once' handling from `anchorrule' and `pfrule'
* Remove/shorten duplicate code block
* Fix typo I introduced with r1.678 that frees the wrong buffer (twice)
OK sashan
> @@ -1112,35 +1112,13 @@ pfctl_show_limits(int dev, int opts)
>
> /* callbacks for rule/nat/rdr/addr */
> int
> -pfctl_add_rule(struct pfctl *pf, struct pf_rule *r, const char *anchor_call)
> +pfctl_add_rule(struct pfctl *pf, struct pf_rule *r)
Now that you touch the signature, you might as well make it void as it
always returns 0.
> {
> struct pf_rule *rule;
> struct pf_ruleset *rs;
> char*p;
`p' is now unused.
Re: vmctl: usage on extra arguments
I blatantly missed the argc/argv adjustments after getopt(3), resulting in valid commands like `vmctl create a -s 1G' to fail. Noticed by ajacoutot the hard way. OK? Index: usr.sbin/vmctl/main.c === RCS file: /cvs/src/usr.sbin/vmctl/main.c,v retrieving revision 1.53 diff -u -p -r1.53 main.c --- usr.sbin/vmctl/main.c 1 Mar 2019 10:34:14 - 1.53 +++ usr.sbin/vmctl/main.c 1 Mar 2019 12:25:23 - @@ -598,6 +598,8 @@ ctl_create(struct parse_result *res, int /* NOTREACHED */ } } + argc -= optind; + argv += optind; if (argc > 0) ctl_usage(res->ctl); @@ -915,6 +917,8 @@ ctl_start(struct parse_result *res, int /* NOTREACHED */ } } + argc -= optind; + argv += optind; if (argc > 0) ctl_usage(res->ctl); @@ -959,6 +963,8 @@ ctl_stop(struct parse_result *res, int a /* NOTREACHED */ } } + argc -= optind; + argv += optind; if (argc > 0) ctl_usage(res->ctl);
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 10:22:14PM +, Jason McIntyre wrote:
> i think more properly we should show
>
> -t id | name
It's about referencing the VM to be started itself, not templates.
`-t id' is not possible.
But you pointed out how my addition would errornously imply that, so
change the `start' synopsis from `name' to `id | name' as well as the
command description to differentiate between both cases.
"Starts" to "Start" while here.
Is that clearer?
Index: usr.sbin/vmctl/vmctl.8
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
retrieving revision 1.61
diff -u -p -r1.61 vmctl.8
--- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 - 1.61
+++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 23:06:31 -
@@ -144,7 +144,7 @@ under the same path.
An alias for the
.Cm status
command.
-.It Xo Cm start Ar name
+.It Xo Cm start Ar id | name
.Op Fl cL
.Bk -words
.Op Fl B Ar device
@@ -157,7 +157,11 @@ command.
.Op Fl t Ar name
.Ek
.Xc
-Starts a VM defined by the specified name and parameters:
+Start a new VM
+.Ar name
+with the specified parameters.
+An existing VM may be referenced by its
+.Ar id .
.Bl -tag -width "-I parent"
.It Fl B Ar device
Force system to boot from the specified device for this boot.
Index: usr.sbin/vmctl/main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.54
diff -u -p -r1.54 main.c
--- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 - 1.54
+++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -
@@ -81,7 +81,7 @@ struct ctl_command ctl_commands[] = {
{ "reset", CMD_RESET, ctl_reset, "[all | switches |
vms]" },
{ "send", CMD_SEND, ctl_send, "id", 1},
{ "show", CMD_STATUS, ctl_status, "[id]" },
- { "start", CMD_START, ctl_start, "name"
+ { "start", CMD_START, ctl_start, "id | name"
" [-cL] [-B device] [-b path] [-d disk] [-i count]\n"
"\t\t[-m size] [-n switch] [-r path] [-t name]" },
{ "status", CMD_STATUS, ctl_status, "[id]" },
Re: vmd/vmctl: improve VM name checks/error handling
On Thu, Mar 07, 2019 at 10:21:50PM +0100, Klemens Nanni wrote: > # vmctl start 1 > vmctl: started vm 1 successfully, tty /dev/ttypo > # vmctl stop 1 -f > stopping vm: forced to terminate vm 1 > # vmctl start a > vmctl: started vm 2 successfully, tty /dev/ttypo > > That is wonky, but I see how `start' must indeed accept IDs in the case > of predefined VMs. > > I'll work on docs to clarify this further without touching code, thanks. How about this? Index: usr.sbin/vmctl/vmctl.8 === RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v retrieving revision 1.61 diff -u -p -r1.61 vmctl.8 --- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 - 1.61 +++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 20:31:07 - @@ -233,6 +233,11 @@ The instance will inherit settings from except for exclusive options such as disk, interface lladdr, or interface names. .El +.Pp +For existing VMs, +.Ar name +may be the numerical +.Ar id . .It Cm status Op Ar id Lists VMs running on the host, optionally listing just the selected VM .Ar id .
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 11:41:02PM +, Jason McIntyre wrote: > i don;t understand why you special case "id" in a separate paragraph. Specifying an ID is valid only if you want to start an existing VM. You cannot create new VMs using a numerical name, that is an ID. This limitation is the point I wanted to clarify in the first place. Without the separate paragraph, this information is lost and `id | name' could just be `id' again. But as shown in one of my previous mails, something like `vmctl start 60 ...' to create a new VM called "60" is not possible and leads to non-obvious errors, hence my attempt to differentiate between those cases. > just document "name" and "id" as normal arguments. That's what I just did, no? > within the "start" command it would be consistent, but within the page > less so. i think this should be fixed wholesale, in a separate diff. Sure.
vmd/vmctl: improve VM name checks/error handling
vmd(8) does not support numerical names with `start' and `receive'.
It never worked, the manuals are now clearer about this, but error
handling can still be improved:
$ vmctl start 60 -t test -d 60.qcow2
vmctl: start vm command failed: No such file or directory
That's from vm_start_complete() in vmctl.c:254 where vmd(8) unexpectedly
returns EPERM after it failed to create the VM.
$ vmctl receive 60 "VM" for consistency with all other "invalid VM name" occurences
throughout the sources.
Feedback? OK?
Index: usr.sbin/vmctl/main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.54
diff -u -p -r1.54 main.c
--- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 - 1.54
+++ usr.sbin/vmctl/main.c 7 Mar 2019 19:14:15 -
@@ -524,7 +524,7 @@ parse_vmid(struct parse_result *res, cha
id = strtonum(word, 0, UINT32_MAX, );
if (error == NULL) {
if (needname) {
- warnx("invalid vm name");
+ warnx("invalid VM name");
return (-1);
} else {
res->id = id;
@@ -842,8 +842,8 @@ ctl_start(struct parse_result *res, int
if (argc < 2)
ctl_usage(res->ctl);
- if (parse_vmid(res, argv[1], 0) == -1)
- errx(1, "invalid id: %s", argv[1]);
+ if (parse_vmid(res, argv[1], 1) == -1)
+ exit(1);
argc--;
argv++;
@@ -1046,7 +1046,7 @@ ctl_receive(struct parse_result *res, in
err(1, "pledge");
if (argc == 2) {
if (parse_vmid(res, argv[1], 1) == -1)
- errx(1, "invalid id: %s", argv[1]);
+ exit(1);
} else if (argc != 2)
ctl_usage(res->ctl);
Index: usr.sbin/vmctl/vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.65
diff -u -p -r1.65 vmctl.c
--- usr.sbin/vmctl/vmctl.c 6 Dec 2018 09:23:15 - 1.65
+++ usr.sbin/vmctl/vmctl.c 7 Mar 2019 19:14:15 -
@@ -154,12 +154,7 @@ vm_start(uint32_t start_id, const char *
}
}
if (name != NULL) {
- /*
-* Allow VMs names with alphanumeric characters, dot, hyphen
-* and underscore. But disallow dot, hyphen and underscore at
-* the start.
-*/
- if (*name == '-' || *name == '.' || *name == '_')
+ if (!isalpha(*name))
errx(1, "invalid VM name");
for (s = name; *s != '\0'; ++s) {
Index: usr.sbin/vmd/vmd.c
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.108
diff -u -p -r1.108 vmd.c
--- usr.sbin/vmd/vmd.c 9 Dec 2018 12:26:38 - 1.108
+++ usr.sbin/vmd/vmd.c 7 Mar 2019 19:14:15 -
@@ -1257,11 +1257,7 @@ vm_register(struct privsep *ps, struct v
vcp->vcp_ndisks == 0 && strlen(vcp->vcp_cdrom) == 0) {
log_warnx("no kernel or disk/cdrom specified");
goto fail;
- } else if (strlen(vcp->vcp_name) == 0) {
- log_warnx("invalid VM name");
- goto fail;
- } else if (*vcp->vcp_name == '-' || *vcp->vcp_name == '.' ||
- *vcp->vcp_name == '_') {
+ } else if (strlen(vcp->vcp_name) == 0 || !isalpha(*vcp->vcp_name)) {
log_warnx("invalid VM name");
goto fail;
} else {
===
Stats: --- 15 lines 531 chars
Stats: +++ 6 lines 185 chars
Stats: -9 lines
Stats: -346 chars
Re: vmd/vmctl: improve VM name checks/error handling
On Thu, Mar 07, 2019 at 09:00:55PM +0100, Theo Buehler wrote: > On Thu, Mar 07, 2019 at 08:52:45PM +0100, Klemens Nanni wrote: > > vmd(8) does not support numerical names with `start' and `receive'. > > It never worked, the manuals are now clearer about this, but error > > handling can still be improved: > > I'm not sure I understand. This works fine for me with -current from > yesterday (I have vms configured in my vm.conf). > > $ vmctl start 1 > vmctl: started vm 1 successfully, tty /dev/ttyp0 Hm. That works because the first VM defined in your vm.conf(5) has a valid non-numerical name and gets assigned ID 1. With predefined VMs it works as you showed because ctl_start() parses the name as ID really, that is valid IDs will be accepted although the vmctl(8) always said `vmctl start name ...' not `vmctl start id ...'. The problem I faced is creating new VMs on the fly, possibly without any vm.conf present. Here the name passed on the command line is used to create a new VM instead of referencing an existing one. This never worked but the manual said it would be possible. With `receive' it's even more visible, as it never possibly uses the passed name as ID. Here's an example: $ vmctl create /10.img -s 10M vmctl: raw imagefile created # cat >/etc/vm.conf <
pfctl: anchor names must not be empty, unify sanity checks
When using anchors, they ought to have a non-empty name or none at all.
By accident, I discovered the following:
$ printf 'anchor ""\n' | pfctl -vnf-
pass all no state
No errors and it parses in a potentially harmful way. Other use cases
behave badly as well:
$ printf 'anchor "" {\n}\n' | pfctl -vnf-
pfctl: anchorrule: unable to create ruleset: Permission denied
$ printf 'load anchor "" from "/dev/null"\n' | pfctl -vnf-
Loading anchor from /dev/null
None of them make sense, so I propose to error out on empty anchor names
as early as possible. Given that typos happen and folks generate their
pf.conf automatically, such errors do not seem entirely out of scope.
The following diff makes `load anchor' use the `anchorname' production
like everything else does in the parser; it moves all sanity checks
in one place and thus also makes `load anchor' benefiet as well
(it currently accepts names beginning with "_1").
`pfctl -a ""' does behave like `pfctl -a /' or no anchor at all, hence
it uses the main anchor. This diff errors out on `-a ""' as well for
consistency and to discourage the use of the empty name, but as it does
not fix anything, this hunk might just be omitted as well.
Regress passes.
Feedback? OK?
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.690
diff -u -p -r1.690 parse.y
--- sbin/pfctl/parse.y 31 Jan 2019 18:08:36 - 1.690
+++ sbin/pfctl/parse.y 6 Feb 2019 19:52:11 -
@@ -809,7 +809,21 @@ varset : STRING '=' varstring {
}
;
-anchorname : STRING{ $$ = $1; }
+anchorname : STRING{
+ if ($1[0] == '\0') {
+ free($1);
+ yyerror("anchor name must not be empty");
+ YYERROR;
+ }
+ if (strlen(pf->anchor->path) + 1 +
+ strlen($1) >= PATH_MAX) {
+ free($1);
+ yyerror("anchor name is longer than %u",
+ PATH_MAX - 1);
+ YYERROR;
+ }
+ $$ = $1;
+ }
| /* empty */ { $$ = NULL; }
;
@@ -949,14 +963,11 @@ anchorrule: ANCHOR anchorname dir quick
}
;
-loadrule : LOAD ANCHOR string FROM string{
+loadrule : LOAD ANCHOR anchorname FROM string{
struct loadanchors *loadanchor;
- if (strlen(pf->anchor->path) + 1 +
- strlen($3) >= PATH_MAX) {
- yyerror("anchorname %s too long, max %u\n",
- $3, PATH_MAX - 1);
- free($3);
+ if ($3 == NULL) {
+ yyerror("anchor name is missing");
YYERROR;
}
loadanchor = calloc(1, sizeof(struct loadanchors));
Index: sbin/pfctl/pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.369
diff -u -p -r1.369 pfctl.c
--- sbin/pfctl/pfctl.c 29 Jan 2019 10:58:31 - 1.369
+++ sbin/pfctl/pfctl.c 6 Feb 2019 18:58:22 -
@@ -2441,6 +2441,8 @@ main(int argc, char *argv[])
memset(anchorname, 0, sizeof(anchorname));
if (anchoropt != NULL) {
+ if (anchoropt[0] == '\0')
+ errx(1, "anchor name must not be empty");
if (mode == O_RDONLY && showopt == NULL && tblcmdopt == NULL) {
warnx("anchors apply to -f, -F, -s, and -T only");
usage();
