Re: freezero(3) for stdio's internal buffers
On Tue, 28 Nov 2017 05:52:25 +, Theo de Raadt wrote: > > In fact, can recallocarray be faster than plain free followed by calloc? > > Yes. > > I think you are missing some nuances. These added functions have fast paths > and slow paths. freezero() isn't just a bzero, it also has munmap() > sequences. You are adding forced bzero or munmap() in circumstances > where previously a malloc implementation could keep the freed'd memory in > a cache for reuse. Hence programs willing to avoid leaking secrets to such cache cannot safely use stdio functions to manipulate those secrets. But, sure, this may very well be acceptable as, after all, stdio is merely a library provided for convenience, and programs needing complete control over their memory could use their own wrappers around raw system calls instead. > I find it hard to read your diff as convert other than "convert all free > operations to freezero", or always pay the cost no matter what. I indeed took a systematic approach, so perhaps the bits in vfprintf.c and vfwprintf.c are in fact overkill, and they could be left out. However, this does not invalidate the other parts of my first patch, many of which simply mirror the already established use of recallocarray on the same buffers. Regards, kshe
Re: relayd: 6.1-stable and relay_http.c rev 1.58
Below is a DEBUG dump from failing OPTIONS+GET+GET, e.g. header X-Forwarded-Port is not set. accept_reserve: inflight incremented, now 1 relay_read_http: session 1: size 87, to read -2 relay_read_http: session 1: header 'OPTIONS: /options.php HTTP/1.1' relay_read_http: session 1: header 'Host: test.jesper.office.se.domain.com' relay_read_http: session 1: header 'Accept: */*' relay_test: session 1: matched rule 0 relay_test:1767: next rule relay_test: session 1, res 0 relay_test: session 1: matched rule 1 relay_test:1767: next rule relay_test: session 1, res 0 relay_test: session 1: matched rule 2 relay_test:1767: next rule relay_test: session 1, res 0 relay_test: session 1: matched rule 3 relay_test:1767: next rule relay_test: session 1, res 0 relay_test:1747: next rule relay_test: session 1, res 0 relay_test: session 1: action 1 relay_writeheader_kv: Accept: */* relay_writeheader_kv: Host: test.jesper.office.se.domain.com relay_writeheader_kv: Keep-Alive: 600 relay_writeheader_kv: X-Forwarded-By: 172.16.1.101:80 relay_writeheader_kv: X-Forwarded-For: 172.17.2.21 relay_writeheader_kv: X-Forwarded-Port: 80 relay_from_table: session 1: table jesper:80 host 172.16.1.30, p 0x1f1046c6927e3ff3, idx 0, cnt 0, max 1 relay_connect: inflight decremented, now 0 relay_connected: session 1: successful relay_splice: session 1: splice dir 2, nothing to read -2 relay_splice: session 1: splice dir 1, maximum -1, successful relay_read_http: session 1: size 182, to read -2 relay_read_http: session 1: header 'HTTP/1.1: 200 OK' http_version HTTP/1.1 http_rescode 200 http_resmesg OK relay_read_http: session 1: header 'Date: Mon, 27 Nov 2017 19:01:55 GMT' relay_read_http: session 1: header 'Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips' relay_read_http: session 1: header 'Content-Length: 2' relay_read_http: session 1: header 'Content-Type: text/html; charset=UTF-8' relay_test:1729: skip 1 rules relay_test: session 1: action 1 version: HTTP/1.1 rescode: 200 resmsg: OK relay_writeheader_kv: Content-Length: 2 relay_writeheader_kv: Content-Type: text/html; charset=UTF-8 relay_writeheader_kv: Date: Mon, 27 Nov 2017 19:01:55 GMT relay_writeheader_kv: Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips relay_splice: session 1: splice dir 2, dirty buffer relay_read_httpcontent: session 1: size 2, to read 2 relay_read_httpcontent: done, size 2, to read 0 relay_read_http: session 1: size 0, to read -2 relay_splice: session 1: splice dir 2, nothing to read -2 relay_read_http: session 1: size 195, to read -2 relay_read_http: session 1: header 'HTTP/1.1: 403 Forbidden' http_version HTTP/1.1 http_rescode 403 http_resmesg Forbidden relay_read_http: session 1: header 'Date: Mon, 27 Nov 2017 19:01:55 GMT' relay_read_http: session 1: header 'Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips' relay_read_http: session 1: header 'Content-Length: 8' relay_read_http: session 1: header 'Content-Type: text/html; charset=UTF-8' relay_test:1729: skip 1 rules relay_test: session 1: action 1 version: HTTP/1.1 rescode: 403 resmsg: Forbidden relay_writeheader_kv: Content-Length: 8 relay_writeheader_kv: Content-Type: text/html; charset=UTF-8 relay_writeheader_kv: Date: Mon, 27 Nov 2017 19:01:55 GMT relay_writeheader_kv: Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips relay_splice: session 1: splice dir 2, dirty buffer relay_read_httpcontent: session 1: size 8, to read 8 relay_read_httpcontent: done, size 8, to read 0 relay_read_http: session 1: size 0, to read -2 relay_splice: session 1: splice dir 2, nothing to read -2 relay_read_http: session 1: size 195, to read -2 relay_read_http: session 1: header 'HTTP/1.1: 403 Forbidden' http_version HTTP/1.1 http_rescode 403 http_resmesg Forbidden relay_read_http: session 1: header 'Date: Mon, 27 Nov 2017 19:01:55 GMT' relay_read_http: session 1: header 'Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips' relay_read_http: session 1: header 'Content-Length: 8' relay_read_http: session 1: header 'Content-Type: text/html; charset=UTF-8' relay_test:1729: skip 1 rules relay_test: session 1: action 1 version: HTTP/1.1 rescode: 403 resmsg: Forbidden relay_writeheader_kv: Content-Length: 8 relay_writeheader_kv: Content-Type: text/html; charset=UTF-8 relay_writeheader_kv: Date: Mon, 27 Nov 2017 19:01:55 GMT relay_writeheader_kv: Server: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips relay_splice: session 1: splice dir 2, dirty buffer relay_read_httpcontent: session 1: size 8, to read 8 relay_read_httpcontent: done, size 8, to read 0 relay_read_http: session 1: size 0, to read -2 relay_splice: session 1: splice dir 2, nothing to read -2 relay web_test, session 1 (1 active), 0, 172.17.2.21 -> 172.16.1.30:80, done, OPTIONS //mxb > 27 nov. 2017 kl. 20:20 skrev Maxim Bourmistrov : > > Here is setup which reproduces this problem. Also exists in 6.2. > > Server: > Apache with mod_php serving following content: > > ———cut options.php——
Re: freezero(3) for stdio's internal buffers
> In fact, can recallocarray be faster than plain free followed by calloc? Yes. I think you are missing some nuances. These added functions have fast paths and slow paths. freezero() isn't just a bzero, it also has munmap() sequences. You are adding forced bzero or munmap() in circumstances where previously a malloc implementation could keep the freed'd memory in a cache for reuse. I find it hard to read your diff as convert other than "convert all free operations to freezero", or always pay the cost no matter what. You could compare make build times. I suspect this will slightly exceed the noise floor.
Re: freezero(3) for stdio's internal buffers
On Mon, 27 Nov 2017 08:01:25 +, Otto Moerbeek wrote:
> On Mon, Nov 27, 2017 at 05:48:14AM +, kshe wrote:
> > On Mon, 27 Nov 2017 00:42:01 +, Theo de Raadt wrote:
> > > This needs worst-case performance measurements.
> >
> > The only instances where performance could be a problem are in
> > vfprintf.c and vfwprintf.c, where the calls happen inside a loop; but
> > for these, in fact, the best solution would be to use recallocarray
> > directly: rather than repeatedly freeing (and clearing) `convbuf' and
> > reallocating a fresh one every time it is needed, it should be kept
> > around and passed to __wcsconv() and __mbsconv(), along with some
> > accounting of its current size, so that these functions could then call
> > recallocarray appropriately. However, this optimisation would be more
> > intrusive than the diff I submitted, and would therefore demand greater
> > familiarity with stdio's source, which I have not yet acquired.
> >
> > That being said, in all other cases, since the way stdio functions fill
> > their buffers is much more costly than simply writing a bunch of zeros
> > in sequence (or merely calling munmap(2)), no real slowdown is to be
> > expected. I should also point out that recallocarray is currently used
> > inside several loops in fvwrite.c, fgetln.c and getdelim.c, but no one
> > complained that the affected functions became too slow, because doing
> > things, as stdio does, like reading from and writing to disk or decoding
> > and converting user input will always dominate the effect of a few calls
> > to discard temporary buffers.
> >
> > Regards,
> >
> > kshe
>
> I was thinking: only point against your statement is that there could
> be cases where a very large buffer is used, but only a small part of
> it is used. In that case, clearing the buffer is more costly. Luckily,
> in those cases malloc just calls munmap(2) without clearing.
>
> Still, I would like to see benchmarks to validate assumptions.
Well, I looked under regress/lib/libc/, but nothing here seems to test
for pathological instances where performance is known to be an issue. I
would happily run benchmarks if someone could provide examples of such
cases.
In the mean time, here is a better diff for vfprintf.c and vfwprintf.c,
which should eliminate all potential performance regressions,
implementing the optimisation described in my previous message. In
fact, can recallocarray be faster than plain free followed by calloc?
If so, in the case of vfwprintf.c, this patch may actually improve
performance. At least, it shall not degrade it.
Nevertheless, this needs to be reviewed carefully since, as stated
above, I do not have much familiarity with stdio's intricate code.
Index: vfprintf.c
===
RCS file: /cvs/src/lib/libc/stdio/vfprintf.c,v
retrieving revision 1.77
diff -u -p -r1.77 vfprintf.c
--- vfprintf.c 29 Aug 2016 12:20:57 - 1.77
+++ vfprintf.c 28 Nov 2017 04:06:43 -
@@ -153,7 +153,7 @@ __sbprintf(FILE *fp, const char *fmt, va
* string is null-terminated.
*/
static char *
-__wcsconv(wchar_t *wcsarg, int prec)
+__wcsconv(wchar_t *wcsarg, int prec, char *oconvbuf, size_t *convbuf_size)
{
mbstate_t mbs;
char buf[MB_LEN_MAX];
@@ -191,18 +191,19 @@ __wcsconv(wchar_t *wcsarg, int prec)
return (NULL);
}
}
- if ((convbuf = malloc(nbytes + 1)) == NULL)
+ convbuf = recallocarray(oconvbuf, *convbuf_size, nbytes + 1, 1);
+ if (convbuf == NULL)
return (NULL);
+ *convbuf_size = nbytes + 1;
/* Fill the output buffer. */
p = wcsarg;
memset(&mbs, 0, sizeof(mbs));
- if ((nbytes = wcsrtombs(convbuf, (const wchar_t **)&p,
- nbytes, &mbs)) == (size_t)-1) {
- free(convbuf);
+ nbytes = wcsrtombs(convbuf, (const wchar_t **)&p, nbytes, &mbs);
+ if (nbytes == (size_t)-1) {
+ freezero(convbuf, *convbuf_size);
return (NULL);
}
- convbuf[nbytes] = '\0';
return (convbuf);
}
#endif
@@ -330,6 +331,7 @@ __vfprintf(FILE *fp, const char *fmt0, _
va_list orgap; /* original argument pointer */
#ifdef PRINTF_WIDE_CHAR
char *convbuf; /* buffer for wide to multi-byte conversion */
+ size_t convbuf_size;
#endif
/*
@@ -857,8 +859,6 @@ fp_common:
if (flags & LONGINT) {
wchar_t *wcp;
- free(convbuf);
- convbuf = NULL;
if ((wcp = GETARG(wchar_t *)) == NULL) {
struct syslog_data sdata =
SYSLOG_DATA_INIT;
int save_errno = errno;
@@ -867,9 +867,13 @@ fp_common:
"vfprintf %%ls NULL in \"%s\"",
fmt0);
Re: athn/ar5008: fix rssi reporting
On Mon, Nov 27, 2017 at 09:39:03PM +0100, Stefan Sperling wrote: > > This makes athn(4) report similar RSSI values as iwn(4) does, > instead of bugos positive dBm values. The driver forgot about > adding the default noisefloor to measured RSSI values. > > The same is already done in the USB athn(4) driver. > > It looks like noisefloor calibration is not yet enabled in this driver. > If that was fixed later, we could read the actual noisefloor from hardware. > Enabling noisefloor calibration might also address other known issues. ok kevlo@ > Index: ar5008.c > === > RCS file: /cvs/src/sys/dev/ic/ar5008.c,v > retrieving revision 1.45 > diff -u -p -r1.45 ar5008.c > --- ar5008.c 17 Jul 2017 14:25:29 - 1.45 > +++ ar5008.c 27 Nov 2017 20:24:20 - > @@ -925,6 +925,7 @@ ar5008_rx_process(struct athn_softc *sc) > /* Send the frame to the 802.11 layer. */ > rxi.rxi_flags = 0; /* XXX */ > rxi.rxi_rssi = MS(ds->ds_status4, AR_RXS4_RSSI_COMBINED); > + rxi.rxi_rssi += AR_DEFAULT_NOISE_FLOOR; > rxi.rxi_tstamp = ds->ds_status2; > ieee80211_input(ifp, m, ni, &rxi); > > @@ -1927,7 +1928,7 @@ ar5008_bb_load_noisefloor(struct athn_so > > /* Restore noisefloor values to initial (max) values. */ > for (i = 0; i < AR_MAX_CHAINS; i++) > - nf[i] = nf_ext[i] = -50 * 2; > + nf[i] = nf_ext[i] = AR_DEFAULT_NOISE_FLOOR; > ar5008_write_noisefloor(sc, nf, nf_ext); > } > > Index: athnreg.h > === > RCS file: /cvs/src/sys/dev/ic/athnreg.h,v > retrieving revision 1.20 > diff -u -p -r1.20 athnreg.h > --- athnreg.h 19 May 2017 11:42:48 - 1.20 > +++ athnreg.h 27 Nov 2017 20:13:48 - > @@ -1449,6 +1449,8 @@ > #define AR_CTL_2GHT407 > #define AR_CTL_5GHT408 > > +#define AR_DEFAULT_NOISE_FLOOR (-95) > + > /* > * Macros to access registers. > */ > >
Re: athn/ar5008: extend RSSI-related macros
On Mon, Nov 27, 2017 at 09:30:59PM +0100, Stefan Sperling wrote:
>
> Fix a comment which misidentifies the field where RSSI values occur.
> Add macros to access RSSI info in ds_status4 as well.
>
> ok?
Sure, ok kevlo@
> Index: ar5008reg.h
> ===
> RCS file: /cvs/src/sys/dev/ic/ar5008reg.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 ar5008reg.h
> --- ar5008reg.h 12 Jan 2017 16:32:28 - 1.4
> +++ ar5008reg.h 27 Nov 2017 19:29:26 -
> @@ -872,7 +872,7 @@ struct ar_rx_desc {
> #define AR_RXC1_BUF_LEN_S0
> #define AR_RXC1_INTR_REQ 0x2000
>
> -/* Bits for ds_ctl2. */
> +/* Bits for ds_status0. */
> #define AR_RXS0_RSSI_ANT00(x)(((x) >> 0) & 0xff)
> #define AR_RXS0_RSSI_ANT01(x)(((x) >> 8) & 0xff)
> #define AR_RXS0_RSSI_ANT02(x)(((x) >> 16) & 0xff)
> @@ -894,6 +894,9 @@ struct ar_rx_desc {
> #define AR_RXS3_RATE_S 2
>
> /* Bits for ds_status4. */
> +#define AR_RXS0_RSSI_ANT10(x)(((x) >> 0) & 0xff)
> +#define AR_RXS0_RSSI_ANT11(x)(((x) >> 8) & 0xff)
> +#define AR_RXS0_RSSI_ANT12(x)(((x) >> 16) & 0xff)
> #define AR_RXS4_RSSI_COMBINED_M 0xff00
> #define AR_RXS4_RSSI_COMBINED_S 24
>
>
>
Re: isakmpd: use monotonic clock for event timeouts
> On Nov 27, 2017, at 9:54 AM, Jeremie Courreges-Anglas wrote: > > On Fri, Nov 24 2017, Scott Cheloha wrote: >> Hi, >> >> [...] >> >> Thoughts and feedback? > > This seems to mix refactoring, eg changing the signature of > timer_add_event(), with semantic changes. Could you please > split your diff in seperate steps easier to review? Sure thing. > (I find that ports/devel/quilt helps a lot maintain a stack of patches.) Ah! there *is* a better way. I had been vaguely wondering about whether or not there was a middle ground between using the git mirror and using everyone's favorite version control system: mv file.c file.c.bak cvs -q up . Thanks. -- Scott Cheloha >> -- >> Scott Cheloha >> >> [...]
pfctl regression tests for load anchor
Hello,
adds two test cases for issues reported by Leonardo. I've created extra
pfloadanchors target in regress/sbin/pfctl/Makefile. The 'load anchor ... from
' construct still needs more love to be covered by existing targets.
consider output for command 'pfctl -o none -nvf pf113.in':
8<---8<---8<--8<
anchor "one" all
anchor "two" all
addrs = "{ 1.2.3.4,10.20.30.40,2.4.6.8,
20.40.60.80,4.8.12.16, 40.80.120.160,
5.6.7.8,50.60.70.80,
10.12.14.16,100.120.140.160 }"
pass inet from 1.2.3.4 to any flags S/SA
pass inet from 10.20.30.40 to any flags S/SA
pass inet from 2.4.6.8 to any flags S/SA
pass inet from 20.40.60.80 to any flags S/SA
pass inet from 4.8.12.16 to any flags S/SA
pass inet from 40.80.120.160 to any flags S/SA
pass inet from 5.6.7.8 to any flags S/SA
pass inet from 50.60.70.80 to any flags S/SA
pass inet from 10.12.14.16 to any flags S/SA
pass inet from 100.120.140.160 to any flags S/SA
8<---8<---8<--8<
The output above thoygh it's syntactically valid, is not the ruleset, which
would loaded to kernel. See output here:
8<---8<---8<--8<
pfctl -a regress -o none -f pf113.in
pfctl -a 'regress/*' -sr
anchor "one" all {
anchor "two" all {
pass inet from 1.2.3.4 to any flags S/SA
pass inet from 10.20.30.40 to any flags S/SA
pass inet from 2.4.6.8 to any flags S/SA
pass inet from 20.40.60.80 to any flags S/SA
pass inet from 4.8.12.16 to any flags S/SA
pass inet from 40.80.120.160 to any flags S/SA
pass inet from 5.6.7.8 to any flags S/SA
pass inet from 50.60.70.80 to any flags S/SA
pass inet from 10.12.14.16 to any flags S/SA
pass inet from 100.120.140.160 to any flags S/SA
}
}
8<---8<---8<--8<
I've tried to fix it, but decided to postpone the work. I'll eventually get
back to it.
The test cases below, load rules to kernel and then dump them using 'pfctl
-sr'. rules pf112.in verify the user-defined table can get loaded in anchor
'two'. pf113.in checks if optimizer still works for nested anchors.
OK?
thanks and
regards
sasha
8<---8<---8<--8<
diff --git a/regress/sbin/pfctl/Makefile b/regress/sbin/pfctl/Makefile
index adc236cc8b7..744aa408d97 100644
--- a/regress/sbin/pfctl/Makefile
+++ b/regress/sbin/pfctl/Makefile
@@ -30,6 +30,7 @@ PFIF2IP=1 2 3
PFCHKSUM=1 2 3
PFCMD=1
PFCMDFAIL=1
+PFLOADANCHORS=112 113
PFCTL ?= /sbin/pfctl
@@ -331,6 +332,21 @@ pfchksum-update: ${PFCHKSUM_UPDATES}
NODEFAULT_TARGETS+=pfchksum
REGRESS_ROOT_TARGETS+=pfchksum
+.for n in ${PFLOADANCHORS}
+PFLOADANCHORS_TARGETS+=pfloadanchors${n}
+
+pfloadanchors${n}:
+ ${SUDO} ${PFCTL} -a regress -v -f - < ${.CURDIR}/pf${n}.in
+ (${SUDO} ${PFCTL} -a 'regress/*' -sr | \
+ sed -e 's/__automatic_[0-9a-f]*_.*>/__automatic_>/' ) | \
+ diff -u ${.CURDIR}/pf${n}.ok /dev/stdin
+ ${SUDO} ${PFCTL} -o none -a regress -Fr >/dev/null 2>&1
+.endfor
+
+pfloadanchors: ${PFLOADANCHORS_TARGETS}
+
+REGRESS_TARGETS+=pfloadanchors
+
update:${UPDATE_TARGETS}
alltests: ${REGRESS_TARGETS} ${NODEFAULT_TARGETS}
diff --git a/regress/sbin/pfctl/pf112.in b/regress/sbin/pfctl/pf112.in
new file mode 100644
index 000..5b40dc0e69d
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.in
@@ -0,0 +1,2 @@
+anchor "one"
+load anchor "one" from "pf112.one"
diff --git a/regress/sbin/pfctl/pf112.ok b/regress/sbin/pfctl/pf112.ok
new file mode 100644
index 000..67420f7eea0
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.ok
@@ -0,0 +1,5 @@
+anchor "one" all {
+ anchor "two" all {
+pass from to any flags S/SA
+ }
+}
diff --git a/regress/sbin/pfctl/pf112.one b/regress/sbin/pfctl/pf112.one
new file mode 100644
index 000..68e20033087
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.one
@@ -0,0 +1,2 @@
+anchor "two"
+load anchor "two" from "pf112.two"
diff --git a/regress/sbin/pfctl/pf112.two b/regress/sbin/pfctl/pf112.two
new file mode 100644
index 000..84e5f759569
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.two
@@ -0,0 +1,2 @@
+table { 10.0.0.1 }
+pass from
diff --git a/regress/sbin/pfctl/pf113.in b/regress/sbin/pfctl/pf113.in
new file mode 100644
index 000..f62fa7ab84a
--- /dev/null
+++ b/regress/sbin/pfctl/pf113.in
@@ -0,0 +1,2 @@
+anchor "one"
+load anchor "one" from "pf113.one"
diff --git a/regress/sbin/pfctl/pf113.ok b/regress/sbin/pfctl/pf113.ok
new file mode 100644
index 000..a599c3c7e74
--- /dev/null
+++ b/regress/sbin/pfctl/pf113.ok
@@ -0,0 +1,5 @@
+anchor "one" all {
+ anchor "two" all {
+pass inet from <__automatic_> to any flags S/SA
+ }
+}
diff --git a/regress/sbin/pfctl/pf113.one b/regress/sbin/pfctl/p
armv7/sxitimer difference between systat & vmstat -i
Hi,
i had forgotten about this already, but this has existed for a while.
systat does consistently show "66 tick"(around 108 for stattick), while
root@av7marsb:~ # vmstat -i
interrupt total rate
irq32/sximmc0 35222 19
irq55/sxie0 38602
irq1/com0 172229
irq22/tick 181011 99
irq23/stattick 293512 162
Total 530827 293
how come there is such consistent difference? guessing they must source
time differently, so does either use interface leading to rtc _gettime()
instead of timecounters _get_timecount()?
while trying to fix this(, before remembering about "vmstat -i"), i saw
this sillyness in sxitimer irq handlers, unconditional looping that can
be avoided w/slight reordering, and given how it doesn't support other
clocksources, i think it does make sense to avoid the useless bus_space
branching for TIMER_OSC24M-bit while restarting the timers too.
what i mean with above is, given how there is such _sync "price to pay",
for touching _INTV; timer needs to be stopped, and shouldn't be enabled
until 2*Tcycles has passed. it doesn't make sense to delay disabling
to the end right before it is to be restarted.
-Artturi
diff --git a/sys/arch/armv7/sunxi/sxitimer.c b/sys/arch/armv7/sunxi/sxitimer.c
index 65d455adaeb..8abd2137304 100644
--- a/sys/arch/armv7/sunxi/sxitimer.c
+++ b/sys/arch/armv7/sunxi/sxitimer.c
@@ -71,6 +71,9 @@
#defineTIMER_CONTINOUS (0 << 7)
#defineTIMER_SINGLESHOT(1 << 7)
+#defineTIMER_RESTART (TIMER_ENABLE | TIMER_RELOAD | \
+ TIMER_OSC24M | TIMER_SINGLESHOT)
+
#defineTICKTIMER 0
#defineSTATTIMER 1
#defineCNTRTIMER 2
@@ -84,8 +87,8 @@ int sxitimer_statintr(void *);
void sxitimer_cpu_initclocks(void);
void sxitimer_setstatclockrate(int);
uint64_t sxitimer_readcnt64(void);
-uint32_t sxitimer_readcnt32(void);
-void sxitimer_sync(void);
+static inline uint32_t sxitimer_readcnt32(void);
+static inline void sxitimer_sync(uint32_t);
void sxitimer_delay(u_int);
u_int sxitimer_get_timecount(struct timecounter *);
@@ -290,7 +293,6 @@ int
sxitimer_tickintr(void *frame)
{
uint32_t now, nextevent;
- uint32_t val;
int rc = 0;
splassert(IPL_CLOCK);
@@ -299,8 +301,11 @@ sxitimer_tickintr(void *frame)
bus_space_write_4(sxitimer_iot, sxitimer_ioh,
TIMER_ISR, TIMER_IRQ(TICKTIMER));
- now = sxitimer_readcnt32();
+ /* stop/disable for sync */
+ bus_space_write_4(sxitimer_iot, sxitimer_ioh,
+ TIMER_CTRL(TICKTIMER), 0);
+ now = sxitimer_readcnt32();
while ((int32_t)(now - sxitimer_tick_nextevt) < 0) {
sxitimer_tick_nextevt -= sxitimer_tick_tpi;
sxitimer_ticks_err_sum += sxitimer_ticks_err_cnt;
@@ -328,21 +333,13 @@ sxitimer_tickintr(void *frame)
sxitimer_tick_nextevt = now;
}
- val = bus_space_read_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(TICKTIMER));
- bus_space_write_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(TICKTIMER), val & ~TIMER_ENABLE);
-
- sxitimer_sync();
+ sxitimer_sync(now);
bus_space_write_4(sxitimer_iot, sxitimer_ioh,
TIMER_INTV(TICKTIMER), nextevent);
- val = bus_space_read_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(TICKTIMER));
bus_space_write_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(TICKTIMER),
- val | TIMER_ENABLE | TIMER_RELOAD | TIMER_SINGLESHOT);
+ TIMER_CTRL(TICKTIMER), TIMER_RESTART);
return rc;
}
@@ -351,7 +348,6 @@ int
sxitimer_statintr(void *frame)
{
uint32_t now, nextevent, r;
- uint32_t val;
int rc = 0;
splassert(IPL_STATCLOCK);
@@ -360,6 +356,10 @@ sxitimer_statintr(void *frame)
bus_space_write_4(sxitimer_iot, sxitimer_ioh,
TIMER_ISR, TIMER_IRQ(STATTIMER));
+ /* stop/disable for sync */
+ bus_space_write_4(sxitimer_iot, sxitimer_ioh,
+ TIMER_CTRL(STATTIMER), 0);
+
now = sxitimer_readcnt32();
while ((int32_t)(now - sxitimer_stat_nextevt) < 0) {
do {
@@ -386,21 +386,13 @@ sxitimer_statintr(void *frame)
sxitimer_stat_nextevt = now;
}
- val = bus_space_read_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(STATTIMER));
- bus_space_write_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(STATTIMER), val & ~TIMER_ENABLE);
-
- sxitimer_sync();
+ sxitimer_sync(now);
bus_space_write_4(sxitimer_iot, sxitimer_ioh,
TIMER_INTV(STATTIMER), nextevent);
- val = bus_space_read_4(sxitimer_iot, sxitimer_ioh,
- TIMER_CTRL(STATTIMER));
relayd add relay_reset_event
Factor out the reset / closing of a relay connection. Currently only
relay_close is using this but I want to use this in relay_http.c to close
the backend connection after a request is done.
OK?
--
:wq Claudio
Index: relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.234
diff -u -p -r1.234 relay.c
--- relay.c 28 Nov 2017 00:17:56 - 1.234
+++ relay.c 28 Nov 2017 00:47:41 -
@@ -1698,17 +1698,8 @@ relay_close(struct rsession *con, const
(*proto->close)(con);
free(con->se_priv);
- if (con->se_in.bev != NULL)
- bufferevent_free(con->se_in.bev);
- if (con->se_in.output != NULL)
- evbuffer_free(con->se_in.output);
- if (con->se_in.tls != NULL)
- tls_close(con->se_in.tls);
- tls_free(con->se_in.tls);
- tls_config_free(con->se_in.tls_cfg);
- free(con->se_in.tlscert);
- if (con->se_in.s != -1) {
- close(con->se_in.s);
+
+ if (relay_reset_event(&con->se_in)) {
if (con->se_out.s == -1) {
/*
* the output was never connected,
@@ -1719,26 +1710,18 @@ relay_close(struct rsession *con, const
__func__, relay_inflight);
}
}
+ if (con->se_in.output != NULL)
+ evbuffer_free(con->se_in.output);
- if (con->se_out.bev != NULL)
- bufferevent_free(con->se_out.bev);
- if (con->se_out.output != NULL)
- evbuffer_free(con->se_out.output);
- if (con->se_out.tls != NULL)
- tls_close(con->se_out.tls);
- tls_free(con->se_out.tls);
- tls_config_free(con->se_out.tls_cfg);
- free(con->se_out.tlscert);
- if (con->se_out.s != -1) {
- close(con->se_out.s);
-
+ if (relay_reset_event(&con->se_out)) {
/* Some file descriptors are available again. */
if (evtimer_pending(&rlay->rl_evt, NULL)) {
evtimer_del(&rlay->rl_evt);
event_add(&rlay->rl_ev, NULL);
}
}
- con->se_out.state = STATE_INIT;
+ if (con->se_out.output != NULL)
+ evbuffer_free(con->se_out.output);
if (con->se_log != NULL)
evbuffer_free(con->se_log);
@@ -1753,6 +1736,34 @@ relay_close(struct rsession *con, const
free(con);
relay_sessions--;
+}
+
+int
+relay_reset_event(struct ctl_relay_event *cre)
+{
+ int rv = 0;
+
+ DPRINTF("%s: state %d dir %d", __func__, cre->state, cre->dir);
+
+ if (cre->bev != NULL)
+ bufferevent_free(cre->bev);
+ if (cre->tls != NULL)
+ tls_close(cre->tls);
+ tls_free(cre->tls);
+ tls_config_free(cre->tls_cfg);
+ free(cre->tlscert);
+ if (cre->s != -1) {
+ close(cre->s);
+ rv = 1;
+ }
+ cre->state = STATE_DONE;
+ cre->bev = NULL;
+ cre->tls = NULL;
+ cre->tls_cfg = NULL;
+ cre->tlscert = NULL;
+ cre->s = -1;
+
+ return (rv);
}
int
Index: relayd.h
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
retrieving revision 1.245
diff -u -p -r1.245 relayd.h
--- relayd.h27 Nov 2017 23:21:16 - 1.245
+++ relayd.h28 Nov 2017 00:47:41 -
@@ -192,7 +192,8 @@ enum relay_state {
STATE_INIT,
STATE_PENDING,
STATE_PRECONNECT,
- STATE_CONNECTED
+ STATE_CONNECTED,
+ STATE_DONE
};
struct ctl_relay_event {
@@ -1172,6 +1173,7 @@ intrelay_session_cmp(struct rsession *
char *relay_load_fd(int, off_t *);
int relay_load_certfiles(struct relay *);
voidrelay_close(struct rsession *, const char *);
+int relay_reset_event(struct ctl_relay_event *);
voidrelay_natlook(int, short, void *);
voidrelay_session(struct rsession *);
int relay_from_table(struct rsession *);
Re: dc.1: document non-portability of `e'
On Mon, Nov 27, 2017 at 07:00:51PM +0100, Otto Moerbeek wrote: > On Mon, Nov 27, 2017 at 10:34:50AM +, Jason McIntyre wrote: > > > On Sun, Nov 26, 2017 at 07:18:33PM +, kshe wrote: > > > Hi, > > > > > > The manual page for dc(1) is very careful about signalling which > > > commands are non-portable extensions, with the exception of the `e' > > > command, which is a more recent addition. > > > > > > Index: dc.1 > > > === > > > RCS file: /cvs/src/usr.bin/dc/dc.1,v > > > retrieving revision 1.30 > > > diff -u -p -r1.30 dc.1 > > > --- dc.1 23 Feb 2017 06:40:17 - 1.30 > > > +++ dc.1 26 Oct 2017 04:44:01 - > > > @@ -189,6 +189,7 @@ The top value on the stack is duplicated > > > Equivalent to > > > .Ic p , > > > except that the output is written to the standard error stream. > > > +This is a non-portable extension. > > > .It Ic f > > > All values on the stack are printed, separated by newlines. > > > .It Ic G > > > > > > Regards, > > > > > > kshe > > > > > > > morning. > > > > posix doesn;t describe a separate dc utility, as far as i can see. the > > STANDARDS section of dc(1) discusses only the "arithmetic operations" of > > dc. > > > > bc(1) however does describe -e as an extension, in STANDARDS. > > > > i think here dc(1) is a bit of a special case, but i think how it is now > > is fine. i definitely don;t want to start plastering "this is an > > extension" onto the end of every option not described by posix. > > > > jmc > > This is not about the -e flag but about the 'e' command. The frame of > reference is the set of command implemented by the original dc(1). I > think it is a good think to mention that 'e' is non-portable. > > -Otto > fair enough, but in STANDARDS... jmc
change auto-allocation for /usr/src
Hi,
1G is too smallto hold a cvs checkout, its 1.1G right now.
ok?
(benno_disklabel_editor)
Index: editor.c
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.308
diff -u -p -r1.308 editor.c
--- editor.c29 Sep 2017 18:32:09 - 1.308
+++ editor.c27 Nov 2017 23:53:39 -
@@ -77,7 +77,7 @@ struct space_allocation alloc_big[] = {
{ MEG(900), GIG(2), 5, "/usr"},
{ MEG(512), GIG(1), 3, "/usr/X11R6" },
{ MEG(1200),GIG(10), 15, "/usr/local" },
- {GIG(1), GIG(2), 2, "/usr/src"},
+ { MEG(1300), GIG(2), 2, "/usr/src"},
{GIG(3), GIG(6), 4, "/usr/obj"},
{GIG(1), GIG(300), 35, "/home" }
/* Anything beyond this leave for the user to decide */
pf divert type
Hi,
This converts the pf rule structure to use the divert type.
Old semantics was:
divert.port: divert-to or divert-reply
divert.addr: divert-to
divert_packet.port: divert-packet
Now we have one divert structure with an explicit type.
ok?
bluhm
Index: sys/net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1047
diff -u -p -r1.1047 pf.c
--- sys/net/pf.c22 Nov 2017 12:28:49 - 1.1047
+++ sys/net/pf.c27 Nov 2017 23:25:27 -
@@ -1380,7 +1380,8 @@ pf_remove_divert_state(struct pf_state_k
TAILQ_FOREACH(si, &sk->states, entry) {
if (sk == si->s->key[PF_SK_STACK] && si->s->rule.ptr &&
- si->s->rule.ptr->divert.port) {
+ (si->s->rule.ptr->divert.type == PF_DIVERT_TO ||
+ si->s->rule.ptr->divert.type == PF_DIVERT_REPLY)) {
pf_remove_state(si->s);
break;
}
@@ -6981,18 +6982,21 @@ done:
if (pd.destchg && pd.dir == PF_OUT)
pd.m->m_pkthdr.pf.flags |= PF_TAG_REROUTE;
- if (pd.dir == PF_IN && action == PF_PASS && r->divert.port) {
+ if (pd.dir == PF_IN && action == PF_PASS &&
+ (r->divert.type == PF_DIVERT_TO ||
+ r->divert.type == PF_DIVERT_REPLY)) {
struct pf_divert *divert;
if ((divert = pf_get_divert(pd.m))) {
pd.m->m_pkthdr.pf.flags |= PF_TAG_DIVERTED;
+ divert->addr = r->divert.addr;
divert->port = r->divert.port;
divert->rdomain = pd.rdomain;
- divert->addr = r->divert.addr;
+ divert->type = r->divert.type;
}
}
- if (action == PF_PASS && r->divert_packet.port)
+ if (action == PF_PASS && r->divert.type == PF_DIVERT_PACKET)
action = PF_DIVERT;
#if NPFLOG > 0
@@ -7023,13 +7027,12 @@ done:
case PF_DIVERT:
switch (pd.af) {
case AF_INET:
- if (!divert_packet(pd.m, pd.dir, r->divert_packet.port))
+ if (!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_packet.port))
+ if (!divert6_packet(pd.m, pd.dir, r->divert.port))
pd.m = NULL;
break;
#endif /* INET6 */
Index: sys/net/pf_ioctl.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.325
diff -u -p -r1.325 pf_ioctl.c
--- sys/net/pf_ioctl.c 13 Nov 2017 11:30:11 - 1.325
+++ sys/net/pf_ioctl.c 27 Nov 2017 23:25:27 -
@@ -2796,8 +2796,7 @@ pf_rule_copyin(struct pf_rule *from, str
to->flush = from->flush;
to->divert.addr = from->divert.addr;
to->divert.port = from->divert.port;
- to->divert_packet.addr = from->divert_packet.addr;
- to->divert_packet.port = from->divert_packet.port;
+ to->divert.type = from->divert.type;
to->prio = from->prio;
to->set_prio[0] = from->set_prio[0];
to->set_prio[1] = from->set_prio[1];
Index: sys/net/pfvar.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
retrieving revision 1.468
diff -u -p -r1.468 pfvar.h
--- sys/net/pfvar.h 27 Nov 2017 23:21:50 - 1.468
+++ sys/net/pfvar.h 27 Nov 2017 23:25:27 -
@@ -590,7 +590,8 @@ struct pf_rule {
struct {
struct pf_addr addr;
u_int16_t port;
- } divert, divert_packet;
+ u_int8_ttype;
+ } divert;
SLIST_ENTRY(pf_rule) gcle;
struct pf_ruleset *ruleset;
@@ -1394,6 +1395,7 @@ struct pf_divert {
struct pf_addr addr;
u_int16_t port;
u_int16_t rdomain;
+ u_int8_ttype;
};
enum pf_divert_types {
Index: sys/netinet/raw_ip.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/raw_ip.c,v
retrieving revision 1.106
diff -u -p -r1.106 raw_ip.c
--- sys/netinet/raw_ip.c20 Nov 2017 10:35:24 - 1.106
+++ sys/netinet/raw_ip.c27 Nov 2017 23:25:27 -
@@ -149,7 +149,7 @@ rip_input(struct mbuf **mp, int *offp, i
/* XXX rdomain support */
if ((divert = pf_find_divert(m)) == NULL)
continue;
- if (!divert->addr.v4.s_addr)
+
Re: pfctl divert type
Hello, > Hi, > > The divert structure uses the port number to indicate that divert-to > or divert-reply is active. Divert packet uses a separate structure. > This is confusing and makes it hard to add new features. It is > better to have a divert type that explicitly says what is configured. > > This is the first part of the diff that converts the pfctl(8) rule > parser. Kernel cleanup will be the next step. > > ok? > the patch looks good to me. OK sashan
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:46:54PM +0100, Claudio Jeker wrote:
> On Mon, Nov 27, 2017 at 11:34:49PM +0100, Alexander Bluhm wrote:
> > On Mon, Nov 27, 2017 at 11:26:06PM +0100, Claudio Jeker wrote:
> > > Guess we should make the 3 fatalx() in that code different so that it
> > > becomes more clear on which call it fails.
> >
> > As in the diff below?
> >
> > > Wonder what kind of startup race we lose...
> >
> > I can reproduce one of the errors on my laptop:
> >
> > make run-regress-args-https-inspect.pl # create test certificates
> > find / >/dev/null # clear the buffer cache for a while
> > make run-regress-args-https-inspect.pl # fails at the first try
> >
> > run-regress-args-https-inspect.pl
> > time SUDO=sudo KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd
> > /usr/src/regress/usr.sbin/relayd/relayd.pl copy
> > /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl
> > Client child status: 65280 exit: 255 at
> > /usr/src/regress/usr.sbin/relayd/Proc.pm line 161.
> > *** Error 255 in /usr/src/regress/usr.sbin/relayd (Makefile:68
> > 'run-regress-args-https-inspect.pl')
> >
> > Now amd64, T430s, ssd, single machine
> >
> > execute: sudo relayd -dvv -f relayd.conf
> > startup
> > relay_load_certfiles: using ca certificate ca.crt
> > pfe: filter init done
> > socket_rlimit: max open files 1024
> > socket_rlimit: max open files 1024
> > relay_load_certfiles: using ca key ca.key
> > relay_load_certfiles: using certificate /etc/ssl/127.0.0.1.crt
> > relay_load_certfiles: using private key /etc/ssl/private/127.0.0.1.key
> > parent_tls_ticket_rekey: rekeying tickets
> > relay_privinit: adding relay relay-args-https-inspect.pl
> > protocol 1: name proto-args-https-inspect.pl
> > flags: used, relay flags: tls, tls client, tls inspect
> > tcp flags: no splice
> > tls flags: tlsv1.2, cipher-server-preference
> > tls session tickets: disabled
> > type: http
> > match request header log "foo" value "*"
> > match response header log "bar" value "*"
> > ca_engine_init: using RSA privsep engine
> > ca_engine_init: using RSA privsep engine
> > relay_tls_ctx_create: loading certificate
> > failed to load tls CA certificate: Invalid argument
> > relay: relay_launch: failed to create TLS context
> > pfe exiting, pid 18436
> > hce exiting, pid 69282
> > lost child: pid 5175 exited abnormally
> > ca exiting, pid 40825
> > parent terminating, pid 53770
> >
> > bluhm
> >
> > Index: ca.c
> > ===
> > RCS file: /mount/openbsd/cvs/src/usr.sbin/relayd/ca.c,v
> > retrieving revision 1.29
> > diff -u -p -r1.29 ca.c
> > --- ca.c27 Nov 2017 21:06:25 - 1.29
> > +++ ca.c27 Nov 2017 21:45:59 -
> > @@ -123,14 +123,14 @@ ca_launch(void)
> > if (rlay->rl_tls_cert_fd != -1) {
> > if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
> > &len)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert relay_load_fd");
> >
> > if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert BIO_new_mem_buf");
> >
> > if ((cert = PEM_read_bio_X509(in, NULL,
> > NULL, NULL)) == NULL)
> > - fatalx("ca_launch: cert");
> > + fatalx("ca_launch: cert PEM_read_bio_X509");
> >
> > hash_x509(cert, hash, sizeof(hash));
> >
> > @@ -160,14 +160,14 @@ ca_launch(void)
> > if (rlay->rl_tls_cacert_fd != -1) {
> > if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
> > &len)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert relay_load_fd");
> >
> > if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert BIO_new_mem_buf");
> >
> > if ((cert = PEM_read_bio_X509(in, NULL,
> > NULL, NULL)) == NULL)
> > - fatalx("ca_launch: cacert");
> > + fatalx("ca_launch: cacert PEM_read_bio_X509");
> >
> > hash_x509(cert, hash, sizeof(hash));
> >
>
> Have the same diff in my tree. OK claudio@
> The problem is a race in relay_load_fd() according to guenther@ we should
> use pread instead of the lseek/ read combo. Lets see if that works.
>
Like this. Seems to fix it for me.
--
:wq Claudio
Index: relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.231
diff -u -p -r1
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:34:49PM +0100, Alexander Bluhm wrote:
> On Mon, Nov 27, 2017 at 11:26:06PM +0100, Claudio Jeker wrote:
> > Guess we should make the 3 fatalx() in that code different so that it
> > becomes more clear on which call it fails.
>
> As in the diff below?
>
> > Wonder what kind of startup race we lose...
>
> I can reproduce one of the errors on my laptop:
>
> make run-regress-args-https-inspect.pl# create test certificates
> find / >/dev/null # clear the buffer cache for a while
> make run-regress-args-https-inspect.pl# fails at the first try
>
> run-regress-args-https-inspect.pl
> time SUDO=sudo KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd
> /usr/src/regress/usr.sbin/relayd/relayd.pl copy
> /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl
> Client child status: 65280 exit: 255 at
> /usr/src/regress/usr.sbin/relayd/Proc.pm line 161.
> *** Error 255 in /usr/src/regress/usr.sbin/relayd (Makefile:68
> 'run-regress-args-https-inspect.pl')
>
> Now amd64, T430s, ssd, single machine
>
> execute: sudo relayd -dvv -f relayd.conf
> startup
> relay_load_certfiles: using ca certificate ca.crt
> pfe: filter init done
> socket_rlimit: max open files 1024
> socket_rlimit: max open files 1024
> relay_load_certfiles: using ca key ca.key
> relay_load_certfiles: using certificate /etc/ssl/127.0.0.1.crt
> relay_load_certfiles: using private key /etc/ssl/private/127.0.0.1.key
> parent_tls_ticket_rekey: rekeying tickets
> relay_privinit: adding relay relay-args-https-inspect.pl
> protocol 1: name proto-args-https-inspect.pl
> flags: used, relay flags: tls, tls client, tls inspect
> tcp flags: no splice
> tls flags: tlsv1.2, cipher-server-preference
> tls session tickets: disabled
> type: http
> match request header log "foo" value "*"
> match response header log "bar" value "*"
> ca_engine_init: using RSA privsep engine
> ca_engine_init: using RSA privsep engine
> relay_tls_ctx_create: loading certificate
> failed to load tls CA certificate: Invalid argument
> relay: relay_launch: failed to create TLS context
> pfe exiting, pid 18436
> hce exiting, pid 69282
> lost child: pid 5175 exited abnormally
> ca exiting, pid 40825
> parent terminating, pid 53770
>
> bluhm
>
> Index: ca.c
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/relayd/ca.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 ca.c
> --- ca.c 27 Nov 2017 21:06:25 - 1.29
> +++ ca.c 27 Nov 2017 21:45:59 -
> @@ -123,14 +123,14 @@ ca_launch(void)
> if (rlay->rl_tls_cert_fd != -1) {
> if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
> &len)) == NULL)
> - fatalx("ca_launch: cert");
> + fatalx("ca_launch: cert relay_load_fd");
>
> if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> - fatalx("ca_launch: cert");
> + fatalx("ca_launch: cert BIO_new_mem_buf");
>
> if ((cert = PEM_read_bio_X509(in, NULL,
> NULL, NULL)) == NULL)
> - fatalx("ca_launch: cert");
> + fatalx("ca_launch: cert PEM_read_bio_X509");
>
> hash_x509(cert, hash, sizeof(hash));
>
> @@ -160,14 +160,14 @@ ca_launch(void)
> if (rlay->rl_tls_cacert_fd != -1) {
> if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
> &len)) == NULL)
> - fatalx("ca_launch: cacert");
> + fatalx("ca_launch: cacert relay_load_fd");
>
> if ((in = BIO_new_mem_buf(buf, len)) == NULL)
> - fatalx("ca_launch: cacert");
> + fatalx("ca_launch: cacert BIO_new_mem_buf");
>
> if ((cert = PEM_read_bio_X509(in, NULL,
> NULL, NULL)) == NULL)
> - fatalx("ca_launch: cacert");
> + fatalx("ca_launch: cacert PEM_read_bio_X509");
>
> hash_x509(cert, hash, sizeof(hash));
>
Have the same diff in my tree. OK claudio@
The problem is a race in relay_load_fd() according to guenther@ we should
use pread instead of the lseek/ read combo. Lets see if that works.
--
:wq Claudio
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:26:06PM +0100, Claudio Jeker wrote:
> Guess we should make the 3 fatalx() in that code different so that it
> becomes more clear on which call it fails.
As in the diff below?
> Wonder what kind of startup race we lose...
I can reproduce one of the errors on my laptop:
make run-regress-args-https-inspect.pl # create test certificates
find / >/dev/null # clear the buffer cache for a while
make run-regress-args-https-inspect.pl # fails at the first try
run-regress-args-https-inspect.pl
time SUDO=sudo KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd
/usr/src/regress/usr.sbin/relayd/relayd.pl copy
/usr/src/regress/usr.sbin/relayd/args-https-inspect.pl
Client child status: 65280 exit: 255 at
/usr/src/regress/usr.sbin/relayd/Proc.pm line 161.
*** Error 255 in /usr/src/regress/usr.sbin/relayd (Makefile:68
'run-regress-args-https-inspect.pl')
Now amd64, T430s, ssd, single machine
execute: sudo relayd -dvv -f relayd.conf
startup
relay_load_certfiles: using ca certificate ca.crt
pfe: filter init done
socket_rlimit: max open files 1024
socket_rlimit: max open files 1024
relay_load_certfiles: using ca key ca.key
relay_load_certfiles: using certificate /etc/ssl/127.0.0.1.crt
relay_load_certfiles: using private key /etc/ssl/private/127.0.0.1.key
parent_tls_ticket_rekey: rekeying tickets
relay_privinit: adding relay relay-args-https-inspect.pl
protocol 1: name proto-args-https-inspect.pl
flags: used, relay flags: tls, tls client, tls inspect
tcp flags: no splice
tls flags: tlsv1.2, cipher-server-preference
tls session tickets: disabled
type: http
match request header log "foo" value "*"
match response header log "bar" value "*"
ca_engine_init: using RSA privsep engine
ca_engine_init: using RSA privsep engine
relay_tls_ctx_create: loading certificate
failed to load tls CA certificate: Invalid argument
relay: relay_launch: failed to create TLS context
pfe exiting, pid 18436
hce exiting, pid 69282
lost child: pid 5175 exited abnormally
ca exiting, pid 40825
parent terminating, pid 53770
bluhm
Index: ca.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/relayd/ca.c,v
retrieving revision 1.29
diff -u -p -r1.29 ca.c
--- ca.c27 Nov 2017 21:06:25 - 1.29
+++ ca.c27 Nov 2017 21:45:59 -
@@ -123,14 +123,14 @@ ca_launch(void)
if (rlay->rl_tls_cert_fd != -1) {
if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
&len)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert relay_load_fd");
if ((in = BIO_new_mem_buf(buf, len)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert BIO_new_mem_buf");
if ((cert = PEM_read_bio_X509(in, NULL,
NULL, NULL)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert PEM_read_bio_X509");
hash_x509(cert, hash, sizeof(hash));
@@ -160,14 +160,14 @@ ca_launch(void)
if (rlay->rl_tls_cacert_fd != -1) {
if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
&len)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert relay_load_fd");
if ((in = BIO_new_mem_buf(buf, len)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert BIO_new_mem_buf");
if ((cert = PEM_read_bio_X509(in, NULL,
NULL, NULL)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert PEM_read_bio_X509");
hash_x509(cert, hash, sizeof(hash));
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:11:59PM +0100, Alexander Bluhm wrote: > On Mon, Nov 27, 2017 at 11:04:49PM +0100, Alexander Bluhm wrote: > > And I am waiting for my loop to fail ... > > Now I have got a different error: > > execute: ssh ot2 perl -I /usr/src/regress/usr.sbin/relayd > /usr/src/regress/usr.sbin/relayd/remote.pl copy 10.188.81.22 10.188.81.21 > 37198 /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl > execute: relayd -dvv -f /usr/src/regress/usr.sbin/relayd/relayd.conf > startup > relay_load_certfiles: using ca certificate ca.crt > pfe: filter init done > relay_load_certfiles: using ca key ca.key > relay_load_certfiles: using certificate /etc/ssl/10.188.81.22.crt > relay_load_certfiles: using private key /etc/ssl/private/10.188.81.22.key > parent_tls_ticket_rekey: rekeying tickets > relay_privinit: adding relay relay-args-https-inspect.pl > protocol 1: name proto-args-https-inspect.pl > flags: used, relay flags: tls, tls client, tls inspect > tcp flags: no splice > tls flags: tlsv1.2, cipher-server-preference > tls session tickets: disabled > type: http > match request header log "foo" value "*" > match response header log "bar" value "*" > socket_rlimit: max open files 1024 > socket_rlimit: max open files 1024 > ca_engine_init: using RSA privsep engine > ca_engine_init: using RSA privsep engine > relay_tls_ctx_create: loading certificate > failed to load tls certificate: Invalid argument > relay: relay_launch: failed to create TLS context > lost child: pid 77881 exited abnormally > hce exiting, pid 50562 > pfe exiting, pid 83847 > ca exiting, pid 16315 > parent terminating, pid 52245 > listen sock: 10.188.81.22 11154 > stdin closed > So relay_load_fd() fails. The code is trashing errno so that may not be a real EINVAL. Guess this is another change to do. -- :wq Claudio
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:04:49PM +0100, Alexander Bluhm wrote: > On Mon, Nov 27, 2017 at 10:40:33PM +0100, Claudio Jeker wrote: > > Does not happen here. Running > > while make run-regress-args-https-inspect.pl ; do echo -n; done > > for a few minutes now and no failure. > > It takes a while. I am running it on > - very old and slow i386 machine, different timing > - relayd runs on remote machine, different network timing > - I have mpi@'s TCP unlock kernel, should not matter > > > Very strange. Can you provide me the log files of such a failure? > > It seems that we get an fatal error. ca: ca_launch: cert > Guess we should make the 3 fatalx() in that code different so that it becomes more clear on which call it fails. Wonder what kind of startup race we lose... -- :wq Claudio
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 11:04:49PM +0100, Alexander Bluhm wrote: > And I am waiting for my loop to fail ... Now I have got a different error: execute: ssh ot2 perl -I /usr/src/regress/usr.sbin/relayd /usr/src/regress/usr.sbin/relayd/remote.pl copy 10.188.81.22 10.188.81.21 37198 /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl execute: relayd -dvv -f /usr/src/regress/usr.sbin/relayd/relayd.conf startup relay_load_certfiles: using ca certificate ca.crt pfe: filter init done relay_load_certfiles: using ca key ca.key relay_load_certfiles: using certificate /etc/ssl/10.188.81.22.crt relay_load_certfiles: using private key /etc/ssl/private/10.188.81.22.key parent_tls_ticket_rekey: rekeying tickets relay_privinit: adding relay relay-args-https-inspect.pl protocol 1: name proto-args-https-inspect.pl flags: used, relay flags: tls, tls client, tls inspect tcp flags: no splice tls flags: tlsv1.2, cipher-server-preference tls session tickets: disabled type: http match request header log "foo" value "*" match response header log "bar" value "*" socket_rlimit: max open files 1024 socket_rlimit: max open files 1024 ca_engine_init: using RSA privsep engine ca_engine_init: using RSA privsep engine relay_tls_ctx_create: loading certificate failed to load tls certificate: Invalid argument relay: relay_launch: failed to create TLS context lost child: pid 77881 exited abnormally hce exiting, pid 50562 pfe exiting, pid 83847 ca exiting, pid 16315 parent terminating, pid 52245 listen sock: 10.188.81.22 11154 stdin closed
Specify ECDHE curves like httpd does
Adapt the changes Joel Sing did to httpd a while ago to move to
tls_config_set_ecdhecurves() to relayd.
This removes and changes the ecdhe config in relayd.conf but I assume not
many people are setting non default values there anyway. With this diff
multipl cureves can be selected instead of just one.
--
:wq Claudio
Index: config.c
===
RCS file: /cvs/src/usr.sbin/relayd/config.c,v
retrieving revision 1.33
diff -u -p -r1.33 config.c
--- config.c14 Sep 2017 08:59:54 - 1.33
+++ config.c27 Nov 2017 21:42:10 -
@@ -100,9 +100,9 @@ config_init(struct relayd *env)
(void)strlcpy(env->sc_proto_default.tlsciphers,
TLSCIPHERS_DEFAULT,
sizeof(env->sc_proto_default.tlsciphers));
- (void)strlcpy(env->sc_proto_default.tlsecdhcurve,
- TLSECDHCURVE_DEFAULT,
- sizeof(env->sc_proto_default.tlsecdhcurve));
+ (void)strlcpy(env->sc_proto_default.tlsecdhecurves,
+ TLSECDHECURVES_DEFAULT,
+ sizeof(env->sc_proto_default.tlsecdhecurves));
(void)strlcpy(env->sc_proto_default.tlsdhparams,
TLSDHPARAM_DEFAULT,
sizeof(env->sc_proto_default.tlsdhparams));
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
retrieving revision 1.218
diff -u -p -r1.218 parse.y
--- parse.y 16 Nov 2017 14:24:34 - 1.218
+++ parse.y 27 Nov 2017 21:43:18 -
@@ -171,8 +171,8 @@ typedef struct {
%token RESPONSE RETRY QUICK RETURN ROUNDROBIN ROUTE SACK SCRIPT SEND SESSION
%token SNMP SOCKET SPLICE SSL STICKYADDR STYLE TABLE TAG TAGGED TCP TIMEOUT TLS
%token TO ROUTER RTLABEL TRANSPARENT TRAP UPDATES URL VIRTUAL WITH TTL RTABLE
-%token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE PASSWORD ECDH
-%token EDH CURVE TICKETS
+%token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE PASSWORD ECDHE
+%token EDH TICKETS
%token STRING
%token NUMBER
%typehostname interface table value optstring
@@ -1006,8 +1006,8 @@ proto : relay_proto PROTO STRING {
TAILQ_INIT(&p->rules);
(void)strlcpy(p->tlsciphers, TLSCIPHERS_DEFAULT,
sizeof(p->tlsciphers));
- (void)strlcpy(p->tlsecdhcurve, TLSECDHCURVE_DEFAULT,
- sizeof(p->tlsecdhcurve));
+ (void)strlcpy(p->tlsecdhecurves, TLSECDHECURVES_DEFAULT,
+ sizeof(p->tlsecdhecurves));
(void)strlcpy(p->tlsdhparams, TLSDHPARAM_DEFAULT,
sizeof(p->tlsdhparams));
if (last_proto_id == INT_MAX) {
@@ -1161,37 +1161,29 @@ tlsflags: SESSION TICKETS { proto->tick
}
free($3);
}
- | NO ECDH {
- (void)strlcpy(proto->tlsecdhcurve, "none",
- sizeof(proto->tlsecdhcurve));
- }
- | ECDH {
- (void)strlcpy(proto->tlsecdhcurve, "auto",
- sizeof(proto->tlsecdhcurve));
- }
- | ECDH CURVE STRING {
+ | ECDHE STRING {
struct tls_config *tls_cfg;
if ((tls_cfg = tls_config_new()) == NULL) {
yyerror("tls_config_new failed");
- free($3);
+ free($2);
YYERROR;
}
- if (tls_config_set_ecdhecurve(tls_cfg, $3) != 0) {
- yyerror("tls ecdh curve %s: %s", $3,
+ if (tls_config_set_ecdhecurves(tls_cfg, $2) != 0) {
+ yyerror("tls ecdhe %s: %s", $2,
tls_config_error(tls_cfg));
tls_config_free(tls_cfg);
- free($3);
+ free($2);
YYERROR;
}
tls_config_free(tls_cfg);
- if (strlcpy(proto->tlsecdhcurve, $3,
- sizeof(proto->tlsecdhcurve)) >=
- sizeof(proto->tlsecdhcurve)) {
- yyerror("tls ecdh truncated");
- free($3);
+ if (strlcpy(proto->tlsecdhecurves, $2,
+ sizeof(proto->tlsecdhecurves)) >=
+ sizeof(proto->tlsecdhecurves)) {
+ yyerror("tls ecdhe cu
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 10:40:33PM +0100, Claudio Jeker wrote:
> Does not happen here. Running
> while make run-regress-args-https-inspect.pl ; do echo -n; done
> for a few minutes now and no failure.
It takes a while. I am running it on
- very old and slow i386 machine, different timing
- relayd runs on remote machine, different network timing
- I have mpi@'s TCP unlock kernel, should not matter
> Very strange. Can you provide me the log files of such a failure?
It seems that we get an fatal error. ca: ca_launch: cert
execute: ssh ot2 perl -I /usr/src/regress/usr.sbin/relayd
/usr/src/regress/usr.sbin/relayd/remote.pl splice 10.188.81.22 10.188.81.21
30722 /usr/src/regress/usr.sbin/relayd/args-https-inspect.pl
execute: relayd -dvv -f /usr/src/regress/usr.sbin/relayd/relayd.conf
startup
relay_load_certfiles: using ca certificate ca.crt
pfe: filter init done
relay_load_certfiles: using ca key ca.key
relay_load_certfiles: using certificate /etc/ssl/10.188.81.22.crt
relay_load_certfiles: using private key /etc/ssl/private/10.188.81.22.key
parent_tls_ticket_rekey: rekeying tickets
relay_privinit: adding relay relay-args-https-inspect.pl
protocol 1: name proto-args-https-inspect.pl
flags: used, relay flags: tls, tls client, tls inspect
tls flags: tlsv1.2, cipher-server-preference
tls session tickets: disabled
type: http
match request header log "foo" value "*"
match response header log "bar" value "*"
socket_rlimit: max open files 1024
socket_rlimit: max open files 1024
ca_engine_init: using RSA privsep engine
ca_engine_init: using RSA privsep engine
relay_tls_ctx_create: loading certificate
ca: ca_launch: cert
hce exiting, pid 62790
relay_tls_ctx_create: loading CA certificate
relay_tls_ctx_create: loading certificate
pfe exiting, pid 66220
relay_launch: running relay relay-args-https-inspect.pl
lost child: pid 35965 exited abnormally
relay exiting, pid 5
parent terminating, pid 69884
listen sock: 10.188.81.22 39157
stdin closed
Unfortunately there are three places with that log message. So I
use the current code from cvs with this diff applied. I think every
error message should be unique. And I am waiting for my loop to
fail ...
ok for this debugging aid?
bluhm
Index: ca.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/relayd/ca.c,v
retrieving revision 1.29
diff -u -p -r1.29 ca.c
--- ca.c27 Nov 2017 21:06:25 - 1.29
+++ ca.c27 Nov 2017 21:45:59 -
@@ -123,14 +123,14 @@ ca_launch(void)
if (rlay->rl_tls_cert_fd != -1) {
if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
&len)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert relay_load_fd");
if ((in = BIO_new_mem_buf(buf, len)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert BIO_new_mem_buf");
if ((cert = PEM_read_bio_X509(in, NULL,
NULL, NULL)) == NULL)
- fatalx("ca_launch: cert");
+ fatalx("ca_launch: cert PEM_read_bio_X509");
hash_x509(cert, hash, sizeof(hash));
@@ -160,14 +160,14 @@ ca_launch(void)
if (rlay->rl_tls_cacert_fd != -1) {
if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
&len)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert relay_load_fd");
if ((in = BIO_new_mem_buf(buf, len)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert BIO_new_mem_buf");
if ((cert = PEM_read_bio_X509(in, NULL,
NULL, NULL)) == NULL)
- fatalx("ca_launch: cacert");
+ fatalx("ca_launch: cacert PEM_read_bio_X509");
hash_x509(cert, hash, sizeof(hash));
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 10:27:50PM +0100, Alexander Bluhm wrote: > On Mon, Nov 27, 2017 at 08:23:29PM +0100, Claudio Jeker wrote: > > Instead of using imsg to pass certificates, pass the fd to the cert to the > > relay processes. This allows for large certificates and esp. ca file to > > work. OCSP stapling will also be added through this. > > relayd regression tests pass with that diff. > > When I make run-regress-args-https-inspect.pl in a loop it fails > after a while. So there is a race, question is whether it is the > test or in relayd. I did not see that before, so it might be > introduced by your change. I will keep an eye on this. Does not happen here. Running while make run-regress-args-https-inspect.pl ; do echo -n; done for a few minutes now and no failure. Very strange. Can you provide me the log files of such a failure? -- :wq Claudio
Re: relayd load certificates via fd
On Mon, Nov 27, 2017 at 08:23:29PM +0100, Claudio Jeker wrote: > Instead of using imsg to pass certificates, pass the fd to the cert to the > relay processes. This allows for large certificates and esp. ca file to > work. OCSP stapling will also be added through this. relayd regression tests pass with that diff. When I make run-regress-args-https-inspect.pl in a loop it fails after a while. So there is a race, question is whether it is the test or in relayd. I did not see that before, so it might be introduced by your change. I will keep an eye on this. bluhm
pfctl divert type
Hi,
The divert structure uses the port number to indicate that divert-to
or divert-reply is active. Divert packet uses a separate structure.
This is confusing and makes it hard to add new features. It is
better to have a divert type that explicitly says what is configured.
This is the first part of the diff that converts the pfctl(8) rule
parser. Kernel cleanup will be the next step.
ok?
bluhm
Index: sys/net/pfvar.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfvar.h,v
retrieving revision 1.467
diff -u -p -r1.467 pfvar.h
--- sys/net/pfvar.h 13 Nov 2017 11:30:11 - 1.467
+++ sys/net/pfvar.h 27 Nov 2017 20:53:47 -
@@ -1396,6 +1396,13 @@ struct pf_divert {
u_int16_t rdomain;
};
+enum pf_divert_types {
+ PF_DIVERT_NONE,
+ PF_DIVERT_TO,
+ PF_DIVERT_REPLY,
+ PF_DIVERT_PACKET
+};
+
/* Fragment entries reference mbuf clusters, so base the default on that. */
#define PFFRAG_FRENT_HIWAT (NMBCLUSTERS / 16) /* Number of entries */
#define PFFRAG_FRAG_HIWAT (NMBCLUSTERS / 32) /* Number of packets */
Index: sbin/pfctl/parse.y
===
RCS file: /data/mirror/openbsd/cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.666
diff -u -p -r1.666 parse.y
--- sbin/pfctl/parse.y 25 Nov 2017 22:26:25 - 1.666
+++ sbin/pfctl/parse.y 27 Nov 2017 21:13:42 -
@@ -211,6 +211,7 @@ struct pool_opts {
struct divertspec {
struct node_host*addr;
u_int16_tport;
+ enum pf_divert_types type;
};
struct redirspec {
@@ -262,7 +263,6 @@ struct filter_opts {
u_int8_t prio;
u_int8_t set_prio[2];
struct divertspecdivert;
- struct divertspecdivert_packet;
struct redirspec nat;
struct redirspec rdr;
struct redirspec rroute;
@@ -1913,7 +1913,6 @@ pfrule: action dir logquick interface
}
if (expand_divertspec(&r, &$8.divert))
YYERROR;
- r.divert_packet.port = $8.divert_packet.port;
expand_rule(&r, 0, $4, &$8.nat, &$8.rdr, &$8.rroute, $6,
$7.src_os,
@@ -2044,6 +2043,11 @@ filter_opt : USER uids {
filter_opts.rtableid = $2;
}
| DIVERTTO STRING PORT portplain {
+ if (filter_opts.divert.type != PF_DIVERT_NONE) {
+ yyerror("more than one divert option");
+ YYERROR;
+ }
+ filter_opts.divert.type = PF_DIVERT_TO;
if ((filter_opts.divert.addr = host($2, pf->opts)) ==
NULL) {
yyerror("could not parse divert address: %s",
$2);
@@ -2058,9 +2062,18 @@ filter_opt : USER uids {
}
}
| DIVERTREPLY {
- filter_opts.divert.port = 1;/* some random value */
+ if (filter_opts.divert.type != PF_DIVERT_NONE) {
+ yyerror("more than one divert option");
+ YYERROR;
+ }
+ filter_opts.divert.type = PF_DIVERT_REPLY;
}
| DIVERTPACKET PORT number {
+ if (filter_opts.divert.type != PF_DIVERT_NONE) {
+ yyerror("more than one divert option");
+ YYERROR;
+ }
+ filter_opts.divert.type = PF_DIVERT_PACKET;
/*
* If IP reassembly was not turned off, also
* forcibly enable TCP reassembly by default.
@@ -2072,8 +2085,7 @@ filter_opt: USER uids {
yyerror("invalid divert port");
YYERROR;
}
-
- filter_opts.divert_packet.port = htons($3);
+ filter_opts.divert.port = htons($3);
}
| SCRUB '(' scrub_opts ')' {
filter_opts.nodf = $3.nodf;
@@ -4076,10 +4088,6 @@ rule_consistent(struct pf_rule *r, int a
yyerror("divert is not supported on match rules");
problems++;
}
- if (r->divert_packet.port) {
- yyerror("divert is not supported on match rules");
- problems++;
- }
if (r->rt) {
yyerror("route-to, reply-to and dup-to "
"are not supported on
athn/ar5008: fix rssi reporting
This makes athn(4) report similar RSSI values as iwn(4) does, instead of bugos positive dBm values. The driver forgot about adding the default noisefloor to measured RSSI values. The same is already done in the USB athn(4) driver. It looks like noisefloor calibration is not yet enabled in this driver. If that was fixed later, we could read the actual noisefloor from hardware. Enabling noisefloor calibration might also address other known issues. Index: ar5008.c === RCS file: /cvs/src/sys/dev/ic/ar5008.c,v retrieving revision 1.45 diff -u -p -r1.45 ar5008.c --- ar5008.c17 Jul 2017 14:25:29 - 1.45 +++ ar5008.c27 Nov 2017 20:24:20 - @@ -925,6 +925,7 @@ ar5008_rx_process(struct athn_softc *sc) /* Send the frame to the 802.11 layer. */ rxi.rxi_flags = 0; /* XXX */ rxi.rxi_rssi = MS(ds->ds_status4, AR_RXS4_RSSI_COMBINED); + rxi.rxi_rssi += AR_DEFAULT_NOISE_FLOOR; rxi.rxi_tstamp = ds->ds_status2; ieee80211_input(ifp, m, ni, &rxi); @@ -1927,7 +1928,7 @@ ar5008_bb_load_noisefloor(struct athn_so /* Restore noisefloor values to initial (max) values. */ for (i = 0; i < AR_MAX_CHAINS; i++) - nf[i] = nf_ext[i] = -50 * 2; + nf[i] = nf_ext[i] = AR_DEFAULT_NOISE_FLOOR; ar5008_write_noisefloor(sc, nf, nf_ext); } Index: athnreg.h === RCS file: /cvs/src/sys/dev/ic/athnreg.h,v retrieving revision 1.20 diff -u -p -r1.20 athnreg.h --- athnreg.h 19 May 2017 11:42:48 - 1.20 +++ athnreg.h 27 Nov 2017 20:13:48 - @@ -1449,6 +1449,8 @@ #define AR_CTL_2GHT40 7 #define AR_CTL_5GHT40 8 +#define AR_DEFAULT_NOISE_FLOOR (-95) + /* * Macros to access registers. */
athn/ar5008: extend RSSI-related macros
Fix a comment which misidentifies the field where RSSI values occur.
Add macros to access RSSI info in ds_status4 as well.
ok?
Index: ar5008reg.h
===
RCS file: /cvs/src/sys/dev/ic/ar5008reg.h,v
retrieving revision 1.4
diff -u -p -r1.4 ar5008reg.h
--- ar5008reg.h 12 Jan 2017 16:32:28 - 1.4
+++ ar5008reg.h 27 Nov 2017 19:29:26 -
@@ -872,7 +872,7 @@ struct ar_rx_desc {
#define AR_RXC1_BUF_LEN_S 0
#define AR_RXC1_INTR_REQ 0x2000
-/* Bits for ds_ctl2. */
+/* Bits for ds_status0. */
#define AR_RXS0_RSSI_ANT00(x) (((x) >> 0) & 0xff)
#define AR_RXS0_RSSI_ANT01(x) (((x) >> 8) & 0xff)
#define AR_RXS0_RSSI_ANT02(x) (((x) >> 16) & 0xff)
@@ -894,6 +894,9 @@ struct ar_rx_desc {
#define AR_RXS3_RATE_S 2
/* Bits for ds_status4. */
+#define AR_RXS0_RSSI_ANT10(x) (((x) >> 0) & 0xff)
+#define AR_RXS0_RSSI_ANT11(x) (((x) >> 8) & 0xff)
+#define AR_RXS0_RSSI_ANT12(x) (((x) >> 16) & 0xff)
#define AR_RXS4_RSSI_COMBINED_M0xff00
#define AR_RXS4_RSSI_COMBINED_S24
Re: __warn_references: drop redundant "warning: " prefix
On Sat, Nov 18, 2017 at 02:58:29AM +0100, Jeremie Courreges-Anglas wrote: > On Sun, Nov 12 2017, Scott Cheloha wrote: > > Hi, > > > > GNU ld has prefixed the contents of .gnu.warning.SYMBOL sections > > with "warning: " since 2003, so the messages themselves need not > > contain the prefix anymore. > > > > If LLVM ld ever acknowledges .gnu.warning sections I imagine it > > would emulate this behavior. > > It would be good if lld gained support for .gnu.warning sections, those > warnings put the light on code that should be improved; not only in the > ports tree, as shown by the libcrypto warnings in base. > > After a quick glance it seems that recent binutils versions have the > same behavior, so it's reasonable to assume that lld would follow > GNU ld(1) by prepending "warning:". Especially if they don't want to > get the same bug report as the gcc folks got: > > https://sourceware.org/ml/binutils/2003-08/msg8.html > > > Thoughts? > > The double "warning: " message looks just weird, removing it makes sense > IMO. ok jca@ I also support removing these. Any objections to putting this in? > > > -- > > Scott Cheloha > > > > Index: lib/libc/arch/i386/string/strcat.S > > === > > RCS file: /cvs/src/lib/libc/arch/i386/string/strcat.S,v > > retrieving revision 1.9 > > diff -u -p -r1.9 strcat.S > > --- lib/libc/arch/i386/string/strcat.S 31 Aug 2015 02:53:56 - > > 1.9 > > +++ lib/libc/arch/i386/string/strcat.S 12 Nov 2017 04:21:37 - > > @@ -9,7 +9,7 @@ > > #if defined(APIWARN) > > #APP > > .section .gnu.warning.strcat > > - .ascii "warning: strcat() is almost always misused, please use > > strlcat()" > > + .ascii "strcat() is almost always misused, please use strlcat()" > > #NO_APP > > #endif > > > > Index: lib/libc/arch/i386/string/strcpy.S > > === > > RCS file: /cvs/src/lib/libc/arch/i386/string/strcpy.S,v > > retrieving revision 1.9 > > diff -u -p -r1.9 strcpy.S > > --- lib/libc/arch/i386/string/strcpy.S 31 Aug 2015 02:53:56 - > > 1.9 > > +++ lib/libc/arch/i386/string/strcpy.S 12 Nov 2017 04:21:37 - > > @@ -9,7 +9,7 @@ > > #if defined(APIWARN) > > #APP > > .section .gnu.warning.strcpy > > - .ascii "warning: strcpy() is almost always misused, please use > > strlcpy()" > > + .ascii "strcpy() is almost always misused, please use strlcpy()" > > #NO_APP > > #endif > > > > Index: lib/libc/compat-43/getwd.c > > === > > RCS file: /cvs/src/lib/libc/compat-43/getwd.c,v > > retrieving revision 1.11 > > diff -u -p -r1.11 getwd.c > > --- lib/libc/compat-43/getwd.c 30 Sep 2013 12:02:30 - 1.11 > > +++ lib/libc/compat-43/getwd.c 12 Nov 2017 04:21:37 - > > @@ -46,4 +46,4 @@ getwd(char *buf) > > } > > > > __warn_references(getwd, > > -"warning: getwd() possibly used unsafely; consider using getcwd()"); > > +"getwd() possibly used unsafely; consider using getcwd()"); > > Index: lib/libc/stdio/mktemp.c > > === > > RCS file: /cvs/src/lib/libc/stdio/mktemp.c,v > > retrieving revision 1.38 > > diff -u -p -r1.38 mktemp.c > > --- lib/libc/stdio/mktemp.c 13 Sep 2015 08:31:47 - 1.38 > > +++ lib/libc/stdio/mktemp.c 12 Nov 2017 04:21:37 - > > @@ -119,7 +119,7 @@ _mktemp(char *path) > > } > > > > __warn_references(mktemp, > > -"warning: mktemp() possibly used unsafely; consider using mkstemp()"); > > +"mktemp() possibly used unsafely; consider using mkstemp()"); > > > > char * > > mktemp(char *path) > > Index: lib/libc/stdio/sprintf.c > > === > > RCS file: /cvs/src/lib/libc/stdio/sprintf.c,v > > retrieving revision 1.18 > > diff -u -p -r1.18 sprintf.c > > --- lib/libc/stdio/sprintf.c1 Oct 2015 02:32:07 - 1.18 > > +++ lib/libc/stdio/sprintf.c12 Nov 2017 04:21:37 - > > @@ -39,7 +39,7 @@ > > > > #if defined(APIWARN) > > __warn_references(sprintf, > > -"warning: sprintf() is often misused, please use snprintf()"); > > +"sprintf() is often misused, please use snprintf()"); > > #endif > > > > int > > Index: lib/libc/stdio/tempnam.c > > === > > RCS file: /cvs/src/lib/libc/stdio/tempnam.c,v > > retrieving revision 1.19 > > diff -u -p -r1.19 tempnam.c > > --- lib/libc/stdio/tempnam.c31 Aug 2015 02:53:57 - 1.19 > > +++ lib/libc/stdio/tempnam.c12 Nov 2017 04:21:37 - > > @@ -37,7 +37,7 @@ > > #include > > > > __warn_references(tempnam, > > -"warning: tempnam() possibly used unsafely; consider using mkstemp()"); > > +"tempnam() possibly used unsafely; consider using mkstemp()"); > > > > char * > > tempnam(const char *dir, const char
relayd load certificates via fd
Instead of using imsg to pass certificates, pass the fd to the cert to the
relay processes. This allows for large certificates and esp. ca file to
work. OCSP stapling will also be added through this.
OK?
--
:wq Claudio
Index: ca.c
===
RCS file: /cvs/src/usr.sbin/relayd/ca.c,v
retrieving revision 1.28
diff -u -p -r1.28 ca.c
--- ca.c9 Aug 2017 21:31:16 - 1.28
+++ ca.c12 Aug 2017 14:05:44 -
@@ -109,18 +109,23 @@ void
ca_launch(void)
{
char hash[TLS_CERT_HASH_SIZE];
+ char*buf;
BIO *in = NULL;
EVP_PKEY*pkey = NULL;
struct relay*rlay;
X509*cert = NULL;
+ off_tlen;
TAILQ_FOREACH(rlay, env->sc_relays, rl_entry) {
if ((rlay->rl_conf.flags & (F_TLS|F_TLSCLIENT)) == 0)
continue;
- if (rlay->rl_conf.tls_cert_len) {
- if ((in = BIO_new_mem_buf(rlay->rl_tls_cert,
- rlay->rl_conf.tls_cert_len)) == NULL)
+ if (rlay->rl_tls_cert_fd != -1) {
+ if ((buf = relay_load_fd(rlay->rl_tls_cert_fd,
+ &len)) == NULL)
+ fatalx("ca_launch: cert");
+
+ if ((in = BIO_new_mem_buf(buf, len)) == NULL)
fatalx("ca_launch: cert");
if ((cert = PEM_read_bio_X509(in, NULL,
@@ -131,8 +136,7 @@ ca_launch(void)
BIO_free(in);
X509_free(cert);
- purge_key(&rlay->rl_tls_cert,
- rlay->rl_conf.tls_cert_len);
+ purge_key(&buf, len);
}
if (rlay->rl_conf.tls_key_len) {
if ((in = BIO_new_mem_buf(rlay->rl_tls_key,
@@ -153,9 +157,12 @@ ca_launch(void)
rlay->rl_conf.tls_key_len);
}
- if (rlay->rl_conf.tls_cacert_len) {
- if ((in = BIO_new_mem_buf(rlay->rl_tls_cacert,
- rlay->rl_conf.tls_cacert_len)) == NULL)
+ if (rlay->rl_tls_cacert_fd != -1) {
+ if ((buf = relay_load_fd(rlay->rl_tls_cacert_fd,
+ &len)) == NULL)
+ fatalx("ca_launch: cacert");
+
+ if ((in = BIO_new_mem_buf(buf, len)) == NULL)
fatalx("ca_launch: cacert");
if ((cert = PEM_read_bio_X509(in, NULL,
@@ -166,8 +173,7 @@ ca_launch(void)
BIO_free(in);
X509_free(cert);
- purge_key(&rlay->rl_tls_cacert,
- rlay->rl_conf.tls_cacert_len);
+ purge_key(&buf, len);
}
if (rlay->rl_conf.tls_cakey_len) {
if ((in = BIO_new_mem_buf(rlay->rl_tls_cakey,
@@ -187,6 +193,7 @@ ca_launch(void)
purge_key(&rlay->rl_tls_cakey,
rlay->rl_conf.tls_cakey_len);
}
+ close(rlay->rl_tls_ca_fd);
}
}
@@ -196,6 +203,9 @@ ca_dispatch_parent(int fd, struct privse
switch (imsg->hdr.type) {
case IMSG_CFG_RELAY:
config_getrelay(env, imsg);
+ break;
+ case IMSG_CFG_RELAY_FD:
+ config_getrelayfd(env, imsg);
break;
case IMSG_CFG_DONE:
config_getcfg(env, imsg);
Index: config.c
===
RCS file: /cvs/src/usr.sbin/relayd/config.c,v
retrieving revision 1.33
diff -u -p -r1.33 config.c
--- config.c14 Sep 2017 08:59:54 - 1.33
+++ config.c30 Sep 2017 16:52:36 -
@@ -47,10 +47,10 @@ config_init(struct relayd *env)
}
ps->ps_what[PROC_PARENT] = CONFIG_ALL;
- ps->ps_what[PROC_PFE] = CONFIG_ALL & ~CONFIG_PROTOS;
+ ps->ps_what[PROC_PFE] = CONFIG_ALL & ~(CONFIG_PROTOS|CONFIG_CERTS);
ps->ps_what[PROC_HCE] = CONFIG_TABLES;
- ps->ps_what[PROC_CA] = CONFIG_RELAYS;
- ps->ps_what[PROC_RELAY] = CONFIG_RELAYS|
+ ps->ps_what[PROC_CA] = CONFIG_RELAYS|CONFIG_CERTS;
+ ps->ps_what[PROC_RELAY] = CONFIG_RELAYS|CONFIG_CERTS|
CONFIG_TABLES|CONFIG_PROTOS|CONFIG_CA_ENGINE;
/* Other configuration */
@@ -771,6 +771,25 @@ config_getrule(struct relayd *env, struc
return (0);
}
+static int
+config_setrelayfd(struct privsep *ps, int id, int n, int rlay_id, int type,
+int ofd)
+{
+ struct ctl_relayfd rfd;
+ int fd;
+
+ rfd.relayid = rlay_id;
+ rfd.type = type;
+
+ if ((fd = dup(ofd)) == -1)
+ return (-1);
+ if (proc_comp
Re: relayd: 6.1-stable and relay_http.c rev 1.58
Here is setup which reproduces this problem. Also exists in 6.2.
Server:
Apache with mod_php serving following content:
———cut options.php——
{ 1.2.3.4 }
relay web_test {
listen on 5.6.7.8 port 80
protocol http_relay
forward to port 80 mode loadbalance check tcp
}
Client:
Runs php from CLI
file to run:
http://5.6.7.8/options.php');
curl_setopt($ch, CURLOPT_HEADER, true);
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "OPTIONS");
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);
curl_exec($ch);
echo PHP_EOL;
curl_setopt($ch, CURLOPT_URL, 'http://5.6.7.8/options.php');
curl_setopt($ch, CURLOPT_HEADER, true);
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "GET");
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);
curl_exec($ch);
echo PHP_EOL;
curl_setopt($ch, CURLOPT_URL, 'http://5.6.7.8');
curl_setopt($ch, CURLOPT_HEADER, true);
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "GET");
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);
curl_exec($ch);
echo PHP_EOL;
With rev. Before 1.58 of relay_http.c following can be observed:
relay web_test, session 1 (1 active), 0, 172.17.2.21 -> 172.16.1.30:80, done,
OPTIONS GET GET
With the rev. current for 6.2:
relay web_test, session 1 (1 active), 0, 172.17.2.21 -> 172.16.1.30:80, done,
OPTIONS
//mxb
> 22 okt. 2017 kl. 21:02 skrev Maxim Bourmistrov :
>
>
>> 22 okt. 2017 kl. 20:16 skrev Maxim Bourmistrov :
>>
>> Hey,
>> with rev 1.58 OPTIONS in relay_http.c got broken
>> or at least logic inside relay_read_http().
>> Quick fix it to cre->toread=0 and break, but this is probably not what
>> should be there.
>>
>> In my test case, from the client side I do an OPTIONS request, followed by a
>> couple of GET.
>> GET in the middle never gets catched and thus breaks intended usage.
>>
>> I’m doing a simple printf() debugging here to catch.
>>
>> cre->toread = strtonum(value, 0, LLONG_MAX, &errstr);
>> printf("-- to read %lld\n", cre->toread);
>>
>>
>> case HTTP_METHOD_GET:
>> printf("GOT GET to read: %lld\n", cre->toread);
>>
>> case HTTP_METHOD_OPTIONS:
>> printf("GOT OPT to read: %lld\n", cre->toread);
>>
>>
>> The output with those in place from ’relayd -d’:
>>
>> host 10.6.128.38, check http code (1ms,http code ok), state up -> up,
>> availability 100.00%
>> GOT OPT to read: -2
>> -- to read 0
>> -- to read 214
>> relay test_api_tls, session 1 (1 active), 0, 176.10.170.140 ->
>> 10.6.128.20:80, done, OPTIONS
>> GOT GET to read: -2
>> -- to read 214
>> relay test_api_tls, session 2 (1 active), 0, 176.10.170.140 ->
>> 10.6.128.36:80, done, GET
>> ^Chce exiting, pid 96033
>>
>> //mxb
>>
>
> With cre->toread=0 I catch it all
>
> relay test_api_tls, session 1 (1 active), 0, 176.10.170.140 ->
> 10.6.128.20:80, done, OPTIONS GET
> relay test_api_tls, session 2 (1 active), 0, 176.10.170.140 ->
> 10.6.128.36:80, done, GET
>
> //mxb
>
Re: hide wpakey from root by default
On Mon, Nov 27 2017, Stefan Sperling wrote:
> On Mon, Nov 27, 2017 at 02:33:59AM +0100, Stefan Sperling wrote:
>> Most people I've talked to seem to be OK with never exposing
>> these secrets to userland in the first place.
Makes sense.
> Better diff for the wireless part.
> WEP keys showed up as 0x0... instead of '' in
> the previous diff.
Yep. ifconfig(8) already handles EPERM properly. We can also kill dead
code in ifconfig(8) (see diff below). I guess that a bunch of wireless
management scripts, which parse the output of ifconfig(8) ran as root,
won't expect the <> notation, but we can live with this IMO.
ok jca@ (carp, sppp + revised 80211 changes)
Index: ifconfig.c
===
RCS file: /d/cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.351
diff -u -p -p -u -r1.351 ifconfig.c
--- ifconfig.c 17 Nov 2017 18:04:51 - 1.351
+++ ifconfig.c 27 Nov 2017 18:56:12 -
@@ -2124,70 +2124,12 @@ ieee80211_status(void)
}
}
- if (inwkey == 0 && nwkey.i_wepon > IEEE80211_NWKEY_OPEN) {
- fputs(" nwkey ", stdout);
- /* try to retrieve WEP keys */
- for (i = 0; i < IEEE80211_WEP_NKID; i++) {
- nwkey.i_key[i].i_keydat = keybuf[i];
- nwkey.i_key[i].i_keylen = sizeof(keybuf[i]);
- }
- if (ioctl(s, SIOCG80211NWKEY, (caddr_t)&nwkey) == -1) {
- fputs("", stdout);
- } else {
- nwkey_verbose = 0;
- /*
-* check to see non default key
-* or multiple keys defined
-*/
- if (nwkey.i_defkid != 1) {
- nwkey_verbose = 1;
- } else {
- for (i = 1; i < IEEE80211_WEP_NKID; i++) {
- if (nwkey.i_key[i].i_keylen != 0) {
- nwkey_verbose = 1;
- break;
- }
- }
- }
- /* check extra ambiguity with keywords */
- if (!nwkey_verbose) {
- if (nwkey.i_key[0].i_keylen >= 2 &&
- isdigit((unsigned
char)nwkey.i_key[0].i_keydat[0]) &&
- nwkey.i_key[0].i_keydat[1] == ':')
- nwkey_verbose = 1;
- else if (nwkey.i_key[0].i_keylen >= 7 &&
- strncasecmp("persist",
- (char *)nwkey.i_key[0].i_keydat, 7) == 0)
- nwkey_verbose = 1;
- }
- if (nwkey_verbose)
- printf("%d:", nwkey.i_defkid);
- for (i = 0; i < IEEE80211_WEP_NKID; i++) {
- if (i > 0)
- putchar(',');
- if (nwkey.i_key[i].i_keylen < 0) {
- fputs("persist", stdout);
- } else {
- /*
-* XXX
-* sanity check nwkey.i_key[i].i_keylen
-*/
- print_string(nwkey.i_key[i].i_keydat,
- nwkey.i_key[i].i_keylen);
- }
- if (!nwkey_verbose)
- break;
- }
- }
- }
+ if (inwkey == 0 && nwkey.i_wepon > IEEE80211_NWKEY_OPEN)
+ fputs(" nwkey ", stdout);
+
+ if (ipsk == 0 && psk.i_enabled)
+ fputs(" wpakey ", stdout);
- if (ipsk == 0 && psk.i_enabled) {
- fputs(" wpakey ", stdout);
- if (psk.i_enabled == 2)
- fputs("", stdout);
- else
- print_string(psk.i_psk, sizeof(psk.i_psk));
- }
if (iwpa == 0 && wpa.i_enabled) {
const char *sep;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: hide wpakey from root by default
On Mon, Nov 27, 2017 at 02:33:59AM +0100, Stefan Sperling wrote:
> Most people I've talked to seem to be OK with never exposing
> these secrets to userland in the first place.
Better diff for the wireless part.
WEP keys showed up as 0x0... instead of '' in
the previous diff.
Index: ieee80211_ioctl.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_ioctl.c,v
retrieving revision 1.57
diff -u -p -r1.57 ieee80211_ioctl.c
--- ieee80211_ioctl.c 6 Nov 2017 11:34:29 - 1.57
+++ ieee80211_ioctl.c 27 Nov 2017 17:12:32 -
@@ -252,8 +252,7 @@ static int
ieee80211_ioctl_getnwkeys(struct ieee80211com *ic,
struct ieee80211_nwkey *nwkey)
{
- struct ieee80211_key *k;
- int error, i;
+ int i;
if (ic->ic_flags & IEEE80211_F_WEPON)
nwkey->i_wepon = IEEE80211_NWKEY_WEP;
@@ -265,19 +264,8 @@ ieee80211_ioctl_getnwkeys(struct ieee802
for (i = 0; i < IEEE80211_WEP_NKID; i++) {
if (nwkey->i_key[i].i_keydat == NULL)
continue;
- /* do not show any keys to non-root user */
- if ((error = suser(curproc, 0)) != 0)
- return error;
- k = &ic->ic_nw_keys[i];
- if (k->k_cipher != IEEE80211_CIPHER_WEP40 &&
- k->k_cipher != IEEE80211_CIPHER_WEP104)
- nwkey->i_key[i].i_keylen = 0;
- else
- nwkey->i_key[i].i_keylen = k->k_len;
- error = copyout(k->k_key, nwkey->i_key[i].i_keydat,
- nwkey->i_key[i].i_keylen);
- if (error != 0)
- return error;
+ /* do not show any keys to userland */
+ return EPERM;
}
return 0;
}
@@ -491,14 +479,10 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
case SIOCG80211WPAPSK:
psk = (struct ieee80211_wpapsk *)data;
if (ic->ic_flags & IEEE80211_F_PSK) {
- psk->i_enabled = 1;
- /* do not show any keys to non-root user */
- if (suser(curproc, 0) != 0) {
- psk->i_enabled = 2;
- memset(psk->i_psk, 0, sizeof(psk->i_psk));
- break; /* return ok but w/o key */
- }
- memcpy(psk->i_psk, ic->ic_psk, sizeof(psk->i_psk));
+ /* do not show any keys to userland */
+ psk->i_enabled = 2;
+ memset(psk->i_psk, 0, sizeof(psk->i_psk));
+ break; /* return ok but w/o key */
} else
psk->i_enabled = 0;
break;
Re: dc.1: document non-portability of `e'
On Mon, Nov 27, 2017 at 10:34:50AM +, Jason McIntyre wrote: > On Sun, Nov 26, 2017 at 07:18:33PM +, kshe wrote: > > Hi, > > > > The manual page for dc(1) is very careful about signalling which > > commands are non-portable extensions, with the exception of the `e' > > command, which is a more recent addition. > > > > Index: dc.1 > > === > > RCS file: /cvs/src/usr.bin/dc/dc.1,v > > retrieving revision 1.30 > > diff -u -p -r1.30 dc.1 > > --- dc.123 Feb 2017 06:40:17 - 1.30 > > +++ dc.126 Oct 2017 04:44:01 - > > @@ -189,6 +189,7 @@ The top value on the stack is duplicated > > Equivalent to > > .Ic p , > > except that the output is written to the standard error stream. > > +This is a non-portable extension. > > .It Ic f > > All values on the stack are printed, separated by newlines. > > .It Ic G > > > > Regards, > > > > kshe > > > > morning. > > posix doesn;t describe a separate dc utility, as far as i can see. the > STANDARDS section of dc(1) discusses only the "arithmetic operations" of > dc. > > bc(1) however does describe -e as an extension, in STANDARDS. > > i think here dc(1) is a bit of a special case, but i think how it is now > is fine. i definitely don;t want to start plastering "this is an > extension" onto the end of every option not described by posix. > > jmc This is not about the -e flag but about the 'e' command. The frame of reference is the set of command implemented by the original dc(1). I think it is a good think to mention that 'e' is non-portable. -Otto
reduce bufferevent abuse by relayd
relayd does this evil thing of replacing the output buffer of bufferevnt
with another one. This makes it impossible to reopen connections. Instead
just write out the output buffer when connecting to the backend and move
on.
THis is needed to make HTTP keep-alive work better.
--
:wq Claudio
Index: relay.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.228
diff -u -p -r1.228 relay.c
--- relay.c 27 Nov 2017 03:40:04 - 1.228
+++ relay.c 27 Nov 2017 17:03:34 -
@@ -712,10 +712,11 @@ relay_connected(int fd, short sig, void
"failed to allocate output buffer event", 0);
return;
}
- evbuffer_free(bev->output);
- bev->output = con->se_out.output;
- if (bev->output == NULL)
- fatal("%s: invalid output buffer", __func__);
+ /* write pending output buffer now */
+ if (bufferevent_write_buffer(bev, con->se_out.output)) {
+ relay_abort_http(con, 500, strerror(errno), 0);
+ return;
+ }
con->se_out.bev = bev;
/* Initialize the TLS wrapper */
@@ -1698,7 +1699,7 @@ relay_close(struct rsession *con, const
free(con->se_priv);
if (con->se_in.bev != NULL)
bufferevent_free(con->se_in.bev);
- else if (con->se_in.output != NULL)
+ if (con->se_in.output != NULL)
evbuffer_free(con->se_in.output);
if (con->se_in.tls != NULL)
tls_close(con->se_in.tls);
@@ -1720,7 +1721,7 @@ relay_close(struct rsession *con, const
if (con->se_out.bev != NULL)
bufferevent_free(con->se_out.bev);
- else if (con->se_out.output != NULL)
+ if (con->se_out.output != NULL)
evbuffer_free(con->se_out.output);
if (con->se_out.tls != NULL)
tls_close(con->se_out.tls);
Re: ksh(1): kill the "version" function
On Sun, 26 Nov 2017 21:55:21 +0100 Jeremie Courreges-Anglas wrote: > Yup. Before someone proposes to remove or significantly change the > content of the version string: please think about shell scripts out > there that might use KSH_VERSION. Long thread from Feb 2015: http://openbsd-archive.7691.n7.nabble.com/ksh-version-lies-td265560.html
Re: hide wpakey from root by default
On 2017/11/27 11:22, Peter Hessler wrote: > :retrieving revision 1.319 > :diff -u -p -r1.319 ip_carp.c > :--- netinet/ip_carp.c21 Nov 2017 09:08:55 - 1.319 > :+++ netinet/ip_carp.c27 Nov 2017 01:29:34 - > :@@ -2158,9 +2158,8 @@ carp_ioctl(struct ifnet *ifp, u_long cmd > : } > : carpr.carpr_advbase = sc->sc_advbase; > : carpr.carpr_balancing = sc->sc_balancing; > :-if (suser(p, 0) == 0) > :-bcopy(sc->sc_key, carpr.carpr_key, > :-sizeof(carpr.carpr_key)); > :+/* do not show any keys to userland */ > :+memset(carpr.carpr_key, 0, sizeof(carpr.carpr_key)); > : carpr.carpr_peer.s_addr = sc->sc_peer.s_addr; > : error = copyout(&carpr, ifr->ifr_data, sizeof(carpr)); > : break; > : > > Best I can tell, ifconfig carp doesn't tell you the key anyways, so OK. ifconfig doesn't. shells/nsh will be a bit of a pain to fix though.
Re: FUSE: link should return EPERM if the source file is a directory
On Mon, Nov 27 2017, Helg wrote:
> fusefs_link returns the wrong error code when attempting to create a
> hard link to a directory. It returns EISDIR when it should instead
> return EPERM. Discovered while running the ffs test suite on ntfs-3g
> and confirmed by comparing to ufs.
>
> This is the description for the test that fails:
>
>
> "link returns EPERM if the source file is a directory"
>
> This patch changes the error code to EPERM.
>
> ok?
ok jca@
> Index: fuse_vnops.c
> ===
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.35
> diff -u -p -u -p -r1.35 fuse_vnops.c
> --- fuse_vnops.c 27 Nov 2017 12:54:13 - 1.35
> +++ fuse_vnops.c 27 Nov 2017 13:49:15 -
> @@ -565,7 +565,7 @@ fusefs_link(void *v)
> }
> if (vp->v_type == VDIR) {
> VOP_ABORTOP(dvp, cnp);
> - error = EISDIR;
> + error = EPERM;
> goto out2;
> }
> if (dvp->v_mount != vp->v_mount) {
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: isakmpd: use monotonic clock for event timeouts
On Fri, Nov 24 2017, Scott Cheloha wrote:
> Hi,
>
> These events are meants to fire after an interval has elapsed,
> so we should use the monotonic clock to measure.
>
> The pattern throughout the daemon of loading the current time,
> adding a timeout to the structure member, and then passing a
> pointer to said structure to timer_add_event() seemed redundant,
> so I modified timer_add_event() to accept the timeout in seconds
> and populate the expiration structure itself before inserting
> the event into the event list.
>
> A number of files no longer need , so I dropped those
> #includes. ipsec.c doesn't use anything in timer.h so I dropped it.
>
> I didn't touch log.c because it seems to work just fine as-is with
> gettimeofday(2).
>
> I don't have a real IPSec gateway set up to test this with, but my
> testing with a dummy configuration shows that with the patch the
> events fire punctually, regardless of what I've done to the system
> clock in the meanwhile.
>
> Thoughts and feedback?
This seems to mix refactoring, eg changing the signature of
timer_add_event(), with semantic changes. Could you please
split your diff in seperate steps easier to review?
(I find that ports/devel/quilt helps a lot maintain a stack of patches.)
> --
> Scott Cheloha
>
> Index: sbin/isakmpd/connection.c
> ===
> RCS file: /cvs/src/sbin/isakmpd/connection.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 connection.c
> --- sbin/isakmpd/connection.c 6 Aug 2017 13:54:04 - 1.38
> +++ sbin/isakmpd/connection.c 24 Nov 2017 20:54:37 -
> @@ -31,8 +31,8 @@
> */
>
> #include
> -#include
> #include
> +
> #include
> #include
>
> @@ -71,7 +71,7 @@ struct connection_passive {
> /* XXX Potential additions to 'connection_passive'. */
> char *isakmp_peer;
> struct sa *sa; /* XXX "Soft" ref to active sa? */
> - struct timeval sa_expiration; /* XXX *sa may expire. */
> + struct timespec sa_expiration; /* XXX *sa may expire. */
> #endif
> };
>
> @@ -144,15 +144,13 @@ connection_init(void)
> static void
> connection_checker(void *vconn)
> {
> - struct timeval now;
> struct connection *conn = vconn;
> char *name;
> + int interval;
>
> - gettimeofday(&now, 0);
> - now.tv_sec += conf_get_num("General", "check-interval",
> - CHECK_INTERVAL);
> + interval = conf_get_num("General", "check-interval", CHECK_INTERVAL);
> conn->ev = timer_add_event("connection_checker",
> - connection_checker, conn, &now);
> + connection_checker, conn, interval);
> if (!conn->ev)
> log_print("%s: could not add timer event", __func__);
> if (ui_daemon_passive)
> @@ -272,7 +270,6 @@ int
> connection_setup(char *name)
> {
> struct connection *conn = 0;
> - struct timeval now;
>
> /* Check for trials to add duplicate connections. */
> if (connection_lookup(name)) {
> @@ -291,9 +288,8 @@ connection_setup(char *name)
> log_error("connection_setup: strdup (\"%s\") failed", name);
> goto fail;
> }
> - gettimeofday(&now, 0);
> conn->ev = timer_add_event("connection_checker", connection_checker,
> - conn, &now);
> + conn, 0);
> if (!conn->ev) {
> log_print("connection_setup: could not add timer event");
> goto fail;
> @@ -405,11 +401,11 @@ void
> connection_report(void)
> {
> struct connection *conn;
> - struct timeval now;
> + struct timespec now;
> struct connection_passive *pconn;
> struct doi *doi = doi_lookup(ISAKMP_DOI_ISAKMP);
>
> - gettimeofday(&now, 0);
> + clock_gettime(CLOCK_MONOTONIC, &now);
> for (conn = TAILQ_FIRST(&connections); conn;
> conn = TAILQ_NEXT(conn, link))
> LOG_DBG((LOG_REPORT, 0,
> Index: sbin/isakmpd/dpd.c
> ===
> RCS file: /cvs/src/sbin/isakmpd/dpd.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 dpd.c
> --- sbin/isakmpd/dpd.c10 Dec 2015 17:27:00 - 1.19
> +++ sbin/isakmpd/dpd.c24 Nov 2017 20:54:37 -
> @@ -216,23 +216,18 @@ dpd_timer_interval(u_int32_t offset)
> static void
> dpd_timer_reset(struct sa *sa, u_int32_t time_passed, enum dpd_tstate mode)
> {
> - struct timeval tv;
> -
> if (sa->dpd_event)
> timer_remove_event(sa->dpd_event);
>
> - gettimeofday(&tv, 0);
> switch (mode) {
> case DPD_TIMER_NORMAL:
> sa->dpd_failcount = 0;
> - tv.tv_sec += dpd_timer_interval(time_passed);
> sa->dpd_event = timer_add_event("dpd_event", dpd_event, sa,
> - &tv);
> + dpd_timer_interval(time_passed));
> break;
> case DPD_TIMER_CHECK:
> - tv.tv_sec += DPD_RETRANS_WAIT;
> sa->
Re: hide wpakey from root by default
On 2017 Nov 27 (Mon) at 16:27:57 +0100 (+0100), Stefan Sperling wrote: :On Mon, Nov 27, 2017 at 11:22:46AM +0100, Peter Hessler wrote: :> This hides the username that is used, not the password/authkey. Is the :> username private information? : :Yes it seems best to avoid exposing these. :We cannot assume that telcos follow best practices of data hygiene. In that case, OK for that chunk too -- Help stamp out and abolish redundancy.
FUSE: link should return EPERM if the source file is a directory
fusefs_link returns the wrong error code when attempting to create a
hard link to a directory. It returns EISDIR when it should instead
return EPERM. Discovered while running the ffs test suite on ntfs-3g
and confirmed by comparing to ufs.
This is the description for the test that fails:
"link returns EPERM if the source file is a directory"
This patch changes the error code to EPERM.
ok?
Index: fuse_vnops.c
===
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.35
diff -u -p -u -p -r1.35 fuse_vnops.c
--- fuse_vnops.c27 Nov 2017 12:54:13 - 1.35
+++ fuse_vnops.c27 Nov 2017 13:49:15 -
@@ -565,7 +565,7 @@ fusefs_link(void *v)
}
if (vp->v_type == VDIR) {
VOP_ABORTOP(dvp, cnp);
- error = EISDIR;
+ error = EPERM;
goto out2;
}
if (dvp->v_mount != vp->v_mount) {
Re: hide wpakey from root by default
On Mon, Nov 27, 2017 at 11:22:46AM +0100, Peter Hessler wrote: > This hides the username that is used, not the password/authkey. Is the > username private information? Yes it seems best to avoid exposing these. We cannot assume that telcos follow best practices of data hygiene. Quoting naddy@ from earlier in this thread: [[[ "But ppp isn't a radio..." You don't know the details. They're in the process of changing it, but for instance Deutsche Telekom's DSL used authentication information for PPPoE that would also be the default access to the online customer account settings. ]]]
Re: Add Diffie-Hellman group negotiation to iked
On Wed, Nov 22, 2017 at 05:26:24PM +0100, Patrick Wildt wrote:
> On 2017/06/25 21:44, Tim Stewart wrote:
> > My first patch did, in fact, break Child SAs rekeying. I have a new
> > patch at the end of this message that simply restricts DH group
> > negotiation to IKE SAs (I *think* that DH group guessing only applies to
> > IKE SAs, and perhaps only the IKE_SA_INIT exchange, but I'm still
> > working through the RFC). This may not be the ultimate solution, but it
> > does allow us to move forward.
>
> Reading RFC 7296 it looks like throwing INVALID_KE_PAYLOAD is fine for
> both establishing the IKE SA and rekeying the Child SAs. If we select a
> proposal from the msg that uses a different DH group than the one that's
> used in the KEi (in the same msg) we need to throw INVALID_KE_PAYLOAD.
>
> Since all messages subsequent to the initial exchange must be encrypted,
> the INVALID_KE_PAYLOAD message on rekeying Child SAs must be encrypted.
> Apparently with the previous diff the Child SA rekeying failed. This is
> because the code sends the INVALID_KE_PAYLOAD response unencrypted.
>
> Also I have found inconsistencies in handling INVALID_KE_PAYLOAD with us
> acting as initiator. I will take a look at both cases and will follow
> up.
>
> Patrick
Hi,
This diff adds suport to rejecting IKE SA messages. This means that we
can reply to IKE SA INIT messages with no proposal chosen, as we already
do for Child SAs. For that the error "adding" is done in a new function
shared by both send error handlers. We need two "send error" functions
because the init error is unencrypted, while all later ones are not.
Now we can add more cases, like Child SA not found or that the DH group
is not what we expect. I think that is quite an improvement.
One "issue" is retransmits. The RFC specifies that IKEv2 is a reliable
protocol. When a packet is lost, the initiator has to retransmit the
request and the responder must retransmit the saved reply. On IKE SA
INIT error we don't save our response, which means that we will handle
the request as if we never had it. The RFC says we shouldn't do that,
because DoS protection and so on. What we should do is keep our reply
around and check if it is indeed a retransmit. If so we reply with the
same reply, thus saving resources. If it is an attempt to create a new
IKE SA, drop the old SA so that iked(8) can attempt to create a new SA.
Thoughts?
Patrick
diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index b536d58e157..050277a2530 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -502,6 +502,7 @@ struct iked_message {
struct iked_proposalsmsg_proposals;
struct iked_spi msg_rekey;
struct ibuf *msg_nonce; /* dh NONCE */
+ uint16_t msg_dhgroup; /* dh group */
struct ibuf *msg_ke;/* dh key exchange */
struct iked_id msg_auth; /* AUTH payload */
struct iked_id msg_id;
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index e908b6230a5..234f8329791 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -76,6 +76,7 @@ intikev2_resp_ike_eap(struct iked *, struct iked_sa *,
struct ibuf *);
int ikev2_send_auth_failed(struct iked *, struct iked_sa *);
int ikev2_send_error(struct iked *, struct iked_sa *,
struct iked_message *, uint8_t);
+int ikev2_send_init_error(struct iked *, struct iked_message *);
int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
struct iked_spi *, uint8_t);
@@ -125,6 +126,7 @@ ssize_t ikev2_add_certreq(struct ibuf *, struct
ikev2_payload **, ssize_t,
ssize_t ikev2_add_ipcompnotify(struct iked *, struct ibuf *,
struct ikev2_payload **, ssize_t, struct iked_sa *);
ssize_t ikev2_add_ts_payload(struct ibuf *, unsigned int, struct
iked_sa *);
+ssize_t ikev2_add_error(struct iked *, struct ibuf *, struct
iked_message *);
int ikev2_add_data(struct ibuf *, void *, size_t);
int ikev2_add_buf(struct ibuf *buf, struct ibuf *);
@@ -446,6 +448,23 @@ ikev2_recv(struct iked *env, struct iked_message *msg)
if ((m = ikev2_msg_lookup(env, &sa->sa_requests, msg, hdr)))
ikev2_msg_dispose(env, &sa->sa_requests, m);
} else {
+ /*
+* IKE_SA_INIT is special since it always uses the message id 0.
+* Even when the message was rejected, and the new message has
+* different proposals, the id will be the same. To discern
+* retransmits and new messages, the RFC suggests to compare the
+* the messages.
+*/
+ if (sa->sa_state == IKEV2_STATE_CLOSED && sa->sa_1stmsg &&
+ hdr->ike_exchange == IKEV2_EXCHANGE_IKE_SA_INIT &&
+ msg->msg_msgid == 0 &&
+ (ibuf_length(msg->msg_data) != ibuf_length(sa->s
Re: filedesc's locking.
On 27/11/17(Mon) 11:49, Mathieu - wrote: > Hi everyone, > > I was looking / poking around the filedesc handling in kern_descrip.c > and found the locking a bit.. weird, Can you define "weird"? Is it a taste thing or did you find any bug? If it's a bug how can you reproduce it? > especially the fd_getfile function > is touching protected members of the filedesc w/o taking any lock. Which "protected members" are you talking about? What makes you think they are protected? What do you mean with protected? Why do you think they need a lock? > This has been already hit previously in [1] and fixed by reordering the > malloc / free operations in rev 1.138 of kern_descrip.c ([2]) The bug that has been found has been fixed. What are you trying to do? > At the time it was proposed to actually lock fd_getfile which entailed > fixing all the callers. This could be useful for a different reason. Can you guess which one? > I tried to go this way but it proved difficult > as sometimes the locking is done one level higher (e.g namei() being > called with or without the filedesc lock held). > called with or without the filedesc lock held). Introducing a _locked > version of the file was also not feasible for the same reasons (and > anyway it was a bit ugly). Yes it's a complicated task. > So I went down the rabbit hole and tried changing the semantics of the > whole filedesc manipulations in order to make this clearer. Can you define "this"? > [...] > Next step imho would be to get rid of the FIF_LARVAL state. Why? > I know the diff is a bit big and scary because it touches sensitive > paths... but I wanted to have an opinion on it before going further. Before you go any further you need: - to explain what you want to achieve - to have a way to test your changes - to convince us that this is what we want Moving the lock above a level might be something we want. But every time you move the lock/unlock dance to a caller function you might introduce a deadlock. So a big diff is not the way to go. You might also not need to fix all the code paths at once. It depends what you're trying to achieve. > It has been running here for a few days, on a workstation, I also ran > the regress tests w/o any regressions. On which filesystems did you build the system? Which threaded application did you ran? I'd suggest you to split your changes in small reviewable diffs. Make sure you have a goal. Make sure you can test it. You might need to write tests for that.
Re: race-less nd6_timer
On Mon, Nov 27, 2017 at 10:43:09AM +0100, Martin Pieuchot wrote:
> Here's a diff that includes that and prevent a user-after-free pointed
> out by visa@. We should not try to dereference `rt' if nd6_free() has
> been called.
>
> Hrvoje Popovski confirmed he couldn't reproduce the panic with this
> diff.
>
> ok?
OK bluhm@
> Index: netinet6/nd6.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.221
> diff -u -p -r1.221 nd6.c
> --- netinet6/nd6.c31 Oct 2017 22:05:13 - 1.221
> +++ netinet6/nd6.c27 Nov 2017 09:35:41 -
> @@ -67,7 +67,8 @@
> #define ND6_RECALC_REACHTM_INTERVAL (60 * 120) /* 2 hours */
>
> /* timer values */
> -time_t nd6_expire_time = -1; /* at which time_uptime nd6_expire runs
> */
> +int nd6_timer_next = -1; /* at which time_uptime nd6_timer runs */
> +time_t nd6_expire_next = -1; /* at which time_uptime nd6_expire runs
> */
> int nd6_delay = 5;/* delay first probe time 5 second */
> int nd6_umaxtries = 3;/* maximum unicast query */
> int nd6_mmaxtries = 3;/* maximum multicast query */
> @@ -90,13 +91,15 @@ int nd6_inuse, nd6_allocated;
>
> int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL;
>
> +void nd6_timer(void *);
> void nd6_slowtimo(void *);
> void nd6_expire(void *);
> void nd6_expire_timer(void *);
> void nd6_invalidate(struct rtentry *);
> void nd6_free(struct rtentry *);
> -void nd6_llinfo_timer(void *);
> +int nd6_llinfo_timer(struct rtentry *);
>
> +struct timeout nd6_timer_to;
> struct timeout nd6_slowtimo_ch;
> struct timeout nd6_expire_timeout;
> struct task nd6_expire_task;
> @@ -120,6 +123,7 @@ nd6_init(void)
> nd6_init_done = 1;
>
> /* start timer */
> + timeout_set_proc(&nd6_timer_to, nd6_timer, &nd6_timer_to);
> timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
> timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
> timeout_set(&nd6_expire_timeout, nd6_expire_timer, NULL);
> @@ -297,45 +301,66 @@ skip1:
> * ND6 timer routine to handle ND6 entries
> */
> void
> -nd6_llinfo_settimer(struct llinfo_nd6 *ln, int secs)
> +nd6_llinfo_settimer(struct llinfo_nd6 *ln, unsigned int secs)
> {
> - if (secs < 0) {
> - ln->ln_rt->rt_expire = 0;
> - timeout_del(&ln->ln_timer_ch);
> - } else {
> - ln->ln_rt->rt_expire = time_uptime + secs;
> - timeout_add_sec(&ln->ln_timer_ch, secs);
> + time_t expire = time_uptime + secs;
> +
> + NET_ASSERT_LOCKED();
> +
> + ln->ln_rt->rt_expire = expire;
> + if (!timeout_pending(&nd6_timer_to) || expire < nd6_timer_next) {
> + nd6_timer_next = expire;
> + timeout_add_sec(&nd6_timer_to, secs);
> }
> }
>
> void
> -nd6_llinfo_timer(void *arg)
> +nd6_timer(void *arg)
> {
> - struct llinfo_nd6 *ln;
> - struct rtentry *rt;
> - struct sockaddr_in6 *dst;
> - struct ifnet *ifp;
> - struct nd_ifinfo *ndi = NULL;
> + struct llinfo_nd6 *ln, *nln;
> + time_t expire = time_uptime + nd6_gctimer;
> + int secs;
>
> NET_LOCK();
> + TAILQ_FOREACH_SAFE(ln, &nd6_list, ln_list, nln) {
> + struct rtentry *rt = ln->ln_rt;
>
> - ln = (struct llinfo_nd6 *)arg;
> + if (rt->rt_expire && rt->rt_expire <= time_uptime)
> + if (nd6_llinfo_timer(rt))
> + continue;
>
> - if ((rt = ln->ln_rt) == NULL)
> - panic("ln->ln_rt == NULL");
> - if ((ifp = if_get(rt->rt_ifidx)) == NULL) {
> - NET_UNLOCK();
> - return;
> + if (rt->rt_expire && rt->rt_expire < expire)
> + expire = rt->rt_expire;
> }
> - ndi = ND_IFINFO(ifp);
> - dst = satosin6(rt_key(rt));
>
> - /* sanity check */
> - if (rt->rt_llinfo != NULL && (struct llinfo_nd6 *)rt->rt_llinfo != ln)
> - panic("rt_llinfo(%p) is not equal to ln(%p)",
> - rt->rt_llinfo, ln);
> - if (!dst)
> - panic("dst=0 in nd6_timer(ln=%p)", ln);
> + secs = expire - time_uptime;
> + if (secs < 0)
> + secs = 0;
> + if (!TAILQ_EMPTY(&nd6_list))
> + timeout_add_sec(&nd6_timer_to, secs);
> +
> + NET_UNLOCK();
> +}
> +
> +/*
> + * ND timer state handling.
> + *
> + * Returns 1 if `rt' should no longer be used, 0 otherwise.
> + */
> +int
> +nd6_llinfo_timer(struct rtentry *rt)
> +{
> + struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
> + struct sockaddr_in6 *dst = satosin6(rt_key(rt));
> + struct ifnet *ifp;
> + struct nd_ifinfo *ndi = NULL;
> +
> + NET_ASSERT_LOCKED();
> +
> + if ((ifp = if_get(rt->rt_ifidx)) == NULL)
> + return 1;
> +
> + ndi = ND_IFINFO(ifp);
>
> switch (ln->ln_state) {
> case ND6_LLINFO_INCOMPLETE:
> @@ -408,7 +433,8 @@ nd6_ll
Re: TCP/UDP/etc input w/o KERNEL_LOCK()
On Mon, Nov 27, 2017 at 12:20:34PM +0100, Martin Pieuchot wrote:
> Questions, comments, tests?
New panic with regress. I think it was sys/kern/sosplice this time.
login: panic: kernel diagnostic assertion "_kernel_lock_held()" failed: file
"/usr/src/sys/kern/uipc_socket.c", line 1882
Stopped at db_enter+0x4: popl%ebp
TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
124975 87761 0 0x2 00 perl
* 64725 54083 0 0x14000 0x2001 softnet
db_enter() at db_enter+0x4
panic() at panic+0xcc
__assert(d09ca4c9,d0b8b601,75a,d0a6b5e8) at __assert+0x19
sohasoutofband(d77dea8c) at sohasoutofband+0x4b
tcp_input(f547e27c,f547e278,6,2) at tcp_input+0x2ba2
ip_deliver(f547e27c,f547e278,6,2) at ip_deliver+0x21f
ip_local(f547e27c,f547e278,d06dcd6a,0) at ip_local+0x139
ipintr() at ipintr+0x54
if_netisr(0) at if_netisr+0x5a
taskq_thread(d7810080) at taskq_thread+0x6c
https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.
bluhm
> Index: kern/sys_socket.c
> ===
> RCS file: /cvs/src/sys/kern/sys_socket.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 sys_socket.c
> --- kern/sys_socket.c 14 Nov 2017 16:01:55 - 1.34
> +++ kern/sys_socket.c 27 Nov 2017 10:46:15 -
> @@ -166,11 +166,11 @@ soo_poll(struct file *fp, int events, st
> if (revents == 0) {
> if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
> selrecord(p, &so->so_rcv.sb_sel);
> - so->so_rcv.sb_flagsintr |= SB_SEL;
> + so->so_rcv.sb_flags |= SB_SEL;
> }
> if (events & (POLLOUT | POLLWRNORM)) {
> selrecord(p, &so->so_snd.sb_sel);
> - so->so_snd.sb_flagsintr |= SB_SEL;
> + so->so_snd.sb_flags |= SB_SEL;
> }
> }
> sounlock(s);
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 uipc_socket.c
> --- kern/uipc_socket.c23 Nov 2017 13:45:46 - 1.209
> +++ kern/uipc_socket.c27 Nov 2017 10:53:44 -
> @@ -135,6 +135,8 @@ socreate(int dom, struct socket **aso, i
> so->so_egid = p->p_ucred->cr_gid;
> so->so_cpid = p->p_p->ps_pid;
> so->so_proto = prp;
> + task_set(&so->so_rcv.sb_wtask, sorwakeup_cb, so);
> + task_set(&so->so_snd.sb_wtask, sowwakeup_cb, so);
>
> s = solock(so);
> error = (*prp->pr_attach)(so, proto);
> @@ -194,7 +196,8 @@ sofree(struct socket *so)
> {
> soassertlocked(so);
>
> - if (so->so_pcb || (so->so_state & SS_NOFDREF) == 0)
> + if ((so->so_pcb != NULL) ||
> + (so->so_state & (SS_NOFDREF|SS_NONOTIF)) != (SS_NOFDREF|SS_NONOTIF))
> return;
> if (so->so_head) {
> /*
> @@ -273,8 +276,15 @@ drop:
> error = error2;
> }
> discard:
> - if (so->so_state & SS_NOFDREF)
> - panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
> + KASSERT((so->so_state & (SS_NOFDREF|SS_NONOTIF)) == 0);
> + /* Make sure possible delayed notification are delivered. */
> + so->so_state |= SS_NONOTIF;
> + if (!task_del(systq, &so->so_rcv.sb_wtask) ||
> + !task_del(systq, &so->so_snd.sb_wtask)) {
> + NET_UNLOCK();
> + taskq_barrier(systq);
> + NET_LOCK();
> + }
> so->so_state |= SS_NOFDREF;
> sofree(so);
> sounlock(s);
> @@ -296,9 +306,8 @@ soaccept(struct socket *so, struct mbuf
> int error = 0;
>
> soassertlocked(so);
> + KASSERT(so->so_state & SS_NOFDREF);
>
> - if ((so->so_state & SS_NOFDREF) == 0)
> - panic("soaccept !NOFDREF: so %p, so_type %d", so, so->so_type);
> so->so_state &= ~SS_NOFDREF;
> if ((so->so_state & SS_ISDISCONNECTED) == 0 ||
> (so->so_proto->pr_flags & PR_ABRTACPTDIS) == 0)
> @@ -1052,12 +1061,8 @@ sorflush(struct socket *so)
> sbunlock(so, sb);
> aso.so_proto = pr;
> aso.so_rcv = *sb;
> - memset(sb, 0, sizeof (*sb));
> - /* XXX - the memset stomps all over so_rcv */
> - if (aso.so_rcv.sb_flags & SB_KNOTE) {
> - sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
> - sb->sb_flags = SB_KNOTE;
> - }
> + memset(&sb->sb_startzero, 0,
> + (caddr_t)&sb->sb_endzero - (caddr_t)&sb->sb_startzero);
> if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
> (*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
> sbrelease(&aso, &aso.so_rcv);
> @@ -1178,8 +1183,8 @@ sosplice(struct socket *so, int fd, off_
>* we sleep, the socket buffers are not marked as spliced yet.
>*/
> if (somove(so
Re: TCP/UDP/etc input w/o KERNEL_LOCK()
On 20/11/17(Mon) 16:22, Martin Pieuchot wrote:
> Diff below remove the KERNEL_LOCK() around all pr_input() routines.
> It's a bit rough so I'd appreciate more tests before splitting it into
> pieces.
>
> I'm using tasks to delay selwakeup/csignal calls, just like I did for
> bpf(4).
Updated version that should fix previous issues.
I'm using a new flag to prevent notification tasks to be scheduled.
Reusing SS_NOFDREF would lead to a race in sofree() since we need to
release the NET_LOCK() around taskq_barrier().
I'm also merging `sb_flagsintr' and `sb_flags'. Both of them are
currently protected by the KERNEL_LOCK(). I'll plan to use atomic
set/clear bits operations to avoid taking the socketlock() when
setting SB_KNOTE.
Questions, comments, tests?
Index: kern/sys_socket.c
===
RCS file: /cvs/src/sys/kern/sys_socket.c,v
retrieving revision 1.34
diff -u -p -r1.34 sys_socket.c
--- kern/sys_socket.c 14 Nov 2017 16:01:55 - 1.34
+++ kern/sys_socket.c 27 Nov 2017 10:46:15 -
@@ -166,11 +166,11 @@ soo_poll(struct file *fp, int events, st
if (revents == 0) {
if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
selrecord(p, &so->so_rcv.sb_sel);
- so->so_rcv.sb_flagsintr |= SB_SEL;
+ so->so_rcv.sb_flags |= SB_SEL;
}
if (events & (POLLOUT | POLLWRNORM)) {
selrecord(p, &so->so_snd.sb_sel);
- so->so_snd.sb_flagsintr |= SB_SEL;
+ so->so_snd.sb_flags |= SB_SEL;
}
}
sounlock(s);
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_socket.c
--- kern/uipc_socket.c 23 Nov 2017 13:45:46 - 1.209
+++ kern/uipc_socket.c 27 Nov 2017 10:53:44 -
@@ -135,6 +135,8 @@ socreate(int dom, struct socket **aso, i
so->so_egid = p->p_ucred->cr_gid;
so->so_cpid = p->p_p->ps_pid;
so->so_proto = prp;
+ task_set(&so->so_rcv.sb_wtask, sorwakeup_cb, so);
+ task_set(&so->so_snd.sb_wtask, sowwakeup_cb, so);
s = solock(so);
error = (*prp->pr_attach)(so, proto);
@@ -194,7 +196,8 @@ sofree(struct socket *so)
{
soassertlocked(so);
- if (so->so_pcb || (so->so_state & SS_NOFDREF) == 0)
+ if ((so->so_pcb != NULL) ||
+ (so->so_state & (SS_NOFDREF|SS_NONOTIF)) != (SS_NOFDREF|SS_NONOTIF))
return;
if (so->so_head) {
/*
@@ -273,8 +276,15 @@ drop:
error = error2;
}
discard:
- if (so->so_state & SS_NOFDREF)
- panic("soclose NOFDREF: so %p, so_type %d", so, so->so_type);
+ KASSERT((so->so_state & (SS_NOFDREF|SS_NONOTIF)) == 0);
+ /* Make sure possible delayed notification are delivered. */
+ so->so_state |= SS_NONOTIF;
+ if (!task_del(systq, &so->so_rcv.sb_wtask) ||
+ !task_del(systq, &so->so_snd.sb_wtask)) {
+ NET_UNLOCK();
+ taskq_barrier(systq);
+ NET_LOCK();
+ }
so->so_state |= SS_NOFDREF;
sofree(so);
sounlock(s);
@@ -296,9 +306,8 @@ soaccept(struct socket *so, struct mbuf
int error = 0;
soassertlocked(so);
+ KASSERT(so->so_state & SS_NOFDREF);
- if ((so->so_state & SS_NOFDREF) == 0)
- panic("soaccept !NOFDREF: so %p, so_type %d", so, so->so_type);
so->so_state &= ~SS_NOFDREF;
if ((so->so_state & SS_ISDISCONNECTED) == 0 ||
(so->so_proto->pr_flags & PR_ABRTACPTDIS) == 0)
@@ -1052,12 +1061,8 @@ sorflush(struct socket *so)
sbunlock(so, sb);
aso.so_proto = pr;
aso.so_rcv = *sb;
- memset(sb, 0, sizeof (*sb));
- /* XXX - the memset stomps all over so_rcv */
- if (aso.so_rcv.sb_flags & SB_KNOTE) {
- sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
- sb->sb_flags = SB_KNOTE;
- }
+ memset(&sb->sb_startzero, 0,
+ (caddr_t)&sb->sb_endzero - (caddr_t)&sb->sb_startzero);
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
(*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
sbrelease(&aso, &aso.so_rcv);
@@ -1178,8 +1183,8 @@ sosplice(struct socket *so, int fd, off_
* we sleep, the socket buffers are not marked as spliced yet.
*/
if (somove(so, M_WAIT)) {
- so->so_rcv.sb_flagsintr |= SB_SPLICE;
- sosp->so_snd.sb_flagsintr |= SB_SPLICE;
+ so->so_rcv.sb_flags |= SB_SPLICE;
+ sosp->so_snd.sb_flags |= SB_SPLICE;
}
release:
@@ -1196,8 +1201,8 @@ sounsplice(struct socket *so, struct soc
task_del(sosplice_taskq, &so->so_splicetask);
filedesc's locking.
Hi everyone,
I was looking / poking around the filedesc handling in kern_descrip.c
and found the locking a bit.. weird, especially the fd_getfile function
is touching protected members of the filedesc w/o taking any lock.
This has been already hit previously in [1] and fixed by reordering the
malloc / free operations in rev 1.138 of kern_descrip.c ([2])
At the time it was proposed to actually lock fd_getfile which entailed
fixing all the callers. I tried to go this way but it proved difficult
as sometimes the locking is done one level higher (e.g namei() being
called with or without the filedesc lock held). Introducing a _locked
version of the file was also not feasible for the same reasons (and
anyway it was a bit ugly).
So I went down the rabbit hole and tried changing the semantics of the
whole filedesc manipulations in order to make this clearer.
I've looked at what others do, fbsd is doing quite the same the thing as
this diff does (ie function are responsible for their own locking, and
file private function need to be called with the lock held).
DFBSD is using a spinlock but essentially took the same approach.
The lock are still exposed because some codepath are fiddling with
members of the struct without going through a dedicated API (like the
UIPC code path).
Next step imho would be to get rid of the FIF_LARVAL state.
I know the diff is a bit big and scary because it touches sensitive
paths... but I wanted to have an opinion on it before going further.
It has been running here for a few days, on a workstation, I also ran
the regress tests w/o any regressions.
Mathieu.
[1] https://marc.info/?l=openbsd-bugs&m=148459545413976&w=2
[2]
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_descrip.c.diff?r1=1.137&r2=1.138
diff --git dev/diskmap.c dev/diskmap.c
index 4550cffc264..0583cdb642e 100644
--- dev/diskmap.c
+++ dev/diskmap.c
@@ -55,7 +55,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag,
struct proc *p)
{
struct dk_diskmap *dm;
struct nameidata ndp;
- struct filedesc *fdp;
struct file *fp = NULL;
struct vnode *vp = NULL, *ovp;
char *devname;
@@ -82,9 +81,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag,
struct proc *p)
if ((error = getvnode(p, fd, &fp)) != 0)
goto invalid;
- fdp = p->p_fd;
- fdplock(fdp);
-
NDINIT(&ndp, 0, 0, UIO_SYSSPACE, devname, p);
ndp.ni_pledge = PLEDGE_RPATH;
if ((error = vn_open(&ndp, fp->f_flag, 0)) != 0)
@@ -116,7 +112,6 @@ diskmapioctl(dev_t dev, u_long cmd, caddr_t addr, int flag,
struct proc *p)
VOP_UNLOCK(vp, p);
FRELE(fp, p);
- fdpunlock(fdp);
free(devname, M_DEVBUF, PATH_MAX);
return 0;
@@ -127,8 +122,6 @@ bad:
if (fp)
FRELE(fp, p);
- fdpunlock(fdp);
-
invalid:
free(devname, M_DEVBUF, PATH_MAX);
diff --git kern/exec_script.c kern/exec_script.c
index 0b9ae86e224..fdc9215953e 100644
--- kern/exec_script.c
+++ kern/exec_script.c
@@ -168,9 +168,7 @@ check_shell:
panic("exec_script_makecmds: epp already has a fd");
#endif
- fdplock(p->p_fd);
error = falloc(p, 0, &fp, &epp->ep_fd);
- fdpunlock(p->p_fd);
if (error)
goto fail;
@@ -255,9 +253,7 @@ fail:
/* kill the opened file descriptor, else close the file */
if (epp->ep_flags & EXEC_HASFD) {
epp->ep_flags &= ~EXEC_HASFD;
- fdplock(p->p_fd);
(void) fdrelease(p, epp->ep_fd);
- fdpunlock(p->p_fd);
} else
vn_close(scriptvp, FREAD, p->p_ucred, p);
diff --git kern/kern_descrip.c kern/kern_descrip.c
index 623015f75fd..2c056682e3d 100644
--- kern/kern_descrip.c
+++ kern/kern_descrip.c
@@ -64,6 +64,10 @@
#include
+#defineFDP_ASSERT_WLOCK(fdp) rw_assert_wrlock(&(fdp)->fd_lock)
+#defineFDP_ASSERT_RLOCK(fdp) rw_assert_rdlock(&(fdp)->fd_lock)
+#defineFDP_ASSERT_LOCK(fdp)rw_assert_anylock(&(fdp)->fd_lock)
+
/*
* Descriptor management.
*/
@@ -73,6 +77,7 @@ int numfiles; /* actual number of open files
*/
static __inline void fd_used(struct filedesc *, int);
static __inline void fd_unused(struct filedesc *, int);
static __inline int find_next_zero(u_int *, int, u_int);
+static int fdrelease_locked(struct proc *, int);
int finishdup(struct proc *, struct file *, int, int, register_t *, int);
int find_last_set(struct filedesc *, int);
int dodup3(struct proc *, int, int, int, register_t *);
@@ -121,6 +126,7 @@ find_next_zero (u_int *bitmap, int want, u_int bits)
return (off << NDENTRYSHIFT) + ffs(~sub) - 1;
}
+/* filedesc needs to be locked (any lock) */
int
find_last_set(struct filedesc *fd, int last)
{
@@ -128,6 +134,8 @@ find_last_set(struct filedesc *fd, int last)
struct file **ofiles = fd->fd_ofiles;
Re: sed.1: miscellaneous corrections
On Sun, Nov 26, 2017 at 07:47:01PM +, kshe wrote:
> Hi,
>
> I noticed a certain number of inaccuracies within the manual page for
> sed. The diff below corrects to most obvious ones, although further
> improvements are certainly possible.
>
> Additionally, the script given in the EXAMPLES section being already
> quite unnecessarily contrived (as it does in twelve commands what could
> be done in merely two), I took the opportunity to make it slightly
> simpler and easier to read.
>
morning.
this diff is too much for review (at least for me anyway). could you
parcel it up into some logical parts and resend?
i'll make some comments inline:
> Index: sed.1
> ===
> RCS file: /cvs/src/usr.bin/sed/sed.1,v
> retrieving revision 1.50
> diff -u -p -r1.50 sed.1
> --- sed.1 19 Jul 2017 21:28:19 - 1.50
> +++ sed.1 17 Nov 2017 02:50:36 -
> @@ -57,10 +57,9 @@ utility reads the specified files, or th
> are specified, modifying the input as specified by a list of commands.
> The input is then written to the standard output.
> .Pp
> -A single command may be specified as the first argument to
> -.Nm sed .
> -Multiple commands may be specified
> -separated by newlines or semicolons,
> +Commands are separated by newlines or semicolons and may be specified
> +either as the first argument to
> +.Nm
ok, all the what can and what cannot go on newline stuff would make one
diff.
> or by using the
> .Fl e
> or
> @@ -95,7 +94,6 @@ to the list of commands.
> Append the editing commands found in the file
> .Ar command_file
> to the list of commands.
> -The editing commands should each be listed on a separate line.
> .It Fl i Ns Op Ar extension
> Edit files in place, saving backups with the specified
> .Ar extension .
> @@ -199,7 +197,7 @@ has the following two additions to BREs:
> In a context address, any character other than a backslash
> .Pq Ql \e
> or newline character may be used to delimit the regular expression.
> -The opening delimiter should be preceded by a backslash
> +The opening delimiter must be preceded by a backslash
what's there is fine, honestly. modal verbs are suble; text which
continually instructs "thou must" is not.
> unless it is a slash.
> Putting a backslash character before the delimiting character
> causes the character to be treated literally.
> @@ -255,7 +253,7 @@ as well as the
> flag to the
> .Ic s
> function,
> -take an optional
> +take a
fluff could be another diff. this seems correct.
> .Ar file
> parameter,
> which should be separated from the function or flag by whitespace.
> @@ -264,23 +262,22 @@ Files are created
> before any input processing begins.
> .Pp
> The
> -.Ic b ,
> -.Ic r ,
> -.Ic s ,
> -.Ic t ,
> -.Ic w ,
> -.Ic y ,
> +.Ar label
> +argument to the
> +.Ic \&: ,
> +.Ic b
> and
> -.Ic \&:
> -functions all accept additional arguments.
> -The synopses below indicate which arguments have to be separated from
> +.Ic t
> +commands may be terminated by either a newline or a semicolon,
> +although the former should be prefered if portability is desired.
> +.Pp
> +For functions that accept additional arguments,
> +the synopses below indicate which have to be separated from
> the function letters by whitespace characters.
> .Pp
> Functions can be combined to form a
> -.Em function list ,
> -a list of
> -.Nm
> -functions each followed by a newline, as follows:
> +.Em function list
> +as follows:
> .Bd -literal -offset indent
> { function
>function
> @@ -297,11 +294,11 @@ in which case they are applied only to l
> .Em not
> selected by the addresses.
> .Bl -tag -width Ds
> -.It [2addr] Ar function-list
> +.It [2addr] Ns Ar function-list
it's not possible without a space?
> Execute
> .Ar function-list
> only when the pattern space is selected.
> -.It Xo [1 addr] Ic a Ns \e
> +.It Xo [1addr] Ns Ic a Ns \e
> .br
> .Ar text
> .Xc
> @@ -317,7 +314,7 @@ Branch to the
> function with the specified
> .Ar label .
> If the label is not specified, branch to the end of the script.
> -.It Xo [2addr] Ic c Ns \e
> +.It Xo [2addr] Ns Ic c Ns \e
> .br
> .Ar text
> .Xc
> @@ -342,7 +339,7 @@ pattern space.
> .It [2addr] Ns Ic H
> Append a newline character followed by the contents of the pattern space
> to the hold space.
> -.It Xo [1addr] Ic i Ns \e
> +.It Xo [1addr] Ns Ic i Ns \e
> .br
> .Ar text
> .Xc
> @@ -413,8 +410,8 @@ in the pattern space.
> Any character other than backslash or newline can be used instead of
> a slash to delimit the regular expression and the replacement.
> Within the regular expression and the replacement,
> -the regular expression delimiter itself can be used as
> -a literal character if it is preceded by a backslash.
> +the delimiter itself can be used as a literal character
> +if it is preceded by a backslash.
> .Pp
> An ampersand
> .Pq Ql &
> @@ -486,10 +483,14 @@ Within
> .Ar string1
> and
> .Ar string2 ,
> -a backslash
Re: dc.1: document non-portability of `e'
On Sun, Nov 26, 2017 at 07:18:33PM +, kshe wrote: > Hi, > > The manual page for dc(1) is very careful about signalling which > commands are non-portable extensions, with the exception of the `e' > command, which is a more recent addition. > > Index: dc.1 > === > RCS file: /cvs/src/usr.bin/dc/dc.1,v > retrieving revision 1.30 > diff -u -p -r1.30 dc.1 > --- dc.1 23 Feb 2017 06:40:17 - 1.30 > +++ dc.1 26 Oct 2017 04:44:01 - > @@ -189,6 +189,7 @@ The top value on the stack is duplicated > Equivalent to > .Ic p , > except that the output is written to the standard error stream. > +This is a non-portable extension. > .It Ic f > All values on the stack are printed, separated by newlines. > .It Ic G > > Regards, > > kshe > morning. posix doesn;t describe a separate dc utility, as far as i can see. the STANDARDS section of dc(1) discusses only the "arithmetic operations" of dc. bc(1) however does describe -e as an extension, in STANDARDS. i think here dc(1) is a bit of a special case, but i think how it is now is fine. i definitely don;t want to start plastering "this is an extension" onto the end of every option not described by posix. jmc
Re: hide wpakey from root by default
On 2017 Nov 27 (Mon) at 02:33:59 +0100 (+0100), Stefan Sperling wrote:
:On Mon, Nov 27, 2017 at 01:31:17AM +0100, Stefan Sperling wrote:
:> On Sun, Nov 26, 2017 at 06:17:14PM +0100, Jeremie Courreges-Anglas wrote:
:> >
:> > I don't think anything has been committed regarding this issue, right?
:>
:> Nope.
:>
:> I've been discussing this with people in person.
:> Will summarize those discussions and send a new diff soon.
:
:Most people I've talked to seem to be OK with never exposing
:these secrets to userland in the first place.
:
:OK?
:
:Index: net/if_spppsubr.c
:===
:RCS file: /cvs/src/sys/net/if_spppsubr.c,v
:retrieving revision 1.173
:diff -u -p -r1.173 if_spppsubr.c
:--- net/if_spppsubr.c 20 Oct 2017 09:35:09 - 1.173
:+++ net/if_spppsubr.c 27 Nov 2017 01:27:31 -
:@@ -4493,9 +4493,8 @@ sppp_get_params(struct sppp *sp, struct
: spa->proto = auth->proto;
: spa->flags = auth->flags;
:
:- /* do not copy the secret, and only let root know the name */
:- if (auth->name != NULL && suser(curproc, 0) == 0)
:- strlcpy(spa->name, auth->name, sizeof(spa->name));
:+ /* do not copy the name and secret to userland */
:+ memset(spa->name, 0, sizeof(spa->name));
:
: if (copyout(spa, (caddr_t)ifr->ifr_data, sizeof(*spa)) != 0) {
: free(spa, M_DEVBUF, 0);
This hides the username that is used, not the password/authkey. Is the
username private information?
:Index: net80211/ieee80211_ioctl.c
:===
:RCS file: /cvs/src/sys/net80211/ieee80211_ioctl.c,v
:retrieving revision 1.57
:diff -u -p -r1.57 ieee80211_ioctl.c
:--- net80211/ieee80211_ioctl.c 6 Nov 2017 11:34:29 - 1.57
:+++ net80211/ieee80211_ioctl.c 27 Nov 2017 01:29:44 -
:@@ -252,9 +252,6 @@ static int
: ieee80211_ioctl_getnwkeys(struct ieee80211com *ic,
: struct ieee80211_nwkey *nwkey)
: {
:- struct ieee80211_key *k;
:- int error, i;
:-
: if (ic->ic_flags & IEEE80211_F_WEPON)
: nwkey->i_wepon = IEEE80211_NWKEY_WEP;
: else
:@@ -262,23 +259,8 @@ ieee80211_ioctl_getnwkeys(struct ieee802
:
: nwkey->i_defkid = ic->ic_wep_txkey + 1;
:
:- for (i = 0; i < IEEE80211_WEP_NKID; i++) {
:- if (nwkey->i_key[i].i_keydat == NULL)
:- continue;
:- /* do not show any keys to non-root user */
:- if ((error = suser(curproc, 0)) != 0)
:- return error;
:- k = &ic->ic_nw_keys[i];
:- if (k->k_cipher != IEEE80211_CIPHER_WEP40 &&
:- k->k_cipher != IEEE80211_CIPHER_WEP104)
:- nwkey->i_key[i].i_keylen = 0;
:- else
:- nwkey->i_key[i].i_keylen = k->k_len;
:- error = copyout(k->k_key, nwkey->i_key[i].i_keydat,
:- nwkey->i_key[i].i_keylen);
:- if (error != 0)
:- return error;
:- }
:+ /* do not show any keys to userland */
:+
: return 0;
: }
:
:@@ -491,14 +473,10 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
: case SIOCG80211WPAPSK:
: psk = (struct ieee80211_wpapsk *)data;
: if (ic->ic_flags & IEEE80211_F_PSK) {
:- psk->i_enabled = 1;
:- /* do not show any keys to non-root user */
:- if (suser(curproc, 0) != 0) {
:- psk->i_enabled = 2;
:- memset(psk->i_psk, 0, sizeof(psk->i_psk));
:- break; /* return ok but w/o key */
:- }
:- memcpy(psk->i_psk, ic->ic_psk, sizeof(psk->i_psk));
:+ /* do not show any keys to userland */
:+ psk->i_enabled = 2;
:+ memset(psk->i_psk, 0, sizeof(psk->i_psk));
:+ break; /* return ok but w/o key */
: } else
: psk->i_enabled = 0;
: break;
OK
:Index: netinet/ip_carp.c
:===
:RCS file: /cvs/src/sys/netinet/ip_carp.c,v
:retrieving revision 1.319
:diff -u -p -r1.319 ip_carp.c
:--- netinet/ip_carp.c 21 Nov 2017 09:08:55 - 1.319
:+++ netinet/ip_carp.c 27 Nov 2017 01:29:34 -
:@@ -2158,9 +2158,8 @@ carp_ioctl(struct ifnet *ifp, u_long cmd
: }
: carpr.carpr_advbase = sc->sc_advbase;
: carpr.carpr_balancing = sc->sc_balancing;
:- if (suser(p, 0) == 0)
:- bcopy(sc->sc_key, carpr.carpr_key,
:- sizeof(carpr.carpr_key));
:+ /* do not show any keys to userland */
:+ memset(carpr.carpr_key, 0, sizeof(carpr.carpr_key));
: c
Function in sys/dev/pci/ppb.c
Hello! In my laptop, the BIOS does not configure properly the hardware. The OpenBSD code does, instead, as regards the PCI-to-PCI bridge before my network card. Because no BIOS update and no other OS can do this, I would like to know what my bridge actually needs. In sys/dev/pci/ppb.c: void ppb_alloc_resources(struct ppb_softc *sc, struct pci_attach_args *pa) this function acts according to the contents in `pa->pa_memex' and `pa->pa_ioex'. These are members of the `struct pci_attach_args pa'. autoconf(9) states that: ``Device data structures are allocated dynamically during autoconfiguration''. If this is the case, what are the autoconf(9) routines that fill the data structure `pci_attach_args' of a `ppb' device? And how can they determine the amount of `memex' and `ioex' needed by the bridge? If someone of you could help me about this question, I would be very grateful. Thank you, Rocky
Re: iked: support MOBIKE (RFC 4555)
On Tue, Nov 14, 2017 at 01:58:41PM +0100, Patrick Wildt wrote:
> On Thu, Nov 09, 2017 at 11:40:30AM +0100, Patrick Wildt wrote:
> > Hi,
> >
> > this diff implements MOBIKE (RFC 4555) support in iked(8), with us
> > acting as responder. In practice this support means that clients like
> > iPhones can roam in different networks (LTE, WiFi) and change their
> > external address without having to re-do the whole handshake. It allows
> > the client to choose how and when to change the external tunnel endpoint
> > addresses on demand, depending on which network is better or even is
> > connected at all.
> >
> > This diff already had a few iterations where race conditions were found
> > and ironed out, so this should be a rather stable version which has been
> > working really well for me now.
> >
> > Key changes with this diff are:
> >
> > * MOBIKE is on by default, but can be turned off (if neccessary)
> >using a config key.
> > * MOBIKE support is then announced to the initiator.
> > * Peers can use different IP addresses in the IKE messages, but the
> >Tunnel is only updated when we receive IKEV2_N_UPDATE_SA_ADDRESSES.
> > * This means we need to store two peer addresses. The one he talked to us
> >last, so that we know where to respond to. And the one that the kernel
> >knows, so that we know if we have to update the SAs and flows.
> > * We need to update the SA as well as the flow. For the SAs I added
> >a way of updating destination addresses of an existing TDB in June.
> >The flows simply need a reload after the SA changed.
> > * COOKIE2 is used for return routability checks. strongswan seems
> >to use it. It's encrypted and should be sent back to the initiator.
> >
> > When you, for instance, use your phone and turn on/off WiFi, you should
> > see that the VPN stays connected and iked should log that the address
> > has changed. Especially `ipsecctl -sa` should show updated SAs and
> > flows.
> >
> > OKs? Objections? Feedback?
>
> I have heard from two more people running it and seeing it work. Anyone
> else?
So far the response has been good. Any OKs or should I just put it in
to give it more exposure?
Patrick
> > Patrick
> >
> > diff --git a/sbin/iked/config.c b/sbin/iked/config.c
> > index 590e4d7f4da..da8d0745f3c 100644
> > --- a/sbin/iked/config.c
> > +++ b/sbin/iked/config.c
> > @@ -814,6 +814,29 @@ config_getcompile(struct iked *env, struct imsg *imsg)
> > return (0);
> > }
> >
> > +int
> > +config_setmobike(struct iked *env)
> > +{
> > + unsigned int boolval;
> > +
> > + boolval = env->sc_mobike;
> > + proc_compose(&env->sc_ps, PROC_IKEV2, IMSG_CTL_MOBIKE,
> > + &boolval, sizeof(boolval));
> > + return (0);
> > +}
> > +
> > +int
> > +config_getmobike(struct iked *env, struct imsg *imsg)
> > +{
> > + unsigned int boolval;
> > +
> > + IMSG_SIZE_CHECK(imsg, &boolval);
> > + memcpy(&boolval, imsg->data, sizeof(boolval));
> > + env->sc_mobike = boolval;
> > + log_debug("%s: %smobike", __func__, env->sc_mobike ? "" : "no ");
> > + return (0);
> > +}
> > +
> > int
> > config_setocsp(struct iked *env)
> > {
> > diff --git a/sbin/iked/iked.c b/sbin/iked/iked.c
> > index 09fef3ea877..548da77d014 100644
> > --- a/sbin/iked/iked.c
> > +++ b/sbin/iked/iked.c
> > @@ -250,6 +250,7 @@ parent_configure(struct iked *env)
> > if (pledge("stdio rpath proc dns inet route sendfd", NULL) == -1)
> > fatal("pledge");
> >
> > + config_setmobike(env);
> > config_setcoupled(env, env->sc_decoupled ? 0 : 1);
> > config_setmode(env, env->sc_passive ? 1 : 0);
> > config_setocsp(env);
> > @@ -280,6 +281,7 @@ parent_reload(struct iked *env, int reset, const char
> > *filename)
> > /* Re-compile policies and skip steps */
> > config_setcompile(env, PROC_IKEV2);
> >
> > + config_setmobike(env);
> > config_setcoupled(env, env->sc_decoupled ? 0 : 1);
> > config_setmode(env, env->sc_passive ? 1 : 0);
> > config_setocsp(env);
> > diff --git a/sbin/iked/iked.conf.5 b/sbin/iked/iked.conf.5
> > index 8c77f24d603..be0e8ef30f6 100644
> > --- a/sbin/iked/iked.conf.5
> > +++ b/sbin/iked/iked.conf.5
> > @@ -136,6 +136,15 @@ This is the default.
> > .It Ic set decouple
> > Don't load the negotiated SAs and flows from the kernel.
> > This mode is only useful for testing and debugging.
> > +.It Ic set mobike
> > +Enable MOBIKE (RFC 4555) support.
> > +This is the default.
> > +MOBIKE allows the change of the peer IP address for IKE and IPsec SAs.
> > +Currenly
> > +.Xr iked 8
> > +only supports MOBIKE when acting as a responder.
> > +.It Ic set nomobike
> > +Disables MOBIKE support.
> > .It Ic set ocsp Ar URL
> > Enable OCSP and set the URL of the OCSP responder.
> > Please note that the matching responder and issuer certificates
> > diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
> > index b536d58e157..c2030ea7460 100644
> > --- a/sbin/iked/iked.
Re: fuse: vfs create does not map 1:1 to fuse create
On 23/11/17(Thu) 21:45, Helg wrote: > On Thu, Nov 23, 2017 at 12:09:34PM +, Helg Bredow wrote: > > - Forwarded message from Martin Pieuchot - > > > > Date: Sat, 18 Nov 2017 11:03:49 +0100 > > From: Martin Pieuchot > > To: Helg Bredow > > CC: "tech@openbsd.org" > > Subject: Re: fuse: vfs create does not map 1:1 to fuse create > > User-Agent: Mutt/1.9.1 (2017-09-22) > > > > On 18/11/17(Sat) 02:22, Helg Bredow wrote: > > > On Fri, 10 Nov 2017 09:09:32 +0100 > > > Martin Pieuchot wrote: > > > > > > > On 09/11/17(Thu) 01:20, Helg Bredow wrote: > > > > > > On 08/11/17(Wed) 14:12, Helg Bredow wrote: > > > > > > > There is a bug when creating a file in fuse-exfat and then > > > > > > > deleting it > > > > > > > again without first unmounting the file system. The reason for > > > > > > > this is > > > > > > > that fuse-exfat maintains strict reference counts and fuse > > > > > > > currently > > > > > > > calls the file system create and open functions when it should > > > > > > > only > > > > > > > call create. > > > > > > > [...] > > > > > > > > > [...] > > > > > [...] > > > > > > I now this caused some confusion but I believe the patch below is the > > > correct solution. > > > > When you use the work "believing" you're asking us to trust you without > > argumenting. > > > > > Here's the mapping of file system calls so it's clear. > > > [...] > > > > That's irrelevant. > > > > There's no such thing as "atomic_open" from the userland point of view. > > So what you're discussing here is an "implementation details" of Linux > > VFS. > > > > What matters is which syscall the application do. I believe it is > > open(2). So how its arguments are translated to the userland FS? If > > I understand your diff correctly, OpenBSD will now do something like > > below (please correct the graph): > > > > > > USER PROCESS fuse-zip > > | ^ > >open(2) | > > | FBT_MKNOD + FBT_OPEN > > \/ | > > - > > > > KERNEL > > > > The only thing I understand is that you're changing the kernel behavior > > for all open(2) syscall operating on FUSE filesystem. Before the kernel > > was generating a FBT_CREATE and FBT_OPEN messages, with some arguments. > > Now you want to generate FTB_MKNOD and FBT_OPEN with other arguments. > > > > To me it sounds that you're working around a problem you don't > > understand. If FUSE FSs want a FUSE 'create' operation then let's > > give them that! What argument do they expect? > > > > So let's start from the beginning: > > > > - Which syscall is causing problem with which arguments? > > - Which FUSE operation(s) FUSE FSs are expecting for this exact syscall > > with these arguments? > > - Which FUSE operation(s) OpenBSD FUSE is currently generating with which > > arguments? > > The syscall that is causing the problem is open(2) with the O_CREAT flag. > With my change, this is the call graph. > > open(2) >| > vn_open() >| >+--VOP_CREATE(9) when O_CREAT >| | >| +-->fusefs_create()-->FBT_MKNOD-->ifuse_ops_mknod() >| | >| +--> fs mknod() >| >+--VOP_SETATTR(9) when O_TRUNC >| | >| +-->fusefs_setattr()-->FBT_SETATTR-->ifuse_ops_setattr() >| | >| +--> fs truncate() >| >+--VOP_OPEN(9) always >| >+--fusefs_open()-->FBT_OPEN-->ifuse_ops_open() >| >+--> fs open() > > > The file systems are expecting the following arguments. > > mknod(path, mode, dev) >mode is that passed to open(2) >dev will be 0 for regular files. FUSE mknod can create regular files. > open(path, fuse_file_info) >fuse_file_info has (almost all) flags passed to open(2) plus other >things that are not set by ifuse_ops_open. The FS will also return >a handle to the file that was opened which it expects to receive >again for functions that operate on the same file. > create(path, mode, fuse_file_info) >mode is that passed to open(2) >fuse_file_info has (almost all) flags passed to open(2) plus other >things that are not set by ifuse_ops_open. The FS will also return >a handle to the file that was opened which it expects to receive >again for functions that operate on the same file. > > OpenBSD FUSE is currently generating the following sequence of FUSE > calls when creating a file with open(2). > > FBT_CREATE + FBT_OPEN > create arguments are hard-coded to O_CREAT | O_RDWR > open arguments are either O_RDONLY, O_RDWR, O_WRONLY depending on > mode > > The problem is in vn_ope
Re: race-less nd6_timer
On 23/11/17(Thu) 15:34, Alexander Bluhm wrote:
> On Wed, Nov 22, 2017 at 04:24:22PM +0100, Martin Pieuchot wrote:
> > Diff below implements 3/ because it seems the simplest approach to
> > me and reduce differences with ARP a bit further.
>
> Yes.
>
> > void
> > -nd6_llinfo_settimer(struct llinfo_nd6 *ln, int secs)
> > +nd6_llinfo_settimer(struct llinfo_nd6 *ln, unsigned int secs)
> > {
> > - if (secs < 0) {
> > - ln->ln_rt->rt_expire = 0;
> > - timeout_del(&ln->ln_timer_ch);
> > - } else {
> > - ln->ln_rt->rt_expire = time_uptime + secs;
> > - timeout_add_sec(&ln->ln_timer_ch, secs);
> > + time_t expire = time_uptime + secs;
> > +
> > + ln->ln_rt->rt_expire = expire;
> > + if (!timeout_pending(&nd6_timer_to) || expire < nd6_timer_next) {
> > + nd6_timer_next = expire;
> > + timeout_add_sec(&nd6_timer_to, secs);
> > }
> > }
>
> The global nd6_timer_next is protected by the net lock. Should we
> put an NET_ASSERT_LOCKED() into nd6_llinfo_settimer()?
Here's a diff that includes that and prevent a user-after-free pointed
out by visa@. We should not try to dereference `rt' if nd6_free() has
been called.
Hrvoje Popovski confirmed he couldn't reproduce the panic with this
diff.
ok?
Index: netinet6/nd6.c
===
RCS file: /cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.221
diff -u -p -r1.221 nd6.c
--- netinet6/nd6.c 31 Oct 2017 22:05:13 - 1.221
+++ netinet6/nd6.c 27 Nov 2017 09:35:41 -
@@ -67,7 +67,8 @@
#define ND6_RECALC_REACHTM_INTERVAL (60 * 120) /* 2 hours */
/* timer values */
-time_t nd6_expire_time = -1; /* at which time_uptime nd6_expire runs */
+intnd6_timer_next = -1; /* at which time_uptime nd6_timer runs */
+time_t nd6_expire_next = -1; /* at which time_uptime nd6_expire runs */
intnd6_delay = 5;/* delay first probe time 5 second */
intnd6_umaxtries = 3;/* maximum unicast query */
intnd6_mmaxtries = 3;/* maximum multicast query */
@@ -90,13 +91,15 @@ int nd6_inuse, nd6_allocated;
int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL;
+void nd6_timer(void *);
void nd6_slowtimo(void *);
void nd6_expire(void *);
void nd6_expire_timer(void *);
void nd6_invalidate(struct rtentry *);
void nd6_free(struct rtentry *);
-void nd6_llinfo_timer(void *);
+int nd6_llinfo_timer(struct rtentry *);
+struct timeout nd6_timer_to;
struct timeout nd6_slowtimo_ch;
struct timeout nd6_expire_timeout;
struct task nd6_expire_task;
@@ -120,6 +123,7 @@ nd6_init(void)
nd6_init_done = 1;
/* start timer */
+ timeout_set_proc(&nd6_timer_to, nd6_timer, &nd6_timer_to);
timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
timeout_set(&nd6_expire_timeout, nd6_expire_timer, NULL);
@@ -297,45 +301,66 @@ skip1:
* ND6 timer routine to handle ND6 entries
*/
void
-nd6_llinfo_settimer(struct llinfo_nd6 *ln, int secs)
+nd6_llinfo_settimer(struct llinfo_nd6 *ln, unsigned int secs)
{
- if (secs < 0) {
- ln->ln_rt->rt_expire = 0;
- timeout_del(&ln->ln_timer_ch);
- } else {
- ln->ln_rt->rt_expire = time_uptime + secs;
- timeout_add_sec(&ln->ln_timer_ch, secs);
+ time_t expire = time_uptime + secs;
+
+ NET_ASSERT_LOCKED();
+
+ ln->ln_rt->rt_expire = expire;
+ if (!timeout_pending(&nd6_timer_to) || expire < nd6_timer_next) {
+ nd6_timer_next = expire;
+ timeout_add_sec(&nd6_timer_to, secs);
}
}
void
-nd6_llinfo_timer(void *arg)
+nd6_timer(void *arg)
{
- struct llinfo_nd6 *ln;
- struct rtentry *rt;
- struct sockaddr_in6 *dst;
- struct ifnet *ifp;
- struct nd_ifinfo *ndi = NULL;
+ struct llinfo_nd6 *ln, *nln;
+ time_t expire = time_uptime + nd6_gctimer;
+ int secs;
NET_LOCK();
+ TAILQ_FOREACH_SAFE(ln, &nd6_list, ln_list, nln) {
+ struct rtentry *rt = ln->ln_rt;
- ln = (struct llinfo_nd6 *)arg;
+ if (rt->rt_expire && rt->rt_expire <= time_uptime)
+ if (nd6_llinfo_timer(rt))
+ continue;
- if ((rt = ln->ln_rt) == NULL)
- panic("ln->ln_rt == NULL");
- if ((ifp = if_get(rt->rt_ifidx)) == NULL) {
- NET_UNLOCK();
- return;
+ if (rt->rt_expire && rt->rt_expire < expire)
+ expire = rt->rt_expire;
}
- ndi = ND_IFINFO(ifp);
- dst = satosin6(rt_key(rt));
- /* sanity check */
- if (rt->rt_llinfo != NULL && (struct llinfo_nd6 *)rt->rt_llinfo != ln)
- panic("rt_llinfo(%p) is not equal to ln(%p)",
- rt->rt_llinfo, ln);
- if (!dst)
- panic("dst=0 in nd6_ti
Re: [patch] snmpd hrStorageSize negative values
On Sat, 25 Nov 2017 11:42:07 -0700 Joel Knight wrote: > On Thu, Mar 9, 2017 at 10:02 PM, Joel Knight wrote: > > Hi. > > > > snmpd(8) uses unsigned ints internally to represent the size and used > > space of a file system. The HOST-RESOURCES-MIB defines the valid > > values for those OIDs as 0..2147483647. With sufficiently large file > > systems, this can cause negative numbers to be returned for the size > > and used space OIDs. > > > > .1.3.6.1.2.1.25.2.3.1.5.36=-1573167768 > > Hi. Just wanted to bump this again and see if anyone that cares about > snmp could take a look? Looking for oks and someone who wouldn't mind > committing it. > > > > At sthen's suggestion, do what net-snmp does and fiddle with the > > values to prevent wrapping. Yes this mucks with the actual values of > > size, used space, and block size, but it allows snmpd to convey the > > proper size and used space of the file system which is what most > > everybody is really interested in. > > > > In case gmail hoses this diff, it's also here: > > https://www.packetmischief.ca/files/patches/snmpd.hrstorage2.diff Hi Joel, I think this won't work unless you also change the type of 'size' and 'used' to u_int64_t. Gerhard
update Mesa to 17.2.6
cd /usr/xenocara/lib ftp https://mesa.freedesktop.org/archive/mesa-17.2.6.tar.gz ftp http://jsg.id.au/mesa-update/mesa.diff.gz tar zxf mesa-17.2.6.tar.gz gunzip mesa.diff.gz patch -p0 < mesa.diff sed -i 's/mesa$/mesa-17.2.6/' Makefile build xenocara as normal Builds on at least amd64, i386, sparc64, armv7 and arm64. I'm interested in reports from people who saw corruption on Intel graphics during the brief period when Mesa 17.1.6 was in the tree.
Re: freezero(3) for stdio's internal buffers
On Mon, Nov 27, 2017 at 05:48:14AM +, kshe wrote: > On Mon, 27 Nov 2017 00:42:01 +, Theo de Raadt wrote: > > This needs worst-case performance measurements. > > The only instances where performance could be a problem are in > vfprintf.c and vfwprintf.c, where the calls happen inside a loop; but > for these, in fact, the best solution would be to use recallocarray > directly: rather than repeatedly freeing (and clearing) `convbuf' and > reallocating a fresh one every time it is needed, it should be kept > around and passed to __wcsconv() and __mbsconv(), along with some > accounting of its current size, so that these functions could then call > recallocarray appropriately. However, this optimisation would be more > intrusive than the diff I submitted, and would therefore demand greater > familiarity with stdio's source, which I have not yet acquired. > > That being said, in all other cases, since the way stdio functions fill > their buffers is much more costly than simply writing a bunch of zeros > in sequence (or merely calling munmap(2)), no real slowdown is to be > expected. I should also point out that recallocarray is currently used > inside several loops in fvwrite.c, fgetln.c and getdelim.c, but no one > complained that the affected functions became too slow, because doing > things, as stdio does, like reading from and writing to disk or decoding > and converting user input will always dominate the effect of a few calls > to discard temporary buffers. > > Regards, > > kshe I was thinking: only point against your statement is that there could be cases where a very large buffer is used, but only a small part of it is used. In that case, clearing the buffer is more costly. Luckily, in those cases malloc just calls munmap(2) without clearing. Still, I would like to see benchmarks to validate assumptions. -Otto
