Re: OpenSMTPD docs: forward.5
Jason McIntyre wrote: > On Fri, Feb 12, 2021 at 03:15:47PM +, Larry Hynes wrote: > > > > Index: forward.5 > > === > > RCS file: /cvs/src/usr.sbin/smtpd/forward.5,v > > retrieving revision 1.9 > > diff -u -p -r1.9 forward.5 > > --- forward.5 13 Mar 2015 22:41:54 - 1.9 > > +++ forward.5 12 Feb 2021 15:14:44 - > > @@ -49,10 +49,10 @@ group or world-writable; > > if the home directory is group writeable; > > or if the file is not owned by the user. > > .Pp > > -Users should avoid editing directly the > > +Users should avoid editing the > > .Nm .forward > > -file to prevent delivery failures from occurring if a message > > -arrives while the file is not fully written. > > +file directly, to prevent delivery failures from occurring if > > +a message arrives while the file is not fully written. > > The best option is to use a temporary file and use the > > .Xr mv 1 > > command to atomically overwrite the former > > > > fixed, thanks. > jmc I meant to note that 'mandoc -Tlint' gives the following gripe about forward.5: forward.5:40:13: STYLE: no blank before trailing delimiter: Pq :include: It can be "fixed" by escaping the closing ':' at EOL with '\&' but I don't know if that's correct (or worth it).
Re: Secure by default
How's is this related to tech@? On Sun, Feb 14, 2021 at 12:44:00AM +0530, sivasubramanian muthusamy wrote: > Hello, > > I am an ordinary computer user, installed 6.8 without connecting to > the Internet yet, (a friend and a technical expert recently advised me > in a different context: do not expose your machine to the Internet- > don't know what that means) > > OpenBSD intro says OpenBSD is secure by default. How is it secure by > default for an average user who does not get to ssh, does not use his > computer as a web-server or as a VM host, who does not have to share > screen etc? What ports are open by default and what applications start > by default? > > Before connecting the computer to the Internet, what other steps > should a very ordinary user take? Block a few more ports? Which ones? > > Thank you. > > Sivasubramanian M >
Re: OpenSMTPD docs: forward.5
On Sat, Feb 13, 2021 at 09:47:41PM +, Larry Hynes wrote: > Jason McIntyre wrote: > > On Fri, Feb 12, 2021 at 03:15:47PM +, Larry Hynes wrote: > > > > > > Index: forward.5 > > > === > > > RCS file: /cvs/src/usr.sbin/smtpd/forward.5,v > > > retrieving revision 1.9 > > > diff -u -p -r1.9 forward.5 > > > --- forward.5 13 Mar 2015 22:41:54 - 1.9 > > > +++ forward.5 12 Feb 2021 15:14:44 - > > > @@ -49,10 +49,10 @@ group or world-writable; > > > if the home directory is group writeable; > > > or if the file is not owned by the user. > > > .Pp > > > -Users should avoid editing directly the > > > +Users should avoid editing the > > > .Nm .forward > > > -file to prevent delivery failures from occurring if a message > > > -arrives while the file is not fully written. > > > +file directly, to prevent delivery failures from occurring if > > > +a message arrives while the file is not fully written. > > > The best option is to use a temporary file and use the > > > .Xr mv 1 > > > command to atomically overwrite the former > > > > > > > fixed, thanks. > > jmc > > I meant to note that 'mandoc -Tlint' gives the following gripe about > forward.5: > > forward.5:40:13: STYLE: no blank before trailing delimiter: > Pq :include: > > It can be "fixed" by escaping the closing ':' at EOL with '\&' but I > don't know if that's correct (or worth it). > the idea is to catch punctuation not separated by whitespace, such as: Such as .Ar foo: but in this case the argument to Pq is correct. it's better to have it as a false positive (i think) than to mangle the source to avoid the warning. jmc
Re: Secure by default
I am not sure about your use case but to myself, my computer is useless these days without a connection. On Sat, Feb 13, 2021 at 1:15 PM sivasubramanian muthusamy < 6.inter...@gmail.com> wrote: > Hello, > > I am an ordinary computer user, installed 6.8 without connecting to > the Internet yet, (a friend and a technical expert recently advised me > in a different context: do not expose your machine to the Internet- > don't know what that means) > > OpenBSD intro says OpenBSD is secure by default. How is it secure by > default for an average user who does not get to ssh, does not use his > computer as a web-server or as a VM host, who does not have to share > screen etc? What ports are open by default and what applications start > by default? > > Before connecting the computer to the Internet, what other steps > should a very ordinary user take? Block a few more ports? Which ones? > > Thank you. > > Sivasubramanian M > >
Re: video(4) multiple opens
On Sat, Feb 13, 2021 at 10:45:17AM +0100, Claudio Jeker wrote: > On Sat, Feb 13, 2021 at 10:26:48AM +0100, Marcus Glocker wrote: > > On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote: > > > > > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote: > > > > On Wed, Feb 10 2021, Martin Pieuchot wrote: > > > > > > > > [...] > > > > > > > > > Which fields is the new lock protecting? Why isn't the KERNEL_LOCK() > > > > > enough? > > > > > > > > When I mentioned this potential lack of locking to Marcus, I was > > > > mistaken. Some of the functions in video.c are called from syscalls > > > > that are marked NOLOCK. But now that I have added some printf > > > > debugging, I can see that the kernel lock is held in places where > > > > I didn't expect it to be held (videoioctl, videoread, videoclose...). > > > > So there's probably a layer of locking that I am missing. > > > > > > > > Marcus, sorry for my misleading diff. O:) > > > > > > > > *crawls back under his rock* > > > > > > Just because a syscall is marked NOLOCK does not mean that all of it will > > > run without the KERNEL_LOCK. For example read and write are marked NOLOCK > > > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors. > > > This is why video(4) still runs with the KERNEL_LOCK. > > > The NOLOCK in syscalls.master just tell the kernel to not grab the > > > KERNEL_LOCK right at the start of the syscall. > > > > OK, that wasn't clear to me either! If Jeremie has some space left > > under his rock, I would join :-) > > > > That basically means we need no extra locking in video(4) for this > > implementation, right? For a test I replaced the rw_enter_write() > > with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent > > ioctl programs, and all went fine. > > > > I would come up with a revised diff then for the device owner part. > > Just doing some polishing together with Anton on that. > > For now this is OK. I have some diffs around which will allow unlocked > read and write down to individual devices (I was experimenting with tun(4) > and tap(4)). For ioctl() this will not happen any time soon. ioctl() is a > world of pain. Maybe for devices like video(4) the call path will be > manageable but for network devices it is just mess. The main issue is that > ioctls work on multiple layers and sometimes the calls have layer > violations and often multiple sleep points built into them. Got it. Thanks for the explanation. Here the adapted diff, including a lot of input from anton@. I sprinkled KERNEL_ASSERT_LOCKED() in those functions where we require locking. Not sure if we should keep them in? Index: sys/dev/video.c === RCS file: /cvs/src/sys/dev/video.c,v retrieving revision 1.48 diff -u -p -u -p -r1.48 video.c --- sys/dev/video.c 31 Jan 2021 19:32:01 - 1.48 +++ sys/dev/video.c 13 Feb 2021 09:49:45 - @@ -47,7 +47,8 @@ struct video_softc { struct device *sc_dev;/* hardware device struct */ struct video_hw_if *hw_if; /* hardware interface */ char sc_dying; /* device detached */ - struct process *sc_owner; /* owner process */ + struct process *sc_owner; /* owner process */ + uint8_t sc_open; /* device opened */ int sc_fsize; uint8_t *sc_fbuffer; @@ -69,6 +70,8 @@ int videoactivate(struct device *, int); intvideoprint(void *, const char *); void video_intr(void *); +intvideo_stop(struct video_softc *); +intvideo_claim(struct video_softc *, struct process *); struct cfattach video_ca = { sizeof(struct video_softc), videoprobe, videoattach, @@ -122,6 +125,9 @@ videoopen(dev_t dev, int flags, int fmt, { int unit; struct video_softc *sc; + int error = 0; + + KERNEL_ASSERT_LOCKED(); unit = VIDEOUNIT(dev); if (unit >= video_cd.cd_ndevs || @@ -129,38 +135,40 @@ videoopen(dev_t dev, int flags, int fmt, sc->hw_if == NULL) return (ENXIO); - if (sc->sc_owner != NULL) { - if (sc->sc_owner == p->p_p) - return (0); - else - return (EBUSY); - } else - sc->sc_owner = p->p_p; + if (sc->sc_open) { + DPRINTF(("%s: device already open\n", __func__)); + return (0); + } else { + sc->sc_open = 1; + DPRINTF(("%s: set device to open\n", __func__)); + } sc->sc_vidmode = VIDMODE_NONE; sc->sc_frames_ready = 0; if (sc->hw_if->open != NULL) - return (sc->hw_if->open(sc->hw_hdl, flags, >sc_fsize, - sc->sc_fbuffer, video_intr, sc)); - else - return (0); +
Secure by default
Hello, I am an ordinary computer user, installed 6.8 without connecting to the Internet yet, (a friend and a technical expert recently advised me in a different context: do not expose your machine to the Internet- don't know what that means) OpenBSD intro says OpenBSD is secure by default. How is it secure by default for an average user who does not get to ssh, does not use his computer as a web-server or as a VM host, who does not have to share screen etc? What ports are open by default and what applications start by default? Before connecting the computer to the Internet, what other steps should a very ordinary user take? Block a few more ports? Which ones? Thank you. Sivasubramanian M
Re: smtpd: use libtls
Hi. The diff seems to work for the few people who tested it (thanks). Anyone wants to ok this? Eric. Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.37 diff -u -p -r1.37 ca.c --- ca.c31 Dec 2020 08:27:15 - 1.37 +++ ca.c19 Jan 2021 11:09:54 - @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign EC_KEY *); +static struct dict pkeys; static uint64_t reqid = 0; static void @@ -132,26 +133,29 @@ ca_init(void) struct pki *pki; const char *k; void*iter_dict; + char*hash; log_debug("debug: init private ssl-tree"); + dict_init(); iter_dict = NULL; while (dict_iter(env->sc_pki_dict, _dict, , (void **))) { if (pki->pki_key == NULL) continue; - if ((in = BIO_new_mem_buf(pki->pki_key, - pki->pki_key_len)) == NULL) - fatalx("ca_launch: key"); - - if ((pkey = PEM_read_bio_PrivateKey(in, - NULL, NULL, NULL)) == NULL) - fatalx("ca_launch: PEM"); + in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len); + if (in == NULL) + fatalx("ca_init: key"); + pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL); + if (pkey == NULL) + fatalx("ca_init: PEM"); BIO_free(in); - pki->pki_pkey = pkey; - - freezero(pki->pki_key, pki->pki_key_len); - pki->pki_key = NULL; + hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len); + if (dict_check(, hash)) + EVP_PKEY_free(pkey); + else + dict_xset(, hash, pkey); + free(hash); } } @@ -223,15 +227,15 @@ end: void ca_imsg(struct mproc *p, struct imsg *imsg) { + EVP_PKEY*pkey; RSA *rsa = NULL; EC_KEY *ecdsa = NULL; const void *from = NULL; unsigned char *to = NULL; struct msg m; - const char *pkiname; + const char *hash; size_t flen, tlen, padding; int buf_len; - struct pki *pki; int ret = 0; uint64_t id; int v; @@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im case IMSG_CA_RSA_PRIVDEC: m_msg(, imsg); m_get_id(, ); - m_get_string(, ); + m_get_string(, ); m_get_data(, , ); m_get_size(, ); m_get_size(, ); m_end(); - pki = dict_get(env->sc_pki_dict, pkiname); - if (pki == NULL || pki->pki_pkey == NULL || - (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL) - fatalx("ca_imsg: invalid pki"); + pkey = dict_get(, hash); + if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL) + fatalx("ca_imsg: invalid pkey hash"); if ((to = calloc(1, tlen)) == NULL) fatalx("ca_imsg: calloc"); @@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im case IMSG_CA_ECDSA_SIGN: m_msg(, imsg); m_get_id(, ); - m_get_string(, ); + m_get_string(, ); m_get_data(, , ); m_end(); - pki = dict_get(env->sc_pki_dict, pkiname); - if (pki == NULL || pki->pki_pkey == NULL || - (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL) - fatalx("ca_imsg: invalid pki"); + pkey = dict_get(, hash); + if (pkey == NULL || + (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL) + fatalx("ca_imsg: invalid pkey hash"); buf_len = ECDSA_size(ecdsa); if ((to = calloc(1, buf_len)) == NULL) @@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned struct imsg imsg; int n, done = 0; const void *toptr; - char*pkiname; + char*hash; size_t tlen; struct msg m; uint64_t id; - if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL) + if ((hash = RSA_get_ex_data(rsa, 0)) == NULL) return (0); /* @@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned m_create(p_ca, cmd, 0, 0, -1); reqid++; m_add_id(p_ca, reqid); -
LibreSSL regressions
Hi, A coworker of mine has made tests with LibreSSL [1] and found some regressions. I took his test descriptions and created the following automated regression test. In the repository he described his findings in detail. I kept the numbers of the files and subtests in the target names for now. So, its easier to match it with his files. I don't know how to handle the result of "test-01-ssl". Thats why its just a comment. Someone may have an idea to handle this properly. Any comments, wishes or OK's? bye, Jan [1]: https://github.com/noxxi/libressl-tests Index: regress/lib/libssl/Makefile === RCS file: /cvs/src/regress/lib/libssl/Makefile,v retrieving revision 1.42 diff -u -p -r1.42 Makefile --- regress/lib/libssl/Makefile 14 Oct 2020 15:53:22 - 1.42 +++ regress/lib/libssl/Makefile 12 Feb 2021 19:42:56 - @@ -16,6 +16,7 @@ SUBDIR += tlsext SUBDIR += tlslegacy SUBDIR += key_schedule SUBDIR += unit +SUBDIR += validate # Things that take a long time should go below here. SUBDIR += tlsfuzzer Index: regress/lib/libssl/validate/Makefile === RCS file: regress/lib/libssl/validate/Makefile diff -N regress/lib/libssl/validate/Makefile --- /dev/null 1 Jan 1970 00:00:00 - +++ regress/lib/libssl/validate/Makefile13 Feb 2021 10:50:30 - @@ -0,0 +1,104 @@ +# Tests from: https://github.com/noxxi/libressl-tests + +PERL=perl + +REGRESS_TARGETS = test-00-01-ssl +REGRESS_TARGETS += test-00-02-ssl +REGRESS_TARGETS += test-00-03-ssl +REGRESS_TARGETS += test-00-04-ssl +REGRESS_TARGETS += test-00-05-ssl +REGRESS_TARGETS += test-00-06-ssl +REGRESS_TARGETS += test-01-ssl +REGRESS_TARGETS += test-02-ssl +REGRESS_ROOT_TARGETS = ${REGRESS_TARGETS} +REGRESS_CLEANUP = cleanup-ssl +REGRESS_SETUP =create-libressl-test-certs + +create-libressl-test-certs: create-libressl-test-certs.pl + ${PERL} ${.CURDIR}/$@.pl + +cleanup-ssl: + pkill openssl || true + rm *.pem *.key + +test-00-01-ssl: + # unusual wildcard cert, no CA given to client + # cleanup + pkill openssl || true + sleep 2 + # start client + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \ + -key server-unusual-wildcard.pem -www & \ + timeout=$$(($$(date +%s) + 5)); \ + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ + do test $$(date +%s) -lt $$timeout || exit 1; done + # start client + echo "data" | openssl s_client -verify_return_error -connect 127.0.0.1:4433 \ + | grep "Verify return code: 21" + +test-00-02-ssl: + # unusual wildcard cert, CA given to client + # cleanup + pkill openssl || true + sleep 2 + # start server + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \ + -key server-unusual-wildcard.pem -www & \ + timeout=$$(($$(date +%s) + 5)); \ + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ + do test $$(date +%s) -lt $$timeout || exit 1; done + # start client + echo "data" | openssl s_client -connect 127.0.0.1:4433 -CAfile caR.pem \ + | grep "Verify return code: 0" + +test-00-03-ssl: + # common wildcard cert, no CA given to client + # cleanup + pkill openssl || true + sleep 2 + # start server + ${KTRACE} openssl s_server -cert server-common-wildcard.pem \ + -key server-common-wildcard.pem -www & \ + timeout=$$(($$(date +%s) + 5)); \ + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ + do test $$(date +%s) -lt $$timeout || exit 1; done + # start client + echo "data" | openssl s_client -connect 127.0.0.1:4433 \ + | grep "Verify return code: 21" + +test-00-04-ssl: + # common wildcard cert, CA given to client + # cleanup + pkill openssl || true + sleep 2 + # start server + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \ + -key server-unusual-wildcard.pem -www & \ + timeout=$$(($$(date +%s) + 5)); \ + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ + do test $$(date +%s) -lt $$timeout || exit 1; done + # start client + echo "data" | openssl s_client -connect 127.0.0.1:4433 -CAfile caR.pem \ + | grep "Verify return code: 21" + +test-00-05-ssl: + # openssl verify, unusual wildcard cert + openssl verify -CAfile caR.pem server-unusual-wildcard.pem \ + | grep "server-unusual-wildcard.pem: OK" + +test-00-06-ssl: + # openssl verify, common wildcard cert + openssl verify -CAfile caR.pem server-common-wildcard.pem \ + | grep "server-common-wildcard.pem: OK" + +test-01-ssl: + # Not all chain certificates are sent in s_server + #
Re: LibreSSL regressions
On Sat, Feb 13, 2021 at 11:58:04AM +0100, Jan Klemkow wrote: > Hi, > > A coworker of mine has made tests with LibreSSL [1] and found some > regressions. I took his test descriptions and created the following > automated regression test. In the repository he described his findings > in detail. I kept the numbers of the files and subtests in the target > names for now. So, its easier to match it with his files. > > I don't know how to handle the result of "test-01-ssl". Thats why its > just a comment. Someone may have an idea to handle this properly. > > Any comments, wishes or OK's? First of all thanks for the effort! The perl script and probably also the Makefile should have a license. Please add a check that tests whether the required perl modules are installed (p5-IO-Socket-SSL and p5-Net-SSLeay) and otherwise prints SKIPPED and their names, so I can install them if they're not present. I never remember their exact capitalization and hyphenation... Various comments inline, and a patch for openssl(1) at the end that may simplify some things. > [1]: https://github.com/noxxi/libressl-tests > > Index: regress/lib/libssl/Makefile > === > RCS file: /cvs/src/regress/lib/libssl/Makefile,v > retrieving revision 1.42 > diff -u -p -r1.42 Makefile > --- regress/lib/libssl/Makefile 14 Oct 2020 15:53:22 - 1.42 > +++ regress/lib/libssl/Makefile 12 Feb 2021 19:42:56 - > @@ -16,6 +16,7 @@ SUBDIR += tlsext > SUBDIR += tlslegacy > SUBDIR += key_schedule > SUBDIR += unit > +SUBDIR += validate I'm not too keen on hooking up a test that fails. It breaks my workflow. I use REGRESS_FAIL_EARLY in my environment to spot unexpected failures. Could you add the currently failing tests to REGRESS_EXPECTED_FAILURES? I also don't think it's a libssl regress test. It's either for lib/libcrypto or usr.bin/openssl, probably the latter. > # Things that take a long time should go below here. > SUBDIR += tlsfuzzer > Index: regress/lib/libssl/validate/Makefile > === > RCS file: regress/lib/libssl/validate/Makefile > diff -N regress/lib/libssl/validate/Makefile > --- /dev/null 1 Jan 1970 00:00:00 - > +++ regress/lib/libssl/validate/Makefile 13 Feb 2021 10:50:30 - > @@ -0,0 +1,104 @@ > +# Tests from: https://github.com/noxxi/libressl-tests > + > +PERL=perl Please use consistent spacing. PERL = perl Could you add OPENSSL ?= openssl and use ${OPENSSL} instead of plain openssl throughout the Makefile? This way we can compare with OpenSSL by doing 'make OPENSSL=eopenssl11'. > + > +REGRESS_TARGETS =test-00-01-ssl > +REGRESS_TARGETS += test-00-02-ssl > +REGRESS_TARGETS += test-00-03-ssl > +REGRESS_TARGETS += test-00-04-ssl > +REGRESS_TARGETS += test-00-05-ssl > +REGRESS_TARGETS += test-00-06-ssl > +REGRESS_TARGETS += test-01-ssl > +REGRESS_TARGETS += test-02-ssl > +REGRESS_ROOT_TARGETS = ${REGRESS_TARGETS} Please drop this line. These tests do not require root. > +REGRESS_CLEANUP =cleanup-ssl > +REGRESS_SETUP = create-libressl-test-certs I think this should be REGRESS_SETUP_ONCE. No need to regen the certs for each and every test. > + > +create-libressl-test-certs: create-libressl-test-certs.pl > + ${PERL} ${.CURDIR}/$@.pl > + > +cleanup-ssl: > + pkill openssl || true > + rm *.pem *.key > + > +test-00-01-ssl: > + # unusual wildcard cert, no CA given to client > + # cleanup > + pkill openssl || true > + sleep 2 pkill + sleep 2 is a bit icky. It's not the first time I wanted to have OpenSSL's -naccept option available for testing. See patch below. > + # start client # start server > + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \ > + -key server-unusual-wildcard.pem -www & \ I'd suggest replacing '-www' with '-naccept 1' and... > + timeout=$$(($$(date +%s) + 5)); \ > + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ > + do test $$(date +%s) -lt $$timeout || exit 1; done > + # start client > + echo "data" | openssl s_client -verify_return_error -connect > 127.0.0.1:4433 \ ... do 'echo "Q"' here. Unless you want to use a random port (which may be more appropriate than killing all openssl processes and assuming that 4433 is free), I would just drop '-connect 127.0.0.1:4433'. > + | grep "Verify return code: 21" > + > +test-00-02-ssl: > + # unusual wildcard cert, CA given to client > + # cleanup > + pkill openssl || true > + sleep 2 > + # start server > + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \ > + -key server-unusual-wildcard.pem -www & \ > + timeout=$$(($$(date +%s) + 5)); \ > + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \ > + do test $$(date +%s) -lt $$timeout || exit 1; done > + # start client > +
Re: video(4) multiple opens
On Sat, Feb 13, 2021 at 10:26:48AM +0100, Marcus Glocker wrote: > On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote: > > > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote: > > > On Wed, Feb 10 2021, Martin Pieuchot wrote: > > > > > > [...] > > > > > > > Which fields is the new lock protecting? Why isn't the KERNEL_LOCK() > > > > enough? > > > > > > When I mentioned this potential lack of locking to Marcus, I was > > > mistaken. Some of the functions in video.c are called from syscalls > > > that are marked NOLOCK. But now that I have added some printf > > > debugging, I can see that the kernel lock is held in places where > > > I didn't expect it to be held (videoioctl, videoread, videoclose...). > > > So there's probably a layer of locking that I am missing. > > > > > > Marcus, sorry for my misleading diff. O:) > > > > > > *crawls back under his rock* > > > > Just because a syscall is marked NOLOCK does not mean that all of it will > > run without the KERNEL_LOCK. For example read and write are marked NOLOCK > > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors. > > This is why video(4) still runs with the KERNEL_LOCK. > > The NOLOCK in syscalls.master just tell the kernel to not grab the > > KERNEL_LOCK right at the start of the syscall. > > OK, that wasn't clear to me either! If Jeremie has some space left > under his rock, I would join :-) > > That basically means we need no extra locking in video(4) for this > implementation, right? For a test I replaced the rw_enter_write() > with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent > ioctl programs, and all went fine. > > I would come up with a revised diff then for the device owner part. > Just doing some polishing together with Anton on that. For now this is OK. I have some diffs around which will allow unlocked read and write down to individual devices (I was experimenting with tun(4) and tap(4)). For ioctl() this will not happen any time soon. ioctl() is a world of pain. Maybe for devices like video(4) the call path will be manageable but for network devices it is just mess. The main issue is that ioctls work on multiple layers and sometimes the calls have layer violations and often multiple sleep points built into them. -- :wq Claudio
Re: video(4) multiple opens
On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote: > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote: > > On Wed, Feb 10 2021, Martin Pieuchot wrote: > > > > [...] > > > > > Which fields is the new lock protecting? Why isn't the KERNEL_LOCK() > > > enough? > > > > When I mentioned this potential lack of locking to Marcus, I was > > mistaken. Some of the functions in video.c are called from syscalls > > that are marked NOLOCK. But now that I have added some printf > > debugging, I can see that the kernel lock is held in places where > > I didn't expect it to be held (videoioctl, videoread, videoclose...). > > So there's probably a layer of locking that I am missing. > > > > Marcus, sorry for my misleading diff. O:) > > > > *crawls back under his rock* > > Just because a syscall is marked NOLOCK does not mean that all of it will > run without the KERNEL_LOCK. For example read and write are marked NOLOCK > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors. > This is why video(4) still runs with the KERNEL_LOCK. > The NOLOCK in syscalls.master just tell the kernel to not grab the > KERNEL_LOCK right at the start of the syscall. OK, that wasn't clear to me either! If Jeremie has some space left under his rock, I would join :-) That basically means we need no extra locking in video(4) for this implementation, right? For a test I replaced the rw_enter_write() with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent ioctl programs, and all went fine. I would come up with a revised diff then for the device owner part. Just doing some polishing together with Anton on that.
Re: OpenSMTPD docs: smtp.1
On Fri, Feb 12, 2021 at 05:35:07PM +, Larry Hynes wrote: > Note: the command 'smtp -h' only returns usage. Usage exits with '1' > (I assume this is to satisfy the default case in switch (ch) in > main()), even when called from the correct use of the '-h' flag, > which I don't think is correct. > > Index: smtp.1 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v > retrieving revision 1.8 > diff -u -p -r1.8 smtp.1 > --- smtp.121 Dec 2020 11:48:38 - 1.8 > +++ smtp.112 Feb 2021 17:30:55 - > @@ -58,7 +58,7 @@ Default to the current username. > .It Fl H Ar helo > Define the hostname to advertise (HELO) when establishing the SMTP session. > .It Fl h > -Display version and usage. > +Display usage. > .It Fl n > Do not actually execute a transaction, > just try to establish an SMTP session and quit. > taken. i think that's the last of your current diffs - thanks for the submissions. jmc
Re: OpenSMTPD docs: table.5
On Fri, Feb 12, 2021 at 03:30:11PM +, Larry Hynes wrote: > > Index: table.5 > === > RCS file: /cvs/src/usr.sbin/smtpd/table.5,v > retrieving revision 1.11 > diff -u -p -r1.11 table.5 > --- table.5 11 Aug 2019 13:00:57 - 1.11 > +++ table.5 12 Feb 2021 15:29:25 - > @@ -52,7 +52,7 @@ value3 > .Ed > .Pp > A mapping will be written with each key and value on a line, > -whitespaces separating both columns: > +whitespace separating both columns: > .Bd -literal -offset indent > key1 value1 > key2 value2 > @@ -90,7 +90,7 @@ user3 otheru...@example.com > .Ed > .Pp > In a virtual domain context, the key is either a user part, a full email > -address or a catch all, following selection rules described in > +address or a catch-all, following selection rules described in taken, since makemap.8 uses the same spelling > .Xr smtpd.conf 5 , > and the value is one or many recipients as described in > .Xr aliases 5 : > @@ -164,7 +164,7 @@ They can only be used in the following c > When used as a "from source", the address of a client is compared to the list > of addresses in the table until a match is found. > .Pp > -A netaddr table can contain exact addresses or netmasks, and looks as follow: > +A netaddr table can contain exact addresses or netmasks, as follows: > .Bd -literal -offset indent > 192.168.1.1 > ::1 > @@ -172,7 +172,7 @@ ipv6:::1 > 192.168.1.0/24 > .Ed > .Ss Userinfo tables > -User info tables are used in rule context to specify an alternate user base, > +Userinfo tables are used in a rule context to specify an alternate userbase, taken > mapping virtual users to local system users by UID, GID and home directory. > .Pp > .D1 Ic action Ar name method Cm userbase Pf < Ar table Ns > > @@ -234,15 +234,15 @@ user@*.domain > .Ed > .Ss Addrname tables > Addrname tables are used to map IP addresses to hostnames. > -They can be used in both listen context and relay context: > +They can be used in both listen and relay contexts: > .Bd -unfilled -offset indent > .Ic listen on Ar interface Cm hostnames Pf < Ar table Ns > > .Ic action Ar name Cm relay helo\-src Pf < Ar table Ns > > .Ed > .Pp > -In listen context, the table is used to look up the server name to advertise > +In a listen context, the table is used to look up the server name to > advertise > depending on the local address of the socket on which a connection is > accepted. > -In relay context, the table is used to determine the hostname for the HELO > +In a relay context, the table is used to determine the hostname for the HELO > sequence of the SMTP protocol, depending on the local address used for the > outgoing connection. > .Pp > changes not commented on not committed. jmc
Re: OpenSMTPD docs: smtpd.conf.5
On Fri, Feb 12, 2021 at 03:29:02PM +, Larry Hynes wrote: > > Index: smtpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v > retrieving revision 1.256 > diff -u -p -r1.256 smtpd.conf.5 > --- smtpd.conf.5 27 Jan 2021 14:59:10 - 1.256 > +++ smtpd.conf.5 12 Feb 2021 15:28:04 - > @@ -184,7 +184,7 @@ Specify how long a message may remain in > .It Cm user Ar username > Specify the > .Ar username > -for performing the delivery, to be looked up with > +to perform the delivery, looked up with > .Xr getpwnam 3 . > .Pp > This is used for virtual hosting where a single username > @@ -202,7 +202,7 @@ function. > .Pp > The > .Cm userbase > -does not apply for the > +does not apply to the taken > .Cm user > option. > .It Cm virtual Pf < Ar table Ns > > @@ -322,13 +322,13 @@ If the list contains more than one addre > in such a way that traffic is routed as efficiently as possible. > .El > .It Ic admd Ar authservid > -The Administrative Management Domain this mailserver belongs to. > +The Administrative Management Domain this mail server belongs to. i committed this since every other instance in this page is two words, not one > The authservid will be forwarded to filters using it to identify or mark > authentication-results headers. > If omitted it defaults to the server name. > .It Ic bounce Cm warn-interval Ar delay Op , Ar delay ... > Send warning messages to the envelope sender when temporary delivery > -failures cause a message to remain on the queue for longer than > +failures cause a message to remain in the queue for longer than i think it's probably ok to say "on the queue" but i agree "in the queue" is probably more usual, so i took this > .Ar delay . > Each > .Ar delay > @@ -359,11 +359,11 @@ directive. > .It Ic filter Ar chain-name Ic chain Brq Ar filter-name Op , Ar ... > Register a chain of filters > .Ar chain-name , > -consisting of the filters listed from > +consisting of the filters listed in taken > .Ar filter-name . > -Filters part of a filter chain are executed in order of declaration for > -each phase that they are registered for. > -A filter chain may be used in place of a filter for any directive but > +Filters in a filter chain are executed in order of declaration > +for each phase that they are registered for. > +A filter chain may be used in place of a filter for any directive except > filter chains themselves. taken > .It Ic filter Ar filter-name Ic phase Ar phase-name Ic match Ar conditions > decision > Register a filter > @@ -372,8 +372,9 @@ A > .Ar decision > about what to do with the mail is taken at phase > .Ar phase-name > -when matching > -.Ar conditions . > +when > +.Ar conditions > +are matched. > Phases, matching conditions, and decisions are described in > .Sx MAIL FILTERING , > below. > @@ -387,15 +388,15 @@ backed by the > process. > .It Ic filter Ar filter-name Ic proc-exec Ar command > Register and execute > -.Qq proc > +.Ar command > +as > +.Qq proc-exec > filter > -.Ar filter-name > -from > -.Ar command . > +.Ar filter-name . i don;t understand enough about what is happening here. can you resubmit an updated diff once i have committed your changes to this file, preferrably with an example, and i will try and get some feedback for it. > If > .Ar command > starts with a slash it is executed with an absolute path, > -else it will be run from > +otherwise it will be run from taken > .Dq /usr/local/libexec/smtpd/ . > .It Ic include Qq Ar pathname > Replace this directive with the content of the additional configuration > @@ -404,7 +405,7 @@ file at the absolute > .It Ic listen on Ar interface Oo Ar family Oc Op Ar options > Listen on the > .Ar interface > -for incoming connections, using the same syntax as for > +for incoming connections, using the same syntax as taken > .Xr ifconfig 8 . > The > .Ar interface > @@ -461,7 +462,7 @@ Override the server name for specific ad > The > .Ar names > table contains a mapping of IP addresses to hostnames. > -If the address on which the connection arrives appears in the mapping, > +If the address on which the connection is made appears in the mapping, > the associated hostname is used. > .It Cm mask-src > Omit the > @@ -485,7 +486,7 @@ Listen on the given > instead of the default port 25. > .It Cm proxy-v2 > Support the PROXYv2 protocol, > -rewriting appropriately source address received from proxy. > +appropriately rewriting the source address passed from the proxy. taken > .It Cm received-auth > In > .Dq Received > @@ -555,7 +556,8 @@ matching envelope, and atomically save t > spool for later processing by the respective dispatcher > .Ar name . > .Pp > -The following matching options are supported and can all be negated: > +The following session matching options are supported and can all > +be negated: > .Bl -tag -width Ds > .It Xo > .Op Ic \&! > @@