Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
One day I'll write you a newer autonomous back-end.. and mirror propagation signalling / tracker, that is OpenBSD, protocol and CDN independent.. but that is probably not needed for now. It's true, not relying on sysupgrade(8) for a long time before it showed up, and a lot other things, but it should not stop me from suggesting a minor fix here and there if it's going to help others. If it's a time waste, so be it. I'm happy with conveniences and OpenBSD proposed scripts anyway. Hence suggesting the sets enum etc.. I still think the suggestions are not that difficult or bad of ideas, though. If you don't like these, I do not insist. Your call, I just said a couple of words that do not matter much to you, it seems. Didn't mean to upset anyone (this time). And it's vanity, not pride, but those are not my qualities, despite what it looks like from your viewpoint. When I suggest something the next time, it will not be self-deprecating again but might lack the commentaries. I'm not going to speak in the third person, even if you need it for submissions. Now I'll stop reading too. On 9/26/23, Theo de Raadt wrote: > I stopped reading as soon as your pride showed. > > > Eponymous Pseudonym wrote: > >> Right, that's exactly it. >> >> I also found out moments after I posted a white space nuisance, and >> the logic details that are not enough in what I proposed as it's not >> factoring the snapshot independently of -release, and also -current >> and -stable cases are incomplete too, and BEFORE that, there is >> missing information from, and changes needed too, on the master >> "release" preparation side. >> >> So can't solve it as simple as I proposed, and was trying to make it a >> recurring set of state table changes and various "edge" case fixes by >> iterative incremental sequence of "improvements" that would be always >> correct regardless of the failure state, even when the index / >> checksum has been mixed and incompletely overlapped.. but you're >> relieving this for now and that's fine with me. >> >> It took me exactly 3 min to solve it in multiple variants my end and I >> am not being inconvenienced with a tweak per "newvers" change, after >> all the extensive routes of precise tracking of these in my custom >> auto-upgrade routines for many years maybe even more than a decade >> prior to sysupgrade(8) here.. >> >> My main critical point is the omission to enumerate the sets in the >> logic of the sysupgrade(8) script and the gratuitous version detection >> based on kernel string and checksum/index rather than normalised >> (tracked) by newvers state published on the mirror (or in the >> signatures) or carried in the sets versioning information in a >> specialised format like BUILDINFO, whatever you like. >> >> Anyway, it needs a bit of a discussion to which I have not been yet >> invited, so will defer that to your schedule and preferred solution, >> needless to say. Thanks for your qualified feedback as usual. >> >> On 9/26/23, Theo de Raadt wrote: >> > Stuart Henderson wrote: >> > >> >> > This results in failure to upgrade to a valid snapshot on the >> >> > mirrors, >> >> > and users having to wait a new snapshot fanout without mixed sets >> >> > and >> >> > checksum files containing both versions (incompletely the older). >> >> >> >> I do not agree with "be lenient on what you expect thing that >> >> used to be popular belief. So IMHO it is better if sysupgrade fails >> >> when the files in the ftp directory do not match its expectations. >> > >> > There are a few different back-end failure modes going on here. It is >> > more than a mirroring problem. The hash files contain a mix of files. >> > I think stuart is right -- we are better off just plain failing. One >> > day I will find time to improve the backend, but that's not now. >> > >> >
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
I stopped reading as soon as your pride showed. Eponymous Pseudonym wrote: > Right, that's exactly it. > > I also found out moments after I posted a white space nuisance, and > the logic details that are not enough in what I proposed as it's not > factoring the snapshot independently of -release, and also -current > and -stable cases are incomplete too, and BEFORE that, there is > missing information from, and changes needed too, on the master > "release" preparation side. > > So can't solve it as simple as I proposed, and was trying to make it a > recurring set of state table changes and various "edge" case fixes by > iterative incremental sequence of "improvements" that would be always > correct regardless of the failure state, even when the index / > checksum has been mixed and incompletely overlapped.. but you're > relieving this for now and that's fine with me. > > It took me exactly 3 min to solve it in multiple variants my end and I > am not being inconvenienced with a tweak per "newvers" change, after > all the extensive routes of precise tracking of these in my custom > auto-upgrade routines for many years maybe even more than a decade > prior to sysupgrade(8) here.. > > My main critical point is the omission to enumerate the sets in the > logic of the sysupgrade(8) script and the gratuitous version detection > based on kernel string and checksum/index rather than normalised > (tracked) by newvers state published on the mirror (or in the > signatures) or carried in the sets versioning information in a > specialised format like BUILDINFO, whatever you like. > > Anyway, it needs a bit of a discussion to which I have not been yet > invited, so will defer that to your schedule and preferred solution, > needless to say. Thanks for your qualified feedback as usual. > > On 9/26/23, Theo de Raadt wrote: > > Stuart Henderson wrote: > > > >> > This results in failure to upgrade to a valid snapshot on the mirrors, > >> > and users having to wait a new snapshot fanout without mixed sets and > >> > checksum files containing both versions (incompletely the older). > >> > >> I do not agree with "be lenient on what you expect thing that > >> used to be popular belief. So IMHO it is better if sysupgrade fails > >> when the files in the ftp directory do not match its expectations. > > > > There are a few different back-end failure modes going on here. It is > > more than a mirroring problem. The hash files contain a mix of files. > > I think stuart is right -- we are better off just plain failing. One > > day I will find time to improve the backend, but that's not now. > > >
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
Right, that's exactly it. I also found out moments after I posted a white space nuisance, and the logic details that are not enough in what I proposed as it's not factoring the snapshot independently of -release, and also -current and -stable cases are incomplete too, and BEFORE that, there is missing information from, and changes needed too, on the master "release" preparation side. So can't solve it as simple as I proposed, and was trying to make it a recurring set of state table changes and various "edge" case fixes by iterative incremental sequence of "improvements" that would be always correct regardless of the failure state, even when the index / checksum has been mixed and incompletely overlapped.. but you're relieving this for now and that's fine with me. It took me exactly 3 min to solve it in multiple variants my end and I am not being inconvenienced with a tweak per "newvers" change, after all the extensive routes of precise tracking of these in my custom auto-upgrade routines for many years maybe even more than a decade prior to sysupgrade(8) here.. My main critical point is the omission to enumerate the sets in the logic of the sysupgrade(8) script and the gratuitous version detection based on kernel string and checksum/index rather than normalised (tracked) by newvers state published on the mirror (or in the signatures) or carried in the sets versioning information in a specialised format like BUILDINFO, whatever you like. Anyway, it needs a bit of a discussion to which I have not been yet invited, so will defer that to your schedule and preferred solution, needless to say. Thanks for your qualified feedback as usual. On 9/26/23, Theo de Raadt wrote: > Stuart Henderson wrote: > >> > This results in failure to upgrade to a valid snapshot on the mirrors, >> > and users having to wait a new snapshot fanout without mixed sets and >> > checksum files containing both versions (incompletely the older). >> >> I do not agree with "be lenient on what you expect thing that >> used to be popular belief. So IMHO it is better if sysupgrade fails >> when the files in the ftp directory do not match its expectations. > > There are a few different back-end failure modes going on here. It is > more than a mirroring problem. The hash files contain a mix of files. > I think stuart is right -- we are better off just plain failing. One > day I will find time to improve the backend, but that's not now. >
rpki-client: constraining Trust Anchors
Dear all,
Two weeks ago AFRINIC was placed under receivership by the Supreme Court
of Mauritius [1]. This event prompted me to rethink our trust and threat
model and associated risk surface.
The RPKI technology was designed to be versatile and flexible to
accommodate a myriad of real-world deployment scenarios including
multiple trust anchors existing, inter-registry transfers, multiple
transports, and permissionless innovation for signed objects, for
example. All good and well ... but there is a fine print.
Over the years various people have expressed astonishment about each RIR
having issued so-called 'all-resources' (0.0.0.0/0 + ::/0) trust anchor
certificates, but this aspect often is misunderstood: the risk is not
necessarily in the listing of 'all-resources' itself, it is in the RIR
being able to issue an 'all-resources' certificate in the first place.
RPKI trust anchor operators indeed can voluntarily reduce the scope of
subordinate Internet Number Resources, but just as easily increase the
scope of their authority. In other words, a trust anchor cannot truly
constrain itself.
Upon reconsideration on how exactly RPKI hooks into the real world; I
concluded trust anchors do not require unbounded trust in order to
provide constructive services in the realm of BGP routing security.
Some examples: ARIN does not support inter-RIR IPv6 transfers, so it
would not make any sense to see a ROA subordinate to ARIN's trust anchor
covering RIPE-managed IPv6 space. Conversely, it wouldn't make sense
to observe a ROA covering ARIN-managed IPv6 space under APNIC's,
LACNIC's, or RIPE's trust anchor - even if a cryptographically valid
certificate path existed. Along these lines AFRINIC doesn't support
inter-RIR transfers of any kind, and none of the RIRs have authority
over private resources like 10.0.0.0/8.
Theo Buehler and myself have worked diligently over the last 2 weeks to
research RIR transfer policies, untangle the history of the IANA->RIR
and RIR->RIR allocation spaghetti, design & implement a maintainable
constraints mechanism for rpki-client(8), and publicly document the
concept.
https://www.ietf.org/archive/id/draft-snijders-constraining-rpki-trust-anchors-00.html
I believe our approach is feasible and beneficial.
The constraints are simple text files and parsed in a restricted
subprocess. Operators can trivially disable the imposition of
constraints by deleting /etc/rpki/*.constraints, if need be.
I do regret to have to propose this giant diff so close to release, but
here we are. Your help is welcome.
Kind regards,
Job
[1]:
https://mybroadband.co.za/news/internet/507882-africas-internet-registry-placed-under-receivership.html
Index: etc/Makefile
===
RCS file: /cvs/src/etc/Makefile,v
retrieving revision 1.486
diff -u -p -r1.486 Makefile
--- etc/Makefile28 Jun 2022 18:46:00 - 1.486
+++ etc/Makefile26 Sep 2023 14:27:45 -
@@ -157,6 +157,8 @@ distribution-etc-root-var: distrib-dirs
cd rpki; \
${INSTALL} -c -o root -g wheel -m 644 \
afrinic.tal apnic.tal lacnic.tal ripe.tal \
+ arin.constraints afrinic.constraints apnic.constraints \
+ lacnic.constraints ripe.constraints \
${DESTDIR}/etc/rpki
cd examples; \
${INSTALL} -c -o root -g wheel -m 644 ${EXAMPLES} \
Index: etc/changelist
===
RCS file: /cvs/src/etc/changelist,v
retrieving revision 1.137
diff -u -p -r1.137 changelist
--- etc/changelist 19 Sep 2023 15:02:54 - 1.137
+++ etc/changelist 26 Sep 2023 14:27:45 -
@@ -112,10 +112,15 @@
/etc/resolv.conf
/etc/ripd.conf
/etc/rpc
+/etc/rpki/afrinic.constraints
/etc/rpki/afrinic.tal
+/etc/rpki/apnic.constraints
/etc/rpki/apnic.tal
+/etc/rpki/arin.constraints
/etc/rpki/arin.tal
+/etc/rpki/lacnic.constraints
/etc/rpki/lacnic.tal
+/etc/rpki/ripe.constraints
/etc/rpki/ripe.tal
/etc/rpki/skiplist
/etc/sasyncd.conf
Index: etc/rpki/afrinic.constraints
===
RCS file: etc/rpki/afrinic.constraints
diff -N etc/rpki/afrinic.constraints
--- /dev/null 1 Jan 1970 00:00:00 -
+++ etc/rpki/afrinic.constraints26 Sep 2023 14:27:45 -
@@ -0,0 +1,612 @@
+# From https://www.iana.org/assignments/ipv4-address-space/
+allow 41.0.0.0/8
+allow 102.0.0.0/8
+allow 105.0.0.0/8
+allow 154.0.0.0/8
+allow 196.0.0.0/7
+
+# From https://www.iana.org/assignments/ipv6-address-space/
+allow 2001:4200::/23
+allow 2c00::/12
+
+# From https://www.iana.org/assignments/as-numbers/
+allow 36864 - 37887
+allow 327680 - 328703
+allow 328704 - 329727
+
+# Holes
+deny 154.8.48.0 - 154.8.255.255 # APNIC
+deny 154.10.0.0/16# APNIC
+deny 154.33.0.0 - 154.34.255.255 # APNIC
+deny 196.1.1.0/24 # APNIC
+deny 196.1.68.0/24
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
Stuart Henderson wrote: > > This results in failure to upgrade to a valid snapshot on the mirrors, > > and users having to wait a new snapshot fanout without mixed sets and > > checksum files containing both versions (incompletely the older). > > I do not agree with "be lenient on what you expect thing that > used to be popular belief. So IMHO it is better if sysupgrade fails > when the files in the ftp directory do not match its expectations. There are a few different back-end failure modes going on here. It is more than a mirroring problem. The hash files contain a mix of files. I think stuart is right -- we are better off just plain failing. One day I will find time to improve the backend, but that's not now.
Re: [newvers] sysupgrade(8) -release to -beta narrow of sets version
On 2023/09/26 11:04, Eponymous Pseudonym wrote: > There is a small window of mixed version sets at the newvers tagging > from release to beta, likely due to the way currently mirrors are > clearing the previous and syncing during that automatically (not > signalled). There will be another one from beta to release and new > version for snaps too. It's when the actual version number changes (73->74) and going from 7.4-beta to 7.4 for release should be fine. (There is another problem with pkg_add etc once the version number changes to just plain "7.4" but there's no good way around that, the workaround is manual -Dsnap or setting PKG_PATH to explicitly include /snapshots/ in the URL). > At these times sysupgrade(8) picks the old sets from partial > incompletely "deleted" version sets breaking the snapshot upgrades, > instead of the latter correct new version ones which are a complete > sets. > > This results in failure to upgrade to a valid snapshot on the mirrors, > and users having to wait a new snapshot fanout without mixed sets and > checksum files containing both versions (incompletely the older). I do not agree with "be lenient on what you expect thing that used to be popular belief. So IMHO it is better if sysupgrade fails when the files in the ftp directory do not match its expectations.
Re: Please test; midi(4): make midi{read,write}_filtops mp safe
On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote:
> Please test this diff, I have no midi(4) devices.
>
> midi(4) already uses `audio_lock' mutex(9) for filterops, but they are
> still kernel locked. Wipe out old selwakeup API and make them MP safe.
> knote_locked(9) will not grab kernel lock, so call it directly from
> interrupt handlers instead of scheduling software interrupts.
>
Works for me (tested with a usb device), pulling the usb cable
properly wakes up any process waiting for input/output.
ok ratchov@ with the two changes below
> @@ -531,20 +518,8 @@ midiattach(struct device *parent, struct
> }
> #endif
>
> - sc->inbuf.softintr = softintr_establish(IPL_SOFTMIDI,
> - midi_buf_wakeup, >inbuf);
> - if (sc->inbuf.softintr == NULL) {
> - printf("%s: can't establish input softintr\n", DEVNAME(sc));
> - return;
> - }
> -
> - sc->outbuf.softintr = softintr_establish(IPL_SOFTMIDI,
> - midi_buf_wakeup, >outbuf);
> - if (sc->outbuf.softintr == NULL) {
> - printf("%s: can't establish output softintr\n", DEVNAME(sc));
> - softintr_disestablish(sc->inbuf.softintr);
> - return;
> - }
> + klist_init_mutex(>inbuf.klist, _lock);
> + klist_init_mutex(>outbuf.klist, _lock);
>
> sc->hw_if = hwif;
> sc->hw_hdl = hdl;
> @@ -580,27 +555,19 @@ mididetach(struct device *self, int flag
> KERNEL_ASSERT_LOCKED();
> if (sc->flags & FREAD) {
> wakeup(>inbuf.blocking);
> - mtx_enter(_lock);
> - selwakeup(>inbuf.sel);
> - mtx_leave(_lock);
> + knote(>inbuf.klist, 0);
AFAIU, the knote() calls are not necessary because klist_invalidate()
already does the job.
> }
> if (sc->flags & FWRITE) {
> wakeup(>outbuf.blocking);
> - mtx_enter(_lock);
> - selwakeup(>outbuf.sel);
> - mtx_leave(_lock);
> + knote(>outbuf.klist, 0);
> }
> sc->hw_if->close(sc->hw_hdl);
> sc->flags = 0;
> }
>
> - klist_invalidate(>inbuf.sel.si_note);
> - klist_invalidate(>outbuf.sel.si_note);
> + klist_invalidate(>inbuf.klist);
> + klist_invalidate(>outbuf.klist);
>
Could you add two klist_free() calls here? They are simple asserts,
but they'd make the attach/detach functions more midi(4) would look
more like audio(4).
[newvers] sysupgrade(8) -release to -beta narrow of sets version
There is a small window of mixed version sets at the newvers tagging
from release to beta, likely due to the way currently mirrors are
clearing the previous and syncing during that automatically (not
signalled). There will be another one from beta to release and new
version for snaps too.
At these times sysupgrade(8) picks the old sets from partial
incompletely "deleted" version sets breaking the snapshot upgrades,
instead of the latter correct new version ones which are a complete
sets.
This results in failure to upgrade to a valid snapshot on the mirrors,
and users having to wait a new snapshot fanout without mixed sets and
checksum files containing both versions (incompletely the older).
It can be fixed by adjusting the sysupgrade(8) script to narrow the
sets version to the current new version of the snapshot sets.
Here is how it looks on the mirrors during that period:
before (contains one version, normal)
https://ftp.hostserver.de/archive/2023-09-18-0105/snapshots/i386/
during (two versions, older incomplete, picked and failing)
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/i386/
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/i386/index.txt
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/i386/SHA256
after (one version again, normal)
https://ftp.hostserver.de/archive/2023-09-20-0105/snapshots/i386/
during (same as i386, probably affecting other arches too)
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/amd64/
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/amd64/index.txt
https://ftp.hostserver.de/archive/2023-09-19-0105/snapshots/amd64/SHA256
The script uses SHA256 for picking up the list of sets, and it picks
the incomplete one and fails. With the change it picks the complete
one and finishes successfully. I have not tested on newvers rolling
locally, so try to proof-reason about it yourself too.
Let me know if you need clarification around the beta release / snap
conditions or the logic of the proposed change, adjust accordingly /
test as you will. Pardon the primitive diff-ing by hand.
Here is the proposed narrowing version sets as a unified diff:
$ diff -u $(dirname `which sysupgrade`)/sysupgrade{,.nv}
--- /usr/sbin/sysupgrade Tue Sep 26 13:35:56 2023
+++ /usr/sbin/sysupgrade.nv Tue Sep 26 13:36:14 2023
@@ -121,7 +121,11 @@
if $RELEASE && [[ ${_KERNV[1]} == '-beta' ]]; then
NEXT_VERSION=${_KERNV[0]}
else
+if ! $SNAP; then
NEXT_VERSION=$(echo ${_KERNV[0]} + 0.1 | bc)
+else
+ NEXT_VERSION=${_KERNV[0]}
+fi
fi
if $SNAP; then
@@ -136,9 +140,12 @@
echo "Fetching from ${URL}"
unpriv -f SHA256.sig ftp -N sysupgrade -Vmo SHA256.sig ${URL}SHA256.sig
-_KEY=openbsd-${_KERNV[0]%.*}${_KERNV[0]#*.}-base.pub
-_NEXTKEY=openbsd-${NEXT_VERSION%.*}${NEXT_VERSION#*.}-base.pub
+_KV=${_KERNV[0]%.*}${_KERNV[0]#*.}
+_NV=${NEXT_VERSION%.*}${NEXT_VERSION#*.}
+_KEY=openbsd-${_KV}-base.pub
+_NEXTKEY=openbsd-${_NV}-base.pub
+
read _LINE
Re: Please test; midi(4): make midi{read,write}_filtops mp safe
On Tue, Sep 26, 2023 at 10:37:29AM +0200, Alexandre Ratchov wrote:
> On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote:
> > Please test this diff, I have no midi(4) devices.
> >
> > midi(4) already uses `audio_lock' mutex(9) for filterops, but they are
> > still kernel locked. Wipe out old selwakeup API and make them MP safe.
> > knote_locked(9) will not grab kernel lock, so call it directly from
> > interrupt handlers instead of scheduling software interrupts.
> >
>
> Works for me (tested with a usb device), pulling the usb cable
> properly wakes up any process waiting for input/output.
>
> ok ratchov@ with the two changes below
>
Thanks for klist_invalidate() explanation. Updated diff below, will
commit it today later.
Index: sys/dev/midi.c
===
RCS file: /cvs/src/sys/dev/midi.c,v
retrieving revision 1.55
diff -u -p -r1.55 midi.c
--- sys/dev/midi.c 2 Jul 2022 08:50:41 - 1.55
+++ sys/dev/midi.c 26 Sep 2023 10:37:44 -
@@ -31,7 +31,6 @@
#include
#include
-#define IPL_SOFTMIDI IPL_SOFTNET
#define DEVNAME(sc)((sc)->dev.dv_xname)
intmidiopen(dev_t, int, int, struct proc *);
@@ -65,41 +64,38 @@ struct cfdriver midi_cd = {
void filt_midiwdetach(struct knote *);
int filt_midiwrite(struct knote *, long);
+int filt_midimodify(struct kevent *, struct knote *);
+int filt_midiprocess(struct knote *, struct kevent *);
const struct filterops midiwrite_filtops = {
- .f_flags= FILTEROP_ISFD,
+ .f_flags= FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_midiwdetach,
.f_event= filt_midiwrite,
+ .f_modify = filt_midimodify,
+ .f_process = filt_midiprocess,
};
void filt_midirdetach(struct knote *);
int filt_midiread(struct knote *, long);
const struct filterops midiread_filtops = {
- .f_flags= FILTEROP_ISFD,
+ .f_flags= FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_midirdetach,
.f_event= filt_midiread,
+ .f_modify = filt_midimodify,
+ .f_process = filt_midiprocess,
};
void
-midi_buf_wakeup(void *addr)
+midi_buf_wakeup(struct midi_buffer *buf)
{
- struct midi_buffer *buf = addr;
-
if (buf->blocking) {
wakeup(>blocking);
buf->blocking = 0;
}
- /*
-* As long as selwakeup() grabs the KERNEL_LOCK() make sure it is
-* already held here to avoid lock ordering problems with `audio_lock'
-*/
- KERNEL_ASSERT_LOCKED();
- mtx_enter(_lock);
- selwakeup(>sel);
- mtx_leave(_lock);
+ knote_locked(>klist, 0);
}
void
@@ -117,13 +113,7 @@ midi_iintr(void *addr, int data)
MIDIBUF_WRITE(mb, data);
- /*
-* As long as selwakeup() needs to be protected by the
-* KERNEL_LOCK() we have to delay the wakeup to another
-* context to keep the interrupt context KERNEL_LOCK()
-* free.
-*/
- softintr_schedule(sc->inbuf.softintr);
+ midi_buf_wakeup(mb);
}
int
@@ -226,14 +216,7 @@ void
midi_out_stop(struct midi_softc *sc)
{
sc->isbusy = 0;
-
- /*
-* As long as selwakeup() needs to be protected by the
-* KERNEL_LOCK() we have to delay the wakeup to another
-* context to keep the interrupt context KERNEL_LOCK()
-* free.
-*/
- softintr_schedule(sc->outbuf.softintr);
+ midi_buf_wakeup(>outbuf);
}
void
@@ -342,11 +325,11 @@ midikqfilter(dev_t dev, struct knote *kn
error = 0;
switch (kn->kn_filter) {
case EVFILT_READ:
- klist = >inbuf.sel.si_note;
+ klist = >inbuf.klist;
kn->kn_fop = _filtops;
break;
case EVFILT_WRITE:
- klist = >outbuf.sel.si_note;
+ klist = >outbuf.klist;
kn->kn_fop = _filtops;
break;
default:
@@ -355,9 +338,7 @@ midikqfilter(dev_t dev, struct knote *kn
}
kn->kn_hook = (void *)sc;
- mtx_enter(_lock);
- klist_insert_locked(klist, kn);
- mtx_leave(_lock);
+ klist_insert(klist, kn);
done:
device_unref(>dev);
return error;
@@ -368,24 +349,15 @@ filt_midirdetach(struct knote *kn)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- mtx_enter(_lock);
- klist_remove_locked(>inbuf.sel.si_note, kn);
- mtx_leave(_lock);
+ klist_remove(>inbuf.klist, kn);
}
int
filt_midiread(struct knote *kn, long hint)
{
struct midi_softc *sc = (struct midi_softc *)kn->kn_hook;
- int retval;
-
- if ((hint & NOTE_SUBMIT) == 0)
- mtx_enter(_lock);
- retval = !MIDIBUF_ISEMPTY(>inbuf);
- if ((hint & NOTE_SUBMIT) == 0)
-
Re: Please test; midi(4): make midi{read,write}_filtops mp safe
On Mon, Sep 25, 2023 at 04:58:56PM +0300, Vitaliy Makkoveev wrote:
> On Mon, Sep 25, 2023 at 05:39:34AM +, Visa Hankala wrote:
> > On Sun, Sep 24, 2023 at 11:03:54PM +0300, Vitaliy Makkoveev wrote:
> > > Please test this diff, I have no midi(4) devices.
> > >
> > > midi(4) already uses `audio_lock' mutex(9) for filterops, but they are
> > > still kernel locked. Wipe out old selwakeup API and make them MP safe.
> > > knote_locked(9) will not grab kernel lock, so call it directly from
> > > interrupt handlers instead of scheduling software interrupts.
> >
> > https://marc.info/?l=openbsd-tech=167604232828221 has minor takeaways
> > if you pay attention.
> >
>
> The only significant difference is mididetach() where you did not replaced
> selwakeup() with knote(). Did I missed something?
>
> @@ -577,30 +562,20 @@ mididetach(struct device *self, int flag
>* in read/write/ioctl, which return EIO.
>*/
> if (sc->flags) {
> - KERNEL_ASSERT_LOCKED();
> - if (sc->flags & FREAD) {
> + if (sc->flags & FREAD)
> wakeup(>inbuf.blocking);
> - mtx_enter(_lock);
> - selwakeup(>inbuf.sel);
> - mtx_leave(_lock);
> - }
IIRC, klist_invalidate() wakes up the process, so the selwakeup()
section should be removed. Does this sound correct?
This is how audio(4) detaches, BTW.
