sparc64 cpu uvm_km_valloc()
Continuing to convert uvm_km_valloc() calls to km_alloc(), sparc64's
struct cpu_info wants to be allocated on an 8 page boundary, so it needs
a custom kmem_va_mode. My T5120 didn't blow up with this, so I think it
works.
ok?
Index: arch/sparc64/sparc64/cpu.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/cpu.c,v
retrieving revision 1.71
diff -u -p -u -p -r1.71 cpu.c
--- arch/sparc64/sparc64/cpu.c 31 Jul 2020 11:19:12 - 1.71
+++ arch/sparc64/sparc64/cpu.c 21 Dec 2020 05:12:32 -
@@ -113,6 +113,12 @@ void hummingbird_init(struct cpu_info *c
#defineIU_IMPL(v) u_int64_t)(v))_IMPL) >> VER_IMPL_SHIFT)
#defineIU_VERS(v) u_int64_t)(v))_MASK) >> VER_MASK_SHIFT)
+/* virtual address allocation mode for struct cpu_info */
+struct kmem_va_mode kv_cpu_info = {
+ .kv_map = _map,
+ .kv_align = 8 * PAGE_SIZE
+};
+
struct cpu_info *
alloc_cpuinfo(struct mainbus_attach_args *ma)
{
@@ -137,7 +143,7 @@ alloc_cpuinfo(struct mainbus_attach_args
if (cpi->ci_upaid == portid)
return cpi;
- va = uvm_km_valloc_align(kernel_map, sz, 8 * PAGE_SIZE, 0);
+ va = (vaddr_t)km_alloc(sz, _cpu_info, _none, _nowait);
if (va == 0)
panic("alloc_cpuinfo: no virtual space");
va0 = va;
[PATCH] Reduce case duplication in kern_sysctl
I tested this by diff'ing sysctl output before/after on amd64. Since
there's a bunch of ifdef'ness I verified RAMDISK still builds.
I deliberately didn't fix the indentation to keep this diff a pure line
motion (would run over 80 chars otherwise). I can either fix that it in
a separate commit or in this one before submitting.
OK?
Subject: [PATCH] Reduce case duplication in kern_sysctl
This changes amd64 GENERIC.MP .text size of kern_sysctl.o from 6440 to 6400.
Surprisingly, RAMDISK grows from 1645 to 1678.
---
sys/kern/kern_sysctl.c | 180 ++---
1 file changed, 79 insertions(+), 101 deletions(-)
diff --git sys/kern/kern_sysctl.c sys/kern/kern_sysctl.c
index 82010e3ee1c..72686560b5d 100644
--- sys/kern/kern_sysctl.c
+++ sys/kern/kern_sysctl.c
@@ -356,31 +356,87 @@ kern_sysctl(int *name, u_int namelen, void *oldp, size_t
*oldlenp, void *newp,
dev_t dev;
extern int pool_debug;
- /* all sysctl names at this level are terminal except a ton of them */
+ /* dispatch the non-terminal nodes first */
if (namelen != 1) {
switch (name[0]) {
- case KERN_PROC:
- case KERN_PROF:
- case KERN_MALLOCSTATS:
- case KERN_TTY:
- case KERN_POOL:
- case KERN_PROC_ARGS:
- case KERN_PROC_CWD:
- case KERN_PROC_NOBROADCASTKILL:
- case KERN_PROC_VMMAP:
- case KERN_SYSVIPC_INFO:
- case KERN_SEMINFO:
- case KERN_SHMINFO:
- case KERN_INTRCNT:
- case KERN_WATCHDOG:
- case KERN_EVCOUNT:
- case KERN_TIMECOUNTER:
- case KERN_CPTIME2:
- case KERN_FILE:
- case KERN_WITNESS:
- case KERN_AUDIO:
- case KERN_CPUSTATS:
- break;
+#ifndef SMALL_KERNEL
+ case KERN_PROC:
+ return (sysctl_doproc(name + 1, namelen - 1, oldp, oldlenp));
+ case KERN_PROC_ARGS:
+ return (sysctl_proc_args(name + 1, namelen - 1, oldp, oldlenp,
+p));
+ case KERN_PROC_CWD:
+ return (sysctl_proc_cwd(name + 1, namelen - 1, oldp, oldlenp,
+p));
+ case KERN_PROC_NOBROADCASTKILL:
+ return (sysctl_proc_nobroadcastkill(name + 1, namelen - 1,
+newp, newlen, oldp, oldlenp, p));
+ case KERN_PROC_VMMAP:
+ return (sysctl_proc_vmmap(name + 1, namelen - 1, oldp, oldlenp,
+p));
+ case KERN_FILE:
+ return (sysctl_file(name + 1, namelen - 1, oldp, oldlenp, p));
+#endif
+#if defined(GPROF) || defined(DDBPROF)
+ case KERN_PROF:
+ return (sysctl_doprof(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+ case KERN_MALLOCSTATS:
+ return (sysctl_malloc(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen, p));
+ case KERN_TTY:
+ return (sysctl_tty(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+ case KERN_POOL:
+ return (sysctl_dopool(name + 1, namelen - 1, oldp, oldlenp));
+#if defined(SYSVMSG) || defined(SYSVSEM) || defined(SYSVSHM)
+ case KERN_SYSVIPC_INFO:
+ return (sysctl_sysvipc(name + 1, namelen - 1, oldp, oldlenp));
+#endif
+#ifdef SYSVSEM
+ case KERN_SEMINFO:
+ return (sysctl_sysvsem(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+#ifdef SYSVSHM
+ case KERN_SHMINFO:
+ return (sysctl_sysvshm(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+#ifndef SMALL_KERNEL
+ case KERN_INTRCNT:
+ return (sysctl_intrcnt(name + 1, namelen - 1, oldp, oldlenp));
+ case KERN_WATCHDOG:
+ return (sysctl_wdog(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+#ifndef SMALL_KERNEL
+ case KERN_EVCOUNT:
+ return (evcount_sysctl(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+ case KERN_TIMECOUNTER:
+ return (sysctl_tc(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+ case KERN_CPTIME2:
+ return (sysctl_cptime2(name + 1, namelen -1, oldp, oldlenp,
+ newp, newlen));
+#ifdef WITNESS
+ case KERN_WITNESSWATCH:
+ return witness_sysctl_watch(oldp, oldlenp, newp, newlen);
+ case KERN_WITNESS:
+ return witness_sysctl(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen);
+#endif
+#if NAUDIO > 0
+ case KERN_AUDIO:
+ return (sysctl_audio(name + 1, namelen - 1, oldp, oldlenp,
+ newp, newlen));
+#endif
+ case KERN_CPUSTATS:
+
Re: [please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
Scott Cheloha writes: > Short version: > > Please test if this patch breaks suspend or hibernate on your > machine. Reply with the results of your test and your dmesg. > > Both are still working on my Lenovo Thinkpad X1 Carbon Gen 6. This breaks resume from zzz on HP EliteBook 840 G1. The machine comes back with frozen-ish X. Switching between VCs works, so I can reboot. There is nothing undue in dmesg or Xorg.0.log. I shared full machine details (dmesg/acpi) before, e.g. https://www.mail-archive.com/tech@openbsd.org/msg60772.html Sorry for the somewhat unactionable feedback. Let me know if some printfs would help. Thanks Greg
[please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
Short version:
Please test if this patch breaks suspend or hibernate on your
machine. Reply with the results of your test and your dmesg.
Both are still working on my Lenovo Thinkpad X1 Carbon Gen 6.
Long version:
This patch converts the remaining tick sleeps in dev/acpi to
nanosecond sleeps.
In acpiec_wait() we have a tsleep(9) for up to 1 tick. There is no
discernable unit in this code and no timeout so as with all the other
1 tick sleeps I think tsleep_nsec(9) for 1ms will work fine. It may
oversleep 1ms but because there is no timeout it doesn't really
matter.
In acpi_event_wait() we have a timeout in milliseconds. Currently we
sleep for up to 1 tick and deduct time from the timeout when the sleep
returns EWOULDBLOCK. Of note here is that this code is broken on
kernels where hz > 1000: we will never time out.
I have changed the code to rwsleep_nsec(9) for at least 1ms. The
timeout is in milliseconds so this seems logical to me.
The caveat is that on default HZ=100 kernels we will oversleep and it
will take much longer for us to time out in this loop. We can fix
this by measuring the sleep and deducting the elapsed time from the
total timeout. Or, if it doesn't really matter, we can just oversleep
and not worry about it.
Preferences?
Assuming we get no bad tests back, OK?
Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.62
diff -u -p -r1.62 acpiec.c
--- acpiec.c26 Aug 2020 03:29:06 - 1.62
+++ acpiec.c20 Dec 2020 22:19:14 -
@@ -105,8 +105,10 @@ acpiec_wait(struct acpiec_softc *sc, uin
sc->sc_gotsci = 1;
if (cold || (stat & EC_STAT_BURST))
delay(1);
- else
- tsleep(, PWAIT, "acpiec", 1);
+ else {
+ tsleep_nsec(, PWAIT, "acpiec",
+ MSEC_TO_NSEC(1));
+ }
}
dnprintf(40, "%s: EC wait_ns, stat: %b\n", DEVNAME(sc), (int)stat,
Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.257
diff -u -p -r1.257 dsdt.c
--- dsdt.c 17 Dec 2020 17:57:19 - 1.257
+++ dsdt.c 20 Dec 2020 22:19:15 -
@@ -2907,10 +2907,10 @@ acpi_event_wait(struct aml_scope *scope,
if (acpi_dotask(acpi_softc))
continue;
if (!cold) {
- if (rwsleep(evt, _softc->sc_lck, PWAIT,
- "acpievt", 1) == EWOULDBLOCK) {
+ if (rwsleep_nsec(evt, _softc->sc_lck, PWAIT,
+ "acpievt", MSEC_TO_NSEC(1)) == EWOULDBLOCK) {
if (timeout < AML_NO_TIMEOUT)
- timeout -= (1000 / hz);
+ timeout--;
}
} else {
delay(1000);
Re: Poison file names
John, I'm now able to clone the repo from windows following this commit: https://github.com/openbsd/src/commit/6e49571c59fe1f1e78405e5a57a1e8dc40029e00 Two caveats: 1) if you wanted to bisect the repo on windows or checkout any earlier commit than the one above, it won't be possible to do from windows. you'd need to get on a different filesystem. 2) there's no guarantees someone doesn't commit something in the future that's a problem (although hopefully chances are low of that happening) Here's how it looked on my end from windows, only a few regress conflicts remain, but the checkout works now. $ git clone https://github.com/openbsd/src.git Cloning into 'src'... remote: Enumerating objects: 137, done. remote: Counting objects: 100% (137/137), done. remote: Compressing objects: 100% (102/102), done. remote: Total 1983701 (delta 53), reused 85 (delta 35), pack-reused 1983564 Receiving objects: 100% (1983701/1983701), 1.14 GiB | 9.48 MiB/s, done. Resolving deltas: 100% (1579758/1579758), done. Updating files: 100% (89835/89835), done. warning: the following paths have collided (e.g. case-sensitive paths on a case-insensitive filesystem) and only one from the same colliding group is in the working tree: 'regress/usr.bin/jot/regress.wDn.out' 'regress/usr.bin/jot/regress.wdn.out' 'regress/usr.bin/jot/regress.wO.out' 'regress/usr.bin/jot/regress.wo.out' 'regress/usr.bin/jot/regress.wU.out' 'regress/usr.bin/jot/regress.wu.out' 'regress/usr.bin/mandoc/roff/esc/O.in' 'regress/usr.bin/mandoc/roff/esc/o.in' 'regress/usr.bin/mandoc/roff/esc/O.out_ascii' 'regress/usr.bin/mandoc/roff/esc/o.out_ascii' 'regress/usr.bin/rcs/rcs-Aflag.out' 'regress/usr.bin/rcs/rcs-aflag.out' On Fri, Dec 11, 2020 at 1:58 PM wrote: > > I would like to be able to clone the github mirror on windows. I do > wind up > using 7z on the tar file as a workaround, but it would be nice if github > "just worked". The com files is what the clone fails on, and those > seemed > easy enough to address, but if it is actually a deep rat hole, I > certainly > understand. >
Re: sigsuspend(2): use "sigsuspend" for sleep string
On Sun, Dec 20, 2020 at 10:11:07PM +0100, Mark Kettenis wrote:
> > Date: Sun, 20 Dec 2020 14:53:16 -0600
> > From: Scott Cheloha
> >
> > I want to see if a process is waiting in sigsuspend(2) from top(1).
> > The current sleep string is "pause", which leaves me wondering what
> > the process is actually doing. The string "sigsuspend" would make it
> > unambiguous.
> >
> > ok?
>
> No this is too long as it will get truncated. KI_WMESGLEN is 8, so
> "sigsusp" would work.
Whoops, sure thing.
While we're at it, can I truncate "nanosleep" (9) to "nanoslp" (7)?
Index: kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.268
diff -u -p -r1.268 kern_sig.c
--- kern_sig.c 7 Dec 2020 16:55:28 - 1.268
+++ kern_sig.c 20 Dec 2020 21:27:52 -
@@ -523,7 +523,7 @@ sys_sigsuspend(struct proc *p, void *v,
struct sigacts *ps = pr->ps_sigacts;
dosigsuspend(p, SCARG(uap, mask) &~ sigcantmask);
- while (tsleep_nsec(ps, PPAUSE|PCATCH, "pause", INFSLP) == 0)
+ while (tsleep_nsec(ps, PPAUSE|PCATCH, "sigsusp", INFSLP) == 0)
/* void */;
/* always return EINTR rather than ERESTART... */
return (EINTR);
Index: kern_time.c
===
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.150
diff -u -p -r1.150 kern_time.c
--- kern_time.c 10 Nov 2020 17:26:54 - 1.150
+++ kern_time.c 20 Dec 2020 21:27:52 -
@@ -294,7 +294,7 @@ sys_nanosleep(struct proc *p, void *v, r
do {
getnanouptime();
nsecs = MAX(1, MIN(TIMESPEC_TO_NSEC(), MAXTSLP));
- error = tsleep_nsec(, PWAIT | PCATCH, "nanosleep", nsecs);
+ error = tsleep_nsec(, PWAIT | PCATCH, "nanoslp", nsecs);
getnanouptime();
timespecsub(, , );
timespecsub(, , );
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 18:15, Chris Bennett > wrote: > > On Sun, Dec 20, 2020 at 09:51:35AM +0100, Gilles CHEHADE wrote: >> >> >>> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: >>> >>> On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: Hello, Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, smtpd will attempt to search for a ~/.forward file in the recipient directory and process it. This may be convenient for some setups but it is an implicit behavior that's not overridable and not always wanted. This diff changes this behavior by requiring the admins to explicitly allow the forward files processing in the actions when desired: action "local_users" maildir forward-file With this diff, if forward-file is not specified, code to request parent process for an fd is bypassed and the expansion layer just pretends parent couldn't find one. This let the code fallback in an already existing code path with the proper behavior and is very uninvasive. >>> >>> if I could understood the direction (which is fine as it makes the >>> daemon less behaviour dependant on a user settings), the default seems >>> wrong to me (at least for now, and for OpenBSD base specifically). >>> >>> Currently, root@ mail delivery is based on /root/.forward file: >>> install is writing this file to redirect root@ mail to user (if user >>> was created at install-time). It is done this way since 2011 (see >>> distrib/miniroot/install.sh rev 1.218). So I assume that all installs >>> which were done with a user configured, since 2011, could use it. >> >> Yes, the default would need to be changed as follows: >> >> mini$ diff -uNp smtpd.conf smtpd.conf.new >> >> >> --- smtpd.conf Mon Dec 14 22:13:04 2020 >> +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 >> @@ -11,7 +11,7 @@ listen on socket >> # >> listen on all hostname debug.poolp.org >> >> -action "local_mail" maildir alias >> +action "local_mail" maildir alias forward-file >> action "outbound" relay >> > > My src tree still has mbox as the default. There was talk of changing > from mbox to maildir as default. Is this now going forward also? Nope, sorry, this was just an example from my machine which uses maildir so that you get the idea, I don’t propose we name machines “debug.poolp.org” by default either ;-) > While mbox is simple, once I moved to Dovecot for IMAP, changing mbox to > maildir was easy, but needed, amongst some other non-mbox choices. > > I think new users will be much happier learning maildir and skipping the > whole mbox thing. > > My 2 cents. :^) I don’t need to be convinced as I was the one who launched the maildir debate, however base is not ready and mail doesn’t support maildir to begin with. If there was a switch to maildir, the first effect would be that you could not read the mail sent to root at install and the daily, weekly and monthly mails, including the insecurity report.
Re: sigsuspend(2): use "sigsuspend" for sleep string
> Date: Sun, 20 Dec 2020 14:53:16 -0600 > From: Scott Cheloha > > I want to see if a process is waiting in sigsuspend(2) from top(1). > The current sleep string is "pause", which leaves me wondering what > the process is actually doing. The string "sigsuspend" would make it > unambiguous. > > ok? No this is too long as it will get truncated. KI_WMESGLEN is 8, so "sigsusp" would work. > Index: kern_sig.c > === > RCS file: /cvs/src/sys/kern/kern_sig.c,v > retrieving revision 1.268 > diff -u -p -r1.268 kern_sig.c > --- kern_sig.c7 Dec 2020 16:55:28 - 1.268 > +++ kern_sig.c20 Dec 2020 20:49:44 - > @@ -523,7 +523,7 @@ sys_sigsuspend(struct proc *p, void *v, > struct sigacts *ps = pr->ps_sigacts; > > dosigsuspend(p, SCARG(uap, mask) &~ sigcantmask); > - while (tsleep_nsec(ps, PPAUSE|PCATCH, "pause", INFSLP) == 0) > + while (tsleep_nsec(ps, PPAUSE|PCATCH, "sigsuspend", INFSLP) == 0) > /* void */; > /* always return EINTR rather than ERESTART... */ > return (EINTR); > >
sigsuspend(2): use "sigsuspend" for sleep string
I want to see if a process is waiting in sigsuspend(2) from top(1). The current sleep string is "pause", which leaves me wondering what the process is actually doing. The string "sigsuspend" would make it unambiguous. ok? Index: kern_sig.c === RCS file: /cvs/src/sys/kern/kern_sig.c,v retrieving revision 1.268 diff -u -p -r1.268 kern_sig.c --- kern_sig.c 7 Dec 2020 16:55:28 - 1.268 +++ kern_sig.c 20 Dec 2020 20:49:44 - @@ -523,7 +523,7 @@ sys_sigsuspend(struct proc *p, void *v, struct sigacts *ps = pr->ps_sigacts; dosigsuspend(p, SCARG(uap, mask) &~ sigcantmask); - while (tsleep_nsec(ps, PPAUSE|PCATCH, "pause", INFSLP) == 0) + while (tsleep_nsec(ps, PPAUSE|PCATCH, "sigsuspend", INFSLP) == 0) /* void */; /* always return EINTR rather than ERESTART... */ return (EINTR);
Re: WITNESS panic: acquiring blockable sleep lock with spinlock or critical section held (rwlock) kmmaplk
> Date: Sat, 19 Dec 2020 18:07:41 -0300
> From: Martin Pieuchot
>
> On 18/12/20(Fri) 08:04, Todd C. Miller wrote:
> > On Fri, 18 Dec 2020 13:34:39 +0100, Mark Kettenis wrote:
> >
> > > Anyway, your analysis is right. When a kernel thread wants to use
> > > pmap_extract(9) on a userland pmap, it needs to lock pm_apte_mtx to
> > > prevent another thread from simultaniously activating a userland pmap
> > > too. So indeed, pm_apte_mtx needs to be properly initialized for the
> > > kernel pmap.
> > >
> > > However, pm_mtx should never be used for the kernel pmap. If we don't
> > > initialize the lock, witness will help us catching this condition, so
> > > maybe we shouldn't...
> >
> > I think a comment is warranted if we don't want to initialize the
> > lock to prevent someone from fixing this in the future ;-)
>
> A solution based on a comment and a non-enabled by option seems very
> fragile to me. I came up with the idea of poisoning the ipl of the
> mutex. What do you think?
Not sure if it makes sense to do this only for i386. And is
splraise() the right place for the check? Instead of mtx_enter()?
> Index: arch/i386/i386/machdep.c
> ===
> RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.641
> diff -u -p -r1.641 machdep.c
> --- arch/i386/i386/machdep.c 8 Nov 2020 20:37:23 - 1.641
> +++ arch/i386/i386/machdep.c 19 Dec 2020 20:57:03 -
> @@ -3996,6 +3996,8 @@ splraise(int ncpl)
> {
> int ocpl;
>
> + KASSERT(ncpl >= IPL_NONE);
> +
> _SPLRAISE(ocpl, ncpl);
> return (ocpl);
> }
> Index: arch/i386/i386/pmap.c
> ===
> RCS file: /cvs/src/sys/arch/i386/i386/pmap.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 pmap.c
> --- arch/i386/i386/pmap.c 24 Sep 2020 11:36:50 - 1.209
> +++ arch/i386/i386/pmap.c 19 Dec 2020 20:58:48 -
> @@ -961,6 +961,8 @@ pmap_bootstrap(vaddr_t kva_start)
>*/
>
> kpm = pmap_kernel();
> + mtx_init(>pm_mtx, -1); /* must not be used */
> + mtx_init(>pm_apte_mtx, IPL_VM);
> uvm_objinit(>pm_obj, NULL, 1);
> bzero(>pm_list, sizeof(kpm->pm_list)); /* pm_list not used */
> kpm->pm_pdir = (vaddr_t)(proc0.p_addr->u_pcb.pcb_cr3 + KERNBASE);
>
>
Re: sndiod: (mostly) suppress aliasing noise
On Sun, Dec 20, 2020 at 05:58:22PM +0200, Paul Irofti wrote: > Hi, > > Interesting diff. > > I did not have time to look at it thoroughly, but here are a few > observations: > Hi, Thanks for looking at this. - why do you keep the symmetric filter coefficients? (this could halve your > while-loop computations too, right?) > The filter function is indeed symetric: h(t) = h(-t) but they are used at points that are not symetric, we need: ... h(-2 + x) h(-1 + x) h(0 + x) h(1 + x) h(2 + x) ... h(-1 + x) is not the same as h(1 + x), so using the symetry to save CPU cycles is not easy I also tried to store only half of the static table to save few bytes; this makes the code larger (and messy) so the benefit is not that important for such a small filter order. > - the diff's mainlobe look kind of strange at the end, why is that? What > kind of filter did you use? > the side-lobes seem also a bit strange as they > do not decrease monotonically nor are the first sidelobes small and then > increasing To measure the response, we're supposed to apply the filter to a peak. But on the link I sent the plot it the result of the whole 8k->48k conversion, so the peak is not really peak anymore and the result is harder to understand. Here's plot of the Fourier transform of the coefficients in the code: https://vm.caoua.org/src/coef-dft.png which should look way more familiar (x-axis is multiples of the Nyquist frequency, y-axis is the Fourier transform amplitude in dB) The filter is an ideal "sinc" low-pass with the cut-off frequency set to 0.75 of the Nyquist frequency, multiplied by a Blackman window of length 8: h(t) = 0.75 * sinc(0.75 * pi * t) * win(t / 8) where win(t) is the Blackman window function with a = 0.16, defined as: win(t) = 0.5 * ((1 - a) + cos(2 * pi * x) + a * cos(4 * pi * x)) At 48kHz, the transition band starts at 0.75 * 24kHz = 18kHz. Besides preserving audible frequencies, with this choice the ideal filter's impulse response (the pure sinc function) crosses zero at the edges of the window, which makes the result smoother (derivative is 0) and in turn improves the roll-off. > - would be nice to see some SNR comparisons if you can to better show the > effects of your anti-aliasing effort > Here's an array of measurements I did. https://vm.caoua.org/src/sweeps.html The signal and the aliasing amplitudes are represented for each frequency for various conversion ratios. Anything except the increasing white curve (the signal, 0dB) is aliasing, the color indicates its strength.
smtp(1) add authentication
Playing around with the filter API I want an easier way to send mail
with authentication instead of doing the transaction manually via
openssl or via bloated mailclients. Turns out we already have all the
plumbing in place and just need to hook it up.
OK?
martijn@
Index: smtpc.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
retrieving revision 1.11
diff -u -p -r1.11 smtpc.c
--- smtpc.c 14 Sep 2020 18:32:11 - 1.11
+++ smtpc.c 20 Dec 2020 18:57:13 -
@@ -56,9 +56,8 @@ usage(void)
{
extern char *__progname;
- fprintf(stderr,
- "usage: %s [-Chnv] [-F from] [-H helo] [-s server] [-S name] rcpt
...\n",
- __progname);
+ fprintf(stderr, "usage: %s [-Chnv] [-F from] [-H helo] [-a authfile] "
+ "[-s server] [-S name] rcpt ...\n", __progname);
exit(1);
}
@@ -66,8 +65,12 @@ int
main(int argc, char **argv)
{
char hostname[256];
+ FILE *authfile;
int ch, i;
char *server = "localhost";
+ char *authstr = NULL;
+ size_t alloc = 0;
+ ssize_t len;
struct passwd *pw;
log_init(1, 0);
@@ -91,7 +94,7 @@ main(int argc, char **argv)
memset(, 0, sizeof(mail));
mail.from = pw->pw_name;
- while ((ch = getopt(argc, argv, "CF:H:S:hns:v")) != -1) {
+ while ((ch = getopt(argc, argv, "CF:H:S:a:hns:v")) != -1) {
switch (ch) {
case 'C':
params.tls_verify = 0;
@@ -107,6 +110,23 @@ main(int argc, char **argv)
break;
case 'h':
usage();
+ break;
+ case 'a':
+ if ((authfile = fopen(optarg, "r")) == NULL)
+ fatal("%s: open", optarg);
+ if ((len = getline(, , authfile)) == -1)
+ fatal("%s: Failed to read username", optarg);
+ if (authstr[len - 1] == '\n')
+ authstr[len - 1] = '\0';
+ params.auth_user = authstr;
+ authstr = NULL;
+ len = 0;
+ if ((len = getline(, , authfile)) == -1)
+ fatal("%s: Failed to read password", optarg);
+ if (authstr[len - 1] == '\n')
+ authstr[len - 1] = '\0';
+ params.auth_pass = authstr;
+ fclose(authfile);
break;
case 'n':
noaction = 1;
Index: smtp.1
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v
retrieving revision 1.7
diff -u -p -r1.7 smtp.1
--- smtp.1 4 Jul 2018 08:23:43 - 1.7
+++ smtp.1 20 Dec 2020 18:57:13 -
@@ -25,6 +25,7 @@
.Op Fl Chnv
.Op Fl F Ar from
.Op Fl H Ar helo
+.Op Fl a Ar authfile
.Op Fl s Ar server
.Op Ar recipient ...
.Sh DESCRIPTION
@@ -49,6 +50,13 @@ Set the return-path (MAIL FROM) for the
Default to the current username.
.It Fl H Ar helo
Define the hostname to advertise (HELO) when establishing the SMTP session.
+.It Fl a Ar authfile
+Perform a login before sending the message.
+The username and password are read from
+.Ar authfile
+and need to be on the first and second line respectively.
+This option requires a TLS or STARTTLS
+.Ar server .
.It Fl h
Display version and usage.
.It Fl n
Re: doas sprinkle some more CFLAGS
Ted Unangst wrote: > On 2020-12-18, Martijn van Duren wrote: > > So I ended up in doas again, this time with the CFLAGS I use for most of > > my other projects. This popped up a few new not very exciting warnings. > > Diff below compiles clean with both clang and gcc on amd64. > > > static int > > match(uid_t uid, gid_t *groups, int ngroups, uid_t target, const char *cmd, > > -const char **cmdargs, struct rule *r) > > +const char * const*cmdargs, struct rule *r) > > That looks ugly, and I don't see the point. Const on the char prevents bugs, > but after that we're just piling on crap. I agree. The compiler warnings are beyond the idiom required for safety.
Re: kdump: show scope for v6 addresses if set
Florian Obser wrote:
> On Sun, Dec 20, 2020 at 02:34:09PM +0100, Claudio Jeker wrote:
> > On Sun, Dec 20, 2020 at 01:39:57PM +0100, Otto Moerbeek wrote:
> > > Hi,
> > >
> > > scope is there, just not shown. While there, use proper constants for
> > > two sizes.
> > >
> > > -Otto
> > >
> > >
> > > Index: ktrstruct.c
> > > ===
> > > RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
> > > retrieving revision 1.28
> > > diff -u -p -r1.28 ktrstruct.c
> > > --- ktrstruct.c 17 Nov 2018 20:46:12 - 1.28
> > > +++ ktrstruct.c 20 Dec 2020 12:34:34 -
> > > @@ -90,7 +90,7 @@ ktrsockaddr(struct sockaddr *sa)
> > > switch(sa->sa_family) {
> > > case AF_INET: {
> > > struct sockaddr_in *sa_in;
> > > - char addr[64];
> > > + char addr[INET_ADDRSTRLEN];
> > >
> > > sa_in = (struct sockaddr_in *)sa;
> > > check_sockaddr_len(in);
> > > @@ -100,12 +100,15 @@ ktrsockaddr(struct sockaddr *sa)
> > > }
> > > case AF_INET6: {
> > > struct sockaddr_in6 *sa_in6;
> > > - char addr[64];
> > > + char addr[INET6_ADDRSTRLEN], scope[12] = { 0 };
> > >
> > > sa_in6 = (struct sockaddr_in6 *)sa;
> > > check_sockaddr_len(in6);
> > > inet_ntop(AF_INET6, _in6->sin6_addr, addr, sizeof addr);
> > > - printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
> > > + if (sa_in6->sin6_scope_id)
> > > + snprintf(scope, sizeof(scope), "%%%u",
> > > + sa_in6->sin6_scope_id);
> >
> > Would it make sense to use if_indextoname() here to translate the string
> > into an interface name? The snprintf would still be needed for the case
> > where NULL is returned by if_indextoname().
>
> (I had already OK'ed this off-list.)
>
> I had thought about this as well, at that point we might as well use
> getnameinfo(3).
>
> However, when we run kdump the interface might no longer exist or
> maybe we run kdump on a different host with different interfaces.
If this is going to come up often, the kernel could add an additional
record which describes it.
But don't we wish this scope-id didn't exist?
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
On Sun, Dec 20, 2020 at 09:51:35AM +0100, Gilles CHEHADE wrote: > > > > On 20 Dec 2020, at 07:13, Sebastien Marie wrote: > > > > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: > >> Hello, > >> > >> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is > >> matched, smtpd will > >> attempt to search for a ~/.forward file in the recipient directory and > >> process it. This > >> may be convenient for some setups but it is an implicit behavior that's > >> not overridable > >> and not always wanted. > >> > >> This diff changes this behavior by requiring the admins to explicitly > >> allow the forward > >> files processing in the actions when desired: > >> > >>action "local_users" maildir forward-file > >> > >> > >> With this diff, if forward-file is not specified, code to request parent > >> process for an > >> fd is bypassed and the expansion layer just pretends parent couldn't find > >> one. This let > >> the code fallback in an already existing code path with the proper > >> behavior and is very > >> uninvasive. > >> > > > > if I could understood the direction (which is fine as it makes the > > daemon less behaviour dependant on a user settings), the default seems > > wrong to me (at least for now, and for OpenBSD base specifically). > > > > Currently, root@ mail delivery is based on /root/.forward file: > > install is writing this file to redirect root@ mail to user (if user > > was created at install-time). It is done this way since 2011 (see > > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > > which were done with a user configured, since 2011, could use it. > > Yes, the default would need to be changed as follows: > > mini$ diff -uNp smtpd.conf smtpd.conf.new > > > > --- smtpd.confMon Dec 14 22:13:04 2020 > +++ smtpd.conf.newSun Dec 20 09:43:22 2020 > @@ -11,7 +11,7 @@ listen on socket > # > listen on all hostname debug.poolp.org > > -action "local_mail" maildir alias > +action "local_mail" maildir alias forward-file > action "outbound" relay > My src tree still has mbox as the default. There was talk of changing from mbox to maildir as default. Is this now going forward also? While mbox is simple, once I moved to Dovecot for IMAP, changing mbox to maildir was easy, but needed, amongst some other non-mbox choices. I think new users will be much happier learning maildir and skipping the whole mbox thing. My 2 cents. :^) Chris Bennett
Re: [diff] usr.sbin/smtpd: fix event handling upon exit
Ping ?
> On 14 Dec 2020, at 11:34, Gilles Chehade wrote:
>
> Hello,
>
> Upon termination, the parent process will call parent_shutdown() which will
> in turn call mproc_clear() to properly terminate IPC with child processes.
>
> In mproc_clear(), event_del() is called but a check is lacking to ensure
> event_add() was called prior to this.
>
> On OpenBSD, this doesn’t seem to cause any issue but on other systems with a
> different libevent, calling event_del() without a matching event_add() either
> causes a runtime warning or a crash upon exit.
>
> Gilles
>
>
> diff --git a/usr.sbin/smtpd/mproc.c b/usr.sbin/smtpd/mproc.c
> index bde229e1..dac38af2 100644
> --- a/usr.sbin/smtpd/mproc.c
> +++ b/usr.sbin/smtpd/mproc.c
> @@ -90,7 +90,8 @@ mproc_clear(struct mproc *p)
> {
> log_debug("debug: clearing p=%s, fd=%d, pid=%d", p->name,
> p->imsgbuf.fd, p->pid);
>
> - event_del(>ev);
> + if (p->events)
> + event_del(>ev);
> close(p->imsgbuf.fd);
> imsg_clear(>imsgbuf);
> }
>
Re: sndiod: (mostly) suppress aliasing noise
Hi,
Interesting diff.
I did not have time to look at it thoroughly, but here are a few
observations:
- why do you keep the symmetric filter coefficients? (this could halve
your while-loop computations too, right?)
- the diff's mainlobe look kind of strange at the end, why is that?
What kind of filter did you use? the side-lobes seem also a bit strange
as they do not decrease monotonically nor are the first sidelobes small
and then increasing
- would be nice to see some SNR comparisons if you can to better show
the effects of your anti-aliasing effort
Thanks for this,
Paul
On 20.12.2020 01:52, Alexandre Ratchov wrote:
Hi,
The current sndiod resampling algorithm is very basic mainly to keep
CPU usage very low, which used to make sense for the zaurus. So,
resampling produces aliasing noise, easily audible in 8kHz to 48kHz
conversions but present in other cases.
The diff below reduces the aliasing noise. It's a trade-off between
audio quality, CPU consumption and code simplicity. It adds a better
resampling filter (8-th order FIR low-pass), which mostly suppresses
aliasing noise in the audible part of the spectrum.
Few plots here: https://vm.caoua.org/src
I measured the CPU usage of resampling 16-bit stereo from 44.1kHz to
48kHz.
On a ~7 years old i5-2500K:
- current code: 0.07% CPU
- with diff: 0.45% CPU
On a ~15 years old Thinkpad x40:
- current: 0.25% CPU
- with diff: 1.90% CPU
This is still acceptable even for slow machines, IMHO. Enjoy and let
me know if this introduces any regression.
Index: dsp.c
===
RCS file: /cvs/src/usr.bin/sndiod/dsp.c,v
retrieving revision 1.15
diff -u -p -r1.15 dsp.c
--- dsp.c 10 Dec 2020 17:30:49 - 1.15
+++ dsp.c 19 Dec 2020 23:38:24 -
@@ -38,6 +38,75 @@ int aparams_ctltovol[128] = {
26008, 27029, 28090, 29193, 30339, 31530, 32768
};
+int resamp_filt[RESAMP_LENGTH / RESAMP_STEP + 1] = {
+ 0, 0, 3, 9, 22, 42, 73, 116,
+ 174, 248, 341, 454, 589, 749, 934,1148,
+ 1392,1666,1974,2316,2693,3107,3560,4051,
+ 4582,5154,5766,6420,7116,7853,8632,9451,
+ 10311, 11210, 12148, 13123, 14133, 15178, 16253, 17359,
+ 18491, 19647, 20824, 22018, 23226, 24443, 25665, 26888,
+ 28106, 29315, 30509, 31681, 32826, 33938, 35009, 36033,
+ 37001, 37908, 38744, 39502, 40174, 40750, 41223, 41582,
+ 41819, 41925, 41890, 41704, 41358, 40842, 40147, 39261,
+ 38176, 36881, 35366, 33623, 31641, 29411, 26923, 24169,
+ 21140, 17827, 14222, 10317,6105,1580, -3267, -8440,
+-13944, -19785, -25967, -32492, -39364, -46584, -54153, -62072,
+-70339, -78953, -87911, -97209, -106843, -116806, -127092, -137692,
+ -148596, -159795, -171276, -183025, -195029, -207271, -219735, -232401,
+ -245249, -258259, -271407, -284670, -298021, -311434, -324880, -338329,
+ -351750, -365111, -378378, -391515, -404485, -417252, -429775, -442015,
+ -453930, -465477, -476613, -487294, -497472, -507102, -516137, -524527,
+ -532225, -539181, -545344, -550664, -555090, -558571, -561055, -562490,
+ -562826, -562010, -559990, -556717, -552139, -546205, -538866, -530074,
+ -519779, -507936, -494496, -479416, -462652, -444160, -423901, -401835,
+ -377923, -352132, -324425, -294772, -263143, -229509, -193847, -156134,
+ -116348, -74474, -30494, 15601, 63822, 114174, 11, 221283,
+278037, 336916, 397911, 461009, 526194, 593446, 662741, 734054,
+807354, 882608, 959779, 1038826, 1119706, 1202370, 1286768, 1372846,
+ 1460546, 1549808, 1640566, 1732753, 1826299, 1921130, 2017169, 2114336,
+ 2212550, 2311723, 2411770, 2512598, 2614116, 2716228, 2818836, 2921841,
+ 3025142, 3128636, 3232218, 3335782, 3439219, 3542423, 3645282, 3747687,
+ 3849526, 3950687, 4051059, 4150530, 4248987, 4346320, 4442415, 4537163,
+ 4630453, 4722177, 4812225, 4900493, 4986873, 5071263, 5153561, 5233668,
+ 5311485, 5386917, 5459872, 5530259, 5597992, 5662986, 5725160, 5784436,
+ 5840739, 5893999, 5944148, 5991122, 6034862, 6075313, 6112422, 6146142,
+ 6176430, 6203247, 6226559, 6246335, 6262551, 6275185, 6284220, 6289647,
+ 6291456, 6289647, 6284220, 6275185, 6262551, 6246335, 6226559, 6203247,
+ 6176430, 6146142, 6112422, 6075313, 6034862, 5991122, 5944148, 5893999,
+ 5840739, 5784436, 5725160, 5662986, 5597992, 5530259, 5459872, 5386917,
+ 5311485, 5233668, 5153561, 5071263, 4986873, 4900493, 4812225, 4722177,
+ 4630453, 4537163, 4442415, 4346320, 4248987, 4150530, 4051059, 3950687,
+
Re: kdump: show scope for v6 addresses if set
On Sun, Dec 20, 2020 at 02:34:09PM +0100, Claudio Jeker wrote:
> On Sun, Dec 20, 2020 at 01:39:57PM +0100, Otto Moerbeek wrote:
> > Hi,
> >
> > scope is there, just not shown. While there, use proper constants for
> > two sizes.
> >
> > -Otto
> >
> >
> > Index: ktrstruct.c
> > ===
> > RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
> > retrieving revision 1.28
> > diff -u -p -r1.28 ktrstruct.c
> > --- ktrstruct.c 17 Nov 2018 20:46:12 - 1.28
> > +++ ktrstruct.c 20 Dec 2020 12:34:34 -
> > @@ -90,7 +90,7 @@ ktrsockaddr(struct sockaddr *sa)
> > switch(sa->sa_family) {
> > case AF_INET: {
> > struct sockaddr_in *sa_in;
> > - char addr[64];
> > + char addr[INET_ADDRSTRLEN];
> >
> > sa_in = (struct sockaddr_in *)sa;
> > check_sockaddr_len(in);
> > @@ -100,12 +100,15 @@ ktrsockaddr(struct sockaddr *sa)
> > }
> > case AF_INET6: {
> > struct sockaddr_in6 *sa_in6;
> > - char addr[64];
> > + char addr[INET6_ADDRSTRLEN], scope[12] = { 0 };
> >
> > sa_in6 = (struct sockaddr_in6 *)sa;
> > check_sockaddr_len(in6);
> > inet_ntop(AF_INET6, _in6->sin6_addr, addr, sizeof addr);
> > - printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
> > + if (sa_in6->sin6_scope_id)
> > + snprintf(scope, sizeof(scope), "%%%u",
> > + sa_in6->sin6_scope_id);
>
> Would it make sense to use if_indextoname() here to translate the string
> into an interface name? The snprintf would still be needed for the case
> where NULL is returned by if_indextoname().
(I had already OK'ed this off-list.)
I had thought about this as well, at that point we might as well use
getnameinfo(3).
However, when we run kdump the interface might no longer exist or
maybe we run kdump on a different host with different interfaces.
>
> > + printf("[%s%s]:%u", addr, scope, htons(sa_in6->sin6_port));
> > break;
> > }
> > case AF_UNIX: {
> >
>
> --
> :wq Claudio
>
--
I'm not entirely sure you are real.
Re: kdump: show scope for v6 addresses if set
On Sun, Dec 20, 2020 at 02:34:09PM +0100, Claudio Jeker wrote:
> On Sun, Dec 20, 2020 at 01:39:57PM +0100, Otto Moerbeek wrote:
> > Hi,
> >
> > scope is there, just not shown. While there, use proper constants for
> > two sizes.
> >
> > -Otto
> >
> >
> > Index: ktrstruct.c
> > ===
> > RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
> > retrieving revision 1.28
> > diff -u -p -r1.28 ktrstruct.c
> > --- ktrstruct.c 17 Nov 2018 20:46:12 - 1.28
> > +++ ktrstruct.c 20 Dec 2020 12:34:34 -
> > @@ -90,7 +90,7 @@ ktrsockaddr(struct sockaddr *sa)
> > switch(sa->sa_family) {
> > case AF_INET: {
> > struct sockaddr_in *sa_in;
> > - char addr[64];
> > + char addr[INET_ADDRSTRLEN];
> >
> > sa_in = (struct sockaddr_in *)sa;
> > check_sockaddr_len(in);
> > @@ -100,12 +100,15 @@ ktrsockaddr(struct sockaddr *sa)
> > }
> > case AF_INET6: {
> > struct sockaddr_in6 *sa_in6;
> > - char addr[64];
> > + char addr[INET6_ADDRSTRLEN], scope[12] = { 0 };
> >
> > sa_in6 = (struct sockaddr_in6 *)sa;
> > check_sockaddr_len(in6);
> > inet_ntop(AF_INET6, _in6->sin6_addr, addr, sizeof addr);
> > - printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
> > + if (sa_in6->sin6_scope_id)
> > + snprintf(scope, sizeof(scope), "%%%u",
> > + sa_in6->sin6_scope_id);
>
> Would it make sense to use if_indextoname() here to translate the string
> into an interface name? The snprintf would still be needed for the case
> where NULL is returned by if_indextoname().
that translation is dependent on the machine kdump is run on. So it
will give the wrong interface often. And even on the same machine the
network config might have changed.
-Otto
> > + printf("[%s%s]:%u", addr, scope, htons(sa_in6->sin6_port));
> > break;
> > }
> > case AF_UNIX: {
> >
>
> --
> :wq Claudio
Re: kdump: show scope for v6 addresses if set
On Sun, Dec 20, 2020 at 01:39:57PM +0100, Otto Moerbeek wrote:
> Hi,
>
> scope is there, just not shown. While there, use proper constants for
> two sizes.
>
> -Otto
>
>
> Index: ktrstruct.c
> ===
> RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 ktrstruct.c
> --- ktrstruct.c 17 Nov 2018 20:46:12 - 1.28
> +++ ktrstruct.c 20 Dec 2020 12:34:34 -
> @@ -90,7 +90,7 @@ ktrsockaddr(struct sockaddr *sa)
> switch(sa->sa_family) {
> case AF_INET: {
> struct sockaddr_in *sa_in;
> - char addr[64];
> + char addr[INET_ADDRSTRLEN];
>
> sa_in = (struct sockaddr_in *)sa;
> check_sockaddr_len(in);
> @@ -100,12 +100,15 @@ ktrsockaddr(struct sockaddr *sa)
> }
> case AF_INET6: {
> struct sockaddr_in6 *sa_in6;
> - char addr[64];
> + char addr[INET6_ADDRSTRLEN], scope[12] = { 0 };
>
> sa_in6 = (struct sockaddr_in6 *)sa;
> check_sockaddr_len(in6);
> inet_ntop(AF_INET6, _in6->sin6_addr, addr, sizeof addr);
> - printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
> + if (sa_in6->sin6_scope_id)
> + snprintf(scope, sizeof(scope), "%%%u",
> + sa_in6->sin6_scope_id);
Would it make sense to use if_indextoname() here to translate the string
into an interface name? The snprintf would still be needed for the case
where NULL is returned by if_indextoname().
> + printf("[%s%s]:%u", addr, scope, htons(sa_in6->sin6_port));
> break;
> }
> case AF_UNIX: {
>
--
:wq Claudio
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:14, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote: >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. > > Well, it wouldn't be the first time. But I agree that such changes > should be rare and have really good reason for. > > So yes, even if the option is desirable and being off-by-default would > be a good default, the flag-day way for handling it is complex. I really want to emphasise that I don’t suggest off-by-default on OpenBSD but just making the feature explicit. The default smtpd.conf could still have the option to retain the default behaviour. > Regarding the option itself, if I recall correctly some descriptions > made by Gilles about smtpd, opening ~/.forward is one of the few tasks > done by the priviligied process of smtpd. So it could make sense to > avoid it if not need. > > Gilles, could you confirm that having an option to remove .forward > capability (whatever the default value of the option is) could > effectively help to reduce the attack surface of smtpd ? Yes, this is one of the benefits. Setups that don’t ask for forward-file don’t go through the parent process at every recipient submitted in a session, that’s one imsg less handled by the privileged process. > For example, as immediate consequence, I see no reason for smtpd > priviliegied process to keep a full filesystem view: it might be > possible to restricted it to few directories with unveil(2) (I assume > priviliegied process is still need for bsd_auth, and bsd_auth will > have some requirements). Yes !
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:03, Gilles CHEHADE wrote: > > >> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: >> >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. >> >> Gilles, I think you should be adding an option that blocks it optionally, >> and then some operators can use that. If they wish. I am surprised you >> think this can be a default, when as Sebastien points out the base system >> uses it today... >> > > I know that this isn’t convenient and my first version of the diff was a > “disalllow-forward-file” option but: > > The diff was to discuss what I think is the right way of doing it, not the > one I find the most convenient. > If this is not desired, I can submit a diff for the convenient way but I > would have hated not showing what I think is right first. > > In addition, my diff is a turn on a feature explicitly whereas the > “disallow-forward-file” option is a turn off an implicit behaviour, > and when I see that some people don’t even know that .forward files are a > thing, I feel it’s the wrong way around. People who > want forward files know they exist and can ask for it, whereas people who > don’t know they exist or who don’t request it will > get it behind their backs. > > As I said to semarie@ and millert@, the default configuration could be > adapted to add forward-file to the mbox action, > and this diff could be adapted to not ignore .forward files but warn that > they are used on a rule without the keyword to > give people two releases to adapt since we can’t expect everyone to change > their files but we can expect them to upgrade > at least every two releases. > > Also, what doesn’t show on this diff is that if we rely on the implicit > behaviour and a “disallow-forward-file” it kind of makes > other features backwards too in terms of configuration. > > Assuming disallow-forward-file, then do we add an option to disallow > execution of an mda or do we add an option to allow it ? > Does the default behaviour of forward files is to execute custom commands or > not ? > If not, then how do we express it if there’s no option visible in the conf ? > > It makes the grammar very weird :-/ I’d like to add something I forgot to mention, there are two bonus benefits to do this: 1- the forward file handling requires an indirection through the parent process to obtain an fd to the .forward file, an indirection through a privileged process which would be completely bypassed for all users who do not explicitly require it. 2- if we make the feature explicit, then it becomes easier to add some security safe-guards in the parent process right before execution of an MDA: if it is asked to execute a custom command but the configuration states there Is no .forward file allowed, then we can detect something is fishy and refuse to fork a child process for delivery.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 03:21, Theo de Raadt wrote: > > Todd C. Miller wrote: > >> I like this direction but I worry about breaking existing configs. >> How are we going to alert existing users that they need to update >> their configs if the behavior silently changes? > > I think the configuration is backwards. > > Every endpoint box will need these new stanza. > > Very strange. I’m really unsure about that. It does make your endpoints and mine need the option because we receive mail on machines where users have shells, but from what I observe this is not the common case. From what I observe, most people expose mail through IMAP, the forward files are not accessible to recipients. I could be wrong but I think we are biased on this.
dig(1): nuke isc_sockaddr_fromin{,6}
Rewrite parse_netprefix to no longer use isc_sockaddr_fromin{,6}.
Since this was the last user of those functions we can delete them.
That turd seems to be reasonably shiny with this...
OK?
diff --git dighost.c dighost.c
index 116de3a1c6d..b822a92c756 100644
--- dighost.c
+++ dighost.c
@@ -933,68 +933,34 @@ parse_bits(char *arg, uint32_t max) {
isc_result_t
parse_netprefix(struct sockaddr_storage **sap, int *plen, const char *value) {
struct sockaddr_storage *sa = NULL;
- struct in_addr in4;
- struct in6_addr in6;
- uint32_t prefix_length = 0x;
- char *slash = NULL;
- int parsed = 0;
- int prefix_parsed = 0;
- char buf[sizeof("::::::XXX.XXX.XXX.XXX/128")];
- const char *errstr;
+ struct in_addr *in4;
+ struct in6_addr *in6;
+ int prefix_length;
REQUIRE(sap != NULL && *sap == NULL);
- if (strlcpy(buf, value, sizeof(buf)) >= sizeof(buf))
- fatal("invalid prefix '%s'\n", value);
-
- sa = malloc(sizeof(*sa));
+ sa = calloc(1, sizeof(*sa));
if (sa == NULL)
fatal("out of memory");
- memset(sa, 0, sizeof(*sa));
- if (strcmp(buf, "0") == 0) {
+ in4 = &((struct sockaddr_in *)sa)->sin_addr;
+ in6 = &((struct sockaddr_in6 *)sa)->sin6_addr;
+
+ if (strcmp(value, "0") == 0) {
sa->ss_family = AF_UNSPEC;
prefix_length = 0;
goto done;
}
- slash = strchr(buf, '/');
- if (slash != NULL) {
- *slash = '\0';
- prefix_length = strtonum(slash + 1, 0, 128, );
- if (errstr != NULL) {
- fatal("prefix length is %s: '%s'", errstr, value);
- }
- prefix_parsed = 1;
- }
-
- if (inet_pton(AF_INET6, buf, ) == 1) {
- parsed = 1;
- isc_sockaddr_fromin6(sa, , 0);
- if (prefix_length > 128)
- prefix_length = 128;
- } else if (inet_pton(AF_INET, buf, ) == 1) {
- parsed = 1;
- isc_sockaddr_fromin(sa, , 0);
- if (prefix_length > 32)
- prefix_length = 32;
- } else if (prefix_parsed) {
- int i;
-
- for (i = 0; i < 3 && strlen(buf) < sizeof(buf) - 2; i++) {
- strlcat(buf, ".0", sizeof(buf));
- if (inet_pton(AF_INET, buf, ) == 1) {
- parsed = 1;
- isc_sockaddr_fromin(sa, , 0);
- break;
- }
- }
-
- if (prefix_length > 32)
- prefix_length = 32;
- }
-
- if (!parsed)
+ if ((prefix_length = inet_net_pton(AF_INET6, value, in6, sizeof(*in6)))
+ != -1) {
+ sa->ss_len = sizeof(struct sockaddr_in6);
+ sa->ss_family = AF_INET6;
+ } else if ((prefix_length = inet_net_pton(AF_INET, value, in4,
+ sizeof(*in4))) != -1) {
+ sa->ss_len = sizeof(struct sockaddr_in);
+ sa->ss_family = AF_INET;
+ } else
fatal("invalid address '%s'", value);
done:
diff --git lib/isc/include/isc/sockaddr.h lib/isc/include/isc/sockaddr.h
index c29bbd30760..365e9528e84 100644
--- lib/isc/include/isc/sockaddr.h
+++ lib/isc/include/isc/sockaddr.h
@@ -82,20 +82,6 @@ isc_sockaddr_anyofpf(struct sockaddr_storage *sockaddr, int
family);
* \li 'family' is AF_INET or AF_INET6.
*/
-void
-isc_sockaddr_fromin(struct sockaddr_storage *sockaddr, const struct in_addr
*ina,
- in_port_t port);
-/*%<
- * Construct an struct sockaddr_storage from an IPv4 address and port.
- */
-
-void
-isc_sockaddr_fromin6(struct sockaddr_storage *sockaddr, const struct in6_addr
*ina6,
-in_port_t port);
-/*%<
- * Construct an struct sockaddr_storage from an IPv6 address and port.
- */
-
int
isc_sockaddr_pf(const struct sockaddr_storage *sockaddr);
/*%<
diff --git lib/isc/sockaddr.c lib/isc/sockaddr.c
index 42ee201fb35..56b79d7c881 100644
--- lib/isc/sockaddr.c
+++ lib/isc/sockaddr.c
@@ -195,18 +195,6 @@ isc_sockaddr_any6(struct sockaddr_storage *sockaddr)
sin6->sin6_port = 0;
}
-void
-isc_sockaddr_fromin(struct sockaddr_storage *sockaddr, const struct in_addr
*ina,
- in_port_t port)
-{
- struct sockaddr_in *sin = (struct sockaddr_in *) sockaddr;
- memset(sockaddr, 0, sizeof(*sockaddr));
- sin->sin_family = AF_INET;
- sin->sin_len = sizeof(*sin);
- sin->sin_addr = *ina;
- sin->sin_port = htons(port);
-}
-
void
isc_sockaddr_anyofpf(struct sockaddr_storage *sockaddr, int pf) {
switch (pf) {
@@ -221,18 +209,6 @@ isc_sockaddr_anyofpf(struct sockaddr_storage *sockaddr,
int pf) {
}
}
-void
-isc_sockaddr_fromin6(struct
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: > > There are thousands of people with smtpd configurations, and sysmerge > is not going to handle this. > > We cannot expect them all to change their files. This is madness. > > Gilles, I think you should be adding an option that blocks it optionally, > and then some operators can use that. If they wish. I am surprised you > think this can be a default, when as Sebastien points out the base system > uses it today... > I know that this isn’t convenient and my first version of the diff was a “disalllow-forward-file” option but: The diff was to discuss what I think is the right way of doing it, not the one I find the most convenient. If this is not desired, I can submit a diff for the convenient way but I would have hated not showing what I think is right first. In addition, my diff is a turn on a feature explicitly whereas the “disallow-forward-file” option is a turn off an implicit behaviour, and when I see that some people don’t even know that .forward files are a thing, I feel it’s the wrong way around. People who want forward files know they exist and can ask for it, whereas people who don’t know they exist or who don’t request it will get it behind their backs. As I said to semarie@ and millert@, the default configuration could be adapted to add forward-file to the mbox action, and this diff could be adapted to not ignore .forward files but warn that they are used on a rule without the keyword to give people two releases to adapt since we can’t expect everyone to change their files but we can expect them to upgrade at least every two releases. Also, what doesn’t show on this diff is that if we rely on the implicit behaviour and a “disallow-forward-file” it kind of makes other features backwards too in terms of configuration. Assuming disallow-forward-file, then do we add an option to disallow execution of an mda or do we add an option to allow it ? Does the default behaviour of forward files is to execute custom commands or not ? If not, then how do we express it if there’s no option visible in the conf ? It makes the grammar very weird :-/
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: >> Hello, >> >> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, >> smtpd will >> attempt to search for a ~/.forward file in the recipient directory and >> process it. This >> may be convenient for some setups but it is an implicit behavior that's not >> overridable >> and not always wanted. >> >> This diff changes this behavior by requiring the admins to explicitly allow >> the forward >> files processing in the actions when desired: >> >>action "local_users" maildir forward-file >> >> >> With this diff, if forward-file is not specified, code to request parent >> process for an >> fd is bypassed and the expansion layer just pretends parent couldn't find >> one. This let >> the code fallback in an already existing code path with the proper behavior >> and is very >> uninvasive. >> > > if I could understood the direction (which is fine as it makes the > daemon less behaviour dependant on a user settings), the default seems > wrong to me (at least for now, and for OpenBSD base specifically). > > Currently, root@ mail delivery is based on /root/.forward file: > install is writing this file to redirect root@ mail to user (if user > was created at install-time). It is done this way since 2011 (see > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > which were done with a user configured, since 2011, could use it. Yes, the default would need to be changed as follows: mini$ diff -uNp smtpd.conf smtpd.conf.new --- smtpd.conf Mon Dec 14 22:13:04 2020 +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 @@ -11,7 +11,7 @@ listen on socket # listen on all hostname debug.poolp.org -action "local_mail" maildir alias +action "local_mail" maildir alias forward-file action "outbound" relay # Uncomment the following to accept external mail for domain "example.org" mini$ > At first step, I would keep the default smtpd.conf with "forward-file" > option set. It would make smtpd(1) to default to no "forward-file" if > not set (what your diff do), but set the default to with > "forward-file" for OpenBSD base. > > Admin could remove the option if he/she doesn't use it. Yes, I agree and I was showing the idea more than suggesting a default configuration for OpenBSD base. If the default config had the diff I showed above, which is what you suggest, then there would be no behaviour change on a default install. As for existing setups, as I answered to millert@, there could even be a two release plan so that not having the keyword would not break forward file but just warn the admin in logs that the keyword is now needed whenever a .forward file is used. This would push them into adapting their configuration file before it breaks a year from now. Since there’s currently no way of not having forward files, then this does not break setups as not doing anything keeps the old behaviour + warnings.
kdump: show scope for v6 addresses if set
Hi,
scope is there, just not shown. While there, use proper constants for
two sizes.
-Otto
Index: ktrstruct.c
===
RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
retrieving revision 1.28
diff -u -p -r1.28 ktrstruct.c
--- ktrstruct.c 17 Nov 2018 20:46:12 - 1.28
+++ ktrstruct.c 20 Dec 2020 12:34:34 -
@@ -90,7 +90,7 @@ ktrsockaddr(struct sockaddr *sa)
switch(sa->sa_family) {
case AF_INET: {
struct sockaddr_in *sa_in;
- char addr[64];
+ char addr[INET_ADDRSTRLEN];
sa_in = (struct sockaddr_in *)sa;
check_sockaddr_len(in);
@@ -100,12 +100,15 @@ ktrsockaddr(struct sockaddr *sa)
}
case AF_INET6: {
struct sockaddr_in6 *sa_in6;
- char addr[64];
+ char addr[INET6_ADDRSTRLEN], scope[12] = { 0 };
sa_in6 = (struct sockaddr_in6 *)sa;
check_sockaddr_len(in6);
inet_ntop(AF_INET6, _in6->sin6_addr, addr, sizeof addr);
- printf("[%s]:%u", addr, htons(sa_in6->sin6_port));
+ if (sa_in6->sin6_scope_id)
+ snprintf(scope, sizeof(scope), "%%%u",
+ sa_in6->sin6_scope_id);
+ printf("[%s%s]:%u", addr, scope, htons(sa_in6->sin6_port));
break;
}
case AF_UNIX: {
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 02:09, Todd C. Miller wrote: > > I like this direction but I worry about breaking existing configs. > How are we going to alert existing users that they need to update > their configs if the behavior silently changes? > > - todd I agree and this diff was more to suggest a direction and spark discussion than a request to get this in. Today there’s no way to disable forward files and OpenBSD supports two releases. If we agreed this is the right direction then we could have a two-release plan: 1- introduce the keyword but not require it yet 2- add the keyword to the default configuration file 3- throw in a warning in logs whenever a .forward file is used with an action that doesn’t have the keyword set With this, existing setups would not break but start warning that a configuration file change is required. If the configuration change is made, the warnings stop right away. People get two releases to fix their configuration before the keyword becomes mandatory. I’ll address other concerns raised by semarie@ and deraadt@ as a reply to their mail.
Re: dig vs ipv6 (scoped) addresses
On Sun, Dec 20, 2020 at 10:48:01AM +0100, Florian Obser wrote:
> On Thu, Dec 17, 2020 at 12:15:16PM +0100, Otto Moerbeek wrote:
> > Hi,
>
> >
> > as noted on misc dig does not like to talk to local link addresses.
> > This fixes that case. While investigating I also found another bug:
>
> Thanks for looking into this. Looks like I got distracted while
> ripping out isc_sockaddr and did not fully clean it up. Probably
> because I found another isc_indirection to delete :/
>
> I'd rather like to get rid of isc_sockaddr_fromin* completely, see
> diff at the end.
>
> > selecting v6 or v4 addresses only from resolv.conf via the -4 or -6
> > command line argument does not work as expected.
>
> Nice catch.
>
> >
> > This fixes both. I did not test binding to a src address with this.
> > This is likely as broken as it was before.
>
> My diff fixes that, too.
>
> I still need to keep isc_sockaddr_fromin* because it's used for
> +subnet i.e. ecs. Which is broken, too. I'm having a look now.
>
> >
> > -Otto
> >
>
> > Index: lib/lwres/lwconfig.c
> > ===
> > RCS file: /cvs/src/usr.bin/dig/lib/lwres/lwconfig.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 lwconfig.c
> > --- lib/lwres/lwconfig.c25 Feb 2020 05:00:43 - 1.6
> > +++ lib/lwres/lwconfig.c17 Dec 2020 11:06:49 -
> > @@ -231,7 +231,7 @@ lwres_conf_parsenameserver(lwres_conf_t
> >
> > res = lwres_create_addr(word, , 1);
> > use_ipv4 = confdata->flags & LWRES_USEIPV4;
> > - use_ipv6 = confdata->flags & LWRES_USEIPV4;
> > + use_ipv6 = confdata->flags & LWRES_USEIPV6;
> > if (res == LWRES_R_SUCCESS &&
> > ((address.family == LWRES_ADDRTYPE_V4 && use_ipv4) ||
> > (address.family == LWRES_ADDRTYPE_V6 && use_ipv6))) {
> >
>
> OK florian for this
Committed
>
> OK for this version for the rest?
A few nits inline. With those either addressed or ignored, OK,
-Otto
>
>
> diff --git dig.c dig.c
> index a0988a0567b..6b142a03239 100644
> --- dig.c
> +++ dig.c
> @@ -17,7 +17,10 @@
> /* $Id: dig.c,v 1.18 2020/09/15 11:47:42 florian Exp $ */
>
> /*! \file */
> -#include
> +#include
> +#include
> +
> +#include
>
> #include
> #include
> @@ -1275,10 +1278,7 @@ dash_option(char *option, char *next, dig_lookup_t
> **lookup,
> dns_rdatatype_t rdtype;
> dns_rdataclass_t rdclass;
> char textname[MXNAME];
> - struct in_addr in4;
> - struct in6_addr in6;
> - in_port_t srcport;
> - char *hash, *cmd;
> + char *cmd;
> uint32_t num;
> const char *errstr;
>
> @@ -1346,28 +1346,39 @@ dash_option(char *option, char *next, dig_lookup_t
> **lookup,
> if (value == NULL)
> goto invalid_option;
> switch (opt) {
> - case 'b':
> + case 'b': {
> + struct addrinfo *ai = NULL, hints;
> + int error;
> + char *hash;
> +
> + memset(, 0, sizeof(hints));
> + hints.ai_flags = AI_NUMERICHOST;
> + hints.ai_socktype = SOCK_STREAM;
It does not realy matter for the rsult, but SOCK_DGRAM feels more
natural for DNS.
> +
> hash = strchr(value, '#');
> if (hash != NULL) {
> - num = strtonum(hash + 1, 0, MAXPORT, );
> - if (errstr != NULL)
> - fatal("port number is %s: '%s'", errstr,
> - hash + 1);
> - srcport = num;
> *hash = '\0';
> + error = getaddrinfo(value, hash + 1, , );
> + *hash = '#';
> } else
> - srcport = 0;
> - if (have_ipv6 && inet_pton(AF_INET6, value, ) == 1)
> - isc_sockaddr_fromin6(_address, , srcport);
> - else if (have_ipv4 && inet_pton(AF_INET, value, ) == 1)
> - isc_sockaddr_fromin(_address, , srcport);
> - else
> + error = getaddrinfo(value, NULL, , );
> +
> + if (error)
> + fatal("invalid address %s: %s", value,
> + gai_strerror(error));
> + if (ai == NULL || ai->ai_addrlen > sizeof(bind_address))
> + fatal("invalid address %s", value);
> + if (!have_ipv4 && ai->ai_family == AF_INET)
> + fatal("invalid address %s", value);
Please be more specific, like "%s: wrong address family"
> + if (!have_ipv6 && ai->ai_family == AF_INET6)
> fatal("invalid address %s", value);
Same here
>
> - if (hash != NULL)
> - *hash = '#';
> + memset(_address, 0, sizeof(bind_address));
> + memcpy(_address, ai->ai_addr, ai->ai_addrlen);
> +
> specified_source = 1;
> return (value_from_next);
> + }
> case 'c':
>
Re: dig vs ipv6 (scoped) addresses
On Thu, Dec 17, 2020 at 12:15:16PM +0100, Otto Moerbeek wrote:
> Hi,
>
> as noted on misc dig does not like to talk to local link addresses.
> This fixes that case. While investigating I also found another bug:
Thanks for looking into this. Looks like I got distracted while
ripping out isc_sockaddr and did not fully clean it up. Probably
because I found another isc_indirection to delete :/
I'd rather like to get rid of isc_sockaddr_fromin* completely, see
diff at the end.
> selecting v6 or v4 addresses only from resolv.conf via the -4 or -6
> command line argument does not work as expected.
Nice catch.
>
> This fixes both. I did not test binding to a src address with this.
> This is likely as broken as it was before.
My diff fixes that, too.
I still need to keep isc_sockaddr_fromin* because it's used for
+subnet i.e. ecs. Which is broken, too. I'm having a look now.
>
> -Otto
>
> Index: lib/lwres/lwconfig.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/lwres/lwconfig.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 lwconfig.c
> --- lib/lwres/lwconfig.c 25 Feb 2020 05:00:43 - 1.6
> +++ lib/lwres/lwconfig.c 17 Dec 2020 11:06:49 -
> @@ -231,7 +231,7 @@ lwres_conf_parsenameserver(lwres_conf_t
>
> res = lwres_create_addr(word, , 1);
> use_ipv4 = confdata->flags & LWRES_USEIPV4;
> - use_ipv6 = confdata->flags & LWRES_USEIPV4;
> + use_ipv6 = confdata->flags & LWRES_USEIPV6;
> if (res == LWRES_R_SUCCESS &&
> ((address.family == LWRES_ADDRTYPE_V4 && use_ipv4) ||
> (address.family == LWRES_ADDRTYPE_V6 && use_ipv6))) {
>
OK florian for this
OK for this version for the rest?
diff --git dig.c dig.c
index a0988a0567b..6b142a03239 100644
--- dig.c
+++ dig.c
@@ -17,7 +17,10 @@
/* $Id: dig.c,v 1.18 2020/09/15 11:47:42 florian Exp $ */
/*! \file */
-#include
+#include
+#include
+
+#include
#include
#include
@@ -1275,10 +1278,7 @@ dash_option(char *option, char *next, dig_lookup_t
**lookup,
dns_rdatatype_t rdtype;
dns_rdataclass_t rdclass;
char textname[MXNAME];
- struct in_addr in4;
- struct in6_addr in6;
- in_port_t srcport;
- char *hash, *cmd;
+ char *cmd;
uint32_t num;
const char *errstr;
@@ -1346,28 +1346,39 @@ dash_option(char *option, char *next, dig_lookup_t
**lookup,
if (value == NULL)
goto invalid_option;
switch (opt) {
- case 'b':
+ case 'b': {
+ struct addrinfo *ai = NULL, hints;
+ int error;
+ char *hash;
+
+ memset(, 0, sizeof(hints));
+ hints.ai_flags = AI_NUMERICHOST;
+ hints.ai_socktype = SOCK_STREAM;
+
hash = strchr(value, '#');
if (hash != NULL) {
- num = strtonum(hash + 1, 0, MAXPORT, );
- if (errstr != NULL)
- fatal("port number is %s: '%s'", errstr,
- hash + 1);
- srcport = num;
*hash = '\0';
+ error = getaddrinfo(value, hash + 1, , );
+ *hash = '#';
} else
- srcport = 0;
- if (have_ipv6 && inet_pton(AF_INET6, value, ) == 1)
- isc_sockaddr_fromin6(_address, , srcport);
- else if (have_ipv4 && inet_pton(AF_INET, value, ) == 1)
- isc_sockaddr_fromin(_address, , srcport);
- else
+ error = getaddrinfo(value, NULL, , );
+
+ if (error)
+ fatal("invalid address %s: %s", value,
+ gai_strerror(error));
+ if (ai == NULL || ai->ai_addrlen > sizeof(bind_address))
+ fatal("invalid address %s", value);
+ if (!have_ipv4 && ai->ai_family == AF_INET)
+ fatal("invalid address %s", value);
+ if (!have_ipv6 && ai->ai_family == AF_INET6)
fatal("invalid address %s", value);
- if (hash != NULL)
- *hash = '#';
+ memset(_address, 0, sizeof(bind_address));
+ memcpy(_address, ai->ai_addr, ai->ai_addrlen);
+
specified_source = 1;
return (value_from_next);
+ }
case 'c':
if ((*lookup)->rdclassset) {
fprintf(stderr, ";; Warning, extra class option\n");
diff --git dighost.c dighost.c
index 52afde3d7d3..3974843600d 100644
--- dighost.c
+++ dighost.c
@@ -498,6 +498,7 @@ get_addresses(const char *hostname, in_port_t dstport,
{
struct addrinfo *ai = NULL, *tmpai, hints;
int result, i;
+ char dport[sizeof("65535")];
REQUIRE(hostname != NULL);
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote: > There are thousands of people with smtpd configurations, and sysmerge > is not going to handle this. > > We cannot expect them all to change their files. This is madness. Well, it wouldn't be the first time. But I agree that such changes should be rare and have really good reason for. So yes, even if the option is desirable and being off-by-default would be a good default, the flag-day way for handling it is complex. Regarding the option itself, if I recall correctly some descriptions made by Gilles about smtpd, opening ~/.forward is one of the few tasks done by the priviligied process of smtpd. So it could make sense to avoid it if not need. Gilles, could you confirm that having an option to remove .forward capability (whatever the default value of the option is) could effectively help to reduce the attack surface of smtpd ? For example, as immediate consequence, I see no reason for smtpd priviliegied process to keep a full filesystem view: it might be possible to restricted it to few directories with unveil(2) (I assume priviliegied process is still need for bsd_auth, and bsd_auth will have some requirements). Thanks. -- Sebastien Marie
Re: IPsec PMTU and reject route
On Sun, Dec 20, 2020 at 01:01:58AM +0100, Alexander Bluhm wrote:
> Hi,
>
> In revision 1.87 of ip_icmp.c claudio@ added ignoring reject routes
> to icmp_mtudisc_clone(). Otherwise TCP would clone these routes
> for PMTU discovery. They will not work, even after dynamic routing
> has found a better route than the reject route.
>
> With IPsec the use case is different. First you need a route, but
> then the flow handles the packet without routing. Usually this
> route should be a reject route to avoid sending unencrypted traffic
> if the flow is missing. But IPsec needs this route for PMTU
> discovery, which currently does not work.
>
> So accept reject and blackhole routes for IPsec PMTU discovery.
>
> ok?
This makes sense and is fine. OK claudio@
> bluhm
>
> Index: netinet/ip_icmp.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 ip_icmp.c
> --- netinet/ip_icmp.c 22 Aug 2020 17:55:54 - 1.183
> +++ netinet/ip_icmp.c 18 Dec 2020 16:59:25 -
> @@ -928,7 +928,7 @@ icmp_sysctl_icmpstat(void *oldp, size_t
> }
>
> struct rtentry *
> -icmp_mtudisc_clone(struct in_addr dst, u_int rtableid)
> +icmp_mtudisc_clone(struct in_addr dst, u_int rtableid, int ipsec)
> {
> struct sockaddr_in sin;
> struct rtentry *rt;
> @@ -942,7 +942,10 @@ icmp_mtudisc_clone(struct in_addr dst, u
> rt = rtalloc(sintosa(), RT_RESOLVE, rtableid);
>
> /* Check if the route is actually usable */
> - if (!rtisvalid(rt) || (rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE)))
> + if (!rtisvalid(rt))
> + goto bad;
> + /* IPsec needs the route only for PMTU, it can use reject for that */
> + if (!ipsec && (rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE)))
> goto bad;
>
> /*
> @@ -1000,7 +1003,7 @@ icmp_mtudisc(struct icmp *icp, u_int rta
> struct ifnet *ifp;
> u_long mtu = ntohs(icp->icmp_nextmtu); /* Why a long? IPv6 */
>
> - rt = icmp_mtudisc_clone(icp->icmp_ip.ip_dst, rtableid);
> + rt = icmp_mtudisc_clone(icp->icmp_ip.ip_dst, rtableid, 0);
> if (rt == NULL)
> return;
>
> Index: netinet/ip_icmp.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.h,v
> retrieving revision 1.31
> diff -u -p -r1.31 ip_icmp.h
> --- netinet/ip_icmp.h 5 Nov 2018 21:50:39 - 1.31
> +++ netinet/ip_icmp.h 18 Dec 2020 16:59:25 -
> @@ -239,7 +239,7 @@ int icmp_reflect(struct mbuf *, struct m
> void icmp_send(struct mbuf *, struct mbuf *);
> int icmp_sysctl(int *, u_int, void *, size_t *, void *, size_t);
> struct rtentry *
> - icmp_mtudisc_clone(struct in_addr, u_int);
> + icmp_mtudisc_clone(struct in_addr, u_int, int);
> void icmp_mtudisc(struct icmp *, u_int);
> int icmp_do_exthdr(struct mbuf *, u_int16_t, u_int8_t, void *, size_t);
> #endif /* _KERNEL */
> Index: netinet/ip_output.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.357
> diff -u -p -r1.357 ip_output.c
> --- netinet/ip_output.c 24 Jun 2020 22:03:43 - 1.357
> +++ netinet/ip_output.c 18 Dec 2020 16:59:25 -
> @@ -605,7 +605,7 @@ ip_output_ipsec_send(struct tdb *tdb, st
> rt = NULL;
> else if (rt == NULL || (rt->rt_flags & RTF_HOST) == 0) {
> rt = icmp_mtudisc_clone(ip->ip_dst,
> - m->m_pkthdr.ph_rtableid);
> + m->m_pkthdr.ph_rtableid, 1);
> rt_mtucloned = 1;
> }
> DPRINTF(("%s: spi %08x mtu %d rt %p cloned %d\n", __func__,
> Index: netinet/tcp_timer.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_timer.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 tcp_timer.c
> --- netinet/tcp_timer.c 11 Jun 2018 07:40:26 - 1.67
> +++ netinet/tcp_timer.c 18 Dec 2020 16:59:25 -
> @@ -292,7 +292,7 @@ tcp_timer_rexmt(void *arg)
> #endif
> case PF_INET:
> rt = icmp_mtudisc_clone(inp->inp_faddr,
> - inp->inp_rtableid);
> + inp->inp_rtableid, 0);
> break;
> }
> if (rt != NULL) {
>
--
:wq Claudio
