Re: tpm(4): removing tvtohz(9)?
On Fri, 18 Dec 2020 at 18:58:43 -0600, Scott Cheloha wrote: > Hi, > > tpm(4) is the last driver in the tree using tvtohz(9). There are no > remaining callers using tstohz(9), so if and when we remove tvtohz(9) > from tpm(4) we can remove both interfaces from the tree. > > tpm(4) is tricky because it converts timeouts from milliseconds to > ticks and then doesn't use tsleep(9) at all. It uses delay(9), which > takes a count of microseconds as argument. This complicates the > conversion to tsleep_nsec(9) because the units don't match up for any > of these delays. Also, delay(9) is incompatible with tsleep(9) > because tsleep(9) yields the CPU while delay(9) busy-waits. > > I don't know if we *need* to delay(9) here. What would happen if we > yielded the CPU with e.g. tsleep(9)? > > The attached patch changes the delays to use the correct units. This > is not the right thing, these timeouts are probably too large to spin > for in delay(9). I'm just guessing here. > > Aside: TPM_READ_TMO is *huge*. 2 minutes for a read timeout seems a > bit large. NetBSD's TPM_READ_TMO has been dropped to 2 seconds, like > the other timeouts. Yes, this driver sucks. Its only purpose is to make certain devices suspend and resume properly and doesn't provide any actual TPM functionality. We (someone other than me) should just take NetBSD's rewrite which also adds TPM 2 support and apparently works on MSFT0101 devices which was committed and backed out in ours because it didn't work.
Re: dig vs ipv6 (scoped) addresses
On 12/17/20 3:15 AM, Otto Moerbeek wrote:
Hi,
as noted on misc dig does not like to talk to local link addresses.
This fixes that case. While investigating I also found another bug:
selecting v6 or v4 addresses only from resolv.conf via the -4 or -6
command line argument does not work as expected.
This fixes both. I did not test binding to a src address with this.
This is likely as broken as it was before.
-Otto
Index: dig.c
===
RCS file: /cvs/src/usr.bin/dig/dig.c,v
retrieving revision 1.18
diff -u -p -r1.18 dig.c
--- dig.c 15 Sep 2020 11:47:42 - 1.18
+++ dig.c 17 Dec 2020 11:06:49 -
@@ -1358,7 +1358,7 @@ dash_option(char *option, char *next, di
} else
srcport = 0;
if (have_ipv6 && inet_pton(AF_INET6, value, ) == 1)
- isc_sockaddr_fromin6(_address, , srcport);
+ isc_sockaddr_fromin6(_address, , srcport, 0);
else if (have_ipv4 && inet_pton(AF_INET, value, ) == 1)
isc_sockaddr_fromin(_address, , srcport);
else
Index: dighost.c
===
RCS file: /cvs/src/usr.bin/dig/dighost.c,v
retrieving revision 1.34
diff -u -p -r1.34 dighost.c
--- dighost.c 15 Sep 2020 11:47:42 - 1.34
+++ dighost.c 17 Dec 2020 11:06:49 -
@@ -540,7 +540,7 @@ get_addresses(const char *hostname, in_p
struct sockaddr_in6 *sin6;
sin6 = (struct sockaddr_in6 *)tmpai->ai_addr;
isc_sockaddr_fromin6([i], >sin6_addr,
-dstport);
+dstport, sin6->sin6_scope_id);
}
i++;
@@ -972,7 +972,7 @@ parse_netprefix(struct sockaddr_storage
if (inet_pton(AF_INET6, buf, ) == 1) {
parsed = 1;
- isc_sockaddr_fromin6(sa, , 0);
+ isc_sockaddr_fromin6(sa, , 0, 0);
if (prefix_length > 128)
prefix_length = 128;
} else if (inet_pton(AF_INET, buf, ) == 1) {
Index: lib/isc/sockaddr.c
===
RCS file: /cvs/src/usr.bin/dig/lib/isc/sockaddr.c,v
retrieving revision 1.14
diff -u -p -r1.14 sockaddr.c
--- lib/isc/sockaddr.c 28 Nov 2020 06:33:55 - 1.14
+++ lib/isc/sockaddr.c 17 Dec 2020 11:06:49 -
@@ -223,7 +223,7 @@ isc_sockaddr_anyofpf(struct sockaddr_sto
void
isc_sockaddr_fromin6(struct sockaddr_storage *sockaddr, const struct in6_addr
*ina6,
-in_port_t port)
+in_port_t port, uint32_t scope)
{
struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sockaddr;
memset(sockaddr, 0, sizeof(*sockaddr));
@@ -231,6 +231,7 @@ isc_sockaddr_fromin6(struct sockaddr_sto
sin6->sin6_len = sizeof(*sin6);
sin6->sin6_addr = *ina6;
sin6->sin6_port = htons(port);
+ sin6->sin6_scope_id = scope;
}
int
Index: lib/isc/include/isc/sockaddr.h
===
RCS file: /cvs/src/usr.bin/dig/lib/isc/include/isc/sockaddr.h,v
retrieving revision 1.7
diff -u -p -r1.7 sockaddr.h
--- lib/isc/include/isc/sockaddr.h 15 Sep 2020 11:47:42 - 1.7
+++ lib/isc/include/isc/sockaddr.h 17 Dec 2020 11:06:49 -
@@ -91,7 +91,7 @@ isc_sockaddr_fromin(struct sockaddr_stor
void
isc_sockaddr_fromin6(struct sockaddr_storage *sockaddr, const struct in6_addr
*ina6,
-in_port_t port);
+in_port_t port, uint32_t scope);
/*%<
* Construct an struct sockaddr_storage from an IPv6 address and port.
*/
Index: lib/lwres/lwconfig.c
===
RCS file: /cvs/src/usr.bin/dig/lib/lwres/lwconfig.c,v
retrieving revision 1.6
diff -u -p -r1.6 lwconfig.c
--- lib/lwres/lwconfig.c25 Feb 2020 05:00:43 - 1.6
+++ lib/lwres/lwconfig.c17 Dec 2020 11:06:49 -
@@ -231,7 +231,7 @@ lwres_conf_parsenameserver(lwres_conf_t
res = lwres_create_addr(word, , 1);
use_ipv4 = confdata->flags & LWRES_USEIPV4;
- use_ipv6 = confdata->flags & LWRES_USEIPV4;
+ use_ipv6 = confdata->flags & LWRES_USEIPV6;
if (res == LWRES_R_SUCCESS &&
((address.family == LWRES_ADDRTYPE_V4 && use_ipv4) ||
(address.family == LWRES_ADDRTYPE_V6 && use_ipv6))) {
Hi Otto,
I just gave this diff a go and everything seems to be working fine. Dig
is now using the proper DNS set in my resolv.conf:
$ ./dig openbsd.org
; <<>> dig 9.10.8-P1 <<>> openbsd.org
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 3506
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1
;; OPT
tpm(4): removing tvtohz(9)?
Hi,
tpm(4) is the last driver in the tree using tvtohz(9). There are no
remaining callers using tstohz(9), so if and when we remove tvtohz(9)
from tpm(4) we can remove both interfaces from the tree.
tpm(4) is tricky because it converts timeouts from milliseconds to
ticks and then doesn't use tsleep(9) at all. It uses delay(9), which
takes a count of microseconds as argument. This complicates the
conversion to tsleep_nsec(9) because the units don't match up for any
of these delays. Also, delay(9) is incompatible with tsleep(9)
because tsleep(9) yields the CPU while delay(9) busy-waits.
I don't know if we *need* to delay(9) here. What would happen if we
yielded the CPU with e.g. tsleep(9)?
The attached patch changes the delays to use the correct units. This
is not the right thing, these timeouts are probably too large to spin
for in delay(9). I'm just guessing here.
Aside: TPM_READ_TMO is *huge*. 2 minutes for a read timeout seems a
bit large. NetBSD's TPM_READ_TMO has been dropped to 2 seconds, like
the other timeouts.
Also perhaps of note is that NetBSD's tpm(4) driver mostly no longer
uses delay(9). They use tsleep(9) in all but one spot:
https://github.com/NetBSD/src/blob/fc83762bc464be0bf351901b2c387a8cfedff7c4/sys/dev/ic/tpm.c
Index: tpm.c
===
RCS file: /cvs/src/sys/dev/acpi/tpm.c,v
retrieving revision 1.10
diff -u -p -r1.10 tpm.c
--- tpm.c 22 May 2020 10:16:37 - 1.10
+++ tpm.c 19 Dec 2020 00:56:02 -
@@ -158,7 +158,6 @@ int tpm_request_locality(struct tpm_soft
void tpm_release_locality(struct tpm_softc *);
inttpm_getburst(struct tpm_softc *);
uint8_ttpm_status(struct tpm_softc *);
-inttpm_tmotohz(int);
struct cfattach tpm_ca = {
sizeof(struct tpm_softc),
@@ -372,7 +371,7 @@ int
tpm_request_locality(struct tpm_softc *sc, int l)
{
uint32_t r;
- int to;
+ int msecs;
if (l != 0)
return EINVAL;
@@ -385,12 +384,12 @@ tpm_request_locality(struct tpm_softc *s
bus_space_write_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS,
TPM_ACCESS_REQUEST_USE);
- to = tpm_tmotohz(TPM_ACCESS_TMO);
-
- while ((r = bus_space_read_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS) &
- (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) !=
- (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY) && to--) {
- DELAY(10);
+ for (msecs = 0; msecs < TPM_ACCESS_TMO; msecs++) {
+ r = bus_space_read_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS);
+ if ((r & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) ==
+ (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY))
+ break;
+ DELAY(1000);
}
if ((r & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) !=
@@ -418,12 +417,10 @@ tpm_release_locality(struct tpm_softc *s
int
tpm_getburst(struct tpm_softc *sc)
{
- int burst, burst2, to;
-
- to = tpm_tmotohz(TPM_BURST_TMO);
+ int burst, burst2, msecs;
burst = 0;
- while (burst == 0 && to--) {
+ for (msecs = 0; msecs < TPM_BURST_TMO; msecs++) {
/*
* Burst count has to be read from bits 8 to 23 without
* touching any other bits, eg. the actual status bits 0 to 7.
@@ -438,7 +435,7 @@ tpm_getburst(struct tpm_softc *sc)
if (burst)
return burst;
- DELAY(10);
+ DELAY(1000);
}
DPRINTF(("%s: getburst timed out\n", sc->sc_dev.dv_xname));
@@ -453,30 +450,19 @@ tpm_status(struct tpm_softc *sc)
}
int
-tpm_tmotohz(int tmo)
-{
- struct timeval tv;
-
- tv.tv_sec = tmo / 1000;
- tv.tv_usec = 1000 * (tmo % 1000);
-
- return tvtohz();
-}
-
-int
-tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int tries)
+tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int msecs)
{
uint8_t status;
while (((status = tpm_status(sc)) & mask) != mask) {
- if (tries == 0) {
+ if (msecs <= 0) {
DPRINTF(("%s: %s: timed out, status 0x%x != 0x%x\n",
sc->sc_dev.dv_xname, __func__, status, mask));
return status;
}
- tries--;
- DELAY(1);
+ msecs -= 1000;
+ DELAY(1000);
}
return 0;
rasops1
Graphics stuff is weird. It doesn't just care about endianness on the
byte level, but also about endianness on the bit level. This matters
for monochrome framebuffers, where a true big-endian framebuffer will
have the leftmost pixel in the MSB wheras a little-endian framebuffer
will have it in the LSB. The optimized rasops1_putchar8() and
rasops1_putchar16() assume the framebuffer uses the big-endian byte
order since that is how our fonts are encoded. The generic
rasops1_putchar() function assumes native bit order though. This is
inconsistent and confusing.
The diff below disables the optimized functions on little-endian
architectures such that we always use rasops1_putchar(). This makes
ssdfb(4) work with the default 8x16 font on arm64.
ok?
Index: dev/rasops/rasops1.c
===
RCS file: /cvs/src/sys/dev/rasops/rasops1.c,v
retrieving revision 1.10
diff -u -p -r1.10 rasops1.c
--- dev/rasops/rasops1.c25 May 2020 09:55:49 - 1.10
+++ dev/rasops/rasops1.c18 Dec 2020 21:19:55 -
@@ -44,7 +44,7 @@ int rasops1_copycols(void *, int, int, i
intrasops1_erasecols(void *, int, int, int, uint32_t);
intrasops1_do_cursor(struct rasops_info *);
intrasops1_putchar(void *, int, int col, u_int, uint32_t);
-#ifndef RASOPS_SMALL
+#if defined(RASOPS_SMALL) && BYTE_ORDER == BIG_ENDIAN
intrasops1_putchar8(void *, int, int col, u_int, uint32_t);
intrasops1_putchar16(void *, int, int col, u_int, uint32_t);
#endif
@@ -58,7 +58,7 @@ rasops1_init(struct rasops_info *ri)
rasops_masks_init();
switch (ri->ri_font->fontwidth) {
-#ifndef RASOPS_SMALL
+#if defined(RASOPS_SMALL) && BYTE_ORDER == BIG_ENDIAN
case 8:
ri->ri_ops.putchar = rasops1_putchar8;
break;
@@ -223,7 +223,7 @@ rasops1_putchar(void *cookie, int row, i
return 0;
}
-#ifndef RASOPS_SMALL
+#if defined(RASOPS_SMALL) && BYTE_ORDER == BIG_ENDIAN
/*
* Paint a single character. This is for 8-pixel wide fonts.
*/
@@ -350,7 +350,7 @@ rasops1_putchar16(void *cookie, int row,
return 0;
}
-#endif /* !RASOPS_SMALL */
+#endif /* !defined(RASOPS_SMALL) && BYTE_ORDER == BIG_ENDIAN */
/*
* Grab routines common to depths where (bpp < 8)
doas sprinkle some more CFLAGS
So I ended up in doas again, this time with the CFLAGS I use for most of
my other projects. This popped up a few new not very exciting warnings.
Diff below compiles clean with both clang and gcc on amd64.
Worth doing?
martijn@
Index: Makefile
===
RCS file: /cvs/src/usr.bin/doas/Makefile,v
retrieving revision 1.3
diff -u -p -r1.3 Makefile
--- Makefile3 Jul 2017 22:21:47 - 1.3
+++ Makefile18 Dec 2020 21:18:51 -
@@ -9,7 +9,11 @@ BINOWN= root
BINMODE=4555
CFLAGS+= -I${.CURDIR}
-COPTS+=-Wall
+CFLAGS+= -Wall
+CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
+CFLAGS+= -Wmissing-declarations
+CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
+CFLAGS+= -Wsign-compare
YFLAGS=
.include
Index: doas.c
===
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.84
diff -u -p -r1.84 doas.c
--- doas.c 9 Oct 2020 07:43:38 - 1.84
+++ doas.c 18 Dec 2020 21:18:51 -
@@ -94,7 +94,7 @@ parsegid(const char *s, gid_t *gid)
static int
match(uid_t uid, gid_t *groups, int ngroups, uid_t target, const char *cmd,
-const char **cmdargs, struct rule *r)
+const char * const*cmdargs, struct rule *r)
{
int i;
@@ -134,7 +134,7 @@ match(uid_t uid, gid_t *groups, int ngro
static int
permit(uid_t uid, gid_t *groups, int ngroups, const struct rule **lastr,
-uid_t target, const char *cmd, const char **cmdargs)
+uid_t target, const char *cmd, const char * const*cmdargs)
{
int i;
@@ -188,7 +188,7 @@ checkconfig(const char *confpath, int ar
exit(0);
if (permit(uid, groups, ngroups, , target, argv[0],
- (const char **)argv + 1)) {
+ (const char * const*)argv + 1)) {
printf("permit%s\n", (rule->options & NOPASS) ? " nopass" : "");
exit(0);
} else {
@@ -244,7 +244,7 @@ good:
}
}
-int
+static int
unveilcommands(const char *ipath, const char *cmd)
{
char *path = NULL, *p;
@@ -271,7 +271,7 @@ unveilcommands(const char *ipath, const
if (cp) {
int r = snprintf(buf, sizeof buf, "%s/%s", cp, cmd);
- if (r >= 0 && r < sizeof buf) {
+ if (r >= 0 && (size_t)r < sizeof buf) {
if (unveil(buf, "x") != -1)
unveils++;
}
@@ -394,7 +394,7 @@ main(int argc, char **argv)
cmd = argv[0];
if (!permit(uid, groups, ngroups, , target, cmd,
- (const char **)argv + 1)) {
+ (const char * const*)argv + 1)) {
syslog(LOG_AUTHPRIV | LOG_NOTICE,
"command not permitted for %s: %s", mypw->pw_name, cmdline);
errc(1, EPERM, NULL);
Index: env.c
===
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.10
diff -u -p -r1.10 env.c
--- env.c 7 Jul 2019 19:21:28 - 1.10
+++ env.c 18 Dec 2020 21:18:51 -
@@ -32,8 +32,8 @@ const char *formerpath;
struct envnode {
RB_ENTRY(envnode) node;
- const char *key;
- const char *value;
+ char *key;
+ char *value;
};
struct env {
Index: parse.y
===
RCS file: /cvs/src/usr.bin/doas/parse.y,v
retrieving revision 1.28
diff -u -p -r1.28 parse.y
--- parse.y 9 Oct 2020 07:43:38 - 1.28
+++ parse.y 18 Dec 2020 21:18:51 -
@@ -56,7 +56,7 @@ static void yyerror(const char *, ...);
static int yylex(void);
static size_t
-arraylen(const char **arr)
+arraylen(const char * const*arr)
{
size_t cnt = 0;
@@ -222,7 +222,8 @@ int
yylex(void)
{
char buf[1024], *ebuf, *p, *str;
- int i, c, quotes = 0, escape = 0, qpos = -1, nonkw = 0;
+ int c, quotes = 0, escape = 0, qpos = -1, nonkw = 0;
+ size_t i;
p = buf;
ebuf = buf + sizeof(buf);
[PATCH]es sysctl_int_bounded goodness
I'm scraping the bottom of the barrel with these, so dumped them all
together for ease of review. Will submit piecemeal as reviews happen.
All verified manually with sysctl -w. Even bothered to find an i386
machine with watchdog and build a WITNESS kernel to run all code paths.
OKs?
Subject: [PATCH 01/10] Use sysctl_int_bounded in sysctl_hwsmt
Prefer error reporting is to silent clipping.
---
sys/kern/kern_sched.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git sys/kern/kern_sched.c sys/kern/kern_sched.c
index eab78f74c60..7a27c2b65e8 100644
--- sys/kern/kern_sched.c
+++ sys/kern/kern_sched.c
@@ -861,13 +861,9 @@ sysctl_hwsmt(void *oldp, size_t *oldlenp, void *newp,
size_t newlen)
int err, newsmt;
newsmt = sched_smt;
- err = sysctl_int(oldp, oldlenp, newp, newlen, );
+ err = sysctl_int_bounded(oldp, oldlenp, newp, newlen, , 0, 1);
if (err)
return err;
- if (newsmt > 1)
- newsmt = 1;
- if (newsmt < 0)
- newsmt = 0;
if (newsmt == sched_smt)
return 0;
--
2.29.2
Subject: [PATCH 02/10] Finish converting ddb_sysctl to sysctl_int_bounded
I missed the verbose pattern that it used for error checking the first
time around.
---
sys/ddb/db_usrreq.c | 32 ++--
1 file changed, 6 insertions(+), 26 deletions(-)
diff --git sys/ddb/db_usrreq.c sys/ddb/db_usrreq.c
index 4b77e63b540..820f9eeb246 100644
--- sys/ddb/db_usrreq.c
+++ sys/ddb/db_usrreq.c
@@ -48,8 +48,6 @@ int
ddb_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
size_t newlen, struct proc *p)
{
- int error, ctlval;
-
/* All sysctl names at this level are terminal. */
if (namelen != 1)
return (ENOTDIR);
@@ -60,14 +58,8 @@ ddb_sysctl(int *name, u_int namelen, void *oldp, size_t
*oldlenp, void *newp,
return (sysctl_int_lower(oldp, oldlenp, newp, newlen,
_panic));
else {
- ctlval = db_panic;
- if ((error = sysctl_int(oldp, oldlenp, newp, newlen,
- )) || newp == NULL)
- return (error);
- if (ctlval != 1 && ctlval != 0)
- return (EINVAL);
- db_panic = ctlval;
- return (0);
+ return (sysctl_int_bounded(oldp, oldlenp, newp, newlen,
+ _panic, 0, 1));
}
break;
case DBCTL_CONSOLE:
@@ -75,14 +67,8 @@ ddb_sysctl(int *name, u_int namelen, void *oldp, size_t
*oldlenp, void *newp,
return (sysctl_int_lower(oldp, oldlenp, newp, newlen,
_console));
else {
- ctlval = db_console;
- if ((error = sysctl_int(oldp, oldlenp, newp, newlen,
- )) || newp == NULL)
- return (error);
- if (ctlval != 1 && ctlval != 0)
- return (EINVAL);
- db_console = ctlval;
- return (0);
+ return (sysctl_int_bounded(oldp, oldlenp, newp, newlen,
+ _console, 0, 1));
}
break;
case DBCTL_TRIGGER:
@@ -104,14 +90,8 @@ ddb_sysctl(int *name, u_int namelen, void *oldp, size_t
*oldlenp, void *newp,
return (sysctl_int_lower(oldp, oldlenp, newp, newlen,
_profile));
else {
- ctlval = db_profile;
- if ((error = sysctl_int(oldp, oldlenp, newp, newlen,
- )) || newp == NULL)
- return (error);
- if (ctlval != 1 && ctlval != 0)
- return (EINVAL);
- db_profile = ctlval;
- return (0);
+ return (sysctl_int_bounded(oldp, oldlenp, newp, newlen,
+ _profile, 0, 1));
}
break;
#endif /* DDBPROF */
--
2.29.2
Subject: [PATCH 03/10] Enforce range with sysctl_int_bounded in tcp_sysctl
One case uses the explicit range from the code and the other was
inferred from reading the usage.
---
sys/netinet/tcp_usrreq.c | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git sys/netinet/tcp_usrreq.c sys/netinet/tcp_usrreq.c
index 64327536ee5..8da961830bf 100644
--- sys/netinet/tcp_usrreq.c
+++ sys/netinet/tcp_usrreq.c
@@ -1056,8 +1056,8 @@ tcp_sysctl(int *name, u_int namelen, void *oldp, size_t
*oldlenp, void *newp,
case TCPCTL_SYN_USE_LIMIT:
NET_LOCK();
- error = sysctl_int(oldp,
tsleep(9): sleep on private channel if ident is NULL
Hi,
This patch adds support for passing NULL as the ident when calling
tsleep(9) etc. When this happens, sleep_setup() will use the address
of the sleep_state struct as the value for p_wchan. This address is
basically always a private value so the thread should never receive a
wakeup(9) broadcast.
Why do we want this? Sometimes there is no logical ident to sleep on.
Sometimes there is legitimately no reason to receive a wakeup(9).
In the past, people have handrolled private channels to work around
this situation. The code often looks like this:
void
foo(void)
{
int chan;
tsleep(, ...);
}
Permitting the use of NULL and letting the implementation choose a
private channel is better than handrolling a private channel for two
reasons:
1. We save a bit of stack space. tsleep(9) etc. already have a
sleep_state struct on the stack and it's at a private address
so there is no space cost to use it.
2. The NULL clearly communicates the author's intent to the reader.
It indicates the author had no wakeup channel in mind when they
wrote the code. The reader then doesn't need to reason about
whether or not the ident value is superfluous. Poring over
a file (or several) to determine whether any thread ever calls
wakeup(9) on a given ident sucks.
FreeBSD/NetBSD have a dedicated interface for this "sleep without a
wakeup channel" operation. They call it "pause". I proposed adding
it but I got mixed feedback on the patch. Then mpi@ proposed this
idea. I think this is simpler and better.
The actual implementation requires just a few small changes to
sleep_setup().
I've added an additional KASSERT to each of tsleep(9), msleep(9), and
rwsleep(9). You now need at least one of (a) an ident or (b) PCATCH
or (c) a timeout, otherwise there is no way to get the thread started
again. This would indicate a programmer error and we should panic if
it ever happens.
I've documented the new NULL ident behavior in tsleep.9.
Also included here is a sample user, sys_nanosleep(). nanosleep(2)
wakes up due to interruption by signal or timeout. It should never be
awoken with wakeup(9). Up until now we had a private channel on the
stack. Now we can just pass NULL. It's simpler.
There are a bunch of other potential users but they can wait until a
later patch.
I'm running with this now so I'm pretty sure this is a sound change.
Feel free to test it out. nanosleep(2) gets called all the time so if
there was an issue I imagine it'd show up pretty quickly.
Thoughts? ok?
Index: share/man/man9/tsleep.9
===
RCS file: /cvs/src/share/man/man9/tsleep.9,v
retrieving revision 1.15
diff -u -p -r1.15 tsleep.9
--- share/man/man9/tsleep.9 20 Mar 2020 03:37:09 - 1.15
+++ share/man/man9/tsleep.9 18 Dec 2020 19:40:04 -
@@ -144,8 +144,11 @@ to the resource for which the process is
The same identifier must be used in a call to
.Fn wakeup
to get the process going again.
+If the thread does not want to receive any
+.Fn wakeup
+broadcasts,
.Fa ident
-should not be
+should be
.Dv NULL .
.It Fa priority
The process priority to be used when the process is awakened and put on
Index: sys/kern/kern_synch.c
===
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.172
diff -u -p -r1.172 kern_synch.c
--- sys/kern/kern_synch.c 7 Dec 2020 16:55:29 - 1.172
+++ sys/kern/kern_synch.c 18 Dec 2020 19:40:04 -
@@ -119,6 +119,7 @@ tsleep(const volatile void *ident, int p
#endif
KASSERT((priority & ~(PRIMASK | PCATCH)) == 0);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
#ifdef MULTIPROCESSOR
KASSERT(timo || _kernel_lock_held());
@@ -214,6 +215,7 @@ msleep(const volatile void *ident, struc
KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
KASSERT(mtx != NULL);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
if (priority & PCATCH)
KERNEL_ASSERT_LOCKED();
@@ -301,6 +303,7 @@ rwsleep(const volatile void *ident, stru
int error, status;
KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
rw_assert_anylock(rwl);
status = rw_status(rwl);
@@ -351,8 +354,6 @@ sleep_setup(struct sleep_state *sls, con
#ifdef DIAGNOSTIC
if (p->p_flag & P_CANTSLEEP)
panic("sleep: %s failed insomnia", p->p_p->ps_comm);
- if (ident == NULL)
- panic("tsleep: no ident");
if (p->p_stat != SONPROC)
panic("tsleep: not SONPROC");
#endif
@@ -378,11 +379,23 @@ sleep_setup(struct sleep_state *sls, con
TRACEPOINT(sched, sleep, NULL);
- p->p_wchan = ident;
+ /*
+* If ident is NULL the caller does not want to receive
+*
Re: rpki-client unmarshal empty strings as NULL
> This is the next step. I added asserts for strings that must be set and > removed some of complications around optional strings. Especially cert.c > and some of the entityq code benefits from this. Looks good and works for me. ok tb
Re: rpki-client unmarshal empty strings as NULL
On Fri, Dec 18, 2020 at 05:50:27PM +0100, Theo Buehler wrote:
> On Fri, Dec 18, 2020 at 05:45:01PM +0100, Claudio Jeker wrote:
> > On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> > > On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > > > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > > > rpki-client passes both empty strings and NULL strings as zero length
> > > > > objects. The unmarshal code then allocates memory in any case and so a
> > > > > NULL string is unmarshalled as empty string. This is not great,
> > > > > currently
> > > > > there are no empty strings but a fair amount of NULL strings.
> > > > > This diff changes the behaviour and now NULL is passed as NULL.
> > > > > This should simplify passing optional strings (e.g. in the entity
> > > > > queue
> > > > > code).
> > > >
> > > > I will commit this later today. It will help with some further cleanup
> > > > of
> > > > the codebase.
> > >
> > > I'm a bit torn about this. Some of the callers clearly do not expect
> > > having to deal with NULL.
> > >
> > > I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> > > never happen but can now happen with this change. I'm unsure that there
> > > are no NULL derefs that this introduces. I'm fine with this going in if
> > > you intend to address this as part of the further cleanup work you
> > > envision.
> > >
> >
> > In most cases the code expects a non-empty string. For example in the
> > rsync.c case neither host nor mod are allowed to be NULL or "".
>
> Yes.
>
> > I guess adding an assert(host && mod) would be enough there.
>
> Right.
>
> > I actually prefer the code to blow up since as mentioned
> > empty strings are almost always wrong (e.g. empty filenames or hashes).
> > Indeed in all those cases a check for NULL should be added in the
> > unmarshal function.
>
> Go ahead. It certainly doesn't make things worse or (more) incorrect.
>
> ok tb
>
This is the next step. I added asserts for strings that must be set and
removed some of complications around optional strings. Especially cert.c
and some of the entityq code benefits from this.
--
:wq Claudio
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.20
diff -u -p -r1.20 cert.c
--- cert.c 7 Dec 2020 13:23:01 - 1.20
+++ cert.c 18 Dec 2020 17:09:45 -
@@ -1262,7 +1262,6 @@ void
cert_buffer(char **b, size_t *bsz, size_t *bmax, const struct cert *p)
{
size_t i;
- int has_crl, has_aki;
io_simple_buffer(b, bsz, bmax, >valid, sizeof(int));
io_simple_buffer(b, bsz, bmax, >ipsz, sizeof(size_t));
@@ -1275,15 +1274,8 @@ cert_buffer(char **b, size_t *bsz, size_
io_str_buffer(b, bsz, bmax, p->mft);
io_str_buffer(b, bsz, bmax, p->notify);
-
- has_crl = (p->crl != NULL);
- io_simple_buffer(b, bsz, bmax, _crl, sizeof(int));
- if (has_crl)
- io_str_buffer(b, bsz, bmax, p->crl);
- has_aki = (p->aki != NULL);
- io_simple_buffer(b, bsz, bmax, _aki, sizeof(int));
- if (has_aki)
- io_str_buffer(b, bsz, bmax, p->aki);
+ io_str_buffer(b, bsz, bmax, p->crl);
+ io_str_buffer(b, bsz, bmax, p->aki);
io_str_buffer(b, bsz, bmax, p->ski);
}
@@ -1327,7 +1319,6 @@ cert_read(int fd)
{
struct cert *p;
size_t i;
- int has_crl, has_aki;
if ((p = calloc(1, sizeof(struct cert))) == NULL)
err(1, NULL);
@@ -1348,14 +1339,12 @@ cert_read(int fd)
cert_as_read(fd, >as[i]);
io_str_read(fd, >mft);
+ assert(p->mft);
io_str_read(fd, >notify);
- io_simple_read(fd, _crl, sizeof(int));
- if (has_crl)
- io_str_read(fd, >crl);
- io_simple_read(fd, _aki, sizeof(int));
- if (has_aki)
- io_str_read(fd, >aki);
+ io_str_read(fd, >crl);
+ io_str_read(fd, >aki);
io_str_read(fd, >ski);
+ assert(p->ski);
return p;
}
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.86
diff -u -p -r1.86 main.c
--- main.c 9 Dec 2020 11:29:04 - 1.86
+++ main.c 18 Dec 2020 17:10:34 -
@@ -114,7 +114,6 @@ struct entity {
int has_pkey; /* whether pkey/sz is specified */
unsigned char *pkey; /* public key (optional) */
size_t pkeysz; /* public key length (optional) */
- int has_descr; /* whether descr is specified */
char*descr; /* tal description */
TAILQ_ENTRY(entity) entries;
};
@@ -233,9 +232,7 @@ entity_read_req(int fd, struct entity *e
io_simple_read(fd, >has_pkey, sizeof(int));
if (ent->has_pkey)
Re: rpki-client unmarshal empty strings as NULL
On Fri, Dec 18, 2020 at 05:45:01PM +0100, Claudio Jeker wrote:
> On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> > On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > > rpki-client passes both empty strings and NULL strings as zero length
> > > > objects. The unmarshal code then allocates memory in any case and so a
> > > > NULL string is unmarshalled as empty string. This is not great,
> > > > currently
> > > > there are no empty strings but a fair amount of NULL strings.
> > > > This diff changes the behaviour and now NULL is passed as NULL.
> > > > This should simplify passing optional strings (e.g. in the entity queue
> > > > code).
> > >
> > > I will commit this later today. It will help with some further cleanup of
> > > the codebase.
> >
> > I'm a bit torn about this. Some of the callers clearly do not expect
> > having to deal with NULL.
> >
> > I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> > never happen but can now happen with this change. I'm unsure that there
> > are no NULL derefs that this introduces. I'm fine with this going in if
> > you intend to address this as part of the further cleanup work you
> > envision.
> >
>
> In most cases the code expects a non-empty string. For example in the
> rsync.c case neither host nor mod are allowed to be NULL or "".
Yes.
> I guess adding an assert(host && mod) would be enough there.
Right.
> I actually prefer the code to blow up since as mentioned
> empty strings are almost always wrong (e.g. empty filenames or hashes).
> Indeed in all those cases a check for NULL should be added in the
> unmarshal function.
Go ahead. It certainly doesn't make things worse or (more) incorrect.
ok tb
>
> --
> :wq Claudio
Re: rpki-client unmarshal empty strings as NULL
On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > rpki-client passes both empty strings and NULL strings as zero length
> > > objects. The unmarshal code then allocates memory in any case and so a
> > > NULL string is unmarshalled as empty string. This is not great, currently
> > > there are no empty strings but a fair amount of NULL strings.
> > > This diff changes the behaviour and now NULL is passed as NULL.
> > > This should simplify passing optional strings (e.g. in the entity queue
> > > code).
> >
> > I will commit this later today. It will help with some further cleanup of
> > the codebase.
>
> I'm a bit torn about this. Some of the callers clearly do not expect
> having to deal with NULL.
>
> I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> never happen but can now happen with this change. I'm unsure that there
> are no NULL derefs that this introduces. I'm fine with this going in if
> you intend to address this as part of the further cleanup work you
> envision.
>
In most cases the code expects a non-empty string. For example in the
rsync.c case neither host nor mod are allowed to be NULL or "".
I guess adding an assert(host && mod) would be enough there.
I actually prefer the code to blow up since as mentioned
empty strings are almost always wrong (e.g. empty filenames or hashes).
Indeed in all those cases a check for NULL should be added in the
unmarshal function.
--
:wq Claudio
Re: WITNESS panic: acquiring blockable sleep lock with spinlock or critical section held (rwlock) kmmaplk
On Fri, 18 Dec 2020 13:34:39 +0100, Mark Kettenis wrote: > Anyway, your analysis is right. When a kernel thread wants to use > pmap_extract(9) on a userland pmap, it needs to lock pm_apte_mtx to > prevent another thread from simultaniously activating a userland pmap > too. So indeed, pm_apte_mtx needs to be properly initialized for the > kernel pmap. > > However, pm_mtx should never be used for the kernel pmap. If we don't > initialize the lock, witness will help us catching this condition, so > maybe we shouldn't... I think a comment is warranted if we don't want to initialize the lock to prevent someone from fixing this in the future ;-) - todd
Re: Enhancing (some) PF state
On Fri, Dec 18, 2020 at 3:53 AM Alexandr Nedvedicky wrote: > > Hello Sven, > > your change makes me wonder: 'what is the actual problem you are trying to > solve'? > > the reason I'm asking is that latency is just one factor, which contributes to > TCP connection performance. The other factor (and perhaps more important) is > to > guess amount of retransmitted data. Processes (a.k.a. endpoints), which > communicate over TCP can experience significant delay once TCP packets starts > to be dropped. Those dropped TCP packets contribute to delay experienced in > the > more significant way, than 'network latency' in sense of roundtrip. > > I'm not much experienced firewall administrator, the only firewall I run is > APU box at my home, hence I'm sorry if my question sounds naive. So basically > what sort of problem in network you hope to diagnose with PF? > > And also don't get me wrong: I like your idea to extend PF to enable firewall > to provide better picture of what happens on network. I just want to point > out that sampling network latency (round-trip) might not be sufficient. > > thanks and > regards > sashan > I need data on perceived server load // compare to network jitter with different locations. The retransmission global counter, while interesting, is certainly not perfect either. But for now I'd like to be able to keep a record of the latency of 'real' TCP connections. I cannot do it on the clients, and cannot do it on the servers. It must be done in the trusted OpenBSD environment. Having retransmissions per tcp connection would definitely be a plus, but it's not the goal here. It is not a problem I need to solve, just additional data I want to extract to make better informed choices and cross validate results. I would like to go through this one step at a time to stay focused on : computing latency timing of TCP connexion in openbsd 'correctly' -- -- - Knowing is not enough; we must apply. Willing is not enough; we must do
Re: rpki-client unmarshal empty strings as NULL
On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > rpki-client passes both empty strings and NULL strings as zero length
> > objects. The unmarshal code then allocates memory in any case and so a
> > NULL string is unmarshalled as empty string. This is not great, currently
> > there are no empty strings but a fair amount of NULL strings.
> > This diff changes the behaviour and now NULL is passed as NULL.
> > This should simplify passing optional strings (e.g. in the entity queue
> > code).
>
> I will commit this later today. It will help with some further cleanup of
> the codebase.
I'm a bit torn about this. Some of the callers clearly do not expect
having to deal with NULL.
I see some *printf("%s", NULL) (for example in proc_rsync()) that should
never happen but can now happen with this change. I'm unsure that there
are no NULL derefs that this introduces. I'm fine with this going in if
you intend to address this as part of the further cleanup work you
envision.
>
> --
> :wq Claudio
>
> ? obj
> Index: io.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 io.c
> --- io.c 2 Dec 2020 15:31:15 - 1.10
> +++ io.c 2 Dec 2020 15:54:38 -
> @@ -153,7 +153,7 @@ io_buf_read_alloc(int fd, void **res, si
> }
>
> /*
> - * Read a string (which may just be \0 and zero-length), allocating
> + * Read a string (returns NULL for zero-length strings), allocating
> * space for it.
> */
> void
> @@ -162,6 +162,10 @@ io_str_read(int fd, char **res)
> size_t sz;
>
> io_simple_read(fd, , sizeof(size_t));
> + if (sz == 0) {
> + *res = NULL;
> + return;
> + }
> if ((*res = calloc(sz + 1, 1)) == NULL)
> err(1, NULL);
> io_simple_read(fd, *res, sz);
>
Re: WITNESS panic: acquiring blockable sleep lock with spinlock or critical section held (rwlock) kmmaplk
> Date: Thu, 17 Dec 2020 19:25:44 -0300
> From: Martin Pieuchot
>
> On 17/12/20(Thu) 23:16, Mark Kettenis wrote:
> > > Date: Thu, 17 Dec 2020 18:56:52 -0300
> > > From: Martin Pieuchot
> > >
> > > On 16/12/20(Wed) 22:49, Greg Steuck wrote:
> > > > I just hit this while booting an i386-current in vmd. The source tree is
> > > > synced to "Remove the assertion in uvm_km_pgremove()."
> > > >
> > > > I enabled WITNESS on top of GENERIC. Naturally, GENERIC-Dec15 snap
> > > > works.
> > > >
> > > > Anybody else see this so I know it's worth a bisect?
> > > > [...]
> > >
> > > I can reproduce it. Diff below fixes it. This is the beginning of a
> > > rabbit hole... thanks!
> > >
> > > > witness: lock_object uninitialized: 0xd0f3c828
> > > > Starting stack trace...
> > > > witness_checkorder(0,d6bb011c,d1155e6c,d02e10e4,90) at
> > > > witness_checkorder+0x8a
> > > > witness_checkorder(d0f3c828,9,0) at witness_checkorder+0x8a
> > > > mtx_enter(d0f3c81c) at mtx_enter+0x27
> > > > pmap_extract_pae(d8bb0d80,f5605000,d8bb0da0) at pmap_extract_pae+0x53
> > > > pmap_pinit_pd_pae(d8bb0d80) at pmap_pinit_pd_pae+0x268
> > > > pmap_create(1,1000,f6fe5e86,d8bbfd54,d0f5ba18) at pmap_create+0xa8
> > > > uvmspace_fork(d0f5b5fc,d8bb3e34,d0f5b5fc,1,d1155f70) at
> > > > uvmspace_fork+0x56
> > > > process_new(d8bb3e34,d0f5b5fc,1) at process_new+0xeb
> > > > fork1(d0eb7b14,1,d04eb560,0,0,d1155f90) at fork1+0x1ba
> > > > panic: acquiring blockable sleep lock with spinlock or critical section
> > > > held (rwlock) kmmaplk
> > >
> > > pmap_kernel()'s mutexes aren't initialized. Diff below does that.
> >
> > Well, that is somewhat intentional. Those mutexes should never be
> > used for the kernel pmap. The kernel pmap is always there and is
> > updated atomically.
> >
> > So how did we end up trying to grab one of these mutexs?
>
> pmap_map_ptes() (both version of them) grab the current's pmap
> `pm_apte_mtx' which ends up being the kernel one in this case.
Actually I think only pmap_pinit_pd_pae() runs into this problem,
because it does this:
if (!pmap_extract(pmap, va + i * NBPG,
(paddr_t *)>pm_pdidx_intel[i]))
panic("%s: can't locate PD page\n", __func__);
I'm womewhat surprized by this. Why are we extracting addresses out
of the freshly created pmap?
Anyway, your analysis is right. When a kernel thread wants to use
pmap_extract(9) on a userland pmap, it needs to lock pm_apte_mtx to
prevent another thread from simultaniously activating a userland pmap
too. So indeed, pm_apte_mtx needs to be properly initialized for the
kernel pmap.
However, pm_mtx should never be used for the kernel pmap. If we don't
initialize the lock, witness will help us catching this condition, so
maybe we shouldn't...
> > > Index: arch/i386/i386/pmap.c
> > > ===
> > > RCS file: /cvs/src/sys/arch/i386/i386/pmap.c,v
> > > retrieving revision 1.209
> > > diff -u -p -r1.209 pmap.c
> > > --- arch/i386/i386/pmap.c 24 Sep 2020 11:36:50 - 1.209
> > > +++ arch/i386/i386/pmap.c 17 Dec 2020 21:47:11 -
> > > @@ -961,6 +961,8 @@ pmap_bootstrap(vaddr_t kva_start)
> > >*/
> > >
> > > kpm = pmap_kernel();
> > > + mtx_init(>pm_mtx, IPL_VM);
> > > + mtx_init(>pm_apte_mtx, IPL_VM);
> > > uvm_objinit(>pm_obj, NULL, 1);
> > > bzero(>pm_list, sizeof(kpm->pm_list)); /* pm_list not used */
> > > kpm->pm_pdir = (vaddr_t)(proc0.p_addr->u_pcb.pcb_cr3 + KERNBASE);
> > >
> > >
>
Re: rpki-client refactor some path building
On Fri, Dec 18, 2020 at 11:42:38AM +0100, Claudio Jeker wrote:
> On Thu, Dec 03, 2020 at 02:33:03PM +0100, Claudio Jeker wrote:
> > Use asprintf with %.*s to construct the path based on the mft file
> > location and the filename of the referenced file.
> >
> > Since the * field in printf(3) is expecting an int type, typecast the
> > ptrdiff_t to an int. Add an assert check to make sure there is no
> > overflow. Also do the same overflow check in mft.c where the same idiom is
> > used.
> >
> > Maybe some PATH_MAX checks should be placed in the mft parser.
>
> Ping
ok tb
>
> --
> :wq Claudio
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 main.c
> --- main.c2 Dec 2020 15:31:15 - 1.85
> +++ main.c3 Dec 2020 12:25:24 -
> @@ -451,23 +451,16 @@ static void
> queue_add_from_mft(int fd, struct entityq *q, const char *mft,
> const struct mftfile *file, enum rtype type, size_t *eid)
> {
> - size_t sz;
> char*cp, *nfile;
>
> /* Construct local path from filename. */
> -
> - sz = strlen(file->file) + strlen(mft);
> - if ((nfile = calloc(sz + 1, 1)) == NULL)
> - err(1, "calloc");
> -
> /* We know this is host/module/... */
>
> - strlcpy(nfile, mft, sz + 1);
> - cp = strrchr(nfile, '/');
> + cp = strrchr(mft, '/');
> assert(cp != NULL);
> - cp++;
> - *cp = '\0';
> - strlcat(nfile, file->file, sz + 1);
> + assert(cp - mft < INT_MAX);
> + if (asprintf(, "%.*s/%s", (int)(cp - mft), mft, file->file) == -1)
> + err(1, "asprintf");
>
> /*
>* Since we're from the same directory as the MFT file, we know
> Index: mft.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 mft.c
> --- mft.c 6 Nov 2020 04:22:18 - 1.19
> +++ mft.c 3 Dec 2020 12:37:15 -
> @@ -17,6 +17,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -457,6 +458,7 @@ mft_validfilehash(const char *fn, const
> /* Check hash of file now, but first build path for it */
> cp = strrchr(fn, '/');
> assert(cp != NULL);
> + assert(cp - fn < INT_MAX);
> if (asprintf(, "%.*s/%s", (int)(cp - fn), fn, m->file) == -1)
> err(1, "asprintf");
>
Re: converting uvm_km_valloc to km_alloc
> Date: Fri, 18 Dec 2020 13:04:42 +0100 > From: Alexander Bluhm > > On Fri, Dec 18, 2020 at 10:36:28AM +1000, Jonathan Matthew wrote: > > Here are a couple of relatively easy ones, applying changes from r1.86 of > > amd64's acpi_machdep.c to i386 and arm64. I've tested i386 but it turns > > out I > > don't have any arm64 machines with acpi. > > A machine like this? Something special to test? Runs fine with > your diff. Yeah, that's good enough ;). ok kettenis@ on the diff (and thanks Jonathan for helping out) > OpenBSD 6.8-current (GENERIC.MP) #0: Fri Dec 18 11:01:32 CET 2020 > r...@ot11.obsd-lab.genua.de:/usr/src/sys/arch/arm64/compile/GENERIC.MP > real mem = 136874385408 (130533MB) > avail mem = 132543135744 (126402MB) > random: good seed from bootblocks > mainbus0 at root: ACPI > psci0 at mainbus0: PSCI 1.1, SMCCC 65535.65535 > cpu0 at mainbus0 mpidr 0: Applied Micro X-Gene r3p2 > cpu0: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu0: 256KB 64b/line 32-way L2 cache > cpu0: CRC32,SHA2,SHA1,AES+PMULL > cpu1 at mainbus0 mpidr 1: Applied Micro X-Gene r3p2 > cpu1: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu1: 256KB 64b/line 32-way L2 cache > cpu1: CRC32,SHA2,SHA1,AES+PMULL > cpu2 at mainbus0 mpidr 100: Applied Micro X-Gene r3p2 > cpu2: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu2: 256KB 64b/line 32-way L2 cache > cpu2: CRC32,SHA2,SHA1,AES+PMULL > cpu3 at mainbus0 mpidr 101: Applied Micro X-Gene r3p2 > cpu3: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu3: 256KB 64b/line 32-way L2 cache > cpu3: CRC32,SHA2,SHA1,AES+PMULL > cpu4 at mainbus0 mpidr 200: Applied Micro X-Gene r3p2 > cpu4: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu4: 256KB 64b/line 32-way L2 cache > cpu4: CRC32,SHA2,SHA1,AES+PMULL > cpu5 at mainbus0 mpidr 201: Applied Micro X-Gene r3p2 > cpu5: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu5: 256KB 64b/line 32-way L2 cache > cpu5: CRC32,SHA2,SHA1,AES+PMULL > cpu6 at mainbus0 mpidr 300: Applied Micro X-Gene r3p2 > cpu6: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu6: 256KB 64b/line 32-way L2 cache > cpu6: CRC32,SHA2,SHA1,AES+PMULL > cpu7 at mainbus0 mpidr 301: Applied Micro X-Gene r3p2 > cpu7: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu7: 256KB 64b/line 32-way L2 cache > cpu7: CRC32,SHA2,SHA1,AES+PMULL > cpu8 at mainbus0 mpidr 400: Applied Micro X-Gene r3p2 > cpu8: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu8: 256KB 64b/line 32-way L2 cache > cpu8: CRC32,SHA2,SHA1,AES+PMULL > cpu9 at mainbus0 mpidr 401: Applied Micro X-Gene r3p2 > cpu9: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu9: 256KB 64b/line 32-way L2 cache > cpu9: CRC32,SHA2,SHA1,AES+PMULL > cpu10 at mainbus0 mpidr 500: Applied Micro X-Gene r3p2 > cpu10: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu10: 256KB 64b/line 32-way L2 cache > cpu10: CRC32,SHA2,SHA1,AES+PMULL > cpu11 at mainbus0 mpidr 501: Applied Micro X-Gene r3p2 > cpu11: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu11: 256KB 64b/line 32-way L2 cache > cpu11: CRC32,SHA2,SHA1,AES+PMULL > cpu12 at mainbus0 mpidr 600: Applied Micro X-Gene r3p2 > cpu12: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu12: 256KB 64b/line 32-way L2 cache > cpu12: CRC32,SHA2,SHA1,AES+PMULL > cpu13 at mainbus0 mpidr 601: Applied Micro X-Gene r3p2 > cpu13: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu13: 256KB 64b/line 32-way L2 cache > cpu13: CRC32,SHA2,SHA1,AES+PMULL > cpu14 at mainbus0 mpidr 700: Applied Micro X-Gene r3p2 > cpu14: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu14: 256KB 64b/line 32-way L2 cache > cpu14: CRC32,SHA2,SHA1,AES+PMULL > cpu15 at mainbus0 mpidr 701: Applied Micro X-Gene r3p2 > cpu15: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu15: 256KB 64b/line 32-way L2 cache > cpu15: CRC32,SHA2,SHA1,AES+PMULL > cpu16 at mainbus0 mpidr 800: Applied Micro X-Gene r3p2 > cpu16: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu16: 256KB 64b/line 32-way L2 cache > cpu16: CRC32,SHA2,SHA1,AES+PMULL > cpu17 at mainbus0 mpidr 801: Applied Micro X-Gene r3p2 > cpu17: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu17: 256KB 64b/line 32-way L2 cache > cpu17: CRC32,SHA2,SHA1,AES+PMULL > cpu18 at mainbus0 mpidr 900: Applied Micro X-Gene r3p2 > cpu18: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu18: 256KB 64b/line 32-way L2 cache > cpu18: CRC32,SHA2,SHA1,AES+PMULL > cpu19 at mainbus0 mpidr 901: Applied Micro X-Gene r3p2 > cpu19: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache > cpu19: 256KB 64b/line 32-way L2 cache > cpu19: CRC32,SHA2,SHA1,AES+PMULL >
Re: converting uvm_km_valloc to km_alloc
On Fri, Dec 18, 2020 at 10:36:28AM +1000, Jonathan Matthew wrote: > Here are a couple of relatively easy ones, applying changes from r1.86 of > amd64's acpi_machdep.c to i386 and arm64. I've tested i386 but it turns out I > don't have any arm64 machines with acpi. A machine like this? Something special to test? Runs fine with your diff. bluhm OpenBSD 6.8-current (GENERIC.MP) #0: Fri Dec 18 11:01:32 CET 2020 r...@ot11.obsd-lab.genua.de:/usr/src/sys/arch/arm64/compile/GENERIC.MP real mem = 136874385408 (130533MB) avail mem = 132543135744 (126402MB) random: good seed from bootblocks mainbus0 at root: ACPI psci0 at mainbus0: PSCI 1.1, SMCCC 65535.65535 cpu0 at mainbus0 mpidr 0: Applied Micro X-Gene r3p2 cpu0: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu0: 256KB 64b/line 32-way L2 cache cpu0: CRC32,SHA2,SHA1,AES+PMULL cpu1 at mainbus0 mpidr 1: Applied Micro X-Gene r3p2 cpu1: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu1: 256KB 64b/line 32-way L2 cache cpu1: CRC32,SHA2,SHA1,AES+PMULL cpu2 at mainbus0 mpidr 100: Applied Micro X-Gene r3p2 cpu2: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu2: 256KB 64b/line 32-way L2 cache cpu2: CRC32,SHA2,SHA1,AES+PMULL cpu3 at mainbus0 mpidr 101: Applied Micro X-Gene r3p2 cpu3: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu3: 256KB 64b/line 32-way L2 cache cpu3: CRC32,SHA2,SHA1,AES+PMULL cpu4 at mainbus0 mpidr 200: Applied Micro X-Gene r3p2 cpu4: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu4: 256KB 64b/line 32-way L2 cache cpu4: CRC32,SHA2,SHA1,AES+PMULL cpu5 at mainbus0 mpidr 201: Applied Micro X-Gene r3p2 cpu5: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu5: 256KB 64b/line 32-way L2 cache cpu5: CRC32,SHA2,SHA1,AES+PMULL cpu6 at mainbus0 mpidr 300: Applied Micro X-Gene r3p2 cpu6: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu6: 256KB 64b/line 32-way L2 cache cpu6: CRC32,SHA2,SHA1,AES+PMULL cpu7 at mainbus0 mpidr 301: Applied Micro X-Gene r3p2 cpu7: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu7: 256KB 64b/line 32-way L2 cache cpu7: CRC32,SHA2,SHA1,AES+PMULL cpu8 at mainbus0 mpidr 400: Applied Micro X-Gene r3p2 cpu8: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu8: 256KB 64b/line 32-way L2 cache cpu8: CRC32,SHA2,SHA1,AES+PMULL cpu9 at mainbus0 mpidr 401: Applied Micro X-Gene r3p2 cpu9: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu9: 256KB 64b/line 32-way L2 cache cpu9: CRC32,SHA2,SHA1,AES+PMULL cpu10 at mainbus0 mpidr 500: Applied Micro X-Gene r3p2 cpu10: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu10: 256KB 64b/line 32-way L2 cache cpu10: CRC32,SHA2,SHA1,AES+PMULL cpu11 at mainbus0 mpidr 501: Applied Micro X-Gene r3p2 cpu11: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu11: 256KB 64b/line 32-way L2 cache cpu11: CRC32,SHA2,SHA1,AES+PMULL cpu12 at mainbus0 mpidr 600: Applied Micro X-Gene r3p2 cpu12: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu12: 256KB 64b/line 32-way L2 cache cpu12: CRC32,SHA2,SHA1,AES+PMULL cpu13 at mainbus0 mpidr 601: Applied Micro X-Gene r3p2 cpu13: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu13: 256KB 64b/line 32-way L2 cache cpu13: CRC32,SHA2,SHA1,AES+PMULL cpu14 at mainbus0 mpidr 700: Applied Micro X-Gene r3p2 cpu14: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu14: 256KB 64b/line 32-way L2 cache cpu14: CRC32,SHA2,SHA1,AES+PMULL cpu15 at mainbus0 mpidr 701: Applied Micro X-Gene r3p2 cpu15: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu15: 256KB 64b/line 32-way L2 cache cpu15: CRC32,SHA2,SHA1,AES+PMULL cpu16 at mainbus0 mpidr 800: Applied Micro X-Gene r3p2 cpu16: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu16: 256KB 64b/line 32-way L2 cache cpu16: CRC32,SHA2,SHA1,AES+PMULL cpu17 at mainbus0 mpidr 801: Applied Micro X-Gene r3p2 cpu17: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu17: 256KB 64b/line 32-way L2 cache cpu17: CRC32,SHA2,SHA1,AES+PMULL cpu18 at mainbus0 mpidr 900: Applied Micro X-Gene r3p2 cpu18: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu18: 256KB 64b/line 32-way L2 cache cpu18: CRC32,SHA2,SHA1,AES+PMULL cpu19 at mainbus0 mpidr 901: Applied Micro X-Gene r3p2 cpu19: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu19: 256KB 64b/line 32-way L2 cache cpu19: CRC32,SHA2,SHA1,AES+PMULL cpu20 at mainbus0 mpidr a00: Applied Micro X-Gene r3p2 cpu20: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu20: 256KB 64b/line 32-way L2 cache cpu20: CRC32,SHA2,SHA1,AES+PMULL cpu21 at mainbus0 mpidr a01: Applied Micro X-Gene r3p2 cpu21: 32KB 64b/line 8-way L1 PIPT I-cache, 32KB 64b/line 8-way L1 D-cache cpu21: 256KB 64b/line
Re: rpki-client unmarshal empty strings as NULL
On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> rpki-client passes both empty strings and NULL strings as zero length
> objects. The unmarshal code then allocates memory in any case and so a
> NULL string is unmarshalled as empty string. This is not great, currently
> there are no empty strings but a fair amount of NULL strings.
> This diff changes the behaviour and now NULL is passed as NULL.
> This should simplify passing optional strings (e.g. in the entity queue
> code).
I will commit this later today. It will help with some further cleanup of
the codebase.
--
:wq Claudio
? obj
Index: io.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
retrieving revision 1.10
diff -u -p -r1.10 io.c
--- io.c2 Dec 2020 15:31:15 - 1.10
+++ io.c2 Dec 2020 15:54:38 -
@@ -153,7 +153,7 @@ io_buf_read_alloc(int fd, void **res, si
}
/*
- * Read a string (which may just be \0 and zero-length), allocating
+ * Read a string (returns NULL for zero-length strings), allocating
* space for it.
*/
void
@@ -162,6 +162,10 @@ io_str_read(int fd, char **res)
size_t sz;
io_simple_read(fd, , sizeof(size_t));
+ if (sz == 0) {
+ *res = NULL;
+ return;
+ }
if ((*res = calloc(sz + 1, 1)) == NULL)
err(1, NULL);
io_simple_read(fd, *res, sz);
Re: rpki-client refactor some path building
On Thu, Dec 03, 2020 at 02:33:03PM +0100, Claudio Jeker wrote:
> Use asprintf with %.*s to construct the path based on the mft file
> location and the filename of the referenced file.
>
> Since the * field in printf(3) is expecting an int type, typecast the
> ptrdiff_t to an int. Add an assert check to make sure there is no
> overflow. Also do the same overflow check in mft.c where the same idiom is
> used.
>
> Maybe some PATH_MAX checks should be placed in the mft parser.
Ping
--
:wq Claudio
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.85
diff -u -p -r1.85 main.c
--- main.c 2 Dec 2020 15:31:15 - 1.85
+++ main.c 3 Dec 2020 12:25:24 -
@@ -451,23 +451,16 @@ static void
queue_add_from_mft(int fd, struct entityq *q, const char *mft,
const struct mftfile *file, enum rtype type, size_t *eid)
{
- size_t sz;
char*cp, *nfile;
/* Construct local path from filename. */
-
- sz = strlen(file->file) + strlen(mft);
- if ((nfile = calloc(sz + 1, 1)) == NULL)
- err(1, "calloc");
-
/* We know this is host/module/... */
- strlcpy(nfile, mft, sz + 1);
- cp = strrchr(nfile, '/');
+ cp = strrchr(mft, '/');
assert(cp != NULL);
- cp++;
- *cp = '\0';
- strlcat(nfile, file->file, sz + 1);
+ assert(cp - mft < INT_MAX);
+ if (asprintf(, "%.*s/%s", (int)(cp - mft), mft, file->file) == -1)
+ err(1, "asprintf");
/*
* Since we're from the same directory as the MFT file, we know
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.19
diff -u -p -r1.19 mft.c
--- mft.c 6 Nov 2020 04:22:18 - 1.19
+++ mft.c 3 Dec 2020 12:37:15 -
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#include
#include
@@ -457,6 +458,7 @@ mft_validfilehash(const char *fn, const
/* Check hash of file now, but first build path for it */
cp = strrchr(fn, '/');
assert(cp != NULL);
+ assert(cp - fn < INT_MAX);
if (asprintf(, "%.*s/%s", (int)(cp - fn), fn, m->file) == -1)
err(1, "asprintf");
bgpd refactor roa-set internals
In preparation for RTR support this diff changes the internal
representation of roa-set to a simple RB tree based on struct roa.
The big difference is that overlapping roas, e.g.
10/8 source-as 3
10/8 maxlen 24 source-as 3
are now merged in the RDE and so bgpd -nv will show both entries instead
of only the second one.
On my testbox there is no difference in OVS state between a -current bgpd
and the one with this diff applied. More testing welcome.
--
:wq Claudio
Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.230
diff -u -p -r1.230 bgpd.c
--- bgpd.c 5 Nov 2020 11:52:59 - 1.230
+++ bgpd.c 18 Dec 2020 10:27:44 -
@@ -502,6 +502,7 @@ send_config(struct bgpd_config *conf)
struct as_set *aset;
struct prefixset*ps;
struct prefixset_item *psi, *npsi;
+ struct roa *roa, *nroa;
reconfpending = 2; /* one per child */
@@ -567,7 +568,6 @@ send_config(struct bgpd_config *conf)
if (imsg_compose(ibuf_rde, IMSG_RECONF_PREFIX_SET_ITEM,
0, 0, -1, psi, sizeof(*psi)) == -1)
return (-1);
- set_free(psi->set);
free(psi);
}
free(ps);
@@ -579,23 +579,12 @@ send_config(struct bgpd_config *conf)
if (imsg_compose(ibuf_rde, IMSG_RECONF_ORIGIN_SET, 0, 0, -1,
ps->name, sizeof(ps->name)) == -1)
return (-1);
- RB_FOREACH_SAFE(psi, prefixset_tree, >psitems, npsi) {
- struct roa_set *rs;
- size_t i, l, n;
- RB_REMOVE(prefixset_tree, >psitems, psi);
- rs = set_get(psi->set, );
- for (i = 0; i < n; i += l) {
- l = (n - i > 1024 ? 1024 : n - i);
- if (imsg_compose(ibuf_rde,
- IMSG_RECONF_ROA_SET_ITEMS,
- 0, 0, -1, rs + i, l * sizeof(*rs)) == -1)
- return -1;
- }
- if (imsg_compose(ibuf_rde, IMSG_RECONF_PREFIX_SET_ITEM,
- 0, 0, -1, psi, sizeof(*psi)) == -1)
+ RB_FOREACH_SAFE(roa, roa_tree, >roaitems, nroa) {
+ RB_REMOVE(roa_tree, >roa, roa);
+ if (imsg_compose(ibuf_rde, IMSG_RECONF_ROA_ITEM, 0, 0,
+ -1, roa, sizeof(*roa)) == -1)
return (-1);
- set_free(psi->set);
- free(psi);
+ free(roa);
}
free(ps);
}
@@ -604,23 +593,12 @@ send_config(struct bgpd_config *conf)
if (imsg_compose(ibuf_rde, IMSG_RECONF_ROA_SET, 0, 0, -1,
NULL, 0) == -1)
return (-1);
- RB_FOREACH_SAFE(psi, prefixset_tree, >roa, npsi) {
- struct roa_set *rs;
- size_t i, l, n;
- RB_REMOVE(prefixset_tree, >roa, psi);
- rs = set_get(psi->set, );
- for (i = 0; i < n; i += l) {
- l = (n - i > 1024 ? 1024 : n - i);
- if (imsg_compose(ibuf_rde,
- IMSG_RECONF_ROA_SET_ITEMS,
- 0, 0, -1, rs + i, l * sizeof(*rs)) == -1)
- return -1;
- }
- if (imsg_compose(ibuf_rde, IMSG_RECONF_PREFIX_SET_ITEM,
- 0, 0, -1, psi, sizeof(*psi)) == -1)
+ RB_FOREACH_SAFE(roa, roa_tree, >roa, nroa) {
+ RB_REMOVE(roa_tree, >roa, roa);
+ if (imsg_compose(ibuf_rde, IMSG_RECONF_ROA_ITEM, 0, 0,
+ -1, roa, sizeof(*roa)) == -1)
return (-1);
- set_free(psi->set);
- free(psi);
+ free(roa);
}
}
Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.405
diff -u -p -r1.405 bgpd.h
--- bgpd.h 5 Nov 2020 11:52:59 - 1.405
+++ bgpd.h 18 Dec 2020 10:27:21 -
@@ -264,6 +264,21 @@ struct rde_prefixset {
};
SIMPLEQ_HEAD(rde_prefixset_head, rde_prefixset);
+struct roa {
+ RB_ENTRY(roa) entry;
+ uint8_t aid;
+ uint8_t prefixlen;
+ uint8_t maxlen;
+ uint8_t pad;
+ uint32_tasnum;
+ union {
+ struct in_addr
Re: Enhancing (some) PF state
Hello Sven, your change makes me wonder: 'what is the actual problem you are trying to solve'? the reason I'm asking is that latency is just one factor, which contributes to TCP connection performance. The other factor (and perhaps more important) is to guess amount of retransmitted data. Processes (a.k.a. endpoints), which communicate over TCP can experience significant delay once TCP packets starts to be dropped. Those dropped TCP packets contribute to delay experienced in the more significant way, than 'network latency' in sense of roundtrip. I'm not much experienced firewall administrator, the only firewall I run is APU box at my home, hence I'm sorry if my question sounds naive. So basically what sort of problem in network you hope to diagnose with PF? And also don't get me wrong: I like your idea to extend PF to enable firewall to provide better picture of what happens on network. I just want to point out that sampling network latency (round-trip) might not be sufficient. thanks and regards sashan On Thu, Dec 17, 2020 at 02:44:41PM -0500, Sven F. wrote: > Dear readers, > > pfctl -vv -ss shows detailed information on states. > I would like to improve the information provided about specific TCP > connections, > regarding the latency of the network. > An obvious way seems to be to measure the time to get ACKs back. > Another way would be to use packets timestamps. > > I patched my kernel to extract this information and > log it, before trying to report it to the userland. > I did not use timestamps, because I am not sure how i could do that. > If you have any advice on that, it would be welcome. > Moreover the patch is a prototype, > So I would appreciate any feedback on my diff (attached): > Currently the code is using a LABEL to trigger the measure, > of course, later it should be a keyword like "latency" in the rules. > For example : > match proto tcp to port 80 latency > Or something else. > This would be discuss after choosing a method for latency computation, > > Maybe there is a better way to extract network TCP latencies information > ( i would like to avoid running in promiscuous, but if a current > packaged software does it well... ) > but I did not come across it. > Please share if you know of a better way to tackle this. > > > Happy holidays !
