Re: relayd patch for websocket upgrade
On Sun, Mar 07, 2021 at 06:46:33PM +0100, Marcus MERIGHI wrote: > Hello Jonathon! > > welcome to the party: > > https://marc.info/?t=15833439123 > > especially the two comments by sthen@: > > https://marc.info/?m=161349608614743 > https://marc.info/?m=16135019371 > > reyk@ removed from CC: on purpose: > https://twitter.com/reykfloeter/status/1284868070901776384 > > Marcus > > jonathon.fletc...@gmail.com (Jonathon Fletcher), 2021.03.06 (Sat) 21:02 (CET): > > When relayd relays a connection upgrade to a websocket, it relays > > the outbound "Connection: Upgrade" header from the interal server. > > > > It also tags on a "Connection: close" header to the outbound > > response - ie the response goes out with two "Connection" > > header lines. > > > > Chrome and Netscape work despite the double upgrade/close connection > > headers. Safari fails. > > > > Small patch below against 6.8 to only send the "Connection: close" > > header if we are not handling a http_status 101. > > > > Thanks, > > Jonathon > > > > > > cvs -q -d /cvs diff -ub -rOPENBSD_6_8 usr.sbin/relayd/relay_http.c > > > > > > Index: usr.sbin/relayd/relay_http.c > > === > > RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v > > retrieving revision 1.79 > > diff -u -p -u -b -r1.79 relay_http.c > > --- usr.sbin/relayd/relay_http.c4 Sep 2020 13:09:14 - 1.79 > > +++ usr.sbin/relayd/relay_http.c6 Mar 2021 19:46:56 - > > @@ -526,7 +526,7 @@ relay_read_http(struct bufferevent *bev, > > * Ask the server to close the connection after this request > > * since we don't read any further request headers. > > */ > > - if (cre->toread == TOREAD_UNLIMITED) > > + if (cre->toread == TOREAD_UNLIMITED && desc->http_status != 101) > > if (kv_add(>http_headers, "Connection", > > "close", 0) == NULL) > > goto fail; > > Marcus, I did not realize that there was already a party. Apologies for my previous, duplicate, patch. Reading through the thread, I came to the conclusion that the comments worried that the previous patch(es) were not specific enough. The current relayd behaviour is that outbound http responses have a "Connection: close" header/value attached to them by relayd. This can result in multiple "Connection" headers in the response which is .. not good. The current behaviour is because relayd does not handle repeated http request/response sequences after the first one and prefers to force the http session to close. Fortunately for websockets, the protocol after the websocket upgrade is not http and so there is no need for relayd to look for or process http headers after the upgrade. Here is an updated patch. This avoids the incorrect current in-tree behaviour in the following specific sitations: 1: The headers for an outbound (internal -> external) response already include "Connection: Upgrade", "Upgrade: websocket" and the config permits websocket upgrades, or 2: The headers for an outbound response already include a Connection header with the value "close" - so do not send a duplicate as the in-tree code currently does. I think this is specfic enough for now. In order for a websocket upgrade to work the external client has to request it and the internal server has to respond in agreement. I am explicit about websocket upgrades in my configs: I require the "Connection" and "Upgrade" headers in the rule that directs traffic to the websocket pool: pass request quick \ header "Host" value "ws.example.com" \ header "Connection" value "Upgrade" \ header "Upgrade" value "websocket" \ forward to This is for my use cases (tls accelerator) and relayd is adept at handling them. I really appreciate the functionality of relayd in base. Let me know if there are specific concerns about the patch below or if there are specific preferred ways to accomplish better compliance with the RFC within the context of relayd. Thanks, Jonathon The Connection header is covered in: https://tools.ietf.org/html/rfc7230#section-6 Index: usr.sbin/relayd/relay_http.c === RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v retrieving revision 1.79 diff -u -p -u -b -r1.79 relay_http.c --- usr.sbin/relayd/relay_http.c4 Sep 2020 13:09:14 - 1.79 +++ usr.sbin/relayd/relay_http.c8 Mar 2021 00:03:31 - @@ -161,6 +161,8 @@ relay_read_http(struct bufferevent *bev, size_t size, linelen; struct kv *hdr = NULL; struct kv *upgrade = NULL, *upgrade_ws = NULL; + struct kv *connection_close = NULL; + int ws_response = 0; getmonotime(>se_tv_last);
A curious case of disappearing iwx
I had an iwx working fine (modulo known issues) for a bit on amd64 current: iwx0 at pci0 dev 20 function 3 "Intel Wi-Fi 6 AX201" rev 0x00, msix iwx0: hw rev 0x350, fw ver 48.1335886879.0, address xxx Then for no discernible reason the kernel started saying: "Intel Wi-Fi 6 AX201" rev 0x00 at pci0 dev 20 function 3 not configured At first I blamed a kernel change, but booting into an old kernel didn't help. What did bring the device back is booting Linux kernel which had this to say: iwlwifi :00:14.3: enabling device ( -> 0002) iwlwifi :00:14.3: Direct firmware load for iwlwifi-QuZ-a0-hr-b0-56.ucode failed with error -2 iwlwifi :00:14.3: api flags index 2 larger than supported by driver iwlwifi :00:14.3: TLV_FW_FSEQ_VERSION: FSEQ Version: 65.3.35.22 iwlwifi :00:14.3: Found debug destination: EXTERNAL_DRAM iwlwifi :00:14.3: Found debug configuration: 0 iwlwifi :00:14.3: loaded firmware version 55.d9698065.0 QuZ-a0-hr-b0-55.ucode op_mode iwlmvm iwlwifi :00:14.3: Direct firmware load for iwl-debug-yoyo.bin failed with error -2 iwlwifi :00:14.3: Detected Intel(R) Wi-Fi 6 AX201 160MHz, REV=0x354 iwlwifi :00:14.3: base HW address: xxx iwlwifi :00:14.3 wlp0s20f3: renamed from wlan0 Maybe the "enabling device ( -> 0002)" part is what fixed the issue? Anyway, I got my iwx back though I still am curious why it went away. Thanks Greg
Re: Elaborate on dhcpleased(8) requirement to interface state
Hi Theo, On 3/7/21 5:45 PM, Theo de Raadt wrote: > Paul Kelly wrote: > >> So...here is a proposal for updating various man pages to try help >> others who make a similar migration to dhcpleased. It's a bit of a >> scattershot approach and the wording can probably be improved, but it is >> hopefully consistent with the existing pattern for describing auto >> interface configuration, and it at least should prompt a user to >> double-check their interface state before they (incorrectly) conclude >> that dhcpleased doesn't work on trunk interfaces. > > I don't understand where you are going with this type of documentation. Fair enough. On reflection, the change to dhcpleased.8 isn't useful because it clumsily tries to make a point about dhcpleased behaviour that is only relevant in the context of dhclient behaviour. I still think the changes for hostname.if and ifconfig are relevant in the context of those man pages as they currently stand, because dhclient and slaacd are already described there. I'll try to add some justification for that below, but I also note that you're hinting at bigger things to come so it might all be redundant in the big picture. > Of course the interface has to be up. Interfaces that are down don't > participate in networking. It is obvious. It's a fair point, and it's certainly obvious when not relying on dhclient to do it automatically. It wasn't clear for me in the transition from dhclient to dhcpleased that the behaviour was different in this respect, and the commit message for dhcpleased seemed to indicate that it should only require a substitution of "dhcp" -> "inet autoconf" in hostname.if. The implicit reliance on dhclient's automatic behaviour is also present in the faq - faq6 makes a point that "dhcp" is all that is required with dhclient. So I wouldn't be too surprised if there are a number of people out there who could try to make the transition and end up scratching their head like I did. In my view there is also a small disconnect between the man pages for dhcpleased man page and hostname.if. The dhcpleased man page mentions the AUTOCONF4 flag and then delegates the details to the man pages for hostname.if and ifconfig. Those man pages don't appear to mention dhcpleased at all, and instead they provide context and examples for dhclient and slaacd. This is a bit jarring and leaves the impression that something is missing. So I don't mean to say there are major faults in the documentation here; but I do think that these relatively small changes would make the transition from dhclient more obvious. > We are discussing bringing the interfaces up in a more automatic > fashion, and then this won't matter. I can't really know exactly what you're alluding to here, but it's certainly possible to see some trends relating to network configuration in the past few weeks. I'm certainly interested to see where it goes. My presumption would normally be that this kind of work would be delayed until 6.10, based on the current timing of 6.9. But maybe there are still some surprises in store for the 6.9 release as well :)
Re: sendsyslog kernel buffer
> On 7 Mar 2021, at 02:17, Alexander Bluhm wrote:
>
> Hi
>
> Early daemons like dhcpleased, slaacd, unwind, resolvd are started
> before syslogd. This results in ugly sendsyslog: dropped 1 message
> logs and the real message is lost.
>
> Changing the start order of syslogd and and network daemons is not
> feasible. A possible solution is a temporary buffer for log meassges
> in kernel.
>
> ok?
>
I wonder they were not buffered. But does it make sense to drop the
most recent messages?
> bluhm
>
> Index: sys/kern/subr_log.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 subr_log.c
> --- sys/kern/subr_log.c 8 Feb 2021 08:18:45 - 1.72
> +++ sys/kern/subr_log.c 5 Mar 2021 21:58:50 -
> @@ -454,6 +454,146 @@ logioctl(dev_t dev, u_long com, caddr_t
> return (0);
> }
>
> +/*
> + * If syslogd is not running, temporarily store a limited amount of messages
> + * in kernel. After log stash is full, drop messages and count them. When
> + * syslogd is available again, next log message will flush the stashed
> + * messages and insert a message with drop count. Calls to malloc(9) and
> + * copyin(9) may sleep, protect data structures with rwlock.
> + */
> +
> +#define LOGSTASH_SIZE100
> +struct logstash_messages {
> + char*lgs_buffer;
> + size_t lgs_size;
> +} logstash_messages[LOGSTASH_SIZE];
> +
> +struct logstash_messages *logstash_in = _messages[0];
> +struct logstash_messages *logstash_out = _messages[0];
> +
> +struct rwlock logstash_rwlock = RWLOCK_INITIALIZER("logstash");
> +
> +int logstash_dropped, logstash_error, logstash_pid;
> +
> +void logstash_insert(const char *, size_t, int, pid_t);
> +void logstash_remove(void);
> +int logstash_sendsyslog(struct proc *);
> +
> +static inline int
> +logstash_empty(void)
> +{
> + rw_assert_anylock(_rwlock);
> +
> + return logstash_out->lgs_buffer == NULL;
> +}
> +
> +static inline int
> +logstash_full(void)
> +{
> + rw_assert_anylock(_rwlock);
> +
> + return logstash_out->lgs_buffer != NULL &&
> + logstash_in == logstash_out;
> +}
> +
> +static inline void
> +logstash_increment(struct logstash_messages **msg)
> +{
> + rw_assert_wrlock(_rwlock);
> +
> + KASSERT((*msg) >= _messages[0]);
> + KASSERT((*msg) < _messages[LOGSTASH_SIZE]);
> + (*msg)++;
> + if ((*msg) == _messages[LOGSTASH_SIZE])
> + (*msg) = _messages[0];
> +}
> +
> +void
> +logstash_insert(const char *buf, size_t nbyte, int error, pid_t pid)
> +{
> + rw_enter_write(_rwlock);
> +
> + if (logstash_full()) {
> + if (logstash_dropped == 0) {
> + logstash_error = error;
> + logstash_pid = pid;
> + }
> + logstash_dropped++;
> +
> + rw_exit(_rwlock);
> + return;
> + }
> +
> + logstash_in->lgs_buffer = malloc(nbyte, M_LOG, M_WAITOK);
> + copyin(buf, logstash_in->lgs_buffer, nbyte);
> + logstash_in->lgs_size = nbyte;
> + logstash_increment(_in);
> +
> + rw_exit(_rwlock);
> +}
> +
> +void
> +logstash_remove(void)
> +{
> + rw_assert_wrlock(_rwlock);
> +
> + KASSERT(!logstash_empty());
> + free(logstash_out->lgs_buffer, M_LOG, logstash_out->lgs_size);
> + logstash_out->lgs_buffer = NULL;
> + logstash_increment(_out);
> +
> + /* Insert dropped message in sequence where messages were dropped. */
> + if (logstash_dropped) {
> + size_t l, nbyte;
> + char buf[80];
> +
> + l = snprintf(buf, sizeof(buf),
> + "<%d>sendsyslog: dropped %d message%s, error %d, pid %d",
> + LOG_KERN|LOG_WARNING, logstash_dropped,
> + logstash_dropped == 1 ? "" : "s",
> + logstash_error, logstash_pid);
> + logstash_dropped = 0;
> + logstash_error = 0;
> + logstash_pid = 0;
> +
> + /* Cannot fail, we have just freed a slot. */
> + KASSERT(!logstash_full());
> + nbyte = ulmin(l, sizeof(buf) - 1);
> + logstash_in->lgs_buffer = malloc(nbyte, M_LOG, M_WAITOK);
> + memcpy(logstash_in->lgs_buffer, buf, nbyte);
> + logstash_in->lgs_size = nbyte;
> + logstash_increment(_in);
> + }
> +}
> +
> +int
> +logstash_sendsyslog(struct proc *p)
> +{
> + int error;
> +
> + rw_enter_write(_rwlock);
> +
> + while (logstash_out->lgs_buffer != NULL) {
> + error = dosendsyslog(p, logstash_out->lgs_buffer,
> + logstash_out->lgs_size, 0, UIO_SYSSPACE);
> + if (error) {
> + rw_exit(_rwlock);
> + return (error);
> + }
> + logstash_remove();
> + }
> +
> + rw_exit(_rwlock);
> + return (0);
> +}
> +
> +/*
Re: smtpd: use mx name for sni
On Sun, 07 Mar 2021 21:47:45 +0100, Eric Faurot wrote: > As spotted by krw@, the mta should use the mx hostname for sni, not > the reverse dns for the peer address. Yes, this matches the previous behavior with ssl_check_name(). OK millert@ - todd
Re: smtpd: use mx name for sni
On Sun, Mar 07, 2021 at 09:47:45PM +0100, Eric Faurot wrote: > As spotted by krw@, the mta should use the mx hostname for sni, not > the reverse dns for the peer address. ok tb > > Eric. > > > Index: mta_session.c > === > RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v > retrieving revision 1.139 > diff -u -p -r1.139 mta_session.c > --- mta_session.c 5 Mar 2021 12:37:32 - 1.139 > +++ mta_session.c 7 Mar 2021 20:18:42 - > @@ -1596,7 +1596,7 @@ mta_tls_init(struct mta_session *s) > return; > } > > - io_connect_tls(s->io, tls, s->route->dst->ptrname); > + io_connect_tls(s->io, tls, s->mxname); > } > > static void >
smtpd: use mx name for sni
As spotted by krw@, the mta should use the mx hostname for sni, not the reverse dns for the peer address. Eric. Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.139 diff -u -p -r1.139 mta_session.c --- mta_session.c 5 Mar 2021 12:37:32 - 1.139 +++ mta_session.c 7 Mar 2021 20:18:42 - @@ -1596,7 +1596,7 @@ mta_tls_init(struct mta_session *s) return; } - io_connect_tls(s->io, tls, s->route->dst->ptrname); + io_connect_tls(s->io, tls, s->mxname); } static void
Re: relayd patch for websocket upgrade
Hello Jonathon! welcome to the party: https://marc.info/?t=15833439123 especially the two comments by sthen@: https://marc.info/?m=161349608614743 https://marc.info/?m=16135019371 reyk@ removed from CC: on purpose: https://twitter.com/reykfloeter/status/1284868070901776384 Marcus jonathon.fletc...@gmail.com (Jonathon Fletcher), 2021.03.06 (Sat) 21:02 (CET): > When relayd relays a connection upgrade to a websocket, it relays > the outbound "Connection: Upgrade" header from the interal server. > > It also tags on a "Connection: close" header to the outbound > response - ie the response goes out with two "Connection" > header lines. > > Chrome and Netscape work despite the double upgrade/close connection > headers. Safari fails. > > Small patch below against 6.8 to only send the "Connection: close" > header if we are not handling a http_status 101. > > Thanks, > Jonathon > > > cvs -q -d /cvs diff -ub -rOPENBSD_6_8 usr.sbin/relayd/relay_http.c > > > Index: usr.sbin/relayd/relay_http.c > === > RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v > retrieving revision 1.79 > diff -u -p -u -b -r1.79 relay_http.c > --- usr.sbin/relayd/relay_http.c 4 Sep 2020 13:09:14 - 1.79 > +++ usr.sbin/relayd/relay_http.c 6 Mar 2021 19:46:56 - > @@ -526,7 +526,7 @@ relay_read_http(struct bufferevent *bev, >* Ask the server to close the connection after this request >* since we don't read any further request headers. >*/ > - if (cre->toread == TOREAD_UNLIMITED) > + if (cre->toread == TOREAD_UNLIMITED && desc->http_status != 101) > if (kv_add(>http_headers, "Connection", > "close", 0) == NULL) > goto fail; >
Re: [PATCH] [src] games/hack/help - fix ordinal directions
On Sun, Mar 07, 2021 at 04:58:02PM +, Raf Czlonka wrote: > Hello, > > Ordinal (intercardinal) directions are swapped in hack(6)'s help. > > For a second there, I though that no one else noticed since the 80s ;^) > ...not the case[0] :^P > > While there, I'm tempted to capitalise both cardinal and ordinal > directions as they traditionally are - patch for each option below. Committed, thanks. I took the version without capitalization, as it seems that various grammar sites agree that it's not usually done for generic directions.
[PATCH] [src] games/hack/help - fix ordinal directions
Hello, Ordinal (intercardinal) directions are swapped in hack(6)'s help. For a second there, I though that no one else noticed since the 80s ;^) ...not the case[0] :^P While there, I'm tempted to capitalise both cardinal and ordinal directions as they traditionally are - patch for each option below. [0] http://cvsweb.netbsd.org/bsdweb.cgi/src/games/hack/help.diff?r1=1.1=1.2_with_tag=MAIN=h Cheers, Raf Index: games/hack/help === RCS file: /cvs/src/games/hack/help,v retrieving revision 1.1.1.1 diff -u -p -r1.1.1.1 help --- games/hack/help 18 Oct 1995 08:49:02 - 1.1.1.1 +++ games/hack/help 7 Mar 2021 16:44:20 - @@ -45,7 +45,7 @@ Commands: > down: go down (just like up). kjhlyubn - go one step in the direction indicated. k: north (i.e., to the top of the screen), - j: south, h: west, l: east, y: ne, u: nw, b: se, n: sw. + j: south, h: west, l: east, y: nw, u: ne, b: sw, n: se. KJHLYUBN - Go in that direction until you hit a wall or run into something. m (followed by one of kjhlyubn): move without picking up And with directions capitalised: Index: games/hack/help === RCS file: /cvs/src/games/hack/help,v retrieving revision 1.1.1.1 diff -u -p -r1.1.1.1 help --- games/hack/help 18 Oct 1995 08:49:02 - 1.1.1.1 +++ games/hack/help 7 Mar 2021 16:45:24 - @@ -44,8 +44,8 @@ Commands: < up: go up the staircase (if you are standing on it). > down: go down (just like up). kjhlyubn - go one step in the direction indicated. - k: north (i.e., to the top of the screen), - j: south, h: west, l: east, y: ne, u: nw, b: se, n: sw. + k: North (i.e., to the top of the screen), + j: South, h: West, l: East, y: NW, u: NE, b: SW, n: SE. KJHLYUBN - Go in that direction until you hit a wall or run into something. m (followed by one of kjhlyubn): move without picking up
Re: Elaborate on dhcpleased(8) requirement to interface state
Paul Kelly wrote: > So...here is a proposal for updating various man pages to try help > others who make a similar migration to dhcpleased. It's a bit of a > scattershot approach and the wording can probably be improved, but it is > hopefully consistent with the existing pattern for describing auto > interface configuration, and it at least should prompt a user to > double-check their interface state before they (incorrectly) conclude > that dhcpleased doesn't work on trunk interfaces. I don't understand where you are going with this type of documentation. Of course the interface has to be up. Interfaces that are down don't participate in networking. It is obvious. We are discussing bringing the interfaces up in a more automatic fashion, and then this won't matter.
Elaborate on dhcpleased(8) requirement to interface state
Hi tech, I've been playing with dhcpleased on my laptop, where I run with a failover trunk, consisting of iwn and em interfaces as trunkports. This configuration has worked well for a few years with dhclient. When testing dhcpleased I found a subtle difference in behaviour that took me a little while to figure out. I initially couldn't get dhcpleased to operate on the trunk interface, and eventually realised that it is because dhcpleased acts on the combination of the AUTOCONF4 flag and interface state (IFF_UP). In contrast, dhclient will bring the interface up automagically during initialisation. Intuitively, I actually expected that the trunk interface state would be controlled by collective state of its trunkports - i.e. up if any single trunkport is marked as up. After further consideration I'm doubtful as to whether that would make sense at all, and such behaviour risks ping-pong up/down games if root decides to intervene and manually set the state. In any case, it's not how trunk actually works. So...here is a proposal for updating various man pages to try help others who make a similar migration to dhcpleased. It's a bit of a scattershot approach and the wording can probably be improved, but it is hopefully consistent with the existing pattern for describing auto interface configuration, and it at least should prompt a user to double-check their interface state before they (incorrectly) conclude that dhcpleased doesn't work on trunk interfaces. Paul Index: sbin/ifconfig/ifconfig.8 === RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v retrieving revision 1.362 diff -u -p -u -p -r1.362 ifconfig.8 --- sbin/ifconfig/ifconfig.8 20 Feb 2021 01:21:04 - 1.362 +++ sbin/ifconfig/ifconfig.8 7 Mar 2021 11:52:31 - @@ -161,6 +161,13 @@ automatically configures IPv6 addresses .Sy AUTOCONF6 set. .Pp +.Xr dhcpleased 8 +automatically configures IPv4 addresses for +.Dq up +interfaces with +.Sy AUTOCONF4 +set. +.Pp .Xr dhclient 8 only configures interfaces with .Sy AUTOCONF4 Index: sbin/dhcpleased/dhcpleased.8 === RCS file: /cvs/src/sbin/dhcpleased/dhcpleased.8,v retrieving revision 1.2 diff -u -p -u -p -r1.2 dhcpleased.8 --- sbin/dhcpleased/dhcpleased.8 26 Feb 2021 17:14:25 - 1.2 +++ sbin/dhcpleased/dhcpleased.8 7 Mar 2021 11:52:54 - @@ -29,8 +29,10 @@ is a dynamic host configuration protocol (DHCP) daemon for clients. It requests IP configuration information from for example those offered by .Xr dhcpd 8 , -on interfaces with the -.Sy AUTOCONF4 +on +.Dq up +interfaces with the +.Dq AUTOCONF4 flag set. See .Xr hostname.if 5 Index: share/man/man5/hostname.if.5 === RCS file: /cvs/src/share/man/man5/hostname.if.5,v retrieving revision 1.73 diff -u -p -u -p -r1.73 hostname.if.5 --- share/man/man5/hostname.if.5 23 Dec 2020 17:22:07 - 1.73 +++ share/man/man5/hostname.if.5 7 Mar 2021 11:53:14 - @@ -229,7 +229,9 @@ up The following packed formats are valid for configuring network interfaces with dynamic addresses. .Pp -For IPv4 dynamic addressing using DHCP, the literal string +For IPv4 dynamic addressing using +.Xr dhclient 8 , +the literal string .Dq dhcp followed first by .Xr dhclient 8 @@ -243,6 +245,20 @@ is executed before .Bd -ragged -offset indent .Li dhcp .Op Va dhclient_options +.Op Va ifconfig_options +.Ed +.Pp +For IPv4 dynamic addressing using +.Xr dhcpleased 8 , +the literal string +.Dq inet autoconf up +followed by any options to be passed to +.Xr ifconfig 8 . +Note that +.Xr dhcpleased 8 +must also be enabled. +.Bd -ragged -offset indent +.Li inet autoconf .Op Va ifconfig_options .Ed .Pp
Re: 2 diffs for dev/acpi/dsdt.c
> Date: Sat, 27 Feb 2021 19:43:31 +0900 (JST)
> From: YASUOKA Masahiko
> Content-Type: Text/Plain; charset=us-ascii
>
> Hi,
>
> Let me update "diff #2".
>
> On Fri, 26 Feb 2021 13:42:32 +0900 (JST)
> YASUOKA Masahiko wrote:
> > My vaio repeatedly crashed by "Data modified on freelist"(*1) or other
> > memory corruptions. After my long time debug, I found the route cause
> > is a handling of references of LocalX, like the following:
> >
> > If ((SMRW (0x0B, 0x16, 0x21, RefOf (Local0)) == Zero))
> >
> > In the called control method, "RefOf (Local1)" is referred as Arg3, is
> > stored a value like the following:
> >
> > Arg3 = \_SB.PCI0.LPCB.EC0.SMD0
> >
> > In aml_store(), lvalue is reset if lvalue is a LocalX. But since that
> > was done before resolving the reference, lvalue was not reset if
> > lvalue is a reference of LocalX.
> >
> > diff #1 fixes that problem. It resets lvalue after resolving
> > references.
> >
> > ok?
> >
> > diff #2 adds aml_die() if any memory corruption occurs when creating
> > field in a buffer. This actually happens on my vaio (pro pk 14) if
> > diff #1 is not applied.
> >
> > ok?
> >
> > diff #2
> >
> > Index: sys/dev/acpi/dsdt.c
> > ===
> > RCS file: /var/cvs/openbsd/src/sys/dev/acpi/dsdt.c,v
> > retrieving revision 1.257
> > diff -u -p -r1.257 dsdt.c
> > --- sys/dev/acpi/dsdt.c 17 Dec 2020 17:57:19 - 1.257
> > +++ sys/dev/acpi/dsdt.c 26 Feb 2021 04:33:21 -
> > @@ -2742,11 +2742,17 @@ aml_rwfield(struct aml_value *fld, int b
> > } else if (mode == ACPI_IOREAD) {
> > /* bufferfield:read */
> > _aml_setvalue(val, AML_OBJTYPE_INTEGER, 0, 0);
> > + if (ref1->length < aml_bytepos(fld->v_field.bitpos) +
> > + aml_bytelen(fld->v_field.bitlen))
> > + aml_die("bufferfield:read out of range");
> > aml_bufcpy(>v_integer, 0, ref1->v_buffer,
> > fld->v_field.bitpos, fld->v_field.bitlen);
> > } else {
> > /* bufferfield:write */
> > val = aml_convert(val, AML_OBJTYPE_INTEGER, -1);
> > + if (ref1->length < aml_bytepos(fld->v_field.bitpos) +
> > + aml_bytelen(fld->v_field.bitlen))
> > + aml_die("bufferfield:write out of range");
> > aml_bufcpy(ref1->v_buffer, fld->v_field.bitpos, >v_integer,
> > 0, fld->v_field.bitlen);
> > aml_delref(, "wrbuffld");
>
> It's better to die when creating a field which refers out of range
> memory.
>
> ok?
I'm not 100% confident about this one. The check seems correct to me,
but there is too much broken AML out there...
Maybe Theo can put this one in snaps for a bit?
> Index: sys/dev/acpi/dsdt.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/dev/acpi/dsdt.c,v
> retrieving revision 1.257
> diff -u -p -r1.257 dsdt.c
> --- sys/dev/acpi/dsdt.c 17 Dec 2020 17:57:19 - 1.257
> +++ sys/dev/acpi/dsdt.c 27 Feb 2021 09:58:31 -
> @@ -2790,6 +2790,11 @@ aml_createfield(struct aml_value *field,
> data->type != AML_OBJTYPE_BUFFER)
> data = aml_convert(data, AML_OBJTYPE_BUFFER, -1);
>
> + if (field->type == AML_OBJTYPE_BUFFERFIELD &&
> + data->length < aml_bytepos(bpos) + aml_bytelen(blen))
> + aml_die("%s(%s) out of range\n", aml_mnem(opcode, 0),
> + aml_nodename(field->node));
> +
> field->v_field.type = opcode;
> field->v_field.bitpos = bpos;
> field->v_field.bitlen = blen;
>
>
Re: 2 diffs for dev/acpi/dsdt.c
> Date: Fri, 26 Feb 2021 13:42:32 +0900 (JST)
> From: YASUOKA Masahiko
>
> Hi,
>
> My vaio repeatedly crashed by "Data modified on freelist"(*1) or other
> memory corruptions. After my long time debug, I found the route cause
> is a handling of references of LocalX, like the following:
>
> If ((SMRW (0x0B, 0x16, 0x21, RefOf (Local0)) == Zero))
>
> In the called control method, "RefOf (Local1)" is referred as Arg3, is
> stored a value like the following:
>
> Arg3 = \_SB.PCI0.LPCB.EC0.SMD0
>
> In aml_store(), lvalue is reset if lvalue is a LocalX. But since that
> was done before resolving the reference, lvalue was not reset if
> lvalue is a reference of LocalX.
>
> diff #1 fixes that problem. It resets lvalue after resolving
> references.
>
> ok?
>
> diff #2 adds aml_die() if any memory corruption occurs when creating
> field in a buffer. This actually happens on my vaio (pro pk 14) if
> diff #1 is not applied.
>
> ok?
>
> diff #1
>
> Index: sys/dev/acpi/dsdt.c
> ===
> RCS file: /var/cvs/openbsd/src/sys/dev/acpi/dsdt.c,v
> retrieving revision 1.257
> diff -u -p -r1.257 dsdt.c
> --- sys/dev/acpi/dsdt.c 17 Dec 2020 17:57:19 - 1.257
> +++ sys/dev/acpi/dsdt.c 26 Feb 2021 04:12:03 -
> @@ -2961,11 +2961,11 @@ aml_store(struct aml_scope *scope, struc
> aml_rwfield(rhs, 0, rhs->v_field.bitlen, , ACPI_IOREAD);
> rhs =
> }
> +
> + lhs = aml_gettgt(lhs, AMLOP_STORE);
Can you add a blank line here?
> /* Store to LocalX: free value */
> if (lhs->stack >= AMLOP_LOCAL0 && lhs->stack <= AMLOP_LOCAL7)
> aml_freevalue(lhs);
> -
> - lhs = aml_gettgt(lhs, AMLOP_STORE);
> switch (lhs->type) {
> case AML_OBJTYPE_UNINITIALIZED:
> aml_copyvalue(lhs, rhs);
diff #1 is ok kettenis@ with the fix mentioned above
Re: sendsyslog kernel buffer
Nice, does what it says on the lid:
Mar 7 11:42:10 openbsd-build dhcpleased[65929]: adding 10.2.1.48 to vio1
(lease from 10.2.1.11)
Mar 7 11:42:10 openbsd-build dhcpleased[65929]: adding nameservers 10.2.1.1
9.9.9.9 8.8.8.8 (lease from 10.2.1.11 on vio1)
On Sun, Mar 07, 2021 at 12:17:18AM +0100, Alexander Bluhm wrote:
> Hi
>
> Early daemons like dhcpleased, slaacd, unwind, resolvd are started
> before syslogd. This results in ugly sendsyslog: dropped 1 message
> logs and the real message is lost.
>
> Changing the start order of syslogd and and network daemons is not
> feasible. A possible solution is a temporary buffer for log meassges
> in kernel.
>
> ok?
>
> bluhm
>
> Index: sys/kern/subr_log.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 subr_log.c
> --- sys/kern/subr_log.c 8 Feb 2021 08:18:45 - 1.72
> +++ sys/kern/subr_log.c 5 Mar 2021 21:58:50 -
> @@ -454,6 +454,146 @@ logioctl(dev_t dev, u_long com, caddr_t
> return (0);
> }
>
> +/*
> + * If syslogd is not running, temporarily store a limited amount of messages
> + * in kernel. After log stash is full, drop messages and count them. When
> + * syslogd is available again, next log message will flush the stashed
> + * messages and insert a message with drop count. Calls to malloc(9) and
> + * copyin(9) may sleep, protect data structures with rwlock.
> + */
> +
> +#define LOGSTASH_SIZE100
> +struct logstash_messages {
> + char*lgs_buffer;
> + size_t lgs_size;
> +} logstash_messages[LOGSTASH_SIZE];
> +
> +struct logstash_messages *logstash_in = _messages[0];
> +struct logstash_messages *logstash_out = _messages[0];
> +
> +struct rwlock logstash_rwlock = RWLOCK_INITIALIZER("logstash");
> +
> +int logstash_dropped, logstash_error, logstash_pid;
> +
> +void logstash_insert(const char *, size_t, int, pid_t);
> +void logstash_remove(void);
> +int logstash_sendsyslog(struct proc *);
> +
> +static inline int
> +logstash_empty(void)
> +{
> + rw_assert_anylock(_rwlock);
> +
> + return logstash_out->lgs_buffer == NULL;
> +}
> +
> +static inline int
> +logstash_full(void)
> +{
> + rw_assert_anylock(_rwlock);
> +
> + return logstash_out->lgs_buffer != NULL &&
> + logstash_in == logstash_out;
> +}
> +
> +static inline void
> +logstash_increment(struct logstash_messages **msg)
> +{
> + rw_assert_wrlock(_rwlock);
> +
> + KASSERT((*msg) >= _messages[0]);
> + KASSERT((*msg) < _messages[LOGSTASH_SIZE]);
> + (*msg)++;
> + if ((*msg) == _messages[LOGSTASH_SIZE])
> + (*msg) = _messages[0];
> +}
> +
> +void
> +logstash_insert(const char *buf, size_t nbyte, int error, pid_t pid)
> +{
> + rw_enter_write(_rwlock);
> +
> + if (logstash_full()) {
> + if (logstash_dropped == 0) {
> + logstash_error = error;
> + logstash_pid = pid;
> + }
> + logstash_dropped++;
> +
> + rw_exit(_rwlock);
> + return;
> + }
> +
> + logstash_in->lgs_buffer = malloc(nbyte, M_LOG, M_WAITOK);
> + copyin(buf, logstash_in->lgs_buffer, nbyte);
> + logstash_in->lgs_size = nbyte;
> + logstash_increment(_in);
> +
> + rw_exit(_rwlock);
> +}
> +
> +void
> +logstash_remove(void)
> +{
> + rw_assert_wrlock(_rwlock);
> +
> + KASSERT(!logstash_empty());
> + free(logstash_out->lgs_buffer, M_LOG, logstash_out->lgs_size);
> + logstash_out->lgs_buffer = NULL;
> + logstash_increment(_out);
> +
> + /* Insert dropped message in sequence where messages were dropped. */
> + if (logstash_dropped) {
> + size_t l, nbyte;
> + char buf[80];
> +
> + l = snprintf(buf, sizeof(buf),
> + "<%d>sendsyslog: dropped %d message%s, error %d, pid %d",
> + LOG_KERN|LOG_WARNING, logstash_dropped,
> + logstash_dropped == 1 ? "" : "s",
> + logstash_error, logstash_pid);
> + logstash_dropped = 0;
> + logstash_error = 0;
> + logstash_pid = 0;
> +
> + /* Cannot fail, we have just freed a slot. */
> + KASSERT(!logstash_full());
> + nbyte = ulmin(l, sizeof(buf) - 1);
> + logstash_in->lgs_buffer = malloc(nbyte, M_LOG, M_WAITOK);
> + memcpy(logstash_in->lgs_buffer, buf, nbyte);
> + logstash_in->lgs_size = nbyte;
> + logstash_increment(_in);
> + }
> +}
> +
> +int
> +logstash_sendsyslog(struct proc *p)
> +{
> + int error;
> +
> + rw_enter_write(_rwlock);
> +
> + while (logstash_out->lgs_buffer != NULL) {
> + error = dosendsyslog(p, logstash_out->lgs_buffer,
> + logstash_out->lgs_size, 0, UIO_SYSSPACE);
> + if (error) {
> +
