Re: Reserved address behavior (alternate broadcast and 240/4)
On Thu, May 05, 2022 at 12:58:06PM +1000, Damien Miller wrote:
> On Wed, 4 May 2022, Seth David Schoen wrote:
>
> [snip]
>
> > Anyway, one thing we would like to propose that OpenBSD update is the
> > in_canforward treatment of 240/4 (former class E) addresses. Apparently
> > mainly as a result of proposals in 2008 to make these addresses more
> > usable, most OSes now no longer treat these specially (although they
> > can't yet be allocated as public address space). They have been seeing
> > some considerable use as unofficial private address space. OpenBSD is
> > one of the numerous systems that already permits these addresses to be
> > assigned to an interface and used by a socket, but there's one remaining
> > discrepancy in the in_canforward definition.
> >
> > This has some odd consequences. For instance, if an OpenBSD system
> > has an interface numbered with an address in 240/4, it can initiate
> > and receive TCP connections using that address, and it can ping other
> > hosts using that address, but it won't respond to pings from other
> > hosts. This patch cleans this up:
> >
> >
> > Index: in.c
> > ===
> > RCS file: /cvs/src/sys/netinet/in.c,v
> > retrieving revision 1.173
> > diff -u -p -r1.173 in.c
> > --- in.c28 Mar 2022 16:31:26 - 1.173
> > +++ in.c5 May 2022 01:05:04 -
> > @@ -103,7 +103,7 @@ in_canforward(struct in_addr in)
> > {
> > u_int32_t net;
> >
> > - if (IN_EXPERIMENTAL(in.s_addr) || IN_MULTICAST(in.s_addr))
> > + if (IN_MULTICAST(in.s_addr))
>
> This seems reasonanble. It looks like this is the only use of
> IN_EXPERIMENTAL() in base, so maybe we should remove the definition too?
>
> There is the posibility that it might affect some ports, but I'd argue
> that unless they similarly adapt then they would already be broken (at
> least wrt 240/4 addresses).
>
> Alternetely, we could retain it as "#define IN_EXPERIMENTAL() 0"
>
Agreed, there is also IN_BADCLASS() which is used by the routing daemons.
IN_EXPERIMENTAL and IN_BADCLASS are the same definition.
Looking at debian code search IN_EXPERIMENTAL() is still referenced in a
bunch of packages. So I wonder if your alternative to just make
IN_EXPERIMENTAL never match is the right approach.
OK on the in.c diff
--
:wq Claudio
Re: [External] : divert packet mutex
Hello, On Wed, May 04, 2022 at 10:50:15PM +0200, Alexander Bluhm wrote: > Hi, > > I missed that pf divert packet is not MP safe yet. The problem is > that divert_packet() is called from pf with shared net lock and > sbappendaddr() needs exclusive net lock. > > The direct call from pf in IP layer to divert in protocol layer is > not nice. I not sure how to address that. one way around it would be to dispatch diverted packets to task. we do the same for route-to. > > As a first step clean up divert_packet(): > - the function never returns an error > - call variables so and sin like everywhere else > - use goto bad for error handling > - fix error counter > - introduce mutex and refcounting for inp like in the other pcb > functions > > Divert packet is still not MP safe, I will fix it later. change looks good to me. OK sashan
Re: Reserved address behavior (alternate broadcast and 240/4)
On Wed, 4 May 2022, Seth David Schoen wrote:
[snip]
> Anyway, one thing we would like to propose that OpenBSD update is the
> in_canforward treatment of 240/4 (former class E) addresses. Apparently
> mainly as a result of proposals in 2008 to make these addresses more
> usable, most OSes now no longer treat these specially (although they
> can't yet be allocated as public address space). They have been seeing
> some considerable use as unofficial private address space. OpenBSD is
> one of the numerous systems that already permits these addresses to be
> assigned to an interface and used by a socket, but there's one remaining
> discrepancy in the in_canforward definition.
>
> This has some odd consequences. For instance, if an OpenBSD system
> has an interface numbered with an address in 240/4, it can initiate
> and receive TCP connections using that address, and it can ping other
> hosts using that address, but it won't respond to pings from other
> hosts. This patch cleans this up:
>
>
> Index: in.c
> ===
> RCS file: /cvs/src/sys/netinet/in.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 in.c
> --- in.c 28 Mar 2022 16:31:26 - 1.173
> +++ in.c 5 May 2022 01:05:04 -
> @@ -103,7 +103,7 @@ in_canforward(struct in_addr in)
> {
> u_int32_t net;
>
> - if (IN_EXPERIMENTAL(in.s_addr) || IN_MULTICAST(in.s_addr))
> + if (IN_MULTICAST(in.s_addr))
This seems reasonanble. It looks like this is the only use of
IN_EXPERIMENTAL() in base, so maybe we should remove the definition too?
There is the posibility that it might affect some ports, but I'd argue
that unless they similarly adapt then they would already be broken (at
least wrt 240/4 addresses).
Alternetely, we could retain it as "#define IN_EXPERIMENTAL() 0"
-d
Reserved address behavior (alternate broadcast and 240/4)
Hi!
I'm working on a project with John Gilmore to make it possible for
people to make better use of historically reserved IPv4 address space.
In many cases address types were reserved in the 1980s for purposes
that made sense at the time but that are no longer relevant today.
There are several changes that we've been targeting in different systems.
We were gratified to see that OpenBSD was the first system to adopt one
of those, considerably before the start of our project, by removing the
duplicate broadcast address. This change landed twelve years ago:
> revision 1.59
> date: 2010/01/13 10:30:31; author: henning; state: Exp; lines: +2 -12;
> 4.2BSD had the host parts bit of the address all zero as broadcast address.
> 4.3BSD (anno 1986) supported the host part bits all one for broadcast as
> well, since that's what everybody agreed on and RFC919 (anno 1984) proposed.
> now, roughly a quarter decade later, we can really stop supporting the all
> zero variant. sorry to you guys still running 4.2BSD. ok theo ryan
We have independently been promoting this elsewhere, so we were very
pleasantly surprised to see that it's already long-standing default
behavior in OpenBSD. We recently also got the same behavior adopted
as a default in Linux and FreeBSD (although you may not immediately see
it in Linux distributions as they often use older kernels). I'm happy
to say that our Linux patch interoperated properly with a stock OpenBSD
system. A particularly convenient thing about the all-zeros broadcast
address change is that the existing Internet standards don't allow
distant hosts to make assumptions about netmasks of networks to which
they aren't directly attached. So using the all-zeros form as a host
address already works everywhere outside the LAN environment.
Anyway, one thing we would like to propose that OpenBSD update is the
in_canforward treatment of 240/4 (former class E) addresses. Apparently
mainly as a result of proposals in 2008 to make these addresses more
usable, most OSes now no longer treat these specially (although they
can't yet be allocated as public address space). They have been seeing
some considerable use as unofficial private address space. OpenBSD is
one of the numerous systems that already permits these addresses to be
assigned to an interface and used by a socket, but there's one remaining
discrepancy in the in_canforward definition.
This has some odd consequences. For instance, if an OpenBSD system
has an interface numbered with an address in 240/4, it can initiate
and receive TCP connections using that address, and it can ping other
hosts using that address, but it won't respond to pings from other
hosts. This patch cleans this up:
Index: in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.173
diff -u -p -r1.173 in.c
--- in.c28 Mar 2022 16:31:26 - 1.173
+++ in.c5 May 2022 01:05:04 -
@@ -103,7 +103,7 @@ in_canforward(struct in_addr in)
{
u_int32_t net;
- if (IN_EXPERIMENTAL(in.s_addr) || IN_MULTICAST(in.s_addr))
+ if (IN_MULTICAST(in.s_addr))
return (0);
if (IN_CLASSA(in.s_addr)) {
net = in.s_addr & IN_CLASSA_NET;
Re: ssh: sshkey.c: reduce code duplication
On Wed, 4 May 2022, Martin Vahlensieck wrote: > Hi > > I noticed that sshkey_unshield_private contains a exact duplicate > of the code in private2_check_padding. So by pulling > private2_check_padding up, the code can be reused. Or is there > a reason for this split? Thanks - this has been applied too. -d
Re: ssh: channels.c: Fix comment and add a const
applied
On Wed, 4 May 2022, Martin Vahlensieck wrote:
> Hi
>
> channel_new no longer frees remote_name. So update the comment
> accordingly. As remote_name is not modified, it can be const
> as well.
>
> Best,
>
> Martin
>
> Index: channels.c
> ===
> RCS file: /home/reposync/cvs/src/usr.bin/ssh/channels.c,v
> retrieving revision 1.418
> diff -u -p -r1.418 channels.c
> --- channels.c4 May 2022 07:31:22 - 1.418
> +++ channels.c4 May 2022 19:02:14 -
> @@ -349,12 +349,11 @@ channel_register_fds(struct ssh *ssh, Ch
> }
>
> /*
> - * Allocate a new channel object and set its type and socket. This will cause
> - * remote_name to be freed.
> + * Allocate a new channel object and set its type and socket.
> */
> Channel *
> channel_new(struct ssh *ssh, char *ctype, int type, int rfd, int wfd, int
> efd,
> -u_int window, u_int maxpack, int extusage, char *remote_name, int
> nonblock)
> +u_int window, u_int maxpack, int extusage, const char *remote_name, int
> nonblock)
> {
> struct ssh_channels *sc = ssh->chanctxt;
> u_int i, found;
> Index: channels.h
> ===
> RCS file: /home/reposync/cvs/src/usr.bin/ssh/channels.h,v
> retrieving revision 1.142
> diff -u -p -r1.142 channels.h
> --- channels.h30 Mar 2022 21:10:25 - 1.142
> +++ channels.h6 Apr 2022 20:26:56 -
> @@ -272,7 +272,7 @@ Channel *channel_by_id(struct ssh *, int
> Channel *channel_by_remote_id(struct ssh *, u_int);
> Channel *channel_lookup(struct ssh *, int);
> Channel *channel_new(struct ssh *, char *, int, int, int, int,
> - u_int, u_int, int, char *, int);
> + u_int, u_int, int, const char *, int);
> void channel_set_fds(struct ssh *, int, int, int, int, int,
> int, int, u_int);
> void channel_free(struct ssh *, Channel *);
>
>
Re: ssh: mux.c: mark argument as const
applied - thanks
On Wed, 4 May 2022, Martin Vahlensieck wrote:
> Index: mux.c
> ===
> RCS file: /home/reposync/cvs/src/usr.bin/ssh/mux.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 mux.c
> --- mux.c 11 Jan 2022 01:26:47 - 1.92
> +++ mux.c 13 Jan 2022 16:27:14 -
> @@ -227,7 +227,7 @@ mux_master_control_cleanup_cb(struct ssh
>
> /* Check mux client environment variables before passing them to mux master.
> */
> static int
> -env_permitted(char *env)
> +env_permitted(const char *env)
> {
> int i, ret;
> char name[1024], *cp;
>
>
divert packet mutex
Hi,
I missed that pf divert packet is not MP safe yet. The problem is
that divert_packet() is called from pf with shared net lock and
sbappendaddr() needs exclusive net lock.
The direct call from pf in IP layer to divert in protocol layer is
not nice. I not sure how to address that.
As a first step clean up divert_packet():
- the function never returns an error
- call variables so and sin like everywhere else
- use goto bad for error handling
- fix error counter
- introduce mutex and refcounting for inp like in the other pcb
functions
Divert packet is still not MP safe, I will fix it later.
ok?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1128
diff -u -p -r1.1128 pf.c
--- net/pf.c3 May 2022 13:32:47 - 1.1128
+++ net/pf.c4 May 2022 20:16:40 -
@@ -7403,13 +7403,13 @@ done:
case PF_DIVERT:
switch (pd.af) {
case AF_INET:
- if (!divert_packet(pd.m, pd.dir, r->divert.port))
- pd.m = NULL;
+ divert_packet(pd.m, pd.dir, r->divert.port);
+ pd.m = NULL;
break;
#ifdef INET6
case AF_INET6:
- if (!divert6_packet(pd.m, pd.dir, r->divert.port))
- pd.m = NULL;
+ divert6_packet(pd.m, pd.dir, r->divert.port);
+ pd.m = NULL;
break;
#endif /* INET6 */
}
Index: netinet/ip_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.66
diff -u -p -r1.66 ip_divert.c
--- netinet/ip_divert.c 25 Feb 2022 23:51:03 - 1.66
+++ netinet/ip_divert.c 4 May 2022 18:17:49 -
@@ -171,30 +171,37 @@ fail:
return (error ? error : EINVAL);
}
-int
+void
divert_packet(struct mbuf *m, int dir, u_int16_t divert_port)
{
- struct inpcb *inp;
- struct socket *sa = NULL;
- struct sockaddr_in addr;
+ struct inpcb *inp = NULL;
+ struct socket *so;
+ struct sockaddr_in sin;
- inp = NULL;
divstat_inc(divs_ipackets);
if (m->m_len < sizeof(struct ip) &&
(m = m_pullup(m, sizeof(struct ip))) == NULL) {
divstat_inc(divs_errors);
- return (0);
+ goto bad;
}
+ mtx_enter(&divbtable.inpt_mtx);
TAILQ_FOREACH(inp, &divbtable.inpt_queue, inp_queue) {
- if (inp->inp_lport == divert_port)
- break;
+ if (inp->inp_lport != divert_port)
+ continue;
+ in_pcbref(inp);
+ break;
+ }
+ mtx_leave(&divbtable.inpt_mtx);
+ if (inp == NULL) {
+ divstat_inc(divs_noport);
+ goto bad;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_len = sizeof(addr);
+ memset(&sin, 0, sizeof(sin));
+ sin.sin_family = AF_INET;
+ sin.sin_len = sizeof(sin);
if (dir == PF_IN) {
struct ifaddr *ifa;
@@ -202,37 +209,34 @@ divert_packet(struct mbuf *m, int dir, u
ifp = if_get(m->m_pkthdr.ph_ifidx);
if (ifp == NULL) {
- m_freem(m);
- return (0);
+ divstat_inc(divs_errors);
+ goto bad;
}
TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
if (ifa->ifa_addr->sa_family != AF_INET)
continue;
- addr.sin_addr.s_addr = satosin(
- ifa->ifa_addr)->sin_addr.s_addr;
+ sin.sin_addr = satosin(ifa->ifa_addr)->sin_addr;
break;
}
if_put(ifp);
}
- if (inp) {
- sa = inp->inp_socket;
- if (sbappendaddr(sa, &sa->so_rcv, sintosa(&addr), m, NULL) ==
0) {
- divstat_inc(divs_fullsock);
- m_freem(m);
- return (0);
- } else {
- KERNEL_LOCK();
- sorwakeup(inp->inp_socket);
- KERNEL_UNLOCK();
- }
- }
-
- if (sa == NULL) {
- divstat_inc(divs_noport);
- m_freem(m);
+ so = inp->inp_socket;
+ if (sbappendaddr(so, &so->so_rcv, sintosa(&sin), m, NULL) == 0) {
+ divstat_inc(divs_fullsock);
+ goto bad;
}
- return (0);
+ KERNEL_LOCK();
+ sorwakeup(inp->inp_socket);
+ KERNEL_UNLOCK();
+
+ in_pcbunref(inp);
+ return;
+
+ bad:
+
ssh: sshkey.c: reduce code duplication
Hi
I noticed that sshkey_unshield_private contains a exact duplicate
of the code in private2_check_padding. So by pulling
private2_check_padding up, the code can be reused. Or is there
a reason for this split?
Best,
Martin
P.S.: This diff also removes two trailing spaces while here.
Index: sshkey.c
===
RCS file: /home/reposync/cvs/src/usr.bin/ssh/sshkey.c,v
retrieving revision 1.120
diff -u -p -r1.120 sshkey.c
--- sshkey.c6 Jan 2022 22:05:42 - 1.120
+++ sshkey.c4 May 2022 19:12:16 -
@@ -2079,14 +2079,38 @@ sshkey_shield_private(struct sshkey *k)
return r;
}
+/* Check deterministic padding after private key */
+static int
+private2_check_padding(struct sshbuf *decrypted)
+{
+ u_char pad;
+ size_t i;
+ int r;
+
+ i = 0;
+ while (sshbuf_len(decrypted)) {
+ if ((r = sshbuf_get_u8(decrypted, &pad)) != 0)
+ goto out;
+ if (pad != (++i & 0xff)) {
+ r = SSH_ERR_INVALID_FORMAT;
+ goto out;
+ }
+ }
+ /* success */
+ r = 0;
+ out:
+ explicit_bzero(&pad, sizeof(pad));
+ explicit_bzero(&i, sizeof(i));
+ return r;
+}
+
int
sshkey_unshield_private(struct sshkey *k)
{
struct sshbuf *prvbuf = NULL;
- u_char pad, *cp, keyiv[SSH_DIGEST_MAX_LENGTH];
+ u_char *cp, keyiv[SSH_DIGEST_MAX_LENGTH];
struct sshcipher_ctx *cctx = NULL;
const struct sshcipher *cipher;
- size_t i;
struct sshkey *kswap = NULL, tmp;
int r = SSH_ERR_INTERNAL_ERROR;
@@ -2148,16 +2172,9 @@ sshkey_unshield_private(struct sshkey *k
/* Parse private key */
if ((r = sshkey_private_deserialize(prvbuf, &kswap)) != 0)
goto out;
- /* Check deterministic padding */
- i = 0;
- while (sshbuf_len(prvbuf)) {
- if ((r = sshbuf_get_u8(prvbuf, &pad)) != 0)
- goto out;
- if (pad != (++i & 0xff)) {
- r = SSH_ERR_INVALID_FORMAT;
- goto out;
- }
- }
+
+ if ((r = private2_check_padding(prvbuf)) != 0)
+ goto out;
/* Swap the parsed key back into place */
tmp = *kswap;
@@ -3966,9 +3983,9 @@ sshkey_private_to_blob2(struct sshkey *p
explicit_bzero(salt, sizeof(salt));
if (key != NULL)
freezero(key, keylen + ivlen);
- if (pubkeyblob != NULL)
+ if (pubkeyblob != NULL)
freezero(pubkeyblob, pubkeylen);
- if (b64 != NULL)
+ if (b64 != NULL)
freezero(b64, strlen(b64));
return r;
}
@@ -4192,31 +4209,6 @@ private2_decrypt(struct sshbuf *decoded,
}
sshbuf_free(kdf);
sshbuf_free(decrypted);
- return r;
-}
-
-/* Check deterministic padding after private key */
-static int
-private2_check_padding(struct sshbuf *decrypted)
-{
- u_char pad;
- size_t i;
- int r = SSH_ERR_INTERNAL_ERROR;
-
- i = 0;
- while (sshbuf_len(decrypted)) {
- if ((r = sshbuf_get_u8(decrypted, &pad)) != 0)
- goto out;
- if (pad != (++i & 0xff)) {
- r = SSH_ERR_INVALID_FORMAT;
- goto out;
- }
- }
- /* success */
- r = 0;
- out:
- explicit_bzero(&pad, sizeof(pad));
- explicit_bzero(&i, sizeof(i));
return r;
}
ssh: mux.c: mark argument as const
Index: mux.c
===
RCS file: /home/reposync/cvs/src/usr.bin/ssh/mux.c,v
retrieving revision 1.92
diff -u -p -r1.92 mux.c
--- mux.c 11 Jan 2022 01:26:47 - 1.92
+++ mux.c 13 Jan 2022 16:27:14 -
@@ -227,7 +227,7 @@ mux_master_control_cleanup_cb(struct ssh
/* Check mux client environment variables before passing them to mux master. */
static int
-env_permitted(char *env)
+env_permitted(const char *env)
{
int i, ret;
char name[1024], *cp;
ssh: channels.c: Fix comment and add a const
Hi
channel_new no longer frees remote_name. So update the comment
accordingly. As remote_name is not modified, it can be const
as well.
Best,
Martin
Index: channels.c
===
RCS file: /home/reposync/cvs/src/usr.bin/ssh/channels.c,v
retrieving revision 1.418
diff -u -p -r1.418 channels.c
--- channels.c 4 May 2022 07:31:22 - 1.418
+++ channels.c 4 May 2022 19:02:14 -
@@ -349,12 +349,11 @@ channel_register_fds(struct ssh *ssh, Ch
}
/*
- * Allocate a new channel object and set its type and socket. This will cause
- * remote_name to be freed.
+ * Allocate a new channel object and set its type and socket.
*/
Channel *
channel_new(struct ssh *ssh, char *ctype, int type, int rfd, int wfd, int efd,
-u_int window, u_int maxpack, int extusage, char *remote_name, int nonblock)
+u_int window, u_int maxpack, int extusage, const char *remote_name, int
nonblock)
{
struct ssh_channels *sc = ssh->chanctxt;
u_int i, found;
Index: channels.h
===
RCS file: /home/reposync/cvs/src/usr.bin/ssh/channels.h,v
retrieving revision 1.142
diff -u -p -r1.142 channels.h
--- channels.h 30 Mar 2022 21:10:25 - 1.142
+++ channels.h 6 Apr 2022 20:26:56 -
@@ -272,7 +272,7 @@ Channel *channel_by_id(struct ssh *, int
Channel*channel_by_remote_id(struct ssh *, u_int);
Channel*channel_lookup(struct ssh *, int);
Channel *channel_new(struct ssh *, char *, int, int, int, int,
- u_int, u_int, int, char *, int);
+ u_int, u_int, int, const char *, int);
voidchannel_set_fds(struct ssh *, int, int, int, int, int,
int, int, u_int);
voidchannel_free(struct ssh *, Channel *);
Re: clang-local.1: document support for source-based code coverage
On Wed, May 04, 2022 at 05:40:43PM +0200, Marc Espie wrote: > On Wed, May 04, 2022 at 07:43:35AM -0400, Bryan Steele wrote: > > On Wed, May 04, 2022 at 01:20:10PM +0200, Frederic Cambus wrote: > > > Hi tech@, > > > > > > The base system includes the compiler-rt profile library for > > > source-based code coverage. > > > > > > So here is a diff to document support in clang-local.1, the same > > > way we document support for the ubsan_minimal sanitizer runtime > > > which is also part of compiler-rt. > > > > > > Comments? OK? > > > > > > Index: share/man/man1/clang-local.1 > > > === > > > RCS file: /cvs/src/share/man/man1/clang-local.1,v > > > retrieving revision 1.23 > > > diff -u -p -r1.23 clang-local.1 > > > --- share/man/man1/clang-local.1 18 Feb 2022 00:39:18 - 1.23 > > > +++ share/man/man1/clang-local.1 4 May 2022 11:03:11 - > > > @@ -99,6 +99,15 @@ See the documentation for the > > > .Fl fsanitize-minimal-runtime > > > flag. > > > .It > > > +The base system includes the compiler-rt profile library for > > > +source-based code coverage. See the documentation for the > > > +.Fl fprofile-instr-generate > > > +and > > > +.Fl fcoverage-mapping > > > +flags. > > > +Note that llvm-profdata and llvm-cov tools from devel/llvm are > > > +required to process coverage data and produce reports. > > > +.It > > > The > > > .Xr malloc 3 , > > > .Xr calloc 3 , > > > > Isn't the purpose of the clang-local(1) to document local changes to the > > system compiler, -fsanitize-minimal-runtime feels like a special case as > > we do not have the complete sanitizer runtime. > > What do you suggest as a good location where people will > find that information easily ? As unfortunate as it is, clang documention is mostly on the website[0]. I don't see why this needs to be in clang-local, is there some reason why users would expect this to not work by default on OpenBSD? -Bryan. [0] https://releases.llvm.org/13.0.0/tools/clang/docs/UsersManual.html
Re: clang-local.1: document support for source-based code coverage
Marc Espie wrote: > On Wed, May 04, 2022 at 07:43:35AM -0400, Bryan Steele wrote: > > On Wed, May 04, 2022 at 01:20:10PM +0200, Frederic Cambus wrote: > > > Hi tech@, > > > > > > The base system includes the compiler-rt profile library for > > > source-based code coverage. > > > > > > So here is a diff to document support in clang-local.1, the same > > > way we document support for the ubsan_minimal sanitizer runtime > > > which is also part of compiler-rt. > > > > > > Comments? OK? > > > > > > Index: share/man/man1/clang-local.1 > > > === > > > RCS file: /cvs/src/share/man/man1/clang-local.1,v > > > retrieving revision 1.23 > > > diff -u -p -r1.23 clang-local.1 > > > --- share/man/man1/clang-local.1 18 Feb 2022 00:39:18 - 1.23 > > > +++ share/man/man1/clang-local.1 4 May 2022 11:03:11 - > > > @@ -99,6 +99,15 @@ See the documentation for the > > > .Fl fsanitize-minimal-runtime > > > flag. > > > .It > > > +The base system includes the compiler-rt profile library for > > > +source-based code coverage. See the documentation for the > > > +.Fl fprofile-instr-generate > > > +and > > > +.Fl fcoverage-mapping > > > +flags. > > > +Note that llvm-profdata and llvm-cov tools from devel/llvm are > > > +required to process coverage data and produce reports. > > > +.It > > > The > > > .Xr malloc 3 , > > > .Xr calloc 3 , > > > > Isn't the purpose of the clang-local(1) to document local changes to the > > system compiler, -fsanitize-minimal-runtime feels like a special case as > > we do not have the complete sanitizer runtime. > > What do you suggest as a good location where people will > find that information easily ? Who knows, but probably not in clang-local.1 I actually find it a bit offensive when a base document has to mention something not in base. While here, let me make a comment on the proposal's use of the token "devel/llvm" -- that is so completely obtuse and out of touch with the potential user base. The average person will not understand that at all. It is hugely presumptious that anyone searching for this compiler tooling will be familiar with the "ports" tree-heiracry; the reality is NOONE uses ports, instead they use packages with has a completely different namespace, and thus "devel/llvm" is completely meaningless to a person who uses packages. Note that adding "Note that" doesn't help either.
Re: clang-local.1: document support for source-based code coverage
On Wed, May 04, 2022 at 07:43:35AM -0400, Bryan Steele wrote: > On Wed, May 04, 2022 at 01:20:10PM +0200, Frederic Cambus wrote: > > Hi tech@, > > > > The base system includes the compiler-rt profile library for > > source-based code coverage. > > > > So here is a diff to document support in clang-local.1, the same > > way we document support for the ubsan_minimal sanitizer runtime > > which is also part of compiler-rt. > > > > Comments? OK? > > > > Index: share/man/man1/clang-local.1 > > === > > RCS file: /cvs/src/share/man/man1/clang-local.1,v > > retrieving revision 1.23 > > diff -u -p -r1.23 clang-local.1 > > --- share/man/man1/clang-local.118 Feb 2022 00:39:18 - 1.23 > > +++ share/man/man1/clang-local.14 May 2022 11:03:11 - > > @@ -99,6 +99,15 @@ See the documentation for the > > .Fl fsanitize-minimal-runtime > > flag. > > .It > > +The base system includes the compiler-rt profile library for > > +source-based code coverage. See the documentation for the > > +.Fl fprofile-instr-generate > > +and > > +.Fl fcoverage-mapping > > +flags. > > +Note that llvm-profdata and llvm-cov tools from devel/llvm are > > +required to process coverage data and produce reports. > > +.It > > The > > .Xr malloc 3 , > > .Xr calloc 3 , > > Isn't the purpose of the clang-local(1) to document local changes to the > system compiler, -fsanitize-minimal-runtime feels like a special case as > we do not have the complete sanitizer runtime. What do you suggest as a good location where people will find that information easily ?
Re: [External] : Re: another syzkaller problem in pf
On Wed, May 04, 2022 at 04:26:18PM +0200, Alexander Bluhm wrote: > On Wed, May 04, 2022 at 02:21:11PM +0200, Alexandr Nedvedicky wrote: > > I'm not sure flipping a flag is a right change. In general we don't want > > to hold NET_LOCK()/PF_LOCK() while waiting for memory. > > - We must not wait for memory when in the packet processing hot path. > Drop the packet instead. > > - We should not wait for memory when holding pf lock. Then we can > replace rw lock with a mutex or something else later. > > - Waiting for memory with net lock is unavoidable. We have to > wait when coming from system call. Doing preallocation may be > possible in some cases but code may get too complicated elsewhere. > > If getting the allocation out of the locks is doable here, it could > be the best solution. I think it's doable. I'll try to finish the for tables and start sending diffs hopefully next week. regards sashan
Re: [External] : Re: another syzkaller problem in pf
On Wed, May 04, 2022 at 02:21:11PM +0200, Alexandr Nedvedicky wrote: > I'm not sure flipping a flag is a right change. In general we don't want > to hold NET_LOCK()/PF_LOCK() while waiting for memory. - We must not wait for memory when in the packet processing hot path. Drop the packet instead. - We should not wait for memory when holding pf lock. Then we can replace rw lock with a mutex or something else later. - Waiting for memory with net lock is unavoidable. We have to wait when coming from system call. Doing preallocation may be possible in some cases but code may get too complicated elsewhere. If getting the allocation out of the locks is doable here, it could be the best solution. bluhm
Re: rpki-client: don't time out in offline mode
On Wed, May 04, 2022 at 04:09:41PM +0200, Theo Buehler wrote:
> On Wed, May 04, 2022 at 04:03:21PM +0200, Claudio Jeker wrote:
> > On Wed, May 04, 2022 at 03:51:02PM +0200, Theo Buehler wrote:
> > > I had output from rpki-client -f something piped into less. After an
> > > hour rpki-client couldn't take it any longer and decided to move on to
> > > a better place. It also left a residue via syslog on its way out. I
> > > don't think it should do that in file mode or offline mode:
> > >
> > > Index: main.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > > retrieving revision 1.201
> > > diff -u -p -r1.201 main.c
> > > --- main.c4 May 2022 13:07:35 - 1.201
> > > +++ main.c4 May 2022 13:46:13 -
> > > @@ -689,7 +689,7 @@ process_start(const char *title, int *fd
> > > /* change working directory to the cache directory */
> > > if (fchdir(cachefd) == -1)
> > > err(1, "fchdir");
> > > - if (timeout)
> > > + if (!noop && timeout > 0)
> > > alarm(timeout);
> > > close(pair[1]);
> > > *fd = pair[0];
> > > @@ -923,7 +923,7 @@ main(int argc, char *argv[])
> > > rrdppid = -1;
> > > }
> > >
> > > - if (timeout) {
> > > + if (!noop && timeout > 0) {
> > > /*
> > >* Commit suicide eventually
> > >* cron will normally start a new one
> > >
> >
> > I agree that the alarm should not be armed for -f mode but I would keep it
> > for -n mode. At least I think the timeout protects also from excessive
> > work in case the cache was provided through other means.
>
> Alright - I was going back and forth between two diffs. Let's go with
> the more cautious one, then:
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.201
> diff -u -p -r1.201 main.c
> --- main.c4 May 2022 13:07:35 - 1.201
> +++ main.c4 May 2022 14:04:51 -
> @@ -689,7 +689,7 @@ process_start(const char *title, int *fd
> /* change working directory to the cache directory */
> if (fchdir(cachefd) == -1)
> err(1, "fchdir");
> - if (timeout)
> + if (!filemode && timeout > 0)
> alarm(timeout);
> close(pair[1]);
> *fd = pair[0];
> @@ -923,7 +923,7 @@ main(int argc, char *argv[])
> rrdppid = -1;
> }
>
> - if (timeout) {
> + if (!filemode && timeout > 0) {
> /*
>* Commit suicide eventually
>* cron will normally start a new one
>
OK.
--
:wq Claudio
Re: rpki-client: don't time out in offline mode
On Wed, May 04, 2022 at 04:03:21PM +0200, Claudio Jeker wrote:
> On Wed, May 04, 2022 at 03:51:02PM +0200, Theo Buehler wrote:
> > I had output from rpki-client -f something piped into less. After an
> > hour rpki-client couldn't take it any longer and decided to move on to
> > a better place. It also left a residue via syslog on its way out. I
> > don't think it should do that in file mode or offline mode:
> >
> > Index: main.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > retrieving revision 1.201
> > diff -u -p -r1.201 main.c
> > --- main.c 4 May 2022 13:07:35 - 1.201
> > +++ main.c 4 May 2022 13:46:13 -
> > @@ -689,7 +689,7 @@ process_start(const char *title, int *fd
> > /* change working directory to the cache directory */
> > if (fchdir(cachefd) == -1)
> > err(1, "fchdir");
> > - if (timeout)
> > + if (!noop && timeout > 0)
> > alarm(timeout);
> > close(pair[1]);
> > *fd = pair[0];
> > @@ -923,7 +923,7 @@ main(int argc, char *argv[])
> > rrdppid = -1;
> > }
> >
> > - if (timeout) {
> > + if (!noop && timeout > 0) {
> > /*
> > * Commit suicide eventually
> > * cron will normally start a new one
> >
>
> I agree that the alarm should not be armed for -f mode but I would keep it
> for -n mode. At least I think the timeout protects also from excessive
> work in case the cache was provided through other means.
Alright - I was going back and forth between two diffs. Let's go with
the more cautious one, then:
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.201
diff -u -p -r1.201 main.c
--- main.c 4 May 2022 13:07:35 - 1.201
+++ main.c 4 May 2022 14:04:51 -
@@ -689,7 +689,7 @@ process_start(const char *title, int *fd
/* change working directory to the cache directory */
if (fchdir(cachefd) == -1)
err(1, "fchdir");
- if (timeout)
+ if (!filemode && timeout > 0)
alarm(timeout);
close(pair[1]);
*fd = pair[0];
@@ -923,7 +923,7 @@ main(int argc, char *argv[])
rrdppid = -1;
}
- if (timeout) {
+ if (!filemode && timeout > 0) {
/*
* Commit suicide eventually
* cron will normally start a new one
Re: rpki-client: don't time out in offline mode
On Wed, May 04, 2022 at 03:51:02PM +0200, Theo Buehler wrote:
> I had output from rpki-client -f something piped into less. After an
> hour rpki-client couldn't take it any longer and decided to move on to
> a better place. It also left a residue via syslog on its way out. I
> don't think it should do that in file mode or offline mode:
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.201
> diff -u -p -r1.201 main.c
> --- main.c4 May 2022 13:07:35 - 1.201
> +++ main.c4 May 2022 13:46:13 -
> @@ -689,7 +689,7 @@ process_start(const char *title, int *fd
> /* change working directory to the cache directory */
> if (fchdir(cachefd) == -1)
> err(1, "fchdir");
> - if (timeout)
> + if (!noop && timeout > 0)
> alarm(timeout);
> close(pair[1]);
> *fd = pair[0];
> @@ -923,7 +923,7 @@ main(int argc, char *argv[])
> rrdppid = -1;
> }
>
> - if (timeout) {
> + if (!noop && timeout > 0) {
> /*
>* Commit suicide eventually
>* cron will normally start a new one
>
I agree that the alarm should not be armed for -f mode but I would keep it
for -n mode. At least I think the timeout protects also from excessive
work in case the cache was provided through other means.
--
:wq Claudio
rpki-client: don't time out in offline mode
I had output from rpki-client -f something piped into less. After an
hour rpki-client couldn't take it any longer and decided to move on to
a better place. It also left a residue via syslog on its way out. I
don't think it should do that in file mode or offline mode:
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.201
diff -u -p -r1.201 main.c
--- main.c 4 May 2022 13:07:35 - 1.201
+++ main.c 4 May 2022 13:46:13 -
@@ -689,7 +689,7 @@ process_start(const char *title, int *fd
/* change working directory to the cache directory */
if (fchdir(cachefd) == -1)
err(1, "fchdir");
- if (timeout)
+ if (!noop && timeout > 0)
alarm(timeout);
close(pair[1]);
*fd = pair[0];
@@ -923,7 +923,7 @@ main(int argc, char *argv[])
rrdppid = -1;
}
- if (timeout) {
+ if (!noop && timeout > 0) {
/*
* Commit suicide eventually
* cron will normally start a new one
Introduce spkrcat (was Re: speaker(4): unhook driver and manpage from build)
Scott Cheloha wrote:
> speaker(4) is a whimsical thing, but I don't think we should have a
> dedicated chiptune interpreter in the kernel.
>
> This patch unhooks the driver and the manpage from the build. The
> driver is built for alpha, amd64, and i386.
>
> A subsequent patch will move all relevant files to the attic and clean
> up manpage cross references.
>
> Nothing in base or xenocara includes .
>
> I see a couple SPKRTONE and SPKRTUNE symbols in ports, but I imagine
> those ports don't use the symbols if they are missing.
>
> ok?
Hello tech@,
So, after the proposal to remove speaker(4), it came up that parsing the
play string in the kernel might not be the nicest thing. Here there is a
first approach to do that. spkrcat is a CLI that takes the play string
as an argument, parses it, and use the appropiate ioctl to play it.
Name is inspired by aucat.
DIFFERENCES WITH spkr.c IMPLEMENTATION
- slightly verbose identifiers names.
- more chatty.
- sound duration math is slightly different:
- for dotted notes, this is not computed correctly in kernelland: for
example, with default articulation (7/8th play time, 1/8th rest
time) and one dot, total length is computed as 25/16th of original
duration instead of 3/2th (== 24/16th).
After compiling with SPKRDEBUG, with 'T60 L4 c.', the total duration
should be 1500ms: spkrcat does this in 1313ms of sound + 187ms of
silence. spkr(4) computes this as 1375ms of sound + 187ms of
silence.
- even for non-dotted notes, I perceived sound duration slightly
different. Maybe related to "rounding errors" in kernelland?
After compiling with SPKRDEBUG, with 'T30 cbdcbdcbd', spkrcat does
59ms sound + 8ms rest, while spkr(4) does 58ms sound + 8ms. The
ideal total duration would be 66.666ms. spkrcat does round away
from zero, spkr(4) doesn't.
- legato is (almost) fully smooth: adjacent tones with the same
frequency are combined into one tone_t with the sum of the durations
(as long as it doesn't overflow an int), which results in a single
call to pcppi_bell.
- instead of using the default value on invalid inputs, clamp the value
to the valid range.
- unveil.
CURIOSITIES CARRIED OVER FROM spkr.c
- bounds are checked in playnote / addsound:
- "O0C-" has the same effect as "P"
- "O6B+" is a noop
MISSING / QUESTIONS
- I have absolutely no clue how many / which of the license headers /
comments from spkr.c should be carried over. As I'm in doubt, I
included all of them.
- no manpage yet: shouldn't be too difficult as it's mostly moving the
description from spkr(4), but haven't found the time to do so.
- ideally, this should be a bit more privsep'd, in a fashion similar to
file(1): a parsing process + a ioctl process. The parsing process can
be heavily pledge'd, as right now there is no way to pledge spkrcat
and use the appropiat ioctl interface. The other option is adding
ioctl(SPKR{TONE,TUNE}) to audio pledge. In any case, am unfamiliar
with both imsg and pledge internals, but I don't mind learning it once
an approach is set.
- no diff against the tree: I don't know where to place this between
usr.bin/ and usr.sbin/, and some part of me even wonders if it even
makes sense to include it there and not in games, or to even include
it in base and not distribute it as a port. The main reason I see for
including it in base (and in particular one of usr.{,s}bin/) is that
it provides an interface for a feature that currently resides in
kernelland.
- no code removal from spkr(4) yet: to be done after knowing in where to
place spkrcat.
Feedback and comments welcome.
-Lucas
# Copyright (c) 2022 Lucas Gabriel Vuotto
#
# Permission to use, copy, modify, and distribute this software for any
# purpose with or without fee is hereby granted, provided that the above
# copyright notice and this permission notice appear in all copies.
#
# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
PROG= spkrcat
NOMAN= spkrcat
.include
/* $OpenBSD$ */
/* $NetBSD: spkr.c,v 1.1 1998/04/15 20:26:18 drochner Exp $*/
/*
* Copyright (c) 2022 Lucas Gabriel Vuotto
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTAB
Re: [External] : Re: another syzkaller problem in pf
On Tue, May 03, 2022 at 09:31:51PM +0200, Alexander Bluhm wrote:
> On Tue, May 03, 2022 at 07:42:34PM +0200, Moritz Buhl wrote:
> > commit 4b3977248902c22d96aaebdb5784840debc2631c
> > Author: mikeb
> > Date: Mon Nov 24 13:22:09 2008 +
> >
> > Fix splasserts seen in pr 5987 by propagating a flag that discribes
> > whether we're called from the interrupt context to the functions
> > performing allocations.
>
> These days pf was protected with kernel lock and spl. Both are
> released when sleeping. Now we have netlock and pflock. These are
> rwlocks and not released during sleep. So this old race should not
> exist anymore.
>
> > And we are not in interrupt context.
>
> Yes, it is ioctl(2). I think we should always malloc with M_WAITOK
> when in syscall. Otherwise userland would have to cope with randomly
> failing syscalls.
>
> > If this is sound, then the only reason why pfr_destroy_ktable was called
> > is that pool_get is called with PR_NOWAIT. And then the following diff
> > would help.
>
> The code is too complex to be sure what the reason of the syzkaller
> panic is. Sleep in malloc is correct anyway and may improve the
> situation.
>
> Functions with argument values 0 or 1 are hard to read. It would
> be much nicer to pass M_WAITOK or M_NOWAIT. And the variable name
> "intr" does not make sense anymore. pf does not run in interrupt
> context. Call it "mflags" like in pfi_kif_alloc(). Or "wait" like
> in other functions.
>
> Could you cleanup that also?
I have a diff, which touches the same area. It's a work in progress change.
It moves all memory allocations outside of NET_LOCK/PF_LOCK. I'm just
sending it for your reference now.
I'm not sure flipping a flag is a right change. In general we don't want
to hold NET_LOCK()/PF_LOCK() while waiting for memory.
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 8315b115474..1f036e1368f 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -2217,12 +2217,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
error = ENODEV;
goto fail;
}
- NET_LOCK();
- PF_LOCK();
error = pfr_add_tables(io->pfrio_buffer, io->pfrio_size,
&io->pfrio_nadd, io->pfrio_flags | PFR_FLAG_USERIOCTL);
- PF_UNLOCK();
- NET_UNLOCK();
break;
}
diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
index fb23bcabe04..cbaa728b105 100644
--- a/sys/net/pf_table.c
+++ b/sys/net/pf_table.c
@@ -189,6 +189,7 @@ void pfr_clstats_ktable(struct
pfr_ktable *, time_t, int);
struct pfr_ktable *pfr_create_ktable(struct pfr_table *, time_t, int,
int);
voidpfr_destroy_ktables(struct pfr_ktableworkq *, int);
+voidpfr_destroy_ktables_aux(struct pfr_ktableworkq *);
voidpfr_destroy_ktable(struct pfr_ktable *, int);
int pfr_ktable_compare(struct pfr_ktable *,
struct pfr_ktable *);
@@ -1493,14 +1494,16 @@ pfr_clr_tables(struct pfr_table *filter, int *ndel, int
flags)
int
pfr_add_tables(struct pfr_table *tbl, int size, int *nadd, int flags)
{
- struct pfr_ktableworkq addq, changeq;
- struct pfr_ktable *p, *q, *r, key;
+ struct pfr_ktableworkq addq, changeq, auxq;
+ struct pfr_ktable *p, *q, *r, *n, *w, key;
int i, rv, xadd = 0;
time_t tzero = gettime();
ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY);
SLIST_INIT(&addq);
SLIST_INIT(&changeq);
+ SLIST_INIT(&auxq);
+ /* pre-allocate all memory outside of locks */
for (i = 0; i < size; i++) {
YIELD(flags & PFR_FLAG_USERIOCTL);
if (COPYIN(tbl+i, &key.pfrkt_t, sizeof(key.pfrkt_t), flags))
@@ -1509,65 +1512,140 @@ pfr_add_tables(struct pfr_table *tbl, int size, int
*nadd, int flags)
flags & PFR_FLAG_USERIOCTL))
senderr(EINVAL);
key.pfrkt_flags |= PFR_TFLAG_ACTIVE;
- p = RB_FIND(pfr_ktablehead, &pfr_ktables, &key);
- if (p == NULL) {
- p = pfr_create_ktable(&key.pfrkt_t, tzero, 1,
- !(flags & PFR_FLAG_USERIOCTL));
- if (p == NULL)
- senderr(ENOMEM);
- SLIST_FOREACH(q, &addq, pfrkt_workq) {
- if (!pfr_ktable_compare(p, q)) {
- pfr_destroy_ktable(p, 0);
- goto _skip;
- }
- }
- SLIST_INSERT_HEAD(&ad
Re: clang-local.1: document support for source-based code coverage
On Wed, May 04, 2022 at 01:20:10PM +0200, Frederic Cambus wrote: > Hi tech@, > > The base system includes the compiler-rt profile library for > source-based code coverage. > > So here is a diff to document support in clang-local.1, the same > way we document support for the ubsan_minimal sanitizer runtime > which is also part of compiler-rt. > > Comments? OK? > > Index: share/man/man1/clang-local.1 > === > RCS file: /cvs/src/share/man/man1/clang-local.1,v > retrieving revision 1.23 > diff -u -p -r1.23 clang-local.1 > --- share/man/man1/clang-local.1 18 Feb 2022 00:39:18 - 1.23 > +++ share/man/man1/clang-local.1 4 May 2022 11:03:11 - > @@ -99,6 +99,15 @@ See the documentation for the > .Fl fsanitize-minimal-runtime > flag. > .It > +The base system includes the compiler-rt profile library for > +source-based code coverage. See the documentation for the > +.Fl fprofile-instr-generate > +and > +.Fl fcoverage-mapping > +flags. > +Note that llvm-profdata and llvm-cov tools from devel/llvm are > +required to process coverage data and produce reports. > +.It > The > .Xr malloc 3 , > .Xr calloc 3 , Isn't the purpose of the clang-local(1) to document local changes to the system compiler, -fsanitize-minimal-runtime feels like a special case as we do not have the complete sanitizer runtime. -Bryan.
clang-local.1: document support for source-based code coverage
Hi tech@, The base system includes the compiler-rt profile library for source-based code coverage. So here is a diff to document support in clang-local.1, the same way we document support for the ubsan_minimal sanitizer runtime which is also part of compiler-rt. Comments? OK? Index: share/man/man1/clang-local.1 === RCS file: /cvs/src/share/man/man1/clang-local.1,v retrieving revision 1.23 diff -u -p -r1.23 clang-local.1 --- share/man/man1/clang-local.118 Feb 2022 00:39:18 - 1.23 +++ share/man/man1/clang-local.14 May 2022 11:03:11 - @@ -99,6 +99,15 @@ See the documentation for the .Fl fsanitize-minimal-runtime flag. .It +The base system includes the compiler-rt profile library for +source-based code coverage. See the documentation for the +.Fl fprofile-instr-generate +and +.Fl fcoverage-mapping +flags. +Note that llvm-profdata and llvm-cov tools from devel/llvm are +required to process coverage data and produce reports. +.It The .Xr malloc 3 , .Xr calloc 3 ,
Re: ratecheck mutex
On Wed, May 04, 2022 at 12:14:01AM +0200, Alexander Bluhm wrote:
> Hi,
>
> We have one comment that locking for ratecheck(9) is missing. In
> all other places locking status of the struct timeval *lasttime
> is unclear.
>
> The easiest fix is a global mutex for all lasttime in ratecheck().
> This covers the usual usecase of the function.
>
> Same for ppsratecheck(9), lasttime and curpps are protected.
>
> Remove a useless #if 1 while there.
>
> ok?
For now this is the quickest way to move forward. OK claudio@
It would be nice to make ratecheck() and ppsratecheck() only require a
lock in the unusual case of hitting the limit. At least in the common case
these calls should be as cheap as possible.
This is a nice little project for someone to explore how to implement this
in a fast way that does not introduce bus locks or other slow operations.
> Index: kern/kern_malloc.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_malloc.c,v
> retrieving revision 1.146
> diff -u -p -r1.146 kern_malloc.c
> --- kern/kern_malloc.c16 May 2021 15:10:20 - 1.146
> +++ kern/kern_malloc.c3 May 2022 21:51:21 -
> @@ -188,7 +188,6 @@ malloc(size_t size, int type, int flags)
> if (size > 65535 * PAGE_SIZE) {
> if (flags & M_CANFAIL) {
> #ifndef SMALL_KERNEL
> - /* XXX lock */
> if (ratecheck(&malloc_lasterr, &malloc_errintvl))
> printf("malloc(): allocation too large, "
> "type = %d, size = %lu\n", type, size);
> Index: kern/kern_time.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.154
> diff -u -p -r1.154 kern_time.c
> --- kern/kern_time.c 18 Jun 2021 15:59:14 - 1.154
> +++ kern/kern_time.c 3 May 2022 21:51:21 -
> @@ -782,11 +782,13 @@ itimerdecr(struct itimerspec *itp, long
> int
> ratecheck(struct timeval *lasttime, const struct timeval *mininterval)
> {
> + static struct mutex mtx = MUTEX_INITIALIZER(IPL_HIGH);
> struct timeval tv, delta;
> int rv = 0;
>
> getmicrouptime(&tv);
>
> + mtx_enter(&mtx);
> timersub(&tv, lasttime, &delta);
>
> /*
> @@ -798,6 +800,7 @@ ratecheck(struct timeval *lasttime, cons
> *lasttime = tv;
> rv = 1;
> }
> + mtx_leave(&mtx);
>
> return (rv);
> }
> @@ -808,11 +811,13 @@ ratecheck(struct timeval *lasttime, cons
> int
> ppsratecheck(struct timeval *lasttime, int *curpps, int maxpps)
> {
> + static struct mutex mtx = MUTEX_INITIALIZER(IPL_HIGH);
> struct timeval tv, delta;
> int rv;
>
> microuptime(&tv);
>
> + mtx_enter(&mtx);
> timersub(&tv, lasttime, &delta);
>
> /*
> @@ -837,20 +842,11 @@ ppsratecheck(struct timeval *lasttime, i
> else
> rv = 0;
>
> -#if 1 /*DIAGNOSTIC?*/
> /* be careful about wrap-around */
> if (*curpps + 1 > *curpps)
> *curpps = *curpps + 1;
> -#else
> - /*
> - * assume that there's not too many calls to this function.
> - * not sure if the assumption holds, as it depends on *caller's*
> - * behavior, not the behavior of this function.
> - * IMHO it is wrong to make assumption on the caller's behavior,
> - * so the above #if is #if 1, not #ifdef DIAGNOSTIC.
> - */
> - *curpps = *curpps + 1;
> -#endif
> +
> + mtx_leave(&mtx);
>
> return (rv);
> }
>
--
:wq Claudio
Re: ratecheck mutex
On Wed, May 04, 2022 at 04:42:21AM -0500, Scott Cheloha wrote: > > On May 3, 2022, at 17:16, Alexander Bluhm wrote: > > > > ???Hi, > > > > We have one comment that locking for ratecheck(9) is missing. In > > all other places locking status of the struct timeval *lasttime > > is unclear. > > > > The easiest fix is a global mutex for all lasttime in ratecheck(). > > This covers the usual usecase of the function. > > Why not declare a struct ratecheck with > a per-struct mutex? Because that diff is more work. It is the cleaner solution, but touches a lot of code. > It seems odd to be heading toward more > parallel processing in e.g. the networking > stack and introduce a global point of > contention. I don't expect contention on the rate limit. Make things stable, run in parallel, tweak performance. That's why I have chosen this aproach. But if it is too dirty, I can create the larger diff. bluhm
Re: ratecheck mutex
> On May 3, 2022, at 17:16, Alexander Bluhm wrote: > > Hi, > > We have one comment that locking for ratecheck(9) is missing. In > all other places locking status of the struct timeval *lasttime > is unclear. > > The easiest fix is a global mutex for all lasttime in ratecheck(). > This covers the usual usecase of the function. Why not declare a struct ratecheck with a per-struct mutex? It seems odd to be heading toward more parallel processing in e.g. the networking stack and introduce a global point of contention.
Re: acme-client: check token names
Florian Obser wrote:
> On 2022-05-03 17:41 +0430, Ali Farzanrad wrote:
> >
> > Hi Florian,
> >
> > Yes, I read the RFC, it should work, but I couldn't test it yet, because
> > my domain manager is a little lazy (I've registeret 2 subdomains for my
> > domain, but it is not listed in name servers yet). I'll probably test
> > it tomorrow.
> >
> > I read the isalnum man page, it said that isalnum might have different
> > behavior based on locale, so I decide to not use that. Is it OK?
>
> No, please do a
> setlocale(LC_CTYPE, "");
> at the beginning of chngproc, before the unveil and then use isalnum
> instead of handrolling it. It will fit into chngproc, so no need for
> another function
>
> --
> I'm not entirely sure you are real.
>
OK, I've tested following diff on my own domain and it works.
I did 2 modifications:
1. I explicitly call setlocate with "C" to ensure C locale,
2. I did a string length check. According to RFC it must have 128 bit
of random entropy, so it should have at least 22 base64 characters,
but I was unsure. So I only check for empty strings.
Index: chngproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/chngproc.c,v
retrieving revision 1.16
diff -u -p -r1.16 chngproc.c
--- chngproc.c 12 Jul 2021 15:09:20 - 1.16
+++ chngproc.c 4 May 2022 08:37:32 -
@@ -16,9 +16,11 @@
*/
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -42,6 +44,11 @@ chngproc(int netsock, const char *root)
goto out;
}
+ if (setlocale(LC_CTYPE, "C") == NULL) {
+ warnx("setlocale");
+ goto out;
+ }
+
if (pledge("stdio cpath wpath", NULL) == -1) {
warn("pledge");
goto out;
@@ -77,6 +84,18 @@ chngproc(int netsock, const char *root)
goto out;
else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
goto out;
+ else if (strlen(tok) < 1) {
+ warnx("token is too short");
+ goto out;
+ }
+
+ for (i = 0; tok[i]; ++i) {
+ int ch = (unsigned char)tok[i];
+ if (!isalnum(ch) && ch != '-' && ch != '_') {
+ warnx("token is not a valid base64url");
+ goto out;
+ }
+ }
if (asprintf(&fmt, "%s.%s", tok, th) == -1) {
warn("asprintf");
