Re: acme-client: another trivial accessor conversion
I'm not a huge fan of these long if else if chains in this code base, so
fine by me. OK
On 2021-11-22 00:18 +01, Theo Buehler wrote:
> bio->num_write aka BIO_number_written(bio). Straightforward. The main
> reason I'm asking is that keeping the two else results in overlong lines
> and awkward line wrapping. So I decided to drop them hoping that's
> acceptable. Otherwise please tell me the preferred way to wrap the
> lines in this part of the tree.
>
> Index: revokeproc.c
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/revokeproc.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 revokeproc.c
> --- revokeproc.c 13 Oct 2021 18:09:42 - 1.18
> +++ revokeproc.c 21 Nov 2021 23:07:37 -
> @@ -186,15 +186,17 @@ revokeproc(int fd, const char *certfile,
> if (bio == NULL) {
> warnx("BIO_new");
> goto out;
> - } else if (!X509V3_EXT_print(bio, ex, 0, 0)) {
> + }
> + if (!X509V3_EXT_print(bio, ex, 0, 0)) {
> warnx("X509V3_EXT_print");
> goto out;
> - } else if ((san = calloc(1, bio->num_write + 1)) == NULL) {
> + }
> + if ((san = calloc(1, BIO_number_written(bio) + 1)) == NULL) {
> warn("calloc");
> goto out;
> }
> - ssz = BIO_read(bio, san, bio->num_write);
> - if (ssz < 0 || (unsigned)ssz != bio->num_write) {
> + ssz = BIO_read(bio, san, BIO_number_written(bio));
> + if (ssz < 0 || (unsigned)ssz != BIO_number_written(bio)) {
> warnx("BIO_read");
> goto out;
> }
>
--
I'm not entirely sure you are real.
Re: tee(1): increase read buffer to MAXBSIZE bytes
On 11/21/21 10:36 PM, Theo de Raadt wrote: Scott Cheloha wrote: The point of diminishing returns on my machine is 128K. ... So, is 128K ok? Any objections? Many of us have forgotten that our testing machines are at the fast end of the curve. I recommend 64K. I suspect that is still the sweet spot for userland. Above 64K, I think you are hogging the layers of dcache against other processes. Testing this program in isolation is a micro-benchmark. On my slow, small machines transfers > 64K definitely run slower than -exactly- 64K. Any transfer > MAXPHYS needs 2 i/o ops. Anything less wastes time in system calls. My machines are 1.5-2GHz 8GB 2-4 core low end Intel CPUs
Re: vmm(4): copyout guest regs, irqready on VM_EXIT_NONE
Mike Larkin writes:
> On Sat, Nov 20, 2021 at 09:14:31PM -0500, Dave Voutila wrote:
>> The below diff fixes an issue reported by kn@ on bugs@ [1]. joshe@ also
>> observed the issue and confirmed the below diff resolves it.
>>
>> The symptoms were quite odd: errors from fdc(4) during an OpenBSD guest
>> booting under vmm(4)/vmd(8). We don't emulate a floppy disk drive!!!
>>
>> I introduced a bug in r1.287 [2] when simplifying parts of
>> vcpu_run_{svm,vmx} by letting the functions return 0 instead of
>> voluntarily yielding. The edge case I didn't account for is if after a
>> vmexit for an IN instruction, the io port address isn't one emulated by
>> vmd(8) in userland, vmm(4) will perform the emulation (not the bug) by
>> writing the appropriate number of 0xff bytes to AL/AX/EAX. IF the
>> scheduler would like us to yield, we return setting a vrp exit code of
>> VM_EXIT_NONE (since we aren't asking userland/vmd to help with any
>> emulation).
>>
>> vmd(8) correctly handles this exit, but vmm(4) never copies out the
>> current vcpu registers and irqready state. When vmd(8) runs the vcpu
>> again, the vcpu's guest state still has a vmexit related to the IO
>> operation and presumes vmd(8) modified RAX and overwrites the vcpu's
>> RAX before re-entering the guest.
>>
>> This behavior occurs on both Intel and AMD. To confirm, I added some
>> printfs to fdc(4) and specifically checked when the dma reads returned
>> something other than 0xff on instances of both types of host. (Since
>> it's probabilistic, it's not uncommon to see it happen only 3-4 times
>> out of the 100k bus reads out_fdc() attempts, but it seems more
>> reproducible on older hardware.)
>>
>> ok?
>>
>> -dv
>>
>
> ok mlarkin if not already committed
>
Prior to commit I was was testing my diff with some others I'm working
on and found I missed one thing. We should be using the vrp exit reason,
not the one in the vcpu.vc_gueststate. While it's a bit odd to carry the
exit reason in two places, the one we send to userland is a part of
vrp...and vrp is where we check for the continue flag.
I was slightly off in my prior assessment. We emulate rax on the
vc_gueststate and set the PREVIOUS rax on the vc_exit data sent to
userland. So while my diff vastly reduced the probability of delivering
a response that would convince fdc(4) that there's a device, it was
still missing this piece to make sure we don't clobber the vc_gueststate
with the rax result that we emulated the in/out vmexit.
diff 73abbd0403c8988c9d44e9b268fc8d0b7e665e30 /usr/src
blob - 10832653c2c271a7fd1dd8bd8ae57a5f3c3c2fe3
file + sys/arch/amd64/amd64/vmm.c
--- sys/arch/amd64/amd64/vmm.c
+++ sys/arch/amd64/amd64/vmm.c
@@ -4644,7 +4644,7 @@ vcpu_run_vmx(struct vcpu *vcpu, struct vm_run_params *
irq = vrp->vrp_irq;
if (vrp->vrp_continue) {
- switch (vcpu->vc_gueststate.vg_exit_reason) {
+ switch (vrp->vrp_exit_reason) {
case VMX_EXIT_IO:
if (vcpu->vc_exit.vei.vei_dir == VEI_DIR_IN)
vcpu->vc_gueststate.vg_rax =
@@ -7099,7 +7099,7 @@ vcpu_run_svm(struct vcpu *vcpu, struct vm_run_params *
* exit data structure.
*/
if (vrp->vrp_continue) {
- switch (vcpu->vc_gueststate.vg_exit_reason) {
+ switch (vrp->vrp_exit_reason) {
case SVM_VMEXIT_IOIO:
if (vcpu->vc_exit.vei.vei_dir == VEI_DIR_IN) {
vcpu->vc_gueststate.vg_rax =
>> [1] https://marc.info/?l=openbsd-bugs=163682062027764=2
>> [2]
>> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/amd64/amd64/vmm.c.diff?r1=1.286=1.287
>>
>>
>> Index: sys/arch/amd64/amd64/vmm.c
>> ===
>> RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
>> retrieving revision 1.294
>> diff -u -p -r1.294 vmm.c
>> --- sys/arch/amd64/amd64/vmm.c 26 Oct 2021 16:29:49 - 1.294
>> +++ sys/arch/amd64/amd64/vmm.c 20 Nov 2021 21:46:07 -
>> @@ -4301,9 +4301,10 @@ vm_run(struct vm_run_params *vrp)
>> rw_exit_write(_softc->vm_lock);
>> }
>> ret = 0;
>> -} else if (ret == EAGAIN) {
>> +} else if (ret == 0 || ret == EAGAIN) {
>> /* If we are exiting, populate exit data so vmd can help. */
>> -vrp->vrp_exit_reason = vcpu->vc_gueststate.vg_exit_reason;
>> +vrp->vrp_exit_reason = (ret == 0) ? VM_EXIT_NONE
>> +: vcpu->vc_gueststate.vg_exit_reason;
>> vrp->vrp_irqready = vcpu->vc_irqready;
>> vcpu->vc_state = VCPU_STATE_STOPPED;
>>
>> @@ -4312,9 +4313,6 @@ vm_run(struct vm_run_params *vrp)
>> ret = EFAULT;
>> } else
>> ret = 0;
>> -} else if (ret == 0) {
>> -vrp->vrp_exit_reason = VM_EXIT_NONE;
>> -vcpu->vc_state = VCPU_STATE_STOPPED;
>>
Re: tee(1): increase read buffer to MAXBSIZE bytes
Scott Cheloha wrote: > The point of diminishing returns on my machine is 128K. ... > So, is 128K ok? Any objections? Many of us have forgotten that our testing machines are at the fast end of the curve. I recommend 64K. I suspect that is still the sweet spot for userland. Above 64K, I think you are hogging the layers of dcache against other processes. Testing this program in isolation is a micro-benchmark.
Re: tee(1): increase read buffer to MAXBSIZE bytes
On Fri, Nov 19, 2021 at 08:40:37PM -0700, Theo de Raadt wrote:
> Scott Cheloha wrote:
>
> > On Fri, Nov 19, 2021 at 07:42:18PM -0700, Theo de Raadt wrote:
> > > +#include /* for MAXBSIZE */
> > >
> > > No way, that is non-POSIX namespace. We are going in precisely the
> > > opposite
> > > direction, eliminating this non-portability from the tree.
> > >
> > > No biblical scrolls have it written "all programs must use buffer sizes
> > > decided by a system header file".
> > >
> > > If you want 64*1024 in this specific program then just say 64*1024.
> >
> > Is there a nicer way to pick a "reasonable" buffer size when we just
> > want to move as many bytes as possible on a given platform without
> > hogging the machine?
>
> Pick a number.
>
> Use a number.
>
> [...]
>
> The right buffer size should be chosen for in a specific program. It is
> a number which is small enough to not hog the system, yet large enough
> to gain reasonable benefits versus the cost of servicing/faulting
> memory. Obviously the number chosen will change during the era, unlike
> MAXBSIZE and BUFSIZ which are going on 30 years old and were chosen by
> someone sharing a vax 11/780 (or even smaller machine).
The point of diminishing returns on my machine is 128K.
I ran this synthetic benchmark on doubling buffer sizes from 8K up
through 1M:
for i in $(jot 100); do
dd if=/dev/zero bs=1M count=1K 2>/dev/null \
| nanotime tee /dev/null > /dev/null 2>>time.$size
done
On an otherwise quiet machine I got these results:
x time.8192
+ time.16384
* time.32768
% time.65536
# time.131072
@ time.262144
& time.524288
$ time.1048576
N Min Max Median AvgStddev
x 100 0.94371683 1.0566744 0.95342365 0.97797553 0.031421145
+ 100 0.61271914 0.64374531 0.63532658 0.6346069 0.0055198003
Difference at 99.5% confidence
-0.343369 +/- 0.00985781
-35.1101% +/- 1.00798%
(Student's t, pooled s = 0.0225583)
* 100 0.41367684 0.44712464 0.43615004 0.43494362 0.0071550676
Difference at 99.5% confidence
-0.543032 +/- 0.00995768
-55.5261% +/- 1.01819%
(Student's t, pooled s = 0.0227869)
% 100 0.26071834 0.28996026 0.28033773 0.27985476 0.0057852679
Difference at 99.5% confidence
-0.698121 +/- 0.00987233
-71.3843% +/- 1.00947%
(Student's t, pooled s = 0.0225916)
# 100 0.24090659 0.26264406 0.25455999 0.25441678 0.0044142139
Difference at 99.5% confidence
-0.723559 +/- 0.00980448
-73.9854% +/- 1.00253%
(Student's t, pooled s = 0.0224363)
@ 100 0.24297249 0.26462487 0.25686952 0.25593464 0.0047320572
Difference at 99.5% confidence
-0.722041 +/- 0.00981862
-73.8302% +/- 1.00397%
(Student's t, pooled s = 0.0224687)
& 100 0.24499786 0.26534237 0.25667553 0.2562827 0.0039443348
Difference at 99.5% confidence
-0.721693 +/- 0.00978533
-73.7946% +/- 1.00057%
(Student's t, pooled s = 0.0223925)
$ 100 0.24157916 0.2657958 0.25527962 0.25451313 0.0049337784
Difference at 99.5% confidence
-0.723462 +/- 0.0098281
-73.9755% +/- 1.00494%
(Student's t, pooled s = 0.0224903)
Beyond 128K you are buying little to nothing with the additional
memory:
x time.131072
+ time.262144
* time.524288
% time.1048576
N Min Max Median AvgStddev
x 100 0.24090659 0.26264406 0.25455999 0.25441678 0.0044142139
+ 100 0.24297249 0.26462487 0.25686952 0.25593464 0.0047320572
No difference proven at 99.5% confidence
* 100 0.24499786 0.26534237 0.25667553 0.2562827 0.0039443348
Difference at 99.5% confidence
0.00186592 +/- 0.00182919
0.73341% +/- 0.718975%
(Student's t, pooled s = 0.00418587)
% 100 0.24157916 0.2657958 0.25527962 0.25451313 0.0049337784
No difference proven at 99.5% confidence
I think the .73% improvement at 512KB is a fluke. Even if it isn't,
quadruple the memory for less than 1% improvement isn't worth it.
So, is 128K ok? Any objections?
Index: tee.c
===
RCS file: /cvs/src/usr.bin/tee/tee.c,v
retrieving revision 1.13
diff -u -p -r1.13 tee.c
--- tee.c 21 Nov 2021 16:15:43 - 1.13
+++ tee.c 22 Nov 2021 00:04:24 -
@@ -43,6 +43,8 @@
#include
#include
+#define SIZE (128 * 1024)
+
struct list {
SLIST_ENTRY(list) next;
int fd;
@@ -67,9 +69,10 @@ main(int argc, char *argv[])
{
struct list *p;
int fd;
+ size_t size;
ssize_t n, rval, wval;
int append, ch, exitval;
- char buf[8192];
+ char *buf;
if (pledge("stdio wpath cpath", NULL) == -1)
err(1, "pledge");
@@ -109,6 +112,10 @@ main(int argc, char *argv[])
if (pledge("stdio", NULL) == -1)
err(1, "pledge");
+ size = SIZE;
+ buf = malloc(size);
+ if
acme-client: another trivial accessor conversion
bio->num_write aka BIO_number_written(bio). Straightforward. The main
reason I'm asking is that keeping the two else results in overlong lines
and awkward line wrapping. So I decided to drop them hoping that's
acceptable. Otherwise please tell me the preferred way to wrap the
lines in this part of the tree.
Index: revokeproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/revokeproc.c,v
retrieving revision 1.18
diff -u -p -r1.18 revokeproc.c
--- revokeproc.c13 Oct 2021 18:09:42 - 1.18
+++ revokeproc.c21 Nov 2021 23:07:37 -
@@ -186,15 +186,17 @@ revokeproc(int fd, const char *certfile,
if (bio == NULL) {
warnx("BIO_new");
goto out;
- } else if (!X509V3_EXT_print(bio, ex, 0, 0)) {
+ }
+ if (!X509V3_EXT_print(bio, ex, 0, 0)) {
warnx("X509V3_EXT_print");
goto out;
- } else if ((san = calloc(1, bio->num_write + 1)) == NULL) {
+ }
+ if ((san = calloc(1, BIO_number_written(bio) + 1)) == NULL) {
warn("calloc");
goto out;
}
- ssz = BIO_read(bio, san, bio->num_write);
- if (ssz < 0 || (unsigned)ssz != bio->num_write) {
+ ssz = BIO_read(bio, san, BIO_number_written(bio));
+ if (ssz < 0 || (unsigned)ssz != BIO_number_written(bio)) {
warnx("BIO_read");
goto out;
}
libkeynote: prepare for opaque EVP_PKEY, fix some leaks
The fix I need introduces the use of EVP_PKEY_get0_RSA().
Ownership handling in this scope is a bit wonky: X509_get_pubkey()
increments the refcount of pPublicKey. What we actually want is a
reference of its pkey.rsa. So use X509_get0_pubkey() instead and up the
refcount of the RSA. Finally, let's not leak the cert on success.
Index: signature.c
===
RCS file: /cvs/src/lib/libkeynote/signature.c,v
retrieving revision 1.26
diff -u -p -r1.26 signature.c
--- signature.c 9 May 2017 13:52:45 - 1.26
+++ signature.c 21 Nov 2021 22:41:00 -
@@ -522,7 +522,7 @@ kn_decode_key(struct keynote_deckey *dc,
return -1;
}
- if ((pPublicKey = X509_get_pubkey(px509Cert)) == NULL) {
+ if ((pPublicKey = X509_get0_pubkey(px509Cert)) == NULL) {
free(ptr);
X509_free(px509Cert);
keynote_errno = ERROR_SYNTAX;
@@ -530,9 +530,11 @@ kn_decode_key(struct keynote_deckey *dc,
}
/* RSA-specific */
- dc->dec_key = pPublicKey->pkey.rsa;
+ dc->dec_key = EVP_PKEY_get0_RSA(pPublicKey);
+ RSA_up_ref(dc->dec_key);
free(ptr);
+ X509_free(px509Cert);
return 0;
}
Re: IPsec tdb ref counting
Updated tdb refcounting diff after merging with mvs@'s commit.
Index: net/if_bridge.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.358
diff -u -p -r1.358 if_bridge.c
--- net/if_bridge.c 11 Nov 2021 18:08:17 - 1.358
+++ net/if_bridge.c 21 Nov 2021 21:51:15 -
@@ -1567,20 +1567,28 @@ bridge_ipsec(struct ifnet *ifp, struct e
tdb->tdb_xform != NULL) {
if (tdb->tdb_first_use == 0) {
tdb->tdb_first_use = gettime();
- if (tdb->tdb_flags & TDBF_FIRSTUSE)
- timeout_add_sec(>tdb_first_tmo,
- tdb->tdb_exp_first_use);
- if (tdb->tdb_flags & TDBF_SOFT_FIRSTUSE)
- timeout_add_sec(>tdb_sfirst_tmo,
- tdb->tdb_soft_first_use);
+ if (tdb->tdb_flags & TDBF_FIRSTUSE) {
+ if (timeout_add_sec(
+ >tdb_first_tmo,
+ tdb->tdb_exp_first_use))
+ tdb_ref(tdb);
+ }
+ if (tdb->tdb_flags & TDBF_SOFT_FIRSTUSE) {
+ if (timeout_add_sec(
+ >tdb_sfirst_tmo,
+ tdb->tdb_soft_first_use))
+ tdb_ref(tdb);
+ }
}
prot = (*(tdb->tdb_xform->xf_input))(, tdb, hlen,
off);
+ tdb_unref(tdb);
if (prot != IPPROTO_DONE)
ip_deliver(, , prot, af);
return (1);
} else {
+ tdb_unref(tdb);
skiplookup:
/* XXX do an input policy lookup */
return (0);
Index: net/if_pfsync.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.298
diff -u -p -r1.298 if_pfsync.c
--- net/if_pfsync.c 11 Nov 2021 12:35:01 - 1.298
+++ net/if_pfsync.c 21 Nov 2021 21:51:15 -
@@ -1325,11 +1325,13 @@ pfsync_update_net_tdb(struct pfsync_tdb
/* Neither replay nor byte counter should ever decrease. */
if (pt->rpl < tdb->tdb_rpl ||
pt->cur_bytes < tdb->tdb_cur_bytes) {
+ tdb_unref(tdb);
goto bad;
}
tdb->tdb_rpl = pt->rpl;
tdb->tdb_cur_bytes = pt->cur_bytes;
+ tdb_unref(tdb);
}
return;
Index: net/pfkeyv2.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.221
diff -u -p -r1.221 pfkeyv2.c
--- net/pfkeyv2.c 25 Oct 2021 18:25:01 - 1.221
+++ net/pfkeyv2.c 21 Nov 2021 21:51:15 -
@@ -1043,8 +1043,11 @@ pfkeyv2_sa_flush(struct tdb *tdb, void *
{
if (!(*((u_int8_t *) satype_vp)) ||
tdb->tdb_satype == *((u_int8_t *) satype_vp)) {
+ /* keep in sync with tdb_delete() */
tdb_unlink_locked(tdb);
- tdb_free(tdb);
+ tdb_unbundle(tdb);
+ tdb_deltimeouts(tdb);
+ tdb_unref(tdb);
}
return (0);
}
@@ -1327,7 +1330,7 @@ pfkeyv2_send(struct socket *so, void *me
if ((rval = pfkeyv2_get_proto_alg(newsa->tdb_satype,
>tdb_sproto, ))) {
- tdb_free(freeme);
+ tdb_unref(freeme);
freeme = NULL;
NET_UNLOCK();
goto ret;
@@ -1363,7 +1366,7 @@ pfkeyv2_send(struct socket *so, void *me
headers[SADB_X_EXT_DST_MASK],
headers[SADB_X_EXT_PROTOCOL],
headers[SADB_X_EXT_FLOW_TYPE]))) {
- tdb_free(freeme);
+ tdb_unref(freeme);
freeme = NULL;
NET_UNLOCK();
goto ret;
@@ -1386,7 +1389,7 @@ pfkeyv2_send(struct socket *so, void *me
rval = tdb_init(newsa, alg, );
if (rval) {
rval = EINVAL;
- tdb_free(freeme);
+
PMTU in IPsec IPv6 in IPv6 tunnel
Hi,
Path MTU discovery for IPv6 in IPv6 tunnel over IPsec does not work.
Sending ICMP6 too big is not implemented. Copying the code from
ip_forward() fixes it.
While there, do some cleanup.
- #ifdef IPSEC makes no sense. While IPsec needs this code, only
the route and interface are used and all protocols are affected.
- Sort the cases in v4 and v6 the same way.
- Initialize the error variable in both functions. Is is not
needed in ip_forward() but the gotos don't make it obvious. The
gotos in ip6_forward() are differnet and error = 0 is neccessary.
- Do not send an icmp packet if destmtu could not be set.
- The pf(4) EACCES case also makes sense for IPv4.
ok?
bluhm
Index: netinet/ip_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.363
diff -u -p -r1.363 ip_input.c
--- netinet/ip_input.c 21 Jun 2021 22:09:14 - 1.363
+++ netinet/ip_input.c 21 Nov 2021 22:04:20 -
@@ -1436,7 +1436,7 @@ ip_forward(struct mbuf *m, struct ifnet
struct ip *ip = mtod(m, struct ip *);
struct sockaddr_in *sin;
struct route ro;
- int error, type = 0, code = 0, destmtu = 0, fake = 0, len;
+ int error = 0, type = 0, code = 0, destmtu = 0, fake = 0, len;
u_int32_t dest;
dest = 0;
@@ -1535,29 +1535,17 @@ ip_forward(struct mbuf *m, struct ifnet
goto freecopy;
switch (error) {
-
case 0: /* forwarded, but need redirect */
/* type, code set above */
break;
- case ENETUNREACH: /* shouldn't happen, checked above */
- case EHOSTUNREACH:
- case ENETDOWN:
- case EHOSTDOWN:
- default:
- type = ICMP_UNREACH;
- code = ICMP_UNREACH_HOST;
- break;
-
case EMSGSIZE:
type = ICMP_UNREACH;
code = ICMP_UNREACH_NEEDFRAG;
-
-#ifdef IPSEC
if (rt != NULL) {
- if (rt->rt_mtu)
+ if (rt->rt_mtu) {
destmtu = rt->rt_mtu;
- else {
+ } else {
struct ifnet *destifp;
destifp = if_get(rt->rt_ifidx);
@@ -1566,8 +1554,9 @@ ip_forward(struct mbuf *m, struct ifnet
if_put(destifp);
}
}
-#endif /*IPSEC*/
ipstat_inc(ips_cantfrag);
+ if (destmtu == 0)
+ goto freecopy;
break;
case EACCES:
@@ -1576,6 +1565,7 @@ ip_forward(struct mbuf *m, struct ifnet
* packet back since pf(4) takes care of it.
*/
goto freecopy;
+
case ENOBUFS:
/*
* a router should not generate ICMP_SOURCEQUENCH as
@@ -1584,8 +1574,16 @@ ip_forward(struct mbuf *m, struct ifnet
* or the underlying interface is rate-limited.
*/
goto freecopy;
- }
+ case ENETUNREACH: /* shouldn't happen, checked above */
+ case EHOSTUNREACH:
+ case ENETDOWN:
+ case EHOSTDOWN:
+ default:
+ type = ICMP_UNREACH;
+ code = ICMP_UNREACH_HOST;
+ break;
+ }
mcopy = m_copym(, 0, len, M_DONTWAIT);
if (mcopy)
icmp_error(mcopy, type, code, dest, destmtu);
Index: netinet6/ip6_forward.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_forward.c,v
retrieving revision 1.101
diff -u -p -r1.101 ip6_forward.c
--- netinet6/ip6_forward.c 14 Oct 2021 17:39:42 - 1.101
+++ netinet6/ip6_forward.c 21 Nov 2021 22:04:22 -
@@ -88,7 +88,7 @@ ip6_forward(struct mbuf *m, struct rtent
struct sockaddr_in6 *sin6;
struct route_in6 ro;
struct ifnet *ifp = NULL;
- int error = 0, type = 0, code = 0;
+ int error = 0, type = 0, code = 0, destmtu = 0;
struct mbuf *mcopy = NULL;
#ifdef IPSEC
struct tdb *tdb = NULL;
@@ -340,6 +340,7 @@ senderr:
#endif
if (mcopy == NULL)
goto out;
+
switch (error) {
case 0:
if (type == ND_REDIRECT) {
@@ -349,7 +350,30 @@ senderr:
goto freecopy;
case EMSGSIZE:
- /* xxx MTU is constant in PPP? */
+ type = ICMP6_PACKET_TOO_BIG;
+ code = 0;
+ if (rt != NULL) {
+ if (rt->rt_mtu) {
+ destmtu = rt->rt_mtu;
+ } else {
+ struct ifnet *destifp;
+
+ destifp = if_get(rt->rt_ifidx);
+ if
Re: asr(3): strip AD flag in responses
On Sun, Nov 21 2021, Jeremie Courreges-Anglas wrote: > On Sat, Nov 20 2021, Florian Obser wrote: [...] >>> Index: lib/libc/asr/res_mkquery.c >>> === >>> RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v >>> retrieving revision 1.13 >>> diff -u -p -r1.13 res_mkquery.c >>> --- lib/libc/asr/res_mkquery.c 14 Jan 2019 06:49:42 - 1.13 >>> +++ lib/libc/asr/res_mkquery.c 20 Nov 2021 14:24:08 - >>> @@ -62,6 +62,9 @@ res_mkquery(int op, const char *dname, i >>> h.flags |= RD_MASK; >>> if (ac->ac_options & RES_USE_CD) >>> h.flags |= CD_MASK; >>> + if ((ac->ac_options & RES_TRUSTAD) && >>> + !(ac->ac_options & RES_USE_DNSSEC)) >>> + h.flags |= AD_MASK; >> >> do you remember why you check for !RES_USE_DNSSEC? >> I'd like to leave it out. > > First, here's my understanding of RES_USE_DNSSEC: it's supposed to > activate EDNS and set the DO bit. The server is then supposed to reply > with AD set only if the data has been validated (with or without > DNSSEC), and possibly append add DNSSEC data if available. > > Since I didn't know the semantics of both setting AD and DO in a query > (I would expect such semantics to be sane) I wrote those more > conservative checks instead. Does this make sense? If so, maybe > a comment would help? > > /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */ > if ((ac->ac_options & RES_TRUSTAD) && > !(ac->ac_options & RES_USE_DNSSEC)) > >> In fact I don't think RES_USE_DNSSEC is useful >> at all. >> If you want to set the DO flag and start DNSSEC from first principles >> you are better of talking to the network directly, i.e. rewrite unwind. > > RES_USE_DNSSEC has been there since some time already and it's used by > software in the ports tree, precisely to detect AD=1 - and not so much > for the key records I think. BTW clearing AD might break software that depends on it for stuff like DANE. postfix uses RES_USE_DNSSEC for this, but also force-enables RES_TRUSTAD if available so it shouldn't be affected (upstream's stance is that you should only use a trusted resolver when running postfix). On the other hand mail/exim knowlingly doesn't force RES_TRUSTAD. I don't know of other ports using RES_USE_DNSSEC and caring about the AD flag. The effect of RES_TRUSTAD/trust-ad on RES_USE_DNSSEC ought to be documented, but let's do that in another diff: the documentation of the latter option is already lacking. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: asr(3): strip AD flag in responses
On Sat, Nov 20 2021, Florian Obser wrote:
> On 2021-11-20 17:05 +01, Jeremie Courreges-Anglas wrote:
[...]
>> Index: lib/libc/asr/getrrsetbyname_async.c
>> ===
>> RCS file: /home/cvs/src/lib/libc/asr/getrrsetbyname_async.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 getrrsetbyname_async.c
>> --- lib/libc/asr/getrrsetbyname_async.c 23 Feb 2017 17:04:02 -
>> 1.11
>> +++ lib/libc/asr/getrrsetbyname_async.c 20 Nov 2021 14:24:08 -
>> @@ -32,7 +32,8 @@
>> #include "asr_private.h"
>>
>> static int getrrsetbyname_async_run(struct asr_query *, struct asr_result
>> *);
>> -static void get_response(struct asr_result *, const char *, int);
>> +static void get_response(struct asr_query *, struct asr_result *, const
>> char *,
>> +int);
>>
>> struct asr_query *
>> getrrsetbyname_async(const char *hostname, unsigned int rdclass,
>> @@ -150,7 +151,7 @@ getrrsetbyname_async_run(struct asr_quer
>> break;
>> }
>>
>> -get_response(ar, ar->ar_data, ar->ar_datalen);
>> +get_response(as, ar, ar->ar_data, ar->ar_datalen);
>> free(ar->ar_data);
>> async_set_state(as, ASR_STATE_HALT);
>> break;
>> @@ -255,7 +256,8 @@ static void free_dns_response(struct dns
>> static int count_dns_rr(struct dns_rr *, u_int16_t, u_int16_t);
>>
>> static void
>> -get_response(struct asr_result *ar, const char *pkt, int pktlen)
>> +get_response(struct asr_query *as, struct asr_result *ar, const char *pkt,
>> +int pktlen)
>> {
>> struct rrsetinfo *rrset = NULL;
>> struct dns_response *response = NULL;
>> @@ -287,7 +289,7 @@ get_response(struct asr_result *ar, cons
>> rrset->rri_nrdatas = response->header.ancount;
>>
>> /* check for authenticated data */
>> -if (response->header.ad == 1)
>> +if (as->as_ctx->ac_options & RES_TRUSTAD && response->header.ad == 1)
>> rrset->rri_flags |= RRSET_VALIDATED;
>
> we can skip all this if we mask AD correctly in the header
That's much nicer indeed!
>> /* copy name from answer section */
>> Index: lib/libc/asr/res_mkquery.c
>> ===
>> RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v
>> retrieving revision 1.13
>> diff -u -p -r1.13 res_mkquery.c
>> --- lib/libc/asr/res_mkquery.c 14 Jan 2019 06:49:42 - 1.13
>> +++ lib/libc/asr/res_mkquery.c 20 Nov 2021 14:24:08 -
>> @@ -62,6 +62,9 @@ res_mkquery(int op, const char *dname, i
>> h.flags |= RD_MASK;
>> if (ac->ac_options & RES_USE_CD)
>> h.flags |= CD_MASK;
>> +if ((ac->ac_options & RES_TRUSTAD) &&
>> +!(ac->ac_options & RES_USE_DNSSEC))
>> +h.flags |= AD_MASK;
>
> do you remember why you check for !RES_USE_DNSSEC?
> I'd like to leave it out.
First, here's my understanding of RES_USE_DNSSEC: it's supposed to
activate EDNS and set the DO bit. The server is then supposed to reply
with AD set only if the data has been validated (with or without
DNSSEC), and possibly append add DNSSEC data if available.
Since I didn't know the semantics of both setting AD and DO in a query
(I would expect such semantics to be sane) I wrote those more
conservative checks instead. Does this make sense? If so, maybe
a comment would help?
/* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
if ((ac->ac_options & RES_TRUSTAD) &&
!(ac->ac_options & RES_USE_DNSSEC))
> In fact I don't think RES_USE_DNSSEC is useful
> at all.
> If you want to set the DO flag and start DNSSEC from first principles
> you are better of talking to the network directly, i.e. rewrite unwind.
RES_USE_DNSSEC has been there since some time already and it's used by
software in the ports tree, precisely to detect AD=1 - and not so much
for the key records I think.
>> h.qdcount = 1;
>> if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>> h.arcount = 1;
>> Index: lib/libc/asr/res_send_async.c
>> ===
>> RCS file: /home/cvs/src/lib/libc/asr/res_send_async.c,v
>> retrieving revision 1.39
>> diff -u -p -r1.39 res_send_async.c
>> --- lib/libc/asr/res_send_async.c28 Sep 2019 11:21:07 - 1.39
>> +++ lib/libc/asr/res_send_async.c20 Nov 2021 14:24:08 -
>> @@ -378,6 +378,9 @@ setup_query(struct asr_query *as, const
>> h.flags |= RD_MASK;
>> if (as->as_ctx->ac_options & RES_USE_CD)
>> h.flags |= CD_MASK;
>> +if ((as->as_ctx->ac_options & RES_TRUSTAD) &&
>> +!(as->as_ctx->ac_options & RES_USE_DNSSEC))
>> +h.flags |= AD_MASK;
>> h.qdcount = 1;
>> if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>> h.arcount = 1;
>> @@ -700,6 +703,9 @@ validate_packet(struct asr_query *as)
>>
Re: rge(4) diff copying over re(4) rev 1.211
OK gnezdo@
Brad Smith writes:
> Being that rge(4) is derived from re(4) it looks like it has the same
> issues as fixed in re(4) rev 1.211.
>
> revision 1.211
> date: 2021/05/17 11:59:53; author: visa; state: Exp; lines: +4 -1;
> commitid: aS9a9xwYauxPaauQ;
> Fix mbuf leaks after reception error in re_rxeof().
>
> Also, increment the error counter when an unexpected fragment is seen.
>
> Index: if_rge.c
> ===
> RCS file: /home/cvs/src/sys/dev/pci/if_rge.c,v
> retrieving revision 1.15
> diff -u -p -u -p -r1.15 if_rge.c
> --- if_rge.c 16 Aug 2021 01:30:27 - 1.15
> +++ if_rge.c 16 Aug 2021 01:49:37 -
> @@ -1223,6 +1223,8 @@ rge_rxeof(struct rge_queues *q)
>
> if ((rxstat & (RGE_RDCMDSTS_SOF | RGE_RDCMDSTS_EOF)) !=
> (RGE_RDCMDSTS_SOF | RGE_RDCMDSTS_EOF)) {
> + ifp->if_ierrors++;
> + m_freem(m);
> rge_discard_rxbuf(q, i);
> continue;
> }
> @@ -1237,6 +1239,7 @@ rge_rxeof(struct rge_queues *q)
> m_freem(q->q_rx.rge_head);
> q->q_rx.rge_head = q->q_rx.rge_tail = NULL;
> }
> + m_freem(m);
> rge_discard_rxbuf(q, i);
> continue;
> }
Re: vmm(4): copyout guest regs, irqready on VM_EXIT_NONE
On Sat, Nov 20, 2021 at 09:14:31PM -0500, Dave Voutila wrote:
> The below diff fixes an issue reported by kn@ on bugs@ [1]. joshe@ also
> observed the issue and confirmed the below diff resolves it.
>
> The symptoms were quite odd: errors from fdc(4) during an OpenBSD guest
> booting under vmm(4)/vmd(8). We don't emulate a floppy disk drive!!!
>
> I introduced a bug in r1.287 [2] when simplifying parts of
> vcpu_run_{svm,vmx} by letting the functions return 0 instead of
> voluntarily yielding. The edge case I didn't account for is if after a
> vmexit for an IN instruction, the io port address isn't one emulated by
> vmd(8) in userland, vmm(4) will perform the emulation (not the bug) by
> writing the appropriate number of 0xff bytes to AL/AX/EAX. IF the
> scheduler would like us to yield, we return setting a vrp exit code of
> VM_EXIT_NONE (since we aren't asking userland/vmd to help with any
> emulation).
>
> vmd(8) correctly handles this exit, but vmm(4) never copies out the
> current vcpu registers and irqready state. When vmd(8) runs the vcpu
> again, the vcpu's guest state still has a vmexit related to the IO
> operation and presumes vmd(8) modified RAX and overwrites the vcpu's
> RAX before re-entering the guest.
>
> This behavior occurs on both Intel and AMD. To confirm, I added some
> printfs to fdc(4) and specifically checked when the dma reads returned
> something other than 0xff on instances of both types of host. (Since
> it's probabilistic, it's not uncommon to see it happen only 3-4 times
> out of the 100k bus reads out_fdc() attempts, but it seems more
> reproducible on older hardware.)
>
> ok?
>
> -dv
>
ok mlarkin if not already committed
> [1] https://marc.info/?l=openbsd-bugs=163682062027764=2
> [2]
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/amd64/amd64/vmm.c.diff?r1=1.286=1.287
>
>
> Index: sys/arch/amd64/amd64/vmm.c
> ===
> RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> retrieving revision 1.294
> diff -u -p -r1.294 vmm.c
> --- sys/arch/amd64/amd64/vmm.c26 Oct 2021 16:29:49 - 1.294
> +++ sys/arch/amd64/amd64/vmm.c20 Nov 2021 21:46:07 -
> @@ -4301,9 +4301,10 @@ vm_run(struct vm_run_params *vrp)
> rw_exit_write(_softc->vm_lock);
> }
> ret = 0;
> - } else if (ret == EAGAIN) {
> + } else if (ret == 0 || ret == EAGAIN) {
> /* If we are exiting, populate exit data so vmd can help. */
> - vrp->vrp_exit_reason = vcpu->vc_gueststate.vg_exit_reason;
> + vrp->vrp_exit_reason = (ret == 0) ? VM_EXIT_NONE
> + : vcpu->vc_gueststate.vg_exit_reason;
> vrp->vrp_irqready = vcpu->vc_irqready;
> vcpu->vc_state = VCPU_STATE_STOPPED;
>
> @@ -4312,9 +4313,6 @@ vm_run(struct vm_run_params *vrp)
> ret = EFAULT;
> } else
> ret = 0;
> - } else if (ret == 0) {
> - vrp->vrp_exit_reason = VM_EXIT_NONE;
> - vcpu->vc_state = VCPU_STATE_STOPPED;
> } else {
> vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
> vcpu->vc_state = VCPU_STATE_TERMINATED;
Re: asr(3): strip AD flag in responses
Otto Moerbeek wrote:
> > right. So I have tried my patch (without the memcpy dance) on sparc64
> > over udp and tcp and I have also tracked this down in the code. This
> > should be fine as is. ar->ar_data comes directly out of malloc
> > (reallocarray) in ensure_ibuf() and the struct is defined thusly:
> >
> > struct asr_dns_header {
> > uint16_tid;
> > uint16_tflags;
> > uint16_tqdcount;
> > uint16_tancount;
> > uint16_tnscount;
> > uint16_tarcount;
> > };
> >
>
> So that is indeed safe as long as nobody starts allocating packet
> buffers in different ways,
The common problem is in a libpcap parser, finding it in a packet.
There, the 14-byte ethernet header comes into play, offsetting everything
by 2 bytes. However, this struct has 2-byte alignment, so the compiler will
generate safe alignment accesses.
The problem really shows itself when you find per-byte packed encapsulation
headers, due to an insufficient number of axe murderers lurking the halls
of IETF. That would place these 2-byte objects at 1-byte alignment, and the
compiler generated 2-byte instructions bomb. Such encapsulations are far
more rare. The major libpcap parser (tcpdump) avoids this by always considering
packets byte aligned..
Re: asr(3): strip AD flag in responses
On Sun, Nov 21, 2021 at 04:51:45PM +0100, Florian Obser wrote:
> On 2021-11-20 21:16 +01, Otto Moerbeek wrote:
> > On Sat, Nov 20, 2021 at 06:44:58PM +0100, Florian Obser wrote:
> >
> >> On 2021-11-20 18:41 +01, Florian Obser wrote:
> >> > On 2021-11-20 18:19 +01, Florian Obser wrote:
> >> >
> >> >> +/*
> >> >> + * Clear AD flag in the answer.
> >> >> + */
> >> >> +static void
> >> >> +clear_ad(struct asr_result *ar)
> >> >> +{
> >> >> + struct asr_dns_header *h;
> >> >> + uint16_t flags;
> >> >> +
> >> >> + h = (struct asr_dns_header *)ar->ar_data;
> >> >> + flags = ntohs(h->flags);
> >> >> + flags &= ~(AD_MASK);
> >> >> + h->flags = htons(flags);
> >> >> +}
> >> >> +
> >> >
> >> > btw. is it possible that this is not alligned correctly on sparc64?
> >> >
> >> > should be do something like (not even compile tested)
> >> >
> >> > static void
> >> > clear_ad(struct asr_result *ar)
> >> > {
> >> > struct asr_dns_headerh;
> >> >
> >> > memmove(, ar->ar_data, sizeof(h));
> >> > h.flags = ntohs(h.flags);
> >> > h.flags &= ~(AD_MASK);
> >> > h.flags = htons(h.flags);
> >> > memmove(ar->ar_data, , sizeof(h));
> >> > }
> >> >
> >>
> >> memcpy obviously, I was distracted by the copious amount of memmove in
> >> asr code...
> >
> > It is not needed to copy the "whole" header just to change the flags.
> > You could just copy out, modify and copy back the flags field only.
> >
> > otoh, it's just 12 bytes, so no big deal.
>
> right. So I have tried my patch (without the memcpy dance) on sparc64
> over udp and tcp and I have also tracked this down in the code. This
> should be fine as is. ar->ar_data comes directly out of malloc
> (reallocarray) in ensure_ibuf() and the struct is defined thusly:
>
> struct asr_dns_header {
> uint16_tid;
> uint16_tflags;
> uint16_tqdcount;
> uint16_tancount;
> uint16_tnscount;
> uint16_tarcount;
> };
>
So that is indeed safe as long as nobody starts allocating packet
buffers in different ways,
-Otto
Re: asr(3): strip AD flag in responses
On 2021-11-20 21:16 +01, Otto Moerbeek wrote:
> On Sat, Nov 20, 2021 at 06:44:58PM +0100, Florian Obser wrote:
>
>> On 2021-11-20 18:41 +01, Florian Obser wrote:
>> > On 2021-11-20 18:19 +01, Florian Obser wrote:
>> >
>> >> +/*
>> >> + * Clear AD flag in the answer.
>> >> + */
>> >> +static void
>> >> +clear_ad(struct asr_result *ar)
>> >> +{
>> >> + struct asr_dns_header *h;
>> >> + uint16_t flags;
>> >> +
>> >> + h = (struct asr_dns_header *)ar->ar_data;
>> >> + flags = ntohs(h->flags);
>> >> + flags &= ~(AD_MASK);
>> >> + h->flags = htons(flags);
>> >> +}
>> >> +
>> >
>> > btw. is it possible that this is not alligned correctly on sparc64?
>> >
>> > should be do something like (not even compile tested)
>> >
>> > static void
>> > clear_ad(struct asr_result *ar)
>> > {
>> >struct asr_dns_headerh;
>> >
>> > memmove(, ar->ar_data, sizeof(h));
>> > h.flags = ntohs(h.flags);
>> > h.flags &= ~(AD_MASK);
>> > h.flags = htons(h.flags);
>> > memmove(ar->ar_data, , sizeof(h));
>> > }
>> >
>>
>> memcpy obviously, I was distracted by the copious amount of memmove in
>> asr code...
>
> It is not needed to copy the "whole" header just to change the flags.
> You could just copy out, modify and copy back the flags field only.
>
> otoh, it's just 12 bytes, so no big deal.
right. So I have tried my patch (without the memcpy dance) on sparc64
over udp and tcp and I have also tracked this down in the code. This
should be fine as is. ar->ar_data comes directly out of malloc
(reallocarray) in ensure_ibuf() and the struct is defined thusly:
struct asr_dns_header {
uint16_tid;
uint16_tflags;
uint16_tqdcount;
uint16_tancount;
uint16_tnscount;
uint16_tarcount;
};
>
> -Otto
>
--
I'm not entirely sure you are real.
ksh: diff to add tab completion for '..'
Hi!
I always found it annoying that, in ksh, doing:
$ ls ..
followed by TAB doesn't allow me to list the options (i.e. show files/dirs
in '..'). I need to do add a trailing '/' to this 'ls' command in order
to have the completions listed.
This diff makes this work without the trailing '/', but I'll be honest:
I'm not familiar with this code which does all sort of complex parsing
stuff. So... yeah, I may be breaking something somewhere.
Cheers,
--
Luís
diff 6911632ba3a60c1920af7c2d3d79a0a56f9f2d4c /usr/src
blob - 3089d195d2084b3fa81196fa359818ec914c54b0
file + bin/ksh/edit.c
--- bin/ksh/edit.c
+++ bin/ksh/edit.c
@@ -701,7 +701,10 @@ add_glob(const char *str, int slen)
if (slen < 0)
return NULL;
- toglob = str_nsave(str, slen + 1, ATEMP); /* + 1 for "*" */
+ /*
+* + 2 for '*' and for '/' if str is '..'
+*/
+ toglob = str_nsave(str, slen + 2, ATEMP); /* + 1 for "*" */
toglob[slen] = '\0';
/*
@@ -720,8 +723,15 @@ add_glob(const char *str, int slen)
saw_slash = true;
}
if (!*s && (*toglob != '~' || saw_slash)) {
- toglob[slen] = '*';
- toglob[slen + 1] = '\0';
+ /* If we're dealing with '..' */
+ if (slen == 2 && toglob[0] == '.' && toglob[1] == '.') {
+ toglob[slen] = '/';
+ toglob[slen + 1] = '*';
+ toglob[slen + 2] = '\0';
+ } else {
+ toglob[slen] = '*';
+ toglob[slen + 1] = '\0';
+ }
}
return toglob;
Some USB memory allocation cleanup
A friend of mine is trying to use two full HD cams on a OpenBSD laptop
to record his band sessions from two different angles. He can start
the first cam fine with 1920x1080, but if he tries to start the second
cam with 1920x1080 he runs out of USB memory.
That made me look in to the USB memory allocation, and it's a bit
sub-optimal currently. We have three memory pools for USB:
M_USB USB general.
M_USBDEVUSB device driver.
M_USBHC USB host controller.
A lot of USB devices drivers still use M_DEVBUF for their memory
allocations, including uvideo(4). I think we should use M_USBDEV
instead for USB device drivers, and M_USBHC for USB HC drivers.
Therefore, USB memory allocation ends up in M_DEVBUF often, while
M_USBDEV is not very much utilized. The attached diff is changing the
USB device drivers to use M_USBDEV instead of M_DEVBUF, and the USB HC
drivers to use M_USBHC instead of M_DEVBUF. Exception for the USB
device drivers are the free(9) calls if loadfirmware(9) is used.
Following an example on a xhci(4) host as of today.
Memory allocation after boot:
kern.malloc.kmemstat.devbuf: memuse = 11749K
kern.malloc.kmemstat.USB: memuse = 44K
kern.malloc.kmemstat.USB_device: memuse = 4K
kern.malloc.kmemstat.USB_HC: memuse = 0K
Memory allocation after uvideo(4) device attaches:
kern.malloc.kmemstat.devbuf: memuse = 15942K
kern.malloc.kmemstat.USB: memuse = 53K
kern.malloc.kmemstat.USB_device: memuse = 4K
kern.malloc.kmemstat.USB_HC: memuse = 0K
Memory allocation after uvideo(4) device is in use:
kern.malloc.kmemstat.devbuf: memuse = 52394K <---
kern.malloc.kmemstat.USB: memuse = 57K
kern.malloc.kmemstat.USB_device: memuse = 4K
kern.malloc.kmemstat.USB_HC: memuse = 0K
Following the same procedure with the diff applied.
Memory allocation after boot:
kern.malloc.kmemstat.devbuf: memuse = 11770K
kern.malloc.kmemstat.USB: memuse = 44K
kern.malloc.kmemstat.USB_device: memuse = 4K
kern.malloc.kmemstat.USB_HC: memuse = 1K
Memory allocation after uvideo(4) device attaches:
kern.malloc.kmemstat.devbuf: memuse = 15962K
kern.malloc.kmemstat.USB: memuse = 53K
kern.malloc.kmemstat.USB_device: memuse = 5K
kern.malloc.kmemstat.USB_HC: memuse = 1K
Memory allocation after uvideo(4) device is in use:
kern.malloc.kmemstat.devbuf: memuse = 15962K
kern.malloc.kmemstat.USB: memuse = 57K
kern.malloc.kmemstat.USB_device: memuse = 36457K <---
kern.malloc.kmemstat.USB_HC: memuse = 1K
With this I can run two full HD cams on the same host since the M_USBDEV
memory pool has usually more capacity than M_DEVBUF. Tested on a
xhci(4) and ehci(4) host.
Comments? OKs?
Cheers,
Marcus
Index: sys/dev/usb/ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.215
diff -u -p -u -p -r1.215 ehci.c
--- sys/dev/usb/ehci.c 26 Oct 2021 16:29:49 - 1.215
+++ sys/dev/usb/ehci.c 21 Nov 2021 10:51:38 -
@@ -331,7 +331,7 @@ ehci_init(struct ehci_softc *sc)
return (err);
if (ehcixfer == NULL) {
- ehcixfer = malloc(sizeof(struct pool), M_DEVBUF, M_NOWAIT);
+ ehcixfer = malloc(sizeof(struct pool), M_USBHC, M_NOWAIT);
if (ehcixfer == NULL) {
printf("%s: unable to allocate pool descriptor\n",
sc->sc_bus.bdev.dv_xname);
Index: sys/dev/usb/if_athn_usb.c
===
RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 if_athn_usb.c
--- sys/dev/usb/if_athn_usb.c 31 Oct 2021 12:24:02 - 1.62
+++ sys/dev/usb/if_athn_usb.c 21 Nov 2021 10:51:38 -
@@ -1178,7 +1178,7 @@ athn_usb_node_alloc(struct ieee80211com
{
struct athn_node *an;
- an = malloc(sizeof(struct athn_node), M_DEVBUF, M_NOWAIT | M_ZERO);
+ an = malloc(sizeof(struct athn_node), M_USBDEV, M_NOWAIT | M_ZERO);
return (struct ieee80211_node *)an;
}
Index: sys/dev/usb/if_otus.c
===
RCS file: /cvs/src/sys/dev/usb/if_otus.c,v
retrieving revision 1.69
diff -u -p -u -p -r1.69 if_otus.c
--- sys/dev/usb/if_otus.c 25 Feb 2021 02:48:20 - 1.69
+++ sys/dev/usb/if_otus.c 21 Nov 2021 10:51:39 -
@@ -884,7 +884,7 @@ otus_write_barrier(struct otus_softc *sc
struct ieee80211_node *
otus_node_alloc(struct ieee80211com *ic)
{
- return malloc(sizeof (struct otus_node), M_DEVBUF, M_NOWAIT | M_ZERO);
+ return malloc(sizeof (struct otus_node), M_USBDEV, M_NOWAIT | M_ZERO);
}
int
Index: sys/dev/usb/if_run.c
===
RCS file:
Re: snmpd(8): New application layer - step towards agentx support
On Sun, 2021-11-14 at 14:35 +, Stuart Henderson wrote: > On 2021/11/14 11:49, Martijn van Duren wrote: > > sthen@ found an issue when using this diff with netsnmp tools. > > > > The problem was that I put the requestID in the msgID, resulting > > in a mismatch upon receiving the reply. The reason that snmp(1) > > works is because msgID and requestID are the same. > > Diff below fixes things. > > This version works for me, and the runtime increase with librenms > fetches and polls (which use a mixture of get/bulkwalk) is acceptable > (10% or so). > Anyone else put this through a test? I want to move forward with this. martijn@
Re: snmpd: tweak listen on
On Sat, 2021-11-20 at 14:17 +, Stuart Henderson wrote: > On 2021/11/20 10:20, Martijn van Duren wrote: > > On Sun, 2021-11-14 at 22:30 +0100, Sebastian Benoit wrote: > > > If there is no obvious reason (i.e. be different because you need it for a > > > specific feature) why not to use the same host*() function as other > > > parse.y? > > > it would be better to stay in sync with otehrr daemons. That way if there > > > is > > > an issue in one daemon, we can fix it in all of them. > > > > > > Or, to turn the argument around: if you have found a way to improve those > > > functions in parse.y, you could push for your changes to be applied to all > > > parse.y that use the same function. > > > > > > This applies to other parse.y functions too. The more they deviate, the > > > harder maintanance becomes. > > > > This is the original message[0] (code committed had some tweaks not in this > > mail). > > In there I mention reyk's commit message saying that host_* could be merged. > > This commit tried to implement that (but apparently doesn't hold true any > > more, or maybe it never did). Moving away from struct address in host() also > > has the benefit that having different implementations of struct address to > > be more forgiving and it's less code overall. > > > > Since it took over a year to find this particular edge case I think it could > > be a good idea to push this code to the other daemons as well, but I'm short > > on time at the moment. > > > > Diff below should fix this particular issue and is easy enough to revert if > > we decide to go for the behaviour change of getaddrinfo proposed in my > > previous mail. > > ok committed, thanks. > > > I'm not afraid of moving the other way in getaddrinfo (that AI_NUMERICHOST > > is also subject to the family statement in resolv.conf), because that would > > I don't like that at all. And I don't think it matches resolv.conf's > definition of "family": > > family Specify which type of Internet protocol family to prefer, if > a host is reachable using different address families. > > the "if a host is reachable using different address families" > means that this doesn't apply for numeric addresses. (OK there could be > a side-case with CLAT on OS that don't force IPV6_V6ONLY but not on > OpenBSD). > I fully agree and that was the line of thought in my original diff. However, I still think the behaviour should be made consistent between AI_NUMERICHOST and !AI_NUMERICHOST. Iff someone were to make a solid argument for the behaviour of !AI_NUMERICHOST in AI_NUMERICHOST my code is not any more dangerous than the current host_v6 cases in the other daemons. That was my only point I wanted to make. > > > > also break all current host_v6 implementations in parse.y, so that would > > be an overhaul in all parse.y files anyway. > > > > martijn@ > > > > > > Martijn van Duren(openbsd+t...@list.imperialat.at) on 2021.11.14 00:23:59 > > > +0100: > > > > On Sat, 2021-11-13 at 13:23 +, Stuart Henderson wrote: > > > > > On 2021/08/09 20:55, Martijn van Duren wrote: > > > > > > On Mon, 2021-08-09 at 11:57 +0200, Martijn van Duren wrote: > > > > > > > > > > > > > > This diff fixes all of the above: > > > > > > > - Allow any to be used resolving to 0.0.0.0 and :: > > > > > > > - Set SO_REUSEADDR on sockets, so we can listen on both any and > > > > > > > ?? localhost > > > > > > > - Document that we listen on any by default > > > > > > > > > > I've discovered a problem with this, if you have "family inet4" or > > > > > "family inet6" in resolv.conf then startup fails, either with the > > > > > implicit listen: > > > > > > > > > > snmpd: Unexpected resolving of :: > > > > > > > > > > or with explicit e.g. "listen on any snmpv3": > > > > > > > > > > /etc/snmpd.conf:3: invalid address: any > > > > > > > > > > Config-based workaround is e.g. "listen on 0.0.0.0 snmpv3" > > > > > > > > > > Should host() use a specific ai_family instead of PF_UNSPEC where we > > > > > already know that it's a v4 or v6 address? Or just do like > > > > > httpd/parse.y > > > > > where host() tries v4, then v6 if that fails, then dns? > > > > > > > > > [0] https://marc.info/?l=openbsd-tech=159838549814986=2 > > > > Index: parse.y > > === > > RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v > > retrieving revision 1.72 > > diff -u -p -r1.72 parse.y > > --- parse.y 25 Oct 2021 11:21:32 - 1.72 > > +++ parse.y 20 Nov 2021 09:19:00 - > > @@ -1600,7 +1600,16 @@ host(const char *s, const char *port, in > > bzero(, sizeof(hints)); > > hints.ai_family = PF_UNSPEC; > > hints.ai_socktype = type; > > + /* > > +* Without AI_NUMERICHOST getaddrinfo might not resolve ip addresses > > +* for families not specified in the "family" statement in resolv.conf. > > +*/ > > + hints.ai_flags = AI_NUMERICHOST; > > error = getaddrinfo(s, port, , ); > > + if (error
Re: Explicitly tag commands in vi(1)
Hi Simon,
Simon Branch wrote on Sat, Nov 20, 2021 at 03:10:22PM -0800:
> Here's a diff that adds explicit .Tg macros to vi(1),
We don't want that:
$ man -O tag=Tg mdoc
[...]
In most cases, adding a Tg macro would be redundant because mandoc(1)
is able to automatically tag most definitions. This macro is intended
for cases where automatic tagging of a term is unsatisfactory, for
example if a definition is not tagged automatically (false negative)
or if places are tagged that do not define the term (false positives).
[...]
This is a typical example of a .Tg macro that should not be added
because it is redundant:
> +.Tg c
> .It Fl c Ar cmd
The command
$ man -O tag=c vi
already works without your patch.
I do not think .Tg should be used to enforce ambiguous tags (like "c"
pointing to both the command line option and the vi(1) "change" command).
On the one hand, adding multiple explicit .Tg macros for the same name
is very noisy. On the other hand, it is much less useful than having
an unambiguous tag because both "-O tag=c" for terminal output and the
fragment identifier "#c" in HTML output only target the first copy.
Consequently, the cost-to-benefit ratio is bad for ambiguous manual
tagging.
In general, if the same term is defined at multiple places, or if -
like in this case - multiple different terms represented by the same
word are defined in the same file, tagging does not work very well yet.
The basic concept isn't fully worked out for such complicated cases,
and i'm not even convinced designing a solution for that task that
is simple enough to be worth implementing is possible. As long as
the basic design is an unsolved problem, trying to handle individual
cases by using the .Tg macro is a bad idea.
> so that you can jump to vi or ex commands using -Otag or :t.
> This patch *should* include every command, but I couldn't figure out
> how to tag the '!' and ':!' commands; none of these worked:
>
>.Tg
>.Tg !
>.Tg !\&
>.Tg "!"
man(1) already writes the "!" tag even without your patch:
$ grep ^! /tmp/man.*
/tmp/man.JF8ZO3DibX:! /tmp/man.sZVbqIr51P 729
which alreday targets this paragraph:
! argument(s)
[range] ! argument(s)
Execute a shell command, or filter lines through a shell command.
But less(1) does not support that, see /usr/src/usr.bin/less/tags.c
line 217:
/*
* Search the tags file for the desired tag.
*/
while (fgets(tline, sizeof (tline), f) != NULL) {
if (tline[0] == '!')
/* Skip header of extended format. */
continue;
The ctags(1) format is not perfect. Some strings exist that it cannot
tag, and strings starting with an exclamation mark are an example of
such strings. Strings containing space characters are another.
> This patch doesn't tag special keys either (word erase, literal next,
> etc.), because tag names can't contain whitespace and I'd prefer
> consistency with the manpage. One solution would be to say WERASE and
> LNEXT instead, which is what ksh(1) does, and this also makes it easier
> to lookup what 'word erase' and 'literal next' really are.
>
>$ man -k Dv=WERASE Dv=LNEXT
>stty(1) - set the options for a terminal device interface
>termios(4) - general terminal line discipline
>
>$ man 4 termios -Otag=WERASE
>WERASESpecial character on input and is recognized if the ICANON
> flag is set. Erases the last word ...
Maybe; but one thing at a time.
We really should not discuss text changes in a thread about tagging.
> If a line is explicitly tagged with the .Tg macro, mandoc removes any
> other automatically-created tags with the same name. So this patch also
> explicitly tags the 'print', 'number', and 'list' options and the
> [-ceFRrSstvw] flags.
>
> Comments? OK?
Not OK. This is excessive. The .Tg macro is intended to be used
sparingly, not to be smeared all over the place.
I do not object to marking unambiguos targets that cannot be recognized
automatically. But you cannot reasonably aim for completeness in this
respect.
Yours,
Ingo
