Re: Silence vmd rtc_update_rega non-32KHz timebase spam
Ping with complete diff. Thanks.
Brian Conway
diff --git usr.sbin/vmd/mc146818.c usr.sbin/vmd/mc146818.c
index e3599c685..001c1a055 100644
--- usr.sbin/vmd/mc146818.c
+++ usr.sbin/vmd/mc146818.c
@@ -34,7 +34,6 @@
#include "vmd.h"
#include "vmm.h"
-#define MC_DIVIDER_MASK 0xe0
#define MC_RATE_MASK 0xf
#define NVRAM_CENTURY 0x32
@@ -236,10 +235,6 @@ rtc_reschedule_per(void)
static void
rtc_update_rega(uint32_t data)
{
-if ((data & MC_DIVIDER_MASK) != MC_BASE_32_KHz)
-log_warnx("%s: set non-32KHz timebase not supported",
-__func__);
-
rtc.regs[MC_REGA] = data;
if ((rtc.regs[MC_REGA] ^ data) & 0x0f)
vm_pipe_send(_pipe, MC146818_RESCHEDULE_PER);
On Thu, Nov 18, 2021 at 8:02 AM Brian Conway wrote:
>
> Per https://marc.info/?l=openbsd-misc=159113575425726 , mlarkin@
> suggested someone can remove it. It's still pretty spammy at the
> current time for me.
>
> Brian Conway
> Software Engineer, Owner
> RCE Software, LLC
>
> diff --git usr.sbin/vmd/mc146818.c usr.sbin/vmd/mc146818.c
> index e3599c68504..17cf21221e5 100644
> --- usr.sbin/vmd/mc146818.c
> +++ usr.sbin/vmd/mc146818.c
> @@ -236,10 +236,6 @@ rtc_reschedule_per(void)
> static void
> rtc_update_rega(uint32_t data)
> {
> -if ((data & MC_DIVIDER_MASK) != MC_BASE_32_KHz)
> -log_warnx("%s: set non-32KHz timebase not supported",
> -__func__);
> -
> rtc.regs[MC_REGA] = data;
> if ((rtc.regs[MC_REGA] ^ data) & 0x0f)
> vm_pipe_send(_pipe, MC146818_RESCHEDULE_PER);
sndiod: -F does not switch back to preferred device
Following https://www.openbsd.org/faq/faq13.html#usbaudio and reading
sndiod(8)'s
-F device
Specify an alternate device to use. If it doesn't work, the one
given with the last -f or -F options will be used. For
instance,
specifying a USB device following a PCI device allows sndiod to
use the USB one preferably when it's connected and to fall back
to the PCI one when it's disconnected. Alternate devices may be
switched with the server.device control of the sndioctl(1)
utility.
I configured things as follows in order to play audio via USB and
fall back to internal sound if USB is not available:
$ rcctl get sndiod flags
-f rsnd/0 -F rsnd/1
Plugging in an USB headset and restarting sndiod or forcing the device
with `sndioctl server.device=1' then plays sound via USB.
Unplugging the device makes playback fall back to internal sound.
But plugging USB back in does not prefer USB to internal as I'd expect
now. I am currently resorting to the following hotplugd(8) script to
always select the USB sound device whenever I plug it in:
#!/bin/ksh
set -Cefu -o pipefail
readonly DEVCLASS=$1 DEVNAME=$2
typeset -i devid
case "${DEVCLASS}-${DEVNAME}" in
0-audio*) # switch sndio(4) to USB headset when plugging it in
devid=${DEVNAME#audio}
sndioctl server.device=${devid}
;;
esac
I'd expect sndiod to *always* use USB whenever possible.
Is this uni-directional behaviour of sndiod intentional/by-design?
If so, can we clarify the manual?
Re: net write in pcb hash
On Wed, Dec 08, 2021 at 03:28:34PM -0300, Martin Pieuchot wrote:
> On 04/12/21(Sat) 01:02, Alexander Bluhm wrote:
> > Hi,
> >
> > As I want a read-only net lock for forwarding, all paths should be
> > checked for the correct net lock and asserts. I found code in
> > in_pcbhashlookup() where reading the PCB table results in a write
> > to optimize the cache.
> >
> > Porperly protecting PCB hashes is out of scope for parallel forwarding.
> > Can we get away with this hack? Only update the cache when we are
> > in TCP oder UDP stack with the write lock. The access from pf is
> > read-only.
> >
> > NET_WLOCKED() indicates whether we may change data structures.
>
> I recall that we currently do not want to introduce such idiom: change
> the behavior of the code depending on which lock is held by the caller.
>
> Can we instead assert that a write-lock is held before modifying the
> list?
We could also pass down the kind of lock that is used. Goal is
that pf uses shared net lock. TCP and UDP will keep the exclusice
net lock for a while.
Diff gets longer but perhaps a bit clearer what is going on.
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1122
diff -u -p -r1.1122 pf.c
--- net/pf.c7 Jul 2021 18:38:25 - 1.1122
+++ net/pf.c8 Dec 2021 21:16:16 -
@@ -3317,14 +3317,12 @@ pf_socket_lookup(struct pf_pdesc *pd)
sport = pd->hdr.tcp.th_sport;
dport = pd->hdr.tcp.th_dport;
PF_ASSERT_LOCKED();
- NET_ASSERT_LOCKED();
tb =
break;
case IPPROTO_UDP:
sport = pd->hdr.udp.uh_sport;
dport = pd->hdr.udp.uh_dport;
PF_ASSERT_LOCKED();
- NET_ASSERT_LOCKED();
tb =
break;
default:
@@ -3348,22 +3346,24 @@ pf_socket_lookup(struct pf_pdesc *pd)
* Fails when rtable is changed while evaluating the ruleset
* The socket looked up will not match the one hit in the end.
*/
- inp = in_pcbhashlookup(tb, saddr->v4, sport, daddr->v4, dport,
- pd->rdomain);
+ NET_ASSERT_LOCKED();
+ inp = in_pcbhashlookup_wlocked(tb, saddr->v4, sport, daddr->v4,
+ dport, pd->rdomain, 0);
if (inp == NULL) {
- inp = in_pcblookup_listen(tb, daddr->v4, dport,
- NULL, pd->rdomain);
+ inp = in_pcblookup_listen_wlocked(tb, daddr->v4, dport,
+ NULL, pd->rdomain, 0);
if (inp == NULL)
return (-1);
}
break;
#ifdef INET6
case AF_INET6:
- inp = in6_pcbhashlookup(tb, >v6, sport, >v6,
- dport, pd->rdomain);
+ NET_ASSERT_LOCKED();
+ inp = in6_pcbhashlookup_wlocked(tb, >v6, sport,
+ >v6, dport, pd->rdomain, 0);
if (inp == NULL) {
- inp = in6_pcblookup_listen(tb, >v6, dport,
- NULL, pd->rdomain);
+ inp = in6_pcblookup_listen_wlocked(tb, >v6,
+ dport, NULL, pd->rdomain, 0);
if (inp == NULL)
return (-1);
}
Index: netinet/in_pcb.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.256
diff -u -p -r1.256 in_pcb.c
--- netinet/in_pcb.c25 Oct 2021 22:20:47 - 1.256
+++ netinet/in_pcb.c8 Dec 2021 21:16:16 -
@@ -1051,6 +1051,15 @@ in_pcbresize(struct inpcbtable *table, i
intin_pcbnotifymiss = 0;
#endif
+struct inpcb *
+in_pcbhashlookup(struct inpcbtable *table, struct in_addr faddr,
+u_int fport_arg, struct in_addr laddr, u_int lport_arg, u_int rtable)
+{
+ NET_ASSERT_WLOCKED();
+ return in_pcbhashlookup_wlocked(table, faddr ,fport_arg, laddr,
+ lport_arg, rtable, 1);
+}
+
/*
* The in(6)_pcbhashlookup functions are used to locate connected sockets
* quickly:
@@ -1061,8 +1070,9 @@ int in_pcbnotifymiss = 0;
* After those two lookups no other are necessary.
*/
struct inpcb *
-in_pcbhashlookup(struct inpcbtable *table, struct in_addr faddr,
-u_int fport_arg, struct in_addr laddr, u_int lport_arg, u_int rtable)
+in_pcbhashlookup_wlocked(struct inpcbtable *table, struct in_addr faddr,
+u_int fport_arg, struct in_addr laddr, u_int lport_arg, u_int rtable,
+int wlocked)
{
struct inpcbhead *head;
struct inpcb *inp;
@@ -1093,7 +1103,7 @@ in_pcbhashlookup(struct inpcbtable *tabl
}
}
#ifdef DIAGNOSTIC
- if (inp == NULL &&
Re: Do not send SIGHUP to syslogd on wtmp rotation
Comitted, thanks. Although your patch was mangled, so I needed to apply it by hand. Please make sure that you mail is properly formatted. martijn@ On Wed, 2021-12-08 at 13:03 +0300, Антон Касимов wrote: > Hi, > I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp > rotation. > But syslogd does not deal with the wtmp log file so there is no need for > SIGHUP. > > I propose to make slightly changes to default newsyslog.conf file: > > --- /etc/newsyslog.conf Thu Oct 11 22:18:27 2018 > +++ /tmp/newsyslog.conf Wed Dec 8 02:05:10 2021 > @@ -10,7 +10,7 @@ > /var/log/maillog 640 7 *24Z > /var/log/messages 644 5 300 * Z > /var/log/secure 600 7 *168 Z > -/var/log/wtmp 644 7 *$W6D4 B > +/var/log/wtmp 644 7 *$W6D4 B "" > /var/log/xferlog 640 7 250 * Z > /var/log/pflog 600 3 250 * ZB "pkill -HUP -u root -U root -t - > -x pflogd" > /var/www/logs/access.log 644 4 *$W0 Z "pkill -USR1 -u root -U > root -x httpd" > > > -- Forwarded message - > От: Martijn van Duren > Date: ср, 8 дек. 2021 г. в 09:50 > Subject: Re: Proposal for improvement of newsyslog.conf > To: Антон Касимов , > > > On Wed, 2021-12-08 at 02:12 +0300, Антон Касимов wrote: > > Hi, > > I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp > > rotation. > > But syslogd does not deal with the wtmp log file so there is no need for > > SIGHUP. > > > > I propose to make slightly changes to default newsyslog.conf file: > > 13c13 > > < /var/log/wtmp 644 7 *$W6D4 B > > --- > > > /var/log/wtmp 644 7 *$W6D4 B "" > > > > Is misc a proper mailing list, or shall I send this message to bugs? > > > tech@ is usually the best place for this kind of suggestions. > Also make sure that you make your diff unified (diff -u). > > martijn@ > >
Re: kbind(2): push kernel lock down as needed
> On Dec 8, 2021, at 12:24, Martin Pieuchot wrote: > > On 06/12/21(Mon) 14:58, Scott Cheloha wrote: >> On Mon, Dec 06, 2021 at 08:35:15PM +0100, Mark Kettenis wrote: Date: Sun, 5 Dec 2021 18:01:04 -0600 From: Scott Cheloha Two things in sys_kbind() need an explicit kernel lock. First, sigexit(). Second, uvm_map_extract(), because the following call chain panics without it: [...] With this committed we can unlock kbind(2). Thoughts? ok? >>> >>> To be honest, I don't think this makes sense unless you can make the >>> "normal" code path lock free. You're replacing a single >>> KERNEL_LOCK/UNLOCK pair with (potentially) a bunch of them. That may >>> actually make things worse. So I think we need to make >>> uvm_map_extract() mpsafe first. >> >> Unlocking uvm_map_extract() would improve things, yes. > > Yes, please. What's missing? uvm_map_extract() calls uvm_mapent_clone() which calls the pgo_reference function for the new map entry's underlying object if it has one. All of those reference functions require the kernel lock. Your UVM Object Locking patch clears that up.
Re: relayd: small ssl.c cleanup
On Wed, Dec 08, 2021 at 04:59:36AM +0100, Theo Buehler wrote:
> BIO_new_mem_buf has had const since 2018, so this workaround is no
> longer needed.
OK bluhm@
> Index: ssl.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/ssl.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 ssl.c
> --- ssl.c 27 Jan 2021 20:33:05 - 1.35
> +++ ssl.c 8 Dec 2021 03:47:48 -
> @@ -123,16 +123,9 @@ ssl_update_certificate(const uint8_t *ol
> BIO *in, *out = NULL;
> BUF_MEM *bptr = NULL;
> X509*cert = NULL;
> - uint8_t *newcert = NULL, *foo = NULL;
> + uint8_t *newcert = NULL;
>
> - /* XXX BIO_new_mem_buf is not using const so work around this */
> - if ((foo = malloc(oldlen)) == NULL) {
> - log_warn("%s: malloc", __func__);
> - return (NULL);
> - }
> - memcpy(foo, oldcert, oldlen);
> -
> - if ((in = BIO_new_mem_buf(foo, oldlen)) == NULL) {
> + if ((in = BIO_new_mem_buf(oldcert, oldlen)) == NULL) {
> log_warnx("%s: BIO_new_mem_buf failed", __func__);
> goto done;
> }
> @@ -193,7 +186,6 @@ ssl_update_certificate(const uint8_t *ol
> *newlen = bptr->length;
>
> done:
> - free(foo);
> if (in)
> BIO_free(in);
> if (out)
Re: Do not send SIGHUP to syslogd on wtmp rotation
On Wed, Dec 08, 2021 at 01:03:10PM +0300, ?? ?? wrote: > Hi, > I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp > rotation. > But syslogd does not deal with the wtmp log file so there is no need for > SIGHUP. > > I propose to make slightly changes to default newsyslog.conf file: Yes. syslogd reads utmp, but not wtmp. Both are never written by syslogd. OK bluhm@ > --- /etc/newsyslog.conf Thu Oct 11 22:18:27 2018 > +++ /tmp/newsyslog.conf Wed Dec 8 02:05:10 2021 > @@ -10,7 +10,7 @@ > /var/log/maillog 640 7 *24Z > /var/log/messages 644 5 300 * Z > /var/log/secure 600 7 *168 Z > -/var/log/wtmp 644 7 *$W6D4 B > +/var/log/wtmp 644 7 *$W6D4 B "" > /var/log/xferlog 640 7 250 * Z > /var/log/pflog 600 3 250 * ZB "pkill -HUP -u root -U root -t - > -x pflogd" > /var/www/logs/access.log 644 4 *$W0 Z "pkill -USR1 -u root -U > root -x httpd" > > > -- Forwarded message - > : Martijn van Duren > Date: , 8 ??. 2021 ??. ?? 09:50 > Subject: Re: Proposal for improvement of newsyslog.conf > To: ?? ?? , > > > On Wed, 2021-12-08 at 02:12 +0300, ?? ?? wrote: > > Hi, > > I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp > > rotation. > > But syslogd does not deal with the wtmp log file so there is no need for > > SIGHUP. > > > > I propose to make slightly changes to default newsyslog.conf file: > > 13c13 > > < /var/log/wtmp 644 7 *$W6D4 B > > --- > > > /var/log/wtmp 644 7 *$W6D4 B "" > > > > Is misc a proper mailing list, or shall I send this message to bugs? > > > tech@ is usually the best place for this kind of suggestions. > Also make sure that you make your diff unified (diff -u). > > martijn@ > > > -- > ?? ?? / Anton Kasimov
Re: net write in pcb hash
On 04/12/21(Sat) 01:02, Alexander Bluhm wrote:
> Hi,
>
> As I want a read-only net lock for forwarding, all paths should be
> checked for the correct net lock and asserts. I found code in
> in_pcbhashlookup() where reading the PCB table results in a write
> to optimize the cache.
>
> Porperly protecting PCB hashes is out of scope for parallel forwarding.
> Can we get away with this hack? Only update the cache when we are
> in TCP oder UDP stack with the write lock. The access from pf is
> read-only.
>
> NET_WLOCKED() indicates whether we may change data structures.
I recall that we currently do not want to introduce such idiom: change
the behavior of the code depending on which lock is held by the caller.
Can we instead assert that a write-lock is held before modifying the
list?
> Also move the assert from pf to in_pcb where the critical section
> is.
>
> bluhm
>
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1122
> diff -u -p -r1.1122 pf.c
> --- net/pf.c 7 Jul 2021 18:38:25 - 1.1122
> +++ net/pf.c 3 Dec 2021 22:20:32 -
> @@ -3317,14 +3317,12 @@ pf_socket_lookup(struct pf_pdesc *pd)
> sport = pd->hdr.tcp.th_sport;
> dport = pd->hdr.tcp.th_dport;
> PF_ASSERT_LOCKED();
> - NET_ASSERT_LOCKED();
> tb =
> break;
> case IPPROTO_UDP:
> sport = pd->hdr.udp.uh_sport;
> dport = pd->hdr.udp.uh_dport;
> PF_ASSERT_LOCKED();
> - NET_ASSERT_LOCKED();
> tb =
> break;
> default:
> Index: netinet/in_pcb.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.256
> diff -u -p -r1.256 in_pcb.c
> --- netinet/in_pcb.c 25 Oct 2021 22:20:47 - 1.256
> +++ netinet/in_pcb.c 3 Dec 2021 22:20:32 -
> @@ -1069,6 +1069,8 @@ in_pcbhashlookup(struct inpcbtable *tabl
> u_int16_t fport = fport_arg, lport = lport_arg;
> u_int rdomain;
>
> + NET_ASSERT_LOCKED();
> +
> rdomain = rtable_l2(rtable);
> head = in_pcbhash(table, rdomain, , fport, , lport);
> LIST_FOREACH(inp, head, inp_hash) {
> @@ -1085,7 +1087,7 @@ in_pcbhashlookup(struct inpcbtable *tabl
>* repeated accesses are quicker. This is analogous to
>* the historic single-entry PCB cache.
>*/
> - if (inp != LIST_FIRST(head)) {
> + if (NET_WLOCKED() && inp != LIST_FIRST(head)) {
> LIST_REMOVE(inp, inp_hash);
> LIST_INSERT_HEAD(head, inp, inp_hash);
> }
> @@ -1119,6 +1121,8 @@ in_pcblookup_listen(struct inpcbtable *t
> u_int16_t lport = lport_arg;
> u_int rdomain;
>
> + NET_ASSERT_LOCKED();
> +
> key1 =
> key2 = _addr;
> #if NPF > 0
> @@ -1185,7 +1189,7 @@ in_pcblookup_listen(struct inpcbtable *t
>* repeated accesses are quicker. This is analogous to
>* the historic single-entry PCB cache.
>*/
> - if (inp != NULL && inp != LIST_FIRST(head)) {
> + if (NET_WLOCKED() && inp != NULL && inp != LIST_FIRST(head)) {
> LIST_REMOVE(inp, inp_hash);
> LIST_INSERT_HEAD(head, inp, inp_hash);
> }
> Index: sys/systm.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/systm.h,v
> retrieving revision 1.154
> diff -u -p -r1.154 systm.h
> --- sys/systm.h 2 Jun 2021 00:39:25 - 1.154
> +++ sys/systm.h 3 Dec 2021 22:20:32 -
> @@ -344,6 +344,8 @@ extern struct rwlock netlock;
> #define NET_RLOCK_IN_IOCTL()do { rw_enter_read(); } while
> (0)
> #define NET_RUNLOCK_IN_IOCTL() do { rw_exit_read(); } while (0)
>
> +#define NET_WLOCKED() (rw_status() == RW_WRITE)
> +
> #ifdef DIAGNOSTIC
>
> #define NET_ASSERT_UNLOCKED()
> \
>
Re: kbind(2): push kernel lock down as needed
On 06/12/21(Mon) 14:58, Scott Cheloha wrote: > On Mon, Dec 06, 2021 at 08:35:15PM +0100, Mark Kettenis wrote: > > > Date: Sun, 5 Dec 2021 18:01:04 -0600 > > > From: Scott Cheloha > > > > > > Two things in sys_kbind() need an explicit kernel lock. First, > > > sigexit(). Second, uvm_map_extract(), because the following call > > > chain panics without it: > > > > > > [...] > > > > > > With this committed we can unlock kbind(2). > > > > > > Thoughts? ok? > > > > To be honest, I don't think this makes sense unless you can make the > > "normal" code path lock free. You're replacing a single > > KERNEL_LOCK/UNLOCK pair with (potentially) a bunch of them. That may > > actually make things worse. So I think we need to make > > uvm_map_extract() mpsafe first. > > Unlocking uvm_map_extract() would improve things, yes. Yes, please. What's missing?
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 03:26:25PM +0100, Gerhard Roth wrote: > > I don't want to fight for this diff, if you think that it's too naive to > > expect proper reset from unresponsive device - that's fine, I'm ready to > > drop the diff, but who knows how those devices are engineered and trade > > of of not being able to run other watchdogs comparing to possible > > network recovery does look reasonable to me. > > > > I don't blame the idea of revitializing urndis_watchdog(). But that > code has been disabled for more than 10 years. And its quite different > for what all the other watchdog routines of USB network interface > drivers do. Maybe the code needs rethinking. I was looking on other implementations and didn't find any signs of the same protocol logic - with keepalives and reset messages, so this flow is pretty unique for urndis driver, and I currently don't understand how to re-do it to avoid waiting on timeout. It already looks pretty straightforward and complete.
Re: [patch] urndis: uncomment watchdog
On 12/8/21 15:13, Mikhail wrote:
On Wed, Dec 08, 2021 at 02:43:04PM +0100, Gerhard Roth wrote:
Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
messages anymore, but now you hope that it'll still process the
REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
I'd say a usbd_reset_port() might be more effective.
BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
same timeout applies to the reset message.
I think if the device don't ack the keepalive message the driver will
just output an error to the log and return, there should be no second 5
sec timeout:
748 rval = urndis_ctrl_send(sc, keep, sizeof(*keep));
749 free(keep, M_TEMP, sizeof *keep);
750
751 if (rval != RNDIS_STATUS_SUCCESS) {
752 printf("%s: keepalive failed\n", DEVNAME(sc));
753 return rval;
754 }
755
756 if ((hdr = urndis_ctrl_recv(sc)) == NULL) {
757 printf("%s: unable to get keepalive response\n",
DEVNAME(sc));
758 return RNDIS_STATUS_FAILURE;
759 }
As you see it calls urndis_ctrl_recv() which in turn calls
urndis_ctrl_msg() which in turn calls usbd_do_request().
usbd_do_request() calls usbd_do_request_flags() with a timeout
of USBD_DEFAULT_TIMEOUT (which is 5 seconds). And
usbd_do_request_flags() sets up an xfer with the USBD_SYNCHRONOUS
flag. Hence usbd_transfer() will wait for the completion up to
USBD_DEFAULT_TIMEOUT seconds.
It may be that the transfer fails with USBD_IOERROR. In that case
the I/O completes faster.
I don't want to fight for this diff, if you think that it's too naive to
expect proper reset from unresponsive device - that's fine, I'm ready to
drop the diff, but who knows how those devices are engineered and trade
of of not being able to run other watchdogs comparing to possible
network recovery does look reasonable to me.
I don't blame the idea of revitializing urndis_watchdog(). But that
code has been disabled for more than 10 years. And its quite different
for what all the other watchdog routines of USB network interface
drivers do. Maybe the code needs rethinking.
smime.p7s
Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 02:43:04PM +0100, Gerhard Roth wrote:
> Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG
> messages anymore, but now you hope that it'll still process the
> REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking.
> I'd say a usbd_reset_port() might be more effective.
> BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the
> same timeout applies to the reset message.
>
I think if the device don't ack the keepalive message the driver will
just output an error to the log and return, there should be no second 5
sec timeout:
748 rval = urndis_ctrl_send(sc, keep, sizeof(*keep));
749 free(keep, M_TEMP, sizeof *keep);
750
751 if (rval != RNDIS_STATUS_SUCCESS) {
752 printf("%s: keepalive failed\n", DEVNAME(sc));
753 return rval;
754 }
755
756 if ((hdr = urndis_ctrl_recv(sc)) == NULL) {
757 printf("%s: unable to get keepalive response\n",
DEVNAME(sc));
758 return RNDIS_STATUS_FAILURE;
759 }
I don't want to fight for this diff, if you think that it's too naive to
expect proper reset from unresponsive device - that's fine, I'm ready to
drop the diff, but who knows how those devices are engineered and trade
of of not being able to run other watchdogs comparing to possible
network recovery does look reasonable to me.
Re: IPsec TDB locking
ok mvs@
> On 8 Dec 2021, at 16:41, Alexander Bluhm wrote:
>
> On Wed, Dec 08, 2021 at 02:41:47AM +0300, Vitaliy Makkoveev wrote:
>> [IN] looks strange. If this field modified after creation it is
>> mutable. There are cases when such field could modified only once,
>> but it still is atomic.
>
> You are right. tdb_dst and tdb_src are initialized before the TDB
> is linked to the list. They may be modified under the exclusive
> net lock. So from a user perspective, net lock is needed to access
> these fields and [N] is the correct documentation.
>
> ok?
>
> bluhm
>
> Index: net/pfkeyv2.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.226
> diff -u -p -r1.226 pfkeyv2.c
> --- net/pfkeyv2.c 3 Dec 2021 19:04:49 - 1.226
> +++ net/pfkeyv2.c 7 Dec 2021 22:16:20 -
> @@ -800,6 +800,8 @@ pfkeyv2_get(struct tdb *tdb, void **head
> int rval, i;
> void *p;
>
> + NET_ASSERT_LOCKED();
> +
> /* Find how much space we need */
> i = sizeof(struct sadb_sa) + sizeof(struct sadb_lifetime) +
> sizeof(struct sadb_x_counter);
> @@ -2347,6 +2349,8 @@ pfkeyv2_expire(struct tdb *tdb, u_int16_
> struct sadb_msg *smsg;
> int rval = 0;
> int i;
> +
> + NET_ASSERT_LOCKED();
>
> switch (tdb->tdb_sproto) {
> case IPPROTO_AH:
> Index: netinet/ip_ipsp.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
> retrieving revision 1.262
> diff -u -p -r1.262 ip_ipsp.c
> --- netinet/ip_ipsp.c 7 Dec 2021 17:28:46 - 1.262
> +++ netinet/ip_ipsp.c 7 Dec 2021 22:39:59 -
> @@ -243,8 +243,6 @@ reserve_spi(u_int rdomain, u_int32_t ssp
> u_int32_t spi;
> int nums;
>
> - NET_ASSERT_LOCKED();
> -
> /* Don't accept ranges only encompassing reserved SPIs. */
> if (sproto != IPPROTO_IPCOMP &&
> (tspi < sspi || tspi <= SPI_RESERVED_MAX)) {
> @@ -343,6 +341,8 @@ gettdb_dir(u_int rdomain, u_int32_t spi,
> u_int32_t hashval;
> struct tdb *tdbp;
>
> + NET_ASSERT_LOCKED();
> +
> mtx_enter(_sadb_mtx);
> hashval = tdb_hash(spi, dst, proto);
>
> @@ -374,7 +374,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> mtx_enter(_sadb_mtx);
> hashval = tdb_hash(0, src, proto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if (tdbp->tdb_sproto == proto &&
> (spi == 0 || tdbp->tdb_spi == spi) &&
> ((!reverse && tdbp->tdb_rdomain == rdomain) ||
> @@ -384,7 +384,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> !memcmp(>tdb_dst, dst, dst->sa.sa_len)) &&
> !memcmp(>tdb_src, src, src->sa.sa_len))
> break;
> -
> + }
> if (tdbp != NULL) {
> tdb_ref(tdbp);
> mtx_leave(_sadb_mtx);
> @@ -395,7 +395,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> su_null.sa.sa_len = sizeof(struct sockaddr);
> hashval = tdb_hash(0, _null, proto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if (tdbp->tdb_sproto == proto &&
> (spi == 0 || tdbp->tdb_spi == spi) &&
> ((!reverse && tdbp->tdb_rdomain == rdomain) ||
> @@ -405,7 +405,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> !memcmp(>tdb_dst, dst, dst->sa.sa_len)) &&
> tdbp->tdb_src.sa.sa_family == AF_UNSPEC)
> break;
> -
> + }
> tdb_ref(tdbp);
> mtx_leave(_sadb_mtx);
> return tdbp;
> @@ -494,7 +494,7 @@ gettdbbysrc(u_int rdomain, union sockadd
> mtx_enter(_sadb_mtx);
> hashval = tdb_hash(0, src, sproto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if ((tdbp->tdb_sproto == sproto) &&
> (tdbp->tdb_rdomain == rdomain) &&
> ((tdbp->tdb_flags & TDBF_INVALID) == 0) &&
> @@ -504,7 +504,7 @@ gettdbbysrc(u_int rdomain, union sockadd
> continue;
> break;
> }
> -
> + }
> tdb_ref(tdbp);
> mtx_leave(_sadb_mtx);
> return tdbp;
> @@ -900,8 +900,7 @@ tdb_unlink_locked(struct tdb *tdbp)
>
> if (tdbsrc[hashval] == tdbp) {
> tdbsrc[hashval] = tdbp->tdb_snext;
> - }
> - else {
> + } else {
> for (tdbpp = tdbsrc[hashval]; tdbpp != NULL;
> tdbpp = tdbpp->tdb_snext) {
> if (tdbpp->tdb_snext == tdbp) {
> @@ -1031,8 +1030,6 @@ struct tdb *
> tdb_alloc(u_int
Re: IPsec TDB locking
>> [IN] looks strange. If this field modified after creation it is >> mutable. There are cases when such field could modified only once, >> but it still is atomic. It is mistype. I mean "..could modified only once, but it still is mutable" > On 8 Dec 2021, at 08:56, Visa Hankala wrote: > > On Wed, Dec 08, 2021 at 02:41:47AM +0300, Vitaliy Makkoveev wrote: >> [IN] looks strange. If this field modified after creation it is >> mutable. There are cases when such field could modified only once, >> but it still is atomic. >> >> We have cases where we do assignment only once, like `unp_addr’ >> when we bind(2)ing socket and we don’t modify if until socket’s >> destruction. Since we could connect to successfully bind(2)ed >> socket only we could say within unp_connect() that `unp_addr’ >> could be accessed without lock. >> >> We also have the case where we could bind(2) already connected >> socket. I such case we could say `unp_addr’ is atomic because >> this is the pointer assignment which points to read-only data >> and this data immutable until socket’s destruction. >> >> But `unp_addr’ is still mutable. > > Please stop saying that a thing is simply "atomic". > > If one sees "atomic" in a locking annotation, one has no idea what > exactly it means. Nearly everything in the kernel can be seen as > "atomic" by choosing a suitable story. > > Whenever a piece of code accesses shared data without locking, there > has to be a well-defined protocol for the accesses. Otherwise the code > cannot function reliably. > > Locking annotations should at least hint toward the protocol. > Annotation "immutable after creation" or "immutable after setting" > are clearly better than just "atomic". >
Re: [patch] urndis: uncomment watchdog
On 12/8/21 14:31, Mikhail wrote: On Wed, Dec 08, 2021 at 02:10:49PM +0100, Gerhard Roth wrote: Well, there's only one watchdog thread for all of the network interfaces. If it is blocked, not other watchdogs can run. I don't think this is a big loss. On one side - no other watchdogs can run for 5 secs, but on other side - watchdog can potentially recover the network service with automatic reset of urndis device and return network connectivity. Isn't it a fair trade of? Well, the RNDIS device doesn't respond to REMOTE_NDIS_KEEPALIVE_MSG messages anymore, but now you hope that it'll still process the REMOTE_NDIS_RESET_MSG we are sending? Sounds like wishful thinking. I'd say a usbd_reset_port() might be more effective. BTW: I was wrong about the 5 seconds. In fact its 10 seconds since the same timeout applies to the reset message. smime.p7s Description: S/MIME cryptographic signature
Re: IPsec TDB locking
On Wed, Dec 08, 2021 at 02:41:47AM +0300, Vitaliy Makkoveev wrote:
> [IN] looks strange. If this field modified after creation it is
> mutable. There are cases when such field could modified only once,
> but it still is atomic.
You are right. tdb_dst and tdb_src are initialized before the TDB
is linked to the list. They may be modified under the exclusive
net lock. So from a user perspective, net lock is needed to access
these fields and [N] is the correct documentation.
ok?
bluhm
Index: net/pfkeyv2.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.226
diff -u -p -r1.226 pfkeyv2.c
--- net/pfkeyv2.c 3 Dec 2021 19:04:49 - 1.226
+++ net/pfkeyv2.c 7 Dec 2021 22:16:20 -
@@ -800,6 +800,8 @@ pfkeyv2_get(struct tdb *tdb, void **head
int rval, i;
void *p;
+ NET_ASSERT_LOCKED();
+
/* Find how much space we need */
i = sizeof(struct sadb_sa) + sizeof(struct sadb_lifetime) +
sizeof(struct sadb_x_counter);
@@ -2347,6 +2349,8 @@ pfkeyv2_expire(struct tdb *tdb, u_int16_
struct sadb_msg *smsg;
int rval = 0;
int i;
+
+ NET_ASSERT_LOCKED();
switch (tdb->tdb_sproto) {
case IPPROTO_AH:
Index: netinet/ip_ipsp.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.262
diff -u -p -r1.262 ip_ipsp.c
--- netinet/ip_ipsp.c 7 Dec 2021 17:28:46 - 1.262
+++ netinet/ip_ipsp.c 7 Dec 2021 22:39:59 -
@@ -243,8 +243,6 @@ reserve_spi(u_int rdomain, u_int32_t ssp
u_int32_t spi;
int nums;
- NET_ASSERT_LOCKED();
-
/* Don't accept ranges only encompassing reserved SPIs. */
if (sproto != IPPROTO_IPCOMP &&
(tspi < sspi || tspi <= SPI_RESERVED_MAX)) {
@@ -343,6 +341,8 @@ gettdb_dir(u_int rdomain, u_int32_t spi,
u_int32_t hashval;
struct tdb *tdbp;
+ NET_ASSERT_LOCKED();
+
mtx_enter(_sadb_mtx);
hashval = tdb_hash(spi, dst, proto);
@@ -374,7 +374,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
mtx_enter(_sadb_mtx);
hashval = tdb_hash(0, src, proto);
- for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
+ for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
if (tdbp->tdb_sproto == proto &&
(spi == 0 || tdbp->tdb_spi == spi) &&
((!reverse && tdbp->tdb_rdomain == rdomain) ||
@@ -384,7 +384,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
!memcmp(>tdb_dst, dst, dst->sa.sa_len)) &&
!memcmp(>tdb_src, src, src->sa.sa_len))
break;
-
+ }
if (tdbp != NULL) {
tdb_ref(tdbp);
mtx_leave(_sadb_mtx);
@@ -395,7 +395,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
su_null.sa.sa_len = sizeof(struct sockaddr);
hashval = tdb_hash(0, _null, proto);
- for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
+ for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
if (tdbp->tdb_sproto == proto &&
(spi == 0 || tdbp->tdb_spi == spi) &&
((!reverse && tdbp->tdb_rdomain == rdomain) ||
@@ -405,7 +405,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
!memcmp(>tdb_dst, dst, dst->sa.sa_len)) &&
tdbp->tdb_src.sa.sa_family == AF_UNSPEC)
break;
-
+ }
tdb_ref(tdbp);
mtx_leave(_sadb_mtx);
return tdbp;
@@ -494,7 +494,7 @@ gettdbbysrc(u_int rdomain, union sockadd
mtx_enter(_sadb_mtx);
hashval = tdb_hash(0, src, sproto);
- for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
+ for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
if ((tdbp->tdb_sproto == sproto) &&
(tdbp->tdb_rdomain == rdomain) &&
((tdbp->tdb_flags & TDBF_INVALID) == 0) &&
@@ -504,7 +504,7 @@ gettdbbysrc(u_int rdomain, union sockadd
continue;
break;
}
-
+ }
tdb_ref(tdbp);
mtx_leave(_sadb_mtx);
return tdbp;
@@ -900,8 +900,7 @@ tdb_unlink_locked(struct tdb *tdbp)
if (tdbsrc[hashval] == tdbp) {
tdbsrc[hashval] = tdbp->tdb_snext;
- }
- else {
+ } else {
for (tdbpp = tdbsrc[hashval]; tdbpp != NULL;
tdbpp = tdbpp->tdb_snext) {
if (tdbpp->tdb_snext == tdbp) {
@@ -1031,8 +1030,6 @@ struct tdb *
tdb_alloc(u_int rdomain)
{
struct tdb *tdbp;
-
- NET_ASSERT_LOCKED();
tdbp = pool_get(_pool, PR_WAITOK | PR_ZERO);
Index: netinet/ip_ipsp.h
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 02:10:49PM +0100, Gerhard Roth wrote: > Well, there's only one watchdog thread for all of the > network interfaces. If it is blocked, not other watchdogs > can run. I don't think this is a big loss. On one side - no other watchdogs can run for 5 secs, but on other side - watchdog can potentially recover the network service with automatic reset of urndis device and return network connectivity. Isn't it a fair trade of?
Re: [patch] urndis: uncomment watchdog
On 12/8/21 14:08, Mikhail wrote: On Wed, Dec 08, 2021 at 10:39:15AM +0100, Gerhard Roth wrote: urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. That means if the device stopped responding, the if_watchdog_thread will be blocked for 5 seconds. Not sure if that's a good idea. Hello, I think this behavior is like it is supposed to work. What are drawbacks of having this keepalive thread being blocked while waiting the answer for keepalive? - in case of timeout we will have error message in the logs and user will be able to handle the problem manually. Well, there's only one watchdog thread for all of the network interfaces. If it is blocked, not other watchdogs can run. smime.p7s Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Wed, Dec 08, 2021 at 10:39:15AM +0100, Gerhard Roth wrote: > urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS > keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. > That means if the device stopped responding, the if_watchdog_thread > will be blocked for 5 seconds. Not sure if that's a good idea. Hello, I think this behavior is like it is supposed to work. What are drawbacks of having this keepalive thread being blocked while waiting the answer for keepalive? - in case of timeout we will have error message in the logs and user will be able to handle the problem manually.
Do not send SIGHUP to syslogd on wtmp rotation
Hi, I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp rotation. But syslogd does not deal with the wtmp log file so there is no need for SIGHUP. I propose to make slightly changes to default newsyslog.conf file: --- /etc/newsyslog.conf Thu Oct 11 22:18:27 2018 +++ /tmp/newsyslog.conf Wed Dec 8 02:05:10 2021 @@ -10,7 +10,7 @@ /var/log/maillog 640 7 *24Z /var/log/messages 644 5 300 * Z /var/log/secure 600 7 *168 Z -/var/log/wtmp 644 7 *$W6D4 B +/var/log/wtmp 644 7 *$W6D4 B "" /var/log/xferlog 640 7 250 * Z /var/log/pflog 600 3 250 * ZB "pkill -HUP -u root -U root -t - -x pflogd" /var/www/logs/access.log 644 4 *$W0 Z "pkill -USR1 -u root -U root -x httpd" -- Forwarded message - От: Martijn van Duren Date: ср, 8 дек. 2021 г. в 09:50 Subject: Re: Proposal for improvement of newsyslog.conf To: Антон Касимов , On Wed, 2021-12-08 at 02:12 +0300, Антон Касимов wrote: > Hi, > I've noticed that newsyslog sends SIGHUP to syslogd on /var/log/wtmp > rotation. > But syslogd does not deal with the wtmp log file so there is no need for > SIGHUP. > > I propose to make slightly changes to default newsyslog.conf file: > 13c13 > < /var/log/wtmp 644 7 *$W6D4 B > --- > > /var/log/wtmp 644 7 *$W6D4 B "" > > Is misc a proper mailing list, or shall I send this message to bugs? > tech@ is usually the best place for this kind of suggestions. Also make sure that you make your diff unified (diff -u). martijn@ -- Антон Касимов / Anton Kasimov
Re: [patch] urndis: uncomment watchdog
On 12/8/21 10:26, Mikhail wrote: On Tue, Nov 30, 2021 at 09:40:35PM +0300, Mikhail wrote: Currently watchdog functionality for urndis driver is disabled (commented), I've tested it and it works properly - reset messages are correctly sent and cmplt packets are received according to RNDIS spec (I patched the driver to forcedly send reset message and act on it with device reset every 5 seconds). I suggest to uncomment it. The hardware is Megafon 4G МM200-1: urndis0 at uhub3 port 2 configuration 1 interface 0 "Qualcomm MM200-1" rev 2.00/3.18 addr 5 Tests, comments or objections? Ping. Commenting was introduced by mk@ in 61cec9357e4f with the following note: At some point we probably want to use the watchdog functionality but the current code is completely untested so disable it entirely rather than enabling it this close to release. I've tested it and it works, maybe there will be a committer who can enable the watchdog? - the functionality is widely used in other drivers and seem to be useful in general. Hi, urndis_watchdog() calls urndis_ctrl_keepalive() which sends an RNDIS keepalive msg and then waits for the reply with USBD_DEFAULT_TIMEOUT. That means if the device stopped responding, the if_watchdog_thread will be blocked for 5 seconds. Not sure if that's a good idea. Gerhard smime.p7s Description: S/MIME cryptographic signature
Re: [patch] urndis: uncomment watchdog
On Tue, Nov 30, 2021 at 09:40:35PM +0300, Mikhail wrote: > Currently watchdog functionality for urndis driver is disabled > (commented), I've tested it and it works properly - reset messages are > correctly sent and cmplt packets are received according to RNDIS spec (I > patched the driver to forcedly send reset message and act on it with > device reset every 5 seconds). I suggest to uncomment it. > > The hardware is Megafon 4G МM200-1: > > urndis0 at uhub3 port 2 configuration 1 interface 0 "Qualcomm MM200-1" rev > 2.00/3.18 addr 5 > > Tests, comments or objections? Ping. Commenting was introduced by mk@ in 61cec9357e4f with the following note: > At some point we probably want to use the watchdog functionality but the > current code is completely untested so disable it entirely rather than > enabling it this close to release. I've tested it and it works, maybe there will be a committer who can enable the watchdog? - the functionality is widely used in other drivers and seem to be useful in general.
