Re: smtpd: Allow labels containing "@"
On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> My mail is klem...@posteo.de and the provider expects this full address
> as username, so that makes for the following perfectly
> valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
>
> I've been doing that with mutt(1) for ages.
>
> smtpd.conf(5) has the following syntax:
>
> host relay-url
> Do not perform MX lookups but relay messages to the relay
> host described by relay-url. The format for relay-url is
> [proto://[label@]]host[:port]. The following protocols
> are available:
>
> yielding the following config in my case:
>
> action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
>
>
> However, when parsing the label in the `relay-url', smtpd(8) stops at
> the first "@" sign, not expecting labels to contain it. The following
> diff fixes this spanning the label to the last occurence of "@" as is
> already done in other code places.
>
> Feedback? Objections?
> OK?
>
no objection, ok
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c 30 Dec 2018 23:09:58 - 1.35
> +++ to.c 22 Jul 2019 21:02:48 -
> @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
> relay->port = 0;
>
> /* first, we extract the label if any */
> - if ((q = strchr(p, '@')) != NULL) {
> + if ((q = strrchr(p, '@')) != NULL) {
> *q = 0;
> if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
> >= sizeof (relay->authlabel))
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: [PATCH] Provide static_ASN1_*(). From OpenSSL 1.1.0 API.
Hi Theo, On 24/05/2019 16:27, Theo Buehler wrote: > On Fri, May 24, 2019 at 04:22:06PM +0300, Stefan Strogin wrote: >> Could somebody review the patch please? > > We saw the pull request and will look into it soon. Apologies, we are > all currently very busy. How do you do? Do you expect some spare time for a review in the near future?
Re: ping(8): better "-i wait" edge case handling
> On Jul 22, 2019, at 01:49, Claudio Jeker wrote: > >> On Sun, Jul 21, 2019 at 08:30:23AM -0500, Scott Cheloha wrote: >> On Sun, Jul 21, 2019 at 11:49:10AM +0200, Mark Kettenis wrote: Date: Sun, 21 Jul 2019 10:50:27 +0200 From: Claudio Jeker > On Sat, Jul 20, 2019 at 08:12:40AM -0500, Scott Cheloha wrote: > There are cases for ping(8)'s "-i wait" option that we don't handle > correctly. > > Negative values smaller than -1: > > $ doas ping -i -0.1 localhost > [no error] > > Positive values less than one microsecond: > > $ doas ping -i 0.001 localhost > [no error] > > Large positive values that actually fit into a timeval: > > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost > ping: illegal timing interval 9223372036854775807 > > The first two cases are bugs in the input checking. The latter case > is a limitation of IEEE doubles: with only 52 bits of significand you > can't represent the full range of a timeval on a platform with 64-bit > time_t. > > This patch addresses the first two cases with better error checking. > It also tries to handle the latter case a bit better by using IEEE > quads, i.e. long doubles. With 64 bits of significand you can cover > our time_t and the above case works. It isn't ~perfect~, but it's as > close as we can get to perfect without parsing the number by hand as > we do in sleep(1) (cf. bin/sleep/sleep.c). > > With this patch those cases all work as expected: > > $ doas ping -i -0.1 localhost > ping: interval is invalid: -0.1 > $ doas ping -i 0.001 localhost > ping: interval is too small: 0.001 > $ ping -i $(bc -e '2^63' -e quit) localhost > ping: interval is too large: 9223372036854775808 > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost > PING localhost.local (23.195.69.106): 56 data bytes > 64 bytes from 23.195.69.106: icmp_seq=0 ttl=54 time=18.001 ms > [stalls forever] > > -- > > While we're modifying this code, I think "-i interval" is better than > "-i wait". The "i for interval" mnemonic is particularly nice. The > current "wait" argument name sort-of implies a relationship with the > "-w maxwait" option, which is not the case. We're also already using > "timing interval" in these error messages here. I've tweaked the > error messages to look more like the usual strtonum(3) error messages. > > ok? Can't we limit the -i maximum value to something that fits into the double instead of using long double in ping. Nobody will ever try to using ping with a timeout that is longer than the operators expected life time. >> >> Sure. Here's a diff that uses UINT_MAX. The oldest known person >> lived to 123. French, if I recall correctly. UINT_MAX gives us 136 >> years, so we have a bit of room to grow just in case someone beats the >> record. And 52 bits of significand is plenty for 32 bits of integral >> plus a fractional portion. >> >>> Using a long double isn't a solution anyway, since we have quite a few >>> architectures where long double is the same as double. >> >> Oh, hmmm, didn't know that. >> >> -- >> >> this diff better? > > Can't this just use the overflow detection of strtod()? You mean the ERANGE thing? That isn't useful here. The normal range for a double is far more vast than what a timeval can represent. We have to check by hand.
smtpd: default to standard ports in relay-host
Using the following configuration, I wrongly assumed smtpd(8) to pick
the appropiate ports by default depending on the protocol as it already
does for `listen' statements:
action "relay" relay host smtps://la...@example.com auth
but no matter the protocol ("smtps" here), port 25 is being used. Debug
output from `smtpd -dv' does not print the port being used so I found
out via tcpdump'ing the traffic.
Given that smtpd.conf(5) already mentions all of the relevant ports
somewhere else, it seems only sensible, if not even technically correct,
to do so; here the respective `listen' options for ease of review:
port [port]
Listen on the given port instead of the default port 25.
smtps Support SMTPS, by default on port 465. Mutually
exclusive with tls.
tls Support STARTTLS, by default on port 25. Mutually
exclusive with smtps.
LMTP is special is it requires an explicitly configured port.
Diff below therefore makes the following `relay-host' strings equivalent:
smtp://la...@example.com:25
smtp://la...@example.com
smtp+tls://la...@example.com:587
smtp+tls://la...@example.com
lmtp://la...@example.com:1234
lmtp://la...@example.com# invalid
smtps://la...@example.com:465
smtps://la...@example.com
Should "smtp" default to 25 or 587? smtpd(8) defaults to 25 itself as
a server and is the current default so I'll leave that, but 587 seems
more common practise to me.
I'm not really happy with the manual addition, yet; is that too explicit
in that readers might think `[:port]' is only valid for "lmtp"?
Adding "by default" to every protocol seems too much, however. Maybe
adding "Requires port." to the LMTP line?
Suggestions welcome, here's the excerpt with my diff applied:
host relay-url
Do not perform MX lookups but relay messages to the relay
host described by relay-url. The format for relay-url is
[proto://[label@]]host[:port]. The following protocols
are available:
smtpNormal SMTP session with opportunistic
STARTTLS on port 25 (the default).
smtp+tlsNormal SMTP session with mandatory STARTTLS
on port 587.
smtp+notls Plain text SMTP session without TLS on port
25.
lmtpLMTP session.
smtps SMTP session with forced TLS on connection on
port 465.
The label corresponds to an entry in a credentials table,
as documented in table(5). It is used with the
“smtp+tls” and “smtps” protocols for authentication.
Server certificates for those protocols are verified by
default.
Feedback? Objections?
I've sent all of my today's smtpd diffs through a local instance running
with all diffs combined withotu problems.
Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.210
diff -u -p -r1.210 smtpd.conf.5
--- smtpd.conf.522 Dec 2018 08:54:02 - 1.210
+++ smtpd.conf.522 Jul 2019 23:29:16 -
@@ -242,16 +242,15 @@ The following protocols are available:
.Pp
.Bl -tag -width "smtp+notls" -compact
.It smtp
-Normal SMTP session with opportunistic STARTTLS
-(the default).
+Normal SMTP session with opportunistic STARTTLS on port 25 (the default).
.It smtp+tls
-Normal SMTP session with mandatory STARTTLS.
+Normal SMTP session with mandatory STARTTLS on port 587.
.It smtp+notls
-Plain text SMTP session without TLS.
+Plain text SMTP session without TLS on port 25.
.It lmtp
LMTP session.
.It smtps
-SMTP session with forced TLS on connection.
+SMTP session with forced TLS on connection on port 465.
.El
.Pp
The
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.36
diff -u -p -r1.36 to.c
--- to.c22 Jul 2019 23:01:48 - 1.36
+++ to.c22 Jul 2019 23:29:16 -
@@ -305,16 +305,17 @@ text_to_relayhost(struct relayhost *rela
const char *name;
int tls;
uint16_t flags;
+ uint16_t port;
} schemas [] = {
/*
* new schemas should be *appended* otherwise the default
* schema index needs to be updated later in this function.
*/
- { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0
},
- { "smtp+tls://",RELAY_TLS_STARTTLS, 0
},
- { "smtp+notls://", RELAY_TLS_NO,0
smtpd: Use IPPORT_HILASTAUTO not 0xffff
More mnemonic and readable.
OK?
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.35
diff -u -p -r1.35 to.c
--- to.c30 Dec 2018 23:09:58 - 1.35
+++ to.c22 Jul 2019 22:16:29 -
@@ -385,7 +385,7 @@ text_to_relayhost(struct relayhost *rela
/* finally, we extract the port */
p = beg + len;
if (*p == ':') {
- relay->port = strtonum(p+1, 1, 0x, );
+ relay->port = strtonum(p+1, 1, IPPORT_HILASTAUTO, );
if (errstr)
return 0;
}
Re: Diff to stop using reserved words for smtpd.conf(5) examples
On Mon, Jul 22, 2019 at 05:25:28PM -0400, Kurt Mosiejczuk wrote: > If you look at the existing man page, it is left off on the examples > where they aren't using reserved words. For a few seconds I thought it > was a sloppy examples, but then I realized the only reason the quotes > are there are because of the use of the reserved words. smtpd won't > accept those as labels without the quotes. It will flag it as a syntax > error. If you don't use configuration syntax for a label, you don't need > the quotes there. Yes, I get that smtpd won't accept such labels without quotes because they resemble reserved words. That is my point entirely: provide quotes where sensible to make it absoloutely obvious that the quoted text is not a grammatical keyword (but may very well be as long as it is quoted). I'll take the first example: table aliases file:/etc/mail/aliases table secrets file:/etc/mail/secrets listen on lo0 action "local" mbox alias action "relay" relay host smtp+tls://b...@smtp.example.com \ auth match for local action "local" match for any action "relay" Removing quotes will result in syntax errors as you noted. So I suggest leaving those quotes where they are because users should be able to copy these examples, substitute "local" with "local-mail" and be done with it. This example does not quote file paths but the next one does; this is inconsistent but I think of slightly less importance because it is even more obvious that "file:/something" is not a single reserved keyword. So I'm with you on providing sane examples, but removing quotes where not necessary is working against that goal - it simply does not buy you anything but potentially more trouble than current examples do.
smtpd: Allow labels containing "@"
My mail is klem...@posteo.de and the provider expects this full address
as username, so that makes for the following perfectly
valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
I've been doing that with mutt(1) for ages.
smtpd.conf(5) has the following syntax:
host relay-url
Do not perform MX lookups but relay messages to the relay
host described by relay-url. The format for relay-url is
[proto://[label@]]host[:port]. The following protocols
are available:
yielding the following config in my case:
action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
However, when parsing the label in the `relay-url', smtpd(8) stops at
the first "@" sign, not expecting labels to contain it. The following
diff fixes this spanning the label to the last occurence of "@" as is
already done in other code places.
Feedback? Objections?
OK?
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.35
diff -u -p -r1.35 to.c
--- to.c30 Dec 2018 23:09:58 - 1.35
+++ to.c22 Jul 2019 21:02:48 -
@@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
relay->port = 0;
/* first, we extract the label if any */
- if ((q = strchr(p, '@')) != NULL) {
+ if ((q = strrchr(p, '@')) != NULL) {
*q = 0;
if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
>= sizeof (relay->authlabel))
Re: Diff to stop using reserved words for smtpd.conf(5) examples
On Mon, Jul 22, 2019 at 05:05:01PM -0400, Kurt Mosiejczuk wrote: > This is a diff for that changes the example smtpd.conf and smtpd.conf.5 > so that it no longer uses words that are parts of the configuration > syntax as labels for actions. A large chunk of my delay to a release > with the new smtpd.conf configuration syntax was my confusion with the > examples. Even writing this diff I realized that the quotes were only > necessary in the examples because configuration grammar was being > reused as labels. I get the point of not reusing reserved words but I am not convinced of "relay" -> "inet-mail" and the like, yet. Either ways, you should not omit quotes. They are not optional in every case; please provide properly quoted examples so people can safely substitute the text and easingly distinguish reserved words from arbitrary text.
Re: extend 802.11 Rx aggregation "gap timeout"
On Mon, Jul 22, 2019 at 07:59:40PM +0200, Mark Kettenis wrote: > Given your description above combined with the 100 ms value that Linux > uses, this sounds like a bad idea to me. OK, fine. We can leave it as it is. If I'm not mistaken, an upper bound of "medium busy" time for a single aggregate frame is about 180ms (a 64 subframe A-MPDU with 1500 bytes per subframe sent at MCS 0; ignoring IEEE80211 headers in subframes). In practice, aggregates we send should be much smaller than 64 subframes. I've seen iwn(4) send aggregates with about 4, 8, 16 subframes. So even at 300 ms there should be several Tx opportunities per subframe, in the worst case, unless the medium is very badly contended. Also, I just realized I can no longer trigger the gap timeout problem I observed without my patch. The timeout might have triggered due to some underlying problem where frames were not received correctly by athn, either because of bugs in the driver or interference on the channel...
vfs posix compatibility change
Hi,
here is another POSIX compatibility change I found while running
NetBSD system call tests:
The incompatibilities can be reproduced with the followding comands:
`mv / foo`
mv: rename / to foo: Is a directory
`man 2 rename` mentions EISDIR:
[EISDIR] to is a directory, but from is not a directory.
Which is mentioned similarly in POSIX. But it doesn't fit the current case.
`mkdir /`
mkdir: /: Is a directory
EISDIR is not mentioned in `man 2 mkdir`. Also not mentioned in POSIX.
I believe EBUSY would be good here.
`rmdir /`
rmdir: /: Is a directory
EISDIR is not mentioned in `man 2 rmdir`. Also not mentioned in POSIX.
unlink("/");
printf("%d\n", errno);
21 // EISDIR
Also neither mentioned in man nor POSIX.
Here is some POSIX references:
https://pubs.opengroup.org/onlinepubs/9699919799/
unlink (2):
[EBUSY]
The file named by the path argument cannot be unlinked because it is
being used by the system or another process and the implementation
considers this an error.
rename (2):
[EBUSY]
The directory named by old or new is currently in use by the system or
another process, and the implementation considers this an error.
rmdir (2):
[EBUSY]
The directory to be removed is currently in use by the system or
some process and the implementation considers this to be an error.
mkdir (2):
[EEXIST]
The named file exists.
Attached is a patch that will make mkdir return EEXIST, EBUSY for others.
I believe this corer case is quite essential as it is in namei because of
this (and because NetBSD and FreeBSD differ a lot in the vfs area) I hope
for some comments on this.
The patched code hasn't changed a lot since 44BSD. duh POSIX.
There already were discussions on EISDIR in the past: (from unlink POSIX)
> The standard developers reviewed TR 24715-2006 and noted that LSB-conforming
> implementations may return [EISDIR] instead of [EPERM] when unlinking a
> directory. A change to permit this behavior by changing the requirement for
> [EPERM] to [EPERM] or [EISDIR] was considered, but decided against since it
> would break existing strictly conforming and conforming applications.
> Applications written for portability to both POSIX.1-2017 and the LSB should
> be prepared to handle either error code.
duh.
thanks,
mbuhl
Index: sys/kern/vfs_lookup.c
===
RCS file: /cvs/src/sys/kern/vfs_lookup.c,v
retrieving revision 1.80
diff -u -p -r1.80 vfs_lookup.c
--- sys/kern/vfs_lookup.c 18 Jul 2019 18:06:17 - 1.80
+++ sys/kern/vfs_lookup.c 22 Jul 2019 16:10:14 -
@@ -440,7 +440,10 @@ vfs_lookup(struct nameidata *ndp)
*/
if (cnp->cn_nameptr[0] == '\0') {
if (ndp->ni_dvp == NULL && wantparent) {
- error = EISDIR;
+ if (cnp->cn_nameiop == CREATE)
+ error = EEXIST;
+ else
+ error = EBUSY;
goto bad;
}
ndp->ni_vp = dp;
Index: lib/libc/sys/rename.2
===
RCS file: /cvs/src/lib/libc/sys/rename.2,v
retrieving revision 1.22
diff -u -p -r1.22 rename.2
--- lib/libc/sys/rename.2 10 Sep 2015 17:55:21 - 1.22
+++ lib/libc/sys/rename.2 22 Jul 2019 16:45:16 -
@@ -123,6 +123,12 @@ characters, or an entire pathname (inclu
exceeded
.Dv PATH_MAX
bytes.
+.It Bq Er EBUSY
+The directory containing
+.Fa from
+or
+.Fa to
+is currently in use by the system.
.It Bq Er ENOENT
A component of the
.Fa from
Re: extend 802.11 Rx aggregation "gap timeout"
> Date: Mon, 22 Jul 2019 19:38:59 +0200
> From: Stefan Sperling
> Content-Type: text/plain; charset="utf-8"
> Content-Disposition: inline
>
> As noted in the description of my Tx agg patch earlier today,
> there seems to be a small problem in our Rx aggregation code.
>
> With 802.11n Tx/Rx aggregation, frames may arrive out of order.
> But the wireless layer must deliver incoming frames to upper layers
> in order. Frames received from the driver are buffered in a fixed-sized
> queue, with queue slots based on frame sequence numbers and queue
> length based on the size of the 'block ack' window.
>
> If a frame is lost, we can end up in a situation where the head of
> this queue contains no frame. But later frames may have been received
> and are now stuck in the queue until the missing frame arrives.
>
> The catch is of course that the missing frame may never arrive,
> be it due to a bug in the sender or some other problem. This is
> the case where we seem to have a problem.
>
> To avoid frames getting stuck forever we must invent a heuristic which
> eventually allows stuck frames to progress. An easy solution, implemented
> by OpenBSD, FreeBSD, and Linux, is to use a timer which begins ticking
> when the queue head is found to be empty after inserting a frame into
> the queue. Once this timer expires, we assume the gap in the queue will
> never be filled, skip the missing frame, and pass frames stuck in the
> queue to upper layers, in order. This event is visible in 'netstat -W':
>
> '13 input block ack window gaps timed out'
>
> When testing iwn(4) Tx aggregation against athn(4) I observed that this
> timout triggered quite regularly, causing athn(4) to discard frames which
> iwn firmware hadn't yet managed to (re-)send. So it looks like I didn't
> pick a very good value when implementing Rx aggregation years ago.
>
> I've looked at this again and checked how other systems handle this:
>
> OpenBSD uses 300 ms.
>
> FreeBSD uses 500 ms.
>
> Linux uses HZ / 10 "jiffies" (whatever that means; jiffies is not a
> fixed unit and HZ can be overriden in the kernel config)
As far as I know a Linux "jiffie" is pretty much the same as a BSD
"tick" and HZ / 10 is equivalent to 100ms.
> I decided to try a value slightly higher than FreeBSD's. It seems to
> work well for iwn(4) and results in gap timeouts being counted less
> frequently.
>
> Of course, the higher this value, the higher the worst-case frame latency
> becomes. Which can hurt e.g. UDP data streams. But on the other hand,
> losing too many frames hurts TCP interactivity (SSH) and TCP throughput,
> so we need to find some good middle ground.
>
> OK to make everyone try 800 ms?
Given your description above combined with the 100 ms value that Linux
uses, this sounds like a bad idea to me.
> diff 1e8763280b150de91c515b29633b68a29acfe36f /usr/src
> blob - 0b891938db0ea1348914da513f667833ef7792d1
> file + sys/net80211/ieee80211_node.h
> --- sys/net80211/ieee80211_node.h
> +++ sys/net80211/ieee80211_node.h
> @@ -221,7 +221,7 @@ struct ieee80211_rx_ba {
> u_int16_t ba_winsize;
> u_int16_t ba_head;
> struct timeout ba_gap_to;
> -#define IEEE80211_BA_GAP_TIMEOUT 300 /* msec */
> +#define IEEE80211_BA_GAP_TIMEOUT 800 /* msec */
> /* Counter for consecutive frames which missed the BA window. */
> int ba_winmiss;
> /* Sequence number of previous frame which missed the BA window. */
>
>
extend 802.11 Rx aggregation "gap timeout"
As noted in the description of my Tx agg patch earlier today,
there seems to be a small problem in our Rx aggregation code.
With 802.11n Tx/Rx aggregation, frames may arrive out of order.
But the wireless layer must deliver incoming frames to upper layers
in order. Frames received from the driver are buffered in a fixed-sized
queue, with queue slots based on frame sequence numbers and queue
length based on the size of the 'block ack' window.
If a frame is lost, we can end up in a situation where the head of
this queue contains no frame. But later frames may have been received
and are now stuck in the queue until the missing frame arrives.
The catch is of course that the missing frame may never arrive,
be it due to a bug in the sender or some other problem. This is
the case where we seem to have a problem.
To avoid frames getting stuck forever we must invent a heuristic which
eventually allows stuck frames to progress. An easy solution, implemented
by OpenBSD, FreeBSD, and Linux, is to use a timer which begins ticking
when the queue head is found to be empty after inserting a frame into
the queue. Once this timer expires, we assume the gap in the queue will
never be filled, skip the missing frame, and pass frames stuck in the
queue to upper layers, in order. This event is visible in 'netstat -W':
'13 input block ack window gaps timed out'
When testing iwn(4) Tx aggregation against athn(4) I observed that this
timout triggered quite regularly, causing athn(4) to discard frames which
iwn firmware hadn't yet managed to (re-)send. So it looks like I didn't
pick a very good value when implementing Rx aggregation years ago.
I've looked at this again and checked how other systems handle this:
OpenBSD uses 300 ms.
FreeBSD uses 500 ms.
Linux uses HZ / 10 "jiffies" (whatever that means; jiffies is not a
fixed unit and HZ can be overriden in the kernel config)
I decided to try a value slightly higher than FreeBSD's. It seems to
work well for iwn(4) and results in gap timeouts being counted less
frequently.
Of course, the higher this value, the higher the worst-case frame latency
becomes. Which can hurt e.g. UDP data streams. But on the other hand,
losing too many frames hurts TCP interactivity (SSH) and TCP throughput,
so we need to find some good middle ground.
OK to make everyone try 800 ms?
diff 1e8763280b150de91c515b29633b68a29acfe36f /usr/src
blob - 0b891938db0ea1348914da513f667833ef7792d1
file + sys/net80211/ieee80211_node.h
--- sys/net80211/ieee80211_node.h
+++ sys/net80211/ieee80211_node.h
@@ -221,7 +221,7 @@ struct ieee80211_rx_ba {
u_int16_t ba_winsize;
u_int16_t ba_head;
struct timeout ba_gap_to;
-#define IEEE80211_BA_GAP_TIMEOUT 300 /* msec */
+#define IEEE80211_BA_GAP_TIMEOUT 800 /* msec */
/* Counter for consecutive frames which missed the BA window. */
int ba_winmiss;
/* Sequence number of previous frame which missed the BA window. */
Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.
On Mon, Jul 22, 2019 at 06:24:28PM +0200, Ingo Schwarze wrote: > But make sure that doesn't cause bugs to not get reported at all > because the process causes too much work or takes too long. :) > Oh yeah, no worries! :-)
Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.
Hi Jesper, Jesper Wallin wrote on Mon, Jul 22, 2019 at 06:09:03PM +0200: > On Mon, Jul 22, 2019 at 03:23:16PM +0200, Ingo Schwarze wrote: >> 3. Jesper, including a patch according to the best of your >> understanding is always welcome. Even if it turns out to be a >> bad patch, because often even a bad patch helps to understand >> what the OP thinks the problem might be: the saying is, bad >> patches trigger good ones. > I'm sorry for late reply, I've been busy/offline the last few days. No problem at all. > The lessons I take with me from this: > 1. Give myself more time to fully understand both the issue itself > and what the code actually does, before trying to fix it. > 2. Test, test with ktrace, test again and then run more tests. ;-) Sounds reasonable! Testing is important and often neglected. But make sure that doesn't cause bugs to not get reported at all because the process causes too much work or takes too long. :) When running out of time, in particular for bugs that seem important or when it is unclear if and when you might find more time to look at the issue, sending a preliminary report or a preliminary patch with a remark like "i'm trying to work on a patch" or "i only did rudimentary testing so far and ran out of time" might make sense to avoid needless delays. Yours, Ingo
Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.
On Mon, Jul 22, 2019 at 03:23:16PM +0200, Ingo Schwarze wrote: > > 3. Jesper, including a patch according to the best of your > understanding is always welcome. Even if it turns out to be a > bad patch, because often even a bad patch helps to understand > what the OP thinks the problem might be: the saying is, bad > patches trigger good ones. > Hi Ingo, I'm sorry for late reply, I've been busy/offline the last few days. The lessons I take with me from this: 1. Give myself more time to fully understand both the issue itself and what the code actually does, before trying to fix it. 2. Test, test with ktrace, test again and then run more tests. ;-) Thanks a *lot* for your wise pointers and your thorough analysis! Jesper Wallin
Re: [patch] Re-add 'proc' to vi(1) when running in secure mode.
Hi, Bryan Steele wrote on Sun, Jul 21, 2019 at 01:53:49PM -0400: > On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote: >> Oh, you're right. A bit ironic that I didn't notice the exec violation >> due to the fork being permitted now. Thanks for pointing this out! >> Scrap my old patch, here's a better proposal: [ ... and later, on a different patch ] > I somehow missed that. Indeed, moving the check into rcv_email before > fork makes more sense. Sorry for jumping the gun. Some observations, in decreasing order of importance: 1. Jesper, thanks for finding and reporting the regression. It is fixed now. 2. Bryan, thanks for suggesting the direction how to fix it. 3. Jesper, including a patch according to the best of your understanding is always welcome. Even if it turns out to be a bad patch, because often even a bad patch helps to understand what the OP thinks the problem might be: the saying is, bad patches trigger good ones. 4. Merely because something different eventually gets committed does not necessarily mean the original idea was bad. For many problems, there is more than one solution, and it is often a matter of judgement or taste which one is better. Jesper, in this case, re-adding "proc exec" would indeed have been an alternative solution - even though preventing "exec" is kind of the whole point of the -S option. 5. It might seem obvious, but after writing a patch and before sending it out, testing it makes sense. The easy part is testing that it actually solves the original problem. The slightly harder part is testing that is also solves similar problems (in this case, when catching SIGTERM). The hardest part is testing that it causes no regressions. 6. The history of this thread (the original commit of rev. 1.29, the initial suggestion to re-add "proc" only, the suggestion to skip rcv_mailfile()) proves once again that testing is surprisingly hard in practice. Most definitely, i also botched testing for many of the commits that i did in the past, and i also got many OKs for broken patches that i posted prematurely. It happens, even when trying to be careful. 7. Rarely used options rarely get tested. They attracks bugs like rotting fruits attract flies. So think twice before adding a new option to a program. Yours, Ingo CVSROOT:/cvs Module name:src Changes by: schwa...@cvs.openbsd.org2019/07/22 06:39:02 Modified files: usr.bin/vi/common: recover.c Log message: In secure mode (-S), skip sending mail when executing the :pre[serve] command or when dying from SIGTERM. This way, creating the recovery file works again without re-adding "proc exec" to the pledge(2). As reported by Jesper Wallin , this got broken by common/main.c rev. 1.29 (Nov 19, 2015). The general direction of the fix was suggested by brynet@. OK brynet@ and no opposition when shown on tech@
802.11n Tx aggregation for iwn(4)
This diff adds support for 11n Tx aggregation to net80211 and
implements driver support in iwn(4). Other drivers will follow later.
I chose to start with iwn(4) because damien@ left some unfinished bits
of Tx agg support in this driver.
Please use wifi as usual with this diff applied and check for regressions.
To see if Tx aggregation is active check the 'output block ack agreement'
counter with netstat; it should have a non-zero value:
$ netstat -W iwn0 | grep block | head -n2
1 new input block ack agreement
1 new output block ack agreement
^
Tx agg is only enabled on encrypted WPA2 wifi networks because buggy
A-MPDU receivers can otherwise be abused to inject frames and bypass
firewalls. See https://github.com/rpp0/aggr-inject for details.
I have tested the following iwn(4) devices:
WiFi Link 4965 ok; fatal firmware errors on 5GHz with + without txagg patch
WiFi Link 1000 ok
WiFi Link 5300 ok
Wireless-N 2200 ok
Advanced-N 6205 ok; tested by benno
Ultimate-N 6300 ok
Advanced-N 6200 ok
WiFi Link 5100 ok
With the following APs:
1) OpenBSD athn(4): Does not perform very well, likely due to a problem
visible in netstat on the AP ("input block ack window gaps timed out");
Will need a separate fix for OpenBSD's Rx agg implementation.
2) Asus RT-AC68U (broadcom-based): Fairly ok.
3) Pepwave AP one AC mini: Faster than the above.
There are lot of debug prints added in this diff which trigger with:
ifconfig iwn0 debug
Only enable debugging if you observe a problem.
Before reporting a problem please try to reproduce it without this patch,
not only with this patch applied. Report problems directly to me and attach
a compressed copy of /var/log/messages with debug output from this patch.
On some machines iwn(4) will stop working if debug is enabled because too
many printfs will cause interrupts being missed which results in firmware
errors. I don't intend to commit any of the new debug prints to CVS, they
will only remain while the diff is being tested.
diff refs/heads/master refs/heads/txagg
blob - e6818d039cd5019e7ed8e6c1d27b2e8cc29f714b
blob + 933b2a763d009055c324af50f26ac47ea10e3182
--- sys/dev/pci/if_iwn.c
+++ sys/dev/pci/if_iwn.c
@@ -57,6 +57,8 @@
#include
#include
#include
+#include /* for SEQ_LT */
+#undef DPRINTF /* defined in ieee80211_priv.h */
#include
#include
@@ -161,12 +163,20 @@ void iwn5000_rx_calib_results(struct
iwn_softc *,
struct iwn_rx_desc *, struct iwn_rx_data *);
void iwn_rx_statistics(struct iwn_softc *, struct iwn_rx_desc *,
struct iwn_rx_data *);
+void iwn_ampdu_txq_advance(struct iwn_softc *, struct iwn_tx_ring *,
+ int, int);
+void iwn_ampdu_tx_done(struct iwn_softc *, struct iwn_tx_ring *,
+ struct iwn_rx_desc *, uint16_t, struct iwn_txagg_status *,
+ int, uint32_t);
void iwn4965_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
struct iwn_rx_data *);
void iwn5000_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
struct iwn_rx_data *);
+void iwn_tx_done_free_txdata(struct iwn_softc *,
+ struct iwn_tx_data *);
+void iwn_clear_oactive(struct iwn_softc *, struct iwn_tx_ring *);
void iwn_tx_done(struct iwn_softc *, struct iwn_rx_desc *,
- uint8_t, uint8_t, uint8_t, uint16_t);
+ uint8_t, int, int, uint16_t);
void iwn_cmd_done(struct iwn_softc *, struct iwn_rx_desc *);
void iwn_notif_intr(struct iwn_softc *);
void iwn_wakeup_intr(struct iwn_softc *);
@@ -174,6 +184,7 @@ voidiwn_fatal_intr(struct iwn_softc *);
intiwn_intr(void *);
void iwn4965_update_sched(struct iwn_softc *, int, int, uint8_t,
uint16_t);
+void iwn4965_reset_sched(struct iwn_softc *, int, int);
void iwn5000_update_sched(struct iwn_softc *, int, int, uint8_t,
uint16_t);
void iwn5000_reset_sched(struct iwn_softc *, int, int);
@@ -469,6 +480,7 @@ iwn_attach(struct device *parent, struct device *self,
ic->ic_aselcaps = 0;
ic->ic_ampdu_params = (IEEE80211_AMPDU_PARAM_SS_4 | 0x3 /* 64k */);
if (sc->sc_flags & IWN_FLAG_HAS_11N) {
+ ic->ic_caps |= (IEEE80211_C_QOS | IEEE80211_C_TX_AMPDU);
/* Set HT capabilities. */
ic->ic_htcaps = IEEE80211_HTCAP_SGI20;
#ifdef notyet
@@ -526,10 +538,8 @@ iwn_attach(struct device *parent, struct device *self,
ic->ic_update_htprot = iwn_update_htprot;
ic->ic_ampdu_rx_start = iwn_ampdu_rx_start;
ic->ic_ampdu_rx_stop = iwn_ampdu_rx_stop;
-#ifdef notyet
ic->ic_ampdu_tx_start = iwn_ampdu_tx_start;
ic->ic_ampdu_tx_stop = iwn_ampdu_tx_stop;
-#endif
/* Override 802.11 state transition machine. */
sc->sc_newstate =
Re: fix: NULL dereference in bios(4)
On Sat, Jul 20, 2019 at 07:16:05PM +1000, Jonathan Gray wrote:
> On Fri, Jul 19, 2019 at 02:15:03PM +0200, Jan Klemkow wrote:
> > On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> > > On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > > > fixstring() can return NULL and it does on one of my machines.
> > > > Here is s fix that checks for NULL and return an empty string.
> > > >
> > > > ok?
> > >
> > > If it returns NULL shouldn't the strlcpy and printf both be skipped?
> >
> > I was unsure about this.
> >
> > > You've also not included the i386 portion of this.
> >
> > I missed that.
> >
> > The diff below addresses all issues.
> >
> > ok?
>
> What argument does fixstring have in this case?
Ten spaces (10x 0x20).
> Does you dmesg not include a date in the 'bios0' line?
no.
> I was considering making this a pointer/dynamically allocated so it can
> be NULL. If there is no valid date it should probably be NULL.
>
> >
> > Thanks,
> > Jan
> >
> > Index: arch/amd64/amd64/bios.c
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
> > retrieving revision 1.38
> > diff -u -p -r1.38 bios.c
> > --- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 - 1.38
> > +++ arch/amd64/amd64/bios.c 19 Jul 2019 11:45:11 -
> > @@ -144,9 +144,11 @@ bios_attach(struct device *parent, struc
> > fixstring(scratch));
> > if ((smbios_get_string(, sb->release,
> > scratch, sizeof(scratch))) != NULL) {
> > - strlcpy(smbios_bios_date, fixstring(scratch),
> > + char *s = fixstring(scratch) == NULL ? "" :
> > + fixstring(scratch);
> > + strlcpy(smbios_bios_date, s,
> > sizeof(smbios_bios_date));
> > - printf(" date %s", fixstring(scratch));
> > + printf(" date %s", s);
> > }
> > }
> >
> > Index: arch/i386/i386/bios.c
> > ===
> > RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
> > retrieving revision 1.121
> > diff -u -p -r1.121 bios.c
> > --- arch/i386/i386/bios.c 15 Jul 2019 00:35:10 - 1.121
> > +++ arch/i386/i386/bios.c 19 Jul 2019 11:58:57 -
> > @@ -308,10 +308,11 @@ biosattach(struct device *parent, struct
> > fixstring(scratch));
> > if ((smbios_get_string(, sb->release,
> > scratch, sizeof(scratch))) != NULL) {
> > - strlcpy(smbios_bios_date,
> > - fixstring(scratch),
> > + char *s = fixstring(scratch) == NULL ?
> > + "" : fixstring(scratch);
> > + strlcpy(smbios_bios_date, s,
> > sizeof(smbios_bios_date));
> > - printf(" date %s", fixstring(scratch));
> > + printf(" date %s", s);
> > }
> > }
> > smbios_info(sc->sc_dev.dv_xname);
> >
bgpd RIB handling cleanup part 1
The RIB handling, especially on reloads could be a bit nicer. This diff
is one of the preparation steps for this. The goal in the end is it to
make it possible to have RIBs (in the multi-RIB case) not only dependent
on Adj-RIB-In but also allow to use other RIBs as input. This way the
filters for the multi-RIB case becomes a lot simpler.
Anyway this diff cleans up the kroute code. It introduces a way to flush a
FIB table from the RDE which is currently unused but will be used later
on (this is a lot more efficent than removing each route with a imsg).
Also it fixes some bugs when multiple RIBs are in use and one is
removed. In this case all nexthops get disconnected which is bad, only
clear nexthops for the main RIB (the one where rdomainid == rtableid,
which holds the nexthops).
Finally I removed F_RIB_HASNOFIB since that define confused me. It is
enough to handle F_RIB_NOFIB and F_RIB_NOFIBSYNC, having a 3rd define with
NOFIB that does something different is just confusing for no good reason.
Especially the nexthop change should fix some bugs people may have
encountered when using more than one RIB.
--
:wq Claudio
Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.220
diff -u -p -r1.220 bgpd.c
--- bgpd.c 19 Jul 2019 07:40:41 - 1.220
+++ bgpd.c 22 Jul 2019 07:36:26 -
@@ -740,6 +740,14 @@ dispatch_imsg(struct imsgbuf *ibuf, int
conf->fib_priority))
rv = -1;
break;
+ case IMSG_KROUTE_FLUSH:
+ if (idx != PFD_PIPE_ROUTE)
+ log_warnx("route request not from RDE");
+ else if (imsg.hdr.len != IMSG_HEADER_SIZE)
+ log_warnx("wrong imsg len");
+ else if (kr_flush(imsg.hdr.peerid))
+ rv = -1;
+ break;
case IMSG_NEXTHOP_ADD:
if (idx != PFD_PIPE_ROUTE)
log_warnx("nexthop request not from RDE");
Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.389
diff -u -p -r1.389 bgpd.h
--- bgpd.h 19 Jul 2019 07:40:41 - 1.389
+++ bgpd.h 22 Jul 2019 07:39:48 -
@@ -506,6 +506,7 @@ enum imsg_type {
IMSG_MRT_CLOSE,
IMSG_KROUTE_CHANGE,
IMSG_KROUTE_DELETE,
+ IMSG_KROUTE_FLUSH,
IMSG_NEXTHOP_ADD,
IMSG_NEXTHOP_REMOVE,
IMSG_NEXTHOP_UPDATE,
@@ -1073,7 +1074,6 @@ extern struct rib_names ribnames;
#define F_RIB_NOEVALUATE 0x0002
#define F_RIB_NOFIB0x0004
#define F_RIB_NOFIBSYNC0x0008
-#define F_RIB_HASNOFIB (F_RIB_NOFIB | F_RIB_NOEVALUATE)
/* 4-byte magic AS number */
#define AS_TRANS 23456
@@ -1191,6 +1191,7 @@ void ktable_postload(u_int8_t);
int ktable_exists(u_int, u_int *);
int kr_change(u_int, struct kroute_full *, u_int8_t);
int kr_delete(u_int, struct kroute_full *, u_int8_t);
+int kr_flush(u_int);
voidkr_shutdown(u_int8_t, u_int);
voidkr_fib_couple(u_int, u_int8_t);
voidkr_fib_couple_all(u_int8_t);
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.236
diff -u -p -r1.236 kroute.c
--- kroute.c6 May 2019 09:49:26 - 1.236
+++ kroute.c22 Jul 2019 07:36:26 -
@@ -311,7 +311,8 @@ ktable_new(u_int rtableid, u_int rdomid,
/* everything is up and running */
kt->state = RECONF_REINIT;
- log_debug("%s: %s for rtableid %d", __func__, name, rtableid);
+ log_debug("%s: %s with rtableid %d rdomain %d", __func__, name,
+ rtableid, rdomid);
return (0);
}
@@ -351,7 +352,9 @@ ktable_destroy(struct ktable *kt, u_int8
log_debug("%s: freeing ktable %s rtableid %u", __func__, kt->descr,
kt->rtableid);
- knexthop_clear(kt);
+ /* only clear nexthop table if it is the main rdomain table */
+ if (kt->rtableid == kt->nhtableid)
+ knexthop_clear(kt);
kroute_clear(kt);
kroute6_clear(kt);
kr_net_clear(kt);
@@ -395,7 +398,7 @@ ktable_update(u_int rtableid, char *name
}
}
- if (flags & F_RIB_HASNOFIB)
+ if (flags & (F_RIB_NOFIB | F_RIB_NOEVALUATE))
/* only rdomain table must exist */
return (0);
@@ -758,6 +761,42 @@ kr_delete(u_int rtableid, struct kroute_
}
int
+kr_flush(u_int rtableid)
+{
+ struct ktable *kt;
+ struct kroute_node *kr, *next;
+ struct kroute6_node *kr6, *next6;
+
+ if ((kt = ktable_get(rtableid)) ==
Re: ping(8): better "-i wait" edge case handling
On Sun, Jul 21, 2019 at 08:30:23AM -0500, Scott Cheloha wrote: > On Sun, Jul 21, 2019 at 11:49:10AM +0200, Mark Kettenis wrote: > > > Date: Sun, 21 Jul 2019 10:50:27 +0200 > > > From: Claudio Jeker > > > > > > On Sat, Jul 20, 2019 at 08:12:40AM -0500, Scott Cheloha wrote: > > > > There are cases for ping(8)'s "-i wait" option that we don't handle > > > > correctly. > > > > > > > > Negative values smaller than -1: > > > > > > > > $ doas ping -i -0.1 localhost > > > > [no error] > > > > > > > > Positive values less than one microsecond: > > > > > > > > $ doas ping -i 0.001 localhost > > > > [no error] > > > > > > > > Large positive values that actually fit into a timeval: > > > > > > > > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost > > > > ping: illegal timing interval 9223372036854775807 > > > > > > > > The first two cases are bugs in the input checking. The latter case > > > > is a limitation of IEEE doubles: with only 52 bits of significand you > > > > can't represent the full range of a timeval on a platform with 64-bit > > > > time_t. > > > > > > > > This patch addresses the first two cases with better error checking. > > > > It also tries to handle the latter case a bit better by using IEEE > > > > quads, i.e. long doubles. With 64 bits of significand you can cover > > > > our time_t and the above case works. It isn't ~perfect~, but it's as > > > > close as we can get to perfect without parsing the number by hand as > > > > we do in sleep(1) (cf. bin/sleep/sleep.c). > > > > > > > > With this patch those cases all work as expected: > > > > > > > > $ doas ping -i -0.1 localhost > > > > ping: interval is invalid: -0.1 > > > > $ doas ping -i 0.001 localhost > > > > ping: interval is too small: 0.001 > > > > $ ping -i $(bc -e '2^63' -e quit) localhost > > > > ping: interval is too large: 9223372036854775808 > > > > $ ping -i $(bc -e '2^63 - 1' -e quit) localhost > > > > PING localhost.local (23.195.69.106): 56 data bytes > > > > 64 bytes from 23.195.69.106: icmp_seq=0 ttl=54 time=18.001 ms > > > > [stalls forever] > > > > > > > > -- > > > > > > > > While we're modifying this code, I think "-i interval" is better than > > > > "-i wait". The "i for interval" mnemonic is particularly nice. The > > > > current "wait" argument name sort-of implies a relationship with the > > > > "-w maxwait" option, which is not the case. We're also already using > > > > "timing interval" in these error messages here. I've tweaked the > > > > error messages to look more like the usual strtonum(3) error messages. > > > > > > > > ok? > > > > > > Can't we limit the -i maximum value to something that fits into the double > > > instead of using long double in ping. Nobody will ever try to using ping > > > with a timeout that is longer than the operators expected life time. > > Sure. Here's a diff that uses UINT_MAX. The oldest known person > lived to 123. French, if I recall correctly. UINT_MAX gives us 136 > years, so we have a bit of room to grow just in case someone beats the > record. And 52 bits of significand is plenty for 32 bits of integral > plus a fractional portion. > > > Using a long double isn't a solution anyway, since we have quite a few > > architectures where long double is the same as double. > > Oh, hmmm, didn't know that. > > -- > > this diff better? Can't this just use the overflow detection of strtod()? > Index: ping.c > === > RCS file: /cvs/src/sbin/ping/ping.c,v > retrieving revision 1.237 > diff -u -p -r1.237 ping.c > --- ping.c20 Jul 2019 00:49:54 - 1.237 > +++ ping.c21 Jul 2019 13:23:55 - > @@ -258,7 +258,7 @@ main(int argc, char *argv[]) > char *e, *target, hbuf[NI_MAXHOST], *source = NULL; > char rspace[3 + 4 * NROUTES + 1]; /* record route space */ > const char *errstr; > - double intval; > + double fraction, integral, seconds; > uid_t ouid, uid; > gid_t gid; > u_int rtableid = 0; > @@ -332,17 +332,21 @@ main(int argc, char *argv[]) > case 'S': /* deprecated */ > source = optarg; > break; > - case 'i': /* wait between sending packets */ > - intval = strtod(optarg, ); > - if (*optarg == '\0' || *e != '\0') > - errx(1, "illegal timing interval %s", optarg); > - if (intval < 1 && ouid) > - errx(1, "only root may use interval < 1s"); > - interval.tv_sec = (time_t)intval; > - interval.tv_usec = > - (long)((intval - interval.tv_sec) * 100); > - if (interval.tv_sec < 0) > - errx(1, "illegal timing interval %s", optarg); > + case 'i': /* interval between packets */ > +
