ftp.openbsd.org currently unreachable
Hello all, I'm writing to provide a heads up that ftp.openbsd.org appears to currently be unreachable. For example, `curl -O https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.6.3.tar.gz` is currently producing: curl: (28) Failed to connect to ftp.openbsd.org port 443 after 129852 ms: Connection timed out OR curl: (7) Failed to connect to ftp.openbsd.org port 443 after 19097 ms: Couldn't connect to server OR curl: (7) Failed to connect to ftp.openbsd.org port 443 after 54 ms: No route to host depending on the platform. Regards, Alex -- All that is necessary for evil to succeed is for good people to do nothing.
Re: avoid truncation of filtered smtpd data lines
On Tue, 20 Jun 2023 21:38:49 +0200, Omar Polo wrote:
> Then I realized that we don't need to copy the line at all. We're
> already using strtoull to parse the number and the payload is the last
> field of the received line, so we can just advance the pointer. The
> drawback is that we now need to use a few strncmp, but I think it's
> worth it.
This seems like a good approach, minor comments inline.
- todd
> diff /usr/src
> commit - 5c586f5f5360442b12bbc4ea18ce006ea0c3d126
> path + /usr/src
> blob - a714446c26fee299f4450ff1ad40289b5b327824
> file + usr.sbin/smtpd/lka_filter.c
> --- usr.sbin/smtpd/lka_filter.c
> +++ usr.sbin/smtpd/lka_filter.c
> @@ -593,40 +593,27 @@ lka_filter_process_response(const char *name, const ch
> {
> uint64_t reqid;
> uint64_t token;
> - char buffer[LINE_MAX];
> char *ep = NULL;
> - char *kind = NULL;
> - char *qid = NULL;
> - /*char *phase = NULL;*/
> - char *response = NULL;
> - char *parameter = NULL;
> + const char *kind = NULL;
> + const char *qid = NULL;
> + const char *response = NULL;
> + const char *parameter = NULL;
> struct filter_session *fs;
>
> - (void)strlcpy(buffer, line, sizeof buffer);
> - if ((ep = strchr(buffer, '|')) == NULL)
> + kind = line;
> +
> + if ((ep = strchr(kind, '|')) == NULL)
> fatalx("Missing token: %s", line);
> - ep[0] = '\0';
> -
> - kind = buffer;
> -
> qid = ep+1;
> - if ((ep = strchr(qid, '|')) == NULL)
> - fatalx("Missing reqid: %s", line);
> - ep[0] = '\0';
> -
This is not a new problem but we really should set errno=0 before
calling strtoull() for the first time. It is possible for errno
to already be set to ERANGE which causes problems if strtoull()
returns ULLONG_MAX and there is not an error. It's fine if you
don't want to fix that in this commit but I do think it should get
fixed.
> reqid = strtoull(qid, &ep, 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid reqid: %s", line);
> if (errno == ERANGE && reqid == ULLONG_MAX)
> fatal("Invalid reqid: %s", line);
>
> - qid = ep+1;
> - if ((ep = strchr(qid, '|')) == NULL)
> - fatal("Missing directive: %s", line);
> - ep[0] = '\0';
> -
> + qid = ep + 1;
> token = strtoull(qid, &ep, 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid token: %s", line);
> if (errno == ERANGE && token == ULLONG_MAX)
> fatal("Invalid token: %s", line);
> @@ -637,7 +624,7 @@ lka_filter_process_response(const char *name, const ch
> if ((fs = tree_get(&sessions, reqid)) == NULL)
> return;
>
> - if (strcmp(kind, "filter-dataline") == 0) {
> + if (strncmp(kind, "filter-dataline|", 16) == 0) {
> if (fs->phase != FILTER_DATA_LINE)
> fatalx("filter-dataline out of dataline phase");
> filter_data_next(token, reqid, response);
> @@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
> if (fs->phase == FILTER_DATA_LINE)
> fatalx("filter-result in dataline phase");
>
> - if ((ep = strchr(response, '|'))) {
> + if ((ep = strchr(response, '|')) != NULL)
> parameter = ep + 1;
> - ep[0] = '\0';
> - }
>
It took me a minute to realize that this is OK, but it seems fine.
> if (strcmp(response, "proceed") == 0) {
> - if (parameter != NULL)
> - fatalx("Unexpected parameter after proceed: %s", line);
> filter_protocol_next(token, reqid, 0);
> return;
> } else if (strcmp(response, "junk") == 0) {
> - if (parameter != NULL)
> - fatalx("Unexpected parameter after junk: %s", line);
> if (fs->phase == FILTER_COMMIT)
> fatalx("filter-reponse junk after DATA");
> filter_result_junk(reqid);
> @@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
> if (parameter == NULL)
> fatalx("Missing parameter: %s", line);
>
> - if (strcmp(response, "rewrite") == 0)
> + if (strncmp(response, "rewrite|", 8) == 0)
> filter_result_rewrite(reqid, parameter);
> - else if (strcmp(response, "reject") == 0)
> + else if (strncmp(response, "reject|", 7) == 0)
> filter_result_reject(reqid, parameter);
> - else if (strcmp(response, "disconnect") == 0)
> + else if (strncmp(response, "disconnect|", 11) == 0)
> filter_result_disconnect(reqid, parameter);
> else
> fatalx("Invalid directive: %s", line);
>
Re: rpki-client: disallow empty RFC 3779 extensions
On Tue, Jun 20, 2023 at 10:10:43PM +0200, Theo Buehler wrote: > The first warning cannot be hit because the X509v3_asid_is_canonical() > errors on empty asIdsOrRanges sequences. This is not the case for > IPAddrBlocks... > > There is some ambiguity in RFC 6487, 4.8.10 whether empty > ipAddressesOrRanges are allowed or not. I opted for the stricter > interpretation matching AS numbers and likely the intent. I concur, extensions with empty containers are a mis-issuance of the product. OK job@
rpki-client: disallow empty RFC 3779 extensions
The first warning cannot be hit because the X509v3_asid_is_canonical()
errors on empty asIdsOrRanges sequences. This is not the case for
IPAddrBlocks...
There is some ambiguity in RFC 6487, 4.8.10 whether empty
ipAddressesOrRanges are allowed or not. I opted for the stricter
interpretation matching AS numbers and likely the intent.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.108
diff -u -p -r1.108 cert.c
--- cert.c 9 May 2023 10:34:32 - 1.108
+++ cert.c 20 Jun 2023 20:05:44 -
@@ -204,6 +204,11 @@ sbgp_assysnum(struct parse *p, X509_EXTE
goto out;
}
+ if (asz == 0) {
+ warnx("%s: RFC 6487 section 4.8.11: empty asIdsOrRanges",
+ p->fn);
+ goto out;
+ }
if (asz >= MAX_AS_SIZE) {
warnx("%s: too many AS number entries: limit %d",
p->fn, MAX_AS_SIZE);
@@ -371,6 +376,11 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
p->fn, af->ipAddressChoice->type);
goto out;
}
+ if (ipsz == p->res->ipsz) {
+ warnx("%s: RFC 6487 4.8.10: empty ipAddressesOrRanges",
+ p->fn);
+ goto out;
+ }
if (ipsz >= MAX_IP_SIZE)
goto out;
@@ -410,6 +420,11 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
goto out;
}
}
+ }
+
+ if (p->res->ipsz == 0) {
+ warnx("%s: RFC 6487 section 4.8.10: empty ipAddrBlock", p->fn);
+ goto out;
}
rc = 1;
avoid truncation of filtered smtpd data lines
hello tech@,
this was reported some time ago on the OpenSMTPD-portable repository[0]
[0]: https://github.com/OpenSMTPD/OpenSMTPD/pull/1192
Filters can register to the data-line event to alter the mail content.
smtpd, when parsing the filter' output it first copies the received
line in a temporary buffer. Here truncation may happen if a line of
the mail is longer than LINE_MAX (minus 50 to be exact).
The proposed diff on github bumps the local buffer to SMTP_LINE_MAX +
256, but I think it's quite wasteful to have a buffer that long on the
stack (SMTP_LINE_MAX is 65535 bytes).
A very simple fix could be:
: if (strcmp(kind, "filter-dataline") == 0) {
: if (fs->phase != FILTER_DATA_LINE)
: fatalx("filter-dataline out of dataline phase");
: - filter_data_next(token, reqid, response);
: + filter_data_next(token, reqid, line + (response - buffer));
: return;
: }
: if (fs->phase == FILTER_DATA_LINE)
since `line' contains the original line and it's guaranteed that the
fields of the reply excluding the last (variable width) fits the
buffer, it works. maybe it's too clever.
Then I realized that we don't need to copy the line at all. We're
already using strtoull to parse the number and the payload is the last
field of the received line, so we can just advance the pointer. The
drawback is that we now need to use a few strncmp, but I think it's
worth it.
disclaimer: i've run this only on localhost so far.
To reproduce, you can use an awk script like the following
#!/usr/bin/awk -f
# filter-dummy: copies the data lines as-is.
BEGIN { FS = "|" }
/^config\|ready$/ {
print "register|filter|smtp-in|data-line"
print "register|ready"
fflush()
}
"filter|smtp-in|data-line" == $1"|"$4"|"$5 {
line = substr($0, length($1$2$3$4$5$6$7) + 8)
printf "filter-dataline|%s|%s|%s\n", $6, $7, line
fflush()
}
with a configuration like:
table aliases file:/etc/mail/aliases
filter "dummy" proc-exec "/path/to/filter-dummy"
listen on lo0 filter "dummy"
action "local" mbox alias
match from local for local action "local"
and then sending a mail with a very long line:
% tr -cd '[:print:]' phase != FILTER_DATA_LINE)
fatalx("filter-dataline out of dataline phase");
filter_data_next(token, reqid, response);
@@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
if (fs->phase == FILTER_DATA_LINE)
fatalx("filter-result in dataline phase");
- if ((ep = strchr(response, '|'))) {
+ if ((ep = strchr(response, '|')) != NULL)
parameter = ep + 1;
- ep[0] = '\0';
- }
if (strcmp(response, "proceed") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after proceed: %s", line);
filter_protocol_next(token, reqid, 0);
return;
} else if (strcmp(response, "junk") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after junk: %s", line);
if (fs->phase == FILTER_COMMIT)
fatalx("filter-reponse junk after DATA");
filter_result_junk(reqid);
@@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
if (parameter == NULL)
fatalx("Missing parameter: %s", line);
- if (strcmp(response, "rewrite") == 0)
+ if (strncmp(response, "rewrite|", 8) == 0)
filter_result_rewrite(reqid, parameter);
- else if (strcmp(response, "reject") == 0)
+ else if (strncmp(response, "reject|", 7) == 0)
filter_result_reject(reqid, parameter);
- else if (strcmp(response, "disconnect") == 0)
+ else if (strncmp(response, "disconnect|", 11) == 0)
filter_result_disconnect(reqid, parameter);
else
fatalx("Invalid directive: %s", line);
Re: rpki-client: better handling of X509_get_ext_d2i() errors
On Tue, Jun 20, 2023 at 07:50:19PM +0200, Theo Buehler wrote: > X509_get_ext_d2i() is one of those very special OpenSSL interfaces... > > It can return NULL for various reasons. If it returns NULL and crit is > not -1, something bad happened. If crit is -2, multiple extensions with > the same OID as the one corresponding to the nid were found (this is not > allowed per RFC 5280, 4.2). It returns NULL if it failed to deserialize > the extension, be it due to bad DER or an allocation failure. In these > cases crit will be 1 or 0, depending on whether the extension was marked > critical. > > So instead of accepting an object in the situation that crit != -1, we > should warn and throw an error. We can't errx() since we can't really > tell allocation failure from failure due to a malformed extension. > > I also added a check for NULL and criticality of basic constraints, > which were missing (per RFC 5280 criticality is optional, so libcrypto > doesn't check that, but RFC 6487 is clear here). > > The warnings in x509*inherits() are minimal. The callers of > x509_inherits() warn. For x509_any_inherits() this is not so. The > annoying bit is that it is used in auth_insert(). I know how I want > to deal with that, but that is largely independent of this diff. OK job@
Re: rpki-client: warn on duplicate X509v3 extensions
On Tue, Jun 20, 2023 at 08:58:23PM +0200, Theo Buehler wrote: > For some reason libcrypto doesn't check this part of RFC 5280, 4.2: A > certificate MUST NOT include more than one instance of a particular > extension. > > With the badCertSIA2x.cer from Ties's test artefacts, I get this > warning: > > rpki-client: badCertSIA2x.cer: RFC 5280 section 4.2: duplicate > subjectInfoAccess extension > > https://github.com/ties/rpki-commons-object-scan/tree/main/app/src/test/resources/bbn-conformance/root > > The diff below is straightforward. It does not help EE certs. That > could be done similarly. OK job@
rpki-client: warn on duplicate X509v3 extensions
For some reason libcrypto doesn't check this part of RFC 5280, 4.2:
A certificate MUST NOT include more than one instance of a particular
extension.
With the badCertSIA2x.cer from Ties's test artefacts, I get this warning:
rpki-client: badCertSIA2x.cer: RFC 5280 section 4.2: duplicate
subjectInfoAccess extension
https://github.com/ties/rpki-commons-object-scan/tree/main/app/src/test/resources/bbn-conformance/root
The diff below is straightforward. It does not help EE certs. That
could be done similarly.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.109
diff -u -p -r1.109 cert.c
--- cert.c 20 Jun 2023 12:28:08 - 1.109
+++ cert.c 20 Jun 2023 18:46:22 -
@@ -648,7 +648,6 @@ cert_parse_pre(const char *fn, const uns
{
const unsigned char *oder;
int extsz;
- int sia_present = 0;
size_t i;
X509*x = NULL;
X509_EXTENSION *ext = NULL;
@@ -658,7 +657,10 @@ cert_parse_pre(const char *fn, const uns
ASN1_OBJECT *obj;
EVP_PKEY*pkey;
struct parse p;
- int nid;
+ int nid, ip, as, sia, cp, crldp, aia, aki, ski,
+eku, bc, ku;
+
+ nid = ip = as = sia = cp = crldp = aia = aki = ski = eku = bc = ku = 0;
/* just fail for empty buffers, the warning was printed elsewhere */
if (der == NULL)
@@ -721,38 +723,58 @@ cert_parse_pre(const char *fn, const uns
obj = X509_EXTENSION_get_object(ext);
assert(obj != NULL);
- switch (OBJ_obj2nid(obj)) {
+ switch ((nid = OBJ_obj2nid(obj))) {
case NID_sbgp_ipAddrBlock:
+ if (ip++ >= 1)
+ goto dup;
if (!sbgp_ipaddrblk(&p, ext))
goto out;
break;
case NID_sbgp_autonomousSysNum:
+ if (as++ > 0)
+ goto dup;
if (!sbgp_assysnum(&p, ext))
goto out;
break;
case NID_sinfo_access:
- sia_present = 1;
+ if (sia++ > 0)
+ goto dup;
if (!sbgp_sia(&p, ext))
goto out;
break;
case NID_certificate_policies:
+ if (cp++ > 0)
+ goto dup;
if (!certificate_policies(&p, ext))
goto out;
break;
case NID_crl_distribution_points:
- /* ignored here, handled later */
+ if (crldp++ > 0)
+ goto dup;
break;
case NID_info_access:
+ if (aia++ > 0)
+ goto dup;
break;
case NID_authority_key_identifier:
+ if (aki++ > 0)
+ goto dup;
break;
case NID_subject_key_identifier:
+ if (ski++ > 0)
+ goto dup;
break;
case NID_ext_key_usage:
+ if (eku++ > 0)
+ goto dup;
break;
case NID_basic_constraints:
+ if (bc++ > 0)
+ goto dup;
break;
case NID_key_usage:
+ if (ku++ > 0)
+ goto dup;
break;
default:
/* unexpected extensions warrant investigation */
@@ -831,7 +853,7 @@ cert_parse_pre(const char *fn, const uns
goto out;
}
}
- if (sia_present) {
+ if (sia) {
warnx("%s: unexpected SIA extension in BGPsec cert",
p.fn);
goto out;
@@ -850,7 +872,10 @@ cert_parse_pre(const char *fn, const uns
p.res->x509 = x;
return p.res;
-out:
+ dup:
+ warnx("%s: RFC 5280 section 4.2: duplicate %s extension", fn,
+ OBJ_nid2sn(nid));
+ out:
cert_free(p.res);
X509_free(x);
return NULL;
rpki-client: better handling of X509_get_ext_d2i() errors
X509_get_ext_d2i() is one of those very special OpenSSL interfaces...
It can return NULL for various reasons. If it returns NULL and crit is
not -1, something bad happened. If crit is -2, multiple extensions with
the same OID as the one corresponding to the nid were found (this is not
allowed per RFC 5280, 4.2). It returns NULL if it failed to deserialize
the extension, be it due to bad DER or an allocation failure. In these
cases crit will be 1 or 0, depending on whether the extension was marked
critical.
So instead of accepting an object in the situation that crit != -1, we
should warn and throw an error. We can't errx() since we can't really
tell allocation failure from failure due to a malformed extension.
I also added a check for NULL and criticality of basic constraints,
which were missing (per RFC 5280 criticality is optional, so libcrypto
doesn't check that, but RFC 6487 is clear here).
The warnings in x509*inherits() are minimal. The callers of
x509_inherits() warn. For x509_any_inherits() this is not so. The
annoying bit is that it is used in auth_insert(). I know how I want
to deal with that, but that is largely independent of this diff.
Index: x509.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
retrieving revision 1.71
diff -u -p -r1.71 x509.c
--- x509.c 22 May 2023 15:07:02 - 1.71
+++ x509.c 20 Jun 2023 11:55:18 -
@@ -146,8 +146,14 @@ x509_get_aki(X509 *x, const char *fn, ch
*aki = NULL;
akid = X509_get_ext_d2i(x, NID_authority_key_identifier, &crit, NULL);
- if (akid == NULL)
+ if (akid == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.3: error parsing AKI",
+ fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.3: "
"AKI: extension not non-critical", fn);
@@ -200,8 +206,14 @@ x509_get_ski(X509 *x, const char *fn, ch
*ski = NULL;
os = X509_get_ext_d2i(x, NID_subject_key_identifier, &crit, NULL);
- if (os == NULL)
+ if (os == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.2: error parsing SKI",
+ fn);
+ return 0;
+ }
return 1;
+ }
if (crit != 0) {
warnx("%s: RFC 6487 section 4.8.2: "
"SKI: extension not non-critical", fn);
@@ -258,6 +270,20 @@ x509_get_purpose(X509 *x, const char *fn
if (X509_check_ca(x) == 1) {
bc = X509_get_ext_d2i(x, NID_basic_constraints, &crit, NULL);
+ if (bc == NULL) {
+ if (crit != -1)
+ warnx("%s: RFC 6487 section 4.8.1: "
+ "error parsing basic constraints", fn);
+ else
+ warnx("%s: RFC 6487 section 4.8.1: "
+ "missing basic constraints", fn);
+ goto out;
+ }
+ if (crit != 1) {
+ warnx("%s: RFC 6487 section 4.8.1: Basic Constraints "
+ "must be marked critical", fn);
+ goto out;
+ }
if (bc->pathlen != NULL) {
warnx("%s: RFC 6487 section 4.8.1: Path Length "
"Constraint must be absent", fn);
@@ -274,7 +300,10 @@ x509_get_purpose(X509 *x, const char *fn
eku = X509_get_ext_d2i(x, NID_ext_key_usage, &crit, NULL);
if (eku == NULL) {
- warnx("%s: EKU: extension missing", fn);
+ if (crit != -1)
+ warnx("%s: error parsing EKU", fn);
+ else
+ warnx("%s: EKU: extension missing", fn);
goto out;
}
if (crit != 0) {
@@ -372,13 +401,13 @@ x509_get_aia(X509 *x, const char *fn, ch
*aia = NULL;
info = X509_get_ext_d2i(x, NID_info_access, &crit, NULL);
- if (info == NULL)
+ if (info == NULL) {
+ if (crit != -1) {
+ warnx("%s: RFC 6487 section 4.8.7: error parsing AIA",
+ fn);
+ return 0;
+ }
return 1;
-
- if ((X509_get_extension_flags(x) & EXFLAG_SS) != 0) {
- warnx("%s: RFC 6487 section 4.8.7: AIA must be absent from "
- "a self-signed certificate", fn);
- goto out;
}
if (crit != 0) {
@@ -387,6 +416,12 @@ x509_get_aia(X509 *x, const char *fn, ch
goto out;
}
+ if ((X509_get_extension_flags(x) & EXFLAG_SS) != 0) {
+ warnx("%s: RFC 6487 section 4.8.7: AIA
Re: open_memstream cleanup
On Tue, 20 Jun 2023 17:49:46 +0200, Claudio Jeker wrote: > In open_memstream() the code does a bzero() of the new memory even though > recallocarray() used which does this already. > > In open_wmemstream() the code does the same but is still using > reallocarray(). So adjust that code to be like open_memstream(). OK millert@ - todd
Re: more relayd cleanup
> On 20 Jun 2023, at 18:16, Claudio Jeker wrote: > > On Tue, Jun 20, 2023 at 03:35:11PM +0200, Theo Buehler wrote: >>> On Tue, Jun 20, 2023 at 02:17:06PM +0200, Claudio Jeker wrote: >>> Ok, this went overboard. I just wanted to clean up a bit more in >>> check_tcp.c but noticed check_send_expect and CHECK_BINSEND_EXPECT. >>> >>> This code is not very consitent in the differnt ways the strings are >>> encoded. Especially check_send_expect() is a bit of a mess because of >>> that. >>> >>> While there I noticed string2binary() and decided to write it in simpler >>> way (copying code over from rpki-client). >>> >>> All in all I think this diff is improving the situation a little bit. >> >> Write "I will not modify Reyk's daemons more than I need to" 100 times. >> >> Then it's ok. >> > > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to say my name three times … > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more than I need to > I will not modify Reyk's daemons more th
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 06:29:59PM +0200, Claudio Jeker wrote:
> On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote:
> > This diff updates ospfd to use the new ibuf API.
> >
> > It mainly removes the use of ibuf_seek() and replaces these calls with
> > ibuf_set().
> >
> > Regress still passes with this diff in.
>
> Here the same diff for ospf6d.
ok
Two minor comments below. Feel free to ignore them.
> --
> :wq Claudio
>
> Index: database.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 database.c
> --- database.c8 Mar 2023 04:43:14 - 1.22
> +++ database.c16 Jun 2023 10:27:04 -
> @@ -51,7 +51,7 @@ send_db_description(struct nbr *nbr)
> goto fail;
>
> /* reserve space for database description header */
> - if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL)
> + if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1)
> goto fail;
>
> switch (nbr->state) {
> @@ -134,8 +134,9 @@ send_db_description(struct nbr *nbr)
> dd_hdr.bits = bits;
> dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num);
>
> - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)),
> - &dd_hdr, sizeof(dd_hdr));
> + if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr,
> + sizeof(dd_hdr)) == -1)
> + goto fail;
>
> /* calculate checksum */
> if (upd_ospf_hdr(buf, nbr->iface))
> Index: lsupdate.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 lsupdate.c
> --- lsupdate.c8 Mar 2023 04:43:14 - 1.22
> +++ lsupdate.c16 Jun 2023 10:27:15 -
> @@ -177,7 +177,7 @@ prepare_ls_update(struct iface *iface, i
> goto fail;
>
> /* reserve space for number of lsa field */
> - if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL)
> + if (ibuf_add_zero(buf, sizeof(u_int32_t)) == -1)
> goto fail;
>
> return (buf);
> @@ -208,8 +208,10 @@ add_ls_update(struct ibuf *buf, struct i
> age = ntohs(age);
> if ((age += older + iface->transmit_delay) >= MAX_AGE)
> age = MAX_AGE;
> - age = htons(age);
> - memcpy(ibuf_seek(buf, ageoff, sizeof(age)), &age, sizeof(age));
> + if (ibuf_set_n16(buf, ageoff, age) == -1) {
> + log_warn("add_ls_update");
This error is weid but it's like the others nearby.
> + return (0);
> + }
>
> return (1);
> }
> @@ -218,9 +220,8 @@ int
> send_ls_update(struct ibuf *buf, struct iface *iface, struct in6_addr addr,
> u_int32_t nlsa)
> {
> - nlsa = htonl(nlsa);
> - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(nlsa)),
> - &nlsa, sizeof(nlsa));
> + if (ibuf_set_n32(buf, sizeof(struct ospf_hdr), nlsa) == -1)
> + goto fail;
> /* calculate checksum */
> if (upd_ospf_hdr(buf, iface))
> goto fail;
> Index: ospfe.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/ospfe.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 ospfe.c
> --- ospfe.c 8 Mar 2023 04:43:14 - 1.68
> +++ ospfe.c 20 Jun 2023 16:25:52 -
> @@ -780,11 +780,11 @@ orig_rtr_lsa(struct area *area)
> fatal("orig_rtr_lsa");
>
> /* reserve space for LSA header and LSA Router header */
> - if (ibuf_reserve(buf, sizeof(lsa_hdr)) == NULL)
> - fatal("orig_rtr_lsa: ibuf_reserve failed");
> + if (ibuf_add_zero(buf, sizeof(lsa_hdr)) == -1)
> + fatal("orig_rtr_lsa: ibuf_add_zero failed");
>
> - if (ibuf_reserve(buf, sizeof(lsa_rtr)) == NULL)
> - fatal("orig_rtr_lsa: ibuf_reserve failed");
> + if (ibuf_add_zero(buf, sizeof(lsa_rtr)) == -1)
> + fatal("orig_rtr_lsa: ibuf_add_zero failed");
These two could be combined like the others below, but I'm fine with
keeping it as close to mechanical as possible.
>
> /* links */
> LIST_FOREACH(iface, &area->iface_list, entry) {
> @@ -944,8 +944,8 @@ orig_rtr_lsa(struct area *area)
> LSA_24_SETLO(lsa_rtr.opts, area_ospf_options(area));
> LSA_24_SETHI(lsa_rtr.opts, flags);
> lsa_rtr.opts = htonl(lsa_rtr.opts);
> - memcpy(ibuf_seek(buf, sizeof(lsa_hdr), sizeof(lsa_rtr)),
> - &lsa_rtr, sizeof(lsa_rtr));
> + if (ibuf_set(buf, sizeof(lsa_hdr), &lsa_rtr, sizeof(lsa_rtr)) == -1)
> + fatal("orig_rtr_lsa: ibuf_set failed");
>
> /* LSA header */
> lsa_hdr.age = htons(DEFAULT_AGE);
> @@ -956,11 +956,12 @@ orig_rtr_lsa(struct area *area)
> lsa_hdr.seq_num = htonl(INIT_SEQ_NUM);
> lsa_hdr.len = htons(buf->wpos);
> lsa_hdr.ls_chksum = 0; /* updated later */
> - memcpy(ibuf_seek(buf, 0, sizeof(lsa_hdr)), &lsa_hdr, siz
[s...@spacehopper.org: ospf6d fib reload [Re: bgpd fix for possible crash in SE]]
This hasn't blown up yet... any interest?
- Forwarded message from Stuart Henderson -
From: Stuart Henderson
Date: Fri, 26 May 2023 14:40:45 +0100
To: tech@openbsd.org
Subject: ospf6d fib reload [Re: bgpd fix for possible crash in SE]
Mail-Followup-To: tech@openbsd.org
On 2023/05/26 13:52, Stuart Henderson wrote:
> I think my main issues come around LS_REFRESH_TIME intervals, when
> there's loads of churn and "ospf6d: ospf engine" can be busy for
> minutes at a time (not always, but very often). Don't know if that rings
> any bells for anyone... (I am now reminded that RTM_DESYNC isn't handled
> by ospf6d which probably doesn't help matters).
Here's a first attempt at porting the fib reload/desync diffs from
ospfd to ospf6d ... Not sure if it's good yet, but it didn't immediately
crash and burn when I ran "ospf6ctl fib reload", at least.
Index: ospf6ctl/ospf6ctl.8
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.8,v
retrieving revision 1.13
diff -u -p -r1.13 ospf6ctl.8
--- ospf6ctl/ospf6ctl.8 2 Mar 2023 17:09:53 - 1.13
+++ ospf6ctl/ospf6ctl.8 26 May 2023 13:37:55 -
@@ -58,6 +58,9 @@ Remove the learned routes from the FIB.
Decoupling the FIB from an OSPF router may create routing loops and could cause
major routing issues in the complete OSPF cloud.
Only routers with just one link to the OSPF cloud can safely decouple the FIB.
+.It Cm fib reload
+Refetches and relearns the routes in the Forwarding Information Base
+a.k.a. the kernel routing table.
.It Cm log brief
Disable verbose debug logging.
.It Cm log verbose
Index: ospf6ctl/ospf6ctl.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v
retrieving revision 1.53
diff -u -p -r1.53 ospf6ctl.c
--- ospf6ctl/ospf6ctl.c 27 Dec 2022 12:11:39 - 1.53
+++ ospf6ctl/ospf6ctl.c 26 May 2023 13:37:55 -
@@ -225,6 +225,11 @@ main(int argc, char *argv[])
printf("decouple request sent.\n");
done = 1;
break;
+ case FIB_RELOAD:
+ imsg_compose(ibuf, IMSG_CTL_FIB_RELOAD, 0, 0, -1, NULL, 0);
+ printf("reload request sent.\n");
+ done = 1;
+ break;
case LOG_VERBOSE:
verbose = 1;
/* FALLTHROUGH */
@@ -304,6 +309,7 @@ main(int argc, char *argv[])
case FIB:
case FIB_COUPLE:
case FIB_DECOUPLE:
+ case FIB_RELOAD:
case LOG_VERBOSE:
case LOG_BRIEF:
case RELOAD:
Index: ospf6ctl/parser.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.c,v
retrieving revision 1.14
diff -u -p -r1.14 parser.c
--- ospf6ctl/parser.c 26 May 2019 09:27:09 - 1.14
+++ ospf6ctl/parser.c 26 May 2023 13:37:55 -
@@ -73,6 +73,7 @@ static const struct token t_main[] = {
static const struct token t_fib[] = {
{ KEYWORD, "couple", FIB_COUPLE, NULL},
{ KEYWORD, "decouple", FIB_DECOUPLE, NULL},
+ { KEYWORD, "reload", FIB_RELOAD, NULL},
{ ENDTOKEN, "", NONE, NULL}
};
Index: ospf6ctl/parser.h
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.h,v
retrieving revision 1.9
diff -u -p -r1.9 parser.h
--- ospf6ctl/parser.h 26 May 2019 09:27:09 - 1.9
+++ ospf6ctl/parser.h 26 May 2023 13:37:55 -
@@ -29,6 +29,7 @@ enum actions {
FIB,
FIB_COUPLE,
FIB_DECOUPLE,
+ FIB_RELOAD,
LOG_VERBOSE,
LOG_BRIEF,
SHOW,
Index: ospf6d/control.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/control.c,v
retrieving revision 1.31
diff -u -p -r1.31 control.c
--- ospf6d/control.c8 Mar 2023 04:43:14 - 1.31
+++ ospf6d/control.c26 May 2023 13:37:55 -
@@ -279,6 +279,7 @@ control_dispatch_imsg(int fd, short even
case IMSG_CTL_FIB_DECOUPLE:
ospfe_fib_update(imsg.hdr.type);
/* FALLTHROUGH */
+ case IMSG_CTL_FIB_RELOAD:
case IMSG_CTL_RELOAD:
c->iev.ibuf.pid = imsg.hdr.pid;
ospfe_imsg_compose_parent(imsg.hdr.type, 0, NULL, 0);
Index: ospf6d/kroute.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.67
diff -u -p -r1.67 kroute.c
--- ospf6d/kroute.c 8 Mar 2023 04:43:14 - 1.67
+++ ospf6d/kroute.c 26 May 2023 13:37:55 -
@@ -45,16 +45,22 @@ struct {
u_int32_t rtseq;
pid_t pid;
int f
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote:
> This diff updates ospfd to use the new ibuf API.
>
> It mainly removes the use of ibuf_seek() and replaces these calls with
> ibuf_set().
>
> Regress still passes with this diff in.
Here the same diff for ospf6d.
--
:wq Claudio
Index: database.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v
retrieving revision 1.22
diff -u -p -r1.22 database.c
--- database.c 8 Mar 2023 04:43:14 - 1.22
+++ database.c 16 Jun 2023 10:27:04 -
@@ -51,7 +51,7 @@ send_db_description(struct nbr *nbr)
goto fail;
/* reserve space for database description header */
- if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL)
+ if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1)
goto fail;
switch (nbr->state) {
@@ -134,8 +134,9 @@ send_db_description(struct nbr *nbr)
dd_hdr.bits = bits;
dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num);
- memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)),
- &dd_hdr, sizeof(dd_hdr));
+ if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr,
+ sizeof(dd_hdr)) == -1)
+ goto fail;
/* calculate checksum */
if (upd_ospf_hdr(buf, nbr->iface))
Index: lsupdate.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v
retrieving revision 1.22
diff -u -p -r1.22 lsupdate.c
--- lsupdate.c 8 Mar 2023 04:43:14 - 1.22
+++ lsupdate.c 16 Jun 2023 10:27:15 -
@@ -177,7 +177,7 @@ prepare_ls_update(struct iface *iface, i
goto fail;
/* reserve space for number of lsa field */
- if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL)
+ if (ibuf_add_zero(buf, sizeof(u_int32_t)) == -1)
goto fail;
return (buf);
@@ -208,8 +208,10 @@ add_ls_update(struct ibuf *buf, struct i
age = ntohs(age);
if ((age += older + iface->transmit_delay) >= MAX_AGE)
age = MAX_AGE;
- age = htons(age);
- memcpy(ibuf_seek(buf, ageoff, sizeof(age)), &age, sizeof(age));
+ if (ibuf_set_n16(buf, ageoff, age) == -1) {
+ log_warn("add_ls_update");
+ return (0);
+ }
return (1);
}
@@ -218,9 +220,8 @@ int
send_ls_update(struct ibuf *buf, struct iface *iface, struct in6_addr addr,
u_int32_t nlsa)
{
- nlsa = htonl(nlsa);
- memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(nlsa)),
- &nlsa, sizeof(nlsa));
+ if (ibuf_set_n32(buf, sizeof(struct ospf_hdr), nlsa) == -1)
+ goto fail;
/* calculate checksum */
if (upd_ospf_hdr(buf, iface))
goto fail;
Index: ospfe.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/ospfe.c,v
retrieving revision 1.68
diff -u -p -r1.68 ospfe.c
--- ospfe.c 8 Mar 2023 04:43:14 - 1.68
+++ ospfe.c 20 Jun 2023 16:25:52 -
@@ -780,11 +780,11 @@ orig_rtr_lsa(struct area *area)
fatal("orig_rtr_lsa");
/* reserve space for LSA header and LSA Router header */
- if (ibuf_reserve(buf, sizeof(lsa_hdr)) == NULL)
- fatal("orig_rtr_lsa: ibuf_reserve failed");
+ if (ibuf_add_zero(buf, sizeof(lsa_hdr)) == -1)
+ fatal("orig_rtr_lsa: ibuf_add_zero failed");
- if (ibuf_reserve(buf, sizeof(lsa_rtr)) == NULL)
- fatal("orig_rtr_lsa: ibuf_reserve failed");
+ if (ibuf_add_zero(buf, sizeof(lsa_rtr)) == -1)
+ fatal("orig_rtr_lsa: ibuf_add_zero failed");
/* links */
LIST_FOREACH(iface, &area->iface_list, entry) {
@@ -944,8 +944,8 @@ orig_rtr_lsa(struct area *area)
LSA_24_SETLO(lsa_rtr.opts, area_ospf_options(area));
LSA_24_SETHI(lsa_rtr.opts, flags);
lsa_rtr.opts = htonl(lsa_rtr.opts);
- memcpy(ibuf_seek(buf, sizeof(lsa_hdr), sizeof(lsa_rtr)),
- &lsa_rtr, sizeof(lsa_rtr));
+ if (ibuf_set(buf, sizeof(lsa_hdr), &lsa_rtr, sizeof(lsa_rtr)) == -1)
+ fatal("orig_rtr_lsa: ibuf_set failed");
/* LSA header */
lsa_hdr.age = htons(DEFAULT_AGE);
@@ -956,11 +956,12 @@ orig_rtr_lsa(struct area *area)
lsa_hdr.seq_num = htonl(INIT_SEQ_NUM);
lsa_hdr.len = htons(buf->wpos);
lsa_hdr.ls_chksum = 0; /* updated later */
- memcpy(ibuf_seek(buf, 0, sizeof(lsa_hdr)), &lsa_hdr, sizeof(lsa_hdr));
+ if (ibuf_set(buf, 0, &lsa_hdr, sizeof(lsa_hdr)) == -1)
+ fatal("orig_rtr_lsa: ibuf_set failed");
- chksum = htons(iso_cksum(buf->buf, buf->wpos, LS_CKSUM_OFFSET));
- memcpy(ibuf_seek(buf, LS_CKSUM_OFFSET, sizeof(chksum)),
- &chksum, sizeof(chksum));
+ chksum = iso_cksum(buf->buf, buf->wpos, LS_CKSUM_OFFSET);
+ if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, c
Re: more relayd cleanup
On Tue, Jun 20, 2023 at 03:35:11PM +0200, Theo Buehler wrote: > On Tue, Jun 20, 2023 at 02:17:06PM +0200, Claudio Jeker wrote: > > Ok, this went overboard. I just wanted to clean up a bit more in > > check_tcp.c but noticed check_send_expect and CHECK_BINSEND_EXPECT. > > > > This code is not very consitent in the differnt ways the strings are > > encoded. Especially check_send_expect() is a bit of a mess because of > > that. > > > > While there I noticed string2binary() and decided to write it in simpler > > way (copying code over from rpki-client). > > > > All in all I think this diff is improving the situation a little bit. > > Write "I will not modify Reyk's daemons more than I need to" 100 times. > > Then it's ok. > I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk's daemons more than I need to I will not modify Reyk
Re: open_memstream cleanup
On Tue, Jun 20, 2023 at 05:49:46PM +0200, Claudio Jeker wrote: > In open_memstream() the code does a bzero() of the new memory even though > recallocarray() used which does this already. > > In open_wmemstream() the code does the same but is still using > reallocarray(). So adjust that code to be like open_memstream(). ok > > -- > :wq Claudio > > Index: open_memstream.c > === > RCS file: /cvs/src/lib/libc/stdio/open_memstream.c,v > retrieving revision 1.8 > diff -u -p -r1.8 open_memstream.c > --- open_memstream.c 2 May 2019 08:30:10 - 1.8 > +++ open_memstream.c 8 Jun 2023 12:21:50 - > @@ -53,7 +53,6 @@ memstream_write(void *v, const char *b, > p = recallocarray(st->string, st->size, sz, 1); > if (!p) > return (-1); > - bzero(p + st->size, sz - st->size); > *st->pbuf = st->string = p; > st->size = sz; > } > Index: open_wmemstream.c > === > RCS file: /cvs/src/lib/libc/stdio/open_wmemstream.c,v > retrieving revision 1.8 > diff -u -p -r1.8 open_wmemstream.c > --- open_wmemstream.c 12 Sep 2015 16:23:14 - 1.8 > +++ open_wmemstream.c 15 Jun 2023 14:54:42 - > @@ -52,10 +52,9 @@ wmemstream_write(void *v, const char *b, > > if (sz < end + 1) > sz = end + 1; > - p = reallocarray(st->string, sz, sizeof(wchar_t)); > + p = recallocarray(st->string, st->size, sz, sizeof(wchar_t)); > if (!p) > return (-1); > - bzero(p + st->size, (sz - st->size) * sizeof(wchar_t)); > *st->pbuf = st->string = p; > st->size = sz; > } >
Re: uvm_meter: improve periodic execution logic for uvm_loadav()
On Tue, Jun 20, 2023 at 10:25:02AM -0500, Scott Cheloha wrote:
> On Tue, Jun 20, 2023 at 11:47:10AM +0200, Claudio Jeker wrote:
> > On Mon, Jun 19, 2023 at 04:45:03PM -0500, Scott Cheloha wrote:
> >
> > [...]
> >
> > > Index: uvm/uvm_meter.c
> > > ===
> > > RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> > > retrieving revision 1.42
> > > diff -u -p -r1.42 uvm_meter.c
> > > --- uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
> > > +++ uvm/uvm_meter.c 19 Jun 2023 21:35:22 -
> > > @@ -42,6 +42,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -65,6 +66,9 @@
> > > int maxslp = MAXSLP; /* patchable ... */
> > > struct loadavg averunnable;
> > >
> > > +/* Update load averages every five seconds. */
> > > +#define UVM_METER_INTVL 5
> > > +
> > > /*
> > > * constants for averages over 1, 5, and 15 minutes when sampling at
> > > * 5 second intervals.
> > > @@ -78,17 +82,29 @@ static fixpt_t cexp[3] = {
> > >
> > >
> > > static void uvm_loadav(struct loadavg *);
> > > +void uvm_meter(void *);
> > > void uvm_total(struct vmtotal *);
> > > void uvmexp_read(struct uvmexp *);
> > >
> > > +void
> > > +uvm_meter_start(void)
> > > +{
> > > + static struct timeout to = TIMEOUT_INITIALIZER(uvm_meter, &to);
> > > +
> > > + uvm_meter(&to);
> > > +}
> > > +
> > > /*
> > > * uvm_meter: calculate load average and wake up the swapper (if needed)
> > > */
> > > void
> > > -uvm_meter(void)
> > > +uvm_meter(void *arg)
> > > {
> > > - if ((gettime() % 5) == 0)
> > > - uvm_loadav(&averunnable);
> > > + struct timeout *to = arg;
> > > +
> > > + timeout_add_sec(to, UVM_METER_INTVL);
> > > +
> > > + uvm_loadav(&averunnable);
> > > if (proc0.p_slptime > (maxslp / 2))
> > > wakeup(&proc0);
> > > }
> >
> > Why add uvm_meter_start() using a static global value and then pass that
> > value around. This code could just be:
> >
> > struct timeout uvm_meter_to = TIMEOUT_INITIALIZER(uvm_meter, NULL);
> >
> > void
> > uvm_meter(void *arg)
> > {
> > timeout_add_sec(&uvm_meter_to, UVM_METER_INTVL);
> > uvm_loadav(&averunnable);
> > }
> >
> > and then just call uvm_meter() once in scheduler_start().
> > I don't understand why all extra this indirection is needed it does not
> > make the code better..
> >
> > Apart from that and the fact that the proc0 wakeup and go I'm OK with this
> > diff.
>
> I like that better. I'll commit the attached tomorrow unless I hear
> otherwise.
>
> Index: share/man/man9/uvm_init.9
> ===
> RCS file: /cvs/src/share/man/man9/uvm_init.9,v
> retrieving revision 1.5
> diff -u -p -r1.5 uvm_init.9
> --- share/man/man9/uvm_init.9 21 May 2023 05:11:38 - 1.5
> +++ share/man/man9/uvm_init.9 20 Jun 2023 15:20:59 -
> @@ -168,7 +168,7 @@ argument is ignored.
> .Ft void
> .Fn uvm_kernacc "caddr_t addr" "size_t len" "int rw"
> .Ft void
> -.Fn uvm_meter
> +.Fn uvm_meter "void *"
> .Ft int
> .Fn uvm_sysctl "int *name" "u_int namelen" "void *oldp" "size_t *oldlenp"
> "void *newp " "size_t newlen" "struct proc *p"
> .Ft int
> @@ -212,7 +212,7 @@ access, in the kernel address space.
> .Pp
> The
> .Fn uvm_meter
> -function calculates the load average and wakes up the swapper if necessary.
> +function periodically recomputes the load average.
> .Pp
> The
> .Fn uvm_sysctl
> Index: sys/kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 sched_bsd.c
> --- sys/kern/sched_bsd.c 4 Feb 2023 19:33:03 - 1.74
> +++ sys/kern/sched_bsd.c 20 Jun 2023 15:20:59 -
> @@ -234,7 +234,6 @@ schedcpu(void *arg)
> }
> SCHED_UNLOCK(s);
> }
> - uvm_meter();
> wakeup(&lbolt);
> timeout_add_sec(to, 1);
> }
> @@ -669,6 +668,7 @@ scheduler_start(void)
>
> rrticks_init = hz / 10;
> schedcpu(&schedcpu_to);
> + uvm_meter(NULL);
>
> #ifndef SMALL_KERNEL
> if (perfpolicy == PERFPOL_AUTO)
> Index: sys/uvm/uvm_meter.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 uvm_meter.c
> --- sys/uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
> +++ sys/uvm/uvm_meter.c 20 Jun 2023 15:20:59 -
> @@ -65,6 +65,9 @@
> int maxslp = MAXSLP; /* patchable ... */
> struct loadavg averunnable;
>
> +#define UVM_METER_INTVL 5
> +struct timeout uvm_meter_to = TIMEOUT_INITIALIZER(uvm_meter, NULL);
> +
> /*
> * constants for averages over 1, 5, and 15 minutes when sampling at
> * 5 second intervals.
> @@ -82,15 +85,13 @@ void uvm_total(struct vmtotal *);
> void uvmexp_read(struct uvmexp *);
>
> /*
> - * uvm_meter: ca
open_memstream cleanup
In open_memstream() the code does a bzero() of the new memory even though recallocarray() used which does this already. In open_wmemstream() the code does the same but is still using reallocarray(). So adjust that code to be like open_memstream(). -- :wq Claudio Index: open_memstream.c === RCS file: /cvs/src/lib/libc/stdio/open_memstream.c,v retrieving revision 1.8 diff -u -p -r1.8 open_memstream.c --- open_memstream.c2 May 2019 08:30:10 - 1.8 +++ open_memstream.c8 Jun 2023 12:21:50 - @@ -53,7 +53,6 @@ memstream_write(void *v, const char *b, p = recallocarray(st->string, st->size, sz, 1); if (!p) return (-1); - bzero(p + st->size, sz - st->size); *st->pbuf = st->string = p; st->size = sz; } Index: open_wmemstream.c === RCS file: /cvs/src/lib/libc/stdio/open_wmemstream.c,v retrieving revision 1.8 diff -u -p -r1.8 open_wmemstream.c --- open_wmemstream.c 12 Sep 2015 16:23:14 - 1.8 +++ open_wmemstream.c 15 Jun 2023 14:54:42 - @@ -52,10 +52,9 @@ wmemstream_write(void *v, const char *b, if (sz < end + 1) sz = end + 1; - p = reallocarray(st->string, sz, sizeof(wchar_t)); + p = recallocarray(st->string, st->size, sz, sizeof(wchar_t)); if (!p) return (-1); - bzero(p + st->size, (sz - st->size) * sizeof(wchar_t)); *st->pbuf = st->string = p; st->size = sz; }
Re: uvm_meter: improve periodic execution logic for uvm_loadav()
On Tue, Jun 20, 2023 at 11:47:10AM +0200, Claudio Jeker wrote:
> On Mon, Jun 19, 2023 at 04:45:03PM -0500, Scott Cheloha wrote:
>
> [...]
>
> > Index: uvm/uvm_meter.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> > retrieving revision 1.42
> > diff -u -p -r1.42 uvm_meter.c
> > --- uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
> > +++ uvm/uvm_meter.c 19 Jun 2023 21:35:22 -
> > @@ -42,6 +42,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -65,6 +66,9 @@
> > int maxslp = MAXSLP; /* patchable ... */
> > struct loadavg averunnable;
> >
> > +/* Update load averages every five seconds. */
> > +#define UVM_METER_INTVL5
> > +
> > /*
> > * constants for averages over 1, 5, and 15 minutes when sampling at
> > * 5 second intervals.
> > @@ -78,17 +82,29 @@ static fixpt_t cexp[3] = {
> >
> >
> > static void uvm_loadav(struct loadavg *);
> > +void uvm_meter(void *);
> > void uvm_total(struct vmtotal *);
> > void uvmexp_read(struct uvmexp *);
> >
> > +void
> > +uvm_meter_start(void)
> > +{
> > + static struct timeout to = TIMEOUT_INITIALIZER(uvm_meter, &to);
> > +
> > + uvm_meter(&to);
> > +}
> > +
> > /*
> > * uvm_meter: calculate load average and wake up the swapper (if needed)
> > */
> > void
> > -uvm_meter(void)
> > +uvm_meter(void *arg)
> > {
> > - if ((gettime() % 5) == 0)
> > - uvm_loadav(&averunnable);
> > + struct timeout *to = arg;
> > +
> > + timeout_add_sec(to, UVM_METER_INTVL);
> > +
> > + uvm_loadav(&averunnable);
> > if (proc0.p_slptime > (maxslp / 2))
> > wakeup(&proc0);
> > }
>
> Why add uvm_meter_start() using a static global value and then pass that
> value around. This code could just be:
>
> struct timeout uvm_meter_to = TIMEOUT_INITIALIZER(uvm_meter, NULL);
>
> void
> uvm_meter(void *arg)
> {
> timeout_add_sec(&uvm_meter_to, UVM_METER_INTVL);
> uvm_loadav(&averunnable);
> }
>
> and then just call uvm_meter() once in scheduler_start().
> I don't understand why all extra this indirection is needed it does not
> make the code better..
>
> Apart from that and the fact that the proc0 wakeup and go I'm OK with this
> diff.
I like that better. I'll commit the attached tomorrow unless I hear
otherwise.
Index: share/man/man9/uvm_init.9
===
RCS file: /cvs/src/share/man/man9/uvm_init.9,v
retrieving revision 1.5
diff -u -p -r1.5 uvm_init.9
--- share/man/man9/uvm_init.9 21 May 2023 05:11:38 - 1.5
+++ share/man/man9/uvm_init.9 20 Jun 2023 15:20:59 -
@@ -168,7 +168,7 @@ argument is ignored.
.Ft void
.Fn uvm_kernacc "caddr_t addr" "size_t len" "int rw"
.Ft void
-.Fn uvm_meter
+.Fn uvm_meter "void *"
.Ft int
.Fn uvm_sysctl "int *name" "u_int namelen" "void *oldp" "size_t *oldlenp"
"void *newp " "size_t newlen" "struct proc *p"
.Ft int
@@ -212,7 +212,7 @@ access, in the kernel address space.
.Pp
The
.Fn uvm_meter
-function calculates the load average and wakes up the swapper if necessary.
+function periodically recomputes the load average.
.Pp
The
.Fn uvm_sysctl
Index: sys/kern/sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.74
diff -u -p -r1.74 sched_bsd.c
--- sys/kern/sched_bsd.c4 Feb 2023 19:33:03 - 1.74
+++ sys/kern/sched_bsd.c20 Jun 2023 15:20:59 -
@@ -234,7 +234,6 @@ schedcpu(void *arg)
}
SCHED_UNLOCK(s);
}
- uvm_meter();
wakeup(&lbolt);
timeout_add_sec(to, 1);
}
@@ -669,6 +668,7 @@ scheduler_start(void)
rrticks_init = hz / 10;
schedcpu(&schedcpu_to);
+ uvm_meter(NULL);
#ifndef SMALL_KERNEL
if (perfpolicy == PERFPOL_AUTO)
Index: sys/uvm/uvm_meter.c
===
RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
retrieving revision 1.42
diff -u -p -r1.42 uvm_meter.c
--- sys/uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
+++ sys/uvm/uvm_meter.c 20 Jun 2023 15:20:59 -
@@ -65,6 +65,9 @@
int maxslp = MAXSLP; /* patchable ... */
struct loadavg averunnable;
+#define UVM_METER_INTVL5
+struct timeout uvm_meter_to = TIMEOUT_INITIALIZER(uvm_meter, NULL);
+
/*
* constants for averages over 1, 5, and 15 minutes when sampling at
* 5 second intervals.
@@ -82,15 +85,13 @@ void uvm_total(struct vmtotal *);
void uvmexp_read(struct uvmexp *);
/*
- * uvm_meter: calculate load average and wake up the swapper (if needed)
+ * uvm_meter: recompute load averages
*/
void
-uvm_meter(void)
+uvm_meter(void *unused)
{
- if ((gettime() % 5) == 0)
- uvm_loadav(&averunnable);
- if (proc0.p_slptime > (maxslp / 2))
- wakeup(&proc0);
+ timeout_add_sec(&uvm_meter_to,
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 03:46:23PM +0200, Theo Buehler wrote:
> On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote:
> > This diff updates ospfd to use the new ibuf API.
> >
> > It mainly removes the use of ibuf_seek() and replaces these calls with
> > ibuf_set().
> >
> > Regress still passes with this diff in.
>
> There's a function vs fatal mismatch in orig_rtr_lsa
>
> > + if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1)
> > + fatal("orig_rtr_lsa: ibuf_set failed");
>
> not sure if that's deliberate. Similarly in orig_net_lsa.
It is not deliberate. I will fix them before commit.
> ok
--
:wq Claudio
Re: profclock, gmonclock: new callbacks for profil(2)/GPROF statclock() code
Claudio Jeker wrote: > On Mon, Jun 19, 2023 at 06:41:14PM -0500, Scott Cheloha wrote: > > > On Jun 19, 2023, at 18:07, Theo de Raadt wrote: > > > > > > Make sure to STOP all kernel profiling before attempting to > > >suspend or hibernate your machine. Otherwise I expect it > > >will hang. > > > > > > It is completely acceptable if it produces wrong results, but it must > > > not hang the system. > > > > The hang is present in -current, with or > > without this patch. > > > > I am working to figure it out. > > I don't think the suspend or hibernate code has any code to disable > kernel profiling. This is bad since these code paths are extremly > sensitive and try not to do side-effects. > > So in the suspend/hibernate code path we should disable profiling early > on. It makes no sense to try to run gprof collection in those code paths. Yes, that's right. It will be somewhere in kern/subr_suspend.c Be careful that the "stop profiling" and "restart profiling" are at the correct places. The sleep_state() function has a bunch of unrolling goto's which are not 100% reflexive, so be careful.
Re: rpki-client use new ibuf API
On Tue, Jun 20, 2023 at 03:44:56PM +0200, Claudio Jeker wrote: > Use the ibuf_fd_*() API for file descriptor passing and also ibuf_set() > instead of ibuf_seek(). ok tb
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote:
> This diff updates ospfd to use the new ibuf API.
>
> It mainly removes the use of ibuf_seek() and replaces these calls with
> ibuf_set().
>
> Regress still passes with this diff in.
There's a function vs fatal mismatch in orig_rtr_lsa
> + if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1)
> + fatal("orig_rtr_lsa: ibuf_set failed");
not sure if that's deliberate. Similarly in orig_net_lsa.
ok
rpki-client use new ibuf API
Use the ibuf_fd_*() API for file descriptor passing and also ibuf_set()
instead of ibuf_seek().
--
:wq Claudio
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.76
diff -u -p -r1.76 http.c
--- http.c 12 Jun 2023 15:27:52 - 1.76
+++ http.c 12 Jun 2023 16:03:16 -
@@ -2150,7 +2150,7 @@ proc_http(char *bind_addr, int fd)
io_read_str(b, &mod);
/* queue up new requests */
- http_req_new(id, uri, mod, 0, b->fd);
+ http_req_new(id, uri, mod, 0, ibuf_fd_get(b));
ibuf_free(b);
}
}
Index: io.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
retrieving revision 1.22
diff -u -p -r1.22 io.c
--- io.c14 Dec 2022 15:19:16 - 1.22
+++ io.c16 Jun 2023 14:37:42 -
@@ -41,7 +41,7 @@ io_new_buffer(void)
if ((b = ibuf_dynamic(64, INT32_MAX)) == NULL)
err(1, NULL);
- ibuf_reserve(b, sizeof(size_t));/* can not fail */
+ ibuf_add_zero(b, sizeof(size_t)); /* can not fail */
return b;
}
@@ -88,7 +88,7 @@ io_close_buffer(struct msgbuf *msgbuf, s
size_t len;
len = ibuf_size(b) - sizeof(len);
- memcpy(ibuf_seek(b, 0, sizeof(len)), &len, sizeof(len));
+ ibuf_set(b, 0, &len, sizeof(len));
ibuf_close(msgbuf, b);
}
@@ -280,7 +280,7 @@ io_buf_recvfd(int fd, struct ibuf **ib)
for (i = 0; i < j; i++) {
f = ((int *)CMSG_DATA(cmsg))[i];
if (i == 0)
- b->fd = f;
+ ibuf_fd_set(b, f);
else
close(f);
}
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.241
diff -u -p -r1.241 main.c
--- main.c 30 May 2023 16:02:28 - 1.241
+++ main.c 12 Jun 2023 07:09:58 -
@@ -341,7 +341,7 @@ http_fetch(unsigned int id, const char *
io_str_buffer(b, uri);
io_str_buffer(b, last_mod);
/* pass file as fd */
- b->fd = fd;
+ ibuf_fd_set(b, fd);
io_close_buffer(&httpq, b);
}
@@ -362,7 +362,7 @@ rrdp_http_fetch(unsigned int id, const c
b = io_new_buffer();
io_simple_buffer(b, &type, sizeof(type));
io_simple_buffer(b, &id, sizeof(id));
- b->fd = pi[0];
+ ibuf_fd_set(b, pi[0]);
io_close_buffer(&rrdpq, b);
http_fetch(id, uri, last_mod, pi[1]);
Index: rrdp.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rrdp.c,v
retrieving revision 1.30
diff -u -p -r1.30 rrdp.c
--- rrdp.c 3 May 2023 07:51:08 - 1.30
+++ rrdp.c 16 Jun 2023 15:06:58 -
@@ -431,20 +431,20 @@ rrdp_input_handler(int fd)
io_read_str(b, &session_id);
io_read_buf(b, &serial, sizeof(serial));
io_read_str(b, &last_mod);
- if (b->fd != -1)
+ if (ibuf_fd_avail(b))
errx(1, "received unexpected fd");
rrdp_new(id, local, notify, session_id, serial, last_mod);
break;
case RRDP_HTTP_INI:
- if (b->fd == -1)
- errx(1, "expected fd not received");
s = rrdp_get(id);
if (s == NULL)
errx(1, "http ini, rrdp session %u does not exist", id);
if (s->state != RRDP_STATE_WAIT)
errx(1, "%s: bad internal state", s->local);
- s->infd = b->fd;
+ s->infd = ibuf_fd_get(b);
+ if (s->infd == -1)
+ errx(1, "expected fd not received");
s->state = RRDP_STATE_PARSE;
if (s->aborted) {
rrdp_abort_req(s);
@@ -454,7 +454,7 @@ rrdp_input_handler(int fd)
case RRDP_HTTP_FIN:
io_read_buf(b, &res, sizeof(res));
io_read_str(b, &last_mod);
- if (b->fd != -1)
+ if (ibuf_fd_avail(b))
errx(1, "received unexpected fd");
s = rrdp_get(id);
@@ -472,7 +472,7 @@ rrdp_input_handler(int fd)
s = rrdp_get(id);
if (s == NULL)
errx(1, "file, rrdp session %u does not exist", id);;
- if (b->fd != -1)
+ if (ibuf_fd_avail(b))
errx(1, "received unexpected fd");
io_read_buf(b, &ok, sizeof(ok));
Re: more relayd cleanup
On Tue, Jun 20, 2023 at 02:17:06PM +0200, Claudio Jeker wrote: > Ok, this went overboard. I just wanted to clean up a bit more in > check_tcp.c but noticed check_send_expect and CHECK_BINSEND_EXPECT. > > This code is not very consitent in the differnt ways the strings are > encoded. Especially check_send_expect() is a bit of a mess because of > that. > > While there I noticed string2binary() and decided to write it in simpler > way (copying code over from rpki-client). > > All in all I think this diff is improving the situation a little bit. Write "I will not modify Reyk's daemons more than I need to" 100 times. Then it's ok.
ospfd use new ibuf functions
This diff updates ospfd to use the new ibuf API.
It mainly removes the use of ibuf_seek() and replaces these calls with
ibuf_set().
Regress still passes with this diff in.
--
:wq Claudio
Index: auth.c
===
RCS file: /cvs/src/usr.sbin/ospfd/auth.c,v
retrieving revision 1.20
diff -u -p -r1.20 auth.c
--- auth.c 5 May 2015 01:26:37 - 1.20
+++ auth.c 16 Jun 2023 10:25:48 -
@@ -141,35 +141,44 @@ auth_gen(struct ibuf *buf, struct iface
{
MD5_CTX hash;
u_int8_t digest[MD5_DIGEST_LENGTH];
- struct ospf_hdr *ospf_hdr;
+ struct crypt crypt;
struct auth_md *md;
-
- if ((ospf_hdr = ibuf_seek(buf, 0, sizeof(*ospf_hdr))) == NULL)
- fatalx("auth_gen: buf_seek failed");
+ u_int16_tchksum;
/* update length */
if (ibuf_size(buf) > USHRT_MAX)
fatalx("auth_gen: resulting ospf packet too big");
- ospf_hdr->len = htons(ibuf_size(buf));
- /* clear auth_key field */
- bzero(ospf_hdr->auth_key.simple, sizeof(ospf_hdr->auth_key.simple));
+ if (ibuf_set_n16(buf, offsetof(struct ospf_hdr, len),
+ ibuf_size(buf)) == -1)
+ fatalx("auth_gen: ibuf_set_n16 failed");
switch (iface->auth_type) {
case AUTH_NONE:
- ospf_hdr->chksum = in_cksum(buf->buf, ibuf_size(buf));
+ chksum = in_cksum(buf->buf, ibuf_size(buf));
+ if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum),
+ &chksum, sizeof(chksum)) == -1)
+ fatalx("auth_gen: ibuf_set failed");
break;
case AUTH_SIMPLE:
- ospf_hdr->chksum = in_cksum(buf->buf, ibuf_size(buf));
+ chksum = in_cksum(buf->buf, ibuf_size(buf));
+ if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum),
+ &chksum, sizeof(chksum)) == -1)
+ fatalx("auth_gen: ibuf_set failed");
- strncpy(ospf_hdr->auth_key.simple, iface->auth_key,
- sizeof(ospf_hdr->auth_key.simple));
+ if (ibuf_set(buf, offsetof(struct ospf_hdr, auth_key),
+ iface->auth_key, strlen(iface->auth_key)) == -1)
+ fatalx("auth_gen: ibuf_set failed");
break;
case AUTH_CRYPT:
- ospf_hdr->chksum = 0;
- ospf_hdr->auth_key.crypt.keyid = iface->auth_keyid;
- ospf_hdr->auth_key.crypt.seq_num = htonl(iface->crypt_seq_num);
- ospf_hdr->auth_key.crypt.len = MD5_DIGEST_LENGTH;
+ bzero(&crypt, sizeof(crypt));
+ crypt.keyid = iface->auth_keyid;
+ crypt.seq_num = htonl(iface->crypt_seq_num);
+ crypt.len = MD5_DIGEST_LENGTH;
iface->crypt_seq_num++;
+
+ if (ibuf_set(buf, offsetof(struct ospf_hdr, auth_key),
+ &crypt, sizeof(crypt)) == -1)
+ fatalx("auth_gen: ibuf_set failed");
/* insert plaintext key */
if ((md = md_list_find(&iface->auth_md_list,
Index: database.c
===
RCS file: /cvs/src/usr.sbin/ospfd/database.c,v
retrieving revision 1.36
diff -u -p -r1.36 database.c
--- database.c 8 Mar 2023 04:43:14 - 1.36
+++ database.c 16 Jun 2023 10:26:00 -
@@ -53,7 +53,7 @@ send_db_description(struct nbr *nbr)
goto fail;
/* reserve space for database description header */
- if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL)
+ if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1)
goto fail;
switch (nbr->state) {
@@ -140,8 +140,9 @@ send_db_description(struct nbr *nbr)
dd_hdr.bits = bits;
dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num);
- memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)),
- &dd_hdr, sizeof(dd_hdr));
+ if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr,
+ sizeof(dd_hdr)) == -1)
+ goto fail;
/* update authentication and calculate checksum */
if (auth_gen(buf, nbr->iface))
Index: lsupdate.c
===
RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v
retrieving revision 1.51
diff -u -p -r1.51 lsupdate.c
--- lsupdate.c 8 Mar 2023 04:43:14 - 1.51
+++ lsupdate.c 16 Jun 2023 10:26:17 -
@@ -158,7 +158,7 @@ prepare_ls_update(struct iface *iface)
goto fail;
/* reserve space for number of lsa field */
- if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL)
+ if (ibuf_add_zero(buf, sizeof(u_int32_t)) == -1)
goto fail;
return (buf);
@@ -194,8 +194,10 @@ add_ls_update(struct ibuf *buf, struct i
age = ntohs(age);
if ((age += older + iface->transm
more relayd cleanup
Ok, this went overboard. I just wanted to clean up a bit more in
check_tcp.c but noticed check_send_expect and CHECK_BINSEND_EXPECT.
This code is not very consitent in the differnt ways the strings are
encoded. Especially check_send_expect() is a bit of a mess because of
that.
While there I noticed string2binary() and decided to write it in simpler
way (copying code over from rpki-client).
All in all I think this diff is improving the situation a little bit.
--
:wq Claudio
Index: check_tcp.c
===
RCS file: /cvs/src/usr.sbin/relayd/check_tcp.c,v
retrieving revision 1.59
diff -u -p -r1.59 check_tcp.c
--- check_tcp.c 20 Jun 2023 09:54:57 - 1.59
+++ check_tcp.c 20 Jun 2023 10:55:12 -
@@ -183,10 +183,6 @@ tcp_host_up(struct ctl_tcp_event *cte)
return;
}
- if (cte->table->sendbuf != NULL && cte->table->sendbinbuf == NULL) {
- cte->req = cte->table->sendbuf;
- } else if (cte->table->sendbinbuf != NULL)
- cte->req = cte->table->sendbinbuf->buf;
if (cte->table->sendbuf != NULL || cte->table->sendbinbuf != NULL) {
event_again(&cte->ev, cte->s, EV_TIMEOUT|EV_WRITE, tcp_send_req,
&cte->tv_start, &cte->table->conf.timeout, cte);
@@ -203,6 +199,7 @@ void
tcp_send_req(int s, short event, void *arg)
{
struct ctl_tcp_event*cte = arg;
+ char*req;
int bs;
int len;
@@ -214,14 +211,17 @@ tcp_send_req(int s, short event, void *a
if (cte->table->sendbinbuf != NULL) {
len = ibuf_size(cte->table->sendbinbuf);
+ req = ibuf_data(cte->table->sendbinbuf);
log_debug("%s: table %s sending binary", __func__,
cte->table->conf.name);
print_hex(cte->table->sendbinbuf->buf, 0, len);
- } else
- len = strlen(cte->req);
+ } else {
+ len = strlen(cte->table->sendbuf);
+ req = cte->table->sendbuf;
+ }
do {
- bs = write(s, cte->req, len);
+ bs = write(s, req, len);
if (bs == -1) {
if (errno == EAGAIN || errno == EINTR)
goto retry;
@@ -230,7 +230,7 @@ tcp_send_req(int s, short event, void *a
hce_notify_done(cte->host, HCE_TCP_WRITE_FAIL);
return;
}
- cte->req += bs;
+ req += bs;
len -= bs;
} while (len > 0);
@@ -302,20 +302,22 @@ check_send_expect(struct ctl_tcp_event *
u_char *b;
if (cte->table->conf.check == CHECK_BINSEND_EXPECT) {
+ size_t exlen;
+
+ exlen = strlen(cte->table->conf.exbuf) / 2;
log_debug("%s: table %s expecting binary",
__func__, cte->table->conf.name);
- print_hex(cte->table->conf.exbinbuf, 0,
- strlen(cte->table->conf.exbuf) / 2);
+ print_hex(cte->table->conf.exbinbuf, 0, exlen);
- if (memcmp(cte->table->conf.exbinbuf, cte->buf->buf,
- strlen(cte->table->conf.exbuf) / 2) == 0) {
+ if (ibuf_size(cte->buf) >= exlen && memcmp(ibuf_data(cte->buf),
+ cte->table->conf.exbinbuf, exlen) == 0) {
cte->host->he = HCE_SEND_EXPECT_OK;
cte->host->up = HOST_UP;
return (0);
- } else {
+ } else if (ibuf_size(cte->buf) >= exlen) {
log_debug("%s: table %s received mismatching binary",
__func__, cte->table->conf.name);
- print_hex(cte->buf->buf, 0, ibuf_size(cte->buf));
+ print_hex(ibuf_data(cte->buf), 0, ibuf_size(cte->buf));
}
} else if (cte->table->conf.check == CHECK_SEND_EXPECT) {
/*
@@ -353,7 +355,7 @@ check_http_code(struct ctl_tcp_event *ct
if (ibuf_add_zero(cte->buf, 1) == -1)
fatal("out of memory");
- head = cte->buf->buf;
+ head = ibuf_data(cte->buf);
host = cte->host;
host->he = HCE_HTTP_CODE_ERROR;
host->code = 0;
@@ -404,7 +406,7 @@ check_http_digest(struct ctl_tcp_event *
if (ibuf_add_zero(cte->buf, 1) == -1)
fatal("out of memory");
- head = cte->buf->buf;
+ head = ibuf_data(cte->buf);
host = cte->host;
host->he = HCE_HTTP_DIGEST_ERROR;
Index: relayd.h
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
retrieving revision 1.269
diff -u -p -r1.269 relayd.h
--- relayd.h31 Aug 2022 16:17:18 - 1.269
+++ relayd.h20 Jun 2023 10:47:36 -
@@ -176,7 +176,6 @@ st
Re: uvm_meter: improve periodic execution logic for uvm_loadav()
On Mon, Jun 19, 2023 at 04:45:03PM -0500, Scott Cheloha wrote:
> On Mon, Jun 19, 2023 at 10:22:56AM +0200, Claudio Jeker wrote:
> > On Sun, Jun 18, 2023 at 12:43:18PM -0500, Scott Cheloha wrote:
> > > On Sun, Jun 18, 2023 at 12:36:07PM -0500, Scott Cheloha wrote:
> > > > On Sun, Jun 18, 2023 at 07:32:56PM +0200, Mark Kettenis wrote:
> > > > > > Date: Sun, 18 Jun 2023 12:27:17 -0500
> > > > > > From: Scott Cheloha
> > > > > >
> > > > > > The intent here is to update the load averages every five seconds.
> > > > > > However:
> > > > > >
> > > > > > 1. Measuring elapsed time with the UTC clock is unreliable because
> > > > > > of
> > > > > >settimeofday(2).
> > > > > >
> > > > > > 2. "Call uvm_loadav() no more than once every five seconds", is not
> > > > > > equivalent to "call uvm_loadav() if the current second is equal
> > > > > > to zero, modulo five".
> > > > > >
> > > > > >Not hard to imagine edge cases where timeouts are delayed and
> > > > > >the load averages are not updated.
> > > > > >
> > > > > > So, (1) use the monotonic clock, and (2) keep the next uvm_loadav()
> > > > > > call time in a static value.
> > > > > >
> > > > > > ok?
> > > > >
> > > > > I really don't see why the calculatin of something vague like the load
> > > > > average warrants complicating the code like this.
> > > >
> > > > Aren't load averages used to make decisions about thread placement in
> > > > the scheduler?
> > > >
> > > > Regardless, the code is still wrong. At minimum you should use
> > > > getuptime(9).
> > >
> > > Maybe I misunderstood. Are you suggesting this?
> > >
> > >
> > > now = getuptime();
> > > if (now >= next_uvm_loadav) {
> > > next_uvm_loadav = now + 5;
> > > uvm_loadav(...);
> > > }
> > >
> > > The patch I posted preserves the current behavior. It is equivalent
> > > to:
> > >
> > > while (next_uvm_loadav <= now)
> > > next_uvm_loadav += 5;
> > >
> > > Changing it to (now + 5) changes the behavior.
> >
> > To be honest, I think the uvm_meter should be called via timeout(9) and
> > not be called via the current path using schedcpu().
> > At some point schedcpu() may be removed and then we need to fix this
> > proper anyway.
>
> See attached.
>
> This changes the wakeup interval for the proc0 (the swapper) on
> patched kernels where maxslp is patched (highly unusual).
>
> On default kernels, maxslp is 20, which divides evenly into 5, so
> the patch does not change the proc0 wakeup interval.
>
> Another approach would be to run the uvm_meter() timeout every second
> and track the uvm_loadav() deadline in a static variable.
>
> > Running the lbolt wakeup inside schedcpu() has the same issue.
>
> Eliminating lbolt has been a goal of mine for several years. Tried
> to remove as many users as possible a few years ago. The tricky cases
> are in the sys/kern tty code.
>
> --
>
> Index: kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 sched_bsd.c
> --- kern/sched_bsd.c 4 Feb 2023 19:33:03 - 1.74
> +++ kern/sched_bsd.c 19 Jun 2023 21:35:22 -
> @@ -234,7 +234,6 @@ schedcpu(void *arg)
> }
> SCHED_UNLOCK(s);
> }
> - uvm_meter();
> wakeup(&lbolt);
> timeout_add_sec(to, 1);
> }
> @@ -669,6 +668,7 @@ scheduler_start(void)
>
> rrticks_init = hz / 10;
> schedcpu(&schedcpu_to);
> + uvm_meter_start();
>
> #ifndef SMALL_KERNEL
> if (perfpolicy == PERFPOL_AUTO)
> Index: uvm/uvm_meter.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_meter.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 uvm_meter.c
> --- uvm/uvm_meter.c 28 Dec 2020 14:01:23 - 1.42
> +++ uvm/uvm_meter.c 19 Jun 2023 21:35:22 -
> @@ -42,6 +42,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -65,6 +66,9 @@
> int maxslp = MAXSLP; /* patchable ... */
> struct loadavg averunnable;
>
> +/* Update load averages every five seconds. */
> +#define UVM_METER_INTVL 5
> +
> /*
> * constants for averages over 1, 5, and 15 minutes when sampling at
> * 5 second intervals.
> @@ -78,17 +82,29 @@ static fixpt_t cexp[3] = {
>
>
> static void uvm_loadav(struct loadavg *);
> +void uvm_meter(void *);
> void uvm_total(struct vmtotal *);
> void uvmexp_read(struct uvmexp *);
>
> +void
> +uvm_meter_start(void)
> +{
> + static struct timeout to = TIMEOUT_INITIALIZER(uvm_meter, &to);
> +
> + uvm_meter(&to);
> +}
> +
> /*
> * uvm_meter: calculate load average and wake up the swapper (if needed)
> */
> void
> -uvm_meter(void)
> +uvm_meter(void *arg)
> {
> - if ((gettime() % 5) == 0)
> - uvm_loadav(&averunnable);
> + struct timeout *to = arg;
> +
> + timeout_add_sec(to, UVM_METER_INTVL);
> +
> + u
Re: convert relayd to use new ibuf function
On Tue, Jun 20, 2023 at 11:34:22AM +0200, Claudio Jeker wrote: > Instead of ibuf_reserve() just use ibuf_add_zero(buf, 1) to add a > NUL byte to the buffer. ok tb
convert relayd to use new ibuf function
Instead of ibuf_reserve() just use ibuf_add_zero(buf, 1) to add a
NUL byte to the buffer.
There is more needed in here but lets start small.
--
:wq Claudio
Index: check_tcp.c
===
RCS file: /cvs/src/usr.sbin/relayd/check_tcp.c,v
retrieving revision 1.58
diff -u -p -r1.58 check_tcp.c
--- check_tcp.c 18 Sep 2021 19:44:46 - 1.58
+++ check_tcp.c 10 Jun 2023 07:36:17 -
@@ -344,17 +344,14 @@ check_http_code(struct ctl_tcp_event *ct
char*head;
char scode[4];
const char *estr;
- u_char *b;
int code;
struct host *host;
/*
* ensure string is nul-terminated.
*/
- b = ibuf_reserve(cte->buf, 1);
- if (b == NULL)
+ if (ibuf_add_zero(cte->buf, 1) == -1)
fatal("out of memory");
- *b = '\0';
head = cte->buf->buf;
host = cte->host;
@@ -398,17 +395,14 @@ int
check_http_digest(struct ctl_tcp_event *cte)
{
char*head;
- u_char *b;
char digest[SHA1_DIGEST_STRING_LENGTH];
struct host *host;
/*
* ensure string is nul-terminated.
*/
- b = ibuf_reserve(cte->buf, 1);
- if (b == NULL)
+ if (ibuf_add_zero(cte->buf, 1) == -1)
fatal("out of memory");
- *b = '\0';
head = cte->buf->buf;
host = cte->host;
tr(1): don't use indirect variable, supplement comment
Hello,
I suggest this simplification in the "tr [-Ccs] string1 string2"
section of main. It makes it easier to understand what's
happening in the -C case.
Luka
diff --git a/usr.bin/tr/tr.c b/usr.bin/tr/tr.c
index ab78898a986..a47fb409f34 100644
--- a/usr.bin/tr/tr.c
+++ b/usr.bin/tr/tr.c
@@ -167,8 +167,9 @@ main(int argc, char *argv[])
/*
* tr [-Ccs] string1 string2
* Replace all characters (or complemented characters) in string1 with
- * the character in the same position in string2. If the -s option is
- * specified, squeeze all the characters in string2.
+ * the character in the same position in string2. If string2 runs out
+ * of characters (or with -Cc option), use the last one specified. If
+ * the -s option is specified, squeeze all the characters in string2.
*/
if (argc != 2)
usage();
@@ -183,23 +184,21 @@ main(int argc, char *argv[])
if (!next(&s2))
errx(1, "empty string2");
- /* If string2 runs out of characters, use the last one specified. */
- ch = s2.lastch;
if (sflag)
while (next(&s1)) {
- translate[s1.lastch] = ch = s2.lastch;
- squeeze[ch] = 1;
+ translate[s1.lastch] = s2.lastch;
+ squeeze[s2.lastch] = 1;
(void)next(&s2);
}
else
while (next(&s1)) {
- translate[s1.lastch] = ch = s2.lastch;
+ translate[s1.lastch] = s2.lastch;
(void)next(&s2);
}
if (cflag)
for (cnt = 0, p = translate; cnt < NCHARS; ++p, ++cnt)
- *p = *p == OOBCH ? ch : cnt;
+ *p = *p == OOBCH ? s2.lastch : cnt;
if (sflag)
for (lastch = OOBCH; (ch = getchar()) != EOF;) {
Re: uvm_meter: improve periodic execution logic for uvm_loadav()
On Tue, Jun 20, 2023 at 08:36:58AM +0200, Claudio Jeker wrote:
> On Mon, Jun 19, 2023 at 04:45:03PM -0500, Scott Cheloha wrote:
> > On Mon, Jun 19, 2023 at 10:22:56AM +0200, Claudio Jeker wrote:
> > > On Sun, Jun 18, 2023 at 12:43:18PM -0500, Scott Cheloha wrote:
> > > > On Sun, Jun 18, 2023 at 12:36:07PM -0500, Scott Cheloha wrote:
> > > > > On Sun, Jun 18, 2023 at 07:32:56PM +0200, Mark Kettenis wrote:
> > > > > > > Date: Sun, 18 Jun 2023 12:27:17 -0500
> > > > > > > From: Scott Cheloha
> > > > > > >
> > > > > > > The intent here is to update the load averages every five seconds.
> > > > > > > However:
> > > > > > >
> > > > > > > 1. Measuring elapsed time with the UTC clock is unreliable
> > > > > > > because of
> > > > > > >settimeofday(2).
> > > > > > >
> > > > > > > 2. "Call uvm_loadav() no more than once every five seconds", is
> > > > > > > not
> > > > > > > equivalent to "call uvm_loadav() if the current second is
> > > > > > > equal
> > > > > > > to zero, modulo five".
> > > > > > >
> > > > > > >Not hard to imagine edge cases where timeouts are delayed and
> > > > > > >the load averages are not updated.
> > > > > > >
> > > > > > > So, (1) use the monotonic clock, and (2) keep the next
> > > > > > > uvm_loadav()
> > > > > > > call time in a static value.
> > > > > > >
> > > > > > > ok?
> > > > > >
> > > > > > I really don't see why the calculatin of something vague like the
> > > > > > load
> > > > > > average warrants complicating the code like this.
> > > > >
> > > > > Aren't load averages used to make decisions about thread placement in
> > > > > the scheduler?
> > > > >
> > > > > Regardless, the code is still wrong. At minimum you should use
> > > > > getuptime(9).
> > > >
> > > > Maybe I misunderstood. Are you suggesting this?
> > > >
> > > >
> > > > now = getuptime();
> > > > if (now >= next_uvm_loadav) {
> > > > next_uvm_loadav = now + 5;
> > > > uvm_loadav(...);
> > > > }
> > > >
> > > > The patch I posted preserves the current behavior. It is equivalent
> > > > to:
> > > >
> > > > while (next_uvm_loadav <= now)
> > > > next_uvm_loadav += 5;
> > > >
> > > > Changing it to (now + 5) changes the behavior.
> > >
> > > To be honest, I think the uvm_meter should be called via timeout(9) and
> > > not be called via the current path using schedcpu().
> > > At some point schedcpu() may be removed and then we need to fix this
> > > proper anyway.
> >
> > See attached.
> >
> > This changes the wakeup interval for the proc0 (the swapper) on
> > patched kernels where maxslp is patched (highly unusual).
> >
> > On default kernels, maxslp is 20, which divides evenly into 5, so
> > the patch does not change the proc0 wakeup interval.
> >
> > Another approach would be to run the uvm_meter() timeout every second
> > and track the uvm_loadav() deadline in a static variable.
>
> I think the proper fix is to just remove this wakeup.
> Since proc0 does nothing anymore.
>
> This wakeup triggers this other bit of code:
>
> /*
> * proc0: nothing to do, back to sleep
> */
> while (1)
> tsleep_nsec(&proc0, PVM, "scheduler", INFSLP);
> /* NOTREACHED */
>
> at the end of the main(). So we wakeup a while (1) loop.
>
> I need to look more into this. I wonder if there is some silly side-effect
> that this code tickles. All of this just smells like old cruft.
So in Rev. 1.45 of uvm_glue() miod@ killed the swapper and with that
turned uvm_scheduler() into a dumb while(1) tsleep() loop.
Since then this periodic update has not done anything apart from burning
CPU time. Later on that loop was moved by blambert to init_main.c but
nobody ever looked at the wakeup() side of this tsleep_nsec().
So I guess we can burn this part of the code.
--
:wq Claudio
