route(8) example for "out of prefix" default gateway
Seems some hosting providers have annoying "out of prefix" default gateways whuch are painful to configure (https://marc.info/?t=16678224225=1=2), should we give a pointer in route(8)? Index: route.8 === RCS file: /cvs/src/sbin/route/route.8,v retrieving revision 1.104 diff -u -p -r1.104 route.8 --- route.8 29 Jul 2022 18:28:32 - 1.104 +++ route.8 9 Nov 2022 07:29:59 - @@ -596,6 +596,14 @@ Delete the route to the 192.168.5.0/24 network: .Pp .Dl # route delete -inet 192.168.5.0/24 +.Pp +Add a static +.Xr inet6 4 +route to a host which is on the vio0 interface that is outside the prefix +configured on the interface, and use that host as a default gateway: +.Pp +.Dl # route add -inet6 2001:db8:efef::1 -cloning -link -iface vio0 +.Dl # route add -inet6 default 2001:db8:efef::1 .Sh DIAGNOSTICS .Bl -diag .It "%s: gateway %s flags %x"
Re: xenstore.c: return error number
From: Martin Pieuchot Date: Tue, 8 Nov 2022 11:12:43 + > On 01/11/22(Tue) 15:26, Masato Asou wrote: >> Hi, >> >> Return error number instead of call panic(). > > Makes sense to me. Do you know how this error can occur? Is is a logic > error or are we trusting values produced by a third party? You can reproduce this error by writing 1025 bytes from Dom0 and reading this from OpenBSD on the DomU as follows: dom0$ sudo xenstore-write /vm/408d6e98-1b95-43c2-803b-de24ee205631/data 012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789abcde domu$ hostctl /vm/408d6e98-1b95-43c2-803b-de24ee205631/data -- ASOU Masato >> comment, ok? >> -- >> ASOU Masato >> >> diff --git a/sys/dev/pv/xenstore.c b/sys/dev/pv/xenstore.c >> index 1e4f15d30eb..dc89ba0fa6d 100644 >> --- a/sys/dev/pv/xenstore.c >> +++ b/sys/dev/pv/xenstore.c >> @@ -118,6 +118,7 @@ struct xs_msg { >> struct xs_msghdr xsm_hdr; >> uint32_t xsm_read; >> uint32_t xsm_dlen; >> +int xsm_error; >> uint8_t *xsm_data; >> TAILQ_ENTRY(xs_msg) xsm_link; >> }; >> @@ -566,9 +567,7 @@ xs_intr(void *arg) >> } >> >> if (xsm->xsm_hdr.xmh_len > xsm->xsm_dlen) >> -panic("message too large: %d vs %d for type %d, rid %u", >> -xsm->xsm_hdr.xmh_len, xsm->xsm_dlen, xsm->xsm_hdr.xmh_type, >> -xsm->xsm_hdr.xmh_rid); >> +xsm->xsm_error = EMSGSIZE; >> >> len = MIN(xsm->xsm_hdr.xmh_len - xsm->xsm_read, avail); >> if (len) { >> @@ -800,7 +799,9 @@ xs_cmd(struct xs_transaction *xst, int cmd, const char >> *path, >> error = xs_geterror(xsm); >> DPRINTF("%s: xenstore request %d \"%s\" error %s\n", >> xs->xs_sc->sc_dev.dv_xname, cmd, path, xsm->xsm_data); >> -} else if (mode == READ) { >> +} else if (xsm->xsm_error != 0) >> +error = xsm->xsm_error; >> +else if (mode == READ) { >> KASSERT(iov && iov_cnt); >> error = xs_parse(xst, xsm, iov, iov_cnt); >> } >>
pause.3: Use Fn
Index: pause.3 === RCS file: /cvs/src/lib/libc/gen/pause.3,v retrieving revision 1.15 diff -u -p -r1.15 pause.3 --- pause.3 2 Aug 2022 01:23:23 - 1.15 +++ pause.3 8 Nov 2022 21:22:46 - @@ -39,7 +39,8 @@ .Fn pause void .Sh DESCRIPTION .Bf -symbolic -Pause is made obsolete by +.Fn pause +is made obsolete by .Xr sigsuspend 2 . .Ef .Pp
Re: ssh-keygen(1): by default generate ed25519 key (instead of rsa)
On Tue, 8 Nov 2022 at 14:23, Joerg Sonnenberger wrote: > Am Tue, Nov 08, 2022 at 01:23:52PM +1100 schrieb Darren Tucker: [...] > > Not quite: the default value for IdentityFile has RSA before ED25519. [...] > I tried that first and it picked up id_ed25519 from the agent, even if > both keys are accepted by the server. It prefers keys present in the agent as those don't require entering a passphrase. It'll also prefer keys explicitly specified by the user on the command line since that demonstrates user intent. And the behaviour is also modified by IdentitiesOnly. > I guess that makes the answer a case of "it's complicated". It is. And IdentityFile works differently to most other options (it's cumulative, not first-match) which was probably a mistake, but we're kind of stuck with it. -- Darren Tucker (dtucker at dtucker.net) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
Re: libcrypto: fix leak in BN_mpi2bn()
On Tue, Nov 08, 2022 at 11:06:43AM -0700, Todd C. Miller wrote: > On Tue, 08 Nov 2022 18:33:48 +0100, Tobias Heider wrote: > > > If ain == NULL then a points to newly malloced memory which should be > > freed when BN_bin2bn() fails. > > We don't have an "ain" function argument in LibreSSL so you will > need a larger diff. Perhaps something like this (untested). ok tb
ypbind segmentation fault
There is a segmentation fault that occurs in ypbind when domainname is not set Index: usr.sbin/ypbind/ypbind.c === RCS file: /cvs/src/usr.sbin/ypbind/ypbind.c,v retrieving revision 1.76 diff -u -r1.76 ypbind.c --- usr.sbin/ypbind/ypbind.c17 Jul 2022 03:12:20 - 1.76 +++ usr.sbin/ypbind/ypbind.c8 Nov 2022 11:04:29 - @@ -345,7 +345,7 @@ struct dirent *dent; yp_get_default_domain(); - if (domain[0] == '\0') { + if (domain == NULL) { fprintf(stderr, "domainname not set. Aborting.\n"); exit(1); }
Re: libcrypto: fix leak in x509_name_ex_d2i()
On 22-11-08 18:48:44, Tobias Heider wrote: > nm.a is initialized to NULL until it gets alloced by x509_name_ex_new(). > The following 'goto err' should free nm.a before returning. > > ok? Unless I'm missing something, I do not believe this is correct - nm is a union and nm.a is the same pointer as nm.x - nm.x is already freed via X509_NAME_free(), which would make this a double free. > Index: asn1/x_name.c > === > RCS file: /cvs/src/lib/libcrypto/asn1/x_name.c,v > retrieving revision 1.37 > diff -u -p -r1.37 x_name.c > --- asn1/x_name.c 25 Dec 2021 13:17:48 - 1.37 > +++ asn1/x_name.c 8 Nov 2022 17:45:08 - > @@ -340,6 +340,7 @@ x509_name_ex_d2i(ASN1_VALUE **val, const > err: > if (nm.x != NULL) > X509_NAME_free(nm.x); > + free(nm.a); > ASN1error(ERR_R_NESTED_ASN1_ERROR); > return 0; > }
Document global interface group list locking
The per-interface group list is protected by the net lock and already documented as such. The global interface group list `ifg_head' is also protected by the net lock and all access to it (all within if.c) take it accordingly. OK? --- sys/net/if.c | 3 ++- sys/net/if_var.h | 8 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index ad7d8ea5956..6a0ac9ee77f 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -226,7 +226,8 @@ voidif_idxmap_alloc(struct ifnet *); void if_idxmap_insert(struct ifnet *); void if_idxmap_remove(struct ifnet *); -TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); +TAILQ_HEAD(, ifg_group) ifg_head = +TAILQ_HEAD_INITIALIZER(ifg_head); /* [N] list of interface groups */ LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); /* [I] list of clonable interfaces */ diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 6272be882f4..275ea85bd2a 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -267,18 +267,18 @@ struct ifg_group { u_intifg_refcnt; caddr_t ifg_pf_kif; int ifg_carp_demoted; - TAILQ_HEAD(, ifg_member) ifg_members; - TAILQ_ENTRY(ifg_group) ifg_next; + TAILQ_HEAD(, ifg_member) ifg_members; /* [N] list of members per group */ + TAILQ_ENTRY(ifg_group) ifg_next;/* [N] all groups are chained */ }; struct ifg_member { - TAILQ_ENTRY(ifg_member) ifgm_next; + TAILQ_ENTRY(ifg_member) ifgm_next; /* [N] all members are chained */ struct ifnet*ifgm_ifp; }; struct ifg_list { struct ifg_group*ifgl_group; - TAILQ_ENTRY(ifg_list)ifgl_next; + TAILQ_ENTRY(ifg_list)ifgl_next; /* [N] all groups are chained */ }; #defineIFNET_SLOWTIMO 1 /* granularity is 1 second */ -- 2.38.1
Re: libcrypto potential memory leak in OBJ_NAME_add error path
> Actually, it needs freeing only here. If lh_OBJ_NAME_error() returns 0, > there was no error and a hash-entry containing NULL was replaced. The below diff is ok if you want to commit. PS: our initial diff causes a uaf caught by omalloc via: $ openssl x509 -in /etc/ssl/cert.pem -out /dev/null Index: objects/o_names.c === RCS file: /cvs/src/lib/libcrypto/objects/o_names.c,v retrieving revision 1.22 diff -u -p -r1.22 o_names.c --- objects/o_names.c 29 Jan 2017 17:49:23 - 1.22 +++ objects/o_names.c 8 Nov 2022 17:35:42 - @@ -197,6 +197,7 @@ OBJ_NAME_add(const char *name, int type, free(ret); } else { if (lh_OBJ_NAME_error(names_lh)) { + free(onp); /* ERROR */ return (0); }
Re: Document ifc_list immutability
ok > On 8 Nov 2022, at 21:38, Klemens Nanni wrote: > > On Tue, Nov 08, 2022 at 09:34:36PM +0300, Vitaliy Makkoveev wrote: >>> On 8 Nov 2022, at 21:26, Klemens Nanni wrote: >>> >>> On Tue, Nov 08, 2022 at 09:18:47PM +0300, Vitaliy Makkoveev wrote: > On 8 Nov 2022, at 21:08, Klemens Nanni wrote: > > Now properly. How about a single comment block at the top instead of > repeating it for every struct? > > You forgot to mark [I] `if_cloners’ within net/if.c. >>> >>> Like this? First locking commit in if.c. >>> >>> >> >> Please, do this in consistency with other places: >> >> LIST_HEAD(, ipsec_ids) ipsp_ids_gc_list = >>LIST_HEAD_INITIALIZER(ipsp_ids_gc_list);/* [F] */ > > OK? > > diff --git a/sys/net/if.c b/sys/net/if.c > index 58a972b802c..a7b0b9bb8a3 100644 > --- a/sys/net/if.c > +++ b/sys/net/if.c > @@ -228,8 +228,9 @@ void if_idxmap_remove(struct ifnet *); > > TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); > > -LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); > -int if_cloners_count; > +LIST_HEAD(, if_clone) if_cloners = > + LIST_HEAD_INITIALIZER(if_cloners); /* [I] list of clonable interfaces */ > +int if_cloners_count;/* [I] number of clonable interfaces */ > > struct rwlock if_cloners_lock = RWLOCK_INITIALIZER("clonelk"); > > diff --git a/sys/net/if_var.h b/sys/net/if_var.h > index 28514a0bfcd..6272be882f4 100644 > --- a/sys/net/if_var.h > +++ b/sys/net/if_var.h > @@ -73,6 +73,18 @@ > * interfaces. These routines live in the files if.c and route.c > */ > > +/* > + * Locks used to protect struct members in this file: > + * I immutable after creation > + * d protection left do the driver > + * c only used in ioctl or routing socket contexts (kernel lock) > + * K kernel lock > + * N net lock > + * > + * For SRP related structures that allow lock-free reads, the write lock > + * is indicated below. > + */ > + > struct rtentry; > struct ifnet; > struct task; > @@ -82,7 +94,7 @@ struct cpumem; > * Structure describing a `cloning' interface. > */ > struct if_clone { > - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ > + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ > const char *ifc_name; /* name of device, e.g. `gif' */ > size_t ifc_namelen; /* length of name */ > > @@ -99,17 +111,6 @@ struct if_clone { > .ifc_destroy= destroy, > \ > } > > -/* > - * Locks used to protect struct members in this file: > - * I immutable after creation > - * d protection left do the driver > - * c only used in ioctl or routing socket contexts (kernel lock) > - * K kernel lock > - * N net lock > - * > - * For SRP related structures that allow lock-free reads, the write lock > - * is indicated below. > - */ > /* > * Structure defining a queue for a network interface. > *
Re: Document ifc_list immutability
On Tue, Nov 08, 2022 at 09:34:36PM +0300, Vitaliy Makkoveev wrote: > > On 8 Nov 2022, at 21:26, Klemens Nanni wrote: > > > > On Tue, Nov 08, 2022 at 09:18:47PM +0300, Vitaliy Makkoveev wrote: > >>> On 8 Nov 2022, at 21:08, Klemens Nanni wrote: > >>> > >>> Now properly. How about a single comment block at the top instead of > >>> repeating it for every struct? > >>> > >>> > >> > >> You forgot to mark [I] `if_cloners’ within net/if.c. > > > > Like this? First locking commit in if.c. > > > > > > Please, do this in consistency with other places: > > LIST_HEAD(, ipsec_ids) ipsp_ids_gc_list = > LIST_HEAD_INITIALIZER(ipsp_ids_gc_list);/* [F] */ OK? diff --git a/sys/net/if.c b/sys/net/if.c index 58a972b802c..a7b0b9bb8a3 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -228,8 +228,9 @@ voidif_idxmap_remove(struct ifnet *); TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); -LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); -int if_cloners_count; +LIST_HEAD(, if_clone) if_cloners = + LIST_HEAD_INITIALIZER(if_cloners); /* [I] list of clonable interfaces */ +int if_cloners_count; /* [I] number of clonable interfaces */ struct rwlock if_cloners_lock = RWLOCK_INITIALIZER("clonelk"); diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 28514a0bfcd..6272be882f4 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -73,6 +73,18 @@ * interfaces. These routines live in the files if.c and route.c */ +/* + * Locks used to protect struct members in this file: + * I immutable after creation + * d protection left do the driver + * c only used in ioctl or routing socket contexts (kernel lock) + * K kernel lock + * N net lock + * + * For SRP related structures that allow lock-free reads, the write lock + * is indicated below. + */ + struct rtentry; struct ifnet; struct task; @@ -82,7 +94,7 @@ struct cpumem; * Structure describing a `cloning' interface. */ struct if_clone { - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ const char *ifc_name; /* name of device, e.g. `gif' */ size_t ifc_namelen; /* length of name */ @@ -99,17 +111,6 @@ struct if_clone { .ifc_destroy = destroy, \ } -/* - * Locks used to protect struct members in this file: - * I immutable after creation - * d protection left do the driver - * c only used in ioctl or routing socket contexts (kernel lock) - * K kernel lock - * N net lock - * - * For SRP related structures that allow lock-free reads, the write lock - * is indicated below. - */ /* * Structure defining a queue for a network interface. *
Re: Document ifc_list immutability
> On 8 Nov 2022, at 21:26, Klemens Nanni wrote: > > On Tue, Nov 08, 2022 at 09:18:47PM +0300, Vitaliy Makkoveev wrote: >>> On 8 Nov 2022, at 21:08, Klemens Nanni wrote: >>> >>> Now properly. How about a single comment block at the top instead of >>> repeating it for every struct? >>> >>> >> >> You forgot to mark [I] `if_cloners’ within net/if.c. > > Like this? First locking commit in if.c. > > Please, do this in consistency with other places: LIST_HEAD(, ipsec_ids) ipsp_ids_gc_list = LIST_HEAD_INITIALIZER(ipsp_ids_gc_list);/* [F] */ > diff --git a/sys/net/if.c b/sys/net/if.c > index 58a972b802c..b7fc5258bd4 100644 > --- a/sys/net/if.c > +++ b/sys/net/if.c > @@ -228,6 +228,7 @@ void if_idxmap_remove(struct ifnet *); > > TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); > > +/* [I] list of clonable interfaces */ > LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); > int if_cloners_count; > > diff --git a/sys/net/if_var.h b/sys/net/if_var.h > index 28514a0bfcd..6272be882f4 100644 > --- a/sys/net/if_var.h > +++ b/sys/net/if_var.h > @@ -73,6 +73,18 @@ > * interfaces. These routines live in the files if.c and route.c > */ > > +/* > + * Locks used to protect struct members in this file: > + * I immutable after creation > + * d protection left do the driver > + * c only used in ioctl or routing socket contexts (kernel lock) > + * K kernel lock > + * N net lock > + * > + * For SRP related structures that allow lock-free reads, the write lock > + * is indicated below. > + */ > + > struct rtentry; > struct ifnet; > struct task; > @@ -82,7 +94,7 @@ struct cpumem; > * Structure describing a `cloning' interface. > */ > struct if_clone { > - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ > + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ > const char *ifc_name; /* name of device, e.g. `gif' */ > size_t ifc_namelen; /* length of name */ > > @@ -99,17 +111,6 @@ struct if_clone { > .ifc_destroy= destroy, > \ > } > > -/* > - * Locks used to protect struct members in this file: > - * I immutable after creation > - * d protection left do the driver > - * c only used in ioctl or routing socket contexts (kernel lock) > - * K kernel lock > - * N net lock > - * > - * For SRP related structures that allow lock-free reads, the write lock > - * is indicated below. > - */ > /* > * Structure defining a queue for a network interface. > * >
Re: Document ifc_list immutability
On Tue, Nov 08, 2022 at 09:18:47PM +0300, Vitaliy Makkoveev wrote: > > On 8 Nov 2022, at 21:08, Klemens Nanni wrote: > > > > Now properly. How about a single comment block at the top instead of > > repeating it for every struct? > > > > > > You forgot to mark [I] `if_cloners’ within net/if.c. Like this? First locking commit in if.c. diff --git a/sys/net/if.c b/sys/net/if.c index 58a972b802c..b7fc5258bd4 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -228,6 +228,7 @@ voidif_idxmap_remove(struct ifnet *); TAILQ_HEAD(, ifg_group) ifg_head = TAILQ_HEAD_INITIALIZER(ifg_head); +/* [I] list of clonable interfaces */ LIST_HEAD(, if_clone) if_cloners = LIST_HEAD_INITIALIZER(if_cloners); int if_cloners_count; diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 28514a0bfcd..6272be882f4 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -73,6 +73,18 @@ * interfaces. These routines live in the files if.c and route.c */ +/* + * Locks used to protect struct members in this file: + * I immutable after creation + * d protection left do the driver + * c only used in ioctl or routing socket contexts (kernel lock) + * K kernel lock + * N net lock + * + * For SRP related structures that allow lock-free reads, the write lock + * is indicated below. + */ + struct rtentry; struct ifnet; struct task; @@ -82,7 +94,7 @@ struct cpumem; * Structure describing a `cloning' interface. */ struct if_clone { - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ const char *ifc_name; /* name of device, e.g. `gif' */ size_t ifc_namelen; /* length of name */ @@ -99,17 +111,6 @@ struct if_clone { .ifc_destroy = destroy, \ } -/* - * Locks used to protect struct members in this file: - * I immutable after creation - * d protection left do the driver - * c only used in ioctl or routing socket contexts (kernel lock) - * K kernel lock - * N net lock - * - * For SRP related structures that allow lock-free reads, the write lock - * is indicated below. - */ /* * Structure defining a queue for a network interface. *
Re: Document ifc_list immutability
> On 8 Nov 2022, at 21:08, Klemens Nanni wrote: > > Now properly. How about a single comment block at the top instead of > repeating it for every struct? > > You forgot to mark [I] `if_cloners’ within net/if.c. > diff --git a/sys/net/if_var.h b/sys/net/if_var.h > index 28514a0bfcd..6272be882f4 100644 > --- a/sys/net/if_var.h > +++ b/sys/net/if_var.h > @@ -73,6 +73,18 @@ > * interfaces. These routines live in the files if.c and route.c > */ > > +/* > + * Locks used to protect struct members in this file: > + * I immutable after creation > + * d protection left do the driver > + * c only used in ioctl or routing socket contexts (kernel lock) > + * K kernel lock > + * N net lock > + * > + * For SRP related structures that allow lock-free reads, the write lock > + * is indicated below. > + */ > + > struct rtentry; > struct ifnet; > struct task; > @@ -82,7 +94,7 @@ struct cpumem; > * Structure describing a `cloning' interface. > */ > struct if_clone { > - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ > + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ > const char *ifc_name; /* name of device, e.g. `gif' */ > size_t ifc_namelen; /* length of name */ > > @@ -99,17 +111,6 @@ struct if_clone { > .ifc_destroy= destroy, > \ > } > > -/* > - * Locks used to protect struct members in this file: > - * I immutable after creation > - * d protection left do the driver > - * c only used in ioctl or routing socket contexts (kernel lock) > - * K kernel lock > - * N net lock > - * > - * For SRP related structures that allow lock-free reads, the write lock > - * is indicated below. > - */ > /* > * Structure defining a queue for a network interface. > * >
Re: libcrypto: fix leak in BN_mpi2bn()
On Tue, Nov 08, 2022 at 11:06:43AM -0700, Todd C. Miller wrote: > On Tue, 08 Nov 2022 18:33:48 +0100, Tobias Heider wrote: > > > If ain == NULL then a points to newly malloced memory which should be > > freed when BN_bin2bn() fails. > > We don't have an "ain" function argument in LibreSSL so you will > need a larger diff. Perhaps something like this (untested). > > - todd huh right, mixed them up. Updated diff looks ok to me. > > Index: lib/libcrypto/bn/bn_mpi.c > === > RCS file: /cvs/src/lib/libcrypto/bn/bn_mpi.c,v > retrieving revision 1.8 > diff -u -p -u -r1.8 bn_mpi.c > --- lib/libcrypto/bn/bn_mpi.c 29 Jan 2017 17:49:22 - 1.8 > +++ lib/libcrypto/bn/bn_mpi.c 8 Nov 2022 18:03:36 - > @@ -92,8 +92,9 @@ BN_bn2mpi(const BIGNUM *a, unsigned char > } > > BIGNUM * > -BN_mpi2bn(const unsigned char *d, int n, BIGNUM *a) > +BN_mpi2bn(const unsigned char *d, int n, BIGNUM *ain) > { > + BIGNUM *a = ain; > long len; > int neg = 0; > > @@ -121,8 +122,11 @@ BN_mpi2bn(const unsigned char *d, int n, > d += 4; > if ((*d) & 0x80) > neg = 1; > - if (BN_bin2bn(d, (int)len, a) == NULL) > + if (BN_bin2bn(d, (int)len, a) == NULL) { > + if (ain == NULL) > + BN_free(a); > return (NULL); > + } > a->neg = neg; > if (neg) { > BN_clear_bit(a, BN_num_bits(a) - 1); >
Document ifc_list immutability
Now properly. How about a single comment block at the top instead of repeating it for every struct? diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 28514a0bfcd..6272be882f4 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -73,6 +73,18 @@ * interfaces. These routines live in the files if.c and route.c */ +/* + * Locks used to protect struct members in this file: + * I immutable after creation + * d protection left do the driver + * c only used in ioctl or routing socket contexts (kernel lock) + * K kernel lock + * N net lock + * + * For SRP related structures that allow lock-free reads, the write lock + * is indicated below. + */ + struct rtentry; struct ifnet; struct task; @@ -82,7 +94,7 @@ struct cpumem; * Structure describing a `cloning' interface. */ struct if_clone { - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ const char *ifc_name; /* name of device, e.g. `gif' */ size_t ifc_namelen; /* length of name */ @@ -99,17 +111,6 @@ struct if_clone { .ifc_destroy = destroy, \ } -/* - * Locks used to protect struct members in this file: - * I immutable after creation - * d protection left do the driver - * c only used in ioctl or routing socket contexts (kernel lock) - * K kernel lock - * N net lock - * - * For SRP related structures that allow lock-free reads, the write lock - * is indicated below. - */ /* * Structure defining a queue for a network interface. *
[no subject]
On Tue, 08 Nov 2022 18:05:24 +, Klemens Nanni wrote: > Subject: Document ifc_list immutability Sure. OK millert@ - todd
Re: libcrypto: fix leak in BN_mpi2bn()
On Tue, 08 Nov 2022 18:33:48 +0100, Tobias Heider wrote: > If ain == NULL then a points to newly malloced memory which should be > freed when BN_bin2bn() fails. We don't have an "ain" function argument in LibreSSL so you will need a larger diff. Perhaps something like this (untested). - todd Index: lib/libcrypto/bn/bn_mpi.c === RCS file: /cvs/src/lib/libcrypto/bn/bn_mpi.c,v retrieving revision 1.8 diff -u -p -u -r1.8 bn_mpi.c --- lib/libcrypto/bn/bn_mpi.c 29 Jan 2017 17:49:22 - 1.8 +++ lib/libcrypto/bn/bn_mpi.c 8 Nov 2022 18:03:36 - @@ -92,8 +92,9 @@ BN_bn2mpi(const BIGNUM *a, unsigned char } BIGNUM * -BN_mpi2bn(const unsigned char *d, int n, BIGNUM *a) +BN_mpi2bn(const unsigned char *d, int n, BIGNUM *ain) { + BIGNUM *a = ain; long len; int neg = 0; @@ -121,8 +122,11 @@ BN_mpi2bn(const unsigned char *d, int n, d += 4; if ((*d) & 0x80) neg = 1; - if (BN_bin2bn(d, (int)len, a) == NULL) + if (BN_bin2bn(d, (int)len, a) == NULL) { + if (ain == NULL) + BN_free(a); return (NULL); + } a->neg = neg; if (neg) { BN_clear_bit(a, BN_num_bits(a) - 1);
[no subject]
Subject: Document ifc_list immutability OK? diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 28514a0bfcd..a472c586f3c 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -78,11 +78,15 @@ struct ifnet; struct task; struct cpumem; +/* + * Locks used to protect struct members in this file: + * I immutable after creation + */ /* * Structure describing a `cloning' interface. */ struct if_clone { - LIST_ENTRY(if_clone) ifc_list; /* on list of cloners */ + LIST_ENTRY(if_clone) ifc_list; /* [I] on list of cloners */ const char *ifc_name; /* name of device, e.g. `gif' */ size_t ifc_namelen; /* length of name */
Re: riscv64: switch to clockintr(9)
On Tue, Nov 08 2022, Mark Kettenis wrote: >> Date: Tue, 8 Nov 2022 13:06:51 + >> From: Scott Cheloha >> >> On Mon, Nov 07, 2022 at 09:23:26PM +0100, Jeremie Courreges-Anglas wrote: >> > On Sun, Nov 06 2022, Scott Cheloha wrote: >> > > This patch switches riscv64 to clockintr(9). >> > > >> > > jca@ has been testing it (on a SiFive board?). It has survived two >> > > parallel release builds and upgrades from the resulting bsd.rd. >> > >> > I still get the same results on my HiFive Unmatched (produced by >> > SiFive indeed). The diff LGTM. >> > >> > Looks like clock and stat evcounts get merged with this diff. Not a big >> > problem but probably not intended? >> >> Whoops, good eye, forgot to mention that. >> >> This change is intended. With this patch, we can't keep a distinct >> count of hardclock() and statclock() calls anymore because >> clockintr_dispatch(9) handles the dispatch on behalf of the riscv64 >> code. So the "stat" evcount goes away. >> >> The "clock" evcount now represents a count of clock interrupts. >> That's it. One per riscv64 clock_intr() call. ack. Still ok jca@ > And I think that is fine. Those are the actual interrupts. Fine with me too! -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
libcrypto: fix leak in x509_name_ex_d2i()
nm.a is initialized to NULL until it gets alloced by x509_name_ex_new(). The following 'goto err' should free nm.a before returning. ok? Index: asn1/x_name.c === RCS file: /cvs/src/lib/libcrypto/asn1/x_name.c,v retrieving revision 1.37 diff -u -p -r1.37 x_name.c --- asn1/x_name.c 25 Dec 2021 13:17:48 - 1.37 +++ asn1/x_name.c 8 Nov 2022 17:45:08 - @@ -340,6 +340,7 @@ x509_name_ex_d2i(ASN1_VALUE **val, const err: if (nm.x != NULL) X509_NAME_free(nm.x); + free(nm.a); ASN1error(ERR_R_NESTED_ASN1_ERROR); return 0; }
Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 08:31:16PM +0300, Vitaliy Makkoveev wrote: > No reason to keep kernel lock around if_clone_list() call. Yes, that is why I will remove it in the very next commit. This way there is one "move existing lock, make things clearer" and one "actually remove a lock for a specific code path" change.
libcrypto: fix leak in BN_mpi2bn()
If ain == NULL then a points to newly malloced memory which should be freed when BN_bin2bn() fails. ok? Index: bn/bn_mpi.c === RCS file: /cvs/src/lib/libcrypto/bn/bn_mpi.c,v retrieving revision 1.8 diff -u -p -r1.8 bn_mpi.c --- bn/bn_mpi.c 29 Jan 2017 17:49:22 - 1.8 +++ bn/bn_mpi.c 8 Nov 2022 17:30:33 - @@ -121,8 +121,11 @@ BN_mpi2bn(const unsigned char *d, int n, d += 4; if ((*d) & 0x80) neg = 1; - if (BN_bin2bn(d, (int)len, a) == NULL) + if (BN_bin2bn(d, (int)len, a) == NULL) { + if (ain == NULL) + BN_free(a); return (NULL); + } a->neg = neg; if (neg) { BN_clear_bit(a, BN_num_bits(a) - 1);
Re: vmm(4): remove locking in vm_intr_pending
Mike Larkin writes: > This lock/unlock around an atomic operation was causing delays delivering > interupts into VMs. Pointed out by claudio@ when he ran md5 - in a VM > and it became very sluggish. > > Debugging help from dlg and mpi, thanks. > > ok? > ok dv@. This is overly defensive and from the time I was chasing issues on Intel hosts. Now that the VMCS is properly protected this can go. Removing the lock here speeds up booting my Ubuntu iso 10x, too 8) > -ml > > > Index: arch/amd64/amd64/vmm.c > === > RCS file: /cvs/src/sys/arch/amd64/amd64/vmm.c,v > retrieving revision 1.326 > diff -u -p -a -u -r1.326 vmm.c > --- arch/amd64/amd64/vmm.c7 Nov 2022 12:29:12 - 1.326 > +++ arch/amd64/amd64/vmm.c8 Nov 2022 15:29:10 - > @@ -894,9 +894,7 @@ vm_intr_pending(struct vm_intr_params *v > goto out; > } > > - rw_enter_write(>vc_lock); > vcpu->vc_intr = vip->vip_intr; > - rw_exit_write(>vc_lock); > > refcnt_rele_wake(>vc_refcnt); > out: > @@ -3526,7 +3524,7 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, s > vmx_setmsrbrw(vcpu, MSR_FSBASE); > vmx_setmsrbrw(vcpu, MSR_GSBASE); > vmx_setmsrbrw(vcpu, MSR_KERNELGSBASE); > - > + > vmx_setmsrbr(vcpu, MSR_MISC_ENABLE); > vmx_setmsrbr(vcpu, MSR_TSC); > > Index: arch/amd64/include/vmmvar.h > === > RCS file: /cvs/src/sys/arch/amd64/include/vmmvar.h,v > retrieving revision 1.81 > diff -u -p -a -u -r1.81 vmmvar.h > --- arch/amd64/include/vmmvar.h 1 Sep 2022 22:01:40 - 1.81 > +++ arch/amd64/include/vmmvar.h 8 Nov 2022 15:29:10 - > @@ -937,7 +937,7 @@ struct vcpu { > struct cpu_info *vc_last_pcpu; /* [v] */ > struct vm_exit vc_exit; /* [v] */ > > - uint16_t vc_intr; /* [v] */ > + uint16_t vc_intr; /* [a] */ > uint8_t vc_irqready;/* [v] */ > > uint8_t vc_fpuinited; /* [v] */
Re: push kernel lock inside ifioctl_get()
No reason to keep kernel lock around if_clone_list() call. > On 8 Nov 2022, at 20:27, Klemens Nanni wrote: > > On Tue, Nov 08, 2022 at 08:04:16PM +0300, Vitaliy Makkoveev wrote: >> The `if_cloners’ list is immutable. You don't need kernel lock >> around if_clone_list() call. >> >>> case SIOCIFGCLONERS: >>> + KERNEL_LOCK(); >>> error = if_clone_list((struct if_clonereq *)data); >>> + KERNEL_UNLOCK(); >>> return (error); >> >> >> With this fix diff looks good for me. > > This is going the be first unlocking diff, I just want to clearly > separate setup churn (like this) from actual lock semantic changes. >
Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 04:47:23PM +, Martin Pieuchot wrote: > On 08/11/22(Tue) 15:28, Klemens Nanni wrote: > > After this mechanical move, I can unlock the individual SIOCG* in there. > > I'd suggest grabbing the KERNEL_LOCK() after NET_LOCK_SHARED(). > Otherwise you might spin for the first one then release it when going > to sleep. I can do that inside the first switch, but we must grab the net lock before if_unit() which is called before grabbing the shared net lock. I can shuffle this around if you really want, or I can simply move the existing kernel lock further down and keep it in the same order just like it is now already. > > > OK? > > > > Index: if.c > > === > > RCS file: /cvs/src/sys/net/if.c,v > > retrieving revision 1.667 > > diff -u -p -r1.667 if.c > > --- if.c8 Nov 2022 15:20:24 - 1.667 > > +++ if.c8 Nov 2022 15:26:07 - > > @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data) > > size_t bytesdone; > > const char *label; > > > > - KERNEL_LOCK(); > > - > > switch(cmd) { > > case SIOCGIFCONF: > > + KERNEL_LOCK(); > > NET_LOCK_SHARED(); > > error = ifconf(data); > > NET_UNLOCK_SHARED(); > > + KERNEL_UNLOCK(); > > return (error); > > case SIOCIFGCLONERS: > > + KERNEL_LOCK(); > > error = if_clone_list((struct if_clonereq *)data); > > + KERNEL_UNLOCK(); > > return (error); > > case SIOCGIFGMEMB: > > + KERNEL_LOCK(); > > NET_LOCK_SHARED(); > > error = if_getgroupmembers(data); > > NET_UNLOCK_SHARED(); > > + KERNEL_UNLOCK(); > > return (error); > > case SIOCGIFGATTR: > > + KERNEL_LOCK(); > > NET_LOCK_SHARED(); > > error = if_getgroupattribs(data); > > NET_UNLOCK_SHARED(); > > + KERNEL_UNLOCK(); > > return (error); > > case SIOCGIFGLIST: > > + KERNEL_LOCK(); > > NET_LOCK_SHARED(); > > error = if_getgrouplist(data); > > NET_UNLOCK_SHARED(); > > + KERNEL_UNLOCK(); > > return (error); > > } > > + > > + KERNEL_LOCK(); > > > > ifp = if_unit(ifr->ifr_name); > > if (ifp == NULL) { > > >
Re: push kernel lock inside ifioctl_get()
On Tue, Nov 08, 2022 at 08:04:16PM +0300, Vitaliy Makkoveev wrote: > The `if_cloners’ list is immutable. You don't need kernel lock > around if_clone_list() call. > > > case SIOCIFGCLONERS: > > + KERNEL_LOCK(); > > error = if_clone_list((struct if_clonereq *)data); > > + KERNEL_UNLOCK(); > > return (error); > > > With this fix diff looks good for me. This is going the be first unlocking diff, I just want to clearly separate setup churn (like this) from actual lock semantic changes.
libcrypto: leak in DSA_print()
Same diff as for RSA_print(). Old version leaks when EVP_PKEY_set1_DSA() fails. ok? Index: dsa/dsa_prn.c === RCS file: /cvs/src/lib/libcrypto/dsa/dsa_prn.c,v retrieving revision 1.6 diff -u -p -r1.6 dsa_prn.c --- dsa/dsa_prn.c 29 Jan 2017 17:49:22 - 1.6 +++ dsa/dsa_prn.c 8 Nov 2022 17:22:08 - @@ -98,12 +98,16 @@ int DSA_print(BIO *bp, const DSA *x, int off) { EVP_PKEY *pk; - int ret; + int ret = 0; + + if ((pk = EVP_PKEY_new()) == NULL) + goto out; + + if (!EVP_PKEY_set1_DSA(pk, (DSA *)x)) + goto out; - pk = EVP_PKEY_new(); - if (!pk || !EVP_PKEY_set1_DSA(pk, (DSA *)x)) - return 0; ret = EVP_PKEY_print_private(bp, pk, off, NULL); + out: EVP_PKEY_free(pk); return ret; }
Re: push kernel lock inside ifioctl_get()
The `if_cloners’ list is immutable. You don't need kernel lock around if_clone_list() call. > case SIOCIFGCLONERS: > + KERNEL_LOCK(); > error = if_clone_list((struct if_clonereq *)data); > + KERNEL_UNLOCK(); > return (error); With this fix diff looks good for me. > On 8 Nov 2022, at 18:28, Klemens Nanni wrote: > > After this mechanical move, I can unlock the individual SIOCG* in there. > > OK? > > Index: if.c > === > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.667 > diff -u -p -r1.667 if.c > --- if.c 8 Nov 2022 15:20:24 - 1.667 > +++ if.c 8 Nov 2022 15:26:07 - > @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data) > size_t bytesdone; > const char *label; > > - KERNEL_LOCK(); > - > switch(cmd) { > case SIOCGIFCONF: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = ifconf(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCIFGCLONERS: > + KERNEL_LOCK(); > error = if_clone_list((struct if_clonereq *)data); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGMEMB: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgroupmembers(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGATTR: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgroupattribs(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGLIST: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgrouplist(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > } > + > + KERNEL_LOCK(); > > ifp = if_unit(ifr->ifr_name); > if (ifp == NULL) { >
Re: push kernel lock inside ifioctl_get()
On 08/11/22(Tue) 15:28, Klemens Nanni wrote: > After this mechanical move, I can unlock the individual SIOCG* in there. I'd suggest grabbing the KERNEL_LOCK() after NET_LOCK_SHARED(). Otherwise you might spin for the first one then release it when going to sleep. > OK? > > Index: if.c > === > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.667 > diff -u -p -r1.667 if.c > --- if.c 8 Nov 2022 15:20:24 - 1.667 > +++ if.c 8 Nov 2022 15:26:07 - > @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data) > size_t bytesdone; > const char *label; > > - KERNEL_LOCK(); > - > switch(cmd) { > case SIOCGIFCONF: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = ifconf(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCIFGCLONERS: > + KERNEL_LOCK(); > error = if_clone_list((struct if_clonereq *)data); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGMEMB: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgroupmembers(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGATTR: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgroupattribs(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > case SIOCGIFGLIST: > + KERNEL_LOCK(); > NET_LOCK_SHARED(); > error = if_getgrouplist(data); > NET_UNLOCK_SHARED(); > + KERNEL_UNLOCK(); > return (error); > } > + > + KERNEL_LOCK(); > > ifp = if_unit(ifr->ifr_name); > if (ifp == NULL) { >
Re: remove eeprom(8) sun4 leftovers
> Date: Tue, 8 Nov 2022 15:49:19 + > From: Miod Vallat > > The following diff removes the last mentions of the sun4 old style > eeprom behaviour in the eeprom(8) manual page, as well as options > specific to it. sure; ok kettenis@ > Index: eeprom.8 > === > RCS file: /OpenBSD/src/usr.sbin/eeprom/eeprom.8,v > retrieving revision 1.22 > diff -u -p -r1.22 eeprom.8 > --- eeprom.8 8 Jan 2020 14:45:36 - 1.22 > +++ eeprom.8 8 Nov 2022 15:42:12 - > @@ -33,12 +33,11 @@ > .Os > .Sh NAME > .Nm eeprom > -.Nd display or modify contents of the EEPROM or OpenPROM > +.Nd display or modify contents of the OpenPROM > .Sh SYNOPSIS > .Nm eeprom > -.Op Fl cipv > +.Op Fl pv > .Op Fl f Ar device > -.Op Fl N Ar system > .Oo > .Ar field Ns Op = Ns Ar value > .Ar ... > @@ -46,7 +45,7 @@ > .Sh DESCRIPTION > .Nm eeprom > provides an interface for displaying and changing the contents of the > -EEPROM or OpenPROM. > +OpenPROM. > Without any arguments, > .Nm eeprom > will list all of the known fields and their corresponding values. > @@ -55,145 +54,26 @@ When given the name of a specific field, > will display that value or set it if the field name is followed by > .Sq = > and a value. > -Only the superuser may modify the contents of the EEPROM or OpenPROM. > +Only the superuser may modify the contents of the OpenPROM. > .Pp > The options are as follows: > .Bl -tag -width Ds > .It Fl > Commands are taken from stdin and displayed on stdout. > -.It Fl c > -.Nm eeprom > -will fix incorrect checksum values and exit. > -This flag is quietly ignored on systems with an OpenPROM. > .It Fl f Ar device > -On systems with an EEPROM, use > -.Ar device > -instead of the default > -.Pa /dev/eeprom . > -On systems with an OpenPROM, use > +Use > .Ar device > instead of the default > .Pa /dev/openprom . > -.It Fl i > -If checksum values are incorrect, > -.Nm eeprom > -will ignore them and continue after displaying a warning. > -This flag is quietly ignored on systems with an OpenPROM. > -.It Fl N Ar system > -Use the system image > -.Ar system > -instead of the default > -.Pa /bsd . > .It Fl p > -On systems with an OpenPROM, display the tree derived from it and exit. > -This flag is quietly ignored on systems with an EEPROM. > +Display the tree derived from the OpenPROM and exit. > .It Fl v > -On systems with an OpenPROM, be verbose when setting a value. > -Systems with an EEPROM are always verbose. > +Be verbose when setting a value. > .El > .Sh FIELDS AND VALUES > -The following fields and values are for systems with an EEPROM: > -.Bl -tag -width "watchdog_reboot " > -.It Ar hwupdate > -A valid date, such as > -.Dq 7/12/95 . > -The strings > -.Dq today > -and > -.Dq now > -are also acceptable. > -.It Ar memsize > -How much memory, in megabytes, is installed in the system. > -.It Ar memtest > -How much memory, in megabytes, is to be tested upon power-up. > -.It Ar scrsize > -The size of the screen. > -Acceptable values are > -.Dq 1024x1024 , > -.Dq 1152x900 , > -.Dq 1600x1280 , > -and > -.Dq 1440x1440 . > -.It Ar watchdog_reboot > -If true, the system will reboot upon reset. > -Otherwise, the system will fall into the monitor. > -.It Ar default_boot > -If true, the system will use the boot device stored in > -.Ar bootdev . > -.It Ar bootdev > -Specifies the default boot device in the form cc(x,x,x), where > -.Dq cc > -is a combination of two letters such as > -.Dq sd > -or > -.Dq le > -and each > -.Dq x > -is a hexadecimal number between 0 and ff, less the prepending > -.Dq 0x . > -.It Ar kbdtype > -This value is > -.Dq 0 > -for all Sun keyboards. > -.It Ar console > -Specifies the console type. > -Valid values are > -.Dq b , > -.Dq ttya , > -.Dq ttyb , > -.Dq color , > -and > -.Dq p4opt . > -.It Ar keyclick > -If true, the keys click annoyingly. > -.It Ar diagdev > -This is a string very similar to that used by > -.Ar bootdev . > -It specifies the default boot device when the diagnostic switch is > -turned on. > -.It Ar diagpath > -A 40-character, NULL-terminated string specifying the kernel or stand-alone > -program to load when the diagnostic switch is turned on. > -.It Ar columns > -An 8-bit integer specifying the number of columns on the console. > -.It Ar rows > -An 8-bit integer specifying the number of rows on the console. > -.It Ar ttya_use_baud > -Use the baud rate stored in > -.Ar ttya_baud > -instead of the default 9600. > -.It Ar ttya_baud > -A 16-bit integer specifying the baud rate to use on ttya. > -.It Ar ttya_no_rtsdtr > -If true, disables RTS/DTR. > -.It Ar ttyb_use_baud > -Similar to > -.Ar ttya_use_baud , > -but for ttyb. > -.It Ar ttyb_baud > -Similar to > -.Ar ttya_baud , > -but for ttyb. > -.It Ar ttyb_no_rtsdtr > -Similar to > -.Ar ttya_no_rtsdtr , > -but for ttyb. > -.It Ar banner > -An 80-character, NULL-terminated string to use at power-up instead > -of the default Sun banner. > -.El > -.Pp > -Note that the
remove eeprom(8) sun4 leftovers
The following diff removes the last mentions of the sun4 old style eeprom behaviour in the eeprom(8) manual page, as well as options specific to it. Index: eeprom.8 === RCS file: /OpenBSD/src/usr.sbin/eeprom/eeprom.8,v retrieving revision 1.22 diff -u -p -r1.22 eeprom.8 --- eeprom.88 Jan 2020 14:45:36 - 1.22 +++ eeprom.88 Nov 2022 15:42:12 - @@ -33,12 +33,11 @@ .Os .Sh NAME .Nm eeprom -.Nd display or modify contents of the EEPROM or OpenPROM +.Nd display or modify contents of the OpenPROM .Sh SYNOPSIS .Nm eeprom -.Op Fl cipv +.Op Fl pv .Op Fl f Ar device -.Op Fl N Ar system .Oo .Ar field Ns Op = Ns Ar value .Ar ... @@ -46,7 +45,7 @@ .Sh DESCRIPTION .Nm eeprom provides an interface for displaying and changing the contents of the -EEPROM or OpenPROM. +OpenPROM. Without any arguments, .Nm eeprom will list all of the known fields and their corresponding values. @@ -55,145 +54,26 @@ When given the name of a specific field, will display that value or set it if the field name is followed by .Sq = and a value. -Only the superuser may modify the contents of the EEPROM or OpenPROM. +Only the superuser may modify the contents of the OpenPROM. .Pp The options are as follows: .Bl -tag -width Ds .It Fl Commands are taken from stdin and displayed on stdout. -.It Fl c -.Nm eeprom -will fix incorrect checksum values and exit. -This flag is quietly ignored on systems with an OpenPROM. .It Fl f Ar device -On systems with an EEPROM, use -.Ar device -instead of the default -.Pa /dev/eeprom . -On systems with an OpenPROM, use +Use .Ar device instead of the default .Pa /dev/openprom . -.It Fl i -If checksum values are incorrect, -.Nm eeprom -will ignore them and continue after displaying a warning. -This flag is quietly ignored on systems with an OpenPROM. -.It Fl N Ar system -Use the system image -.Ar system -instead of the default -.Pa /bsd . .It Fl p -On systems with an OpenPROM, display the tree derived from it and exit. -This flag is quietly ignored on systems with an EEPROM. +Display the tree derived from the OpenPROM and exit. .It Fl v -On systems with an OpenPROM, be verbose when setting a value. -Systems with an EEPROM are always verbose. +Be verbose when setting a value. .El .Sh FIELDS AND VALUES -The following fields and values are for systems with an EEPROM: -.Bl -tag -width "watchdog_reboot " -.It Ar hwupdate -A valid date, such as -.Dq 7/12/95 . -The strings -.Dq today -and -.Dq now -are also acceptable. -.It Ar memsize -How much memory, in megabytes, is installed in the system. -.It Ar memtest -How much memory, in megabytes, is to be tested upon power-up. -.It Ar scrsize -The size of the screen. -Acceptable values are -.Dq 1024x1024 , -.Dq 1152x900 , -.Dq 1600x1280 , -and -.Dq 1440x1440 . -.It Ar watchdog_reboot -If true, the system will reboot upon reset. -Otherwise, the system will fall into the monitor. -.It Ar default_boot -If true, the system will use the boot device stored in -.Ar bootdev . -.It Ar bootdev -Specifies the default boot device in the form cc(x,x,x), where -.Dq cc -is a combination of two letters such as -.Dq sd -or -.Dq le -and each -.Dq x -is a hexadecimal number between 0 and ff, less the prepending -.Dq 0x . -.It Ar kbdtype -This value is -.Dq 0 -for all Sun keyboards. -.It Ar console -Specifies the console type. -Valid values are -.Dq b , -.Dq ttya , -.Dq ttyb , -.Dq color , -and -.Dq p4opt . -.It Ar keyclick -If true, the keys click annoyingly. -.It Ar diagdev -This is a string very similar to that used by -.Ar bootdev . -It specifies the default boot device when the diagnostic switch is -turned on. -.It Ar diagpath -A 40-character, NULL-terminated string specifying the kernel or stand-alone -program to load when the diagnostic switch is turned on. -.It Ar columns -An 8-bit integer specifying the number of columns on the console. -.It Ar rows -An 8-bit integer specifying the number of rows on the console. -.It Ar ttya_use_baud -Use the baud rate stored in -.Ar ttya_baud -instead of the default 9600. -.It Ar ttya_baud -A 16-bit integer specifying the baud rate to use on ttya. -.It Ar ttya_no_rtsdtr -If true, disables RTS/DTR. -.It Ar ttyb_use_baud -Similar to -.Ar ttya_use_baud , -but for ttyb. -.It Ar ttyb_baud -Similar to -.Ar ttya_baud , -but for ttyb. -.It Ar ttyb_no_rtsdtr -Similar to -.Ar ttya_no_rtsdtr , -but for ttyb. -.It Ar banner -An 80-character, NULL-terminated string to use at power-up instead -of the default Sun banner. -.El -.Pp -Note that the -.Ar secure , -.Ar bad_login , -and -.Ar password -fields are not currently supported. -.Pp Since the OpenPROM is designed such that the field names are arbitrary, explaining them here is dubious. -Below are field names and values that -one is likely to see on a system with an OpenPROM. +Below are field names and values that one is likely to see. NOTE: this list may be incomplete or incorrect due to differences
vmm(4): remove locking in vm_intr_pending
This lock/unlock around an atomic operation was causing delays delivering interupts into VMs. Pointed out by claudio@ when he ran md5 - in a VM and it became very sluggish. Debugging help from dlg and mpi, thanks. ok? -ml Index: arch/amd64/amd64/vmm.c === RCS file: /cvs/src/sys/arch/amd64/amd64/vmm.c,v retrieving revision 1.326 diff -u -p -a -u -r1.326 vmm.c --- arch/amd64/amd64/vmm.c 7 Nov 2022 12:29:12 - 1.326 +++ arch/amd64/amd64/vmm.c 8 Nov 2022 15:29:10 - @@ -894,9 +894,7 @@ vm_intr_pending(struct vm_intr_params *v goto out; } - rw_enter_write(>vc_lock); vcpu->vc_intr = vip->vip_intr; - rw_exit_write(>vc_lock); refcnt_rele_wake(>vc_refcnt); out: @@ -3526,7 +3524,7 @@ vcpu_reset_regs_vmx(struct vcpu *vcpu, s vmx_setmsrbrw(vcpu, MSR_FSBASE); vmx_setmsrbrw(vcpu, MSR_GSBASE); vmx_setmsrbrw(vcpu, MSR_KERNELGSBASE); - + vmx_setmsrbr(vcpu, MSR_MISC_ENABLE); vmx_setmsrbr(vcpu, MSR_TSC); Index: arch/amd64/include/vmmvar.h === RCS file: /cvs/src/sys/arch/amd64/include/vmmvar.h,v retrieving revision 1.81 diff -u -p -a -u -r1.81 vmmvar.h --- arch/amd64/include/vmmvar.h 1 Sep 2022 22:01:40 - 1.81 +++ arch/amd64/include/vmmvar.h 8 Nov 2022 15:29:10 - @@ -937,7 +937,7 @@ struct vcpu { struct cpu_info *vc_last_pcpu; /* [v] */ struct vm_exit vc_exit; /* [v] */ - uint16_t vc_intr; /* [v] */ + uint16_t vc_intr; /* [a] */ uint8_t vc_irqready;/* [v] */ uint8_t vc_fpuinited; /* [v] */
push kernel lock inside ifioctl_get()
After this mechanical move, I can unlock the individual SIOCG* in there. OK? Index: if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.667 diff -u -p -r1.667 if.c --- if.c8 Nov 2022 15:20:24 - 1.667 +++ if.c8 Nov 2022 15:26:07 - @@ -2426,33 +2426,43 @@ ifioctl_get(u_long cmd, caddr_t data) size_t bytesdone; const char *label; - KERNEL_LOCK(); - switch(cmd) { case SIOCGIFCONF: + KERNEL_LOCK(); NET_LOCK_SHARED(); error = ifconf(data); NET_UNLOCK_SHARED(); + KERNEL_UNLOCK(); return (error); case SIOCIFGCLONERS: + KERNEL_LOCK(); error = if_clone_list((struct if_clonereq *)data); + KERNEL_UNLOCK(); return (error); case SIOCGIFGMEMB: + KERNEL_LOCK(); NET_LOCK_SHARED(); error = if_getgroupmembers(data); NET_UNLOCK_SHARED(); + KERNEL_UNLOCK(); return (error); case SIOCGIFGATTR: + KERNEL_LOCK(); NET_LOCK_SHARED(); error = if_getgroupattribs(data); NET_UNLOCK_SHARED(); + KERNEL_UNLOCK(); return (error); case SIOCGIFGLIST: + KERNEL_LOCK(); NET_LOCK_SHARED(); error = if_getgrouplist(data); NET_UNLOCK_SHARED(); + KERNEL_UNLOCK(); return (error); } + + KERNEL_LOCK(); ifp = if_unit(ifr->ifr_name); if (ifp == NULL) {
Re: tweak pfsync status output in ifconfig
On Wed, Nov 09, 2022 at 12:09:50AM +1000, David Gwynne wrote: > i'm hacking on pfsync(4) at the moment, and i wasted way too much time > wondering how i broke the pfsync ioctls after i didn't the pfsync_status > output. turns out if you don't have a sync interface set, it skips > output. > > i think it's useful to show that the sync interface is not set, so i > came up with this. > > an unconfigured pfsync interface looks like this with my diff: > > pfsync0: flags=0<> mtu 1500 > index 5 priority 0 llprio 3 > pfsync: syncdev none maxupd 128 defer off > groups: carp pfsync looks useful to me. I certainly don't object. OK sashan
tweak pfsync status output in ifconfig
i'm hacking on pfsync(4) at the moment, and i wasted way too much time wondering how i broke the pfsync ioctls after i didn't the pfsync_status output. turns out if you don't have a sync interface set, it skips output. i think it's useful to show that the sync interface is not set, so i came up with this. an unconfigured pfsync interface looks like this with my diff: pfsync0: flags=0<> mtu 1500 index 5 priority 0 llprio 3 pfsync: syncdev none maxupd 128 defer off groups: carp pfsync Index: ifconfig.c === RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v retrieving revision 1.457 diff -u -p -r1.457 ifconfig.c --- ifconfig.c 26 Oct 2022 17:06:31 - 1.457 +++ ifconfig.c 8 Nov 2022 14:06:01 - @@ -5086,22 +5086,23 @@ setpfsync_defer(const char *val, int d) void pfsync_status(void) { - struct pfsyncreq preq; + struct pfsyncreq pfsyncr; + const char *syncif = "none"; - bzero(, sizeof(struct pfsyncreq)); - ifr.ifr_data = (caddr_t) + bzero(, sizeof(pfsyncr)); + ifr.ifr_data = (caddr_t) if (ioctl(sock, SIOCGETPFSYNC, (caddr_t)) == -1) return; - if (preq.pfsyncr_syncdev[0] != '\0') { - printf("\tpfsync: syncdev: %s ", preq.pfsyncr_syncdev); - if (preq.pfsyncr_syncpeer.s_addr != htonl(INADDR_PFSYNC_GROUP)) - printf("syncpeer: %s ", - inet_ntoa(preq.pfsyncr_syncpeer)); - printf("maxupd: %d ", preq.pfsyncr_maxupdates); - printf("defer: %s\n", preq.pfsyncr_defer ? "on" : "off"); - } + if (pfsyncr.pfsyncr_syncdev[0] != '\0') + syncif = pfsyncr.pfsyncr_syncdev; + + printf("\tpfsync: syncdev %s ", syncif); + if (pfsyncr.pfsyncr_syncpeer.s_addr != htonl(INADDR_PFSYNC_GROUP)) + printf("syncpeer %s ", inet_ntoa(pfsyncr.pfsyncr_syncpeer)); + printf("maxupd %d ", pfsyncr.pfsyncr_maxupdates); + printf("defer %s\n", pfsyncr.pfsyncr_defer ? "on" : "off"); } void
Re: simplify handling of 'once' rules in pf(4)
On Mon, Nov 07, 2022 at 06:53:47PM +0100, Alexandr Nedvedicky wrote: > Hello, > > updated diff. It buys suggestions from Klemens: > > pf.conf(5) section which covers once rules reads as follows: > > onceCreates a one shot rule. The first matching packet marks rule as > expired. The expired rule is never evaluated then. pfctl(8) > does not report expired rules unless run in verbose mode ('-vv'). > In verbose mode pfctl(8) appends '# expired' to note the once > rule which got hit by packet other already. Wording and markup can be improved, but I don't want to block this change with boring churn, so I'm happy to do that later. > > this how pfctl reports expired once rules now: > > pf# pfctl -a test/foo/bar -vv -sr > @0 pass out proto tcp from any to any port = 80 flags S/SA once # > expired > [ Evaluations: 25Packets: 5489 Bytes: 4936817 > States: 0 ] > [ Inserted: uid 0 pid 2768 State Creations: 1 ] > > updated diff is below. OK kn > > thanks and > regards > sashan > > 8<---8<---8<--8< > diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c > index 6f39ad72384..fbe19747fc6 100644 > --- a/sbin/pfctl/pfctl_parser.c > +++ b/sbin/pfctl/pfctl_parser.c > @@ -1148,6 +1148,9 @@ print_rule(struct pf_rule *r, const char *anchor_call, > int opts) > printf(" "); > print_pool(>route, 0, 0, r->af, PF_POOL_ROUTE, verbose); > } > + > + if (r->rule_flag & PFRULE_EXPIRED) > + printf(" # expired"); > } > > void > diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 > index 3e5a17acb95..b98b9f2fd9c 100644 > --- a/share/man/man5/pf.conf.5 > +++ b/share/man/man5/pf.conf.5 > @@ -661,10 +661,14 @@ When the rate is exceeded, all ICMP is blocked until > the rate falls below > 100 per 10 seconds again. > .Pp > .It Cm once > -Creates a one shot rule that will remove itself from an active ruleset after > -the first match. > -In case this is the only rule in the anchor, the anchor will be destroyed > -automatically after the rule is matched. > +Creates a one shot rule. The first matching packet marks rule as expired. > +The expired rule is never evaluated then. > +.Xr pfctl 8 > +does not report expired rules unless run in verbose mode ('-vv'). In verbose > +mode > +.Xr pfctl 8 > +appends '# expired' to note the once rule which got hit by packet other > +already. > .Pp > .It Cm probability Ar number Ns % > A probability attribute can be attached to a rule, > diff --git a/sys/net/pf.c b/sys/net/pf.c > index 2c6124e74f2..6295b4eb9d7 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -313,9 +313,6 @@ RB_GENERATE(pf_state_tree, pf_state_key, entry, > pf_state_compare_key); > RB_GENERATE(pf_state_tree_id, pf_state, > entry_id, pf_state_compare_id); > > -SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl = > - SLIST_HEAD_INITIALIZER(pf_rule_gcl); > - > __inline int > pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af) > { > @@ -1478,23 +1475,6 @@ pf_state_import(const struct pfsync_state *sp, int > flags) > > /* END state table stuff */ > > -void > -pf_purge_expired_rules(void) > -{ > - struct pf_rule *r; > - > - PF_ASSERT_LOCKED(); > - > - if (SLIST_EMPTY(_rule_gcl)) > - return; > - > - while ((r = SLIST_FIRST(_rule_gcl)) != NULL) { > - SLIST_REMOVE(_rule_gcl, r, pf_rule, gcle); > - KASSERT(r->rule_flag & PFRULE_EXPIRED); > - pf_purge_rule(r); > - } > -} > - > void pf_purge_states(void *); > struct task pf_purge_states_task = >TASK_INITIALIZER(pf_purge_states, NULL); > @@ -1588,7 +1568,6 @@ pf_purge(void *null) > PF_LOCK(); > > pf_purge_expired_src_nodes(); > - pf_purge_expired_rules(); > > PF_UNLOCK(); > > @@ -3916,8 +3895,11 @@ pf_match_rule(struct pf_test_ctx *ctx, struct > pf_ruleset *ruleset) > struct pf_rule *save_a; > struct pf_ruleset *save_aruleset; > > +retry: > r = TAILQ_FIRST(ruleset->rules.active.ptr); > while (r != NULL) { > + PF_TEST_ATTRIB(r->rule_flag & PFRULE_EXPIRED, > + TAILQ_NEXT(r, entries)); > r->evaluations++; > PF_TEST_ATTRIB( > (pfi_kif_match(r->kif, ctx->pd->kif) == r->ifnot), > @@ -4042,6 +4024,19 @@ pf_match_rule(struct pf_test_ctx *ctx, struct > pf_ruleset *ruleset) > if (r->tag) > ctx->tag = r->tag; > if (r->anchor == NULL) { > + > + if (r->rule_flag & PFRULE_ONCE) { > + u_int32_t rule_flag; > + > + rule_flag = r->rule_flag; > + if (((rule_flag & PFRULE_EXPIRED) == 0) && > +
push kernel lock into ifioctl_get()
Another mechanical diff without semantic changes to avoid churn in actual unlocking diffs. OK? Index: if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.666 diff -u -p -r1.666 if.c --- if.c8 Nov 2022 11:25:01 - 1.666 +++ if.c8 Nov 2022 13:53:27 - @@ -1979,9 +1979,7 @@ ifioctl(struct socket *so, u_long cmd, c case SIOCGIFRDOMAIN: case SIOCGIFGROUP: case SIOCGIFLLPRIO: - KERNEL_LOCK(); error = ifioctl_get(cmd, data); - KERNEL_UNLOCK(); return (error); } @@ -2428,6 +2426,8 @@ ifioctl_get(u_long cmd, caddr_t data) size_t bytesdone; const char *label; + KERNEL_LOCK(); + switch(cmd) { case SIOCGIFCONF: NET_LOCK_SHARED(); @@ -2455,8 +2455,10 @@ ifioctl_get(u_long cmd, caddr_t data) } ifp = if_unit(ifr->ifr_name); - if (ifp == NULL) + if (ifp == NULL) { + KERNEL_UNLOCK(); return (ENXIO); + } NET_LOCK_SHARED(); @@ -2527,6 +2529,8 @@ ifioctl_get(u_long cmd, caddr_t data) } NET_UNLOCK_SHARED(); + + KERNEL_UNLOCK(); if_put(ifp);
Re: riscv64: switch to clockintr(9)
> Date: Tue, 8 Nov 2022 13:06:51 + > From: Scott Cheloha > > On Mon, Nov 07, 2022 at 09:23:26PM +0100, Jeremie Courreges-Anglas wrote: > > On Sun, Nov 06 2022, Scott Cheloha wrote: > > > This patch switches riscv64 to clockintr(9). > > > > > > jca@ has been testing it (on a SiFive board?). It has survived two > > > parallel release builds and upgrades from the resulting bsd.rd. > > > > I still get the same results on my HiFive Unmatched (produced by > > SiFive indeed). The diff LGTM. > > > > Looks like clock and stat evcounts get merged with this diff. Not a big > > problem but probably not intended? > > Whoops, good eye, forgot to mention that. > > This change is intended. With this patch, we can't keep a distinct > count of hardclock() and statclock() calls anymore because > clockintr_dispatch(9) handles the dispatch on behalf of the riscv64 > code. So the "stat" evcount goes away. > > The "clock" evcount now represents a count of clock interrupts. > That's it. One per riscv64 clock_intr() call. And I think that is fine. Those are the actual interrupts. > > > It could use testing on another machine. Are there other practical > > > machines aside from SiFive? > > > > There aren't that many vendors/machines that we do support: > > > > https://www.openbsd.org/riscv64.html > > Okay, noted. > >
Re: Mark sched_yield(2) as NOLOCK
On Tue, Nov 08, 2022 at 01:14:02PM +, Martin Pieuchot wrote: > Now that mmap/munmap/mprotect(2) are no longer creating contention it is > possible to see that sched_yield(2) is one of the syscalls waiting for > the KERNEL_LOCK() to be released. However this is no longer necessary. > > Traversing `ps_threads' require either the KERNEL_LOCK() or the > SCHED_LOCK() and we are holding both in this case. So let's drop the > requirement for the KERNEL_LOCK(). > > ok? > > Index: kern/syscalls.master > === > RCS file: /cvs/src/sys/kern/syscalls.master,v > retrieving revision 1.235 > diff -u -p -r1.235 syscalls.master > --- kern/syscalls.master 8 Nov 2022 11:05:57 - 1.235 > +++ kern/syscalls.master 8 Nov 2022 13:09:10 - > @@ -531,7 +531,7 @@ > #else > 297 UNIMPL > #endif > -298 STD { int sys_sched_yield(void); } > +298 STD NOLOCK { int sys_sched_yield(void); } > 299 STD NOLOCK { pid_t sys_getthrid(void); } > 300 OBSOL t32___thrsleep > 301 STD NOLOCK { int sys___thrwakeup(const volatile void *ident, \ > Works here. This doesn't move the needle as much as the mmap unlock diff did though :)
Mark sched_yield(2) as NOLOCK
Now that mmap/munmap/mprotect(2) are no longer creating contention it is possible to see that sched_yield(2) is one of the syscalls waiting for the KERNEL_LOCK() to be released. However this is no longer necessary. Traversing `ps_threads' require either the KERNEL_LOCK() or the SCHED_LOCK() and we are holding both in this case. So let's drop the requirement for the KERNEL_LOCK(). ok? Index: kern/syscalls.master === RCS file: /cvs/src/sys/kern/syscalls.master,v retrieving revision 1.235 diff -u -p -r1.235 syscalls.master --- kern/syscalls.master8 Nov 2022 11:05:57 - 1.235 +++ kern/syscalls.master8 Nov 2022 13:09:10 - @@ -531,7 +531,7 @@ #else 297UNIMPL #endif -298STD { int sys_sched_yield(void); } +298STD NOLOCK { int sys_sched_yield(void); } 299STD NOLOCK { pid_t sys_getthrid(void); } 300OBSOL t32___thrsleep 301STD NOLOCK { int sys___thrwakeup(const volatile void *ident, \
Re: riscv64: switch to clockintr(9)
On Mon, Nov 07, 2022 at 09:23:26PM +0100, Jeremie Courreges-Anglas wrote: > On Sun, Nov 06 2022, Scott Cheloha wrote: > > This patch switches riscv64 to clockintr(9). > > > > jca@ has been testing it (on a SiFive board?). It has survived two > > parallel release builds and upgrades from the resulting bsd.rd. > > I still get the same results on my HiFive Unmatched (produced by > SiFive indeed). The diff LGTM. > > Looks like clock and stat evcounts get merged with this diff. Not a big > problem but probably not intended? Whoops, good eye, forgot to mention that. This change is intended. With this patch, we can't keep a distinct count of hardclock() and statclock() calls anymore because clockintr_dispatch(9) handles the dispatch on behalf of the riscv64 code. So the "stat" evcount goes away. The "clock" evcount now represents a count of clock interrupts. That's it. One per riscv64 clock_intr() call. > > It could use testing on another machine. Are there other practical > > machines aside from SiFive? > > There aren't that many vendors/machines that we do support: > > https://www.openbsd.org/riscv64.html Okay, noted.
Re: tc_setclock: don't print warning when tc_windup() rejects RTC time
> Date: Tue, 8 Nov 2022 11:59:17 + > From: Scott Cheloha > > On some arm64 machines, the agtimer(4) ticks slowly enough that the > tc_delta() doesn't overflow across brief suspends. While working on > arm64 suspend/resume, kettenis@ has been seeing warnings like this > during resume: > > tc_setclock: cannot rewind uptime to 307.253324249 > > The warning is misleading and should be removed. The code is behaving > as intended, but in a way I didn't anticipate when I added the warning > a few years ago. > > It might be useful print a warning in inittodr(9) during resume if the > RTC time predates the system UTC suspend timestamp, but that's a > distinct concern. > > ok? ok kettenis@ > Index: kern_tc.c > === > RCS file: /cvs/src/sys/kern/kern_tc.c,v > retrieving revision 1.78 > diff -u -p -r1.78 kern_tc.c > --- kern_tc.c 18 Sep 2022 20:47:09 - 1.78 > +++ kern_tc.c 8 Nov 2022 11:53:01 - > @@ -552,7 +552,6 @@ void > tc_setclock(const struct timespec *ts) > { > struct bintime new_naptime, old_naptime, uptime, utc; > - struct timespec tmp; > static int first = 1; > #ifndef SMALL_KERNEL > struct bintime elapsed; > @@ -582,12 +581,6 @@ tc_setclock(const struct timespec *ts) > new_naptime = timehands->th_naptime; > > mtx_leave(_mtx); > - > - if (bintimecmp(_naptime, _naptime, ==)) { > - BINTIME_TO_TIMESPEC(, ); > - printf("%s: cannot rewind uptime to %lld.%09ld\n", > - __func__, (long long)tmp.tv_sec, tmp.tv_nsec); > - } > > #ifndef SMALL_KERNEL > /* convert the bintime to ticks */ > >
Re: libcrypto potential memory leak in OBJ_NAME_add error path
On Tue, Nov 08, 2022 at 01:33:47PM +0100, Moritz Buhl wrote: > In case lh_OBJ_NAME_insert returns NULL due to a failed malloc, onp > is leaked in OBJ_NAME_add. > > Found by CodeChecker. > > OK? > mbuhl > > Index: lib/libcrypto/objects/o_names.c > === > RCS file: /cvs/src/lib/libcrypto/objects/o_names.c,v > retrieving revision 1.22 > diff -u -p -r1.22 o_names.c > --- lib/libcrypto/objects/o_names.c 29 Jan 2017 17:49:23 - 1.22 > +++ lib/libcrypto/objects/o_names.c 8 Nov 2022 12:24:47 - > @@ -196,6 +196,7 @@ OBJ_NAME_add(const char *name, int type, > } > free(ret); > } else { > + free(onp); > if (lh_OBJ_NAME_error(names_lh)) { Actually, it needs freeing only here. If lh_OBJ_NAME_error() returns 0, there was no error and a hash-entry containing NULL was replaced. > /* ERROR */ > return (0);
libcrypto potential memory leak in OBJ_NAME_add error path
In case lh_OBJ_NAME_insert returns NULL due to a failed malloc, onp is leaked in OBJ_NAME_add. Found by CodeChecker. OK? mbuhl Index: lib/libcrypto/objects/o_names.c === RCS file: /cvs/src/lib/libcrypto/objects/o_names.c,v retrieving revision 1.22 diff -u -p -r1.22 o_names.c --- lib/libcrypto/objects/o_names.c 29 Jan 2017 17:49:23 - 1.22 +++ lib/libcrypto/objects/o_names.c 8 Nov 2022 12:24:47 - @@ -196,6 +196,7 @@ OBJ_NAME_add(const char *name, int type, } free(ret); } else { + free(onp); if (lh_OBJ_NAME_error(names_lh)) { /* ERROR */ return (0);
Re: libcrypto: leak in RSA_print()
On Tue, Nov 08, 2022 at 01:01:17PM +0100, Tobias Heider wrote: > If EVP_PKEY_set1_RSA() returns 0 we seem leak pk here. > > ok? > > Index: rsa/rsa_prn.c > === > RCS file: /cvs/src/lib/libcrypto/rsa/rsa_prn.c,v > retrieving revision 1.7 > diff -u -p -r1.7 rsa_prn.c > --- rsa/rsa_prn.c 29 Jan 2017 17:49:23 - 1.7 > +++ rsa/rsa_prn.c 8 Nov 2022 11:59:28 - > @@ -85,8 +85,10 @@ RSA_print(BIO *bp, const RSA *x, int off > int ret; > > pk = EVP_PKEY_new(); > - if (!pk || !EVP_PKEY_set1_RSA(pk, (RSA *)x)) > + if (!pk || !EVP_PKEY_set1_RSA(pk, (RSA *)x)) { > + EVP_PKEY_free(pk); > return 0; > + } I'd rewrite this as: if ((pk = EVP_PKEY_new()) == NULL) goto out; if (!EVP_PKEY_set1_RSA(pk, (RSA *)x)) goto out; > ret = EVP_PKEY_print_private(bp, pk, off, NULL); out: > EVP_PKEY_free(pk); > return ret; >
Re: tc_setclock: don't print warning when tc_windup() rejects RTC time
On Tue, Nov 08, 2022 at 11:59:17AM +, Scott Cheloha wrote: > On some arm64 machines, the agtimer(4) ticks slowly enough that the > tc_delta() doesn't overflow across brief suspends. While working on > arm64 suspend/resume, kettenis@ has been seeing warnings like this > during resume: > > tc_setclock: cannot rewind uptime to 307.253324249 > > The warning is misleading and should be removed. The code is behaving > as intended, but in a way I didn't anticipate when I added the warning > a few years ago. > > It might be useful print a warning in inittodr(9) during resume if the > RTC time predates the system UTC suspend timestamp, but that's a > distinct concern. > > ok? > > Index: kern_tc.c > === > RCS file: /cvs/src/sys/kern/kern_tc.c,v > retrieving revision 1.78 > diff -u -p -r1.78 kern_tc.c > --- kern_tc.c 18 Sep 2022 20:47:09 - 1.78 > +++ kern_tc.c 8 Nov 2022 11:53:01 - > @@ -552,7 +552,6 @@ void > tc_setclock(const struct timespec *ts) > { > struct bintime new_naptime, old_naptime, uptime, utc; > - struct timespec tmp; > static int first = 1; > #ifndef SMALL_KERNEL > struct bintime elapsed; > @@ -582,12 +581,6 @@ tc_setclock(const struct timespec *ts) > new_naptime = timehands->th_naptime; > > mtx_leave(_mtx); > - > - if (bintimecmp(_naptime, _naptime, ==)) { > - BINTIME_TO_TIMESPEC(, ); > - printf("%s: cannot rewind uptime to %lld.%09ld\n", > - __func__, (long long)tmp.tv_sec, tmp.tv_nsec); > - } > > #ifndef SMALL_KERNEL > /* convert the bintime to ticks */ > ok mlarkin
libcrypto: leak in RSA_print()
If EVP_PKEY_set1_RSA() returns 0 we seem leak pk here. ok? Index: rsa/rsa_prn.c === RCS file: /cvs/src/lib/libcrypto/rsa/rsa_prn.c,v retrieving revision 1.7 diff -u -p -r1.7 rsa_prn.c --- rsa/rsa_prn.c 29 Jan 2017 17:49:23 - 1.7 +++ rsa/rsa_prn.c 8 Nov 2022 11:59:28 - @@ -85,8 +85,10 @@ RSA_print(BIO *bp, const RSA *x, int off int ret; pk = EVP_PKEY_new(); - if (!pk || !EVP_PKEY_set1_RSA(pk, (RSA *)x)) + if (!pk || !EVP_PKEY_set1_RSA(pk, (RSA *)x)) { + EVP_PKEY_free(pk); return 0; + } ret = EVP_PKEY_print_private(bp, pk, off, NULL); EVP_PKEY_free(pk); return ret;
tc_setclock: don't print warning when tc_windup() rejects RTC time
On some arm64 machines, the agtimer(4) ticks slowly enough that the tc_delta() doesn't overflow across brief suspends. While working on arm64 suspend/resume, kettenis@ has been seeing warnings like this during resume: tc_setclock: cannot rewind uptime to 307.253324249 The warning is misleading and should be removed. The code is behaving as intended, but in a way I didn't anticipate when I added the warning a few years ago. It might be useful print a warning in inittodr(9) during resume if the RTC time predates the system UTC suspend timestamp, but that's a distinct concern. ok? Index: kern_tc.c === RCS file: /cvs/src/sys/kern/kern_tc.c,v retrieving revision 1.78 diff -u -p -r1.78 kern_tc.c --- kern_tc.c 18 Sep 2022 20:47:09 - 1.78 +++ kern_tc.c 8 Nov 2022 11:53:01 - @@ -552,7 +552,6 @@ void tc_setclock(const struct timespec *ts) { struct bintime new_naptime, old_naptime, uptime, utc; - struct timespec tmp; static int first = 1; #ifndef SMALL_KERNEL struct bintime elapsed; @@ -582,12 +581,6 @@ tc_setclock(const struct timespec *ts) new_naptime = timehands->th_naptime; mtx_leave(_mtx); - - if (bintimecmp(_naptime, _naptime, ==)) { - BINTIME_TO_TIMESPEC(, ); - printf("%s: cannot rewind uptime to %lld.%09ld\n", - __func__, (long long)tmp.tv_sec, tmp.tv_nsec); - } #ifndef SMALL_KERNEL /* convert the bintime to ticks */
Re: xenstore.c: return error number
On 01/11/22(Tue) 15:26, Masato Asou wrote: > Hi, > > Return error number instead of call panic(). Makes sense to me. Do you know how this error can occur? Is is a logic error or are we trusting values produced by a third party? > comment, ok? > -- > ASOU Masato > > diff --git a/sys/dev/pv/xenstore.c b/sys/dev/pv/xenstore.c > index 1e4f15d30eb..dc89ba0fa6d 100644 > --- a/sys/dev/pv/xenstore.c > +++ b/sys/dev/pv/xenstore.c > @@ -118,6 +118,7 @@ struct xs_msg { > struct xs_msghdr xsm_hdr; > uint32_t xsm_read; > uint32_t xsm_dlen; > + int xsm_error; > uint8_t *xsm_data; > TAILQ_ENTRY(xs_msg) xsm_link; > }; > @@ -566,9 +567,7 @@ xs_intr(void *arg) > } > > if (xsm->xsm_hdr.xmh_len > xsm->xsm_dlen) > - panic("message too large: %d vs %d for type %d, rid %u", > - xsm->xsm_hdr.xmh_len, xsm->xsm_dlen, xsm->xsm_hdr.xmh_type, > - xsm->xsm_hdr.xmh_rid); > + xsm->xsm_error = EMSGSIZE; > > len = MIN(xsm->xsm_hdr.xmh_len - xsm->xsm_read, avail); > if (len) { > @@ -800,7 +799,9 @@ xs_cmd(struct xs_transaction *xst, int cmd, const char > *path, > error = xs_geterror(xsm); > DPRINTF("%s: xenstore request %d \"%s\" error %s\n", > xs->xs_sc->sc_dev.dv_xname, cmd, path, xsm->xsm_data); > - } else if (mode == READ) { > + } else if (xsm->xsm_error != 0) > + error = xsm->xsm_error; > + else if (mode == READ) { > KASSERT(iov && iov_cnt); > error = xs_parse(xst, xsm, iov, iov_cnt); > } >
reorder_kernel: suport ro /usr like library_aslr does
More read-only filesystems mean less fsck during boot after crashes. Especially on crappy machines like the Pinebook Poop, I keep /usr read-only and run with this diff so I still get a relinked kernel. rc's reorder_libs() already does the same remount dance, but for multiple directories/filesystems. My diff works as expected with read-write and read-only /usr as well as interrupting /usr/libexec/reorder_kernel runs with ^C, i.e. a failed run will correctly mount /usr ro again if it was ro before the run. Feedback? OK? Index: reorder_kernel.sh === RCS file: /cvs/src/libexec/reorder_kernel/reorder_kernel.sh,v retrieving revision 1.13 diff -u -p -r1.13 reorder_kernel.sh --- reorder_kernel.sh 7 Nov 2022 15:55:56 - 1.13 +++ reorder_kernel.sh 8 Nov 2022 11:02:42 - @@ -27,13 +27,32 @@ LOGFILE=$KERNEL_DIR/$KERNEL/relink.log PROGNAME=${0##*/} SHA256=/var/db/kernel.SHA256 -# Silently skip if on a NFS mounted filesystem. -df -t nonfs $KERNEL_DIR >/dev/null 2>&1 +# Silently skip if on NFS, otherwise remount the filesystem read-write. +DEV=$(df -t nonfs $KERNEL_DIR 2>/dev/null | awk 'NR == 2 { print $1 }') +MP=$(mount -t ffs | grep ^$DEV) +RO=false +if [[ $MP == *read-only* ]]; then + MP=${MP%% *} + mount -u -w $MP + RO=true +fi + +restore_mount() { + if $RO; then + # Close the logfile to unbusy $MP by switching back to console. + exec 1>/dev/console + exec 2>&1 + mount -u -r $MP + fi +} # Install trap handlers to inform about success or failure via syslog. ERRMSG='failed' -trap 'trap - EXIT; logger -st $PROGNAME "$ERRMSG" >/dev/console 2>&1' ERR -trap 'logger -t $PROGNAME "kernel relinking done"' EXIT +trap 'trap - EXIT + logger -st $PROGNAME "$ERRMSG" 2>/dev/console + restore_mount' ERR +trap 'logger -t $PROGNAME "kernel relinking done" + restore_mount' EXIT # Create kernel compile dir and redirect stdout/stderr to a logfile. mkdir -m 700 -p $KERNEL_DIR/$KERNEL
Re: Please test: unlock mprotect/mmap/munmap
I concur, my 8-cores laptop (amd64) is happy with this patch. Mostly using ungoogled-chromium, mutt, ncspot, xterm along with a few vmd guests as dev machines. No isuses to report. Jesper Wallin On Tue, Nov 08, 2022 at 09:55:36AM +, Stefan Hagen wrote: > Martin Pieuchot wrote (2022-11-06 10:54 WET): > > These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This > > will reduce contention a lot. I'd be happy to hear from test reports > > on many architectures and possible workloads. > > My two amd64 machines are happy with it for two days now. With several > kernel builds and regular desktop use (browsers and such). > > Best Regards, > Stefan >
Re: Please test: unlock mprotect/mmap/munmap
I have now been running it for two days, I *thought * had one hang a day ago, with chrome and local building churning away with me mashing on the editor.. but I’ve now been doing the same thing with witness on for a day and had no issues. So I think whatever I might have seen is not reproducible or unrelated.. Ok beck@ > On Nov 8, 2022, at 10:30 AM, Martin Pieuchot wrote: > > On 08/11/22(Tue) 11:12, Mark Kettenis wrote: >>> Date: Tue, 8 Nov 2022 10:32:14 +0100 >>> From: Christian Weisgerber >>> >>> Martin Pieuchot: >>> These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This will reduce contention a lot. I'd be happy to hear from test reports on many architectures and possible workloads. >>> >>> This survived a full amd64 package build. >> >> \8/ >> >> I think that means it should be comitted. > > I agree. This has been tested on i386, riscv64, m88k, arm64, amd64 (of > course) and sparc64. I'm pretty confident. >
Re: mbufs growing in 7.2
Hi Greg, Hi Joe, dlg@ hinted to me that the ring might overwrite it's own starting position with the current code. Does this help? mbuhl Index: dev/pci/if_igc.c === RCS file: /cvs/src/sys/dev/pci/if_igc.c,v retrieving revision 1.9 diff -u -p -r1.9 if_igc.c --- dev/pci/if_igc.c2 Jun 2022 07:41:17 - 1.9 +++ dev/pci/if_igc.c8 Nov 2022 10:35:39 - @@ -978,7 +978,7 @@ igc_start(struct ifqueue *ifq) mask = sc->num_tx_desc - 1; for (;;) { - if (free <= IGC_MAX_SCATTER) { + if (free <= IGC_MAX_SCATTER + 1) { ifq_set_oactive(ifq); break; } @@ -1005,6 +1005,7 @@ igc_start(struct ifqueue *ifq) /* Consume the first descriptor */ prod++; prod &= mask; + free--; } for (i = 0; i < map->dm_nsegs; i++) {
Re: rc(8): reorder_libs(): print names of relinked libraries
On Tue, Nov 08, 2022 at 10:23:23AM +, Stuart Henderson wrote: > On 2022/11/07 23:54, Theo de Raadt wrote: > > Klemens Nanni wrote: > > > > > > I know this makes rc(8) a bit noisier but it really does improve my > > > > (for want of a better term) "user experience" as I wait for my machine > > > > to boot. > > > > > > I like this and it doesn't add more **lines** to the boot log, but maybe > > > print library names without versions to reduces noise? > > > > Only if it is the short names. > > No need for the .so really either. ld.so libc libcrypto is enough. > > > But I am not sure people need to see this detail. It just takes a bit > > of time. How does knowing what steps are being taken help... > > Sometimes it's a bit of time, sometimes it's a _lot_ of time until > people get a new computer or raid battery or something and it's less > annoying if you can see _some_ progress. Here's a simple diff that prints "ld.so" and "libc". Not trying to take over Scott's diff, just helping out on shell bits. Index: rc === RCS file: /cvs/src/etc/rc,v retrieving revision 1.564 diff -u -p -r1.564 rc --- rc 29 Aug 2022 11:51:05 - 1.564 +++ rc 8 Nov 2022 10:32:32 - @@ -193,7 +193,7 @@ reorder_libs() { # Remount the (read-only) filesystems in _ro_list as read-write. for _mp in $_ro_list; do if ! mount -u -w $_mp; then - echo ' failed.' + echo '(failed).' return fi done @@ -215,6 +215,7 @@ reorder_libs() { cd $_tmpdir ar x $_liba if [[ $_lib == ld.so ]]; then + echo -n " $_lib" args="-g -x -e _dl_start \ --version-script=Symbols.map --shared -Bsymbolic \ --no-undefined" @@ -225,6 +226,7 @@ reorder_libs() { $_install /usr/libexec/ld.so /usr/libexec/ld.so.save $_install ld.so.test $_lib_dir/ld.so else + echo -n " ${_lib%%.*}" cc -shared -o $_lib $(ls *.so | sort -R) $(<.ldadd) [[ -s $_lib ]] && file $_lib | fgrep -q 'shared object' LD_BIND_NOW=1 LD_LIBRARY_PATH=$_tmpdir awk 'BEGIN {exit 0}' @@ -243,9 +245,9 @@ reorder_libs() { done if $_error; then - echo ' failed.' + echo '(failed).' else - echo ' done.' + echo '.' fi }
Re: Please test: unlock mprotect/mmap/munmap
On Tue, Nov 08, 2022 at 10:30:47AM +, Martin Pieuchot wrote: > On 08/11/22(Tue) 11:12, Mark Kettenis wrote: > > I think that means it should be comitted. > > I agree. This has been tested on i386, riscv64, m88k, arm64, amd64 (of > course) and sparc64. I'm pretty confident. OK kn
Re: rc(8): reorder_libs(): print names of relinked libraries
Stuart Henderson wrote: > > But I am not sure people need to see this detail. It just takes a bit > > of time. How does knowing what steps are being taken help... > > Sometimes it's a bit of time, sometimes it's a _lot_ of time until > people get a new computer or raid battery or something and it's less > annoying if you can see _some_ progress. Sigh. Do we need a spinny thingy?
Re: rc(8): reorder_libs(): print names of relinked libraries
Klemens Nanni wrote: > On Tue, Nov 08, 2022 at 10:23:23AM +, Stuart Henderson wrote: > > On 2022/11/07 23:54, Theo de Raadt wrote: > > > Klemens Nanni wrote: > > > > > > > > I know this makes rc(8) a bit noisier but it really does improve my > > > > > (for want of a better term) "user experience" as I wait for my machine > > > > > to boot. > > > > > > > > I like this and it doesn't add more **lines** to the boot log, but maybe > > > > print library names without versions to reduces noise? > > > > > > Only if it is the short names. > > > > No need for the .so really either. ld.so libc libcrypto is enough. > > Yes, but that needs a little more shell tweaking as ${_lib%%.*} will > turn "ld.so" into "ld", so I suggested a simple middle ground before > blowing up the diff... Sorry, but I insist on the shorters cute names.
Re: Please test: unlock mprotect/mmap/munmap
On 08/11/22(Tue) 11:12, Mark Kettenis wrote: > > Date: Tue, 8 Nov 2022 10:32:14 +0100 > > From: Christian Weisgerber > > > > Martin Pieuchot: > > > > > These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This > > > will reduce contention a lot. I'd be happy to hear from test reports > > > on many architectures and possible workloads. > > > > This survived a full amd64 package build. > > \8/ > > I think that means it should be comitted. I agree. This has been tested on i386, riscv64, m88k, arm64, amd64 (of course) and sparc64. I'm pretty confident.
Re: rc(8): reorder_libs(): print names of relinked libraries
On Tue, Nov 08, 2022 at 10:23:23AM +, Stuart Henderson wrote: > On 2022/11/07 23:54, Theo de Raadt wrote: > > Klemens Nanni wrote: > > > > > > I know this makes rc(8) a bit noisier but it really does improve my > > > > (for want of a better term) "user experience" as I wait for my machine > > > > to boot. > > > > > > I like this and it doesn't add more **lines** to the boot log, but maybe > > > print library names without versions to reduces noise? > > > > Only if it is the short names. > > No need for the .so really either. ld.so libc libcrypto is enough. Yes, but that needs a little more shell tweaking as ${_lib%%.*} will turn "ld.so" into "ld", so I suggested a simple middle ground before blowing up the diff... > > > But I am not sure people need to see this detail. It just takes a bit > > of time. How does knowing what steps are being taken help... > > Sometimes it's a bit of time, sometimes it's a _lot_ of time until > people get a new computer or raid battery or something and it's less > annoying if you can see _some_ progress. That and it just goes well in line with how rc.d scripts are printed.
Re: rc(8): reorder_libs(): print names of relinked libraries
On 2022/11/07 23:54, Theo de Raadt wrote: > Klemens Nanni wrote: > > > > I know this makes rc(8) a bit noisier but it really does improve my > > > (for want of a better term) "user experience" as I wait for my machine > > > to boot. > > > > I like this and it doesn't add more **lines** to the boot log, but maybe > > print library names without versions to reduces noise? > > Only if it is the short names. No need for the .so really either. ld.so libc libcrypto is enough. > But I am not sure people need to see this detail. It just takes a bit > of time. How does knowing what steps are being taken help... Sometimes it's a bit of time, sometimes it's a _lot_ of time until people get a new computer or raid battery or something and it's less annoying if you can see _some_ progress.
Re: Please test: unlock mprotect/mmap/munmap
> Date: Tue, 8 Nov 2022 10:32:14 +0100 > From: Christian Weisgerber > > Martin Pieuchot: > > > These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This > > will reduce contention a lot. I'd be happy to hear from test reports > > on many architectures and possible workloads. > > This survived a full amd64 package build. \8/ I think that means it should be comitted.
Re: Please test: unlock mprotect/mmap/munmap
Martin Pieuchot wrote (2022-11-06 10:54 WET): > These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This > will reduce contention a lot. I'd be happy to hear from test reports > on many architectures and possible workloads. My two amd64 machines are happy with it for two days now. With several kernel builds and regular desktop use (browsers and such). Best Regards, Stefan
Re: Please test: unlock mprotect/mmap/munmap
Martin Pieuchot: > These 3 syscalls should now be ready to run w/o KERNEL_LOCK(). This > will reduce contention a lot. I'd be happy to hear from test reports > on many architectures and possible workloads. This survived a full amd64 package build. -- Christian "naddy" Weisgerber na...@mips.inka.de