Fix gdb "target kvm" on amd64
Hi,
Currently gdb "target kvm bsd.0.core" on amd64 doesn't work. It can't
read the registers and can't follow the stack frames. The diff
follows fixes those problems.
ok? comment?
Fix gdb can handle prologues which has the retguard. Also teach gdb
that alltraps_kern() is a trap function. Original diff from IIJ.
Index: gnu/usr.bin/binutils/gdb/amd64-tdep.c
===
RCS file: /cvs/src/gnu/usr.bin/binutils/gdb/amd64-tdep.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 amd64-tdep.c
--- gnu/usr.bin/binutils/gdb/amd64-tdep.c 27 Dec 2004 13:05:33 -
1.1.1.2
+++ gnu/usr.bin/binutils/gdb/amd64-tdep.c 22 Jan 2019 05:28:15 -
@@ -741,13 +741,31 @@ amd64_analyze_prologue (CORE_ADDR pc, CO
struct amd64_frame_cache *cache)
{
static unsigned char proto[3] = { 0x48, 0x89, 0xe5 };
- unsigned char buf[3];
+ unsigned char buf[10];
unsigned char op;
+ CORE_ADDR pc0 = pc;
if (current_pc <= pc)
return current_pc;
- op = read_memory_unsigned_integer (pc, 1);
+ op = read_memory_unsigned_integer (pc0, 1);
+
+ /* Check for retguard */
+ if ((op == 0x4c || op == 0x48) && (current_pc <= pc + 11))
+{
+ read_memory (pc0 + 1, buf, 10);
+ pc0 += 11;
+
+ /* Check for `movq (__retguard_ ## x)(%rip), %reg;'. */
+ if (buf[0] != 0x8b)
+ return pc;
+
+ /* Check for `xorq off(%rsp), %reg'. */
+ if ((buf[6] != 0x4c && buf[6] != 0x48)
+ || buf[7] != 0x33 || buf[9] != 0x24)
+ return pc;
+ op = read_memory_unsigned_integer (pc0, 1);
+}
if (op == 0x55) /* pushq %rbp */
{
@@ -757,17 +775,17 @@ amd64_analyze_prologue (CORE_ADDR pc, CO
cache->sp_offset += 8;
/* If that's all, return now. */
- if (current_pc <= pc + 1)
+ if (current_pc <= pc0 + 1)
return current_pc;
/* Check for `movq %rsp, %rbp'. */
- read_memory (pc + 1, buf, 3);
+ read_memory (pc0 + 1, buf, 3);
if (memcmp (buf, proto, 3) != 0)
- return pc + 1;
+ return pc0 + 1;
/* OK, we actually have a frame. */
cache->frameless_p = 0;
- return pc + 4;
+ return pc0 + 4;
}
return pc;
Index: gnu/usr.bin/binutils/gdb/amd64obsd-tdep.c
===
RCS file: /cvs/src/gnu/usr.bin/binutils/gdb/amd64obsd-tdep.c,v
retrieving revision 1.11
diff -u -p -r1.11 amd64obsd-tdep.c
--- gnu/usr.bin/binutils/gdb/amd64obsd-tdep.c 10 Jul 2018 08:57:44 -
1.11
+++ gnu/usr.bin/binutils/gdb/amd64obsd-tdep.c 22 Jan 2019 05:28:15 -
@@ -452,6 +452,7 @@ amd64obsd_trapframe_sniffer (const struc
return (name && ((strcmp (name, "calltrap") == 0)
|| (name[0] == 'X' && strncmp(name, "Xipi_", 5) != 0)
|| (strcmp (name, "alltraps") == 0)
+ || (strcmp (name, "alltraps_kern") == 0)
|| (strcmp (name, "intr_fast_exit") == 0)
|| (strcmp (name, "intr_exit_recurse") == 0)));
}
Re: replacing timeout_add() with timeout_add_msec()
> The intent was to wait a 30th of a second. In practice it has always > fired 30ms hence. It's a magic number, so I'd just call it 30ms. > miod might have an opinion on whether those extra milliseconds would > be useful in the event that sparc64 is ever able to timeout with > millisecond or better precision on OpenBSD. That's not sparc64-specific. There is the same 1/30th logic in sys/dev/isa/fd.c which is used on i386 and amd64, you know.
Re: sensors hiding with pledge
This approach seems backwards.
It is hiding sensors from programs which are pledged (ie. we put effort into
security, therefore a fig leaf for privacy)
But.. in programs we cannot pledge, we continue exporting.
Yes chrome is pledged so permanently has no access to the information.
I am not loving this.
> We recently had a thread about adding more sensors, but then the browser will
> use them to spy on us, and everybody was sad. We allow hw.sensors even for
> pledge processes because ntpd needs to read the time. However, ntpd only needs
> to read the time.
>
> This diff zeroes out sensors other than timedeltas. Maybe some others can be
> added as needed, but that seemed a good place to start. I didn't want to
> change the code too much (i.e. hide the existence of sensors entirely) so it
> just changes them all to 0 valued plain integer sensors.
>
> Thoughts?
>
> Index: kern_sysctl.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.353
> diff -u -p -r1.353 kern_sysctl.c
> --- kern_sysctl.c 19 Jan 2019 01:53:44 - 1.353
> +++ kern_sysctl.c 22 Jan 2019 02:01:30 -
> @@ -137,7 +137,7 @@ int sysctl_proc_nobroadcastkill(int *, u
> struct proc *);
> int sysctl_proc_vmmap(int *, u_int, void *, size_t *, struct proc *);
> int sysctl_intrcnt(int *, u_int, void *, size_t *);
> -int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t);
> +int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t, struct
> proc *);
> int sysctl_cptime2(int *, u_int, void *, size_t *, void *, size_t);
> #if NAUDIO > 0
> int sysctl_audio(int *, u_int, void *, size_t *, void *, size_t);
> @@ -735,7 +735,7 @@ hw_sysctl(int *name, u_int namelen, void
> #ifndef SMALL_KERNEL
> case HW_SENSORS:
> return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp,
> - newp, newlen));
> + newp, newlen, p));
> case HW_SETPERF:
> return (sysctl_hwsetperf(oldp, oldlenp, newp, newlen));
> case HW_PERFPOLICY:
> @@ -2302,7 +2302,7 @@ sysctl_intrcnt(int *name, u_int namelen,
>
> int
> sysctl_sensors(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> -void *newp, size_t newlen)
> +void *newp, size_t newlen, struct proc *p)
> {
> struct ksensor *ks;
> struct sensor *us;
> @@ -2350,6 +2350,22 @@ sysctl_sensors(int *name, u_int namelen,
> us->status = ks->status;
> us->numt = ks->numt;
> us->flags = ks->flags;
> +
> + /* not all sensors exposed to pledged processes */
> + if (p->p_p->ps_flags & PS_PLEDGE) {
> + switch (us->type) {
> + case SENSOR_TIMEDELTA:
> + break;
> + default:
> + memset(us->desc, 0, sizeof(us->desc));
> + memset(&us->tv, 0, sizeof(us->tv));
> + us->value = 0;
> + us->type = SENSOR_INTEGER;
> + us->status = SENSOR_S_UNKNOWN;
> + us->flags = SENSOR_FUNKNOWN;
> + break;
> + }
> + }
>
> ret = sysctl_rdstruct(oldp, oldlenp, newp, us,
> sizeof(struct sensor));
>
Re: sensors hiding with pledge
Wouldn't this break sensorsd? (It's already been converted to use pledge.)
C.
On Mon, 21 Jan 2019 at 20:19, Ted Unangst wrote:
>
> We recently had a thread about adding more sensors, but then the browser will
> use them to spy on us, and everybody was sad. We allow hw.sensors even for
> pledge processes because ntpd needs to read the time. However, ntpd only needs
> to read the time.
>
> This diff zeroes out sensors other than timedeltas. Maybe some others can be
> added as needed, but that seemed a good place to start. I didn't want to
> change the code too much (i.e. hide the existence of sensors entirely) so it
> just changes them all to 0 valued plain integer sensors.
>
> Thoughts?
>
> Index: kern_sysctl.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.353
> diff -u -p -r1.353 kern_sysctl.c
> --- kern_sysctl.c 19 Jan 2019 01:53:44 - 1.353
> +++ kern_sysctl.c 22 Jan 2019 02:01:30 -
> @@ -137,7 +137,7 @@ int sysctl_proc_nobroadcastkill(int *, u
> struct proc *);
> int sysctl_proc_vmmap(int *, u_int, void *, size_t *, struct proc *);
> int sysctl_intrcnt(int *, u_int, void *, size_t *);
> -int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t);
> +int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t, struct
> proc *);
> int sysctl_cptime2(int *, u_int, void *, size_t *, void *, size_t);
> #if NAUDIO > 0
> int sysctl_audio(int *, u_int, void *, size_t *, void *, size_t);
> @@ -735,7 +735,7 @@ hw_sysctl(int *name, u_int namelen, void
> #ifndefSMALL_KERNEL
> case HW_SENSORS:
> return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp,
> - newp, newlen));
> + newp, newlen, p));
> case HW_SETPERF:
> return (sysctl_hwsetperf(oldp, oldlenp, newp, newlen));
> case HW_PERFPOLICY:
> @@ -2302,7 +2302,7 @@ sysctl_intrcnt(int *name, u_int namelen,
>
> int
> sysctl_sensors(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> -void *newp, size_t newlen)
> +void *newp, size_t newlen, struct proc *p)
> {
> struct ksensor *ks;
> struct sensor *us;
> @@ -2350,6 +2350,22 @@ sysctl_sensors(int *name, u_int namelen,
> us->status = ks->status;
> us->numt = ks->numt;
> us->flags = ks->flags;
> +
> + /* not all sensors exposed to pledged processes */
> + if (p->p_p->ps_flags & PS_PLEDGE) {
> + switch (us->type) {
> + case SENSOR_TIMEDELTA:
> + break;
> + default:
> + memset(us->desc, 0, sizeof(us->desc));
> + memset(&us->tv, 0, sizeof(us->tv));
> + us->value = 0;
> + us->type = SENSOR_INTEGER;
> + us->status = SENSOR_S_UNKNOWN;
> + us->flags = SENSOR_FUNKNOWN;
> + break;
> + }
> + }
>
> ret = sysctl_rdstruct(oldp, oldlenp, newp, us,
> sizeof(struct sensor));
>
Re: replacing timeout_add() with timeout_add_msec()
On Mon, Jan 21, 2019 at 09:05:19PM -0500, Ted Unangst wrote:
> Scott Cheloha wrote:
> > > > diff --git sys/arch/sparc64/dev/fd.c sys/arch/sparc64/dev/fd.c
> > > > index 8d548062f83..654d8c95524 100644
> > > > --- sys/arch/sparc64/dev/fd.c
> > > > +++ sys/arch/sparc64/dev/fd.c
> > > > @@ -1632,7 +1632,7 @@ loop:
> > > > fdc->sc_state = RECALCOMPLETE;
> > > > if (fdc->sc_flags & FDC_NEEDHEADSETTLE) {
> > > > /* allow 1/30 second for heads to settle */
> > > > - timeout_add(&fdc->fdcpseudointr_to, hz / 30);
> > > > + timeout_add_msec(&fdc->fdcpseudointr_to, 33);
> > > > return (1); /* will return later */
> > > > }
> > > >
> > >
> > > Wonder if this should be 30 or 40 since 33 is rather odd.
> >
> > The intent was to wait a 30th of a second. In practice it has always
> > fired 30ms hence. It's a magic number, so I'd just call it 30ms.
> > miod might have an opinion on whether those extra milliseconds would
> > be useful in the event that sparc64 is ever able to timeout with
> > millisecond or better precision on OpenBSD.
>
> make it 30 and the let the next sparc64 user with a floppy drive worry about
> the fallout. :)
I don't think there will be any fallout. sparc64 has HZ = 100:
hz / 30 = 100 / 30 = 3 ticks
so it currently goes off sometime between 20 and 30 milliseconds
from the call to timeout_add(9).
Now we're doing timeout_add_msec(9) with 30 milliseconds, which
becomes
(30 * 1000) / (100 / HZ) = 3 / 1 = 3 ticks
so using 30 milliseconds should not change any behavior unless
I fucked up a number somewhere.
arithmetic is no fun
rework tcp md5sig config in ldpd
currently ldpd only lets you specify md5 params with neighbors (not
targeted neighbors). this is awkward is you want to peer with
arbitrary hosts on a network since you'll have a to generate a lot of
config for each potential peer on that net.
both cisco and juniper let you set up keys for prefixes. this diff
follows their lead and moves tcpmd5 config out of neighbors and up to
the global scope. to work in my test environment, my config now looks
like this:
# global configuration
router-id 192.168.0.25
tcp md5sig password p6zFE8f794c7NaKG inet 192.168.0.0/24
address-family ipv4 {
# explicit-null yes
# keepalive 120
# targeted-hello-accept yes
# transport-address 10.0.0.1
interface vmx1
}
you can then punch holes in this if you want with "no tcp md5sig" lines.
thoughts? ok?
Index: lde.c
===
RCS file: /cvs/src/usr.sbin/ldpd/lde.c,v
retrieving revision 1.73
diff -u -p -r1.73 lde.c
--- lde.c 4 Mar 2017 00:15:35 - 1.73
+++ lde.c 22 Jan 2019 03:33:35 -
@@ -480,6 +480,7 @@ lde_dispatch_parent(int fd, short event,
LIST_INIT(&nconf->tnbr_list);
LIST_INIT(&nconf->nbrp_list);
LIST_INIT(&nconf->l2vpn_list);
+ LIST_INIT(&nconf->auth_list);
break;
case IMSG_RECONF_IFACE:
if ((niface = malloc(sizeof(struct iface))) == NULL)
@@ -534,6 +535,18 @@ lde_dispatch_parent(int fd, short event,
npw->l2vpn = nl2vpn;
LIST_INSERT_HEAD(&nl2vpn->pw_list, npw, entry);
break;
+ case IMSG_RECONF_CONF_AUTH: {
+ struct ldp_auth *auth;
+
+ auth = malloc(sizeof(*auth));
+ if (auth == NULL)
+ fatal(NULL);
+
+ memcpy(auth, imsg.data, sizeof(*auth));
+
+ LIST_INSERT_HEAD(&nconf->auth_list, auth, entry);
+ break;
+ }
case IMSG_RECONF_END:
merge_config(ldeconf, nconf);
nconf = NULL;
Index: ldpd.c
===
RCS file: /cvs/src/usr.sbin/ldpd/ldpd.c,v
retrieving revision 1.62
diff -u -p -r1.62 ldpd.c
--- ldpd.c 3 Mar 2017 23:36:06 - 1.62
+++ ldpd.c 22 Jan 2019 03:33:35 -
@@ -60,6 +60,7 @@ static voidmerge_nbrps(struct ldpd_co
static void merge_l2vpns(struct ldpd_conf *, struct ldpd_conf *);
static void merge_l2vpn(struct ldpd_conf *, struct l2vpn *,
struct l2vpn *);
+static void merge_auths(struct ldpd_conf *, struct ldpd_conf *);
struct ldpd_global global;
struct ldpd_conf *ldpd_conf;
@@ -681,11 +682,18 @@ main_imsg_send_config(struct ldpd_conf *
struct l2vpn*l2vpn;
struct l2vpn_if *lif;
struct l2vpn_pw *pw;
+ struct ldp_auth *auth;
if (main_imsg_compose_both(IMSG_RECONF_CONF, xconf,
sizeof(*xconf)) == -1)
return (-1);
+ LIST_FOREACH(auth, &xconf->auth_list, entry) {
+ if (main_imsg_compose_both(IMSG_RECONF_CONF_AUTH,
+ auth, sizeof(*auth)) == -1)
+ return (-1);
+ }
+
LIST_FOREACH(iface, &xconf->iface_list, entry) {
if (main_imsg_compose_both(IMSG_RECONF_IFACE, iface,
sizeof(*iface)) == -1)
@@ -747,6 +755,7 @@ void
merge_config(struct ldpd_conf *conf, struct ldpd_conf *xconf)
{
merge_global(conf, xconf);
+ merge_auths(conf, xconf);
merge_af(AF_INET, &conf->ipv4, &xconf->ipv4);
merge_af(AF_INET6, &conf->ipv6, &xconf->ipv6);
merge_ifaces(conf, xconf);
@@ -971,7 +980,7 @@ merge_nbrps(struct ldpd_conf *conf, stru
nbr = nbr_find_ldpid(xn->lsr_id.s_addr);
if (nbr) {
session_shutdown(nbr, S_SHUTDOWN, 0, 0);
- if (pfkey_establish(nbr, xn) == -1)
+ if (pfkey_establish(conf, nbr) == -1)
fatalx("pfkey setup failed");
if (nbr_session_active_role(nbr))
nbr_establish_connection(nbr);
@@ -984,9 +993,7 @@ merge_nbrps(struct ldpd_conf *conf, stru
if (nbrp->flags != xn->flags ||
nbrp->keepalive != xn->keepalive ||
nbrp->gtsm_enabled != xn->gtsm_enabled ||
- nbrp->gtsm_hops !=
sensors hiding with pledge
We recently had a thread about adding more sensors, but then the browser will
use them to spy on us, and everybody was sad. We allow hw.sensors even for
pledge processes because ntpd needs to read the time. However, ntpd only needs
to read the time.
This diff zeroes out sensors other than timedeltas. Maybe some others can be
added as needed, but that seemed a good place to start. I didn't want to
change the code too much (i.e. hide the existence of sensors entirely) so it
just changes them all to 0 valued plain integer sensors.
Thoughts?
Index: kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.353
diff -u -p -r1.353 kern_sysctl.c
--- kern_sysctl.c 19 Jan 2019 01:53:44 - 1.353
+++ kern_sysctl.c 22 Jan 2019 02:01:30 -
@@ -137,7 +137,7 @@ int sysctl_proc_nobroadcastkill(int *, u
struct proc *);
int sysctl_proc_vmmap(int *, u_int, void *, size_t *, struct proc *);
int sysctl_intrcnt(int *, u_int, void *, size_t *);
-int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t);
+int sysctl_sensors(int *, u_int, void *, size_t *, void *, size_t, struct proc
*);
int sysctl_cptime2(int *, u_int, void *, size_t *, void *, size_t);
#if NAUDIO > 0
int sysctl_audio(int *, u_int, void *, size_t *, void *, size_t);
@@ -735,7 +735,7 @@ hw_sysctl(int *name, u_int namelen, void
#ifndefSMALL_KERNEL
case HW_SENSORS:
return (sysctl_sensors(name + 1, namelen - 1, oldp, oldlenp,
- newp, newlen));
+ newp, newlen, p));
case HW_SETPERF:
return (sysctl_hwsetperf(oldp, oldlenp, newp, newlen));
case HW_PERFPOLICY:
@@ -2302,7 +2302,7 @@ sysctl_intrcnt(int *name, u_int namelen,
int
sysctl_sensors(int *name, u_int namelen, void *oldp, size_t *oldlenp,
-void *newp, size_t newlen)
+void *newp, size_t newlen, struct proc *p)
{
struct ksensor *ks;
struct sensor *us;
@@ -2350,6 +2350,22 @@ sysctl_sensors(int *name, u_int namelen,
us->status = ks->status;
us->numt = ks->numt;
us->flags = ks->flags;
+
+ /* not all sensors exposed to pledged processes */
+ if (p->p_p->ps_flags & PS_PLEDGE) {
+ switch (us->type) {
+ case SENSOR_TIMEDELTA:
+ break;
+ default:
+ memset(us->desc, 0, sizeof(us->desc));
+ memset(&us->tv, 0, sizeof(us->tv));
+ us->value = 0;
+ us->type = SENSOR_INTEGER;
+ us->status = SENSOR_S_UNKNOWN;
+ us->flags = SENSOR_FUNKNOWN;
+ break;
+ }
+ }
ret = sysctl_rdstruct(oldp, oldlenp, newp, us,
sizeof(struct sensor));
Re: replacing timeout_add() with timeout_add_msec()
Scott Cheloha wrote:
> > > diff --git sys/arch/sparc64/dev/fd.c sys/arch/sparc64/dev/fd.c
> > > index 8d548062f83..654d8c95524 100644
> > > --- sys/arch/sparc64/dev/fd.c
> > > +++ sys/arch/sparc64/dev/fd.c
> > > @@ -1632,7 +1632,7 @@ loop:
> > > fdc->sc_state = RECALCOMPLETE;
> > > if (fdc->sc_flags & FDC_NEEDHEADSETTLE) {
> > > /* allow 1/30 second for heads to settle */
> > > - timeout_add(&fdc->fdcpseudointr_to, hz / 30);
> > > + timeout_add_msec(&fdc->fdcpseudointr_to, 33);
> > > return (1); /* will return later */
> > > }
> > >
> >
> > Wonder if this should be 30 or 40 since 33 is rather odd.
>
> The intent was to wait a 30th of a second. In practice it has always
> fired 30ms hence. It's a magic number, so I'd just call it 30ms.
> miod might have an opinion on whether those extra milliseconds would
> be useful in the event that sparc64 is ever able to timeout with
> millisecond or better precision on OpenBSD.
make it 30 and the let the next sparc64 user with a floppy drive worry about
the fallout. :)
Re: replacing timeout_add() with timeout_add_msec()
On Mon, Jan 21, 2019 at 04:59:38AM +0100, Claudio Jeker wrote:
> On Sat, Jan 12, 2019 at 10:40:35PM -0600, Amit Kulkarni wrote:
> > > [...]
> >
> > Thanks for the feedback Claudio and Mark!
> >
> > Here is a new diff based on your suggestions, [...]
> >
> > diff --git sys/arch/armv7/exynos/exuart.c sys/arch/armv7/exynos/exuart.c
> > index 4b0588750ea..15086bc5976 100644
> > --- sys/arch/armv7/exynos/exuart.c
> > +++ sys/arch/armv7/exynos/exuart.c
> > @@ -283,7 +283,7 @@ exuart_intr(void *arg)
> > [...]
>
> This one is OK.
>
> > diff --git sys/arch/sparc64/dev/fd.c sys/arch/sparc64/dev/fd.c
> > index 8d548062f83..654d8c95524 100644
> > --- sys/arch/sparc64/dev/fd.c
> > +++ sys/arch/sparc64/dev/fd.c
> > @@ -1632,7 +1632,7 @@ loop:
> > fdc->sc_state = RECALCOMPLETE;
> > if (fdc->sc_flags & FDC_NEEDHEADSETTLE) {
> > /* allow 1/30 second for heads to settle */
> > - timeout_add(&fdc->fdcpseudointr_to, hz / 30);
> > + timeout_add_msec(&fdc->fdcpseudointr_to, 33);
> > return (1); /* will return later */
> > }
> >
>
> Wonder if this should be 30 or 40 since 33 is rather odd.
The intent was to wait a 30th of a second. In practice it has always
fired 30ms hence. It's a magic number, so I'd just call it 30ms.
miod might have an opinion on whether those extra milliseconds would
be useful in the event that sparc64 is ever able to timeout with
millisecond or better precision on OpenBSD.
> > diff --git sys/dev/fdt/imxuart.c sys/dev/fdt/imxuart.c
> > index 84c7eb5aee6..c2fd7e4a6d3 100644
> > --- sys/dev/fdt/imxuart.c
> > +++ sys/dev/fdt/imxuart.c
> > @@ -228,7 +228,7 @@ imxuart_intr(void *arg)
> > [...]
>
> > diff --git sys/net/if_pppoe.c sys/net/if_pppoe.c
> > index e55316351fd..9e03310714b 100644
> > --- sys/net/if_pppoe.c
> > +++ sys/net/if_pppoe.c
> > @@ -1346,7 +1346,7 @@ pppoe_tlf(struct sppp *sp)
> > * function and defer disconnecting to the timeout handler.
> > */
> > sc->sc_state = PPPOE_STATE_CLOSING;
> > - timeout_add(&sc->sc_timeout, hz / 50);
> > + timeout_add_msec(&sc->sc_timeout, 20);
> > }
> >
> > static void
>
> I would wait with this one, since there are many more timeout_add() in the
> file that also should be converted.
Agreed.
> Anyone else wants to comment on this or give me an OK?
You're ok cheloha@ math-wise. But sparc64 is greek to me so
caveat emptor.
Re: qsort comparision function bug
On Mon, 21 Jan 2019, Dariusz Sendkowski wrote: > Wouldn't it lead to undefined behavior? > According to the standard: "... The value of the result of an integer > arithmetic or conversion function cannot be represented (7.8.2.1, 7.8.2.2, > 7.8.2.3, 7.8.2.4, 7.22.6.1, 7.22.6.2, 7.22.1) ..." > This dump case is not compiled with -fwrapv flag, so I guess it might lead > to undefined behavior. OpenBSD sets -fwrapv by default. From clang-local(1): - The -fwrapv option to treat signed integer overflows as defined is enabled by default to prevent dangerous optimizations which could remove security critical overflow checks.
Re: video(1) pledge
On Mon, Jan 21, 2019 at 09:44:59PM +0100, Matthieu Herrb wrote:
> On Mon, Jan 21, 2019 at 09:27:57PM +0100, Landry Breuil wrote:
> > Hi,
> >
>
> Hi,
>
> > now that the 'video' promise is in, looking for okays to pledge
> > video(1).
> >
> > with help & hints from semarie@.
>
> One comment in-line.
> >
> > Index: video.c
> > ===
> > RCS file: /cvs/xenocara/app/video/video.c,v
> > retrieving revision 1.25
> > diff -u -r1.25 video.c
> > --- video.c 9 Apr 2018 18:16:44 - 1.25
> > +++ video.c 30 Dec 2018 09:39:27 -
> > @@ -1961,6 +1961,8 @@
> > argv += optind;
> >
> > if (vid.mode & M_QUERY) {
> > + if (pledge("stdio rpath wpath video", NULL) == -1)
> > + err(1, "pledge");
> > dev_dump_query(&vid);
> > cleanup(&vid, 0);
> > }
> > @@ -1970,6 +1972,14 @@
> >
> > if (!setup(&vid))
> > cleanup(&vid, 1);
> > +
> > + if (vid.mode & M_IN_FILE) {
> > + if (pledge("stdio", NULL) == -1)
>
> Like people have found out the hard way recently, X libs need "rpath"
> in case the X error handler needs to be called.
Right, forgot about that issue. new diff :)
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.26
diff -u -r1.26 video.c
--- video.c 4 Jan 2019 17:45:00 - 1.26
+++ video.c 21 Jan 2019 20:50:06 -
@@ -1961,6 +1961,8 @@
argv += optind;
if (vid.mode & M_QUERY) {
+ if (pledge("stdio rpath wpath video", NULL) == -1)
+ err(1, "pledge");
dev_dump_query(&vid);
cleanup(&vid, 0);
}
@@ -1970,6 +1972,14 @@
if (!setup(&vid))
cleanup(&vid, 1);
+
+ if (vid.mode & M_IN_FILE) {
+ if (pledge("stdio rpath", NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath video", NULL) == -1)
+ err(1, "pledge");
+ }
if (!stream(&vid))
cleanup(&vid, 1);
Re: video(1) pledge
On Mon, Jan 21, 2019 at 09:27:57PM +0100, Landry Breuil wrote:
> Hi,
>
Hi,
> now that the 'video' promise is in, looking for okays to pledge
> video(1).
>
> with help & hints from semarie@.
One comment in-line.
>
> Index: video.c
> ===
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c 9 Apr 2018 18:16:44 - 1.25
> +++ video.c 30 Dec 2018 09:39:27 -
> @@ -1961,6 +1961,8 @@
> argv += optind;
>
> if (vid.mode & M_QUERY) {
> + if (pledge("stdio rpath wpath video", NULL) == -1)
> + err(1, "pledge");
> dev_dump_query(&vid);
> cleanup(&vid, 0);
> }
> @@ -1970,6 +1972,14 @@
>
> if (!setup(&vid))
> cleanup(&vid, 1);
> +
> + if (vid.mode & M_IN_FILE) {
> + if (pledge("stdio", NULL) == -1)
Like people have found out the hard way recently, X libs need "rpath"
in case the X error handler needs to be called.
> + err(1, "pledge");
> + } else {
> + if (pledge("stdio rpath video", NULL) == -1)
> + err(1, "pledge");
> + }
>
> if (!stream(&vid))
> cleanup(&vid, 1);
--
Matthieu Herrb
Re: qsort comparision function bug
Wouldn't it lead to undefined behavior? According to the standard: "... The value of the result of an integer arithmetic or conversion function cannot be represented (7.8.2.1, 7.8.2.2, 7.8.2.3, 7.8.2.4, 7.22.6.1, 7.22.6.2, 7.22.1) ..." This dump case is not compiled with -fwrapv flag, so I guess it might lead to undefined behavior. pon., 21 sty 2019 o 20:55 Otto Moerbeek napisał(a): > Hi, > > This code is buggy, since subtraction and then casting to int will not > produce the right result if the values are too far apart. > > There must be more like this in the tree any volunteers? > > In general, you don't want to do subtraction, even for ints. For > example: compare INT_MIN and 1. INT_MIN - 1 is a large positive > number. Sadly the internet is full of bad examples using subtraction. > > -Otto > > Index: optr.c > === > RCS file: /cvs/src/sbin/dump/optr.c,v > retrieving revision 1.39 > diff -u -p -r1.39 optr.c > --- optr.c 12 Oct 2015 15:12:44 - 1.39 > +++ optr.c 21 Jan 2019 19:45:24 - > @@ -423,6 +423,7 @@ datesort(const void *a1, const void *a2) > > diff = strncmp(d1->dd_name, d2->dd_name, sizeof(d1->dd_name)); > if (diff == 0) > - return (d2->dd_ddate - d1->dd_ddate); > + return (d2->dd_ddate < d1->dd_ddate ? -1 : > + (d2->dd_ddate > d1->dd_ddate ? 1 : 0)); > return (diff); > } > >
video(1) pledge
Hi,
now that the 'video' promise is in, looking for okays to pledge
video(1).
with help & hints from semarie@.
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.25
diff -u -r1.25 video.c
--- video.c 9 Apr 2018 18:16:44 - 1.25
+++ video.c 30 Dec 2018 09:39:27 -
@@ -1961,6 +1961,8 @@
argv += optind;
if (vid.mode & M_QUERY) {
+ if (pledge("stdio rpath wpath video", NULL) == -1)
+ err(1, "pledge");
dev_dump_query(&vid);
cleanup(&vid, 0);
}
@@ -1970,6 +1972,14 @@
if (!setup(&vid))
cleanup(&vid, 1);
+
+ if (vid.mode & M_IN_FILE) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath video", NULL) == -1)
+ err(1, "pledge");
+ }
if (!stream(&vid))
cleanup(&vid, 1);
Re: [PATCH] Huawei E8372 USB Mobile Broadband in HiLink Mode, Generic HiLink Mode Logic
I've been doing some more testing on newer OpenBSD releases (namely 6.4)
and have picked up a couple of problems with this patch. There are a
couple of cleanups needed (a duplicated conditional in umsm.c for
example, and small formatting issues ) as a result of bringing this
patch forward, and I've got to work out why the reattach isn't happening
on OpenBSD 6.4+.
If anyone has any feedback in the meantime, that would be very much
appreciated, but until then, I'll be working on diagnosing.
Best,
James
On Mon, Jan 21, 2019 at 05:04:02PM +1100, James Hebden wrote:
> Hello tech@,
>
> Given this is my first email to the list, and the first patch I have
> submitted to the OpenBSD project, I am very open to feedback on
> the attached patch and context provided!
>
> Below I have included a patch I have been working on to modify some of
> the logic when handling E-series Huawei USB mobile broadband dongles.
>
> These devices are able to operate in several modes, the most compatible
> and performant mode being as a USB networking device ('HiLink' mode),
> providing a NAT'ed connection to the mobile broadband service of the
> connected provider.
>
> Using this mode requires no additional software, other than a DHCP
> client in OpenBSD. Even that is optional, give you could statically
> configure the interface.
>
> This patch refactors lightly some of the logic around handling E-Series
> devices, and moves the original device definitions for the E303 to a
> generic 'E-Series' device definition, given all E-Series devices seem to
> share the parts of the intialisation and the USB product and vendor IDs
> which OpenBSD currently assigns to the E303 device.
>
> I have also added additional logic to identify the E8372, which is the
> device I am testing with, as additional logic is required to switch it
> into HiLink mode. This should work for other E-Series HiLink devices
> supporting the same mode. If anyone with one (or more!) of these
> devices (say, the E3372, which appears to be almost identical)
> could test this patch, it would be very much appreciated.
>
> I have tested this patch and it applies cleanly against -current, 6.4
> and 6.3. I am currently using this on the 6.3 kernel as my
> home router, and so far it has proved reliable.
>
> Best Regards,
> James "ec0" Hebden
>
> Patch follows -
>
> Index: dev/usb/if_cdce.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
> retrieving revision 1.75
> diff -u -p -u -p -r1.75 if_cdce.c
> --- dev/usb/if_cdce.c 2 Oct 2018 19:49:10 - 1.75
> +++ dev/usb/if_cdce.c 21 Jan 2019 05:42:47 -
> @@ -103,6 +103,7 @@ const struct cdce_type cdce_devs[] = {
> {{ USB_VENDOR_NETCHIP, USB_PRODUCT_NETCHIP_ETHERNETGADGET }, 0 },
> {{ USB_VENDOR_COMPAQ, USB_PRODUCT_COMPAQ_IPAQLINUX }, 0 },
> {{ USB_VENDOR_AMBIT, USB_PRODUCT_AMBIT_NTL_250 }, CDCE_SWAPUNION },
> +{{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E8372 }, 0 },
> };
> #define cdce_lookup(v, p) \
> ((const struct cdce_type *)usb_lookup(cdce_devs, v, p))
> @@ -134,6 +135,18 @@ cdce_match(struct device *parent, void *
>
> if (cdce_lookup(uaa->vendor, uaa->product) != NULL)
> return (UMATCH_VENDOR_PRODUCT);
> +
> + if (uaa->vendor == USB_VENDOR_HUAWEI &&
> + uaa->product == USB_PRODUCT_HUAWEI_E8372)
> + {
> +return (UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO);
> + }
> +
> + if (uaa->vendor == USB_VENDOR_HUAWEI &&
> + uaa->product == USB_PRODUCT_HUAWEI_E8372)
> + {
> +return (UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO);
> + }
>
> if (id->bInterfaceClass == UICLASS_CDC &&
> (id->bInterfaceSubClass ==
> Index: dev/usb/umsm.c
> ===
> RCS file: /cvs/src/sys/dev/usb/umsm.c,v
> retrieving revision 1.114
> diff -u -p -u -p -r1.114 umsm.c
> --- dev/usb/umsm.c15 Aug 2018 14:13:07 - 1.114
> +++ dev/usb/umsm.c21 Jan 2019 05:42:48 -
> @@ -133,7 +133,7 @@ static const struct umsm_type umsm_devs[
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E182 }, DEV_UMASS5},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E1820 }, DEV_UMASS5},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E220 }, DEV_HUAWEI},
> - {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E303 }, DEV_UMASS5},
> + {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_ESERIES_INIT }, DEV_UMASS5},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E353_INIT }, DEV_UMASS5},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E510 }, DEV_HUAWEI},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E618 }, DEV_HUAWEI},
> @@ -150,6 +150,7 @@ static const struct umsm_type umsm_devs[
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E1750 }, DEV_UMASS5},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E1752 }, 0},
> {{ USB_VENDOR_HUAWEI, USB_PRODUCT_HUAWEI_E3372 }, DEV_UMASS5},
> + {{ US
qsort comparision function bug
Hi, This code is buggy, since subtraction and then casting to int will not produce the right result if the values are too far apart. There must be more like this in the tree any volunteers? In general, you don't want to do subtraction, even for ints. For example: compare INT_MIN and 1. INT_MIN - 1 is a large positive number. Sadly the internet is full of bad examples using subtraction. -Otto Index: optr.c === RCS file: /cvs/src/sbin/dump/optr.c,v retrieving revision 1.39 diff -u -p -r1.39 optr.c --- optr.c 12 Oct 2015 15:12:44 - 1.39 +++ optr.c 21 Jan 2019 19:45:24 - @@ -423,6 +423,7 @@ datesort(const void *a1, const void *a2) diff = strncmp(d1->dd_name, d2->dd_name, sizeof(d1->dd_name)); if (diff == 0) - return (d2->dd_ddate - d1->dd_ddate); + return (d2->dd_ddate < d1->dd_ddate ? -1 : + (d2->dd_ddate > d1->dd_ddate ? 1 : 0)); return (diff); }
Re: Adapt to Allwinner device tree changes in linux >= 5.0-rc1
> Date: Mon, 21 Jan 2019 11:39:42 +1100
> From: Jonathan Gray
>
> Adapt to allwinner device tree changes in linux >= 5.0-rc1
> "allwinner,sun6i-a31-rtc" has been removed from h3/h5/r40/a64
>
> 507c6e89d6c4b2cd68a8e7ff69d1a00cf74b15dd
> ARM: dts: sunxi: h3/h5: Fix up RTC device node and clock references
>
> 44ff3cafcd7f413e7710a58ac40cfdc3a9380097
> arm64: dts: allwinner: a64: Fix up RTC device node and clock references
>
> 5f9e882825467105acafd208520b69bf95adb963
> ARM: dts: sun8i: r40: Add RTC device node
>
> compile tested only
Sure. Can't do any harm.
> Index: sxirtc.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/sxirtc.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 sxirtc.c
> --- sxirtc.c 27 Mar 2017 14:03:19 - 1.2
> +++ sxirtc.c 21 Jan 2019 00:16:02 -
> @@ -76,7 +76,9 @@ sxirtc_match(struct device *parent, void
>
> return (OF_is_compatible(faa->fa_node, "allwinner,sun4i-a10-rtc") ||
> OF_is_compatible(faa->fa_node, "allwinner,sun7i-a20-rtc") ||
> - OF_is_compatible(faa->fa_node, "allwinner,sun6i-a31-rtc"));
> + OF_is_compatible(faa->fa_node, "allwinner,sun6i-a31-rtc") ||
> + OF_is_compatible(faa->fa_node, "allwinner,sun8i-h3-rtc") ||
> + OF_is_compatible(faa->fa_node, "allwinner,sun50i-h5-rtc"));
> }
>
> void
> @@ -98,7 +100,9 @@ sxirtc_attach(struct device *parent, str
> faa->fa_reg[0].size, 0, &sc->sc_ioh))
> panic("sxirtc_attach: bus_space_map failed!");
>
> - if (OF_is_compatible(faa->fa_node, "allwinner,sun6i-a31-rtc")) {
> + if (OF_is_compatible(faa->fa_node, "allwinner,sun6i-a31-rtc") ||
> + OF_is_compatible(faa->fa_node, "allwinner,sun8i-h3-rtc") ||
> + OF_is_compatible(faa->fa_node, "allwinner,sun50i-h5-rtc")) {
> sc->sc_yymmdd = SXIRTC_YYMMDD_A31;
> sc->sc_hhmmss = SXIRTC_HHMMSS_A31;
> } else {
>
>
Re: bwfm(4): show media mode, TX rate, and RSSI
On Sat, Jan 19, 2019 at 10:52:01PM +0100, Patrick Wildt wrote: > > + case IEEE80211_M_HOSTAP: > > + ieee80211_iterate_nodes(ic, bwfm_update_node, sc); > > + break; > > I think HOSTAP needs > > #ifndef IEEE80211_STA_ONLY > > otherwise ramdisk kernels don't compile. Yes, thanks! mlarkin pointed that out earlier as well. Fixed locally. > With that fixed, ok patrick@. :) Before committing this, I'd like to address the problem that this diff cannot work for 11n rates which aren't mentioned in if_media.h. Rates using SGI and those with a 40MHz channel can't be listed there because if_media.h baudrate defintions only support a single key/value number pairing. The same issue will need to be solved for 11ac rates. I'm considering adding complete MCS index tables for 11n and 11ac to net80211, right below our definitions of standard 11a/b/g rate sets. bwfm(4) won't be the only driver to benefit from this. And MiRA will need such tables in any case, once we add support for rates with SGI and for channels > 20MHz to iwn(4) / iwm(4) in the future. Once such tables exist, bwfm(4) will show the correct rate in all cases except where multiple MCS map the same data rate, which is rare. BTW, Linux uses a mix of tables and formulas for this. I think just having full tables is going to be easier to work with.
