Re: disable PPP_BSDCOMP by default
Balder Oddson wrote: > On Thu, Mar 25, 2021 at 03:40:15PM -0600, Theo de Raadt wrote: > > Stuart Henderson wrote: > > > > > > Not having read the code, case in point on principle, I'm arguing that > > > > there should be solid reasoning to enable it by default, as those with a > > > > need for it can certainly enable it and build a kernel. > > > > > > It's not that simple, using a custom kernel means you can't use syspatches > > > so are more likely to stay on a vulnerable version if a kernel bug is > > > fixed. > > > > I am perfectly happy if Balder is running a custom kernel. > > I am also happy if your kernel finds good use for the compression and > deflation code, perhaps fiddling with microcode can be of use to make > sure there is no instruction or data prefetch involved. > > Many interesting points about the immediate consequences for the code, > perhapts its re-used and touching too many things? To summarize for the public, Balder is insinuating that the PPP (historical CSLIP) compression code might be a security risk; apparently to bolster his case that we should remove it from the kernel because it (maybe) harms his strange usage case. Your proposed change will go nowhere, and increasing rhetoric is not helping the case. Balder, please got shove a fork or spoon into your eye and leave us alone. Furthermore if perforate both eyes, you won't need to see the code again!!!
Remove mandatory newline at end of armored openssh private keys
Hi All, These days I live in the automation space where armored ssh keys get loaded in various automation tools. These get pulled out as strings, added to ssh-agents or handed to ssh as identity files. I have seen at a number of clients where various parts of that break down due to tools stripping the trailing newline from these armored SSH keys, causing confusion as ssh-add or ssh says "hey that isn't a valid key." But it _looks_ valid. Side by side they are visually the same, only one is actually a byte shorter. People complain and vendors end up hard coding special checks for openssh private keys to make sure they have a trailing '\n'. This seems an issue that can be fixed in a straightforward manner upstream in sshkey.c. I was hoping there was a PROTOCOL file describing the format similar to PROTOCOL.sshsig which although "PEM" encoded explicitly has no trailing '\n' required. If there is nothing mandating that trailing '\n' for openssh I propose the following diff for discussion/consideration: Index: usr.bin/ssh/sshkey.c === RCS file: /cvs/src/usr.bin/ssh/sshkey.c,v retrieving revision 1.115 diff -u -p -u -p -r1.115 sshkey.c --- usr.bin/ssh/sshkey.c2 Feb 2021 22:36:46 - 1.115 +++ usr.bin/ssh/sshkey.c26 Mar 2021 00:33:54 - @@ -61,7 +61,7 @@ /* openssh private key file format */ #define MARK_BEGIN "-BEGIN OPENSSH PRIVATE KEY-\n" -#define MARK_END "-END OPENSSH PRIVATE KEY-\n" +#define MARK_END "-END OPENSSH PRIVATE KEY-" #define MARK_BEGIN_LEN (sizeof(MARK_BEGIN) - 1) #define MARK_END_LEN (sizeof(MARK_END) - 1) #define KDFNAME"bcrypt" As far as I can tell regress and unit tests continue to function normally. Looking at the code that preps this for the b64 decode, removing that trailing '\n' will have no impact as the code stops at the last \n before the MARK_END, ends the string and hands it to the decode. I am aware of RFC1421 (which mandates \n as part of the MARK) and RFC7468 which explicitly removes this requirement. I'm not suggesting we change code that generates these, just loosen the rules in sshkey.c so both type will be read as valid. Thanks, -Allen
Re: veb(4) exceeds 1514 byte frame size while bridge(4) doesn't?
On Tue, Mar 23, 2021 at 08:24:56PM +1000, David Gwynne wrote:
>
> On Sun, Mar 21, 2021 at 04:24:24PM +0100, Jurjen Oskam wrote:
> > Hi,
> >
> > When trying out veb(4), I ran into a situation where TCP sessions across a
> > veb(4) bridge stalled while the exact same config using bridge(4) worked
> > fine.
> >
> > After some investigation, it seems that veb(4) adds an FCS to the outgoing
> > frame, while bridge(4) doesn't. When this causes the outgoing frame to
> > exceed
> > 1514 bytes, the destination doesn't receive it.
> >
> > I must note that I was using USB NICs, one of them being quite old.
> >
> > Am I doing something wrong, or is the problem in (one of) the NIC(s)?
>
> it looks like ure(4) hardware doesn't strip the fcs before pushing it to
> the host over usb, but the ure(4) driver doesn't strip it.
>
> this usually isn't a huge deal because layers like ip just ignore
> the extra bytes. bridge(4) was ok with this because it actually
> parses ip packets and removes the extra bytes. veb(4) does a lot less
> (by design) so it just lets the fcs on the end of ure packets go out to
> other nics.
>
> from what i can tell, ure should remove the fcs. that's what this diff
> does. can you try it?
You're right, ure(4) should strip the FCS.
ok kevlo@
> cheers,
> dlg
>
> Index: if_ure.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_ure.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 if_ure.c
> --- if_ure.c 14 Oct 2020 23:47:55 - 1.21
> +++ if_ure.c 23 Mar 2021 10:18:54 -
> @@ -1896,10 +1896,17 @@ ure_rxeof(struct usbd_xfer *xfer, void *
> ifp->if_ierrors++;
> goto done;
> }
> + if (pktlen < ETHER_MIN_LEN) {
> + DPRINTF(("ethernet frame is too short\n"));
> + ifp->if_ierrors++;
> + goto done;
> + }
>
> total_len -= roundup(pktlen, URE_RX_BUF_ALIGN);
> buf += sizeof(rxhdr);
>
> + /* trim fcs */
> + pktlen -= ETHER_CRC_LEN;
> m = m_devget(buf, pktlen, ETHER_ALIGN);
> if (m == NULL) {
> DPRINTF(("unable to allocate mbuf for next packet\n"));
>
Re: disable PPP_BSDCOMP by default
On Thu, Mar 25, 2021 at 03:40:15PM -0600, Theo de Raadt wrote: > Stuart Henderson wrote: > > > > Not having read the code, case in point on principle, I'm arguing that > > > there should be solid reasoning to enable it by default, as those with a > > > need for it can certainly enable it and build a kernel. > > > > It's not that simple, using a custom kernel means you can't use syspatches > > so are more likely to stay on a vulnerable version if a kernel bug is fixed. > > I am perfectly happy if Balder is running a custom kernel. I am also happy if your kernel finds good use for the compression and deflation code, perhaps fiddling with microcode can be of use to make sure there is no instruction or data prefetch involved. Many interesting points about the immediate consequences for the code, perhapts its re-used and touching too many things?
Re: monotonic time going back by wrong skews
On Thu, Mar 25, 2021 at 07:24:23PM -0400, Josh Rickmar wrote: > On Thu, Mar 25, 2021 at 05:28:54PM -0500, Scott Cheloha wrote: > > Feel free to share your raw data. > > Attached. Hmmm, interesting stuff. $ ministat -q cpu* x cpu1-skew + cpu2-skew * cpu3-skew % cpu4-skew # cpu5-skew @ cpu6-skew O cpu7-skew N Min MaxMedian AvgStddev x 1000 1.4605543e+09 1.4605543e+09 1.4605543e+09 1.4605543e+09 -nan + 1000 1.4605543e+09 1.4605544e+09 1.4605544e+09 1.4605544e+09 -nan No difference proven at 95.0% confidence * 1000 1.4605543e+09 1.4605543e+09 1.4605543e+09 1.4605543e+09 32.397926 No difference proven at 95.0% confidence % 1000 1.4605543e+09 1.4605544e+09 1.4605544e+09 1.4605544e+09 -nan No difference proven at 95.0% confidence # 1000 1.4605543e+09 1.4605543e+09 1.4605543e+09 1.4605543e+09 45.817587 No difference proven at 95.0% confidence @ 1000 1.4605543e+09 1.4605544e+09 1.4605544e+09 1.4605544e+09 129.59171 Difference at 95.0% confidence 60.42 +/- 6.73519 4.13679e-06% +/- 4.61139e-07% (Student's t, pooled s = 76.8384) O 1000 1.4605543e+09 1.4605543e+09 1.4605543e+09 1.4605543e+09 104.98128 Difference at 95.0% confidence 5.71 +/- 4.81512 3.90947e-07% +/- 3.29678e-07% (Student's t, pooled s = 54.9334) I wish ministat wasn't NaNing the stddev but I don't have the time to work around it right now. Anyway, every AP TSC is off from the BSP by about ~1.46 billion cycles. All skews fall between 1460554280 and 1460554380. So, pretty consistent, but consistently off. You definitely won't be able to use the TSC for userspace timecounting until someone decides how to map the skews into userspace. Or maybe if someone will figure out why the TSCs on machines like yours are not synchronized at boot and devises a way to prevent it from happening at all. Thanks for the data!
cwfg: flag sensor as invalid on bogus reads
Follow-up to "arm64: make cwfg(4) report battery information to apm(4)". This driver continues to report stale hw.sensors values when reading them fails, which can easily be observed on a Pinebook Pro after plugging in the AC cable. Running on battery looks like this (note sensors and apm are in sync): $ sysctl hw.sensors hw.sensors.cwfg0.volt0=3.68 VDC (battery voltage) hw.sensors.cwfg0.raw0=104 (battery remaining minutes) hw.sensors.cwfg0.percent0=27.00% (battery percent) $ apm Battery state: high, 27% remaining, 104 minutes life estimate A/C adapter state: not known Performance adjustment mode: auto (408 MHz) When I plug in the AC cable, `raw0' jumps around considerable one or two times before stalling on the last value (note how `percent0' stayed the same while `raw0' trippled): $ sysctl hw.sensors hw.sensors.cwfg0.volt0=3.98 VDC (battery voltage) hw.sensors.cwfg0.raw0=359 (battery remaining minutes) hw.sensors.cwfg0.percent0=27.00% (battery percent) $ apm Battery state: high, 27% remaining, unknown life estimate A/C adapter state: not known Performance adjustment mode: auto (408 MHz) `raw0' aka. `CWFG_SENSOR_RTT' stops yielding valid values as long as AC is plugged in (no idea if by design or a bug). Hence hw.sensors.cwf0 values become incoherent and drift away from apm's output which --due to the reset logic discussed in the aforementioned tech@ thread-- properly picks up the stalled value as "unknown". I see two approaches to fix this: 1. simple but less user-friendly: flag sensors invalid upfront in apm's fashion and mark them OK iff they yield valid values; this is what other drivers such as rktemp(4) do, but the consequence/intention of `SENSOR_FINVALID' is sysctl(8) and systat(8) skipping such sensors, i.e. above sysctl output would omit the `raw0' line if AC is plugged in (arguably better than printing outdated/misleading values). 2. elaborate but informative: set sensor value/status to 0/unknown like acpibat(4) does for example; the advantage is that sensors no longer come and go but could look like this: hw.sensors.cwfg0.raw0=0 (battery remaining minutes), UNKNOWN I'd do prefer this but am not yet sure if that's how the sensor framework is supposed to be used and/or I'd need to tinker with the code (on multiple notebooks/sensors) to see if it works out. Either way, diff below implements the first approach such that we avoid bogus sysctl/systat lines and hw.sensors gets in sync with apm. One could still switch to the second approach afterwards. Feedback? Objections? OK? Index: cwfg.c === RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v retrieving revision 1.3 diff -u -p -r1.3 cwfg.c --- cwfg.c 25 Mar 2021 12:18:27 - 1.3 +++ cwfg.c 25 Mar 2021 12:25:31 - @@ -348,9 +348,12 @@ cwfg_update_sensors(void *arg) uint8_t val; int error, n; -#if NAPM > 0 - /* reset previous reads so apm(4) never gets stale values + /* invalidate all previous reads to avoid stale/incoherent values * in case of transient cwfg_read() failures below */ + sc->sc_sensor[CWFG_SENSOR_VCELL].flags |= SENSOR_FINVALID; + sc->sc_sensor[CWFG_SENSOR_SOC].flags |= SENSOR_FINVALID; + sc->sc_sensor[CWFG_SENSOR_RTT].flags |= SENSOR_FINVALID; +#if NAPM > 0 cwfg_power.battery_state = APM_BATT_UNKNOWN; cwfg_power.ac_state = APM_AC_UNKNOWN; cwfg_power.battery_life = 0;
Re: monotonic time going back by wrong skews
On Thu, Mar 25, 2021 at 05:28:54PM -0500, Scott Cheloha wrote: > Feel free to share your raw data. Attached. e485skews.tgz Description: application/tar-gz
Re: monotonic time going back by wrong skews
On Thu, Mar 25, 2021 at 02:33:43PM -0400, Josh Rickmar wrote:
> On Thu, Mar 25, 2021 at 01:18:04PM -0500, Scott Cheloha wrote:
> > > On Mar 24, 2021, at 8:29 AM, Josh Rickmar wrote:
> > >
> > > [...]
> >
> > Which diff did you apply? Yasuoka provided two diffs.
> >
> > In any case, ignore this diff:
> >
> > > diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> > > index 238a5a068e1..3b951a8b5a3 100644
> > > --- a/sys/arch/amd64/amd64/tsc.c
> > > +++ b/sys/arch/amd64/amd64/tsc.c
> > > @@ -212,7 +212,8 @@ cpu_recalibrate_tsc(struct timecounter *tc)
> > > u_int
> > > tsc_get_timecount(struct timecounter *tc)
> > > {
> > > - return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> > > + //return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> > > + return rdtsc_lfence();
> > > }
> > >
> > > void
> >
> >
> > We don't want to discard the skews, that's wrong.
> >
> > The reason it "fixes" Yasuoka's problem is because the real skews
> > on the ESXi VMs in question are probably close to zero but our
> > synchronization algorithm is picking huge (wrong) skews due to
> > some other variable interfering with our measurement.
>
> I had both applied. As I understood it, the first patch discarding
> the skews was a proposed fix and the second was only debug code to
> dump the skews. (Unfortunately, I don't know a way to gather this
> information from my device (thinkpad E485), as I don't believe I can
> attach to the serial console to capture the ddb output.)
Given that userland TSC doesn't work on your machine I'm curious what
your skews look like as measured by Yasuoka's patch. If you want to
gather the data and you don't have serial console access...
First enable kern.allowkmem to permit pstat(8) to extract the data
from kernel memory. You'll need to edit /etc/rc.securelevel and
reboot.
Next, run this shell script to read the skews out of kernel memory and
write them out to a file, one file per CPU. The script assumes
doas.conf(5) is written to allow passwordless root access. If you
don't have this then you can edit the script to remove doas and just
run the script as root, though I do not recommend this.
Last, disable kern.allowkmem and reboot.
Here's the script:
---
#! /bin/sh
set -e
# Make sure this is equal to TSC_SYNC_NTIMES!
nsamples=1000
# Each sample is a signed 32-bit integer (4 bytes).
samplesz=4
# Find the starting address of the skew array in memory.
base=$(doas pstat -d lld tsc_difs | tr -d ':' | cut -d ' ' -f 3)
for cpu in $(jot $(sysctl -n hw.ncpu) 0); do
# Ignore the BSP.
if [ $cpu -eq 0 ]; then
continue
fi
# Compute the start of the given CPU's samples in the array.
start=$((base + cpu * nsamples * samplesz))
end=$((start + nsamples * samplesz))
i=$start
# Print each sample to the file ./cpuN-skew
out="./cpu$cpu-skew"
rm -f $out
while [ $i -lt $end ]; do
doas pstat -d d $(printf "0x%x" $i) | cut -d ' ' -f 4 >> $out
i=$((i + samplesz))
done
done
---
My laptop has eight logical CPUs. I get this:
$ sh script.sh
$ wc -l cpu*
1000 cpu1-skew
1000 cpu2-skew
1000 cpu3-skew
1000 cpu4-skew
1000 cpu5-skew
1000 cpu6-skew
1000 cpu7-skew
7000 total
ministat yields this:
x cpu1-skew
+ cpu2-skew
* cpu3-skew
% cpu4-skew
# cpu5-skew
@ cpu6-skew
O cpu7-skew
N Min MaxMedian AvgStddev
x 1000 -12 7 0-0.701 2.93568
+ 1000 -12 7 0-0.476 2.8927136
* 1000 -12 3-3-3.251 1.8898599
% 1000 -2015-3-4.235 6.0109822
# 1000 -23 9-1-1.482 3.630545
@ 1000 -18 8 1-0.563 4.7706173
O 1000 -19 6-6-5.527 3.6647922
Feel free to share your raw data.
> Clock doesn't go backwards with only the second diff.
Good.
Superflouous memcpy() in vmctl/main.c
Looking through the code for vmctl, I came across a repeated memcpy() in
vmctl/main.c.
In the checks below, ret is either set by a memcpy() or defaulted to 0.
If set by memcpy(), and ret != 0, the memcpy() is repeated verbatim,
which seems unnecessary.
diff 09b708f572d76de8db7f7948ea7359b19bbd1c5a /usr/src
blob - 249eaa3ded1ee9c804a81874613c292a74ea4b21
file + usr.sbin/vmctl/main.c
--- usr.sbin/vmctl/main.c
+++ usr.sbin/vmctl/main.c
@@ -300,13 +300,12 @@ vmmaction(struct parse_result *res)
if (imsg.hdr.type == IMSG_CTL_FAIL) {
if (IMSG_DATA_SIZE() == sizeof(ret))
memcpy(, imsg.data, sizeof(ret));
else
ret = 0;
if (ret != 0) {
- memcpy(, imsg.data, sizeof(ret));
errno = ret;
err(1, "command failed");
} else
errx(1, "command failed");
}
Re: disable PPP_BSDCOMP by default
Stuart Henderson wrote: > > Not having read the code, case in point on principle, I'm arguing that > > there should be solid reasoning to enable it by default, as those with a > > need for it can certainly enable it and build a kernel. > > It's not that simple, using a custom kernel means you can't use syspatches > so are more likely to stay on a vulnerable version if a kernel bug is fixed. I am perfectly happy if Balder is running a custom kernel.
Re: disable PPP_BSDCOMP by default
On 2021/03/25 20:53, Balder Oddson wrote: > On Thu, Mar 25, 2021 at 07:09:37PM +0100, Balder Oddson wrote: > > Compression in PPP was great in the age of ISDN to increase speeds. > > The more common use cases, and trends concerning TLS1.3 advancements. > > > > Having this enabled by default, and infrequently used could lead to > > unintended consequences around how the data is passed around. > > > Off list it has been suggested that this is ridiciolous. > And perhaps this is, especially for the justifications given, thought be > sufficient. > > From a UNIX architectural perspectiv, adhering to things like > "everything is a file". > > PPP device are not physical devices that can be used for a PPP > connection, where it is more trivial with a practice to do compression > and decompression before presenting the payload to the kernel so its > forced to go through the usual barriers. > > Back in the day, when everything was a PPP connection for most people, > this made good sense and also gave good performance. > > Having code in the kernel that is even better than say, base64 that can > detangle a malicious payload isn't the point here. There shouldn't be > targetable code in the kernel for these kinds of tasks that can be > exploited. And the task should preferably be something outside of kernel > space or in a real device driver. I don't quite understand what you're saying here but it sounds like you're trying to remove one of the copies of libz is used so it can't be accessed from the kernel, is that it? What's the point, there are others? Do you want to remove hibernate as well? > Not having read the code, case in point on principle, I'm arguing that > there should be solid reasoning to enable it by default, as those with a > need for it can certainly enable it and build a kernel. It's not that simple, using a custom kernel means you can't use syspatches so are more likely to stay on a vulnerable version if a kernel bug is fixed. > Maybe this isn't a huge concern for the network parts of the code, but > as soon as there is multiprocessing and desktop applications involved, > it becomes increasingly unattractive. What's the technical reasons for > this code in 2020? > > option INET6 # IPv6 > > option IPSEC # IPsec > > -option PPP_BSDCOMP # PPP BSD compression > > -option PPP_DEFLATE > > +#optionPPP_BSDCOMP # PPP BSD compression > > +#optionPPP_DEFLATE # Disabled by default, TLS1.3 trends This comment doesn't make any sense > > option PIPEX # Ppp IP EXtension, for npppd > > option MROUTING# Multicast router > > option MPLS# Multi-Protocol Label Switching > > -- > Balder Oddson >
Re: Permit reading kern.somaxconn with unix pledge
Theo de Raadt writes:
> I have reviewed all the pledge using programs in the tree, and I do not
> see additional risk from this change.
>
> Who wants to take care of the commit?
I'll snag it!
>
> Josh Rickmar wrote:
>
>> The kern.somaxconn sysctl was previously permitted under the inet
>> pledge, which allowed pledged Go applications to listen on AF_INET and
>> AF_INET6 domains.
>>
>> https://marc.info/?l=openbsd-tech=158069595809463=2
>> https://marc.info/?l=openbsd-cvs=158081099810301=2
>>
>> But Go will also read this sysctl when only using unix domain sockets.
>> The patch below additionally permits reading this sysctl if the unix
>> pledge is granted.
>>
>> Note that for this to be tested and useful (where useful means not
>> running with the inet pledge), Go's net package also needs a patch:
>> https://gist.github.com/jrick/878236e2f3735d35d5a737936439cb81
>>
>> diff b17f936e67043f9c006633bac4e3630f86dd05c2 /usr/src
>> blob - 9ffb7f2ffb9d05d6dd741e180b62141fb5e91f0b
>> file + sys/kern/kern_pledge.c
>> --- sys/kern/kern_pledge.c
>> +++ sys/kern/kern_pledge.c
>> @@ -888,7 +888,7 @@ pledge_sysctl(struct proc *p, int miblen, int *mib, vo
>> return (0);
>> }
>>
>> -if ((p->p_p->ps_pledge & PLEDGE_INET)) {
>> +if ((p->p_p->ps_pledge & (PLEDGE_INET | PLEDGE_UNIX))) {
>> if (miblen == 2 && /* kern.somaxconn */
>> mib[0] == CTL_KERN && mib[1] == KERN_SOMAXCONN)
>> return (0);
>>
Re: Permit reading kern.somaxconn with unix pledge
I have reviewed all the pledge using programs in the tree, and I do not
see additional risk from this change.
Who wants to take care of the commit?
Josh Rickmar wrote:
> The kern.somaxconn sysctl was previously permitted under the inet
> pledge, which allowed pledged Go applications to listen on AF_INET and
> AF_INET6 domains.
>
> https://marc.info/?l=openbsd-tech=158069595809463=2
> https://marc.info/?l=openbsd-cvs=158081099810301=2
>
> But Go will also read this sysctl when only using unix domain sockets.
> The patch below additionally permits reading this sysctl if the unix
> pledge is granted.
>
> Note that for this to be tested and useful (where useful means not
> running with the inet pledge), Go's net package also needs a patch:
> https://gist.github.com/jrick/878236e2f3735d35d5a737936439cb81
>
> diff b17f936e67043f9c006633bac4e3630f86dd05c2 /usr/src
> blob - 9ffb7f2ffb9d05d6dd741e180b62141fb5e91f0b
> file + sys/kern/kern_pledge.c
> --- sys/kern/kern_pledge.c
> +++ sys/kern/kern_pledge.c
> @@ -888,7 +888,7 @@ pledge_sysctl(struct proc *p, int miblen, int *mib, vo
> return (0);
> }
>
> - if ((p->p_p->ps_pledge & PLEDGE_INET)) {
> + if ((p->p_p->ps_pledge & (PLEDGE_INET | PLEDGE_UNIX))) {
> if (miblen == 2 && /* kern.somaxconn */
> mib[0] == CTL_KERN && mib[1] == KERN_SOMAXCONN)
> return (0);
>
Re: vmd(8) support for gzipped kernels
Josh Rickmar writes:
> On Fri, Mar 19, 2021 at 10:29:10AM -0400, Josh Rickmar wrote:
>> Here's an updated version of the patch I had originally posted to
>> bugs@ adding support for reading gzipped kernels (needed to boot amd64
>> bsd.rd without manually decompressing first), now that the support for
>> booting a kernel discovered on a ffs filesystem in the image file is
>> removed.
>>
>> I've kept the gzFile arguments named 'fp' to reduce the diff; let me
>> know if this should be changed to e.g. 'f' or 'gzf' so as to not
>> confuse it with FILE *.
I've tested the original diff and it works for me.
Anyone else?
>
> Small update: removed from vmd.h since it's no longer
> needed, and add missing to vioqcow2.c.
There are numerous includes that could be cleaned up. That's a separate
issue.
>
> diff a13de4d12a4c9ba0edc05aab2ad635f782449229 /usr/src
> blob - 132221fb960ae8a9184aaeb7b26669d7d715bdf1
> file + usr.sbin/vmd/Makefile
> --- usr.sbin/vmd/Makefile
> +++ usr.sbin/vmd/Makefile
> @@ -14,8 +14,8 @@ CFLAGS+=-Wmissing-declarations
> CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
> CFLAGS+= -Wsign-compare
>
> -LDADD+= -lutil -lpthread -levent
> -DPADD+= ${LIBUTIL} ${LIBPTHREAD} ${LIBEVENT}
> +LDADD+= -lutil -lpthread -levent -lz
> +DPADD+= ${LIBUTIL} ${LIBPTHREAD} ${LIBEVENT} ${LIBZ}
>
> YFLAGS=
>
> blob - 43b79cf6762f77c761723c2189546e9a7fafd79f
> file + usr.sbin/vmd/loadfile.h
> --- usr.sbin/vmd/loadfile.h
> +++ usr.sbin/vmd/loadfile.h
> @@ -30,6 +30,8 @@
> * POSSIBILITY OF SUCH DAMAGE.
> */
>
> +#include
> +
> /*
> * Array indices in the u_long position array
> */
> @@ -73,6 +75,6 @@
> #define PML2_PAGE 0x13000
> #define NPTE_PG (PAGE_SIZE / sizeof(uint64_t))
>
> -int loadfile_elf(FILE *, struct vm_create_params *, struct vcpu_reg_state *);
> +int loadfile_elf(gzFile, struct vm_create_params *, struct vcpu_reg_state *);
>
> -size_t mread(FILE *, paddr_t, size_t);
> +size_t mread(gzFile, paddr_t, size_t);
> blob - 8485ac59ccbc3459d37db1c6e2660b6862b11bd8
> file + usr.sbin/vmd/loadfile_elf.c
> --- usr.sbin/vmd/loadfile_elf.c
> +++ usr.sbin/vmd/loadfile_elf.c
> @@ -115,8 +115,8 @@ union {
>
> static void setsegment(struct mem_segment_descriptor *, uint32_t,
> size_t, int, int, int, int);
> -static int elf32_exec(FILE *, Elf32_Ehdr *, u_long *, int);
> -static int elf64_exec(FILE *, Elf64_Ehdr *, u_long *, int);
> +static int elf32_exec(gzFile, Elf32_Ehdr *, u_long *, int);
> +static int elf64_exec(gzFile, Elf64_Ehdr *, u_long *, int);
> static size_t create_bios_memmap(struct vm_create_params *, bios_memmap_t *);
> static uint32_t push_bootargs(bios_memmap_t *, size_t);
> static size_t push_stack(uint32_t, uint32_t);
> @@ -260,10 +260,11 @@ push_pt_64(void)
> *
> * Return values:
> * 0 if successful
> - * various error codes returned from read(2) or loadelf functions
> + * various error codes returned from gzread(3) or loadelf functions
> */
> int
> -loadfile_elf(FILE *fp, struct vm_create_params *vcp, struct vcpu_reg_state
> *vrs)
> +loadfile_elf(gzFile fp, struct vm_create_params *vcp,
> +struct vcpu_reg_state *vrs)
> {
> int r, is_i386 = 0;
> uint32_t bootargsz;
> @@ -271,7 +272,7 @@ loadfile_elf(FILE *fp, struct vm_create_params *vcp, s
> u_long marks[MARK_MAX];
> bios_memmap_t memmap[VMM_MAX_MEM_RANGES + 1];
>
> - if ((r = fread(, 1, sizeof(hdr), fp)) != sizeof(hdr))
> + if ((r = gzread(fp, , sizeof(hdr))) != sizeof(hdr))
> return 1;
>
> memset(, 0, sizeof(marks));
> @@ -471,7 +472,7 @@ push_stack(uint32_t bootargsz, uint32_t end)
> * into the guest address space at paddr 'addr'.
> *
> * Parameters:
> - * fd: file descriptor of the kernel image file to read from.
> + * fp: kernel image file to read from.
> * addr: guest paddr_t to load to
> * sz: number of bytes to load
> *
> @@ -479,7 +480,7 @@ push_stack(uint32_t bootargsz, uint32_t end)
> * returns 'sz' if successful, or 0 otherwise.
> */
> size_t
> -mread(FILE *fp, paddr_t addr, size_t sz)
> +mread(gzFile fp, paddr_t addr, size_t sz)
> {
> size_t ct;
> size_t i, rd, osz;
> @@ -499,7 +500,7 @@ mread(FILE *fp, paddr_t addr, size_t sz)
> else
> ct = sz;
>
> - if (fread(buf, 1, ct, fp) != ct) {
> + if ((size_t)gzread(fp, buf, ct) != ct) {
> log_warn("%s: error %d in mread", __progname, errno);
> return (0);
> }
> @@ -523,7 +524,7 @@ mread(FILE *fp, paddr_t addr, size_t sz)
> else
> ct = PAGE_SIZE;
>
> - if (fread(buf, 1, ct, fp) != ct) {
> + if ((size_t)gzread(fp, buf, ct) != ct) {
> log_warn("%s: error %d in mread", __progname, errno);
> return (0);
> }
> @@ -628,13 +629,13 @@ mbcopy(void *src, paddr_t dst, int sz)
> /*
>
Re: vmd(8): fix packet handling for dhcpleased(8)
Florian Obser writes: > This might not be a problem in practice. Agreed specifically on the renewal issue. The subtle 1 line change to process all packets in the tx ring is a different issue that so far nobody has reported observing. > > vmd(8) hands us a lease with "infinity" lease time. This is expresed > as UINT32_MAX, i.e. 2^32-1. dhcpleased(8) does not handle infinity > explicitly, it's just a very long lease time (136 years). > > When we configure the lease we enter the BOUND state. After 0.5 * > lease time (T1) we transition to RENEWING: > > RFC 2131, 4.3.2 DHCPREQUEST message: > >o DHCPREQUEST generated during RENEWING state: > > 'server identifier' MUST NOT be filled in, 'requested IP address' > option MUST NOT be filled in, 'ciaddr' MUST be filled in with > client's IP address. In this situation, the client is completely > configured, and is trying to extend its lease. *This message will > be unicast*, so no relay agents will be involved in its > transmission. Because 'giaddr' is therefore not filled in, the > DHCP server will trust the value in 'ciaddr', and use it when > replying to the client. > I'll have to go through the "poor-man's dhcp server" in vmd(8) and check against the RFC to see if it's doing any other oddities. There's at least one superflous thing I can see for another diff. > This is the only state where we send unicast messages. > > After 0.875 * lease_time (T2) we transition to REBINDING which is > again broadcast. > > Now these are very long timeouts. In particular we transition to > RENEWING after 68 years... > > One can trigger a transition from BOUND to renewing via > dhcpleasectl(8)'s send request command. It will basically do the next > state transition, in this case to RENEWING and will be stuck there > because vmd will ignore out request. > > Our lease is however still valid, so everything "just works". > > Maybe the problem is with the send request command. I don't know yet > what to do with it. Maybe it should transition to INIT state. This is > what dhclient(8) is doing when one re-starts it on an interface. > > So vmd(8) is not behaving correctly as dhcp server. I don't know if > this needs fixing though if it's too complicated. I don't feel the change is complicated. It's mostly reyk-ian privsep plumbing. I know the built-in dhcp feature of vmd(8) is heavily used so I'm concerned that it behave close to whatever is expected of a dhcp server to minimize guest software reporting problems. > > On Wed, Mar 24, 2021 at 05:47:36PM -0400, Dave Voutila wrote: >> tech@, >> >> While migrating an existing -current vm to use dhcpleased(8), one of the >> issues discovered was the dhcp/bootp handling built into vmd(8) for >> local interfaces was improperly missing packets sent to the ethernet >> address of the host-side tap(4) device. (vmd(8) was only looking for >> broadcast packets.) >> >> The following diff: >> - includes the host-side tap(4) lladdr in the packet filtering logic for >> intercepting vio(4) dhcp/bootp communication >> - removes a conditional check (dhcpsz == 0) pointed out by deraadt@ that >> could contribute to missed packets while iterating through the vionet >> tx ring >> >> Because of vmd(8)'s priv-sep design, the approach taken is to pass a >> duplicate of the opened tap(4) fd to the "priv" process that is >> unpledged and responsible for setting up networking. A response imsg >> travels between processes, being forwarded until it gets to the intended >> vm process. >> >> For those looking to test the diff, some steps to follow are: >> >> 0. Don't apply the patch yet. >> 1. Use an OpenBSD guest running a recent snapshot that has the latest >> dhcpleased(8)[1] and dhcpleasectl(8)[2] changes committed by florian@. >> 2. Configure the guest for using dhcpleased(8), ideally via >> /etc/hostname.vio0 containing: >> >> inet autoconf >> up >> >> 3. Ensure you get an IP assigned from vmd(8) after boot. >> 4. Check the lease with `dhcpleasectl show interface vio0`. It should >>report [Bound] and have a _very_ long lease time. >> 5. Force a renewal with `dhcpleasectl send request vio0`. >> 6. Check again like in step 4. It will be "stuck" in a [Renewing] state. >> 7. Shut down the guest, vmd(8), and apply the patch. >> 8. Repeat steps 4-6, but the guest should no longer be "stuck" in >>[Renewing] and should report [Bound] after forced renewal. >> >> You can also turn up debug logging for vmd(8) and should see >> corresponding messages of: >> >> vionet: dhcp request, local response size 342 >> >> If you do not, the packet was not intercepted by vmd(8). >> >> I have tested this diff with a variety of conditions, including multiple >> emulated nics per guest. Specifically 1 nic using dhcp/bootp resolved by >> vmd(8) and the other nic connected to a virtual switch using veb(4) and >> an instance of dhcpd(8) running on the host. Also ran a snapshot upgrade >> with no issues. >> >> So in
Re: vmctl: off-by-one error handling mixing -a with a VM id
Theo Buehler wrote:
> However, I think the current logic is both wrong and the wrong way
> around. I believe the following is much clearer. It doesn't have a dead
> else branch and it deletes 'ret', so it doesn't use it uninitialized when
> checking 'res->action == CMD_STOPALL && ret != -1' (e.g. 'vmctl stop -a').
> Since the diff is slightly messy, this is the result:
> if (res->action == CMD_STOPALL) {
> if (argc != 0)
> ctl_usage(res->ctl);
> } else {
> if (argc != 1)
> ctl_usage(res->ctl);
> if (parse_vmid(res, argv[0], 0) == -1)
> errx(1, "invalid id: %s", argv[0]);
> }
FWIW, I agree this is clearer.
Thanks for the feedback.
Preben
Re: Permit reading kern.somaxconn with unix pledge
On Mon, Feb 01, 2021 at 08:18:53PM +, Josh Rickmar wrote:
> The kern.somaxconn sysctl was previously permitted under the inet
> pledge, which allowed pledged Go applications to listen on AF_INET and
> AF_INET6 domains.
>
> https://marc.info/?l=openbsd-tech=158069595809463=2
> https://marc.info/?l=openbsd-cvs=158081099810301=2
>
> But Go will also read this sysctl when only using unix domain sockets.
> The patch below additionally permits reading this sysctl if the unix
> pledge is granted.
>
> Note that for this to be tested and useful (where useful means not
> running with the inet pledge), Go's net package also needs a patch:
> https://gist.github.com/jrick/878236e2f3735d35d5a737936439cb81
>
> diff b17f936e67043f9c006633bac4e3630f86dd05c2 /usr/src
> blob - 9ffb7f2ffb9d05d6dd741e180b62141fb5e91f0b
> file + sys/kern/kern_pledge.c
> --- sys/kern/kern_pledge.c
> +++ sys/kern/kern_pledge.c
> @@ -888,7 +888,7 @@ pledge_sysctl(struct proc *p, int miblen, int *mib, vo
> return (0);
> }
>
> - if ((p->p_p->ps_pledge & PLEDGE_INET)) {
> + if ((p->p_p->ps_pledge & (PLEDGE_INET | PLEDGE_UNIX))) {
> if (miblen == 2 && /* kern.somaxconn */
> mib[0] == CTL_KERN && mib[1] == KERN_SOMAXCONN)
> return (0);
Ping.
The necessary Go patch just landed in their development branch, and
should appear in Go 1.17 at the very latest.
Re: apmd: log ioctl failures
Klemens Nanni(k...@openbsd.org) on 2021.03.25 19:45:04 +0100: > On Sat, Mar 20, 2021 at 07:46:38PM +0100, Klemens Nanni wrote: > > Otherwise there is no way other than reading driver code to determine > > why running zzz(8) for example does not do anything on certain machines. > > > > apm(4/arm64) for one currently does not implement suspend and resume, > > i.e. it yields ENOSUPP which gets lost in userland. > > > > This still does not make `zzz' or `apm -z' report the informative > > warning, but syslog now has it: > > > > $ zzz > > Suspending system... > > $ tail -n3 /var/log/messages > > Mar 20 19:16:57 pine64 apmd: system suspending > > Mar 20 19:16:57 pine64 apmd: battery status: unknown. external power > > status: not known. estimated battery life 0% > > Mar 20 19:16:58 pine64 apmd: suspend: Operation not supported > > > > > > Feedback? OK? > Anyone? > > This is a trivial but helpful change, just like the ones in > these other mails on tech@: > "apm/macppc: Mark unsupported ioctl as such, merge cases" > "apm/arm64: fix errno, merge ioctl cases" reads ok and yeah, it might be useful to know if this doesnt work. ok benno@ > > > Index: apmd.c > === > RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v > retrieving revision 1.101 > diff -u -p -r1.101 apmd.c > --- apmd.c16 Mar 2021 09:00:43 - 1.101 > +++ apmd.c20 Mar 2021 18:47:13 - > @@ -329,7 +329,8 @@ suspend(int ctl_fd) > do_etc_file(_PATH_APM_ETC_SUSPEND); > sync(); > sleep(1); > - ioctl(ctl_fd, APM_IOC_SUSPEND, 0); > + if (ioctl(ctl_fd, APM_IOC_SUSPEND, 0) == -1) > + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); > } > > void > @@ -340,7 +341,8 @@ stand_by(int ctl_fd) > do_etc_file(_PATH_APM_ETC_STANDBY); > sync(); > sleep(1); > - ioctl(ctl_fd, APM_IOC_STANDBY, 0); > + if (ioctl(ctl_fd, APM_IOC_STANDBY, 0) == -1) > + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); > } > > void > @@ -351,7 +353,8 @@ hibernate(int ctl_fd) > do_etc_file(_PATH_APM_ETC_HIBERNATE); > sync(); > sleep(1); > - ioctl(ctl_fd, APM_IOC_HIBERNATE, 0); > + if (ioctl(ctl_fd, APM_IOC_HIBERNATE, 0) == -1) > + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); > } > > void >
Re: vmctl: off-by-one error handling mixing -a with a VM id
On Thu, Mar 25, 2021 at 08:07:53PM +0100, Preben Guldberg wrote:
> Dave Voutila wrote:
> > Preben Guldberg writes:
> > > The patch below addresses an off-by-one error reading argv when
> > > generating the error message.
>
> > > I personally find it clearer if the condition of mixing -a with an id
> > > is highlighted. I included a suggestion in the patch below.
>
> > Since -a and providing an id are mutually exclusive, I think it's more
> > helpful to print usage information via ctl_usage(res->ctl). From the
> > usage details, it's self explanatory what's wrong.
>
> > usage: vmctl [-v] stop [-fw] [id | -a]
>
> The updated diff below would do just that:
>
> % vmctl stop -a testvm
> usage: vmctl [-v] stop [-fw] [id | -a]
Yes, your diff would do that.
However, I think the current logic is both wrong and the wrong way
around. I believe the following is much clearer. It doesn't have a dead
else branch and it deletes 'ret', so it doesn't use it uninitialized when
checking 'res->action == CMD_STOPALL && ret != -1' (e.g. 'vmctl stop -a').
Since the diff is slightly messy, this is the result:
if (res->action == CMD_STOPALL) {
if (argc != 0)
ctl_usage(res->ctl);
} else {
if (argc != 1)
ctl_usage(res->ctl);
if (parse_vmid(res, argv[0], 0) == -1)
errx(1, "invalid id: %s", argv[0]);
}
return (vmmaction(res));
Index: main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.62
diff -u -p -r1.62 main.c
--- main.c 3 Jan 2020 05:32:00 - 1.62
+++ main.c 25 Mar 2021 19:23:16 -
@@ -927,7 +927,7 @@ ctl_start(struct parse_result *res, int
int
ctl_stop(struct parse_result *res, int argc, char *argv[])
{
- int ch, ret;
+ int ch;
while ((ch = getopt(argc, argv, "afw")) != -1) {
switch (ch) {
@@ -948,20 +948,15 @@ ctl_stop(struct parse_result *res, int a
argc -= optind;
argv += optind;
- if (argc == 0) {
- if (res->action != CMD_STOPALL)
+ if (res->action == CMD_STOPALL) {
+ if (argc != 0)
ctl_usage(res->ctl);
- } else if (argc > 1)
- ctl_usage(res->ctl);
- else if (argc == 1)
- ret = parse_vmid(res, argv[0], 0);
- else
- ret = -1;
-
- /* VM id is only expected without the -a flag */
- if ((res->action != CMD_STOPALL && ret == -1) ||
- (res->action == CMD_STOPALL && ret != -1))
- errx(1, "invalid id: %s", argv[1]);
+ } else {
+ if (argc != 1)
+ ctl_usage(res->ctl);
+ if (parse_vmid(res, argv[0], 0) == -1)
+ errx(1, "invalid id: %s", argv[0]);
+ }
return (vmmaction(res));
}
Re: disable PPP_BSDCOMP by default
On Thu, Mar 25, 2021 at 07:09:37PM +0100, Balder Oddson wrote: > Compression in PPP was great in the age of ISDN to increase speeds. > The more common use cases, and trends concerning TLS1.3 advancements. > > Having this enabled by default, and infrequently used could lead to > unintended consequences around how the data is passed around. Off list it has been suggested that this is ridiciolous. And perhaps this is, especially for the justifications given, thought be sufficient. >From a UNIX architectural perspectiv, adhering to things like "everything is a file". PPP device are not physical devices that can be used for a PPP connection, where it is more trivial with a practice to do compression and decompression before presenting the payload to the kernel so its forced to go through the usual barriers. Back in the day, when everything was a PPP connection for most people, this made good sense and also gave good performance. Having code in the kernel that is even better than say, base64 that can detangle a malicious payload isn't the point here. There shouldn't be targetable code in the kernel for these kinds of tasks that can be exploited. And the task should preferably be something outside of kernel space or in a real device driver. Not having read the code, case in point on principle, I'm arguing that there should be solid reasoning to enable it by default, as those with a need for it can certainly enable it and build a kernel. Maybe this isn't a huge concern for the network parts of the code, but as soon as there is multiprocessing and desktop applications involved, it becomes increasingly unattractive. What's the technical reasons for this code in 2020? > > > Index: GENERIC > === > RCS file: /cvs/src/sys/conf/GENERIC,v > retrieving revision 1.274 > diff -u -p -u -p -r1.274 GENERIC > --- GENERIC 25 Feb 2021 01:19:35 - 1.274 > +++ GENERIC 25 Mar 2021 18:07:58 - > @@ -50,8 +50,8 @@ option TCP_SIGNATURE # TCP MD5 Signatur > > option INET6 # IPv6 > option IPSEC # IPsec > -option PPP_BSDCOMP # PPP BSD compression > -option PPP_DEFLATE > +#option PPP_BSDCOMP # PPP BSD compression > +#option PPP_DEFLATE # Disabled by default, TLS1.3 trends > option PIPEX # Ppp IP EXtension, for npppd > option MROUTING# Multicast router > option MPLS# Multi-Protocol Label Switching -- Balder Oddson
mg list-buffers
This is the current format of the output of the mg 'list-buffer' command: MR Buffer Size File -- -- *Buffer List* 0 .** file1.c 6810 /tmp/file1.c file2.c 7105 /tmp/file2.c * *scratch* 0 The 'M' column indicates if the buffer is modified by having an asterisk next to the buffer name. The 'R' column indicates if a buffer is read-only by not having an asterisk in the 'R' column. Therefor the files above with an asterisk in the column next to them are writable. Isn't that unintuitive? Also, perhaps less confusing than the 'R' column, is the first column (note it doesn't have a header character), it has a '.' next to 'file1.c'. The '.' indicates that this is the active buffer in mg. Could a different character indicate the active buffer? The diff below makes the state of buffers above look like this: MR Buffer Size File -- -- * *Buffer List* 0 * file1.c 6810 /tmp/file1.c * file2.c 7105 /tmp/file2.c *scratch* 0 Read-only buffers have an asterisk in the 'R' column and I'm using a '>' character to indicate the active buffer. Is that better? Also, since I had to check the code to see what 'R' meant (I guessed 'M'), and I didn't even notice the '.' character (I thought it was a dodgy pixel), I have added a short explaination to the man page about the 3 columns. Objectsions, oks? Index: buffer.c === RCS file: /cvs/src/usr.bin/mg/buffer.c,v retrieving revision 1.111 diff -u -p -r1.111 buffer.c --- buffer.c23 Mar 2021 18:40:29 - 1.111 +++ buffer.c25 Mar 2021 15:08:36 - @@ -368,9 +368,9 @@ makelist(void) } if (addlinef(blp, "%c%c%c %-*.*s%c%-6d %-*s", - (bp == curbp) ? '.' : ' ', /* current buffer ? */ + (bp == curbp) ? '>' : ' ', /* current buffer ? */ ((bp->b_flag & BFCHG) != 0) ? '*' : ' ', /* changed ? */ - ((bp->b_flag & BFREADONLY) != 0) ? ' ' : '*', + ((bp->b_flag & BFREADONLY) != 0) ? '*' : ' ', w - 5, /* four chars already written */ w - 5, /* four chars already written */ bp->b_bname, /* buffer name */ Index: mg.1 === RCS file: /cvs/src/usr.bin/mg/mg.1,v retrieving revision 1.121 diff -u -p -r1.121 mg.1 --- mg.120 Mar 2021 09:00:49 - 1.121 +++ mg.125 Mar 2021 15:08:36 - @@ -692,6 +692,11 @@ directory. Toggle whether the line number is displayed in the modeline. .It list-buffers Display the list of available buffers. +The first column in the output indicates which buffer is active with a '>' +character. +The second column indicates which buffers are modified. +The third column indicates which buffers are read-only. +The remaining columns are self-explanatory. .It load Prompt the user for a filename, and then execute commands from that file.
Re: apm, apmd: ship manual pages on powerpc64
On Sun, Mar 21, 2021 at 01:24:49PM +0100, Klemens Nanni wrote: > On Sun, Mar 21, 2021 at 06:50:23AM +, Jason McIntyre wrote: > > On Sat, Mar 20, 2021 at 07:29:11PM -0600, Theo de Raadt wrote: > > > There is a pattern we've followed in the past, that when a manpage applies > > > to more than 2 (or 3?) architectures, then we simply make it MI. > > > > > > So MANSUBDIR would get deleted, and then the sets would be updated. > That makes sense. > > > > People with old systems will retain the old files, but I think man(1) > > > selects the MI ones over the MD ones (I think we have no override > > > capability). > > > > > > > man chooses the MD page over the MI page, i think. from man(1): > > > > Machine specific areas are searched before general areas. > > > > so you'd get the old page unless you deleted it. i don;t know if you can > > override that. > Isn't that what we have current.html and sysclean(1) for? > > If so, I'd commit the following and mention the MD manuals for removal > in current.html just like we do when removing old files after upgrading > perl or so. > > Feedback? OK? Ping. Here's both the apm and current.html diff. Index: usr.sbin/apm/Makefile === RCS file: /cvs/src/usr.sbin/apm/Makefile,v retrieving revision 1.19 diff -u -p -r1.19 Makefile --- usr.sbin/apm/Makefile 15 Jul 2020 22:46:52 - 1.19 +++ usr.sbin/apm/Makefile 21 Mar 2021 12:21:33 - @@ -19,6 +19,5 @@ NOPROG=yes .endif MAN= apm.8 -MANSUBDIR=arm64 amd64 i386 loongson macppc sparc64 .include Index: usr.sbin/apmd/Makefile === RCS file: /cvs/src/usr.sbin/apmd/Makefile,v retrieving revision 1.15 diff -u -p -r1.15 Makefile --- usr.sbin/apmd/Makefile 15 Jul 2020 22:46:52 - 1.15 +++ usr.sbin/apmd/Makefile 21 Mar 2021 12:23:15 - @@ -13,6 +13,5 @@ NOPROG=yes .endif MAN= apmd.8 -MANSUBDIR= arm64 amd64 i386 loongson macppc sparc64 .include Index: distrib/sets/lists/man/mi === RCS file: /cvs/src/distrib/sets/lists/man/mi,v retrieving revision 1.1616 diff -u -p -r1.1616 mi --- distrib/sets/lists/man/mi 1 Mar 2021 23:26:59 - 1.1616 +++ distrib/sets/lists/man/mi 21 Mar 2021 12:21:03 - @@ -2296,8 +2296,6 @@ ./usr/share/man/man8/alpha/setnetbootinfo.8 ./usr/share/man/man8/amd.8 ./usr/share/man/man8/amd64/MAKEDEV.8 -./usr/share/man/man8/amd64/apm.8 -./usr/share/man/man8/amd64/apmd.8 ./usr/share/man/man8/amd64/biosboot.8 ./usr/share/man/man8/amd64/boot.8 ./usr/share/man/man8/amd64/boot_amd64.8 @@ -2307,9 +2305,9 @@ ./usr/share/man/man8/amd64/memconfig.8 ./usr/share/man/man8/amd64/pxeboot.8 ./usr/share/man/man8/amq.8 +./usr/share/man/man8/apm.8 +./usr/share/man/man8/apmd.8 ./usr/share/man/man8/arm64/MAKEDEV.8 -./usr/share/man/man8/arm64/apm.8 -./usr/share/man/man8/arm64/apmd.8 ./usr/share/man/man8/arm64/eeprom.8 ./usr/share/man/man8/arm64/gpioctl.8 ./usr/share/man/man8/armv7/MAKEDEV.8 @@ -2383,8 +2381,6 @@ ./usr/share/man/man8/hppa/mkboot.8 ./usr/share/man/man8/httpd.8 ./usr/share/man/man8/i386/MAKEDEV.8 -./usr/share/man/man8/i386/apm.8 -./usr/share/man/man8/i386/apmd.8 ./usr/share/man/man8/i386/biosboot.8 ./usr/share/man/man8/i386/boot.8 ./usr/share/man/man8/i386/boot_i386.8 @@ -2431,15 +2427,11 @@ ./usr/share/man/man8/login_token.8 ./usr/share/man/man8/login_yubikey.8 ./usr/share/man/man8/loongson/MAKEDEV.8 -./usr/share/man/man8/loongson/apm.8 -./usr/share/man/man8/loongson/apmd.8 ./usr/share/man/man8/lpc.8 ./usr/share/man/man8/lpd.8 ./usr/share/man/man8/luna88k/MAKEDEV.8 ./usr/share/man/man8/luna88k/boot_luna88k.8 ./usr/share/man/man8/macppc/MAKEDEV.8 -./usr/share/man/man8/macppc/apm.8 -./usr/share/man/man8/macppc/apmd.8 ./usr/share/man/man8/macppc/boot.8 ./usr/share/man/man8/macppc/boot_macppc.8 ./usr/share/man/man8/macppc/eeprom.8 @@ -2583,8 +2575,6 @@ ./usr/share/man/man8/spamdb.8 ./usr/share/man/man8/spamlogd.8 ./usr/share/man/man8/sparc64/MAKEDEV.8 -./usr/share/man/man8/sparc64/apm.8 -./usr/share/man/man8/sparc64/apmd.8 ./usr/share/man/man8/sparc64/boot_sparc64.8 ./usr/share/man/man8/sparc64/eeprom.8 ./usr/share/man/man8/sparc64/ldomctl.8 Index: current.html === RCS file: /cvs/www/faq/current.html,v retrieving revision 1.1067 diff -u -p -r1.1067 current.html --- current.html12 Mar 2021 17:23:14 - 1.1067 +++ current.html25 Mar 2021 19:04:33 - @@ -274,6 +274,29 @@ RFC 8981 changed the terminology from pr temporary. +2021/03/25 - apm manual pages moved from MD to MI MANPATH + +https://man.openbsd.org/apm.8;>apm(8) and +https://man.openbsd.org/apmd.8;>apmd(8) are now installed machine +independently and the old machine-dependent manual pages should be removed. + + +# rm /usr/share/man/man8/amd64/apm.8 \ +
Re: vmctl: off-by-one error handling mixing -a with a VM id
Dave Voutila wrote: > Preben Guldberg writes: > > The patch below addresses an off-by-one error reading argv when > > generating the error message. > > I personally find it clearer if the condition of mixing -a with an id > > is highlighted. I included a suggestion in the patch below. > Since -a and providing an id are mutually exclusive, I think it's more > helpful to print usage information via ctl_usage(res->ctl). From the > usage details, it's self explanatory what's wrong. > usage: vmctl [-v] stop [-fw] [id | -a] The updated diff below would do just that: % vmctl stop -a testvm usage: vmctl [-v] stop [-fw] [id | -a] diff 09b708f572d76de8db7f7948ea7359b19bbd1c5a /usr/src blob - 249eaa3ded1ee9c804a81874613c292a74ea4b21 file + usr.sbin/vmctl/main.c --- usr.sbin/vmctl/main.c +++ usr.sbin/vmctl/main.c @@ -961,7 +961,7 @@ ctl_stop(struct parse_result *res, int argc, char *arg /* VM id is only expected without the -a flag */ if ((res->action != CMD_STOPALL && ret == -1) || (res->action == CMD_STOPALL && ret != -1)) - errx(1, "invalid id: %s", argv[1]); + ctl_usage(res->ctl); return (vmmaction(res)); }
Re: vmctl: off-by-one error handling mixing -a with a VM id
Preben Guldberg writes: > Thanks to Dave, Theo and Klemens for accepting my previous patch > > In other tests, I noticed the following error: > > % vmctl stop -a testvm > vmctl: invalid id: (null) > > The patch below addresses an off-by-one error reading argv when > generating the error message. > > I personally find it clearer if the condition of mixing -a with an id > is highlighted. I included a suggestion in the patch below. > > Index: usr.sbin/vmctl/main.c > === > RCS file: /cvs/src/usr.sbin/vmctl/main.c,v > retrieving revision 1.62 > diff -u -p -u -p -r1.62 main.c > --- usr.sbin/vmctl/main.c 3 Jan 2020 05:32:00 - 1.62 > +++ usr.sbin/vmctl/main.c 12 Mar 2021 11:23:26 - > @@ -961,7 +961,7 @@ ctl_stop(struct parse_result *res, int a > /* VM id is only expected without the -a flag */ > if ((res->action != CMD_STOPALL && ret == -1) || > (res->action == CMD_STOPALL && ret != -1)) > - errx(1, "invalid id: %s", argv[1]); > + errx(1, "invalid use of id with -a: %s", argv[0]); Since -a and providing an id are mutually exclusive, I think it's more helpful to print usage information via ctl_usage(res->ctl). From the usage details, it's self explanatory what's wrong. usage: vmctl [-v] stop [-fw] [id | -a] > > return (vmmaction(res)); > }
Re: disable PPP_BSDCOMP by default
No way for this diff. This is the wrong way. Surely there are ways to disable compression negotion on specific sessions, but removing the code from the kernel is the wrong knob. Balder Oddson wrote: > Compression in PPP was great in the age of ISDN to increase speeds. > The more common use cases, and trends concerning TLS1.3 advancements. > > Having this enabled by default, and infrequently used could lead to > unintended consequences around how the data is passed around. > > > Index: GENERIC > === > RCS file: /cvs/src/sys/conf/GENERIC,v > retrieving revision 1.274 > diff -u -p -u -p -r1.274 GENERIC > --- GENERIC 25 Feb 2021 01:19:35 - 1.274 > +++ GENERIC 25 Mar 2021 18:07:58 - > @@ -50,8 +50,8 @@ option TCP_SIGNATURE # TCP MD5 Signatur > > option INET6 # IPv6 > option IPSEC # IPsec > -option PPP_BSDCOMP # PPP BSD compression > -option PPP_DEFLATE > +#option PPP_BSDCOMP # PPP BSD compression > +#option PPP_DEFLATE # Disabled by default, TLS1.3 trends > option PIPEX # Ppp IP EXtension, for npppd > option MROUTING# Multicast router > option MPLS# Multi-Protocol Label Switching >
Re: apmd: log ioctl failures
On Sat, Mar 20, 2021 at 07:46:38PM +0100, Klemens Nanni wrote: > Otherwise there is no way other than reading driver code to determine > why running zzz(8) for example does not do anything on certain machines. > > apm(4/arm64) for one currently does not implement suspend and resume, > i.e. it yields ENOSUPP which gets lost in userland. > > This still does not make `zzz' or `apm -z' report the informative > warning, but syslog now has it: > > $ zzz > Suspending system... > $ tail -n3 /var/log/messages > Mar 20 19:16:57 pine64 apmd: system suspending > Mar 20 19:16:57 pine64 apmd: battery status: unknown. external power > status: not known. estimated battery life 0% > Mar 20 19:16:58 pine64 apmd: suspend: Operation not supported > > > Feedback? OK? Anyone? This is a trivial but helpful change, just like the ones in these other mails on tech@: "apm/macppc: Mark unsupported ioctl as such, merge cases" "apm/arm64: fix errno, merge ioctl cases" Index: apmd.c === RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v retrieving revision 1.101 diff -u -p -r1.101 apmd.c --- apmd.c 16 Mar 2021 09:00:43 - 1.101 +++ apmd.c 20 Mar 2021 18:47:13 - @@ -329,7 +329,8 @@ suspend(int ctl_fd) do_etc_file(_PATH_APM_ETC_SUSPEND); sync(); sleep(1); - ioctl(ctl_fd, APM_IOC_SUSPEND, 0); + if (ioctl(ctl_fd, APM_IOC_SUSPEND, 0) == -1) + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); } void @@ -340,7 +341,8 @@ stand_by(int ctl_fd) do_etc_file(_PATH_APM_ETC_STANDBY); sync(); sleep(1); - ioctl(ctl_fd, APM_IOC_STANDBY, 0); + if (ioctl(ctl_fd, APM_IOC_STANDBY, 0) == -1) + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); } void @@ -351,7 +353,8 @@ hibernate(int ctl_fd) do_etc_file(_PATH_APM_ETC_HIBERNATE); sync(); sleep(1); - ioctl(ctl_fd, APM_IOC_HIBERNATE, 0); + if (ioctl(ctl_fd, APM_IOC_HIBERNATE, 0) == -1) + logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno)); } void
Re: monotonic time going back by wrong skews
> From: Scott Cheloha
> Date: Thu, 25 Mar 2021 13:18:04 -0500
>
> > On Mar 24, 2021, at 8:29 AM, Josh Rickmar wrote:
> >
> > On Wed, Mar 24, 2021 at 05:40:21PM +0900, YASUOKA Masahiko wrote:
> >> Hi,
> >>
> >> I hit a problem which is caused by going back of monotonic time. It
> >> happens on hosts on VMware ESXi.
> >>
> >> I wrote the program which repeats the problem.
> >>
> >> % cc -o monotime monotime.c -lpthread
> >> % ./monotime
> >> 194964 Starting
> >> 562210 Starting
> >> 483046 Starting
> >> 148865 Starting
> >> 148865 Back 991.808048665 => 991.007447931
> >> 562210 Back 991.808048885 => 991.007448224
> >> 483046 Back 991.808049115 => 991.007449172
> >> 148865 Stopped
> >> 562210 Stopped
> >> 483046 Stopped
> >> 194964 Stopped
> >> % uname -a
> >> OpenBSD yasuoka-ob-c.tokyo.iiji.jp 6.8 GENERIC.MP#5 amd64
> >> % sysctl kern.version
> >> kern.version=OpenBSD 6.8 (GENERIC.MP) #5: Mon Feb 22 04:36:10 MST 2021
> >>
> >> r...@syspatch-68-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >> %
> >>
> >> monotime.c
> >>
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >>
> >> #define NTHREAD4
> >> #define NTRY 5
> >>
> >> void *
> >> start(void *dummy)
> >> {
> >>int i;
> >>struct timespec ts0, ts1;
> >>
> >>printf("%d Starting\n", (int)getthrid());
> >>clock_gettime(CLOCK_MONOTONIC, );
> >>
> >>for (i = 0; i < NTRY; i++) {
> >>clock_gettime(CLOCK_MONOTONIC, );
> >>if (timespeccmp(, , <=)) {
> >>ts0 = ts1;
> >>continue;
> >>}
> >>printf("%d Back %lld.%09lu => %lld.%09lu\n",
> >>(int)getthrid(), ts0.tv_sec, ts0.tv_nsec, ts1.tv_sec,
> >>ts1.tv_nsec);
> >>break;
> >>}
> >>printf("%d Stopped\n", (int)getthrid());
> >>
> >>return (NULL);
> >> }
> >>
> >> int
> >> main(int argc, char *argv[])
> >> {
> >>int i, n = NTHREAD;
> >>pthread_t *threads;
> >>
> >>threads = calloc(n, sizeof(pthread_t));
> >>
> >>for (i = 0; i < n; i++)
> >>pthread_create([i], NULL, start, NULL);
> >>for (i = 0; i < n; i++)
> >>pthread_join(threads[i], NULL);
> >>
> >> }
> >>
> >>
> >> The machine has 4 vCPUs and showing the following message on boot.
> >>
> >> cpu1: disabling user TSC (skew=-5310)
> >> cpu2: disabling user TSC (skew=-5335)
> >> cpu3: disabling user TSC (skew=-7386)
> >>
> >> This means "user TSC" is disabled because of TSC of cpu{1,2,3} is much
> >> delayed against cpu0.
> >>
> >> Simply ignoring the skews by the following diff seems to workaround
> >> this problem.
> >>
> >> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> >> index 238a5a068e1..3b951a8b5a3 100644
> >> --- a/sys/arch/amd64/amd64/tsc.c
> >> +++ b/sys/arch/amd64/amd64/tsc.c
> >> @@ -212,7 +212,8 @@ cpu_recalibrate_tsc(struct timecounter *tc)
> >> u_int
> >> tsc_get_timecount(struct timecounter *tc)
> >> {
> >> - return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> + //return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> + return rdtsc_lfence();
> >> }
> >>
> >> void
> >>
> >> So I supposed the skews are not calculated properly. Also I found
> >> NetBSD changed the skew calculating so that it checks 1000 times and
> >> take the minimum value.
> >>
> >>
> >> https://github.com/NetBSD/src/commit/1dec05c1ae197b4acfc7038e49dfddabcbed0dff
> >>
> >> https://github.com/NetBSD/src/commit/66d76b89792bac1c71cd5507ba62b08ad02129ef
> >>
> >>
> >> I checked skews on the machine by the following debug code.
> >>
> >> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> >> index 238a5a068e1..83e835e4f82 100644
> >> --- a/sys/arch/amd64/amd64/tsc.c
> >> +++ b/sys/arch/amd64/amd64/tsc.c
> >> @@ -302,16 +302,42 @@ tsc_read_bp(struct cpu_info *ci, uint64_t *bptscp,
> >> uint64_t *aptscp)
> >>*aptscp = tsc_sync_val;
> >> }
> >>
> >> +#define TSC_SYNC_NTIMES 1000
> >> +
> >> +static int tsc_difs[MAXCPUS][TSC_SYNC_NTIMES];
> >> +
> >> +void
> >> +tsc_debug(void)
> >> +{
> >> + int i, cpuid = curcpu()->ci_cpuid;
> >> +
> >> + for (i = 0; i < TSC_SYNC_NTIMES; i++) {
> >> + if (i % 10 == 0)
> >> + printf("%5d", tsc_difs[cpuid][i]);
> >> + else
> >> + printf(" %5d", tsc_difs[cpuid][i]);
> >> + if (i % 10 == 9)
> >> + printf("\n");
> >> + }
> >> + printf("\n");
> >> +}
> >> +
> >> void
> >> tsc_sync_bp(struct cpu_info *ci)
> >> {
> >> + int i, mindif = INT_MAX, dif;
> >>uint64_t bptsc, aptsc;
> >>
> >> - tsc_read_bp(ci, , ); /* discarded - cache effects */
> >> - tsc_read_bp(ci, , );
> >> + for (i = 0; i < TSC_SYNC_NTIMES; i++) {
> >> + tsc_read_bp(ci, , );
> >> + dif = bptsc - aptsc;
> >> + if (abs(dif) < abs(mindif))
> >> + mindif = dif;
> >> +
Re: monotonic time going back by wrong skews
On Thu, Mar 25, 2021 at 01:18:04PM -0500, Scott Cheloha wrote:
> > On Mar 24, 2021, at 8:29 AM, Josh Rickmar wrote:
> >
> > On Wed, Mar 24, 2021 at 05:40:21PM +0900, YASUOKA Masahiko wrote:
> >> Hi,
> >>
> >> I hit a problem which is caused by going back of monotonic time. It
> >> happens on hosts on VMware ESXi.
> >>
> >> I wrote the program which repeats the problem.
> >>
> >> % cc -o monotime monotime.c -lpthread
> >> % ./monotime
> >> 194964 Starting
> >> 562210 Starting
> >> 483046 Starting
> >> 148865 Starting
> >> 148865 Back 991.808048665 => 991.007447931
> >> 562210 Back 991.808048885 => 991.007448224
> >> 483046 Back 991.808049115 => 991.007449172
> >> 148865 Stopped
> >> 562210 Stopped
> >> 483046 Stopped
> >> 194964 Stopped
> >> % uname -a
> >> OpenBSD yasuoka-ob-c.tokyo.iiji.jp 6.8 GENERIC.MP#5 amd64
> >> % sysctl kern.version
> >> kern.version=OpenBSD 6.8 (GENERIC.MP) #5: Mon Feb 22 04:36:10 MST 2021
> >>
> >> r...@syspatch-68-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >> %
> >>
> >> monotime.c
> >>
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >>
> >> #define NTHREAD4
> >> #define NTRY 5
> >>
> >> void *
> >> start(void *dummy)
> >> {
> >>int i;
> >>struct timespec ts0, ts1;
> >>
> >>printf("%d Starting\n", (int)getthrid());
> >>clock_gettime(CLOCK_MONOTONIC, );
> >>
> >>for (i = 0; i < NTRY; i++) {
> >>clock_gettime(CLOCK_MONOTONIC, );
> >>if (timespeccmp(, , <=)) {
> >>ts0 = ts1;
> >>continue;
> >>}
> >>printf("%d Back %lld.%09lu => %lld.%09lu\n",
> >>(int)getthrid(), ts0.tv_sec, ts0.tv_nsec, ts1.tv_sec,
> >>ts1.tv_nsec);
> >>break;
> >>}
> >>printf("%d Stopped\n", (int)getthrid());
> >>
> >>return (NULL);
> >> }
> >>
> >> int
> >> main(int argc, char *argv[])
> >> {
> >>int i, n = NTHREAD;
> >>pthread_t *threads;
> >>
> >>threads = calloc(n, sizeof(pthread_t));
> >>
> >>for (i = 0; i < n; i++)
> >>pthread_create([i], NULL, start, NULL);
> >>for (i = 0; i < n; i++)
> >>pthread_join(threads[i], NULL);
> >>
> >> }
> >>
> >>
> >> The machine has 4 vCPUs and showing the following message on boot.
> >>
> >> cpu1: disabling user TSC (skew=-5310)
> >> cpu2: disabling user TSC (skew=-5335)
> >> cpu3: disabling user TSC (skew=-7386)
> >>
> >> This means "user TSC" is disabled because of TSC of cpu{1,2,3} is much
> >> delayed against cpu0.
> >>
> >> Simply ignoring the skews by the following diff seems to workaround
> >> this problem.
> >>
> >> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> >> index 238a5a068e1..3b951a8b5a3 100644
> >> --- a/sys/arch/amd64/amd64/tsc.c
> >> +++ b/sys/arch/amd64/amd64/tsc.c
> >> @@ -212,7 +212,8 @@ cpu_recalibrate_tsc(struct timecounter *tc)
> >> u_int
> >> tsc_get_timecount(struct timecounter *tc)
> >> {
> >> - return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> + //return rdtsc_lfence() + curcpu()->ci_tsc_skew;
> >> + return rdtsc_lfence();
> >> }
> >>
> >> void
> >>
> >> So I supposed the skews are not calculated properly. Also I found
> >> NetBSD changed the skew calculating so that it checks 1000 times and
> >> take the minimum value.
> >>
> >>
> >> https://github.com/NetBSD/src/commit/1dec05c1ae197b4acfc7038e49dfddabcbed0dff
> >>
> >> https://github.com/NetBSD/src/commit/66d76b89792bac1c71cd5507ba62b08ad02129ef
> >>
> >>
> >> I checked skews on the machine by the following debug code.
> >>
> >> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> >> index 238a5a068e1..83e835e4f82 100644
> >> --- a/sys/arch/amd64/amd64/tsc.c
> >> +++ b/sys/arch/amd64/amd64/tsc.c
> >> @@ -302,16 +302,42 @@ tsc_read_bp(struct cpu_info *ci, uint64_t *bptscp,
> >> uint64_t *aptscp)
> >>*aptscp = tsc_sync_val;
> >> }
> >>
> >> +#define TSC_SYNC_NTIMES 1000
> >> +
> >> +static int tsc_difs[MAXCPUS][TSC_SYNC_NTIMES];
> >> +
> >> +void
> >> +tsc_debug(void)
> >> +{
> >> + int i, cpuid = curcpu()->ci_cpuid;
> >> +
> >> + for (i = 0; i < TSC_SYNC_NTIMES; i++) {
> >> + if (i % 10 == 0)
> >> + printf("%5d", tsc_difs[cpuid][i]);
> >> + else
> >> + printf(" %5d", tsc_difs[cpuid][i]);
> >> + if (i % 10 == 9)
> >> + printf("\n");
> >> + }
> >> + printf("\n");
> >> +}
> >> +
> >> void
> >> tsc_sync_bp(struct cpu_info *ci)
> >> {
> >> + int i, mindif = INT_MAX, dif;
> >>uint64_t bptsc, aptsc;
> >>
> >> - tsc_read_bp(ci, , ); /* discarded - cache effects */
> >> - tsc_read_bp(ci, , );
> >> + for (i = 0; i < TSC_SYNC_NTIMES; i++) {
> >> + tsc_read_bp(ci, , );
> >> + dif = bptsc - aptsc;
> >> + if (abs(dif) < abs(mindif))
> >> + mindif = dif;
> >> +
Re: monotonic time going back by wrong skews
> On Mar 24, 2021, at 8:29 AM, Josh Rickmar wrote:
>
> On Wed, Mar 24, 2021 at 05:40:21PM +0900, YASUOKA Masahiko wrote:
>> Hi,
>>
>> I hit a problem which is caused by going back of monotonic time. It
>> happens on hosts on VMware ESXi.
>>
>> I wrote the program which repeats the problem.
>>
>> % cc -o monotime monotime.c -lpthread
>> % ./monotime
>> 194964 Starting
>> 562210 Starting
>> 483046 Starting
>> 148865 Starting
>> 148865 Back 991.808048665 => 991.007447931
>> 562210 Back 991.808048885 => 991.007448224
>> 483046 Back 991.808049115 => 991.007449172
>> 148865 Stopped
>> 562210 Stopped
>> 483046 Stopped
>> 194964 Stopped
>> % uname -a
>> OpenBSD yasuoka-ob-c.tokyo.iiji.jp 6.8 GENERIC.MP#5 amd64
>> % sysctl kern.version
>> kern.version=OpenBSD 6.8 (GENERIC.MP) #5: Mon Feb 22 04:36:10 MST 2021
>>
>> r...@syspatch-68-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
>> %
>>
>> monotime.c
>>
>> #include
>> #include
>> #include
>> #include
>> #include
>> #include
>> #include
>>
>> #define NTHREAD 4
>> #define NTRY 5
>>
>> void *
>> start(void *dummy)
>> {
>> int i;
>> struct timespec ts0, ts1;
>>
>> printf("%d Starting\n", (int)getthrid());
>> clock_gettime(CLOCK_MONOTONIC, );
>>
>> for (i = 0; i < NTRY; i++) {
>> clock_gettime(CLOCK_MONOTONIC, );
>> if (timespeccmp(, , <=)) {
>> ts0 = ts1;
>> continue;
>> }
>> printf("%d Back %lld.%09lu => %lld.%09lu\n",
>> (int)getthrid(), ts0.tv_sec, ts0.tv_nsec, ts1.tv_sec,
>> ts1.tv_nsec);
>> break;
>> }
>> printf("%d Stopped\n", (int)getthrid());
>>
>> return (NULL);
>> }
>>
>> int
>> main(int argc, char *argv[])
>> {
>> int i, n = NTHREAD;
>> pthread_t *threads;
>>
>> threads = calloc(n, sizeof(pthread_t));
>>
>> for (i = 0; i < n; i++)
>> pthread_create([i], NULL, start, NULL);
>> for (i = 0; i < n; i++)
>> pthread_join(threads[i], NULL);
>>
>> }
>>
>>
>> The machine has 4 vCPUs and showing the following message on boot.
>>
>> cpu1: disabling user TSC (skew=-5310)
>> cpu2: disabling user TSC (skew=-5335)
>> cpu3: disabling user TSC (skew=-7386)
>>
>> This means "user TSC" is disabled because of TSC of cpu{1,2,3} is much
>> delayed against cpu0.
>>
>> Simply ignoring the skews by the following diff seems to workaround
>> this problem.
>>
>> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
>> index 238a5a068e1..3b951a8b5a3 100644
>> --- a/sys/arch/amd64/amd64/tsc.c
>> +++ b/sys/arch/amd64/amd64/tsc.c
>> @@ -212,7 +212,8 @@ cpu_recalibrate_tsc(struct timecounter *tc)
>> u_int
>> tsc_get_timecount(struct timecounter *tc)
>> {
>> -return rdtsc_lfence() + curcpu()->ci_tsc_skew;
>> +//return rdtsc_lfence() + curcpu()->ci_tsc_skew;
>> +return rdtsc_lfence();
>> }
>>
>> void
>>
>> So I supposed the skews are not calculated properly. Also I found
>> NetBSD changed the skew calculating so that it checks 1000 times and
>> take the minimum value.
>>
>>
>> https://github.com/NetBSD/src/commit/1dec05c1ae197b4acfc7038e49dfddabcbed0dff
>>
>> https://github.com/NetBSD/src/commit/66d76b89792bac1c71cd5507ba62b08ad02129ef
>>
>>
>> I checked skews on the machine by the following debug code.
>>
>> diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
>> index 238a5a068e1..83e835e4f82 100644
>> --- a/sys/arch/amd64/amd64/tsc.c
>> +++ b/sys/arch/amd64/amd64/tsc.c
>> @@ -302,16 +302,42 @@ tsc_read_bp(struct cpu_info *ci, uint64_t *bptscp,
>> uint64_t *aptscp)
>> *aptscp = tsc_sync_val;
>> }
>>
>> +#define TSC_SYNC_NTIMES 1000
>> +
>> +static int tsc_difs[MAXCPUS][TSC_SYNC_NTIMES];
>> +
>> +void
>> +tsc_debug(void)
>> +{
>> +int i, cpuid = curcpu()->ci_cpuid;
>> +
>> +for (i = 0; i < TSC_SYNC_NTIMES; i++) {
>> +if (i % 10 == 0)
>> +printf("%5d", tsc_difs[cpuid][i]);
>> +else
>> +printf(" %5d", tsc_difs[cpuid][i]);
>> +if (i % 10 == 9)
>> +printf("\n");
>> +}
>> +printf("\n");
>> +}
>> +
>> void
>> tsc_sync_bp(struct cpu_info *ci)
>> {
>> +int i, mindif = INT_MAX, dif;
>> uint64_t bptsc, aptsc;
>>
>> -tsc_read_bp(ci, , ); /* discarded - cache effects */
>> -tsc_read_bp(ci, , );
>> +for (i = 0; i < TSC_SYNC_NTIMES; i++) {
>> +tsc_read_bp(ci, , );
>> +dif = bptsc - aptsc;
>> +if (abs(dif) < abs(mindif))
>> +mindif = dif;
>> +tsc_difs[ci->ci_cpuid][i] = dif;
>> +}
>>
>> /* Compute final value to adjust for skew. */
>> -ci->ci_tsc_skew = bptsc - aptsc;
>> +ci->ci_tsc_skew = mindif;
>> }
>>
>> /*
>> @@ -342,8 +368,10 @@ tsc_post_ap(struct cpu_info *ci)
>> void
>> tsc_sync_ap(struct cpu_info *ci)
>> {
>> -
disable PPP_BSDCOMP by default
Compression in PPP was great in the age of ISDN to increase speeds. The more common use cases, and trends concerning TLS1.3 advancements. Having this enabled by default, and infrequently used could lead to unintended consequences around how the data is passed around. Index: GENERIC === RCS file: /cvs/src/sys/conf/GENERIC,v retrieving revision 1.274 diff -u -p -u -p -r1.274 GENERIC --- GENERIC 25 Feb 2021 01:19:35 - 1.274 +++ GENERIC 25 Mar 2021 18:07:58 - @@ -50,8 +50,8 @@ optionTCP_SIGNATURE # TCP MD5 Signatur option INET6 # IPv6 option IPSEC # IPsec -option PPP_BSDCOMP # PPP BSD compression -option PPP_DEFLATE +#optionPPP_BSDCOMP # PPP BSD compression +#optionPPP_DEFLATE # Disabled by default, TLS1.3 trends option PIPEX # Ppp IP EXtension, for npppd option MROUTING# Multicast router option MPLS# Multi-Protocol Label Switching
smtpd: set protocols and ciphers
Hi.
This diff allows to specify the protocol versions and ciphers
to use for outgoing TLS sessions on a per relay basis.
Eric.
Index: mta.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.235
diff -u -p -r1.235 mta.c
--- mta.c 5 Mar 2021 12:37:32 - 1.235
+++ mta.c 25 Mar 2021 13:06:20 -
@@ -491,6 +491,7 @@ mta_setup_dispatcher(struct dispatcher *
struct tls_config *config;
struct pki *pki;
struct ca *ca;
+ uint32_t protos;
if (dispatcher->type != DISPATCHER_REMOTE)
return;
@@ -500,10 +501,14 @@ mta_setup_dispatcher(struct dispatcher *
if ((config = tls_config_new()) == NULL)
fatal("smtpd: tls_config_new");
- if (env->sc_tls_ciphers) {
- if (tls_config_set_ciphers(config, env->sc_tls_ciphers) == -1)
- err(1, "%s", tls_config_error(config));
- }
+ if (remote->tls_ciphers &&
+ tls_config_set_ciphers(config, remote->tls_ciphers) == -1)
+ err(1, "%s", tls_config_error(config));
+
+ if (remote->tls_protocols &&
+ (tls_config_parse_protocols(, remote->tls_protocols) == -1
+ || tls_config_set_protocols(config, protos) == -1))
+ err(1, "%s", tls_config_error(config));
if (remote->pki) {
pki = dict_get(env->sc_pki_dict, remote->pki);
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.285
diff -u -p -r1.285 parse.y
--- parse.y 5 Mar 2021 12:37:32 - 1.285
+++ parse.y 25 Mar 2021 13:05:38 -
@@ -190,7 +190,7 @@ typedef struct {
%token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE
MAX_DEFERRED MBOX MDA MTA MX
%token NO_DSN NO_VERIFY NOOP
%token ON
-%token PHASE PKI PORT PROC PROC_EXEC PROXY_V2
+%token PHASE PKI PORT PROC PROC_EXEC PROTOCOLS PROXY_V2
%token QUEUE QUIT
%token RCPT_TO RDNS RECIPIENT RECEIVEDAUTH REGEX RELAY REJECT REPORT REWRITE
RSET
%token SCHEDULER SENDER SENDERS SMTP SMTP_IN SMTP_OUT SMTPS SOCKET SRC SRS
SUB_ADDR_DELIM
@@ -768,6 +768,22 @@ HELO STRING {
dsp->u.remote.ca = $2;
}
+| CIPHERS STRING {
+ if (dsp->u.remote.tls_ciphers) {
+ yyerror("ciphers already specified for this dispatcher");
+ YYERROR;
+ }
+
+ dsp->u.remote.tls_ciphers = $2;
+}
+| PROTOCOLS STRING {
+ if (dsp->u.remote.tls_protocols) {
+ yyerror("protocols already specified for this dispatcher");
+ YYERROR;
+ }
+
+ dsp->u.remote.tls_protocols = $2;
+}
| SRC tables {
struct table *t = $2;
@@ -2682,6 +2698,7 @@ lookup(char *s)
{ "port", PORT },
{ "proc", PROC },
{ "proc-exec", PROC_EXEC },
+ { "protocols", PROTOCOLS },
{ "proxy-v2", PROXY_V2 },
{ "queue", QUEUE },
{ "quit", QUIT },
Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.258
diff -u -p -r1.258 smtpd.conf.5
--- smtpd.conf.55 Mar 2021 12:37:32 - 1.258
+++ smtpd.conf.525 Mar 2021 17:47:00 -
@@ -298,6 +298,18 @@ When used with a smarthost, the protocol
If
.Cm no-verify
is specified, do not require a valid certificate.
+.It Cm protocols Ar protostr
+Define the protocol versions to be used for TLS sessions.
+Refer to the
+.Xr tls_config_parse_protocols 3
+manpage for the format of
+.Ar protostr .
+.It Cm ciphers Ar cipherstr
+Define the list of ciphers that may be used for TLS sessions.
+Refer to the
+.Xr tls_config_set_ciphers 3
+manpage for the format of
+.Ar cipherstr .
.It Cm auth Pf < Ar table Ns >
Use the mapping
.Ar table
Index: smtpd.h
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
retrieving revision 1.662
diff -u -p -r1.662 smtpd.h
--- smtpd.h 5 Mar 2021 12:37:32 - 1.662
+++ smtpd.h 25 Mar 2021 13:06:36 -
@@ -1192,6 +1192,8 @@ struct dispatcher_remote {
char*auth;
int tls_required;
int tls_noverify;
+ char*tls_protocols;
+ char*tls_ciphers;
int backup;
char*backupmx;
vmctl: off-by-one error handling mixing -a with a VM id
Thanks to Dave, Theo and Klemens for accepting my previous patch In other tests, I noticed the following error: % vmctl stop -a testvm vmctl: invalid id: (null) The patch below addresses an off-by-one error reading argv when generating the error message. I personally find it clearer if the condition of mixing -a with an id is highlighted. I included a suggestion in the patch below. Index: usr.sbin/vmctl/main.c === RCS file: /cvs/src/usr.sbin/vmctl/main.c,v retrieving revision 1.62 diff -u -p -u -p -r1.62 main.c --- usr.sbin/vmctl/main.c 3 Jan 2020 05:32:00 - 1.62 +++ usr.sbin/vmctl/main.c 12 Mar 2021 11:23:26 - @@ -961,7 +961,7 @@ ctl_stop(struct parse_result *res, int a /* VM id is only expected without the -a flag */ if ((res->action != CMD_STOPALL && ret == -1) || (res->action == CMD_STOPALL && ret != -1)) - errx(1, "invalid id: %s", argv[1]); + errx(1, "invalid use of id with -a: %s", argv[0]); return (vmmaction(res)); }
Re: vmd(8): fix packet handling for dhcpleased(8)
This might not be a problem in practice. vmd(8) hands us a lease with "infinity" lease time. This is expresed as UINT32_MAX, i.e. 2^32-1. dhcpleased(8) does not handle infinity explicitly, it's just a very long lease time (136 years). When we configure the lease we enter the BOUND state. After 0.5 * lease time (T1) we transition to RENEWING: RFC 2131, 4.3.2 DHCPREQUEST message: o DHCPREQUEST generated during RENEWING state: 'server identifier' MUST NOT be filled in, 'requested IP address' option MUST NOT be filled in, 'ciaddr' MUST be filled in with client's IP address. In this situation, the client is completely configured, and is trying to extend its lease. *This message will be unicast*, so no relay agents will be involved in its transmission. Because 'giaddr' is therefore not filled in, the DHCP server will trust the value in 'ciaddr', and use it when replying to the client. This is the only state where we send unicast messages. After 0.875 * lease_time (T2) we transition to REBINDING which is again broadcast. Now these are very long timeouts. In particular we transition to RENEWING after 68 years... One can trigger a transition from BOUND to renewing via dhcpleasectl(8)'s send request command. It will basically do the next state transition, in this case to RENEWING and will be stuck there because vmd will ignore out request. Our lease is however still valid, so everything "just works". Maybe the problem is with the send request command. I don't know yet what to do with it. Maybe it should transition to INIT state. This is what dhclient(8) is doing when one re-starts it on an interface. So vmd(8) is not behaving correctly as dhcp server. I don't know if this needs fixing though if it's too complicated. On Wed, Mar 24, 2021 at 05:47:36PM -0400, Dave Voutila wrote: > tech@, > > While migrating an existing -current vm to use dhcpleased(8), one of the > issues discovered was the dhcp/bootp handling built into vmd(8) for > local interfaces was improperly missing packets sent to the ethernet > address of the host-side tap(4) device. (vmd(8) was only looking for > broadcast packets.) > > The following diff: > - includes the host-side tap(4) lladdr in the packet filtering logic for > intercepting vio(4) dhcp/bootp communication > - removes a conditional check (dhcpsz == 0) pointed out by deraadt@ that > could contribute to missed packets while iterating through the vionet > tx ring > > Because of vmd(8)'s priv-sep design, the approach taken is to pass a > duplicate of the opened tap(4) fd to the "priv" process that is > unpledged and responsible for setting up networking. A response imsg > travels between processes, being forwarded until it gets to the intended > vm process. > > For those looking to test the diff, some steps to follow are: > > 0. Don't apply the patch yet. > 1. Use an OpenBSD guest running a recent snapshot that has the latest > dhcpleased(8)[1] and dhcpleasectl(8)[2] changes committed by florian@. > 2. Configure the guest for using dhcpleased(8), ideally via > /etc/hostname.vio0 containing: > > inet autoconf > up > > 3. Ensure you get an IP assigned from vmd(8) after boot. > 4. Check the lease with `dhcpleasectl show interface vio0`. It should >report [Bound] and have a _very_ long lease time. > 5. Force a renewal with `dhcpleasectl send request vio0`. > 6. Check again like in step 4. It will be "stuck" in a [Renewing] state. > 7. Shut down the guest, vmd(8), and apply the patch. > 8. Repeat steps 4-6, but the guest should no longer be "stuck" in >[Renewing] and should report [Bound] after forced renewal. > > You can also turn up debug logging for vmd(8) and should see > corresponding messages of: > > vionet: dhcp request, local response size 342 > > If you do not, the packet was not intercepted by vmd(8). > > I have tested this diff with a variety of conditions, including multiple > emulated nics per guest. Specifically 1 nic using dhcp/bootp resolved by > vmd(8) and the other nic connected to a virtual switch using veb(4) and > an instance of dhcpd(8) running on the host. Also ran a snapshot upgrade > with no issues. > > So in summary, if this diff works, you shouldn't notice a difference ;-) > > Thanks, > -Dave > > [1] https://marc.info/?l=openbsd-cvs=161643049815177=2 > [2] https://marc.info/?l=openbsd-cvs=161652157122736=2 > > > Index: config.c > === > RCS file: /cvs/src/usr.sbin/vmd/config.c,v > retrieving revision 1.60 > diff -u -p -r1.60 config.c > --- config.c 19 Mar 2021 09:29:33 - 1.60 > +++ config.c 24 Mar 2021 15:42:13 - > @@ -227,6 +227,7 @@ config_setvm(struct privsep *ps, struct > char base[PATH_MAX]; > unsigned int unit; > struct timeval tv, rate, since_last; > + struct vmop_addr_req var; > > errno = 0; > > @@ -499,6
Re: [External] : [ICMP] IP options lead to malformed reply
Hello,
> > 1) ip_insertoptions() does not update length of IP header (ip_hl)
> >
> > 2) ip_hl is being overridden anyway later in ip_output() called
> > by ip_send_dispatch() to send ICMP error packet out. Looks
> > like ip_send_dispatch() should use IP_RAWOUTPUT flag so
> > ip_hl won't get overridden.
>
> Maybe I have overlooked something, but I see two problems with this.
> When the IP_RAWOUTPUT flag is set, all packets send via ip_send() are
> no longer included in the ips_localout statistic. Also, all callers of
> ip_send() need to fill out the IP header themselves (as it would be
> done in the beginning of ip_output() without the IP_RAWOUTPUT flag). As
> far as I can tell this is not done for packets in a gif tunnel (e.g.
> the ip_id is not calculated/added).
>
no, you have not overlooked anything, you are absolutely right.
my patch is buggy. updated diff introduces ip_send_raw(), which
is an entry point for ipsendraw_task.
whenever icmp_send() adds options, it completes IP header and dispatches
packet to ipsendraw_task.
OK?
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
index a007aa6c2b3..30cb417676d 100644
--- a/sys/netinet/ip_icmp.c
+++ b/sys/netinet/ip_icmp.c
@@ -846,10 +846,15 @@ icmp_send(struct mbuf *m, struct mbuf *opts)
printf("icmp_send dst %s src %s\n", dst, src);
}
#endif
- if (opts != NULL)
+ if (opts != NULL) {
m = ip_insertoptions(m, opts, );
-
- ip_send(m);
+ ip->ip_v = IPVERSION;
+ ip->ip_off &= htons(IP_DF);
+ ip->ip_id = htons(ip_randomid());
+ ipstat_inc(ips_localout);
+ ip_send_raw(m);
+ } else
+ ip_send(m);
}
u_int32_t
diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
index 0ec3f723be4..d4ac9edffaf 100644
--- a/sys/netinet/ip_input.c
+++ b/sys/netinet/ip_input.c
@@ -139,6 +139,7 @@ struct cpumem *ipcounters;
int ip_sysctl_ipstat(void *, size_t *, void *);
static struct mbuf_queue ipsend_mq;
+static struct mbuf_queue ipsendraw_mq;
extern struct niqueue arpinq;
@@ -147,7 +148,11 @@ intip_dooptions(struct mbuf *, struct ifnet *);
intin_ouraddr(struct mbuf *, struct ifnet *, struct rtentry **);
static void ip_send_dispatch(void *);
+static void ip_sendraw_dispatch(void *);
static struct task ipsend_task = TASK_INITIALIZER(ip_send_dispatch,
_mq);
+static struct task ipsendraw_task =
+ TASK_INITIALIZER(ip_sendraw_dispatch, _mq);
+
/*
* Used to save the IP options in case a protocol wants to respond
* to an incoming packet over the same route if the packet got here
@@ -1776,6 +1781,24 @@ ip_savecontrol(struct inpcb *inp, struct mbuf **mp,
struct ip *ip,
}
}
+void
+ip_sendraw_dispatch(void *xmq)
+{
+ struct mbuf_queue *mq = xmq;
+ struct mbuf *m;
+ struct mbuf_list ml;
+
+ mq_delist(mq, );
+ if (ml_empty())
+ return;
+
+ NET_LOCK();
+ while ((m = ml_dequeue()) != NULL) {
+ ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL, 0);
+ }
+ NET_UNLOCK();
+}
+
void
ip_send_dispatch(void *xmq)
{
@@ -1800,3 +1823,10 @@ ip_send(struct mbuf *m)
mq_enqueue(_mq, m);
task_add(net_tq(0), _task);
}
+
+void
+ip_send_raw(struct mbuf *m)
+{
+ mq_enqueue(_mq, m);
+ task_add(net_tq(0), _task);
+}
diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index c01a3e7803c..ea803077304 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -765,6 +765,11 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int
*phlen)
optlen = opt->m_len - sizeof(p->ipopt_dst);
if (optlen + ntohs(ip->ip_len) > IP_MAXPACKET)
return (m); /* XXX should fail */
+
+ /* check if options will fit to IP header */
+ if ((optlen + (ip->ip_hl << 2)) > (0x0f << 2))
+ return (m);
+
if (p->ipopt_dst.s_addr)
ip->ip_dst = p->ipopt_dst;
if (m->m_flags & M_EXT || m->m_data - optlen < m->m_pktdat) {
@@ -790,6 +795,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int
*phlen)
memcpy(ip + 1, p->ipopt_list, optlen);
*phlen = sizeof(struct ip) + optlen;
ip->ip_len = htons(ntohs(ip->ip_len) + optlen);
+ ip->ip_hl += (optlen >> 2);
return (m);
}
diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
index 7ede24ce922..1a43675a7ac 100644
--- a/sys/netinet/ip_var.h
+++ b/sys/netinet/ip_var.h
@@ -240,6 +240,7 @@ struct mbuf *
u_int16_t
ip_randomid(void);
voidip_send(struct mbuf *);
+voidip_send_raw(struct mbuf *);
voidip_slowtimo(void);
struct mbuf *
ip_srcroute(struct mbuf *);
ieee80211_recv_auth: fix wrong seqnum wrap
IEEE 802.11 sequence numbers wrap around at 0xfff, not 0x. ok? diff 567a54141cb7379326a3670b319b26530610e1e8 /usr/src blob - a44e88e5d0e94101a1966fc95d2daceba78c7246 file + sys/net80211/ieee80211_input.c --- sys/net80211/ieee80211_input.c +++ sys/net80211/ieee80211_input.c @@ -2056,7 +2056,7 @@ ieee80211_recv_auth(struct ieee80211com *ic, struct mb /* XXX hack to workaround calling convention */ IEEE80211_SEND_MGMT(ic, ni, IEEE80211_FC0_SUBTYPE_AUTH, - IEEE80211_STATUS_ALG << 16 | ((seq + 1) & 0x)); + IEEE80211_STATUS_ALG << 16 | ((seq + 1) & 0xfff)); } #endif return;
Re: rpki-client http client adjustments
On Thu, Mar 25, 2021 at 10:46:18AM +0100, Claudio Jeker wrote:
> This diff is mostly cleanup and adding the missing bits needed for RRDP.
> Instead of a simple bool ok use an enum to report the state back.
> Can be fail, ok or not-modified (the last is used for 304 Not Modified
> answers (if a If-Modified-Since header was passed in the request).
>
> Additionally add 308 redirect support and remove 206 as a valid status
> response. The http client does not do range requests so a 206 return is
> never expected.
ok tb
>
> --
> :wq Claudio
>
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.55
> diff -u -p -r1.55 extern.h
> --- extern.h 19 Mar 2021 13:56:10 - 1.55
> +++ extern.h 25 Mar 2021 09:10:04 -
> @@ -264,6 +264,12 @@ enum rtype {
> RTYPE_GBR,
> };
>
> +enum http_result {
> + HTTP_FAILED,/* anything else */
> + HTTP_OK,/* 200 OK */
> + HTTP_NOT_MOD, /* 304 Not Modified */
> +};
> +
> /*
> * An entity (MFT, ROA, certificate, etc.) that needs to be downloaded
> * and parsed.
> Index: http.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 http.c
> --- http.c18 Mar 2021 16:15:19 - 1.8
> +++ http.c25 Mar 2021 09:12:18 -
> @@ -264,7 +264,7 @@ http_resolv(struct http_connection *conn
> }
>
> static void
> -http_done(struct http_connection *conn, int ok)
> +http_done(struct http_connection *conn, enum http_result res)
> {
> struct ibuf *b;
>
> @@ -273,10 +273,8 @@ http_done(struct http_connection *conn,
> if ((b = ibuf_dynamic(64, UINT_MAX)) == NULL)
> err(1, NULL);
> io_simple_buffer(b, >id, sizeof(conn->id));
> - io_simple_buffer(b, , sizeof(ok));
> -#if 0/* TODO: cache last_modified */
> + io_simple_buffer(b, , sizeof(res));
> io_str_buffer(b, conn->last_modified);
> -#endif
> ibuf_close(, b);
> }
>
> @@ -284,12 +282,13 @@ static void
> http_fail(size_t id)
> {
> struct ibuf *b;
> - int ok = 0;
> + enum http_result res = HTTP_FAILED;
>
> if ((b = ibuf_dynamic(8, UINT_MAX)) == NULL)
> err(1, NULL);
> io_simple_buffer(b, , sizeof(id));
> - io_simple_buffer(b, , sizeof(ok));
> + io_simple_buffer(b, , sizeof(res));
> + io_str_buffer(b, NULL);
> ibuf_close(, b);
> }
>
> @@ -434,7 +433,7 @@ http_redirect(struct http_connection *co
> {
> char *host, *port, *path;
>
> - warnx("redirect to %s", http_info(uri));
> + logx("redirect to %s", http_info(uri));
>
> if (http_parse_uri(uri, , , ) == -1) {
> free(uri);
> @@ -705,6 +704,7 @@ http_parse_status(struct http_connection
> case 302:
> case 303:
> case 307:
> + case 308:
> if (conn->redirect_loop++ > 10) {
> warnx("%s: Too many redirections requested",
> http_info(conn->url));
> @@ -712,7 +712,6 @@ http_parse_status(struct http_connection
> }
> /* FALLTHROUGH */
> case 200:
> - case 206:
> case 304:
> conn->status = status;
> break;
> @@ -729,7 +728,7 @@ static inline int
> http_isredirect(struct http_connection *conn)
> {
> if ((conn->status >= 301 && conn->status <= 303) ||
> - conn->status == 307)
> + conn->status == 307 || conn->status == 308)
> return 1;
> return 0;
> }
> @@ -865,7 +864,7 @@ http_parse_chunked(struct http_connectio
> conn->chunksz = chunksize;
>
> if (conn->chunksz == 0) {
> - http_done(conn, 1);
> + http_done(conn, HTTP_OK);
> return 0;
> }
>
> @@ -970,7 +969,7 @@ data_write(struct http_connection *conn)
> memmove(conn->buf, conn->buf + s, conn->bufpos);
>
> if (conn->bytes == conn->filesize) {
> - http_done(conn, 1);
> + http_done(conn, HTTP_OK);
> return 0;
> }
>
> @@ -1065,10 +1064,13 @@ http_nextstep(struct http_connection *co
> conn->state = STATE_RESPONSE_HEADER;
> return WANT_POLLIN;
> case STATE_RESPONSE_HEADER:
> - if (conn->status == 200)
> + if (conn->status == 200) {
> conn->state = STATE_RESPONSE_DATA;
> - else {
> - http_done(conn, 0);
> + } else {
> + if (conn->status == 304)
> + http_done(conn, HTTP_NOT_MOD);
> + else
> + http_done(conn, HTTP_FAILED);
> return http_close(conn);
> }
> return WANT_POLLIN;
> Index: main.c
>
Possible negative index on ieee80211_ra.c
Hi,
As the comment already explains `mcs' may come invalid from the the hardware
and then code uses it as index before actually checking its value. The patch
below adjusts it so that it's only used after the check.
Reported on CID 1502921.
Only compile tested. Comments, OK?
Index: ieee80211_ra.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_ra.c,v
retrieving revision 1.1
diff -u -p -u -1 -0 -r1.1 ieee80211_ra.c
--- ieee80211_ra.c 12 Mar 2021 16:26:27 - 1.1
+++ ieee80211_ra.c 24 Mar 2021 08:34:33 -
@@ -537,34 +537,35 @@ ieee80211_ra_valid_rates(struct ieee8021
}
void
ieee80211_ra_add_stats_ht(struct ieee80211_ra_node *rn,
struct ieee80211com *ic, struct ieee80211_node *ni,
int mcs, uint32_t total, uint32_t fail)
{
static const uint64_t alpha = RA_FP_1 / 8; /* 1/8 = 0.125 */
static const uint64_t beta = RA_FP_1 / 4; /* 1/4 = 0.25 */
int s, sgi20;
- struct ieee80211_ra_goodput_stats *g = >g[mcs];
+ struct ieee80211_ra_goodput_stats *g;
uint64_t sfer, rate, delta;
/*
* Ignore invalid values. These values may come from hardware
* so asserting valid values via panic is not appropriate.
*/
if (mcs < 0 || mcs >= IEEE80211_HT_RATESET_NUM_MCS)
return;
if (total == 0)
return;
s = splnet();
+ g = >g[mcs];
g->nprobe_pkts += total;
g->nprobe_fail += fail;
if (g->nprobe_pkts < IEEE80211_RA_MIN_PROBE_FRAMES &&
g->nprobe_fail < IEEE80211_RA_MAX_PROBE_RETRIES) {
splx(s);
return;
}
if (g->nprobe_fail > g->nprobe_pkts) {
Re: Possible negative index on ieee80211_ra.c
On Thu, Mar 25, 2021 at 11:48:22AM +, Ricardo Mestre wrote:
> Hi,
>
> As the comment already explains `mcs' may come invalid from the the hardware
> and then code uses it as index before actually checking its value. The patch
> below adjusts it so that it's only used after the check.
>
> Reported on CID 1502921.
>
> Only compile tested. Comments, OK?
ok stsp@
Thanks!
>
> Index: ieee80211_ra.c
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211_ra.c,v
> retrieving revision 1.1
> diff -u -p -u -1 -0 -r1.1 ieee80211_ra.c
> --- ieee80211_ra.c12 Mar 2021 16:26:27 - 1.1
> +++ ieee80211_ra.c24 Mar 2021 08:34:33 -
> @@ -537,34 +537,35 @@ ieee80211_ra_valid_rates(struct ieee8021
> }
>
> void
> ieee80211_ra_add_stats_ht(struct ieee80211_ra_node *rn,
> struct ieee80211com *ic, struct ieee80211_node *ni,
> int mcs, uint32_t total, uint32_t fail)
> {
> static const uint64_t alpha = RA_FP_1 / 8; /* 1/8 = 0.125 */
> static const uint64_t beta = RA_FP_1 / 4; /* 1/4 = 0.25 */
> int s, sgi20;
> - struct ieee80211_ra_goodput_stats *g = >g[mcs];
> + struct ieee80211_ra_goodput_stats *g;
> uint64_t sfer, rate, delta;
>
> /*
>* Ignore invalid values. These values may come from hardware
>* so asserting valid values via panic is not appropriate.
>*/
> if (mcs < 0 || mcs >= IEEE80211_HT_RATESET_NUM_MCS)
> return;
> if (total == 0)
> return;
>
> s = splnet();
>
> + g = >g[mcs];
> g->nprobe_pkts += total;
> g->nprobe_fail += fail;
>
> if (g->nprobe_pkts < IEEE80211_RA_MIN_PROBE_FRAMES &&
> g->nprobe_fail < IEEE80211_RA_MAX_PROBE_RETRIES) {
> splx(s);
> return;
> }
>
> if (g->nprobe_fail > g->nprobe_pkts) {
>
Re: sdmmc(4) off-by-one on boundary check
On Wed, Mar 24, 2021 at 09:23:22PM +0100, Mark Kettenis wrote:
> > Date: Wed, 24 Mar 2021 20:58:48 +0100
> > From: Marcus Glocker
> >
> > On Tue, Mar 23, 2021 at 09:52:42AM -0600, Theo de Raadt wrote:
> >
> > > Mark Kettenis wrote:
> > >
> > > > > > Index: sys/dev/sdmmc/sdmmc_scsi.c
> > > > > > ===
> > > > > > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_scsi.c,v
> > > > > > retrieving revision 1.59
> > > > > > diff -u -p -u -p -r1.59 sdmmc_scsi.c
> > > > > > --- sys/dev/sdmmc/sdmmc_scsi.c 15 Oct 2020 13:22:13 -
> > > > > > 1.59
> > > > > > +++ sys/dev/sdmmc/sdmmc_scsi.c 23 Mar 2021 07:32:52 -
> > > > > > @@ -365,9 +365,8 @@ sdmmc_scsi_cmd(struct scsi_xfer *xs)
> > > > > > /* A read or write operation. */
> > > > > > sdmmc_scsi_decode_rw(xs, , );
> > > > > >
> > > > > > - if (blockno >= tgt->card->csd.capacity ||
> > > > > > - blockno + blockcnt > tgt->card->csd.capacity) {
> > > > > > - DPRINTF(("%s: out of bounds %u-%u >= %u\n", DEVNAME(sc),
> > > > > > + if (blockno + blockcnt >= tgt->card->csd.capacity) {
> > > > > > + DPRINTF(("%s: out of bounds %u+%u >= %u\n", DEVNAME(sc),
> > > > > > blockno, blockcnt, tgt->card->csd.capacity));
> > > > > > xs->error = XS_DRIVER_STUFFUP;
> > > > > > scsi_done(xs);
> > > >
> > > > Apart from a potential overflow, the original condition looks correct
> > > > to me. If csd.capacity is 16 and blockno is 0 and blockcnt is 16, the
> > > > operation should succeed, shouldn't it?
> >
> > Yes, you're right. In my case the patch suppressed the issue by
> > skipping the last block of the disk.
> >
> > > Hmm. Maybe this is a specific SD card with a bug accessing the last
> > > sector.
> > >
> > > A manual dd using
> > >
> > > skip='c partition size -1' count=1 bs=512
> > >
> > > Is failing on that SD card, but it is working on other SD cards. Which
> > > appear
> > > to all be SD v2 cards.
> > >
> > > Maybe his specific SD card has an off-by-one bug relating to the last
> > > sector,
> > > and we need to determine a way to handle that, or skip the last sector on
> > > all devices of this sub-class?
> > >
> > > I don't see any issue in the controller code. Marcus, can you move the
> > > card
> > > to another machine (different controller) and use dd to read the last
> > > sector?
> > > Though I suspect that might not provide enough clue either, the "fail to
> > > read"
> > > behaviour might vary between controllers...
> > >
> > > > > blockno and blockcnt are both 32-bit, so this feels dangerously close
> > > > > to integer wrap, and I believe the first condition cannot be deleted.
> > > >
> > > > But the second condition can still wrap. So it probably needs to be
> > > > something like:
> > > >
> > > >if (blockno >= tgt->card->csd.capacity ||
> > > >blockcnt > tgt->card->csd.capacity - blockno)
> > >
> > > All 3 variables are int. Not sure how moving the + to - on the other
> > > side changes anything.
> >
> > In the meantime I figured out how to trigger the SD controller command
> > to fail. It always happens if you read more than one block through one
> > command, where the last block is accessing the last disk block. If you
> > only access the last disk block directly, the command doesn't fail.
> > Some examples:
> >
> > # dd if=/dev/rsd0c of=/dev/null skip=15591935 bs=4096 count=1
> > nblks=8, datalen=4096, blklen=512, arg=0x76f4ff8, cmd=0x8052
> > CMD FAIL!
> > bcmsdhost0: transfer timeout!
> >
> > # dd if=/dev/rsd0c of=/dev/null skip=31183871 bs=2048 count=1
> > CMD FAIL!
> > bcmsdhost0: transfer timeout!
> >
> > # dd if=/dev/rsd0c of=/dev/null skip=62367743 bs=1024 count=1
> > CMD FAIL!
> > bcmsdhost0: transfer timeout!
> >
> > # dd if=/dev/rsd0c of=/dev/null skip=124735487 bs=512 count=1
> > < all good >
> >
> > I don't think it's an issue with the microSD card since I have NetBSD
> > installed on a different card of exactly the same type/size, and there I
> > also can trigger the issue:
> >
> > # dd if=/dev/rld0c of=/dev/null skip=15591935 bs=4096 count=1
> > [ 179.4170221] ld0c: error reading fsbn 124735480 of 124735480-124735487
> > (ld0 bn 124735480; cn 60905 tn 63 sn 24), retrying
> >
> > Also I have tried that on the Pine64/sximmc(4) with the same microSD
> > card, and there all is hunky-dory.
> >
> > It seems to be a behavior/bug of the BCM2835 SD controller. But I'm
> > not sure what's the right approach to work around that in the driver, or
> > even if there is a real fix by changing the command code.
> >
> > I mean one can just change the disklabel to skip the last block, but
> > it's not really a solution since somebody lazy like me, who does a
> > very simple disklabel where 'a'=root, and 'b'=swap is the rest of the
> > disk can easily trigger that "bug" through e.g. savecore(8)
rpki-client http client adjustments
This diff is mostly cleanup and adding the missing bits needed for RRDP.
Instead of a simple bool ok use an enum to report the state back.
Can be fail, ok or not-modified (the last is used for 304 Not Modified
answers (if a If-Modified-Since header was passed in the request).
Additionally add 308 redirect support and remove 206 as a valid status
response. The http client does not do range requests so a 206 return is
never expected.
--
:wq Claudio
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.55
diff -u -p -r1.55 extern.h
--- extern.h19 Mar 2021 13:56:10 - 1.55
+++ extern.h25 Mar 2021 09:10:04 -
@@ -264,6 +264,12 @@ enum rtype {
RTYPE_GBR,
};
+enum http_result {
+ HTTP_FAILED,/* anything else */
+ HTTP_OK,/* 200 OK */
+ HTTP_NOT_MOD, /* 304 Not Modified */
+};
+
/*
* An entity (MFT, ROA, certificate, etc.) that needs to be downloaded
* and parsed.
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.8
diff -u -p -r1.8 http.c
--- http.c 18 Mar 2021 16:15:19 - 1.8
+++ http.c 25 Mar 2021 09:12:18 -
@@ -264,7 +264,7 @@ http_resolv(struct http_connection *conn
}
static void
-http_done(struct http_connection *conn, int ok)
+http_done(struct http_connection *conn, enum http_result res)
{
struct ibuf *b;
@@ -273,10 +273,8 @@ http_done(struct http_connection *conn,
if ((b = ibuf_dynamic(64, UINT_MAX)) == NULL)
err(1, NULL);
io_simple_buffer(b, >id, sizeof(conn->id));
- io_simple_buffer(b, , sizeof(ok));
-#if 0 /* TODO: cache last_modified */
+ io_simple_buffer(b, , sizeof(res));
io_str_buffer(b, conn->last_modified);
-#endif
ibuf_close(, b);
}
@@ -284,12 +282,13 @@ static void
http_fail(size_t id)
{
struct ibuf *b;
- int ok = 0;
+ enum http_result res = HTTP_FAILED;
if ((b = ibuf_dynamic(8, UINT_MAX)) == NULL)
err(1, NULL);
io_simple_buffer(b, , sizeof(id));
- io_simple_buffer(b, , sizeof(ok));
+ io_simple_buffer(b, , sizeof(res));
+ io_str_buffer(b, NULL);
ibuf_close(, b);
}
@@ -434,7 +433,7 @@ http_redirect(struct http_connection *co
{
char *host, *port, *path;
- warnx("redirect to %s", http_info(uri));
+ logx("redirect to %s", http_info(uri));
if (http_parse_uri(uri, , , ) == -1) {
free(uri);
@@ -705,6 +704,7 @@ http_parse_status(struct http_connection
case 302:
case 303:
case 307:
+ case 308:
if (conn->redirect_loop++ > 10) {
warnx("%s: Too many redirections requested",
http_info(conn->url));
@@ -712,7 +712,6 @@ http_parse_status(struct http_connection
}
/* FALLTHROUGH */
case 200:
- case 206:
case 304:
conn->status = status;
break;
@@ -729,7 +728,7 @@ static inline int
http_isredirect(struct http_connection *conn)
{
if ((conn->status >= 301 && conn->status <= 303) ||
- conn->status == 307)
+ conn->status == 307 || conn->status == 308)
return 1;
return 0;
}
@@ -865,7 +864,7 @@ http_parse_chunked(struct http_connectio
conn->chunksz = chunksize;
if (conn->chunksz == 0) {
- http_done(conn, 1);
+ http_done(conn, HTTP_OK);
return 0;
}
@@ -970,7 +969,7 @@ data_write(struct http_connection *conn)
memmove(conn->buf, conn->buf + s, conn->bufpos);
if (conn->bytes == conn->filesize) {
- http_done(conn, 1);
+ http_done(conn, HTTP_OK);
return 0;
}
@@ -1065,10 +1064,13 @@ http_nextstep(struct http_connection *co
conn->state = STATE_RESPONSE_HEADER;
return WANT_POLLIN;
case STATE_RESPONSE_HEADER:
- if (conn->status == 200)
+ if (conn->status == 200) {
conn->state = STATE_RESPONSE_DATA;
- else {
- http_done(conn, 0);
+ } else {
+ if (conn->status == 304)
+ http_done(conn, HTTP_NOT_MOD);
+ else
+ http_done(conn, HTTP_FAILED);
return http_close(conn);
}
return WANT_POLLIN;
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.122
diff -u -p -r1.122 main.c
--- main.c 19 Mar 2021 13:56:10 - 1.122
+++ main.c 25 Mar 2021 09:15:22
rge(4): move tx/rx descriptors into their own structs
Hi,
The diff below moves tx/rx descriptors into their own structs.
This is a first step toward making rge work with multiple queues and interrupts.
Only one queue is currently used.
While here, update the RTL8125B microcode.
Index: sys/dev/pci/if_rge.c
===
RCS file: /cvs/src/sys/dev/pci/if_rge.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 if_rge.c
--- sys/dev/pci/if_rge.c11 Feb 2021 16:22:06 - 1.12
+++ sys/dev/pci/if_rge.c25 Mar 2021 09:14:17 -
@@ -61,7 +61,7 @@ int rge_match(struct device *, void *,
void rge_attach(struct device *, struct device *, void *);
intrge_activate(struct device *, int);
intrge_intr(void *);
-intrge_encap(struct rge_softc *, struct mbuf *, int);
+intrge_encap(struct rge_queues *, struct mbuf *, int);
intrge_ioctl(struct ifnet *, u_long, caddr_t);
void rge_start(struct ifqueue *);
void rge_watchdog(struct ifnet *);
@@ -70,13 +70,13 @@ voidrge_stop(struct ifnet *);
intrge_ifmedia_upd(struct ifnet *);
void rge_ifmedia_sts(struct ifnet *, struct ifmediareq *);
intrge_allocmem(struct rge_softc *);
-intrge_newbuf(struct rge_softc *);
-void rge_discard_rxbuf(struct rge_softc *, int);
-void rge_rx_list_init(struct rge_softc *);
-void rge_tx_list_init(struct rge_softc *);
-void rge_fill_rx_ring(struct rge_softc *);
-intrge_rxeof(struct rge_softc *);
-intrge_txeof(struct rge_softc *);
+intrge_newbuf(struct rge_queues *);
+void rge_discard_rxbuf(struct rge_queues *, int);
+void rge_rx_list_init(struct rge_queues *);
+void rge_tx_list_init(struct rge_queues *);
+void rge_fill_rx_ring(struct rge_queues *);
+intrge_rxeof(struct rge_queues *);
+intrge_txeof(struct rge_queues *);
void rge_reset(struct rge_softc *);
void rge_iff(struct rge_softc *);
void rge_set_phy_power(struct rge_softc *, int);
@@ -159,6 +159,7 @@ rge_attach(struct device *parent, struct
pci_intr_handle_t ih;
const char *intrstr = NULL;
struct ifnet *ifp;
+ struct rge_queues *q;
pcireg_t reg;
uint32_t hwrev;
uint8_t eaddr[ETHER_ADDR_LEN];
@@ -184,6 +185,17 @@ rge_attach(struct device *parent, struct
}
}
+ q = malloc(sizeof(struct rge_queues), M_DEVBUF, M_NOWAIT | M_ZERO);
+ if (q == NULL) {
+ printf(": unable to allocate queue memory\n");
+ return;
+ }
+ q->q_sc = sc;
+ q->q_index = 0;
+
+ sc->sc_queues = q;
+ sc->sc_nqueues = 1;
+
/*
* Allocate interrupt.
*/
@@ -323,9 +335,10 @@ int
rge_intr(void *arg)
{
struct rge_softc *sc = arg;
+ struct rge_queues *q = sc->sc_queues;
struct ifnet *ifp = >sc_arpcom.ac_if;
uint32_t status;
- int claimed = 0, rx, tx;
+ int claimed = 0, rv;
if (!(ifp->if_flags & IFF_RUNNING))
return (0);
@@ -345,29 +358,21 @@ rge_intr(void *arg)
if (status & RGE_ISR_PCS_TIMEOUT)
claimed = 1;
- rx = tx = 0;
+ rv = 0;
if (status & sc->rge_intrs) {
- if (status &
- (sc->rge_rx_ack | RGE_ISR_RX_ERR | RGE_ISR_RX_FIFO_OFLOW)) {
- rx |= rge_rxeof(sc);
- claimed = 1;
- }
-
- if (status & (sc->rge_tx_ack | RGE_ISR_TX_ERR)) {
- tx |= rge_txeof(sc);
- claimed = 1;
- }
+ rv |= rge_rxeof(q);
+ rv |= rge_txeof(q);
if (status & RGE_ISR_SYSTEM_ERR) {
KERNEL_LOCK();
rge_init(ifp);
KERNEL_UNLOCK();
- claimed = 1;
}
+ claimed = 1;
}
if (sc->rge_timerintr) {
- if ((tx | rx) == 0) {
+ if (!rv) {
/*
* Nothing needs to be processed, fallback
* to use TX/RX interrupts.
@@ -379,11 +384,11 @@ rge_intr(void *arg)
* race introduced by changing interrupt
* masks.
*/
- rge_rxeof(sc);
- rge_txeof(sc);
+ rge_rxeof(q);
+ rge_txeof(q);
} else
RGE_WRITE_4(sc, RGE_TIMERCNT, 1);
- } else if (tx | rx) {
+ } else if (rv) {
/*
* Assume that using simulated interrupt moderation
* (hardware timer based) could reduce the
Re: rpki-client adjust base64_decode
On Thu, Mar 25, 2021 at 09:57:51AM +0100, Claudio Jeker wrote:
> RRDP has a lot of base64 strings to handle. Because of this adjust the
> base64_decode function in tal.c to take a regular string as input.
> For now keep the function static, will change that once RRDP is ready.
>
> OK?
Since you touch it, I would change the comment to use the common
capitalization Base64 instead of BASE64.
ok
> --
> :wq Claudio
>
> Index: tal.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 tal.c
> --- tal.c 5 Mar 2021 17:15:19 - 1.28
> +++ tal.c 25 Mar 2021 08:54:27 -
> @@ -28,11 +28,11 @@
> #include "extern.h"
>
> static int
> -base64_decode(const unsigned char *in, size_t inlen, unsigned char **out,
> - size_t *outlen)
> +base64_decode(const unsigned char *in, unsigned char **out, size_t *outlen)
> {
> static EVP_ENCODE_CTX *ctx;
> unsigned char *to;
> + size_t inlen;
> int tolen;
>
> if (ctx == NULL && (ctx = EVP_ENCODE_CTX_new()) == NULL)
> @@ -41,6 +41,7 @@ base64_decode(const unsigned char *in, s
> *out = NULL;
> *outlen = 0;
>
> + inlen = strlen(in);
> if (inlen >= INT_MAX - 3)
> return -1;
> tolen = ((inlen + 3) / 4) * 3 + 1;
> @@ -81,7 +82,7 @@ tal_parse_buffer(const char *fn, char *b
> {
> char*nl, *line, *f, *file = NULL;
> unsigned char *der;
> - size_t sz, dersz;
> + size_t dersz;
> int rc = 0;
> struct tal *tal = NULL;
> EVP_PKEY*pkey = NULL;
> @@ -147,16 +148,12 @@ tal_parse_buffer(const char *fn, char *b
> /* sort uri lexicographically so https:// is preferred */
> qsort(tal->uri, tal->urisz, sizeof(tal->uri[0]), tal_cmp);
>
> - sz = strlen(buf);
> - if (sz == 0) {
> + /* Now the BASE64-encoded public key. */
> + if ((base64_decode(buf, , )) == -1) {
> warnx("%s: RFC 7730 section 2.1: subjectPublicKeyInfo: "
> - "zero-length public key", fn);
> + "bad public key", fn);
> goto out;
> }
> -
> - /* Now the BASE64-encoded public key. */
> - if ((base64_decode(buf, sz, , )) == -1)
> - errx(1, "base64 decode");
>
> tal->pkey = der;
> tal->pkeysz = dersz;
>
rpki-client adjust base64_decode
RRDP has a lot of base64 strings to handle. Because of this adjust the
base64_decode function in tal.c to take a regular string as input.
For now keep the function static, will change that once RRDP is ready.
OK?
--
:wq Claudio
Index: tal.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
retrieving revision 1.28
diff -u -p -r1.28 tal.c
--- tal.c 5 Mar 2021 17:15:19 - 1.28
+++ tal.c 25 Mar 2021 08:54:27 -
@@ -28,11 +28,11 @@
#include "extern.h"
static int
-base64_decode(const unsigned char *in, size_t inlen, unsigned char **out,
- size_t *outlen)
+base64_decode(const unsigned char *in, unsigned char **out, size_t *outlen)
{
static EVP_ENCODE_CTX *ctx;
unsigned char *to;
+ size_t inlen;
int tolen;
if (ctx == NULL && (ctx = EVP_ENCODE_CTX_new()) == NULL)
@@ -41,6 +41,7 @@ base64_decode(const unsigned char *in, s
*out = NULL;
*outlen = 0;
+ inlen = strlen(in);
if (inlen >= INT_MAX - 3)
return -1;
tolen = ((inlen + 3) / 4) * 3 + 1;
@@ -81,7 +82,7 @@ tal_parse_buffer(const char *fn, char *b
{
char*nl, *line, *f, *file = NULL;
unsigned char *der;
- size_t sz, dersz;
+ size_t dersz;
int rc = 0;
struct tal *tal = NULL;
EVP_PKEY*pkey = NULL;
@@ -147,16 +148,12 @@ tal_parse_buffer(const char *fn, char *b
/* sort uri lexicographically so https:// is preferred */
qsort(tal->uri, tal->urisz, sizeof(tal->uri[0]), tal_cmp);
- sz = strlen(buf);
- if (sz == 0) {
+ /* Now the BASE64-encoded public key. */
+ if ((base64_decode(buf, , )) == -1) {
warnx("%s: RFC 7730 section 2.1: subjectPublicKeyInfo: "
- "zero-length public key", fn);
+ "bad public key", fn);
goto out;
}
-
- /* Now the BASE64-encoded public key. */
- if ((base64_decode(buf, sz, , )) == -1)
- errx(1, "base64 decode");
tal->pkey = der;
tal->pkeysz = dersz;
Re: httpd: Allow overriding global "no index"
I've also noticed this some time ago. My suggestion was to simply
permit auto-index locations inside no-index servers.
https://marc.info/?l=openbsd-tech=160302351622195=2
On 2021-03-24 22:37, Quentin Rameau wrote:
> Hello,
>
> It's been noted that the "directory no index" configuration
> always shadow any other directory index configuration,
> for example:
>
> directory no index
> location "/foo/" {
> directory auto index
> }
>
> Even when requesting /foo/,
> the global no index option
> overrides the location-specific auto index.
>
> This looks a bit too restrictive
> and contrary to having location-specific options.
>
> Here's a patch proposal modifying the current behaviour.
> Note that if multiple contradictory options
> are defined at the same level,
> the most restrictive option, no index,
> will still have priority over the others.
>
> What do you think?
>
> Index: config.c
> ===
> RCS file: /var/cvs/src/usr.sbin/httpd/config.c,v
> retrieving revision 1.61
> diff -u -p -u -r1.61 config.c
> --- config.c 21 Sep 2020 09:42:07 - 1.61
> +++ config.c 24 Mar 2021 21:26:39 -
> @@ -451,7 +451,7 @@ config_getserver_config(struct httpd *en
> #endif
> struct server_config*srv_conf, *parent;
> uint8_t *p = imsg->data;
> - unsigned int f;
> + unsigned int f, fo;
> size_t s;
>
> if ((srv_conf = calloc(1, sizeof(*srv_conf))) == NULL)
> @@ -489,14 +489,19 @@ config_getserver_config(struct httpd *en
> /* Inherit configuration from the parent */
> f = SRVFLAG_INDEX|SRVFLAG_NO_INDEX;
> if ((srv_conf->flags & f) == 0) {
> - srv_conf->flags |= parent->flags & f;
> + fo = SRVFLAG_AUTO_INDEX|SRVFLAG_NO_AUTO_INDEX;
> + if ((srv_conf->flags & fo) == 0)
> + srv_conf->flags |= parent->flags & f;
> (void)strlcpy(srv_conf->index, parent->index,
> sizeof(srv_conf->index));
> }
>
> f = SRVFLAG_AUTO_INDEX|SRVFLAG_NO_AUTO_INDEX;
> - if ((srv_conf->flags & f) == 0)
> - srv_conf->flags |= parent->flags & f;
> + if ((srv_conf->flags & f) == 0) {
> + fo = SRVFLAG_INDEX|SRVFLAG_NO_INDEX;
> + if ((srv_conf->flags & fo) == 0)
> + srv_conf->flags |= parent->flags & f;
> + }
>
> f = SRVFLAG_ROOT;
> if ((srv_conf->flags & f) == 0) {
>
Re: [External] : [ICMP] IP options lead to malformed reply
> -Original Message-
> From: Alexandr Nedvedicky
> Sent: Mittwoch, 24. März 2021 23:09
> To: Schreilechner, Dominik (RC-AT DI FA DH-GRAZ ICO)
>
> Cc: tech@openbsd.org
> Subject: Re: [External] : [ICMP] IP options lead to malformed reply
>
> Hello,
>
>
>
> > We really need to fix ip_send() such the output task will handle IP
> options
> > properly.
>
> took a look at bug reported by Dominik earlier today. Looks like
> there are two issues:
>
> 1) ip_insertoptions() does not update length of IP header (ip_hl)
>
> 2) ip_hl is being overridden anyway later in ip_output() called
> by ip_send_dispatch() to send ICMP error packet out. Looks
> like ip_send_dispatch() should use IP_RAWOUTPUT flag so
> ip_hl won't get overridden.
Maybe I have overlooked something, but I see two problems with this.
When the IP_RAWOUTPUT flag is set, all packets send via ip_send() are
no longer included in the ips_localout statistic. Also, all callers of
ip_send() need to fill out the IP header themselves (as it would be
done in the beginning of ip_output() without the IP_RAWOUTPUT flag). As
far as I can tell this is not done for packets in a gif tunnel (e.g.
the ip_id is not calculated/added).
>
> Diff below fixes both issues. We still have no good story when
> ip_insertoptions() fails. I'll send another diff later this week.
>
>
> diff below makes 'nping --ip-options T ...' to work. OK?
>
> thanks and
> regards
> sashan
Regards
Dominik
>
> 8<---8<---8<--8<
> diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
> index 0ec3f723be4..234c798e7d4 100644
> --- a/sys/netinet/ip_input.c
> +++ b/sys/netinet/ip_input.c
> @@ -1789,7 +1789,7 @@ ip_send_dispatch(void *xmq)
>
> NET_LOCK();
> while ((m = ml_dequeue()) != NULL) {
> - ip_output(m, NULL, NULL, 0, NULL, NULL, 0);
> + ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL,
> 0);
> }
> NET_UNLOCK();
> }
> diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
> index c01a3e7803c..ea803077304 100644
> --- a/sys/netinet/ip_output.c
> +++ b/sys/netinet/ip_output.c
> @@ -765,6 +765,11 @@ ip_insertoptions(struct mbuf *m, struct mbuf
> *opt, int *phlen)
> optlen = opt->m_len - sizeof(p->ipopt_dst);
> if (optlen + ntohs(ip->ip_len) > IP_MAXPACKET)
> return (m); /* XXX should fail */
> +
> + /* check if options will fit to IP header */
> + if ((optlen + (ip->ip_hl << 2)) > (0x0f << 2))
> + return (m);
> +
> if (p->ipopt_dst.s_addr)
> ip->ip_dst = p->ipopt_dst;
> if (m->m_flags & M_EXT || m->m_data - optlen < m->m_pktdat)
> {
> @@ -790,6 +795,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf
> *opt, int *phlen)
> memcpy(ip + 1, p->ipopt_list, optlen);
> *phlen = sizeof(struct ip) + optlen;
> ip->ip_len = htons(ntohs(ip->ip_len) + optlen);
> + ip->ip_hl += (optlen >> 2);
> return (m);
> }
>
