systat(1): vmstat: compute rates with CLOCK_UPTIME
Hi, systat(1)'s vmstat view displays rates for things like interrupts. Strangely, it uses CPU time to compute these rates, not real time. This is potentially misleading, particularly on an MP system. If I have 4 cores running on a HZ=100 kernel I expect ~400 clock interrupts per second. But systat(1) tells me I have 100 because we have 4 seconds worth of CPU time for every second of real time that elapses. I don't like it. I want to see how many interrupts there really were. This patch changes the vmstat view to use CLOCK_UPTIME to measure elapsed time and uses that when computing rates. The "Big Bar" is still drawn using CPU time, but for everything else I think you would want a rate based on the elapsed real time. Using CPU time isn't intuitive. We want CLOCK_UPTIME, not CLOCK_MONOTONIC, because we aren't interested in what the machine was doing when it was suspended. I have not changed dinfo() to keep the patch simple, but we should be using CLOCK_UPTIME there, too. Thoughts? Index: vmstat.c === RCS file: /cvs/src/usr.bin/systat/vmstat.c,v retrieving revision 1.91 diff -u -p -r1.91 vmstat.c --- vmstat.c28 Jun 2019 13:35:04 - 1.91 +++ vmstat.c16 Sep 2020 03:56:14 - @@ -320,7 +320,7 @@ labelkre(void) #define PUTRATE(fld, l, c, w) \ do { \ Y(fld); \ - putint((int)((float)s.fld/etime + 0.5), l, c, w); \ + putint((int)((float)s.fld / eruntime + 0.5), l, c, w); \ } while (0) #define MAXFAIL 5 @@ -330,12 +330,18 @@ staticchar cpuorder[] = { CP_INTR, CP_S void showkre(void) { + static struct timespec prev; + struct timespec elapsed, now; float f1, f2; int psiz; u_int64_t inttotal, intcnt; int i, l, c; static int failcnt = 0, first_run = 0; - double etime; + double ecputime, eruntime; + + clock_gettime(CLOCK_UPTIME, ); + timespecsub(, , ); + prev = now; if (state == TIME) { if (!first_run) { @@ -343,12 +349,13 @@ showkre(void) return; } } - etime = 0; + eruntime = elapsed.tv_sec + elapsed.tv_nsec / 10.0; + ecputime = 0; for (i = 0; i < CPUSTATES; i++) { X(cpustats.cs_time); - etime += s.cpustats.cs_time[i]; + ecputime += s.cpustats.cs_time[i]; } - if (etime < 5.0) { /* < 5 ticks - ignore this trash */ + if (ecputime < 5.0) { /* < 5 ticks - ignore this trash */ if (failcnt++ >= MAXFAIL) { error("The alternate system clock has died!"); failcnt = 0; @@ -356,12 +363,12 @@ showkre(void) return; } failcnt = 0; - etime /= hertz; + ecputime /= hertz; inttotal = 0; for (i = 0; i < nintr; i++) { t = intcnt = s.intrcnt[i]; s.intrcnt[i] -= s1.intrcnt[i]; - intcnt = (u_int64_t)((float)s.intrcnt[i]/etime + 0.5); + intcnt = (u_int64_t)((float)s.intrcnt[i] / eruntime + 0.5); inttotal += intcnt; if (intrloc[i] != 0) putuint64(intcnt, intrloc[i], INTSCOL, 8);
Re: acme-client: relax certificate parsing
ok Florian Obser(flor...@openbsd.org) on 2020.09.14 17:12:01 +0200: > Relax parsing of pem files a bit. Apparently there are CAs that use > \r\n line endings. > From Bartosz Kuzma as part of a larger diff. > > OK? > > diff --git certproc.c certproc.c > index 7fde96e970e..975e12afaaa 100644 > --- certproc.c > +++ certproc.c > @@ -28,7 +28,8 @@ > > #include "extern.h" > > -#define MARKER "-END CERTIFICATE-\n" > +#define BEGIN_MARKER "-BEGIN CERTIFICATE-" > +#define END_MARKER "-END CERTIFICATE-" > > int > certproc(int netsock, int filesock) > @@ -81,19 +82,25 @@ certproc(int netsock, int filesock) > if ((csr = readbuf(netsock, COMM_CSR, )) == NULL) > goto out; > > - if (csrsz < strlen(MARKER)) { > + if (csrsz < strlen(END_MARKER)) { > warnx("invalid cert"); > goto out; > } > > - chaincp = strstr(csr, MARKER); > + chaincp = strstr(csr, END_MARKER); > > if (chaincp == NULL) { > warnx("invalid cert"); > goto out; > } > > - chaincp += strlen(MARKER); > + chaincp += strlen(END_MARKER); > + > + if ((chaincp = strstr(chaincp, BEGIN_MARKER)) == NULL) { > + warnx("invalid certificate chain"); > + goto out; > + } > + > if ((chain = strdup(chaincp)) == NULL) { > warn("strdup"); > goto out; > > > -- > I'm not entirely sure you are real. >
Re: acme-client(1) and Buypass Go SSL
ok! Florian Obser(flor...@openbsd.org) on 2020.09.14 17:15:37 +0200: > > This fell through the cracks back in April. > > We need to be able to provide contact information to use the > buypass.com acme api. > > OK? > > diff --git etc/examples/acme-client.conf etc/examples/acme-client.conf > index 32ecd8e8655..40d231725ac 100644 > --- etc/examples/acme-client.conf > +++ etc/examples/acme-client.conf > @@ -11,6 +11,18 @@ authority letsencrypt-staging { > account key "/etc/acme/letsencrypt-staging-privkey.pem" > } > > +authority buypass { > +api url "https://api.buypass.com/acme/directory; > +account key "/etc/acme/buypass-privkey.pem" > +contact "mailto:m...@example.com; > +} > + > +authority buypass-test { > +api url "https://api.test4.buypass.no/acme/directory; > +account key "/etc/acme/buypass-test-privkey.pem" > +contact "mailto:m...@example.com; > +} > + > domain example.com { > alternative names { secure.example.com } > domain key "/etc/ssl/private/example.com.key" > diff --git usr.sbin/acme-client/acme-client.conf.5 > usr.sbin/acme-client/acme-client.conf.5 > index 08a47a76ab7..41994d13676 100644 > --- usr.sbin/acme-client/acme-client.conf.5 > +++ usr.sbin/acme-client/acme-client.conf.5 > @@ -98,6 +98,11 @@ It defaults to > Specify the > .Ar url > under which the ACME API is reachable. > +.It Ic contact Ar contact > +Optional > +.Ar contact > +URLs that the authority can use to contact the client for issues related to > +this account. > .El > .Sh DOMAINS > The certificates to be obtained through ACME. > diff --git usr.sbin/acme-client/extern.h usr.sbin/acme-client/extern.h > index 364425b0500..ee341e0950f 100644 > --- usr.sbin/acme-client/extern.h > +++ usr.sbin/acme-client/extern.h > @@ -263,7 +263,7 @@ char *json_getstr(struct jsmnn *, const char > *); > > char *json_fmt_newcert(const char *); > char *json_fmt_chkacc(void); > -char *json_fmt_newacc(void); > +char *json_fmt_newacc(const char *); > char *json_fmt_neworder(const char *const *, size_t); > char *json_fmt_protected_rsa(const char *, > const char *, const char *, const char *); > diff --git usr.sbin/acme-client/json.c usr.sbin/acme-client/json.c > index a6762eeb258..9201f8d2fc3 100644 > --- usr.sbin/acme-client/json.c > +++ usr.sbin/acme-client/json.c > @@ -618,14 +618,24 @@ json_fmt_chkacc(void) > * Format the "newAccount" resource request. > */ > char * > -json_fmt_newacc(void) > +json_fmt_newacc(const char *contact) > { > int c; > - char*p; > + char*p, *cnt = NULL; > + > + if (contact != NULL) { > + c = asprintf(, "\"contact\": [ \"%s\" ], ", contact); > + if (c == -1) { > + warn("asprintf"); > + return NULL; > + } > + } > > c = asprintf(, "{" > + "%s" > "\"termsOfServiceAgreed\": true" > - "}"); > + "}", cnt == NULL ? "" : cnt); > + free(cnt); > if (c == -1) { > warn("asprintf"); > p = NULL; > diff --git usr.sbin/acme-client/netproc.c usr.sbin/acme-client/netproc.c > index 05e36897c38..4490450003e 100644 > --- usr.sbin/acme-client/netproc.c > +++ usr.sbin/acme-client/netproc.c > @@ -369,14 +369,14 @@ sreq(struct conn *c, const char *addr, int kid, const > char *req, char **loc) > * Returns non-zero on success. > */ > static int > -donewacc(struct conn *c, const struct capaths *p) > +donewacc(struct conn *c, const struct capaths *p, const char *contact) > { > struct jsmnn*j = NULL; > int rc = 0; > char*req, *detail, *error = NULL; > long lc; > > - if ((req = json_fmt_newacc()) == NULL) > + if ((req = json_fmt_newacc(contact)) == NULL) > warnx("json_fmt_newacc"); > else if ((lc = sreq(c, p->newaccount, 0, req, >kid)) < 0) > warnx("%s: bad comm", p->newaccount); > @@ -410,7 +410,7 @@ donewacc(struct conn *c, const struct capaths *p) > * Returns non-zero on success. > */ > static int > -dochkacc(struct conn *c, const struct capaths *p) > +dochkacc(struct conn *c, const struct capaths *p, const char *contact) > { > int rc = 0; > char*req; > @@ -425,7 +425,7 @@ dochkacc(struct conn *c, const struct capaths *p) > else if (c->buf.buf == NULL || c->buf.sz == 0) > warnx("%s: empty response", p->newaccount); > else if (lc == 400) > - rc = donewacc(c, p); > + rc = donewacc(c, p, contact); > else > rc = 1; > > @@ -755,7 +755,7 @@ netproc(int kfd, int afd, int Cfd, int cfd, int dfd, int > rfd, > c.newnonce = paths.newnonce; > > /* Check if our account already exists or create it. */ > - if (!dochkacc(, )) > + if (!dochkacc(, , authority->contact)) >
[patch] Don't require a working efifb when probing for EFI/GPT
I successfully installed OpenBSD under xhyve, although there were a few issues, mostly xhyve's fault [1]. One which seems to be an issue with OpenBSD itself rather than xhyve is that with a headless machine, the installer always uses MBR mode. I selected GPT and ignored the warning "An EFI/GPT disk may not boot. Proceed?"; it boots fine, of course, since it's using the TianoCore EDK II firmware. (And MBR mode would not boot, unless I manually created and populated an EFI System Partition.) I tracked this down to install.md checking for efifb (it's expecting something like 'efifb0 at mainbus0: 1024x768, 32bpp' in the dmesg): if dmesg | grep -q 'efifb0 at mainbus0'; then MDEFI=y fi With a headless VM, however, the dmesg line is: efifb at mainbus0 not configured I can't find a better way to detect EFI via dmesg or sysctl hw, so a simple patch to install.md will have to suffice: diff --git distrib/amd64/common/install.md distrib/amd64/common/install.md index d4fc8418a97..0c9db939463 100644 --- distrib/amd64/common/install.md +++ distrib/amd64/common/install.md @@ -35,7 +35,7 @@ MDXAPERTURE=2 MDXDM=y NCPU=$(sysctl -n hw.ncpufound) -if dmesg | grep -q 'efifb0 at mainbus0'; then +if dmesg | egrep -q 'efifb0? at mainbus0'; then MDEFI=y fi (diff also attached) This probably also applies to physical hardware where efifb is broken for whatever reason. I've verified that this still behaves correctly with a working efifb0. No idea if this also applies to any other platforms beside amd64, but a quick glance at i386, arm64, and armv7 seems to say no. -Andrew [1] https://github.com/machyve/xhyve/issues/179#issuecomment-561903641 install-efi-gpt.diff Description: Binary data
PATCH: Add vmmpci device for passthrough PCI
This adds a placeholder vmmpci device that will be used for VMD passthrough PCI. Normally the device will fail to attach unless the PCI domain:bus.dev.func has been registered with vmmpci_add. When the device is registered, it will detach any existing PCI device and reload as vmmpci. It also attaches an interrupt handler and keeps a running counter of triggered interrupts. VMD will use the counter to issue commands through to the guest. diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC index 2c49f91a1..a69c72c26 100644 --- a/sys/arch/amd64/conf/GENERIC +++ b/sys/arch/amd64/conf/GENERIC @@ -108,6 +109,7 @@ ksmn* at pci? # AMD K17 temperature sensor amas* at pci? disable # AMD memory configuration pchtemp* at pci? # Intel C610 termperature sensor ccp* at pci? # AMD Cryptographic Co-processor +vmmpci* at pci?# VMM Placeholder # National Semiconductor LM7[89] and compatible hardware monitors lm0at isa? port 0x290 diff --git a/sys/arch/amd64/conf/files.amd64 b/sys/arch/amd64/conf/files.amd64 index 7a5d40bf4..74c7fe5a9 100644 --- a/sys/arch/amd64/conf/files.amd64 +++ b/sys/arch/amd64/conf/files.amd64 @@ -132,6 +132,10 @@ device pchb: pcibus, agpbus attach pchb at pci file arch/amd64/pci/pchb.c pchb +device vmmpci +attach vmmpci at pci +file arch/amd64/pci/vmmpci.c vmmpci + # AMAS AMD memory address switch device amas attach amas at pci diff --git a/sys/arch/amd64/include/vmmpci.h b/sys/arch/amd64/include/vmmpci.h new file mode 100644 index 0..e012194df --- /dev/null +++ b/sys/arch/amd64/include/vmmpci.h @@ -0,0 +1,24 @@ +/* $OpenBSD: vmmvar.h,v 1.70 2020/04/08 07:39:48 pd Exp $ */ +/* + * Copyright (c) 2020 Jordan Hargrave + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ +#ifndef _VMMPCI_H_ +#define _VMMPCI_H_ + +int vmmpci_find(int, pcitag_t); +int vmmpci_add(int, pcitag_t, int); +int vmmpci_pending(int, pcitag_t, uint32_t *); + +#endif diff --git a/sys/arch/amd64/pci/vmmpci.c b/sys/arch/amd64/pci/vmmpci.c new file mode 100644 index 0..a99efb502 --- /dev/null +++ b/sys/arch/amd64/pci/vmmpci.c @@ -0,0 +1,186 @@ +/* $OpenBSD: pcib.c,v 1.6 2013/05/30 16:15:01 deraadt Exp $*/ +/* $NetBSD: pcib.c,v 1.6 1997/06/06 23:29:16 thorpej Exp $ */ + +/*- + * Copyright (c) 1996 Jordan Hargrave + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include +#include +#include + +#include + +#include +#include + +#include + +#include +#include + +struct vmmpci_softc { + struct device sc_dev; + void*sc_ih; + + int sc_domain; + pci_chipset_tag_t sc_pc; + pcitag_tsc_tag; + + uint32_tpending;// pending interrupt count +}; + +#define VP_VALID 0x8000 + +/* Keep track of registered devices */ +struct vmmpci_dev { + int vp_flags; + int vp_domain; + pcitag_tvp_tag; +}; + +intvmmpci_match(struct device *, void *, void *); +void vmmpci_attach(struct device *, struct device *, void *); +void vmmpci_callback(struct device *); +intvmmpci_print(void *, const char *); +intvmmpci_intr(void *arg); + +struct cfattach vmmpci_ca = { + sizeof(struct vmmpci_softc), vmmpci_match, vmmpci_attach +}; + +struct cfdriver vmmpci_cd = { + NULL, "vmmpci", DV_DULL +}; + +#define MAXVMMPCI 4 + +struct vmmpci_dev
Re: acme-client: improve account creation error message
On 2020-09-15 04:25, Florian Obser wrote: > On Mon, Sep 14, 2020 at 04:26:20PM -0500, Rafael Possamai wrote: >>> please dont drop the all buffer , or keep it with -vv ? >>> example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf); >>> >>> i don't want to ktrace it to see why the new certbot version is not working >> >> Yeah, I think it's good to have the choice to inspect the vomit, maybe you >> stumble upon some useful nuggets for troubleshooting an obscure issue. >> >> I believe the JSON output is valid, just not pretty. Maybe we could add >> syntax highlighting? (just kidding) >> > > I don't understand what you people are going on about, I didn't touch > the -v output. You can still roll around in json. > > It's not about if it's valid json or not, it's untrusted input that we > spit on your console when used with -v. Simple fix: ensure that all control characters are represented as Unicode escape sequences. I believe that the JSON spec requires this anyway.
Re: curproc vs MP vs locking
On Tue, Sep 15, 2020 at 04:38:45PM +0200, Mark Kettenis wrote: > > Date: Tue, 15 Sep 2020 12:34:07 +0200 > > From: Martin Pieuchot > > > > Many functions in the kernel take a "struct proc *" as argument. When > > reviewing diffs or reading the signature of such functions it is not > > clear if this pointer can be any thread or if it is, like in many cases, > > pointing to `curproc'. > > > > This distinction matters when it comes to reading/writing members of > > this "struct proc" and that's why a growing number of functions start > > with the following idiom: > > > > KASSERT(p == curproc); > > > > This is verbose and redundant, so I suggested to always use `curproc' > > and stop passing a "struct proc *" as argument when a function isn't > > meant to modify any thread. claudio@ raised a concern of performance > > claiming that `curproc' isn't always cheap. Is it still true? Does > > the KASSERT()s make us pay the cost anyhow? > > Right, because our kernel has DIAGNOSTIC enabled. > > > If that's the case can we adopt a convention to help review functions > > that take a "struct proc *" but only mean `curproc'? What about naming > > this parameter `curp' instead of `p'? > > That'll result in quite a bit of churn. I'd really like to avoid > doing that. I think the conclusion between the people here at k2k20 is that we should drop the argument from the function and use curproc in the functions (assigning curproc = p early on). This way there will be no way of using the wrong proc. -- :wq Claudio
Re: cap_mkdb: remove igetnext prototype for the function does not exist
On Tue, 15 Sep 2020 11:35:13 +0800, Kevin Lo wrote: > ok? OK millert@ - todd
Re: curproc vs MP vs locking
> Date: Tue, 15 Sep 2020 12:34:07 +0200 > From: Martin Pieuchot > > Many functions in the kernel take a "struct proc *" as argument. When > reviewing diffs or reading the signature of such functions it is not > clear if this pointer can be any thread or if it is, like in many cases, > pointing to `curproc'. > > This distinction matters when it comes to reading/writing members of > this "struct proc" and that's why a growing number of functions start > with the following idiom: > > KASSERT(p == curproc); > > This is verbose and redundant, so I suggested to always use `curproc' > and stop passing a "struct proc *" as argument when a function isn't > meant to modify any thread. claudio@ raised a concern of performance > claiming that `curproc' isn't always cheap. Is it still true? Does > the KASSERT()s make us pay the cost anyhow? Right, because our kernel has DIAGNOSTIC enabled. > If that's the case can we adopt a convention to help review functions > that take a "struct proc *" but only mean `curproc'? What about naming > this parameter `curp' instead of `p'? That'll result in quite a bit of churn. I'd really like to avoid doing that.
Re: sigabort(), p_sigmask & p_siglist
On Tue, Sep 15, 2020 at 12:52:40PM +0200, Martin Pieuchot wrote: > Diff below introduces an helper for sending an uncatchable SIGABRT and > annotate that `p_siglist' and `p_sigmask' are updated using atomic > operations. > > As a result setsigvec() becomes local to kern/kern_sig.c. > > Note that other places in the kernel use sigexit(p, SIGABRT) for the > same purpose and aren't converted by this change. > > Ik? OK claudio@ but I would split the proc.h into its own commit since it is unrelated to the sigabort() introduction. > Index: kern/kern_pledge.c > === > RCS file: /cvs/src/sys/kern/kern_pledge.c,v > retrieving revision 1.263 > diff -u -p -r1.263 kern_pledge.c > --- kern/kern_pledge.c17 Jul 2020 16:28:19 - 1.263 > +++ kern/kern_pledge.c15 Sep 2020 08:30:19 - > @@ -529,7 +529,6 @@ pledge_fail(struct proc *p, int error, u > { > const char *codes = ""; > int i; > - struct sigaction sa; > > /* Print first matching pledge */ > for (i = 0; code && pledgenames[i].bits != 0; i++) > @@ -550,11 +549,7 @@ pledge_fail(struct proc *p, int error, u > p->p_p->ps_acflag |= APLEDGE; > > /* Send uncatchable SIGABRT for coredump */ > - memset(, 0, sizeof sa); > - sa.sa_handler = SIG_DFL; > - setsigvec(p, SIGABRT, ); > - atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); > - psignal(p, SIGABRT); > + sigabort(p); > > p->p_p->ps_pledge = 0; /* Disable all PLEDGE_ flags */ > KERNEL_UNLOCK(); > Index: kern/kern_proc.c > === > RCS file: /cvs/src/sys/kern/kern_proc.c,v > retrieving revision 1.86 > diff -u -p -r1.86 kern_proc.c > --- kern/kern_proc.c 30 Jan 2020 08:51:27 - 1.86 > +++ kern/kern_proc.c 15 Sep 2020 08:52:57 - > @@ -494,7 +494,6 @@ void > db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t count, char *modif) > { > struct process *pr; > - struct sigaction sa; > struct proc *p; > > pr = prfind(addr); > @@ -506,11 +505,7 @@ db_kill_cmd(db_expr_t addr, int have_add > p = TAILQ_FIRST(>ps_threads); > > /* Send uncatchable SIGABRT for coredump */ > - memset(, 0, sizeof sa); > - sa.sa_handler = SIG_DFL; > - setsigvec(p, SIGABRT, ); > - atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); > - psignal(p, SIGABRT); > + sigabort(p); > } > > void > Index: kern/kern_sig.c > === > RCS file: /cvs/src/sys/kern/kern_sig.c,v > retrieving revision 1.262 > diff -u -p -r1.262 kern_sig.c > --- kern/kern_sig.c 13 Sep 2020 13:33:37 - 1.262 > +++ kern/kern_sig.c 15 Sep 2020 08:33:04 - > @@ -122,6 +122,8 @@ const int sigprop[NSIG + 1] = { > #define stopsigmask (sigmask(SIGSTOP) | sigmask(SIGTSTP) | \ > sigmask(SIGTTIN) | sigmask(SIGTTOU)) > > +void setsigvec(struct proc *, int, struct sigaction *); > + > void proc_stop(struct proc *p, int); > void proc_stop_sweep(void *); > void *proc_stop_si; > @@ -1483,6 +1493,21 @@ sigexit(struct proc *p, int signum) > } > exit1(p, 0, signum, EXIT_NORMAL); > /* NOTREACHED */ > +} > + > +/* > + * Send uncatchable SIGABRT for coredump. > + */ > +void > +sigabort(struct proc *p) > +{ > + struct sigaction sa; > + > + memset(, 0, sizeof sa); > + sa.sa_handler = SIG_DFL; > + setsigvec(p, SIGABRT, ); > + atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); > + psignal(p, SIGABRT); > } > > /* > Index: sys/signalvar.h > === > RCS file: /cvs/src/sys/sys/signalvar.h,v > retrieving revision 1.43 > diff -u -p -r1.43 signalvar.h > --- sys/signalvar.h 13 Sep 2020 13:33:37 - 1.43 > +++ sys/signalvar.h 15 Sep 2020 08:34:15 - > @@ -126,9 +126,9 @@ void siginit(struct sigacts *); > void trapsignal(struct proc *p, int sig, u_long code, int type, > union sigval val); > void sigexit(struct proc *, int); > +void sigabort(struct proc *); > int sigismasked(struct proc *, int); > int sigonstack(size_t); > -void setsigvec(struct proc *, int, struct sigaction *); > int killpg1(struct proc *, int, int, int); > > void signal_init(void); > Index: sys/proc.h > === > RCS file: /cvs/src/sys/sys/proc.h,v > retrieving revision 1.299 > diff -u -p -r1.299 proc.h > --- sys/proc.h26 Aug 2020 03:19:09 - 1.299 > +++ sys/proc.h15 Sep 2020 10:15:36 - > @@ -383,14 +383,14 @@ struct proc { > struct kcov_dev *p_kd; /* kcov device handle */ > struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */ > > - int p_siglist; /* Signals arrived but not delivered. */ > + int p_siglist; /*
Re: diff: pfctl: error message for nonexisting rtable
On Tue, Sep 15, 2020 at 12:42:27PM +0900, YASUOKA Masahiko wrote: > It's not clear for me why non-existing rdomain is accepted but > non-existing rtable is rejected. I suppose we can make pf(4) can > handle a packet for the non-existing routing table as if the routing > table is empty. Probably possible, but not without further tests or even changes to pf; I did not want to imply that dynamic `rtable' in pf.conf cannot be done.
sigabort(), p_sigmask & p_siglist
Diff below introduces an helper for sending an uncatchable SIGABRT and annotate that `p_siglist' and `p_sigmask' are updated using atomic operations. As a result setsigvec() becomes local to kern/kern_sig.c. Note that other places in the kernel use sigexit(p, SIGABRT) for the same purpose and aren't converted by this change. Ik? Index: kern/kern_pledge.c === RCS file: /cvs/src/sys/kern/kern_pledge.c,v retrieving revision 1.263 diff -u -p -r1.263 kern_pledge.c --- kern/kern_pledge.c 17 Jul 2020 16:28:19 - 1.263 +++ kern/kern_pledge.c 15 Sep 2020 08:30:19 - @@ -529,7 +529,6 @@ pledge_fail(struct proc *p, int error, u { const char *codes = ""; int i; - struct sigaction sa; /* Print first matching pledge */ for (i = 0; code && pledgenames[i].bits != 0; i++) @@ -550,11 +549,7 @@ pledge_fail(struct proc *p, int error, u p->p_p->ps_acflag |= APLEDGE; /* Send uncatchable SIGABRT for coredump */ - memset(, 0, sizeof sa); - sa.sa_handler = SIG_DFL; - setsigvec(p, SIGABRT, ); - atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); - psignal(p, SIGABRT); + sigabort(p); p->p_p->ps_pledge = 0; /* Disable all PLEDGE_ flags */ KERNEL_UNLOCK(); Index: kern/kern_proc.c === RCS file: /cvs/src/sys/kern/kern_proc.c,v retrieving revision 1.86 diff -u -p -r1.86 kern_proc.c --- kern/kern_proc.c30 Jan 2020 08:51:27 - 1.86 +++ kern/kern_proc.c15 Sep 2020 08:52:57 - @@ -494,7 +494,6 @@ void db_kill_cmd(db_expr_t addr, int have_addr, db_expr_t count, char *modif) { struct process *pr; - struct sigaction sa; struct proc *p; pr = prfind(addr); @@ -506,11 +505,7 @@ db_kill_cmd(db_expr_t addr, int have_add p = TAILQ_FIRST(>ps_threads); /* Send uncatchable SIGABRT for coredump */ - memset(, 0, sizeof sa); - sa.sa_handler = SIG_DFL; - setsigvec(p, SIGABRT, ); - atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); - psignal(p, SIGABRT); + sigabort(p); } void Index: kern/kern_sig.c === RCS file: /cvs/src/sys/kern/kern_sig.c,v retrieving revision 1.262 diff -u -p -r1.262 kern_sig.c --- kern/kern_sig.c 13 Sep 2020 13:33:37 - 1.262 +++ kern/kern_sig.c 15 Sep 2020 08:33:04 - @@ -122,6 +122,8 @@ const int sigprop[NSIG + 1] = { #definestopsigmask (sigmask(SIGSTOP) | sigmask(SIGTSTP) | \ sigmask(SIGTTIN) | sigmask(SIGTTOU)) +void setsigvec(struct proc *, int, struct sigaction *); + void proc_stop(struct proc *p, int); void proc_stop_sweep(void *); void *proc_stop_si; @@ -1483,6 +1493,21 @@ sigexit(struct proc *p, int signum) } exit1(p, 0, signum, EXIT_NORMAL); /* NOTREACHED */ +} + +/* + * Send uncatchable SIGABRT for coredump. + */ +void +sigabort(struct proc *p) +{ + struct sigaction sa; + + memset(, 0, sizeof sa); + sa.sa_handler = SIG_DFL; + setsigvec(p, SIGABRT, ); + atomic_clearbits_int(>p_sigmask, sigmask(SIGABRT)); + psignal(p, SIGABRT); } /* Index: sys/signalvar.h === RCS file: /cvs/src/sys/sys/signalvar.h,v retrieving revision 1.43 diff -u -p -r1.43 signalvar.h --- sys/signalvar.h 13 Sep 2020 13:33:37 - 1.43 +++ sys/signalvar.h 15 Sep 2020 08:34:15 - @@ -126,9 +126,9 @@ voidsiginit(struct sigacts *); void trapsignal(struct proc *p, int sig, u_long code, int type, union sigval val); void sigexit(struct proc *, int); +void sigabort(struct proc *); intsigismasked(struct proc *, int); intsigonstack(size_t); -void setsigvec(struct proc *, int, struct sigaction *); intkillpg1(struct proc *, int, int, int); void signal_init(void); Index: sys/proc.h === RCS file: /cvs/src/sys/sys/proc.h,v retrieving revision 1.299 diff -u -p -r1.299 proc.h --- sys/proc.h 26 Aug 2020 03:19:09 - 1.299 +++ sys/proc.h 15 Sep 2020 10:15:36 - @@ -383,14 +383,14 @@ struct proc { struct kcov_dev *p_kd; /* kcov device handle */ struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */ - int p_siglist; /* Signals arrived but not delivered. */ + int p_siglist; /* [a] Signals arrived & not delivered*/ /* End area that is zeroed on creation. */ #definep_endzero p_startcopy /* The following fields are all copied upon creation in fork. */ #definep_startcopy p_sigmask - sigset_t p_sigmask; /* Current signal mask. */ + sigset_t p_sigmask; /* [a] Current signal
curproc vs MP vs locking
Many functions in the kernel take a "struct proc *" as argument. When reviewing diffs or reading the signature of such functions it is not clear if this pointer can be any thread or if it is, like in many cases, pointing to `curproc'. This distinction matters when it comes to reading/writing members of this "struct proc" and that's why a growing number of functions start with the following idiom: KASSERT(p == curproc); This is verbose and redundant, so I suggested to always use `curproc' and stop passing a "struct proc *" as argument when a function isn't meant to modify any thread. claudio@ raised a concern of performance claiming that `curproc' isn't always cheap. Is it still true? Does the KASSERT()s make us pay the cost anyhow? If that's the case can we adopt a convention to help review functions that take a "struct proc *" but only mean `curproc'? What about naming this parameter `curp' instead of `p'?
Re: trunk: keep interface up on port removal
On Mon, Sep 14, 2020 at 10:57:16AM +0200, Klemens Nanni wrote: > I tested removing a single port from trunk and observed that both > interfaces do end up with the same MAC address, but this happens without > my diff already - I still don't see any behaviour after my diff wrt. MAC > addresses or anything else but the UP flag on port interfaces. I did sloppy testing... tl;dr: MACs have been restored properly before my diff already. I looked at this yet again because removing a single port and destroying the trunk use the same code path, hence it didn't make much sense to me for them to behave differently on second thought. No matter which port is removed, its MAC is reset to what it was before becoming a trunk port. I refrained from inlining supporting shell code/output this time, please test yourself to confirm.
Re: agentx in services
Absolutely. Having it in the file also makes sure that early call to rresvport(3), bindresvport(3), or bindresvport_sa(3) won't allocate it before the daemon is started locally. Martijn van Duren wrote: > I currently don't see any reason for adding agentx over tcp support to > our daemons, but according to RFC2741 section 8.1.1 it should go over > "wel-known port 705". > > Worth adding or just drop it? > > martijn@ > > Index: services > === > RCS file: /cvs/src/etc/services,v > retrieving revision 1.97 > diff -u -p -r1.97 services > --- services 26 Jun 2020 19:51:14 - 1.97 > +++ services 15 Sep 2020 09:43:00 - > @@ -175,6 +175,7 @@ ldaps 636/tcp # LDAP > over SSL > ldaps636/udp > ldp 646/tcp > ldp 646/udp > +agentx 705/tcp > silc 706/tcp # Secure Live Internet > Conferencing > silc 706/udp > kerberos-adm 749/tcp # Kerberos 5 kadmin >
agentx in services
I currently don't see any reason for adding agentx over tcp support to our daemons, but according to RFC2741 section 8.1.1 it should go over "wel-known port 705". Worth adding or just drop it? martijn@ Index: services === RCS file: /cvs/src/etc/services,v retrieving revision 1.97 diff -u -p -r1.97 services --- services26 Jun 2020 19:51:14 - 1.97 +++ services15 Sep 2020 09:43:00 - @@ -175,6 +175,7 @@ ldaps 636/tcp # LDAP over SSL ldaps 636/udp ldp646/tcp ldp646/udp +agentx 705/tcp silc 706/tcp # Secure Live Internet Conferencing silc 706/udp kerberos-adm 749/tcp # Kerberos 5 kadmin
Re: agentx and clang static analyzer
> > Index: subagentx.c > > === > > RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v > > retrieving revision 1.1 > > diff -u -p -r1.1 subagentx.c > > --- subagentx.c 14 Sep 2020 11:30:25 - 1.1 > > +++ subagentx.c 15 Sep 2020 09:05:58 - > > @@ -2929,7 +2929,7 @@ getnext: > > index->sav_idatacomplete = 1; > > break; > > case AGENTX_DATA_TYPE_IPADDRESS: > > - ipaddress = calloc(1, sizeof(ipaddress)); > > + ipaddress = calloc(1, sizeof(*ipaddress)); > > if (ipaddress == NULL) { > > subagentx_log_sag_warn(sag, > > "Failed to bind ipaddress index"); > > Not ok for this, it's probably correct, but there are other instances of this > in this code and so you need to engage brain, not static analyzer, go fix or > don't fix them all in a separate commit. > Could only find one more: Index: subagentx.c === RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v retrieving revision 1.1 diff -u -p -r1.1 subagentx.c --- subagentx.c 14 Sep 2020 11:30:25 - 1.1 +++ subagentx.c 15 Sep 2020 09:27:46 - @@ -2929,7 +2929,7 @@ getnext: index->sav_idatacomplete = 1; break; case AGENTX_DATA_TYPE_IPADDRESS: - ipaddress = calloc(1, sizeof(ipaddress)); + ipaddress = calloc(1, sizeof(*ipaddress)); if (ipaddress == NULL) { subagentx_log_sag_warn(sag, "Failed to bind ipaddress index"); @@ -2951,7 +2951,7 @@ getnext: } if (j <= sav->sav_vb.avb_oid.aoi_idlen) index->sav_idatacomplete = 1; - data->avb_ostring.aos_slen = sizeof(ipaddress); + data->avb_ostring.aos_slen = sizeof(*ipaddress); data->avb_ostring.aos_string = (unsigned char *)ipaddress; break;
Re: agentx and clang static analyzer
On Tue, Sep 15, 2020 at 11:08:04AM +0200, Martijn van Duren wrote: > There are 3 things that actually look like valid complaints when running > clang's static analyzer. > > 1) A dead store in agentx_recv. > 2) sizeof(ipaddress) intead of sizeof(*ipaddress). Since this is ipv4, >this is only a problem if sizeof(pointer) is smaller then 4 bytes, >which can't happen afaik. > 3) dst does indeed need to be dereffed, but since dstlen and buflen are >initialized to 0 and srclen is practically always larger then 0 >we're fine. I'm keeping the *dst here as an additional safeguard. > > The rest seem like false positives to me. > > OK? > > martijn@ > > Index: agentx.c > === > RCS file: /cvs/src/usr.sbin/relayd/agentx.c,v > retrieving revision 1.16 > diff -u -p -r1.16 agentx.c > --- agentx.c 14 Sep 2020 11:30:25 - 1.16 > +++ agentx.c 15 Sep 2020 09:05:57 - > @@ -135,7 +135,6 @@ agentx_recv(struct agentx *ax) > header.aph_packetid = agentx_pdutoh32(, u8); > u8 += 4; > header.aph_plength = agentx_pdutoh32(, u8); > - u8 += 4; > > if (header.aph_version != 1) { > errno = EPROTO; ok for this piece above > Index: subagentx.c > === > RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v > retrieving revision 1.1 > diff -u -p -r1.1 subagentx.c > --- subagentx.c 14 Sep 2020 11:30:25 - 1.1 > +++ subagentx.c 15 Sep 2020 09:05:58 - > @@ -2929,7 +2929,7 @@ getnext: > index->sav_idatacomplete = 1; > break; > case AGENTX_DATA_TYPE_IPADDRESS: > - ipaddress = calloc(1, sizeof(ipaddress)); > + ipaddress = calloc(1, sizeof(*ipaddress)); > if (ipaddress == NULL) { > subagentx_log_sag_warn(sag, > "Failed to bind ipaddress index"); Not ok for this, it's probably correct, but there are other instances of this in this code and so you need to engage brain, not static analyzer, go fix or don't fix them all in a separate commit. > @@ -3833,7 +3833,7 @@ subagentx_strcat(char **dst, const char > } > > srclen = strlen(src); > - if (dst == NULL || dstlen + srclen > buflen) { > + if (*dst == NULL || dstlen + srclen > buflen) { > nbuflen = (((dstlen + srclen) / 512) + 1) * 512; > tmp = recallocarray(*dst, buflen, nbuflen, sizeof(*tmp)); > if (tmp == NULL) > ok for this piece above
agentx and clang static analyzer
There are 3 things that actually look like valid complaints when running clang's static analyzer. 1) A dead store in agentx_recv. 2) sizeof(ipaddress) intead of sizeof(*ipaddress). Since this is ipv4, this is only a problem if sizeof(pointer) is smaller then 4 bytes, which can't happen afaik. 3) dst does indeed need to be dereffed, but since dstlen and buflen are initialized to 0 and srclen is practically always larger then 0 we're fine. I'm keeping the *dst here as an additional safeguard. The rest seem like false positives to me. OK? martijn@ Index: agentx.c === RCS file: /cvs/src/usr.sbin/relayd/agentx.c,v retrieving revision 1.16 diff -u -p -r1.16 agentx.c --- agentx.c14 Sep 2020 11:30:25 - 1.16 +++ agentx.c15 Sep 2020 09:05:57 - @@ -135,7 +135,6 @@ agentx_recv(struct agentx *ax) header.aph_packetid = agentx_pdutoh32(, u8); u8 += 4; header.aph_plength = agentx_pdutoh32(, u8); - u8 += 4; if (header.aph_version != 1) { errno = EPROTO; Index: subagentx.c === RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v retrieving revision 1.1 diff -u -p -r1.1 subagentx.c --- subagentx.c 14 Sep 2020 11:30:25 - 1.1 +++ subagentx.c 15 Sep 2020 09:05:58 - @@ -2929,7 +2929,7 @@ getnext: index->sav_idatacomplete = 1; break; case AGENTX_DATA_TYPE_IPADDRESS: - ipaddress = calloc(1, sizeof(ipaddress)); + ipaddress = calloc(1, sizeof(*ipaddress)); if (ipaddress == NULL) { subagentx_log_sag_warn(sag, "Failed to bind ipaddress index"); @@ -3833,7 +3833,7 @@ subagentx_strcat(char **dst, const char } srclen = strlen(src); - if (dst == NULL || dstlen + srclen > buflen) { + if (*dst == NULL || dstlen + srclen > buflen) { nbuflen = (((dstlen + srclen) / 512) + 1) * 512; tmp = recallocarray(*dst, buflen, nbuflen, sizeof(*tmp)); if (tmp == NULL)
Re: acme-client: improve account creation error message
On Mon, Sep 14, 2020 at 04:26:20PM -0500, Rafael Possamai wrote: > >please dont drop the all buffer , or keep it with -vv ? > >example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf); > > > >i don't want to ktrace it to see why the new certbot version is not working > > Yeah, I think it's good to have the choice to inspect the vomit, maybe you > stumble upon some useful nuggets for troubleshooting an obscure issue. > > I believe the JSON output is valid, just not pretty. Maybe we could add > syntax highlighting? (just kidding) > I don't understand what you people are going on about, I didn't touch the -v output. You can still roll around in json. It's not about if it's valid json or not, it's untrusted input that we spit on your console when used with -v. -- I'm not entirely sure you are real.
Re: PATCH: Add ACPI IVHD_EXT structure to acpireg.h
> Date: Tue, 15 Sep 2020 01:37:33 -0500 > From: Jordan Hargrave > > This patch adds a couple of entries for AMD IOMMU structure > definitions in ACPI ok kettenis@ > Index: acpireg.h > === > RCS file: /cvs/src/sys/dev/acpi/acpireg.h,v > retrieving revision 1.45 > diff -u -p -r1.45 acpireg.h > --- acpireg.h 28 Aug 2019 22:39:09 - 1.45 > +++ acpireg.h 15 Sep 2020 06:29:50 - > @@ -623,6 +623,9 @@ struct acpi_ivmd { > struct acpi_ivhd { > uint8_t type; > uint8_t flags; > +#define IVHD_PPRSUP (1L << 7) > +#define IVHD_PREFSUP (1L << 6) > +#define IVHD_COHERENT(1L << 5) > #define IVHD_IOTLB (1L << 4) > #define IVHD_ISOC(1L << 3) > #define IVHD_RESPASSPW (1L << 2) > @@ -638,13 +641,28 @@ struct acpi_ivhd { > #define IVHD_UNITID_MASK 0x1F > #define IVHD_MSINUM_SHIFT0 > #define IVHD_MSINUM_MASK 0x1F > - uint32_treserved; > + uint32_tfeature; > +} __packed; > + > +struct acpi_ivhd_ext { > + uint8_t type; > + uint8_t flags; > + uint16_tlength; > + uint16_tdevid; > + uint16_tcap; > + uint64_taddress; > + uint16_tsegment; > + uint16_tinfo; > + uint32_tattrib; > + uint64_tefr; > + uint8_t reserved[8]; > } __packed; > > union acpi_ivrs_entry { > struct { > uint8_t type; > #define IVRS_IVHD0x10 > +#define IVRS_IVHD_EXT0x11 > #define IVRS_IVMD_ALL0x20 > #define IVRS_IVMD_SPECIFIED 0x21 > #define IVRS_IVMD_RANGE 0x22 > @@ -652,6 +670,7 @@ union acpi_ivrs_entry { > uint16_tlength; > } __packed; > struct acpi_ivhdivhd; > + struct acpi_ivhd_extivhd_ext; > struct acpi_ivmdivmd; > } __packed; > > >
PATCH: Add ACPI IVHD_EXT structure to acpireg.h
This patch adds a couple of entries for AMD IOMMU structure definitions in ACPI Index: acpireg.h === RCS file: /cvs/src/sys/dev/acpi/acpireg.h,v retrieving revision 1.45 diff -u -p -r1.45 acpireg.h --- acpireg.h 28 Aug 2019 22:39:09 - 1.45 +++ acpireg.h 15 Sep 2020 06:29:50 - @@ -623,6 +623,9 @@ struct acpi_ivmd { struct acpi_ivhd { uint8_t type; uint8_t flags; +#define IVHD_PPRSUP(1L << 7) +#define IVHD_PREFSUP (1L << 6) +#define IVHD_COHERENT (1L << 5) #define IVHD_IOTLB (1L << 4) #define IVHD_ISOC (1L << 3) #define IVHD_RESPASSPW (1L << 2) @@ -638,13 +641,28 @@ struct acpi_ivhd { #define IVHD_UNITID_MASK 0x1F #define IVHD_MSINUM_SHIFT 0 #define IVHD_MSINUM_MASK 0x1F - uint32_treserved; + uint32_tfeature; +} __packed; + +struct acpi_ivhd_ext { + uint8_t type; + uint8_t flags; + uint16_tlength; + uint16_tdevid; + uint16_tcap; + uint64_taddress; + uint16_tsegment; + uint16_tinfo; + uint32_tattrib; + uint64_tefr; + uint8_t reserved[8]; } __packed; union acpi_ivrs_entry { struct { uint8_t type; #define IVRS_IVHD 0x10 +#define IVRS_IVHD_EXT 0x11 #define IVRS_IVMD_ALL 0x20 #define IVRS_IVMD_SPECIFIED0x21 #define IVRS_IVMD_RANGE0x22 @@ -652,6 +670,7 @@ union acpi_ivrs_entry { uint16_tlength; } __packed; struct acpi_ivhdivhd; + struct acpi_ivhd_extivhd_ext; struct acpi_ivmdivmd; } __packed;