Re: net/if.c: nullptr deref in if_hooks_run
On Tue, 10 Mar 2020 10:25:56 +1000, David Gwynne wrote: > how about this? this has the cursor handling move the traversal and > NULL check back to the for loop instead of looping on its own. Much better. OK millert@ - todd
Re: ifq: ifq_dec_sleep may return garbage
On Tue, Mar 10, 2020 at 12:03:24AM +0100, Tobias Heider wrote:
> If 'm = ifq->ifq_ops->ifqop_deq_begin(ifq, )' is not NULL
> the loop is exited and an uninitialized 'int error' is returned.
> Several lines below error is checked for '!= 0', so i assume it
> was meant to be initialized to '0'.
>
> ok?
ok
>
> Index: ifq.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/net/ifq.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 ifq.c
> --- ifq.c 25 Jan 2020 06:31:32 - 1.36
> +++ ifq.c 9 Mar 2020 22:57:58 -
> @@ -395,7 +395,7 @@ ifq_deq_sleep(struct ifqueue *ifq, struc
> {
> struct mbuf *m;
> void *cookie;
> - int error;
> + int error = 0;
>
> ifq_deq_enter(ifq);
> if (ifq->ifq_len == 0 && nbio)
Re: net/if.c: nullptr deref in if_hooks_run
On Mon, Mar 09, 2020 at 11:56:09PM +0100, Klemens Nanni wrote:
> On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote:
> > there seems to be a nullptr dereference in if_hooks_run.
> Did your kernel crash here or did you find reading alone?
>
> > When the inner while loop is exited because 't == NULL' the next
> > line is an access to 't->t_func'.
> Yes, reads obviously wrong.
:'(
> > Because 't==NULL' means the TAILQ is fully traversed I think we
> > should break and exit instead.
> Make sense, OK kn
how about this? this has the cursor handling move the traversal and
NULL check back to the for loop instead of looping on its own.
Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.600
diff -u -p -r1.600 if.c
--- if.c24 Jan 2020 05:14:51 - 1.600
+++ if.c10 Mar 2020 00:24:00 -
@@ -1050,10 +1050,9 @@ if_hooks_run(struct task_list *hooks)
mtx_enter(_hooks_mtx);
for (t = TAILQ_FIRST(hooks); t != NULL; t = nt) {
- while (t->t_func == NULL) { /* skip cursors */
- t = TAILQ_NEXT(t, t_entry);
- if (t == NULL)
- break;
+ if (t->t_func == NULL) { /* skip cursors */
+ nt = TAILQ_NEXT(t, t_entry);
+ continue;
}
func = t->t_func;
arg = t->t_arg;
rpki-client: output.c static/const tweaks
Claudio suggested[0] to restrict the visibility of three helper
functions in this file. The diff below goes a bit further, sprinkling
some static and const magic to help the compiler generate better code.
ok?
[0] https://marc.info/?l=openbsd-tech=158375920102498=2
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.25
diff -u -p -r1.25 extern.h
--- extern.h9 Mar 2020 23:50:01 - 1.25
+++ extern.h10 Mar 2020 00:09:25 -
@@ -370,9 +370,6 @@ extern int outformats;
extern char*outputdir;
int outputfiles(struct vrp_tree *v);
-FILE *output_createtmp(char *);
-voidoutput_cleantmp(void);
-int output_finish(FILE *);
int output_bgpd(FILE *, struct vrp_tree *);
int output_bird1v4(FILE *, struct vrp_tree *);
int output_bird1v6(FILE *, struct vrp_tree *);
Index: output.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.8
diff -u -p -r1.8 output.c
--- output.c9 Mar 2020 23:50:01 - 1.8
+++ output.c10 Mar 2020 00:09:25 -
@@ -29,12 +29,12 @@
#include "extern.h"
char *outputdir;
-charoutput_tmpname[PATH_MAX];
-charoutput_name[PATH_MAX];
-
int outformats;
-struct outputs {
+static char output_tmpname[PATH_MAX];
+static char output_name[PATH_MAX];
+
+static const struct outputs {
int format;
char*name;
int (*fn)(FILE *, struct vrp_tree *);
@@ -48,8 +48,11 @@ struct outputs {
{ 0, NULL }
};
-voidsig_handler(int);
-voidset_signal_handler(void);
+static FILE*output_createtmp(char *);
+static void output_cleantmp(void);
+static int output_finish(FILE *);
+static void sig_handler(int);
+static void set_signal_handler(void);
int
outputfiles(struct vrp_tree *v)
@@ -89,7 +92,7 @@ outputfiles(struct vrp_tree *v)
return rc;
}
-FILE *
+static FILE *
output_createtmp(char *name)
{
FILE *f;
@@ -113,7 +116,7 @@ output_createtmp(char *name)
return f;
}
-int
+static int
output_finish(FILE *out)
{
if (fclose(out) != 0)
@@ -124,7 +127,7 @@ output_finish(FILE *out)
return 0;
}
-void
+static void
output_cleantmp(void)
{
if (*output_tmpname)
@@ -135,7 +138,7 @@ output_cleantmp(void)
/*
* Signal handler that clears the temporary files.
*/
-void
+static void
sig_handler(int sig __unused)
{
output_cleantmp();
@@ -145,7 +148,7 @@ sig_handler(int sig __unused)
/*
* Set signal handler on panic signals.
*/
-void
+static void
set_signal_handler(void)
{
struct sigaction sa;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: snmpd(8): fix use of uninitialized pointer
Looking at RFC1157 section 4.1.6, an snmpv1 trap should also contain a varbindlist. Could you test the diff below? martijn@ On 3/9/20 11:38 PM, Jan Klemkow wrote: > Hi, > > The following diff fixes the use of the uninitialized pointer iter > in trapcmd_exec(). > > iter is just initialized in traphandler_parse() if vers is SNMP_V2. In > all other cases iter stays uninitialized and may dereferenced in > trapcmd_exec(). > > OK? > > bye, > Jan > Index: traphandler.c === RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v retrieving revision 1.15 diff -u -p -r1.15 traphandler.c --- traphandler.c 24 Oct 2019 12:39:27 - 1.15 +++ traphandler.c 9 Mar 2020 23:26:56 - @@ -236,6 +236,8 @@ traphandler_parse(char *buf, size_t n, s trapoid, , , uptime) == -1) goto done; traphandler_v1translate(trapoid, gtype, etype); + *vbinds = + elm->be_sub->be_next->be_next->be_next->be_next->be_next; break; case SNMP_V2:
Re: net/if.c: nullptr deref in if_hooks_run
On Mon, Mar 09, 2020 at 11:56:09PM +0100, Klemens Nanni wrote: > On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote: > > there seems to be a nullptr dereference in if_hooks_run. > Did your kernel crash here or did you find reading alone? Coverity Scan found it > > When the inner while loop is exited because 't == NULL' the next > > line is an access to 't->t_func'. > Yes, reads obviously wrong. > > > Because 't==NULL' means the TAILQ is fully traversed I think we > > should break and exit instead. > Make sense, OK kn >
Re: softraid: too many arguments for sr_error
On Tue, Mar 10, 2020 at 12:01:45AM +0100, Klemens Nanni wrote: > On Mon, Mar 09, 2020 at 11:41:14PM +0100, Tobias Heider wrote: > > sr_error takes a sr_softc and a printf like format string + varargs. > > There's no need to pass DEVNAME(sc) here. > Either that or embed the device name in the format string; its usage > inside error messages is already inconsistent around there, so OK kn > either ways. > I haven't seen any of the other errors use DEVNAME(sc) so i decided to just drop it.
ifq: ifq_dec_sleep may return garbage
If 'm = ifq->ifq_ops->ifqop_deq_begin(ifq, )' is not NULL
the loop is exited and an uninitialized 'int error' is returned.
Several lines below error is checked for '!= 0', so i assume it
was meant to be initialized to '0'.
ok?
Index: ifq.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/ifq.c,v
retrieving revision 1.36
diff -u -p -r1.36 ifq.c
--- ifq.c 25 Jan 2020 06:31:32 - 1.36
+++ ifq.c 9 Mar 2020 22:57:58 -
@@ -395,7 +395,7 @@ ifq_deq_sleep(struct ifqueue *ifq, struc
{
struct mbuf *m;
void *cookie;
- int error;
+ int error = 0;
ifq_deq_enter(ifq);
if (ifq->ifq_len == 0 && nbio)
Re: softraid: too many arguments for sr_error
On Mon, Mar 09, 2020 at 11:41:14PM +0100, Tobias Heider wrote: > sr_error takes a sr_softc and a printf like format string + varargs. > There's no need to pass DEVNAME(sc) here. Either that or embed the device name in the format string; its usage inside error messages is already inconsistent around there, so OK kn either ways.
Re: net/if.c: nullptr deref in if_hooks_run
On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote: > there seems to be a nullptr dereference in if_hooks_run. Did your kernel crash here or did you find reading alone? > When the inner while loop is exited because 't == NULL' the next > line is an access to 't->t_func'. Yes, reads obviously wrong. > Because 't==NULL' means the TAILQ is fully traversed I think we > should break and exit instead. Make sense, OK kn
softraid: too many arguments for sr_error
sr_error takes a sr_softc and a printf like format string + varargs.
There's no need to pass DEVNAME(sc) here.
ok?
Index: softraid.c
===
RCS file: /mount/openbsd/cvs/src/sys/dev/softraid.c,v
retrieving revision 1.398
diff -u -p -r1.398 softraid.c
--- softraid.c 13 Feb 2020 15:11:32 - 1.398
+++ softraid.c 9 Mar 2020 22:37:53 -
@@ -3769,7 +3769,7 @@ sr_ioctl_installboot(struct sr_softc *sc
if (sr_rw(sc, chunk->src_dev_mm, bootblk, bbs,
SR_BOOT_BLOCKS_OFFSET, B_WRITE)) {
- sr_error(sc, "failed to write boot block", DEVNAME(sc));
+ sr_error(sc, "failed to write boot block");
goto done;
}
snmpd(8): fix use of uninitialized pointer
Hi, The following diff fixes the use of the uninitialized pointer iter in trapcmd_exec(). iter is just initialized in traphandler_parse() if vers is SNMP_V2. In all other cases iter stays uninitialized and may dereferenced in trapcmd_exec(). OK? bye, Jan Index: traphandler.c === RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v retrieving revision 1.15 diff -u -p -r1.15 traphandler.c --- traphandler.c 24 Oct 2019 12:39:27 - 1.15 +++ traphandler.c 9 Mar 2020 21:59:56 - @@ -305,7 +305,7 @@ traphandler_fork_handler(struct privsep_ struct sockaddr *sa; char*buf; ssize_t n; - struct ber_element *req, *iter; + struct ber_element *req, *iter = NULL; struct trapcmd *cmd; struct ber_oid trapoid; u_intuptime;
net/if.c: nullptr deref in if_hooks_run
Hi, there seems to be a nullptr dereference in if_hooks_run. When the inner while loop is exited because 't == NULL' the next line is an access to 't->t_func'. Because 't==NULL' means the TAILQ is fully traversed I think we should break and exit instead. ok? Index: if.c === RCS file: /mount/openbsd/cvs/src/sys/net/if.c,v retrieving revision 1.600 diff -u -p -r1.600 if.c --- if.c24 Jan 2020 05:14:51 - 1.600 +++ if.c9 Mar 2020 21:25:06 - @@ -1055,6 +1055,8 @@ if_hooks_run(struct task_list *hooks) if (t == NULL) break; } + if (t == NULL) + break; func = t->t_func; arg = t->t_arg;
Regarding the understanding of the malloc(3) code
Hi there,
I am reading and learning the internals of malloc(3).
So, after compiling the debug version of libc and using it for one
basic sample code for malloc(3).
Not able to understand some parts of the following code snippet:
void
_malloc_init(int from_rthreads)
{
u_int i, nmutexes;
struct dir_info *d;
_MALLOC_LOCK(1);
if (!from_rthreads && mopts.malloc_pool[1]) {
_MALLOC_UNLOCK(1);
return;
}
if (!mopts.malloc_canary)
omalloc_init();
nmutexes = from_rthreads ? mopts.malloc_mutexes : 2;
if (((uintptr_t)_readonly & MALLOC_PAGEMASK) == 0)
mprotect(_readonly, sizeof(malloc_readonly),
PROT_READ | PROT_WRITE);
for (i = 0; i < nmutexes; i++) {
if (mopts.malloc_pool[i])
continue;
if (i == 0) {
omalloc_poolinit(, MAP_CONCEAL);
d->malloc_junk = 2;
d->malloc_cache = 0;
} else {
omalloc_poolinit(, 0);
d->malloc_junk = mopts.def_malloc_junk;
d->malloc_cache = mopts.def_malloc_cache;
}
d->mutex = i;
mopts.malloc_pool[i] = d;
}
if (from_rthreads)
mopts.malloc_mt = 1;
else
mopts.internal_funcs = 1;
/*
* Options have been set and will never be reset.
* Prevent further tampering with them.
*/
if (((uintptr_t)_readonly & MALLOC_PAGEMASK) == 0)
mprotect(_readonly, sizeof(malloc_readonly), PROT_READ);
_MALLOC_UNLOCK(1);
}
In the above code snippet, could some please through some light on the
following queries
1. Use of nmutexes?
2. And, why it is looping till nmutexes and calls function
omalloc_poolinit(, MAP_CONCEAL) /* when i == 0*/
and other calls to omalloc_poolinit(, 0) /* when i != 0 */
So, suppose in the case of nmutexes = 2, I am not sure where are the
uses of these 3 initialized pools, that is, malloc_pool[0],
malloc_pool[1] and malloc_pool[2]?
static void
omalloc_poolinit(struct dir_info **dp, int mmap_flag)
{
char *p;
size_t d_avail, regioninfo_size;
struct dir_info *d;
int i, j;
/*
* Allocate dir_info with a guard page on either side. Also
* randomise offset inside the page at which the dir_info
* lies (subject to alignment by 1 << MALLOC_MINSHIFT)
*/
if ((p = MMAPNONE(DIR_INFO_RSZ + (MALLOC_PAGESIZE * 2), mmap_flag)) ==
MAP_FAILED)
wrterror(NULL, "malloc init mmap failed");
mprotect(p + MALLOC_PAGESIZE, DIR_INFO_RSZ, PROT_READ | PROT_WRITE);
d_avail = (DIR_INFO_RSZ - sizeof(*d)) >> MALLOC_MINSHIFT;
d = (struct dir_info *)(p + MALLOC_PAGESIZE +
(arc4random_uniform(d_avail) << MALLOC_MINSHIFT));
rbytes_init(d);
d->regions_free = d->regions_total = MALLOC_INITIAL_REGIONS;
regioninfo_size = d->regions_total * sizeof(struct region_info);
d->r = MMAP(regioninfo_size, mmap_flag);
if (d->r == MAP_FAILED) {
d->regions_total = 0;
wrterror(NULL, "malloc init mmap failed");
}
for (i = 0; i <= MALLOC_MAXSHIFT; i++) {
LIST_INIT(>chunk_info_list[i]);
for (j = 0; j < MALLOC_CHUNK_LISTS; j++)
LIST_INIT(>chunk_dir[i][j]);
...
...
...
In the above code, inside function omalloc_poolinit(), first, it will
allocate dir_info structure with a guard page on *both sides* like
[guard page] [guard page]?
And, why it is initializing the list chunk_info_list to 32 times and
chunk_dir to 64 times, that is chunk_info_list[0...31] and
chunk_dir[0...31][0...31]?
Could someone please provide some hints on the above queries?
Regards,
Neeraj
Re: rpki-client: check fflush on output files
Jeremie Courreges-Anglas wrote:
Lovely!
> Agreed. Here's an updated diff that tests the return value of
> output_finish(). Suggestions welcome for the error message in this
> case.
>
> It also drops the extra "out = NULL;", and replaces logx() calls with
> warn(3) in this file. I see no reason to drop those messages and errno
> information. logx() seems used mostly for statistics.
>
> Thoughts, tests / oks?
>
>
> Index: extern.h
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 extern.h
> --- extern.h 6 Mar 2020 17:36:42 - 1.24
> +++ extern.h 9 Mar 2020 08:19:20 -
> @@ -372,7 +372,7 @@ extern char* outputdir;
> int outputfiles(struct vrp_tree *v);
> FILE *output_createtmp(char *);
> void output_cleantmp(void);
> -void output_finish(FILE *);
> +int output_finish(FILE *);
> int output_bgpd(FILE *, struct vrp_tree *);
> int output_bird1v4(FILE *, struct vrp_tree *);
> int output_bird1v6(FILE *, struct vrp_tree *);
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 9 Mar 2020 08:20:39 -
> @@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
>
> fout = output_createtmp(outputs[i].name);
> if (fout == NULL) {
> - logx("cannot create %s", outputs[i].name);
> + warn("cannot create %s", outputs[i].name);
> rc = 1;
> continue;
> }
> if ((*outputs[i].fn)(fout, v) != 0) {
> - logx("output for %s format failed", outputs[i].name);
> + warn("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> rc = 1;
> continue;
> }
> - output_finish(fout);
> + if (output_finish(fout) != 0) {
> + warn("finish for %s format failed", outputs[i].name);
> + output_cleantmp();
> + rc = 1;
> + continue;
> + }
> }
>
> return rc;
> @@ -108,14 +113,15 @@ output_createtmp(char *name)
> return f;
> }
>
> -void
> +int
> output_finish(FILE *out)
> {
> - fclose(out);
> - out = NULL;
> -
> - rename(output_tmpname, output_name);
> + if (fclose(out) != 0)
> + return -1;
> + if (rename(output_tmpname, output_name) == -1)
> + return -1;
> output_tmpname[0] = '\0';
> + return 0;
> }
>
> void
Re: diff: init efifb even if VGA is probed.
Hi, Thank you for your test and feedback. On Fri, 6 Mar 2020 16:38:24 -0600 Andrew Daugherity wrote: > On Sun, Mar 1, 2020 at 10:41 PM YASUOKA Masahiko wrote: >> >> Hi, >> >> The problems you are pointing seem to be the same problem. >> >> > I'll try to test this diff next week if I can schedule some downtime. >> >> Test is appreciated. >> >> Also I'd like to know the result of >> >> hexdump -C /var/db/acpi/FACP.1 >> >> when "Load Legacy Video Option ROM" setting is disabled. > > I just tested a -current kernel built yesterday with that diff (your > post on Feb. 20), but unfortunately it does not fix the issue on my > hardware. As before, if "Load Legacy Video Option ROM" is disabled, > output is squished to a purple line and when devices are initialized, > vga1 is the wsdisplay0 device: I see, first diff didn't fix the problem on your machine. > vga1 at pci7 dev 0 function 0 "Matrox MGA G200eR" rev 0x01 > wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation) > wsdisplay0: screen 1-5 added (80x25, vt100 emulation) > efifb0 at mainbus0: 1280x1024, 32bpp > wsdisplay at efifb0 not configured > > vs. with the legacy video ROM setting: > > "Matrox MGA G200eR" rev 0x01 at pci7 dev 0 function 0 not configured > efifb0 at mainbus0: 1024x768, 32bpp > wsdisplay0 at efifb0 mux 1 > wsdisplay0: screen 0-5 added (std, vt100 emulation) > > I'm using a serial console, if it matters. Hmm... I just noticed that > with the legacy ROM setting disabled, both wsdisplay0 at vga1 mux > 1/wskbd0 at ukbd0 *and* com1 claim to be the console. With the > setting enabled (and efifb working), only com1 is listed as console. > > I haven't tried any of the later diffs as I'm not sure which are still > recommended. The last diff should fix the problem since it will initialize efifb before initializing VGA without condition. https://marc.info/?l=openbsd-tech=158280719421562=2 > The FACP.1 table does not change when the "Load Legacy Video Option > ROM" setting is changed. Here is its hexdump: > andrew@gsc-lb1:~/acpidump$ hexdump -C legacy-2.8.1/FACP.1 > 46 41 43 50 0c 01 00 00 05 62 44 45 4c 4c 20 20 |FACP.bDELL | > 0010 50 45 5f 53 43 33 20 20 00 00 00 00 44 45 4c 4c |PE_SC3 DELL| > 0020 01 00 00 00 00 30 f8 8e 00 b0 fc 8e 00 04 09 00 |.0..| > 0030 b2 00 00 00 f0 f1 f2 00 00 18 00 00 00 00 00 00 || > 0040 04 18 00 00 00 00 00 00 50 18 00 00 08 18 00 00 |P...| > 0050 80 18 00 00 00 00 00 00 04 02 01 04 20 00 10 00 | ...| > 0060 65 00 e9 03 00 00 00 00 01 03 0d 00 32 11 00 00 |e...2...| > 0070 a5 86 00 00 01 08 00 01 f9 0c 00 00 00 00 00 00 || > 0080 06 00 00 00 00 00 00 00 00 00 00 00 00 b0 fc 8e || > 0090 00 00 00 00 01 20 00 02 00 18 00 00 00 00 00 00 |. ..| > 00a0 01 00 00 02 00 00 00 00 00 00 00 00 01 10 00 02 || > 00b0 04 18 00 00 00 00 00 00 01 00 00 02 00 00 00 00 || > 00c0 00 00 00 00 01 08 00 01 50 18 00 00 00 00 00 00 |P...| > 00d0 01 20 00 03 08 18 00 00 00 00 00 00 01 00 00 01 |. ..| > 00e0 80 18 00 00 00 00 00 00 01 00 00 01 00 00 00 00 || > 00f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 || > 0100 00 00 00 00 00 00 00 00 00 00 00 00 || > 010c This was to check whether using "VGA Not Present" bit is useful on your machine. "Boot IA-PC Boot Architecture Flags" is 0x6D:6E = 0x0011, LEGACY_DEVICES bit is set, "VGA Not Present" is cleared. This means the bit isn't set as I expected, it isn't useful to know existance of VGA. > The only ACPI change made by toggling that option is the DMAR.25 > table. Here are both versions: > Legacy Video ROM enabled: > 44 4d 41 52 90 00 00 00 01 83 49 4e 54 45 4c 20 |DMAR..INTEL | > 0010 47 4e 4c 52 00 00 00 00 01 00 00 00 49 4e 54 4c |GNLRINTL| > 0020 01 00 00 00 26 01 00 00 00 00 00 00 00 00 00 00 |&...| > 0030 00 00 20 00 01 00 00 00 00 00 d9 fe 00 00 00 00 |.. .| > 0040 03 08 00 00 02 f0 1f 00 04 08 00 00 00 00 1f 00 || > 0050 01 00 20 00 00 00 00 00 00 b0 ba 7c 00 00 00 00 |.. || > 0060 ff 2f bb 84 00 00 00 00 01 08 00 00 00 01 00 00 |./..| > 0070 01 00 20 00 00 00 00 00 00 10 31 8e 00 00 00 00 |.. ...1.| > 0080 ff 0f 33 8e 00 00 00 00 01 08 00 00 00 00 14 00 |..3.| > 0090 > and disabled: > 44 4d 41 52 90 00 00 00 01 05 49 4e 54 45 4c 20 |DMAR..INTEL | > 0010 47 4e 4c 52 00 00 00 00 01 00 00 00 49 4e 54 4c |GNLRINTL| > 0020 01 00 00 00 26 01 00 00 00 00 00 00 00 00 00 00 |&...| > 0030 00 00 20 00 01 00 00 00 00 00 d9 fe 00 00 00 00 |.. .| > 0040 03 08 00 00 02 f0 1f 00 04 08
Re: tweak how amd64 (not intel) cpu topology is calculated
On 2020/03/09 22:50, David Gwynne wrote: > this works better on his epyc 2 box, and works right on my epyc 1, esxi > on epyc 1, and on an apu1. Fine on apu2 (GX-412TC) and the old HP microserver (Turion N40L) also. Diff makes sense and I'm happy you found an alternative to my dodgy CPU_INFO_FOREACH :) >> microserver OpenBSD 6.6-current (GENERIC.MP) #16: Mon Mar 9 12:58:00 GMT 2020 bu...@symphytum.spacehopper.org:/sys/arch/amd64/compile/GENERIC.MP real mem = 4244176896 (4047MB) avail mem = 4102959104 (3912MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.6 @ 0xfb330 (35 entries) bios0: vendor HP version "O41" date 07/29/2011 bios0: HP ProLiant MicroServer acpi0 at bios0: ACPI 3.0 acpi0: sleep states S0 S4 S5 acpi0: tables DSDT FACP APIC MCFG SPMI OEMB HPET EINJ BERT ERST HEST SSDT acpi0: wakeup devices PCE2(S4) PCE3(S4) PCE4(S4) PCE5(S4) PCE6(S4) PCE7(S4) PCE9(S4) PCEA(S4) PCEB(S4) PCEC(S4) SBAZ(S4) P0PC(S4) PE20(S4) PE21(S4) PE22(S4) PE23(S4) acpitimer0 at acpi0: 3579545 Hz, 32 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: AMD Turion(tm) II Neo N40L Dual-Core Processor, 1502.35 MHz, 10-06-03 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,MWAIT,CX16,POPCNT,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,3DNOW2,3DNOW,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,NODEID,ITSC cpu0: 64KB 64b/line 2-way I-cache, 64KB 64b/line 2-way D-cache, 1MB 64b/line 16-way L2 cache cpu0: ITLB 32 4KB entries fully associative, 16 4MB entries fully associative cpu0: DTLB 48 4KB entries fully associative, 48 4MB entries fully associative cpu0: AMD erratum 721 detected and fixed cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 199MHz cpu0: mwait min=64, max=64, IBE cpu1 at mainbus0: apid 1 (application processor) cpu1: AMD Turion(tm) II Neo N40L Dual-Core Processor, 1497.53 MHz, 10-06-03 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,MWAIT,CX16,POPCNT,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,3DNOW2,3DNOW,LAHF,CMPLEG,SVM,EAPICSP,AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,IBS,SKINIT,NODEID,ITSC cpu1: 64KB 64b/line 2-way I-cache, 64KB 64b/line 2-way D-cache, 1MB 64b/line 16-way L2 cache cpu1: ITLB 32 4KB entries fully associative, 16 4MB entries fully associative cpu1: DTLB 48 4KB entries fully associative, 48 4MB entries fully associative cpu1: AMD erratum 721 detected and fixed cpu1: smt 0, core 1, package 0 ioapic0 at mainbus0: apid 2 pa 0xfec0, version 21, 24 pins acpimcfg0 at acpi0 acpimcfg0: addr 0xe000, bus 0-255 acpihpet0 at acpi0: 14318180 Hz acpi0: unable to load \\_SB_._INI.EXH2 acpiprt0 at acpi0: bus 0 (PCI0) acpiprt1 at acpi0: bus 1 (P0P1) acpiprt2 at acpi0: bus -1 (PCE2) acpiprt3 at acpi0: bus -1 (PCE4) acpiprt4 at acpi0: bus 2 (PCE6) acpicpu0 at acpi0: C1(@1 halt!), PSS acpicpu1 at acpi0: C1(@1 halt!), PSS acpipci0 at acpi0 PCI0: 0x0010 0x0011 0x acpicmos0 at acpi0 acpibtn0 at acpi0: PWRB ipmi at mainbus0 not configured cpu0: 1502 MHz: speeds: 1500 1300 1000 800 MHz pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 "AMD RS880 Host" rev 0x00 ppb0 at pci0 dev 1 function 0 vendor "Hewlett-Packard", unknown product 0x9602 rev 0x00 pci1 at ppb0 bus 1 radeondrm0 at pci1 dev 5 function 0 "ATI Mobility Radeon HD 4200" rev 0x00 drm0 at radeondrm0 radeondrm0: apic 2 int 18 ppb1 at pci0 dev 6 function 0 "AMD RS780 PCIE" rev 0x00 pci2 at ppb1 bus 2 bge0 at pci2 dev 0 function 0 "Broadcom BCM5723" rev 0x10, BCM5784 A1 (0x5784100): msi, address e4:11:5b:12:bd:d6 brgphy0 at bge0 phy 1: BCM5784 10/100/1000baseT PHY, rev. 4 ahci0 at pci0 dev 17 function 0 "ATI SBx00 SATA" rev 0x40: apic 2 int 19, AHCI 1.2 ahci0: port 0: 3.0Gb/s ahci0: port 2: 3.0Gb/s scsibus1 at ahci0: 32 targets sd0 at scsibus1 targ 0 lun 0: naa.5000cca222dd41be sd0: 1907729MB, 512 bytes/sector, 3907029168 sectors sd1 at scsibus1 targ 2 lun 0: naa.5000cca222f32c62 sd1: 1907729MB, 512 bytes/sector, 3907029168 sectors ohci0 at pci0 dev 18 function 0 "ATI SB700 USB" rev 0x00: apic 2 int 18, version 1.0, legacy support ehci0 at pci0 dev 18 function 2 "ATI SB700 USB2" rev 0x00: apic 2 int 17 usb0 at ehci0: USB revision 2.0 uhub0 at usb0 configuration 1 interface 0 "ATI EHCI root hub" rev 2.00/1.00 addr 1 ohci1 at pci0 dev 19 function 0 "ATI SB700 USB" rev 0x00: apic 2 int 18, version 1.0, legacy support ehci1 at pci0 dev 19 function 2 "ATI SB700 USB2" rev 0x00: apic 2 int 17 usb1 at ehci1: USB revision 2.0 uhub1 at usb1 configuration 1 interface 0 "ATI EHCI root hub" rev 2.00/1.00 addr 1 piixpm0 at pci0 dev 20 function 0 "ATI SBx00 SMBus" rev 0x42: SMI iic0 at piixpm0 sdtemp0 at iic0 addr 0x18: stts2002 sdtemp1 at iic0 addr 0x19: mcp98243 spdmem0 at iic0 addr 0x50: 2GB DDR3 SDRAM ECC PC3-10600 with thermal sensor spdmem1
Re: rpki-client: check fflush on output files
On Mon, Mar 09, 2020 at 09:41:37AM +0100, Jeremie Courreges-Anglas wrote:
> On Sat, Mar 07 2020, Claudio Jeker wrote:
> > On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
> >> On Fri, Mar 06 2020, "Theo de Raadt" wrote:
> >> >> Jeremie Courreges-Anglas wrote:
> >> >> >
> >> >> >
> >> >> > Checking the return value of fprintf is good but not enough to ensure
> >> >> > that data has properly been written to the file without an error. To
> >> >> > do
> >> >> > that we can use fflush(3) in a single place.
> >> [redacted]
> >> >> How about checking the return value of fclose() in output_finish()
> >> >> instead?
> >> > Oh you want to reach the error-reporting code...
> >>
> >> And the cleanup-on-error code. Doing it here looks more appealing than
> >> adding
> >>if (fflush(out) != 0)
> >>return -1;
> >>
> >> at the end of all the output_* functions.
> >>
> >> If you're careful about write errors you can avoid feeding an
> >> incomplete/garbage file to your BGP daemon. The code was already
> >> careful, but did not account for buffering. This is what this patch
> >> tries to address.
> >>
> >> >> > Build-tested only. ok?
> >> >> >
> >> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
> >> >> > I suspect it's a remnant from a time where the output FILE * was
> >> >> > a global. That would be a separate commit.
> >>
> >> Diff below again for convenience,
> >>
> >>
> >> Index: output.c
> >> ===
> >> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> >> retrieving revision 1.6
> >> diff -u -p -p -u -r1.6 output.c
> >> --- output.c 6 Mar 2020 17:36:42 - 1.6
> >> +++ output.c 6 Mar 2020 23:04:18 -
> >> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
> >>rc = 1;
> >>continue;
> >>}
> >> - if ((*outputs[i].fn)(fout, v) != 0) {
> >> + if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
> >>logx("output for %s format failed", outputs[i].name);
> >>fclose(fout);
> >>output_cleantmp();
> >> @@ -112,8 +112,6 @@ void
> >> output_finish(FILE *out)
> >> {
> >>fclose(out);
> >> - out = NULL;
> >> -
> >>rename(output_tmpname, output_name);
> >>output_tmpname[0] = '\0';
> >> }
> >>
> >
> > I think it would be better to pick up the fclose error in output_finish()
> > and while doing that also check for rename() errors. At least those errors
> > should be logged.
>
> Agreed. Here's an updated diff that tests the return value of
> output_finish(). Suggestions welcome for the error message in this
> case.
>
> It also drops the extra "out = NULL;", and replaces logx() calls with
> warn(3) in this file. I see no reason to drop those messages and errno
> information. logx() seems used mostly for statistics.
>
> Thoughts, tests / oks?
Works for me. Ok claudio@
PS: I think some of the output_* prototypes could be moved to output.c
since those functions are only used internally.
> Index: extern.h
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 extern.h
> --- extern.h 6 Mar 2020 17:36:42 - 1.24
> +++ extern.h 9 Mar 2020 08:19:20 -
> @@ -372,7 +372,7 @@ extern char* outputdir;
> int outputfiles(struct vrp_tree *v);
> FILE *output_createtmp(char *);
> void output_cleantmp(void);
> -void output_finish(FILE *);
> +int output_finish(FILE *);
> int output_bgpd(FILE *, struct vrp_tree *);
> int output_bird1v4(FILE *, struct vrp_tree *);
> int output_bird1v6(FILE *, struct vrp_tree *);
> Index: output.c
> ===
> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 output.c
> --- output.c 6 Mar 2020 17:36:42 - 1.6
> +++ output.c 9 Mar 2020 08:20:39 -
> @@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
>
> fout = output_createtmp(outputs[i].name);
> if (fout == NULL) {
> - logx("cannot create %s", outputs[i].name);
> + warn("cannot create %s", outputs[i].name);
> rc = 1;
> continue;
> }
> if ((*outputs[i].fn)(fout, v) != 0) {
> - logx("output for %s format failed", outputs[i].name);
> + warn("output for %s format failed", outputs[i].name);
> fclose(fout);
> output_cleantmp();
> rc = 1;
> continue;
> }
> - output_finish(fout);
> + if (output_finish(fout) != 0) {
>
Re: tweak how amd64 (not intel) cpu topology is calculated
On Mon, Mar 09, 2020 at 05:00:37PM +1000, David Gwynne wrote:
> ive been running multi-cpu/core openbsd VMs on esxi on top of amd
> epyc cpus, and have noticed for a long time now that for some reason
> openbsd decides that all the virtual cpus are threads on the one core.
> this is annoying, cos setting hw.smt=1 feels dirty and wrong.
>
> i spent the weekend figuring this out, and came up with the following.
>
> our current code assumes that some cpuid fields on recent CPUs contain
> information about package and core topologies which we base the package
> and core ids on. turns out we're pretty much alone in this assumption. if
> you're running on real hardware, they do tend to contain useful info,
> but virtual machines don't fill them in properly.
>
> every other operating system seems to rely on the one mechanism across
> all families. specifically, the initial local apic id provides a
> globally unique identifier that has bits representing at least the
> package (socket) the logical thread is on, plus the core, and if
> supported, the smt id. multi socket cpus provide information about
> how many bits there are before you get to the ones identifying the
> package, so we use that everywhere.
>
> only the most recent generation of cpu (zen) supports SMT, but the cpuid
> that provides that information is available on at least the previous
> gen. fortunately they made it so reading the number of threads per
> core falls back gracefully. this means we can default to there being
> one thread per core, and increase that if the cpuid is available and
> appropriately set.
>
> this seems to fix my vmware problem, but also still seems to work on my
> physical epyc boxes. unfortunately i do not have any other amd systems
> up and running at the moment, so i would appreciate testing on any and
> every amd based amd64 system.
>
> tests please. ok?
of course hrvoje found a bug. i cleaned up my diff too hard before
sending it out, and ended up reading nthreads from the wrong bits.
this works better on his epyc 2 box, and works right on my epyc 1, esxi
on epyc 1, and on an apu1.
Index: identcpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.113
diff -u -p -r1.113 identcpu.c
--- identcpu.c 14 Jun 2019 18:13:55 - 1.113
+++ identcpu.c 9 Mar 2020 12:38:59 -
@@ -824,37 +824,31 @@ cpu_topology(struct cpu_info *ci)
apicid = (ebx >> 24) & 0xff;
if (strcmp(cpu_vendor, "AuthenticAMD") == 0) {
+ uint32_t nthreads = 1; /* per core */
+ uint32_t thread_id; /* within a package */
+
/* We need at least apicid at CPUID 0x8008 */
if (ci->ci_pnfeatset < 0x8008)
goto no_topology;
- if (ci->ci_pnfeatset >= 0x801e) {
- struct cpu_info *ci_other;
- CPU_INFO_ITERATOR cii;
+ CPUID(0x8008, eax, ebx, ecx, edx);
+ core_bits = (ecx >> 12) & 0xf;
+ if (ci->ci_pnfeatset >= 0x801e) {
CPUID(0x801e, eax, ebx, ecx, edx);
- ci->ci_core_id = ebx & 0xff;
- ci->ci_pkg_id = ecx & 0xff;
- ci->ci_smt_id = 0;
- CPU_INFO_FOREACH(cii, ci_other) {
- if (ci != ci_other &&
- ci_other->ci_core_id == ci->ci_core_id &&
- ci_other->ci_pkg_id == ci->ci_pkg_id)
- ci->ci_smt_id++;
- }
- } else {
- CPUID(0x8008, eax, ebx, ecx, edx);
- core_bits = (ecx >> 12) & 0xf;
- if (core_bits == 0)
- goto no_topology;
- /* So coreidsize 2 gives 3, 3 gives 7... */
- core_mask = (1 << core_bits) - 1;
- /* Core id is the least significant considering mask */
- ci->ci_core_id = apicid & core_mask;
- /* Pkg id is the upper remaining bits */
- ci->ci_pkg_id = apicid & ~core_mask;
- ci->ci_pkg_id >>= core_bits;
+ nthreads = ((ebx >> 8) & 0xf) + 1;
}
+
+ /* Shift the core_bits off to get at the pkg bits */
+ ci->ci_pkg_id = apicid >> core_bits;
+
+ /* Get rid of the package bits */
+ core_mask = (1 << core_bits) - 1;
+ thread_id = apicid & core_mask;
+
+ /* Cut logical thread_id into core id, and smt id in a core */
+ ci->ci_core_id = thread_id / nthreads;
+ ci->ci_smt_id = thread_id % nthreads;
} else if (strcmp(cpu_vendor, "GenuineIntel") == 0) {
Re: iked(8): Use TAILQ_FOREACH_SAFE(3)
Hi Wataru,
On Mon, Mar 09, 2020 at 08:09:24PM +0900, Wataru Ashihara wrote:
> to improve readability.
>
> This is the first time of my commit to OpenBSD, so if I went something
> wrong, let me know that.
Thanks for sharing, committed!
>
> Index: sbin/iked/config.c
> ===
> RCS file: /cvs/src/sbin/iked/config.c,v
> retrieving revision 1.53
> diff -u -r1.53 config.c
> --- sbin/iked/config.c16 Jan 2020 20:05:00 - 1.53
> +++ sbin/iked/config.c9 Mar 2020 11:05:37 -
> @@ -258,11 +258,9 @@
> void
> config_free_proposals(struct iked_proposals *head, unsigned int proto)
> {
> - struct iked_proposal*prop, *next;
> -
> - for (prop = TAILQ_FIRST(head); prop != NULL; prop = next) {
> - next = TAILQ_NEXT(prop, prop_entry);
> + struct iked_proposal*prop, *proptmp;
>
> + TAILQ_FOREACH_SAFE(prop, head, prop_entry, proptmp) {
> /* Free any proposal or only selected SA proto */
> if (proto != 0 && prop->prop_protoid != proto)
> continue;
> @@ -293,14 +291,12 @@
> config_free_childsas(struct iked *env, struct iked_childsas *head,
> struct iked_spi *peerspi, struct iked_spi *localspi)
> {
> - struct iked_childsa *csa, *nextcsa, *ipcomp;
> + struct iked_childsa *csa, *csatmp, *ipcomp;
>
> if (localspi != NULL)
> bzero(localspi, sizeof(*localspi));
>
> - for (csa = TAILQ_FIRST(head); csa != NULL; csa = nextcsa) {
> - nextcsa = TAILQ_NEXT(csa, csa_entry);
> -
> + TAILQ_FOREACH_SAFE(csa, head, csa_entry, csatmp) {
> if (peerspi != NULL) {
> /* Only delete matching peer SPIs */
> if (peerspi->spi != csa->csa_peerspi)
> @@ -511,7 +507,7 @@
> int
> config_getreset(struct iked *env, struct imsg *imsg)
> {
> - struct iked_policy *pol, *nextpol;
> + struct iked_policy *pol, *poltmp;
> struct iked_sa *sa, *nextsa;
> struct iked_user*usr, *nextusr;
> unsigned int mode;
> @@ -521,9 +517,7 @@
>
> if (mode == RESET_ALL || mode == RESET_POLICY) {
> log_debug("%s: flushing policies", __func__);
> - for (pol = TAILQ_FIRST(>sc_policies);
> - pol != NULL; pol = nextpol) {
> - nextpol = TAILQ_NEXT(pol, pol_entry);
> + TAILQ_FOREACH_SAFE(pol, >sc_policies, pol_entry, poltmp) {
> config_free_policy(env, pol);
> }
> }
> Index: sbin/iked/ikev2.c
> ===
> RCS file: /cvs/src/sbin/iked/ikev2.c,v
> retrieving revision 1.190
> diff -u -r1.190 ikev2.c
> --- sbin/iked/ikev2.c 1 Mar 2020 19:17:58 - 1.190
> +++ sbin/iked/ikev2.c 9 Mar 2020 11:05:37 -
> @@ -3701,9 +3701,9 @@
> int
> ikev2_ikesa_enable(struct iked *env, struct iked_sa *sa, struct iked_sa *nsa)
> {
> - struct iked_childsa *csa, *nextcsa, *ipcomp;
> - struct iked_flow*flow, *nextflow;
> - struct iked_proposal*prop, *nextprop;
> + struct iked_childsa *csa, *csatmp, *ipcomp;
> + struct iked_flow*flow, *flowtmp;
> + struct iked_proposal*prop, *proptmp;
>
> log_debug("%s: IKE SA %p ispi %s rspi %s replaced"
> " by SA %p ispi %s rspi %s ",
> @@ -3729,9 +3729,7 @@
> sizeof(nsa->sa_peer_loaded));
>
> /* Transfer all Child SAs and flows from the old IKE SA */
> - for (flow = TAILQ_FIRST(>sa_flows); flow != NULL;
> - flow = nextflow) {
> - nextflow = TAILQ_NEXT(flow, flow_entry);
> + TAILQ_FOREACH_SAFE(flow, >sa_flows, flow_entry, flowtmp) {
> TAILQ_REMOVE(>sa_flows, flow, flow_entry);
> TAILQ_INSERT_TAIL(>sa_flows, flow,
> flow_entry);
> @@ -3739,9 +3737,7 @@
> flow->flow_local = >sa_local;
> flow->flow_peer = >sa_peer;
> }
> - for (csa = TAILQ_FIRST(>sa_childsas); csa != NULL;
> - csa = nextcsa) {
> - nextcsa = TAILQ_NEXT(csa, csa_entry);
> + TAILQ_FOREACH_SAFE(csa, >sa_childsas, csa_entry, csatmp) {
> TAILQ_REMOVE(>sa_childsas, csa, csa_entry);
> TAILQ_INSERT_TAIL(>sa_childsas, csa,
> csa_entry);
> @@ -3760,9 +3756,7 @@
> }
> }
> /* Transfer all non-IKE proposals */
> - for (prop = TAILQ_FIRST(>sa_proposals); prop != NULL;
> - prop = nextprop) {
> - nextprop = TAILQ_NEXT(prop, prop_entry);
> + TAILQ_FOREACH_SAFE(prop, >sa_proposals, prop_entry, proptmp) {
> if (prop->prop_protoid == IKEV2_SAPROTO_IKE)
> continue;
> TAILQ_REMOVE(>sa_proposals, prop, prop_entry);
> @@ -5599,13 +5593,11 @@
>
iked(8): Use TAILQ_FOREACH_SAFE(3)
to improve readability.
This is the first time of my commit to OpenBSD, so if I went something
wrong, let me know that.
Index: sbin/iked/config.c
===
RCS file: /cvs/src/sbin/iked/config.c,v
retrieving revision 1.53
diff -u -r1.53 config.c
--- sbin/iked/config.c 16 Jan 2020 20:05:00 - 1.53
+++ sbin/iked/config.c 9 Mar 2020 11:05:37 -
@@ -258,11 +258,9 @@
void
config_free_proposals(struct iked_proposals *head, unsigned int proto)
{
- struct iked_proposal*prop, *next;
-
- for (prop = TAILQ_FIRST(head); prop != NULL; prop = next) {
- next = TAILQ_NEXT(prop, prop_entry);
+ struct iked_proposal*prop, *proptmp;
+ TAILQ_FOREACH_SAFE(prop, head, prop_entry, proptmp) {
/* Free any proposal or only selected SA proto */
if (proto != 0 && prop->prop_protoid != proto)
continue;
@@ -293,14 +291,12 @@
config_free_childsas(struct iked *env, struct iked_childsas *head,
struct iked_spi *peerspi, struct iked_spi *localspi)
{
- struct iked_childsa *csa, *nextcsa, *ipcomp;
+ struct iked_childsa *csa, *csatmp, *ipcomp;
if (localspi != NULL)
bzero(localspi, sizeof(*localspi));
- for (csa = TAILQ_FIRST(head); csa != NULL; csa = nextcsa) {
- nextcsa = TAILQ_NEXT(csa, csa_entry);
-
+ TAILQ_FOREACH_SAFE(csa, head, csa_entry, csatmp) {
if (peerspi != NULL) {
/* Only delete matching peer SPIs */
if (peerspi->spi != csa->csa_peerspi)
@@ -511,7 +507,7 @@
int
config_getreset(struct iked *env, struct imsg *imsg)
{
- struct iked_policy *pol, *nextpol;
+ struct iked_policy *pol, *poltmp;
struct iked_sa *sa, *nextsa;
struct iked_user*usr, *nextusr;
unsigned int mode;
@@ -521,9 +517,7 @@
if (mode == RESET_ALL || mode == RESET_POLICY) {
log_debug("%s: flushing policies", __func__);
- for (pol = TAILQ_FIRST(>sc_policies);
- pol != NULL; pol = nextpol) {
- nextpol = TAILQ_NEXT(pol, pol_entry);
+ TAILQ_FOREACH_SAFE(pol, >sc_policies, pol_entry, poltmp) {
config_free_policy(env, pol);
}
}
Index: sbin/iked/ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.190
diff -u -r1.190 ikev2.c
--- sbin/iked/ikev2.c 1 Mar 2020 19:17:58 - 1.190
+++ sbin/iked/ikev2.c 9 Mar 2020 11:05:37 -
@@ -3701,9 +3701,9 @@
int
ikev2_ikesa_enable(struct iked *env, struct iked_sa *sa, struct iked_sa *nsa)
{
- struct iked_childsa *csa, *nextcsa, *ipcomp;
- struct iked_flow*flow, *nextflow;
- struct iked_proposal*prop, *nextprop;
+ struct iked_childsa *csa, *csatmp, *ipcomp;
+ struct iked_flow*flow, *flowtmp;
+ struct iked_proposal*prop, *proptmp;
log_debug("%s: IKE SA %p ispi %s rspi %s replaced"
" by SA %p ispi %s rspi %s ",
@@ -3729,9 +3729,7 @@
sizeof(nsa->sa_peer_loaded));
/* Transfer all Child SAs and flows from the old IKE SA */
- for (flow = TAILQ_FIRST(>sa_flows); flow != NULL;
-flow = nextflow) {
- nextflow = TAILQ_NEXT(flow, flow_entry);
+ TAILQ_FOREACH_SAFE(flow, >sa_flows, flow_entry, flowtmp) {
TAILQ_REMOVE(>sa_flows, flow, flow_entry);
TAILQ_INSERT_TAIL(>sa_flows, flow,
flow_entry);
@@ -3739,9 +3737,7 @@
flow->flow_local = >sa_local;
flow->flow_peer = >sa_peer;
}
- for (csa = TAILQ_FIRST(>sa_childsas); csa != NULL;
-csa = nextcsa) {
- nextcsa = TAILQ_NEXT(csa, csa_entry);
+ TAILQ_FOREACH_SAFE(csa, >sa_childsas, csa_entry, csatmp) {
TAILQ_REMOVE(>sa_childsas, csa, csa_entry);
TAILQ_INSERT_TAIL(>sa_childsas, csa,
csa_entry);
@@ -3760,9 +3756,7 @@
}
}
/* Transfer all non-IKE proposals */
- for (prop = TAILQ_FIRST(>sa_proposals); prop != NULL;
-prop = nextprop) {
- nextprop = TAILQ_NEXT(prop, prop_entry);
+ TAILQ_FOREACH_SAFE(prop, >sa_proposals, prop_entry, proptmp) {
if (prop->prop_protoid == IKEV2_SAPROTO_IKE)
continue;
TAILQ_REMOVE(>sa_proposals, prop, prop_entry);
@@ -5599,13 +5593,11 @@
ikev2_childsa_delete(struct iked *env, struct iked_sa *sa, uint8_t saproto,
uint64_t spi, uint64_t *spiptr, int cleanup)
{
- struct iked_childsa *csa, *nextcsa = NULL, *ipcomp;
+ struct iked_childsa *csa,
Re: Compiler warning in ctype.h
On 2020/03/09 11:34, Janne Johansson wrote: > Den fre 6 mars 2020 kl 12:29 skrev Thomas de Grivel : > > > Hello, > > > > I was using base gcc but switching to base clang fixes the warnings on > > -current at least. > > Is base gcc not supported anymore ? > > > > I think you are supposed to use whatever gets used when you call "cc" on > the OpenBSD platform you are on, and if need be, get gcc from ports for an > uptodate version of it. Some arches (aarch64) only ever had clang. Some arches (i386, armv7) moved to clang and have removed gcc. Some arches (amd64, mips64 [sgi,octeon]) have moved to clang but still have /usr/bin/gcc present, mostly so that people making changes to the base OS have some easy way to check if their changes don't negatively impact gcc 4.2.1 arches. (However this does make it harder to pick up on things in ports which hardcode "gcc" and fail if not present!). For these arches, clang is the supported compiler. Some arches still use gcc. gcc 3 for m88k. gcc 4.2.1 for alpha hppa mips64el powerpc sh sparc64. Of these, mips64el powerpc sparc64 *also* have clang built, but have not switched to it as the main system compiler. For all of these arches, gcc is the supported compiler. > Since arches are moving from gcc into clang (at various speeds), its not > unthinkable for some of them to have both over the transition, but the > "supported" one is always the binary that gets run if you use "cc" for > compiler and nothing else. yep.
Re: Compiler warning in ctype.h
Den fre 6 mars 2020 kl 12:29 skrev Thomas de Grivel : > Hello, > > I was using base gcc but switching to base clang fixes the warnings on > -current at least. > Is base gcc not supported anymore ? > I think you are supposed to use whatever gets used when you call "cc" on the OpenBSD platform you are on, and if need be, get gcc from ports for an uptodate version of it. Since arches are moving from gcc into clang (at various speeds), its not unthinkable for some of them to have both over the transition, but the "supported" one is always the binary that gets run if you use "cc" for compiler and nothing else. -- May the most significant bit of your life be positive.
Re: rpki-client: check fflush on output files
On Sat, Mar 07 2020, Claudio Jeker wrote:
> On Sat, Mar 07, 2020 at 08:35:39AM +0100, Jeremie Courreges-Anglas wrote:
>> On Fri, Mar 06 2020, "Theo de Raadt" wrote:
>> >> Jeremie Courreges-Anglas wrote:
>> >> >
>> >> >
>> >> > Checking the return value of fprintf is good but not enough to ensure
>> >> > that data has properly been written to the file without an error. To do
>> >> > that we can use fflush(3) in a single place.
>> [redacted]
>> >> How about checking the return value of fclose() in output_finish()
>> >> instead?
>> > Oh you want to reach the error-reporting code...
>>
>> And the cleanup-on-error code. Doing it here looks more appealing than
>> adding
>> if (fflush(out) != 0)
>> return -1;
>>
>> at the end of all the output_* functions.
>>
>> If you're careful about write errors you can avoid feeding an
>> incomplete/garbage file to your BGP daemon. The code was already
>> careful, but did not account for buffering. This is what this patch
>> tries to address.
>>
>> >> > Build-tested only. ok?
>> >> >
>> >> > Bonus: in output_finish(), "out = NULL;" is pointless, so zap it.
>> >> > I suspect it's a remnant from a time where the output FILE * was
>> >> > a global. That would be a separate commit.
>>
>> Diff below again for convenience,
>>
>>
>> Index: output.c
>> ===
>> RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
>> retrieving revision 1.6
>> diff -u -p -p -u -r1.6 output.c
>> --- output.c 6 Mar 2020 17:36:42 - 1.6
>> +++ output.c 6 Mar 2020 23:04:18 -
>> @@ -71,7 +71,7 @@ outputfiles(struct vrp_tree *v)
>> rc = 1;
>> continue;
>> }
>> -if ((*outputs[i].fn)(fout, v) != 0) {
>> +if ((*outputs[i].fn)(fout, v) != 0 || fflush(fout) != 0) {
>> logx("output for %s format failed", outputs[i].name);
>> fclose(fout);
>> output_cleantmp();
>> @@ -112,8 +112,6 @@ void
>> output_finish(FILE *out)
>> {
>> fclose(out);
>> -out = NULL;
>> -
>> rename(output_tmpname, output_name);
>> output_tmpname[0] = '\0';
>> }
>>
>
> I think it would be better to pick up the fclose error in output_finish()
> and while doing that also check for rename() errors. At least those errors
> should be logged.
Agreed. Here's an updated diff that tests the return value of
output_finish(). Suggestions welcome for the error message in this
case.
It also drops the extra "out = NULL;", and replaces logx() calls with
warn(3) in this file. I see no reason to drop those messages and errno
information. logx() seems used mostly for statistics.
Thoughts, tests / oks?
Index: extern.h
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.24
diff -u -p -r1.24 extern.h
--- extern.h6 Mar 2020 17:36:42 - 1.24
+++ extern.h9 Mar 2020 08:19:20 -
@@ -372,7 +372,7 @@ extern char* outputdir;
int outputfiles(struct vrp_tree *v);
FILE *output_createtmp(char *);
voidoutput_cleantmp(void);
-voidoutput_finish(FILE *);
+int output_finish(FILE *);
int output_bgpd(FILE *, struct vrp_tree *);
int output_bird1v4(FILE *, struct vrp_tree *);
int output_bird1v6(FILE *, struct vrp_tree *);
Index: output.c
===
RCS file: /d/cvs/src/usr.sbin/rpki-client/output.c,v
retrieving revision 1.6
diff -u -p -r1.6 output.c
--- output.c6 Mar 2020 17:36:42 - 1.6
+++ output.c9 Mar 2020 08:20:39 -
@@ -67,18 +67,23 @@ outputfiles(struct vrp_tree *v)
fout = output_createtmp(outputs[i].name);
if (fout == NULL) {
- logx("cannot create %s", outputs[i].name);
+ warn("cannot create %s", outputs[i].name);
rc = 1;
continue;
}
if ((*outputs[i].fn)(fout, v) != 0) {
- logx("output for %s format failed", outputs[i].name);
+ warn("output for %s format failed", outputs[i].name);
fclose(fout);
output_cleantmp();
rc = 1;
continue;
}
- output_finish(fout);
+ if (output_finish(fout) != 0) {
+ warn("finish for %s format failed", outputs[i].name);
+ output_cleantmp();
+ rc = 1;
+ continue;
+ }
}
return rc;
@@ -108,14 +113,15 @@ output_createtmp(char *name)
return f;
}
-void
+int
output_finish(FILE *out)
{
- fclose(out);
- out = NULL;
tweak how amd64 (not intel) cpu topology is calculated
ive been running multi-cpu/core openbsd VMs on esxi on top of amd
epyc cpus, and have noticed for a long time now that for some reason
openbsd decides that all the virtual cpus are threads on the one core.
this is annoying, cos setting hw.smt=1 feels dirty and wrong.
i spent the weekend figuring this out, and came up with the following.
our current code assumes that some cpuid fields on recent CPUs contain
information about package and core topologies which we base the package
and core ids on. turns out we're pretty much alone in this assumption. if
you're running on real hardware, they do tend to contain useful info,
but virtual machines don't fill them in properly.
every other operating system seems to rely on the one mechanism across
all families. specifically, the initial local apic id provides a
globally unique identifier that has bits representing at least the
package (socket) the logical thread is on, plus the core, and if
supported, the smt id. multi socket cpus provide information about
how many bits there are before you get to the ones identifying the
package, so we use that everywhere.
only the most recent generation of cpu (zen) supports SMT, but the cpuid
that provides that information is available on at least the previous
gen. fortunately they made it so reading the number of threads per
core falls back gracefully. this means we can default to there being
one thread per core, and increase that if the cpuid is available and
appropriately set.
this seems to fix my vmware problem, but also still seems to work on my
physical epyc boxes. unfortunately i do not have any other amd systems
up and running at the moment, so i would appreciate testing on any and
every amd based amd64 system.
tests please. ok?
Index: identcpu.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.113
diff -u -p -r1.113 identcpu.c
--- identcpu.c 14 Jun 2019 18:13:55 - 1.113
+++ identcpu.c 9 Mar 2020 06:44:41 -
@@ -824,37 +824,31 @@ cpu_topology(struct cpu_info *ci)
apicid = (ebx >> 24) & 0xff;
if (strcmp(cpu_vendor, "AuthenticAMD") == 0) {
+ uint32_t nthreads = 1; /* per core */
+ uint32_t thread_id; /* within a package */
+
/* We need at least apicid at CPUID 0x8008 */
if (ci->ci_pnfeatset < 0x8008)
goto no_topology;
- if (ci->ci_pnfeatset >= 0x801e) {
- struct cpu_info *ci_other;
- CPU_INFO_ITERATOR cii;
+ CPUID(0x8008, eax, ebx, ecx, edx);
+ core_bits = (ecx >> 12) & 0xf;
+ if (ci->ci_pnfeatset >= 0x801e) {
CPUID(0x801e, eax, ebx, ecx, edx);
- ci->ci_core_id = ebx & 0xff;
- ci->ci_pkg_id = ecx & 0xff;
- ci->ci_smt_id = 0;
- CPU_INFO_FOREACH(cii, ci_other) {
- if (ci != ci_other &&
- ci_other->ci_core_id == ci->ci_core_id &&
- ci_other->ci_pkg_id == ci->ci_pkg_id)
- ci->ci_smt_id++;
- }
- } else {
- CPUID(0x8008, eax, ebx, ecx, edx);
- core_bits = (ecx >> 12) & 0xf;
- if (core_bits == 0)
- goto no_topology;
- /* So coreidsize 2 gives 3, 3 gives 7... */
- core_mask = (1 << core_bits) - 1;
- /* Core id is the least significant considering mask */
- ci->ci_core_id = apicid & core_mask;
- /* Pkg id is the upper remaining bits */
- ci->ci_pkg_id = apicid & ~core_mask;
- ci->ci_pkg_id >>= core_bits;
+ nthreads = ((ebx >> 12) & 0xf) + 1;
}
+
+ /* Shift the core_bits off to get at the pkg bits */
+ ci->ci_pkg_id = apicid >> core_bits;
+
+ /* Get rid of the package bits */
+ core_mask = (1 << core_bits) - 1;
+ thread_id = apicid & core_mask;
+
+ /* Cut logical thread_id into core id, and smt id in a core */
+ ci->ci_core_id = thread_id / nthreads;
+ ci->ci_smt_id = thread_id % nthreads;
} else if (strcmp(cpu_vendor, "GenuineIntel") == 0) {
/* We only support leaf 1/4 detection */
if (cpuid_level < 4)
