Re: [External] : Re: make 'set skip on ...' dynamic
Hello,
> > updated diff is attached.
>
> One comment below but this diff is OK claudio@
>
> > thanks and
> > regards
> > sashan
> >
> > 8<---8<---8<--8<
> > void
> > pfi_xcommit(void)
> > {
> > - struct pfi_kif *p;
> > + struct pfi_kif *p, *gkif;
> > + struct ifg_list *g;
> > + struct ifnet*ifp;
> > + size_t n;
> >
> > - RB_FOREACH(p, pfi_ifhead, _ifs)
> > + RB_FOREACH(p, pfi_ifhead, _ifs) {
> > p->pfik_flags = p->pfik_flags_new;
> > + n = strlen(p->pfik_name);
> > + ifp = p->pfik_ifp;
> > + /*
> > +* if kif is backed by existing interface, then we must use
> > +* skip flags found in groups. We use pfik_flags_new, otherwise
> > +* we would need to do two RB_FOREACH() passes: the first to
> > +* commit group changes the second to commit flag changes for
> > +* interfaces.
> > +*/
> > + if (isdigit(p->pfik_name[n - 1]) && ifp != NULL)
>
> No other code in pf_if.c is checking both pfik_name and ifp != NULL in
> similar situations. I think you should skip the isdigit() check here.
> If there is a real interface connected to a pfi_kif than it should be updated.
> Lets not introduce more complexity here.
>
> > + TAILQ_FOREACH(g, >if_groups, ifgl_next) {
> > + gkif =
> > + (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
> > + KASSERT(gkif != NULL);
> > + p->pfik_flags |= gkif->pfik_flags_new;
> > + }
> > + }
yes, there is no reason to keep isdigit() check. if `p` happens to
represent interface group, then ifp is NULL (p->pfik_ifp == NULL
for interface groups).
thanks and
regards
sashan
Re: parallel ip forwarding
On Fri, Dec 24, 2021 at 12:50:23PM +0100, Alexander Bluhm wrote:
> On Fri, Dec 24, 2021 at 04:16:28PM +0900, YASUOKA Masahiko wrote:
> > > - npppd l2pt ipsecflowinfo is not MP safe
> >
> > Does this mean the things we are discussing on the "Fix
> > ipsp_spd_lookup() for transport mode" thread? I wonder if there is
> > another issue.
>
> In this mail thread I was concerned about things might get worse.
>
> Currently I see these problems:
>
> tdb_free() will be called with a shared netlock. From there
> ipsp_ids_free() is called.
>
> if (--ids->id_refcount > 0)
> return;
>
> This ref count needs to be atomic.
>
> if (LIST_EMPTY(_ids_gc_list))
> timeout_add_sec(_ids_gc_timeout, 1);
> LIST_INSERT_HEAD(_ids_gc_list, ids, id_gc_list);
>
> And some mutex should protect ipsp_ids_gc_list.
>
> bluhm
>
The diff below adds `ipsec_flows_mtx' mutex(9) to protect `ipsp_ids_*'
list and trees. ipsp_ids_lookup() returns `ids' with bumped reference
counter.
Index: sys/netinet/ip_ipsp.c
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.267
diff -u -p -r1.267 ip_ipsp.c
--- sys/netinet/ip_ipsp.c 20 Dec 2021 15:59:09 - 1.267
+++ sys/netinet/ip_ipsp.c 25 Dec 2021 18:34:22 -
@@ -47,6 +47,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -84,6 +86,13 @@ void tdb_hashstats(void);
do { } while (0)
#endif
+/*
+ * Locks used to protect global data and struct members:
+ * F ipsec_flows_mtx
+ */
+
+struct mutex ipsec_flows_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
+
inttdb_rehash(void);
void tdb_timeout(void *);
void tdb_firstuse(void *);
@@ -98,16 +107,16 @@ int ipsec_ids_idle = 100; /* keep free
struct pool tdb_pool;
/* Protected by the NET_LOCK(). */
-u_int32_t ipsec_ids_next_flow = 1; /* may not be zero */
-struct ipsec_ids_tree ipsec_ids_tree;
-struct ipsec_ids_flows ipsec_ids_flows;
+u_int32_t ipsec_ids_next_flow = 1; /* [F] may not be zero */
+struct ipsec_ids_tree ipsec_ids_tree; /* [F] */
+struct ipsec_ids_flows ipsec_ids_flows;/* [F] */
struct ipsec_policy_head ipsec_policy_head =
TAILQ_HEAD_INITIALIZER(ipsec_policy_head);
void ipsp_ids_gc(void *);
LIST_HEAD(, ipsec_ids) ipsp_ids_gc_list =
-LIST_HEAD_INITIALIZER(ipsp_ids_gc_list);
+LIST_HEAD_INITIALIZER(ipsp_ids_gc_list); /* [F] */
struct timeout ipsp_ids_gc_timeout =
TIMEOUT_INITIALIZER_FLAGS(ipsp_ids_gc, NULL, TIMEOUT_PROC);
@@ -1191,21 +1200,25 @@ ipsp_ids_insert(struct ipsec_ids *ids)
struct ipsec_ids *found;
u_int32_t start_flow;
- NET_ASSERT_LOCKED();
+ mtx_enter(_flows_mtx);
found = RBT_INSERT(ipsec_ids_tree, _ids_tree, ids);
if (found) {
/* if refcount was zero, then timeout is running */
- if (found->id_refcount++ == 0) {
+ if (atomic_inc_int_nv(>id_refcount) == 1) {
LIST_REMOVE(found, id_gc_list);
if (LIST_EMPTY(_ids_gc_list))
timeout_del(_ids_gc_timeout);
}
+ mtx_leave (_flows_mtx);
DPRINTF("ids %p count %d", found, found->id_refcount);
return found;
}
+
+ ids->id_refcount = 1;
ids->id_flow = start_flow = ipsec_ids_next_flow;
+
if (++ipsec_ids_next_flow == 0)
ipsec_ids_next_flow = 1;
while (RBT_INSERT(ipsec_ids_flows, _ids_flows, ids) != NULL) {
@@ -1214,12 +1227,13 @@ ipsp_ids_insert(struct ipsec_ids *ids)
ipsec_ids_next_flow = 1;
if (ipsec_ids_next_flow == start_flow) {
RBT_REMOVE(ipsec_ids_tree, _ids_tree, ids);
+ mtx_leave(_flows_mtx);
DPRINTF("ipsec_ids_next_flow exhausted %u",
- ipsec_ids_next_flow);
+ start_flow);
return NULL;
}
}
- ids->id_refcount = 1;
+ mtx_leave(_flows_mtx);
DPRINTF("new ids %p flow %u", ids, ids->id_flow);
return ids;
}
@@ -1228,11 +1242,16 @@ struct ipsec_ids *
ipsp_ids_lookup(u_int32_t ipsecflowinfo)
{
struct ipsec_idskey;
-
- NET_ASSERT_LOCKED();
+ struct ipsec_ids*ids;
key.id_flow = ipsecflowinfo;
- return RBT_FIND(ipsec_ids_flows, _ids_flows, );
+
+ mtx_enter(_flows_mtx);
+ ids = RBT_FIND(ipsec_ids_flows, _ids_flows, );
+ atomic_inc_int(>id_refcount);
+ mtx_leave(_flows_mtx);
+
+ return ids;
}
/* free ids only from delayed timeout */
@@ -1241,7 +1260,7 @@ ipsp_ids_gc(void *arg)
{
struct ipsec_ids *ids, *tids;
- NET_LOCK();
+ mtx_enter(_flows_mtx);
Re: parallel ip forwarding
On Sat, Dec 25, 2021 at 09:24:07AM +0100, Hrvoje Popovski wrote:
> On 24.12.2021. 0:55, Alexander Bluhm wrote:
> > I think we can remove the ipsec_in_use workaround now. The IPsec
> > path is protected with the kernel lock.
> >
> > There are some issues left:
> > - npppd l2pt ipsecflowinfo is not MP safe
> > - the acquire SA feature is not MP safe
> > - Hrvoje has seen a panic with sasync
> >
> > If you use one of these, the diff below should trigger crashes faster.
> > If you use only regular IPsec or forwarding, I hope it is stable.
>
> Hi,
>
> after hitting sasyncd setup with this diff for some time i've run
> ipsecctl -sa just to see what's going on and box panic
According to ddb output ipsecctl is running in userland and the
panic is triggered by a kernel timeout. Could it be coincidence?
When I run with 4 softnet threads, I see userland starvation. Maybe
both timeout and ipsecctl wait for the exclusive netlock and are
executed shortly after each other.
> r620-1# ipsecctl -sa
> uvm_fault(0x82200c18, 0x417, 0, 2) -> e
> kernel: page fault trap, code=0
> Stopped at pfsync_delete_tdb+0x84: movq%rcx,0x8(%rsi)
> TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> 290490 40316 0 0x3 03 ipsecctl
> 10869 22801 680x10 05 sasyncd
> 504041 13202 680x10 0x801 isakmpd
> 476980 6400 00x10 02 ntpd
> 224100 72648 0 0x14000 0x2004 reaper
> * 75659 10211 0 0x14000 0x42000K softclock
> pfsync_delete_tdb(812e8008) at pfsync_delete_tdb+0x84
> tdb_free(812e8008) at tdb_free+0x67
> tdb_timeout(812e8008) at tdb_timeout+0x7e
> softclock_thread(8000f260) at softclock_thread+0x13b
> end trace frame: 0x0, count: 11
> https://www.openbsd.org/ddb.html describes the minimum info required in
> bug reports. Insufficient info makes it difficult to find and fix bugs.
> ddb{0}>
/home/bluhm/openbsd/cvs/src/sys/net/if_pfsync.c:2519
5f30: 49 8b 86 10 04 00 00mov0x410(%r14),%rax
5f37: 49 8b 8e 18 04 00 00mov0x418(%r14),%rcx
5f3e: 49 8d 94 24 e8 09 00lea0x9e8(%r12),%rdx
5f45: 00
5f46: 48 8d b0 10 04 00 00lea0x410(%rax),%rsi
5f4d: 48 85 c0test %rax,%rax
* 5f50: 48 0f 44 f2 cmove %rdx,%rsi
5f54: 48 89 4e 08 mov%rcx,0x8(%rsi)
5f58: 49 8b 86 10 04 00 00mov0x410(%r14),%rax
5f5f: 49 8b 8e 18 04 00 00mov0x418(%r14),%rcx
5f66: 48 89 01mov%rax,(%rcx)
5f69: 49 c7 86 18 04 00 00movq $0x,0x418(%r14)
5f70: ff ff ff ff
5f74: 49 c7 86 10 04 00 00movq $0x,0x410(%r14)
5f7b: ff ff ff ff
/home/bluhm/openbsd/cvs/src/sys/net/if_pfsync.c:2520
2508 void
2509 pfsync_delete_tdb(struct tdb *t)
2510 {
2511 struct pfsync_softc *sc = pfsyncif;
2512 size_t nlen;
2513
2514 if (sc == NULL || !ISSET(t->tdb_flags, TDBF_PFSYNC))
2515 return;
2516
2517 mtx_enter(>sc_tdb_mtx);
2518
* 2519 TAILQ_REMOVE(>sc_tdb_q, t, tdb_sync_entry);
2520 CLR(t->tdb_flags, TDBF_PFSYNC);
2521
2522 nlen = sizeof(struct pfsync_tdb);
2523 if (TAILQ_EMPTY(>sc_tdb_q))
2524 nlen += sizeof(struct pfsync_subheader);
2525 atomic_sub_long(>sc_len, nlen);
2526
2527 mtx_leave(>sc_tdb_mtx);
2528 }
Most sc_tdb_q are protected by sc_tdb_mtx. But pfsync_drop_snapshot()
and pfsync_sendout() do not have it.
> rsi0x40f
> rcx 0x
> rax 0x
tqe_next and tqe_prev are -1. Looks like a double remove. Can
pfsync_delete_tdb() be called twice? The tdb_refcnt should enforce
that tdb_free() is only called once per TDB.
> flags: d1040
Flags look reasonable. But a problem could be that they are not
protected in if_pfsync. Then set or cleared bits may be lost.
Fix both missing mutexes . Also put the tdb_sync_entry
modification and TDBF_PFSYNC flag in the same sc_tdb_mtx.
I still have no pf sync setup. Hrvoje, can you try this?
ok?
bluhm
Index: net/if_pfsync.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.299
diff -u -p -r1.299 if_pfsync.c
--- net/if_pfsync.c 25 Nov 2021 13:46:02 - 1.299
+++ net/if_pfsync.c 25 Dec 2021 17:46:06 -
@@ -295,7 +295,7 @@ voidpfsync_bulk_update(void *);
void pfsync_bulk_fail(void *);
void pfsync_grab_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
-void pfsync_drop_snapshot(struct pfsync_snapshot *);
+void pfsync_drop_snapshot(struct
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
Hi Claudio, Claudio Jeker wrote on Sat, Dec 25, 2021 at 05:48:53PM +0100: > On Sat, Dec 25, 2021 at 11:36:50AM +0100, Theo Buehler wrote: >> These extensions MUST be marked critical by the sections of the spec >> mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN >> that is extracted and ignored after the FIXME a few lines below each of >> the two hunks. Rather than getting the info from there, it's easier to >> use an API call that checks what was already parsed by d2i_X509(). > I like this a lot. OK claudio@ Merely to avoid a misunderstanding: I have no oponion whatsoever on this rpki-client patch. > I would love to get rid of X509_V_FLAG_IGNORE_CRITICAL and use a callback > to ensure the right extensions are critical but I never managed to > understand how the X509_verify_cert() callback actually works. > Documentation seems to be non-existent. Not exactly non-existent, but admittedly very poor indeed. There is X509_STORE_CTX_set_verify_cb(3), mostly written by Dr. Stephen Henson in 2009. But it does not really explain how the callback is used and instead only provides four examples. The reasons for this being so badly documented are as follows: 1. X.509 PKI Path Validation as specified in RFC 5280 is ridiculously complicated no matter the implementation. 2. Our API design and implementation was inherited from OpenSSL, and everybody knows what that means. 3. Our documentation was inherited from OpenSSL, and everybody knows what that means. 4. Beck@ has embarked on an epic quest to mitigate parts of the consequences of item 2. Consequently, the surrounding airs have been saturated with large amounts of dust for quite some time now. 5. While parts of that dust have arguably started to settle, the air still feels somewhat dusty to me, and i have been fearing that spending the several days of work required to throughly improve this particular part of the documentation might end up as working straight for the waste paper basket once beck@ achieves his next major steps forward. So given that there are literally hundreds of other public API functions in libcrypto that are still completely undocumented (as opposed to the verification callback being documented very badly), i so far refrained from even trying to attack that particular problem... Yours, Ingo
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
On Sat, Dec 25, 2021 at 11:36:50AM +0100, Theo Buehler wrote:
> These extensions MUST be marked critical by the sections of the spec
> mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN
> that is extracted and ignored after the FIXME a few lines below each of
> the two hunks. Rather than getting the info from there, it's easier to
> use an API call that checks what was already parsed by d2i_X509().
I like this a lot. OK claudio@
I would love to get rid of X509_V_FLAG_IGNORE_CRITICAL and use a callback
to ensure the right extensions are critical but I never managed to
understand how the X509_verify_cert() callback actually works.
Documentation seems to be non-existent.
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 cert.c
> --- cert.c5 Nov 2021 10:50:41 - 1.47
> +++ cert.c24 Dec 2021 23:40:55 -
> @@ -588,6 +588,12 @@ sbgp_assysnum(struct parse *p, X509_EXTE
> int dsz, rc = 0, i, ptag;
> long plen;
>
> + if (!X509_EXTENSION_get_critical(ext)) {
> + cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> + "extension not critical", p->fn);
> + goto out;
> + }
> +
> if ((dsz = i2d_X509_EXTENSION(ext, )) < 0) {
> cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> "failed extension parse", p->fn);
> @@ -890,6 +896,12 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
> ASN1_SEQUENCE_ANY *seq = NULL, *sseq = NULL;
> const ASN1_TYPE *t = NULL;
> int i;
> +
> + if (!X509_EXTENSION_get_critical(ext)) {
> + cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
> + "extension not critical", p->fn);
> + goto out;
> + }
>
> if ((dsz = i2d_X509_EXTENSION(ext, )) < 0) {
> cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
>
--
:wq Claudio
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 04:45:11PM +0100, Alexandre Ratchov wrote: > On Sat, Dec 25, 2021 at 02:51:08PM +, Klemens Nanni wrote: > > > > either "devices that are" or "device that is", I'd say the latter. > > > > commited with "device that is". > > BTW, the example of the -F option description seems more appropriate > for the new hot plugging section. Agreed, OK kn.
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 02:51:08PM +, Klemens Nanni wrote: > > either "devices that are" or "device that is", I'd say the latter. > commited with "device that is". BTW, the example of the -F option description seems more appropriate for the new hot plugging section. OK? diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 index b39d021..5226d80 100644 --- a/sndiod/sndiod.8 +++ b/sndiod/sndiod.8 @@ -193,11 +193,6 @@ the one given with the previous or .Fl F option will be used. -For instance, specifying a USB device following a -PCI device allows -.Nm -to use the USB one preferably when it's connected -and to fall back to the PCI one when it's disconnected. .It Fl f Ar device Add this .Xr sndio 7 @@ -539,6 +534,15 @@ Instead, must be used to change the .Va server.device control. +.Pp +For instance, specifying a USB device with +.Fl F +following a PCI device with +.Fl f +allows +.Nm +to use the USB one preferably when it's connected +and to fall back to the PCI one when it's disconnected. .Sh EXAMPLES Start server using default parameters, creating an additional sub-device for output to channels 2:3 only (rear speakers
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 03:46:48PM +0100, Alexandre Ratchov wrote: > On Sat, Dec 25, 2021 at 01:34:19PM +, Klemens Nanni wrote: > > > > This reads OK kn, but I suspect others will object to the hotplugd(8) > > reference. > > > > here's a new one, with attach/detach replaced by plug/unplug and no > ref to hotplug(8) OK kn > diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 > index 1f95097..c31da66 100644 > --- a/sndiod/sndiod.8 > +++ b/sndiod/sndiod.8 > @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory. > Examples: > .Va u8 , s16le , s24le3 , s24le4lsb . > .It Fl F Ar device > -Specify an alternate device to use. > -If it doesn't work, the one given with the previous > +Same as > +.Fl f > +except that if the device is disconnected, > +the one given with the previous > .Fl f > or > .Fl F > @@ -196,11 +198,6 @@ PCI device allows > .Nm > 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 > -.Va server.device > -control of the > -.Xr sndioctl 1 > -utility. > .It Fl f Ar device > Add this > .Xr sndio 7 > @@ -528,6 +525,20 @@ behave normally, while streams connected to > wait for the MMC start signal and start synchronously. > Regardless of which device a stream is connected to, > its playback volume knob is exposed. > +.Sh HOT PLUGGING > +If devices specified with > +.Fl F > +are unavailable when needed or unplugged at runtime, > +.Nm > +will attempt to seamlessly fall back to the last device specified. > +.Pp > +.Nm > +will not automatically switch to specified devices that is plugged at > runtime. either "devices that are" or "device that is", I'd say the latter. > +Instead, > +.Xr sndioctl 1 > +must be used to change the > +.Va server.device > +control. > .Sh EXAMPLES > Start server using default parameters, creating an > additional sub-device for output to channels 2:3 only (rear speakers >
Re: sndiod: -F does not switch back to preferred device
On Sat, Dec 25, 2021 at 01:34:19PM +, Klemens Nanni wrote: > > This reads OK kn, but I suspect others will object to the hotplugd(8) > reference. > here's a new one, with attach/detach replaced by plug/unplug and no ref to hotplug(8) OK? diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 index 1f95097..c31da66 100644 --- a/sndiod/sndiod.8 +++ b/sndiod/sndiod.8 @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory. Examples: .Va u8 , s16le , s24le3 , s24le4lsb . .It Fl F Ar device -Specify an alternate device to use. -If it doesn't work, the one given with the previous +Same as +.Fl f +except that if the device is disconnected, +the one given with the previous .Fl f or .Fl F @@ -196,11 +198,6 @@ PCI device allows .Nm 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 -.Va server.device -control of the -.Xr sndioctl 1 -utility. .It Fl f Ar device Add this .Xr sndio 7 @@ -528,6 +525,20 @@ behave normally, while streams connected to wait for the MMC start signal and start synchronously. Regardless of which device a stream is connected to, its playback volume knob is exposed. +.Sh HOT PLUGGING +If devices specified with +.Fl F +are unavailable when needed or unplugged at runtime, +.Nm +will attempt to seamlessly fall back to the last device specified. +.Pp +.Nm +will not automatically switch to specified devices that is plugged at runtime. +Instead, +.Xr sndioctl 1 +must be used to change the +.Va server.device +control. .Sh EXAMPLES Start server using default parameters, creating an additional sub-device for output to channels 2:3 only (rear speakers
Re: sndiod: -F does not switch back to preferred device
On Mon, Dec 20, 2021 at 10:11:25AM +0100, Alexandre Ratchov wrote: > On Fri, Dec 17, 2021 at 07:11:41PM +, Klemens Nanni wrote: > > > > So we've concluded that the hotplpugging framework needs work, fine. > > > > I'd still like to improve sndiod(8) regarding its own behaviour. > > > > How about the same diff modulo hotplug referencing/examples? > > This helps me understand `-f' and `-F' usage better. > > > > Feedback? Objections? OK? > > > > Index: sndiod.8 > > === > > RCS file: /cvs/src/usr.bin/sndiod/sndiod.8,v > > retrieving revision 1.11 > > diff -u -p -r1.11 sndiod.8 > > --- sndiod.816 Jul 2021 15:05:58 - 1.11 > > +++ sndiod.817 Dec 2021 19:02:21 - > > @@ -185,7 +185,7 @@ Only the signedness and the precision ar > > Examples: > > .Va u8 , s16le , s24le3 , s24le4lsb . > > .It Fl F Ar device > > -Specify an alternate device to use. > > +Specify a preferred device to use on startup. > > If it doesn't work, the one given with the last > > .Fl f > > or > > Recently we dropped the "alternate device name" hack and now -F and -f > are the same, except that -f may fail, while -F tries the next > device. AFAICS, -F will disappear completely soon. Having just one flag in the future sounds good. > What about just saying it's the same as -f? > > (BTW, the paragraph about USB may be moved to the new "HOT PLUGGING" > section, if we go this way) > > > @@ -528,6 +528,24 @@ behave normally, while streams connected > > wait for the MMC start signal and start synchronously. > > Regardless of which device a stream is connected to, > > its playback volume knob is exposed. > > +.Sh HOT PLUGGING > > +.Nm > > +If preferred devices specified with > > +.Fl F > > +are unavailable at startup or detach at runtime, > > +.Nm > > +will attempt to seamlessly fall back to the last device specified. > > +.Pp > > +.Nm > > +will not automatically switch to specified devices that attach at runtime. > > +Instead, > > +.Xr sndioctl 1 > > +must be used to change the > > +.Va server.device > > +control. > > +.Pp > > +.Xr hotplugd 8 > > +can be used to implement seamless switching at runtime. > > .Sh EXAMPLES > > Start server using default parameters, creating an > > additional sub-device for output to channels 2:3 only (rear speakers > > > > > > "At startup" suggests this is when the daemon starts, imho "When a > device is needed" is more accurate (devices are kept closed when not > used, so when a device is needed again, iteration over -Ff restarts). > > With these tweaks, I end-up with the diff below. > > As this is a the "hot plugging" section, would it make sense to use > plugged/unplugged instead of attach/detach words? Both works for me, whatever you prefer. > diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8 > index 910ce12..7dd72e5 100644 > --- a/sndiod/sndiod.8 > +++ b/sndiod/sndiod.8 > @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory. > Examples: > .Va u8 , s16le , s24le3 , s24le4lsb . > .It Fl F Ar device > -Specify an alternate device to use. > -If it doesn't work, the one given with the previous > +Same as > +.Fl f > +except that if the device is disconnected, > +the one given with the previous > .Fl f > or > .Fl F > @@ -196,11 +198,6 @@ PCI device allows > .Nm > 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 > -.Va server.device > -control of the > -.Xr sndioctl 1 > -utility. > .It Fl f Ar device > Add this > .Xr sndio 7 > @@ -528,6 +525,23 @@ behave normally, while streams connected to > wait for the MMC start signal and start synchronously. > Regardless of which device a stream is connected to, > its playback volume knob is exposed. > +.Sh HOT PLUGGING > +If devices specified with > +.Fl F > +are unavailable when needed or detach at runtime, > +.Nm > +will attempt to seamlessly fall back to the last device specified. > +.Pp > +.Nm > +will not automatically switch to specified devices that attach at runtime. > +Instead, > +.Xr sndioctl 1 > +must be used to change the > +.Va server.device > +control. > +.Pp > +.Xr hotplugd 8 > +can be used to implement seamless switching at runtime. > .Sh EXAMPLES > Start server using default parameters, creating an > additional sub-device for output to channels 2:3 only (rear speakers This reads OK kn, but I suspect others will object to the hotplugd(8) reference. Thanks for following up on this.
Re: Revised version of kqueue-based poll(2)
On Tue, Nov 30, 2021 at 03:11:16PM +, Visa Hankala wrote:
> Here is a revised version of kqueue-based poll(2).
>
> The two most important changes to the previous version are:
>
> * Properly handle pollfd arrays where more than one entries refer
> to the same file descriptor.
>
> * Fix poll(2)'s return value.
>
> The first change would be somewhat tricky with plain kqueue since
> an fd-based knote is identified by a (filter, fd) pair. The poll(2)
> translation layer could manage with this by using an auxiliary array
> to map overlapping pollfd entries into a single knote. However, most
> users of poll(2) appear to avoid fd overlaps. So the mapping would be
> a hindrance in the typical case. Also, overlaps might suggest
> suboptimal I/O patterns in the userspace software.
>
> The patch handles fd overlaps by adding a pollid field to the knote
> identifier. This allows the co-existence of multiple knotes with the
> same filter and fd. Of course, the extra identifier is not totally free
> either. Nevertheless, it looks to solve the problem relatively nicely.
>
> I have shuffled the code a bit so that the register and collect
> routines are next to each other.
>
> In addition, the collect routine now warns if kqueue_scan() returns
> an event that does not activate a pollfd entry. Such a situation is
> likely the result of a bug somewhere.
>
> Please test. Once this way of implementing poll(2) has proven reliable
> enough, it should be possible to replace selwakeup() with kernel lock
> free KNOTE() in subsystems that are sufficiently MP-safe.
Here is a revised version of the patch.
A number of fixes to event filter routines have already been committed.
Changes to the previous version:
* Prevent excessive use of kernel memory with poll(2). Now the code
follows how many knotes are registered. If the number looks high,
kqpoll_done() removes knotes eagerly. The condition could include
`num' as well, but I think the heuristic is good enough already.
* Set forcehup anew on each iteration in ppollregister().
Please test.
Index: kern/kern_event.c
===
RCS file: src/sys/kern/kern_event.c,v
retrieving revision 1.178
diff -u -p -r1.178 kern_event.c
--- kern/kern_event.c 25 Dec 2021 11:04:58 - 1.178
+++ kern/kern_event.c 25 Dec 2021 13:09:06 -
@@ -221,6 +221,7 @@ KQRELE(struct kqueue *kq)
}
KASSERT(TAILQ_EMPTY(>kq_head));
+ KASSERT(kq->kq_nknotes == 0);
free(kq->kq_knlist, M_KEVENT, kq->kq_knlistsize *
sizeof(struct knlist));
@@ -451,7 +452,7 @@ filt_proc(struct knote *kn, long hint)
kev.fflags = kn->kn_sfflags;
kev.data = kn->kn_id; /* parent */
kev.udata = kn->kn_udata; /* preserve udata */
- error = kqueue_register(kq, , NULL);
+ error = kqueue_register(kq, , 0, NULL);
if (error)
kn->kn_fflags |= NOTE_TRACKERR;
}
@@ -814,11 +815,15 @@ void
kqpoll_done(unsigned int num)
{
struct proc *p = curproc;
+ struct kqueue *kq = p->p_kq;
KASSERT(p->p_kq != NULL);
KASSERT(p->p_kq_serial + num >= p->p_kq_serial);
p->p_kq_serial += num;
+
+ if (kq->kq_nknotes > 4 * kq->kq_knlistsize)
+ kqueue_purge(p, kq);
}
void
@@ -944,7 +949,7 @@ sys_kevent(struct proc *p, void *v, regi
for (i = 0; i < n; i++) {
kevp = [i];
kevp->flags &= ~EV_SYSFLAGS;
- error = kqueue_register(kq, kevp, p);
+ error = kqueue_register(kq, kevp, 0, p);
if (error || (kevp->flags & EV_RECEIPT)) {
if (SCARG(uap, nevents) != 0) {
kevp->flags = EV_ERROR;
@@ -1040,7 +1045,8 @@ bad:
#endif
int
-kqueue_register(struct kqueue *kq, struct kevent *kev, struct proc *p)
+kqueue_register(struct kqueue *kq, struct kevent *kev, unsigned int pollid,
+struct proc *p)
{
struct filedesc *fdp = kq->kq_fdp;
const struct filterops *fops = NULL;
@@ -1049,6 +1055,8 @@ kqueue_register(struct kqueue *kq, struc
struct knlist *list = NULL;
int active, error = 0;
+ KASSERT(pollid == 0 || (p != NULL && p->p_kq == kq));
+
if (kev->filter < 0) {
if (kev->filter + EVFILT_SYSCOUNT < 0)
return (EINVAL);
@@ -1096,7 +1104,8 @@ again:
if (list != NULL) {
SLIST_FOREACH(kn, list, kn_link) {
if (kev->filter == kn->kn_filter &&
- kev->ident == kn->kn_id) {
+ kev->ident == kn->kn_id &&
+ pollid == kn->kn_pollid) {
if (!knote_acquire(kn, NULL, 0)) {
rpki-client: check ipAddrBlock and autonomousSysNum for criticality
These extensions MUST be marked critical by the sections of the spec
mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN
that is extracted and ignored after the FIXME a few lines below each of
the two hunks. Rather than getting the info from there, it's easier to
use an API call that checks what was already parsed by d2i_X509().
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.47
diff -u -p -r1.47 cert.c
--- cert.c 5 Nov 2021 10:50:41 - 1.47
+++ cert.c 24 Dec 2021 23:40:55 -
@@ -588,6 +588,12 @@ sbgp_assysnum(struct parse *p, X509_EXTE
int dsz, rc = 0, i, ptag;
long plen;
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
if ((dsz = i2d_X509_EXTENSION(ext, )) < 0) {
cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
"failed extension parse", p->fn);
@@ -890,6 +896,12 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
ASN1_SEQUENCE_ANY *seq = NULL, *sseq = NULL;
const ASN1_TYPE *t = NULL;
int i;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
+ "extension not critical", p->fn);
+ goto out;
+ }
if ((dsz = i2d_X509_EXTENSION(ext, )) < 0) {
cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
Re: parallel ip forwarding
On 24.12.2021. 0:55, Alexander Bluhm wrote:
> I think we can remove the ipsec_in_use workaround now. The IPsec
> path is protected with the kernel lock.
>
> There are some issues left:
> - npppd l2pt ipsecflowinfo is not MP safe
> - the acquire SA feature is not MP safe
> - Hrvoje has seen a panic with sasync
>
> If you use one of these, the diff below should trigger crashes faster.
> If you use only regular IPsec or forwarding, I hope it is stable.
Hi,
after hitting sasyncd setup with this diff for some time i've run
ipsecctl -sa just to see what's going on and box panic
r620-1# ipsecctl -sa
uvm_fault(0x82200c18, 0x417, 0, 2) -> e
kernel: page fault trap, code=0
Stopped at pfsync_delete_tdb+0x84: movq%rcx,0x8(%rsi)
TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
290490 40316 0 0x3 03 ipsecctl
10869 22801 680x10 05 sasyncd
504041 13202 680x10 0x801 isakmpd
476980 6400 00x10 02 ntpd
224100 72648 0 0x14000 0x2004 reaper
* 75659 10211 0 0x14000 0x42000K softclock
pfsync_delete_tdb(812e8008) at pfsync_delete_tdb+0x84
tdb_free(812e8008) at tdb_free+0x67
tdb_timeout(812e8008) at tdb_timeout+0x7e
softclock_thread(8000f260) at softclock_thread+0x13b
end trace frame: 0x0, count: 11
https://www.openbsd.org/ddb.html describes the minimum info required in
bug reports. Insufficient info makes it difficult to find and fix bugs.
ddb{0}>
ddb{0}> show reg
rdi 0x4
rsi0x40f
rbp 0x800022c4e390
rbx0
rdx 0x806b39e8
rcx 0x
rax 0x
r8 0x1f
r9 0
r10 0xbaf844ce8eec335d
r11 0xb9858c0c287d2c4d
r12 0x806b3000
r13 0x821c5e60timeout_proc
r14 0x812e8008
r15 0x806b39f8
rip 0x8131a254pfsync_delete_tdb+0x84
cs 0x8
rflags 0x10286__ALIGN_SIZE+0xf286
rsp 0x800022c4e360
ss 0x10
pfsync_delete_tdb+0x84: movq%rcx,0x8(%rsi)
ddb{0}>
ddb{0}> show all tdb
0x812e8008: f9f247f0 192.168.42.100->192.168.42.112:50 #0 000d1040
0x812e8428: 959c114b 192.168.42.112->192.168.42.100:50 #2 1002
0x812e8848: b7eb65bc 192.168.42.113->192.168.42.100:50 #2 1002
0x812e9ce8: 55495192 192.168.42.100->192.168.42.113:50 #3 000d1002
ddb{0}>
ddb{0}> show tdb /f 0x812e8008
tdb at 0x812e8008
hnext: 0x0
dnext: 0x0
snext: 0x0
inext: 0x0
onext: 0x0
xform: 0x0
refcnt: 0
encalgxform: 0x81f36090
authalgxform: 0x81f36380
compalgxform: 0x0
flags: d1040
seq: 8
exp_allocations: 0
soft_allocations: 0
cur_allocations: 0
exp_bytes: 0
soft_bytes: 0
cur_bytes: 1272048336570
exp_timeout: 1200
soft_timeout: 1080
established: 1640372736
first_use: 1640372754
soft_first_use: 0
exp_first_use: 0
last_used: 1640419926
last_marked: 0
cryptoid: 0
tdb_spi: f9f247f0
amxkeylen: 20
emxkeylen: 20
ivlen: 8
sproto: 50
wnd: 16
satype: 2
updates: 158
dst: 192.168.42.112
src: 192.168.42.100
amxkey: 0x0
emxkey: 0x0
rpl: 5256398086
ids: 0x812d8800
ids_swapped: 0
mtu: 0
mtutimeout: 0
udpencap_port: 0
tag: 0
tap: 0
rdomain: 0
rdomain_post: 0
ddb{0}>
PID TID PPIDUID S FLAGS WAIT COMMAND
40316 290490 5050 0 7 0x3ipsecctl
22801 10869 51239 68 70x10sasyncd
51239 39174 1 0 30x80 kqreadsasyncd
13202 504041 43160 68 70x90isakmpd
43160 53413 1 0 30x80 netio isakmpd
5050 12722 1 0 30x10008b sigsusp ksh
64387 345990 1 0 30x100098 poll cron
58819 219224 16427 95 30x100092 kqreadsmtpd
67207 340852 16427103 30x100092 kqreadsmtpd
97983 457473 16427 95 30x100092 kqreadsmtpd
6608 99059 16427 95 30x100092 kqreadsmtpd
91670 313820 16427 95 30x100092 kqreadsmtpd
7571 97218 16427 95 30x100092
