ampintc: fix interrupt acknowledge mask
Hi,
while looking at OpenBSD/arm64 I had interrupt storm issues. Turns out
the issue is in the interrupt controller code. It's the same controller
as on your typical OpenBSD/armv7 machine.
If the controller has for example 288 interrrupts, ((1 << 288) - 1) is
a bit too much to handle. In the end an iack_val of "33" will be stored
as "0" in irq. This means it will handle a different IRQ.
Instead we should split this "dynamic" mask into two parts. First of
all we need to grab only the IRQ bits relevant to us (ICPIAR_IRQ_M).
The other bits contain the CPU ID. When we have done this we can use
the actual IRQ id to do additional checks (spurious and irq >= nintr).
ok?
Patrick
diff --git a/sys/arch/arm/cortex/ampintc.c b/sys/arch/arm/cortex/ampintc.c
index 693eb0d6d91..a26c45e848f 100644
--- a/sys/arch/arm/cortex/ampintc.c
+++ b/sys/arch/arm/cortex/ampintc.c
@@ -488,11 +488,15 @@ ampintc_irq_handler(void *frame)
}
#endif
- if (iack_val == 1023) {
+ irq = iack_val & ICPIAR_IRQ_M;
+
+ if (irq == 1023) {
sc->sc_spur.ec_count++;
return;
}
- irq = iack_val & ((1 << sc->sc_nintr) - 1);
+
+ if (irq >= sc->sc_nintr)
+ return;
pri = sc->sc_ampintc_handler[irq].iq_irq;
s = ampintc_splraise(pri);
Re: ampintc: fix interrupt acknowledge mask
On Fri, Dec 23, 2016 at 10:07:03AM +0100, Patrick Wildt wrote:
> Hi,
>
> while looking at OpenBSD/arm64 I had interrupt storm issues. Turns out
> the issue is in the interrupt controller code. It's the same controller
> as on your typical OpenBSD/armv7 machine.
>
> If the controller has for example 288 interrrupts, ((1 << 288) - 1) is
> a bit too much to handle. In the end an iack_val of "33" will be stored
> as "0" in irq. This means it will handle a different IRQ.
>
> Instead we should split this "dynamic" mask into two parts. First of
> all we need to grab only the IRQ bits relevant to us (ICPIAR_IRQ_M).
> The other bits contain the CPU ID. When we have done this we can use
> the actual IRQ id to do additional checks (spurious and irq >= nintr).
>
> ok?
ok jsg@ for this and the arm64 equivalent.
>
> Patrick
>
> diff --git a/sys/arch/arm/cortex/ampintc.c b/sys/arch/arm/cortex/ampintc.c
> index 693eb0d6d91..a26c45e848f 100644
> --- a/sys/arch/arm/cortex/ampintc.c
> +++ b/sys/arch/arm/cortex/ampintc.c
> @@ -488,11 +488,15 @@ ampintc_irq_handler(void *frame)
> }
> #endif
>
> - if (iack_val == 1023) {
> + irq = iack_val & ICPIAR_IRQ_M;
> +
> + if (irq == 1023) {
> sc->sc_spur.ec_count++;
> return;
> }
> - irq = iack_val & ((1 << sc->sc_nintr) - 1);
> +
> + if (irq >= sc->sc_nintr)
> + return;
>
> pri = sc->sc_ampintc_handler[irq].iq_irq;
> s = ampintc_splraise(pri);
>
Re: Interrupt race in NET_LOCK/NET_UNLOCK
On 23/12/16(Fri) 06:08, Visa Hankala wrote:
> NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
> should restore the level after releasing the lock. Otherwise, lock
> recursion can occur, most likely right after the splx(). An example:
>
> nd6_slowtimo <- NET_LOCK()
> timeout_run
> softclock
> softintr_dispatch
> dosoftint
> interrupt
> k_intr
> if_netisr <- NET_LOCK()
> taskq_thread
>
> OK?
This should never happen. Simply because the NET_LOCK() MUST NOT be
taken in (soft) interrupt context.
The real problem is that nd6_slowtimo() is set twice, once with
timeout_set_proc(9) and once with timeout_set(9). Diff below fixes
that.
ok?
Index: netinet6/nd6.c
===
RCS file: /cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.200
diff -u -p -r1.200 nd6.c
--- netinet6/nd6.c 22 Dec 2016 13:39:32 - 1.200
+++ netinet6/nd6.c 23 Dec 2016 10:37:33 -
@@ -1479,7 +1479,6 @@ nd6_slowtimo(void *ignored_arg)
NET_LOCK(s);
- timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
TAILQ_FOREACH(ifp, &ifnet, if_list) {
Re: Interrupt race in NET_LOCK/NET_UNLOCK
On 23 December 2016 at 11:41, Martin Pieuchot wrote: > On 23/12/16(Fri) 06:08, Visa Hankala wrote: >> NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK() >> should restore the level after releasing the lock. Otherwise, lock >> recursion can occur, most likely right after the splx(). An example: >> >> nd6_slowtimo <- NET_LOCK() >> timeout_run >> softclock >> softintr_dispatch >> dosoftint >> interrupt >> k_intr >> if_netisr <- NET_LOCK() >> taskq_thread >> >> OK? > > This should never happen. Simply because the NET_LOCK() MUST NOT be > taken in (soft) interrupt context. > > The real problem is that nd6_slowtimo() is set twice, once with > timeout_set_proc(9) and once with timeout_set(9). Diff below fixes > that. > > ok? > Most definitely.
Re: Interrupt race in NET_LOCK/NET_UNLOCK
On Fri, Dec 23, 2016 at 11:41:00AM +0100, Martin Pieuchot wrote:
> On 23/12/16(Fri) 06:08, Visa Hankala wrote:
> > NET_LOCK() should raise IPL before acquiring the lock, and NET_UNLOCK()
> > should restore the level after releasing the lock. Otherwise, lock
> > recursion can occur, most likely right after the splx(). An example:
> >
> > nd6_slowtimo <- NET_LOCK()
> > timeout_run
> > softclock
> > softintr_dispatch
> > dosoftint
> > interrupt
> > k_intr
> > if_netisr <- NET_LOCK()
> > taskq_thread
> >
> > OK?
>
> This should never happen. Simply because the NET_LOCK() MUST NOT be
> taken in (soft) interrupt context.
>
> The real problem is that nd6_slowtimo() is set twice, once with
> timeout_set_proc(9) and once with timeout_set(9). Diff below fixes
> that.
>
> ok?
OK bluhm@
>
> Index: netinet6/nd6.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.200
> diff -u -p -r1.200 nd6.c
> --- netinet6/nd6.c22 Dec 2016 13:39:32 - 1.200
> +++ netinet6/nd6.c23 Dec 2016 10:37:33 -
> @@ -1479,7 +1479,6 @@ nd6_slowtimo(void *ignored_arg)
>
> NET_LOCK(s);
>
> - timeout_set(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
> timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
>
> TAILQ_FOREACH(ifp, &ifnet, if_list) {
ssl: move begin hidden decls
Hi, I kind of think the BEGIN should be before the static since static is still part of the function declaration (if SHA1_ASM is not set). Otherwise clang complains. Comments? Patrick diff --git a/lib/libcrypto/sha/sha_locl.h b/lib/libcrypto/sha/sha_locl.h index bb5f1b20721..3b218a900c6 100644 --- a/lib/libcrypto/sha/sha_locl.h +++ b/lib/libcrypto/sha/sha_locl.h @@ -85,12 +85,12 @@ ix=(a)=ROTATE((a),1) \ ) +__BEGIN_HIDDEN_DECLS + #ifndef SHA1_ASM static #endif -__BEGIN_HIDDEN_DECLS - void sha1_block_data_order (SHA_CTX *c, const void *p,size_t num); __END_HIDDEN_DECLS
Re: ripd(8) fails on P2P links
Jeremie Courreges-Anglas writes: > Piotr Durlej writes: > > [...] > >> Any thoughts? Is the patch ok, wrong, accepted, rejected or unnoticed? > > Your diff wouldn't apply because of mangled whitespace (please don't > copy/paste diffs). Here's an updated diff below (untested). Now tested and committed, thanks. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
llvm: skip installing a few headers
Hi, on my endeavour to OpenBSD/arm64 I stumbled upon the clang provided headers that we install. Especially since clang includes its own directories before /usr/include. This means that for example stddef.h is included from clang's directory and not /usr/include. But clang's stddef.h does not include sys/cdefs.h, which libcrypto kind of depends on for BEGIN_HIDDEN_DECLS. Now FreeBSD seems to not install a few of the clang headers. I have this feeling that we should also skip installing those. This is the list of headers that are not installed by FreeBSD, applied to our Makefile. Comments? Patrick diff --git a/gnu/usr.bin/clang/include/clang/intrin/Makefile b/gnu/usr.bin/clang/include/clang/intrin/Makefile index 6489566bcb1..a4632f00f37 100644 --- a/gnu/usr.bin/clang/include/clang/intrin/Makefile +++ b/gnu/usr.bin/clang/include/clang/intrin/Makefile @@ -29,7 +29,6 @@ HEADERS=adxintrin.h \ cuda_builtin_vars.h \ emmintrin.h \ f16cintrin.h \ - float.h \ fma4intrin.h \ fmaintrin.h \ fxsrintrin.h \ @@ -37,10 +36,6 @@ HEADERS=adxintrin.h \ htmxlintrin.h \ ia32intrin.h \ immintrin.h \ - Intrin.h \ - inttypes.h \ - iso646.h \ - limits.h \ lzcntintrin.h \ mm3dnow.h \ mmintrin.h \ @@ -55,20 +50,10 @@ HEADERS=adxintrin.h \ s390intrin.h \ shaintrin.h \ smmintrin.h \ - stdalign.h \ - stdarg.h \ - stdatomic.h \ - stdbool.h \ - stddef.h \ __stddef_max_align_t.h \ - stdint.h \ - stdnoreturn.h \ tbmintrin.h \ - tgmath.h \ tmmintrin.h \ - unwind.h \ vadefs.h \ - varargs.h \ vecintrin.h \ __wmmintrin_aes.h \ wmmintrin.h \
Re: ssl: move begin hidden decls
Patrick Wildt writes: > Hi, > > I kind of think the BEGIN should be before the static since static > is still part of the function declaration (if SHA1_ASM is not set). > Otherwise clang complains. > > Comments? ok -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
ripd(8) use after free
In the neighbor fsm, NBR_ACT_DEL frees the neighbor structure. But
fields of this structure are later accessed, this is mostly visible with
debug output:
nbr_del: neighbor ID 10.64.55.33, peerid 3
nbr_fsm: event 'RESPONSE SENT' resulted in action 'DELETE NBR' and changing
state for neighbor ID 223.223.223.223 from 'ACTIVE' to 'DOWN'
223 is decimal for 0xdf (chunks freed by malloc).
The diff below moves the code around to avoid using free'd memory.
I couldn't spot a dependency between the switch code and the "new
state" code.
ok?
Index: neighbor.c
===
RCS file: /d/cvs/src/usr.sbin/ripd/neighbor.c,v
retrieving revision 1.10
diff -u -p -p -u -r1.10 neighbor.c
--- neighbor.c 18 Jul 2016 21:20:31 - 1.10
+++ neighbor.c 23 Dec 2016 15:05:20 -
@@ -116,21 +116,6 @@ nbr_fsm(struct nbr *nbr, enum nbr_event
return (0);
}
- switch (nbr_fsm_tbl[i].action) {
- case NBR_ACT_RST_TIMER:
- nbr_set_timer(nbr);
- break;
- case NBR_ACT_STRT_TIMER:
- nbr_set_timer(nbr);
- break;
- case NBR_ACT_DEL:
- nbr_act_del(nbr);
- break;
- case NBR_ACT_NOTHING:
- /* do nothing */
- break;
- }
-
if (new_state != 0)
nbr->state = new_state;
@@ -145,6 +130,21 @@ nbr_fsm(struct nbr *nbr, enum nbr_event
nbr_action_name(nbr_fsm_tbl[i].action),
inet_ntoa(nbr->id), nbr_state_name(old_state),
nbr_state_name(nbr->state));
+ }
+
+ switch (nbr_fsm_tbl[i].action) {
+ case NBR_ACT_RST_TIMER:
+ nbr_set_timer(nbr);
+ break;
+ case NBR_ACT_STRT_TIMER:
+ nbr_set_timer(nbr);
+ break;
+ case NBR_ACT_DEL:
+ nbr_act_del(nbr);
+ break;
+ case NBR_ACT_NOTHING:
+ /* do nothing */
+ break;
}
return (0);
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
pf state key link
Hi,
Christiano Haesbaert has sent me this diff.
They are setting pkt_sk to NULL if pkt_sk->reverse is not
pf_statek_key_isvalid(), but the chunk that creates the pkt_sk->reverse
link actually depends on pkt_sk != NULL.
I think it is correct.
ok?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1004
diff -u -p -u -p -r1.1004 pf.c
--- net/pf.c6 Dec 2016 00:01:55 - 1.1004
+++ net/pf.c23 Dec 2016 14:19:26 -
@@ -1002,13 +1002,14 @@ pf_find_state(struct pfi_kif *kif, struc
if (dir == PF_OUT) {
/* first if block deals with outbound forwarded packet */
pkt_sk = m->m_pkthdr.pf.statekey;
- if (pf_state_key_isvalid(pkt_sk) &&
- pf_state_key_isvalid(pkt_sk->reverse)) {
- sk = pkt_sk->reverse;
- } else {
+
+ if (!pf_state_key_isvalid(pkt_sk)) {
pf_pkt_unlink_state_key(m);
pkt_sk = NULL;
}
+
+ if (pkt_sk && pf_state_key_isvalid(pkt_sk->reverse))
+ sk = pkt_sk->reverse;
if (pkt_sk == NULL) {
/* here we deal with local outbound packet */
splassert with pppd
When I kill pppd, running on top of umsm(4), I see the following splasserts: umsm0 at uhub1 port 1 configuration 1 interface 0 "HUAWEI Technologies HUAWEI Mobile Modem" rev 1.10/0.00 addr 2 umsm0: umass only mode. need to reattach umsm0 detached umsm0 at uhub1 port 1 configuration 1 interface 0 "HUAWEI Technologies HUAWEI Mobile Modem" rev 1.10/0.00 addr 2 ucom0 at umsm0 umsm1 at uhub1 port 1 configuration 1 interface 1 "HUAWEI Technologies HUAWEI Mobile Modem" rev 1.10/0.00 addr 2 ucom1 at umsm1 umass0 at uhub1 port 1 configuration 1 interface 2 "HUAWEI Technologies HUAWEI Mobile Modem" rev 1.10/0.00 addr 2 umass0: using SCSI over Bulk-Only scsibus4 at umass0: 2 targets, initiator 0 cd0 at scsibus4 targ 1 lun 0: SCSI2 5/cdrom removable splassert: if_linkstate: want 1 have 0 Starting stack trace... if_linkstate(1,0,d09dd1e7,d03baa39,80) at if_linkstate+0x3f if_linkstate(d40ee000,d09deb02,ffe9,f60d1bac,d03b0da5) at if_linkstate+0x3f pppdealloc(d40ee000,3,f60d1bec,d03b10f4,f60d1bd8) at pppdealloc+0x36 pppclose(d40f8600,3,d90b8300,d9734e18,d9734e18) at pppclose+0x77 ttioctl(d40f8600,8004741b,f60d1e74,3,d90b8300) at ttioctl+0x6e3 ucom_do_ioctl(d40f7a00,8004741b,f60d1e74,3,d90b8300) at ucom_do_ioctl+0x81 ucomioctl(4280,8004741b,f60d1e74,3,d90b8300,0,d96c9218,d03ec5d2,d96c91c8,d96c91c8,f60d1d6c,d03e899f) at ucomioctl+0x6b spec_ioctl(f60d1d60,8004667d,f60d1ef0,d960a180,8004741b) at spec_ioctl+0x9b VOP_IOCTL(d960a180,8004741b,f60d1e74,3,d97a3c00) at VOP_IOCTL+0x4b vn_ioctl(d90b51b8,8004741b,f60d1e74,d90b8300,27) at vn_ioctl+0x7e sys_ioctl(d90b8300,f60d1f5c,f60d1f7c,0,286) at sys_ioctl+0x19f syscall() at syscall+0x250 --- syscall (number 4) --- 0x7: End of stack trace. splassert: sorwakeup: want 1 have 0 Starting stack trace... sorwakeup(1,0,d09d9be1,0,9cee1169) at sorwakeup+0x3f sorwakeup(d90f89f8,d0b58b50,d900c600,0,d0d31bb8) at sorwakeup+0x3f route_input(d900c600,d0b58b60,d0b58b50,d0b58b40,d09d086b) at route_input+0x26a rt_ifmsg(d40ee000,d09dd1e7,d09dd1e7,d03baa39,80) at rt_ifmsg+0xb2 if_linkstate(d40ee000,d09deb02,ffe9,f60d1bac,d03b0da5) at if_linkstate+0x64 pppdealloc(d40ee000,3,f60d1bec,d03b10f4,f60d1bd8) at pppdealloc+0x36 pppclose(d40f8600,3,d90b8300,d9734e18,d9734e18) at pppclose+0x77 ttioctl(d40f8600,8004741b,f60d1e74,3,d90b8300) at ttioctl+0x6e3 ucom_do_ioctl(d40f7a00,8004741b,f60d1e74,3,d90b8300) at ucom_do_ioctl+0x81 ucomioctl(4280,8004741b,f60d1e74,3,d90b8300,0,d96c9218,d03ec5d2,d96c91c8,d96c91c8,f60d1d6c,d03e899f) at ucomioctl+0x6b spec_ioctl(f60d1d60,8004667d,f60d1ef0,d960a180,8004741b) at spec_ioctl+0x9b VOP_IOCTL(d960a180,8004741b,f60d1e74,3,d97a3c00) at VOP_IOCTL+0x4b vn_ioctl(d90b51b8,8004741b,f60d1e74,d90b8300,27) at vn_ioctl+0x7e sys_ioctl(d90b8300,f60d1f5c,f60d1f7c,0,286) at sys_ioctl+0x19f syscall() at syscall+0x250 --- syscall (number 4) --- 0x7: End of stack trace. splassert: sowakeup: want 1 have 0 Starting stack trace... sowakeup(1,0,d09d9d63,d09c9abf,d03cfbb0) at sowakeup+0x43 sowakeup(d90f89f8,d90f8a48,d09d9be1,0,9cee1169) at sowakeup+0x43 sorwakeup(d90f89f8,d0b58b50,d900c600,0,d0d31bb8) at sorwakeup+0xbf route_input(d900c600,d0b58b60,d0b58b50,d0b58b40,d09d086b) at route_input+0x26a rt_ifmsg(d40ee000,d09dd1e7,d09dd1e7,d03baa39,80) at rt_ifmsg+0xb2 if_linkstate(d40ee000,d09deb02,ffe9,f60d1bac,d03b0da5) at if_linkstate+0x64 pppdealloc(d40ee000,3,f60d1bec,d03b10f4,f60d1bd8) at pppdealloc+0x36 pppclose(d40f8600,3,d90b8300,d9734e18,d9734e18) at pppclose+0x77 ttioctl(d40f8600,8004741b,f60d1e74,3,d90b8300) at ttioctl+0x6e3 ucom_do_ioctl(d40f7a00,8004741b,f60d1e74,3,d90b8300) at ucom_do_ioctl+0x81 ucomioctl(4280,8004741b,f60d1e74,3,d90b8300,0,d96c9218,d03ec5d2,d96c91c8,d96c91c8,f60d1d6c,d03e899f) at ucomioctl+0x6b spec_ioctl(f60d1d60,8004667d,f60d1ef0,d960a180,8004741b) at spec_ioctl+0x9b VOP_IOCTL(d960a180,8004741b,f60d1e74,3,d97a3c00) at VOP_IOCTL+0x4b vn_ioctl(d90b51b8,8004741b,f60d1e74,d90b8300,27) at vn_ioctl+0x7e sys_ioctl(d90b8300,f60d1f5c,f60d1f7c,0,286) at sys_ioctl+0x19f syscall() at syscall+0x250 --- syscall (number 4) --- 0x7: End of stack trace.
Re: BFD: route get and route monitor
On 21.12.2016. 23:15, Sebastian Benoit wrote: >> Hi, >> >> it seems that bfd is working with Force10 S4810 and Extreme Networks >> x460 switches. I can test it with cisco c6k5 if you want? > > Hei, > > i'm sure phessler (who might not read this for a couple of days) is happy > about any test you can do. > > And thanks for doing these tests! > > /Benno Hi, no bfd for me on Cisco c6k5. Will upgrade and report back. Tnx for bfd, really great feature ...
Re: llvm: skip installing a few headers
> Date: Fri, 23 Dec 2016 16:01:24 +0100 > From: Patrick Wildt > > Hi, > > on my endeavour to OpenBSD/arm64 I stumbled upon the clang provided > headers that we install. Especially since clang includes its own > directories before /usr/include. This means that for example stddef.h > is included from clang's directory and not /usr/include. But clang's > stddef.h does not include sys/cdefs.h, which libcrypto kind of depends > on for BEGIN_HIDDEN_DECLS. > > Now FreeBSD seems to not install a few of the clang headers. I have > this feeling that we should also skip installing those. > > This is the list of headers that are not installed by FreeBSD, applied > to our Makefile. > > Comments? Well, clang is a C11/C++14 compiler, whereas our header files don't go much beyond providing C99/C++98 support. I think we need to look at this on a header-by-header basis, and implement the missing support first in our headers before we stop installing the corresponding clang header. > diff --git a/gnu/usr.bin/clang/include/clang/intrin/Makefile > b/gnu/usr.bin/clang/include/clang/intrin/Makefile > index 6489566bcb1..a4632f00f37 100644 > --- a/gnu/usr.bin/clang/include/clang/intrin/Makefile > +++ b/gnu/usr.bin/clang/include/clang/intrin/Makefile > @@ -29,7 +29,6 @@ HEADERS=adxintrin.h \ > cuda_builtin_vars.h \ > emmintrin.h \ > f16cintrin.h \ > - float.h \ > fma4intrin.h \ > fmaintrin.h \ > fxsrintrin.h \ > @@ -37,10 +36,6 @@ HEADERS=adxintrin.h \ > htmxlintrin.h \ > ia32intrin.h \ > immintrin.h \ > - Intrin.h \ > - inttypes.h \ > - iso646.h \ > - limits.h \ > lzcntintrin.h \ > mm3dnow.h \ > mmintrin.h \ > @@ -55,20 +50,10 @@ HEADERS=adxintrin.h \ > s390intrin.h \ > shaintrin.h \ > smmintrin.h \ > - stdalign.h \ > - stdarg.h \ > - stdatomic.h \ > - stdbool.h \ > - stddef.h \ > __stddef_max_align_t.h \ > - stdint.h \ > - stdnoreturn.h \ > tbmintrin.h \ > - tgmath.h \ > tmmintrin.h \ > - unwind.h \ > vadefs.h \ > - varargs.h \ > vecintrin.h \ > __wmmintrin_aes.h \ > wmmintrin.h \ > >
Re: ld.so: -fno-builtin?
On Thu, Dec 22, 2016 at 10:35:25PM -0800, Philip Guenther wrote:
> I'm assuming clang handles asm names like gcc, such that declaring
>void *memcpy(void *__restrict, const void *__restrict, __size_t)
> __dso_hidden __asm("_dl_memcpy");
>
> will make even internally generated calls go to _dl_memcpy instead.
No. The backend normally has no idea about assembler names. What problem
are you trying to solve, really?
Joerg
Re: ld.so: -fno-builtin?
> Date: Fri, 23 Dec 2016 18:13:45 +0100
> From: Joerg Sonnenberger
>
> On Thu, Dec 22, 2016 at 10:35:25PM -0800, Philip Guenther wrote:
> > I'm assuming clang handles asm names like gcc, such that declaring
> >void *memcpy(void *__restrict, const void *__restrict, __size_t)
> > __dso_hidden __asm("_dl_memcpy");
> >
> > will make even internally generated calls go to _dl_memcpy instead.
>
> No. The backend normally has no idea about assembler names. What problem
> are you trying to solve, really?
Right. The solution gere is probably to rename _dl_memcpy back to
memcpy. One of the reasons why _dl_memcpy exists is that we wanted to
prevent exporting ld.so's memcpy implementation such that nothing else
would accidentally pick it up. But now that we explicitly control
which symbols we export from ld.so, that risk doesn't exist anymore.
The downside that we will have functions named memcpy in a dynamic
executable still makes debugging a little bit harder, but I'll get
over it.
We probably should not rename all _dl_-prefixed versions of standard C
functions just yet. Especially those that don't fully implement the
standard C functionality.
Cheers,
Mark
Re: ld.so: -fno-builtin?
> On Thu, Dec 22, 2016 at 10:35:25PM -0800, Philip Guenther wrote:
> > I'm assuming clang handles asm names like gcc, such that declaring
> >void *memcpy(void *__restrict, const void *__restrict, __size_t)
> > __dso_hidden __asm("_dl_memcpy");
> >
> > will make even internally generated calls go to _dl_memcpy instead.
>
> No. The backend normally has no idea about assembler names. What problem
> are you trying to solve, really?
Joerg, If you don't actually run OpenBSD you won't understand. Please pipe
down.
clang fixes for iwn(4) and wpi(4)
Clang warns about static inline functions that aren't used. There are
a couple of those in iwn(4) and wpi(4) that are only used if the debug
code is enabled. The diff below wraps them inside the proper #define.
ok?
Index: dev/pci/if_iwn.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwn.c,v
retrieving revision 1.179
diff -u -p -r1.179 if_iwn.c
--- dev/pci/if_iwn.c18 Dec 2016 10:37:42 - 1.179
+++ dev/pci/if_iwn.c23 Dec 2016 17:49:17 -
@@ -879,6 +879,8 @@ iwn_mem_write_2(struct iwn_softc *sc, ui
iwn_mem_write(sc, addr & ~3, tmp);
}
+#ifdef IWN_DEBUG
+
static __inline void
iwn_mem_read_region_4(struct iwn_softc *sc, uint32_t addr, uint32_t *data,
int count)
@@ -886,6 +888,8 @@ iwn_mem_read_region_4(struct iwn_softc *
for (; count > 0; count--, addr += 4)
*data++ = iwn_mem_read(sc, addr);
}
+
+#endif
static __inline void
iwn_mem_set_region_4(struct iwn_softc *sc, uint32_t addr, uint32_t val,
Index: dev/pci/if_wpi.c
===
RCS file: /cvs/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.136
diff -u -p -r1.136 if_wpi.c
--- dev/pci/if_wpi.c5 Oct 2016 21:26:54 - 1.136
+++ dev/pci/if_wpi.c23 Dec 2016 17:49:17 -
@@ -494,6 +494,8 @@ wpi_prph_write_region_4(struct wpi_softc
wpi_prph_write(sc, addr, *data);
}
+#ifdef WPI_DEBUG
+
static __inline uint32_t
wpi_mem_read(struct wpi_softc *sc, uint32_t addr)
{
@@ -517,6 +519,8 @@ wpi_mem_read_region_4(struct wpi_softc *
for (; count > 0; count--, addr += 4)
*data++ = wpi_mem_read(sc, addr);
}
+
+#endif
int
wpi_read_prom_data(struct wpi_softc *sc, uint32_t addr, void *data, int count)
Re: clang fixes for iwn(4) and wpi(4)
On Fri, Dec 23, 2016 at 06:51:48PM +0100, Mark Kettenis wrote:
> Clang warns about static inline functions that aren't used. There are
> a couple of those in iwn(4) and wpi(4) that are only used if the debug
> code is enabled. The diff below wraps them inside the proper #define.
>
> ok?
ok stsp@
>
>
> Index: dev/pci/if_iwn.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwn.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 if_iwn.c
> --- dev/pci/if_iwn.c 18 Dec 2016 10:37:42 - 1.179
> +++ dev/pci/if_iwn.c 23 Dec 2016 17:49:17 -
> @@ -879,6 +879,8 @@ iwn_mem_write_2(struct iwn_softc *sc, ui
> iwn_mem_write(sc, addr & ~3, tmp);
> }
>
> +#ifdef IWN_DEBUG
> +
> static __inline void
> iwn_mem_read_region_4(struct iwn_softc *sc, uint32_t addr, uint32_t *data,
> int count)
> @@ -886,6 +888,8 @@ iwn_mem_read_region_4(struct iwn_softc *
> for (; count > 0; count--, addr += 4)
> *data++ = iwn_mem_read(sc, addr);
> }
> +
> +#endif
>
> static __inline void
> iwn_mem_set_region_4(struct iwn_softc *sc, uint32_t addr, uint32_t val,
> Index: dev/pci/if_wpi.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_wpi.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 if_wpi.c
> --- dev/pci/if_wpi.c 5 Oct 2016 21:26:54 - 1.136
> +++ dev/pci/if_wpi.c 23 Dec 2016 17:49:17 -
> @@ -494,6 +494,8 @@ wpi_prph_write_region_4(struct wpi_softc
> wpi_prph_write(sc, addr, *data);
> }
>
> +#ifdef WPI_DEBUG
> +
> static __inline uint32_t
> wpi_mem_read(struct wpi_softc *sc, uint32_t addr)
> {
> @@ -517,6 +519,8 @@ wpi_mem_read_region_4(struct wpi_softc *
> for (; count > 0; count--, addr += 4)
> *data++ = wpi_mem_read(sc, addr);
> }
> +
> +#endif
>
> int
> wpi_read_prom_data(struct wpi_softc *sc, uint32_t addr, void *data, int
> count)
>
Re: ND6 and splsoftnet()
On Thu, Dec 22, 2016 at 02:03:51PM +0100, Alexander Bluhm wrote:
> Fine. But let's do the other changes. Move timer initialisation
> to nd6_init() and call timeout_set() only once during init. Then
> I don't have to think about wether it is MP safe.
updated diff, parts have been commited
ok?
bluhm
Index: netinet6/ip6_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.172
diff -u -p -r1.172 ip6_input.c
--- netinet6/ip6_input.c20 Dec 2016 18:33:43 - 1.172
+++ netinet6/ip6_input.c23 Dec 2016 17:57:38 -
@@ -119,7 +119,6 @@ struct niqueue ip6intrq = NIQUEUE_INITIA
struct ip6stat ip6stat;
-void ip6_init2(void *);
int ip6_check_rh0hdr(struct mbuf *, int *);
int ip6_hbhchcheck(struct mbuf *, int *, int *, int *);
@@ -157,19 +156,8 @@ ip6_init(void)
ip6_randomid_init();
nd6_init();
frag6_init();
- ip6_init2(NULL);
mq_init(&ip6send_mq, 64, IPL_SOFTNET);
-}
-
-void
-ip6_init2(void *dummy)
-{
-
- /* nd6_timer_init */
- bzero(&nd6_timer_ch, sizeof(nd6_timer_ch));
- timeout_set(&nd6_timer_ch, nd6_timer, NULL);
- timeout_add_sec(&nd6_timer_ch, 1);
}
/*
Index: netinet6/nd6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.201
diff -u -p -r1.201 nd6.c
--- netinet6/nd6.c 23 Dec 2016 15:08:54 - 1.201
+++ netinet6/nd6.c 23 Dec 2016 17:59:30 -
@@ -93,6 +93,8 @@ struct nd_prhead nd_prefix = { 0 };
int nd6_recalc_reachtm_interval = ND6_RECALC_REACHTM_INTERVAL;
void nd6_slowtimo(void *);
+void nd6_timer_work(void *);
+void nd6_timer(void *);
void nd6_invalidate(struct rtentry *);
struct llinfo_nd6 *nd6_free(struct rtentry *, int);
void nd6_llinfo_timer(void *);
@@ -100,7 +102,6 @@ void nd6_llinfo_timer(void *);
struct timeout nd6_slowtimo_ch;
struct timeout nd6_timer_ch;
struct task nd6_timer_task;
-void nd6_timer_work(void *);
int fill_drlist(void *, size_t *, size_t);
int fill_prlist(void *, size_t *, size_t);
@@ -129,6 +130,8 @@ nd6_init(void)
/* start timer */
timeout_set_proc(&nd6_slowtimo_ch, nd6_slowtimo, NULL);
timeout_add_sec(&nd6_slowtimo_ch, ND6_SLOWTIMER_INTERVAL);
+ timeout_set(&nd6_timer_ch, nd6_timer, NULL);
+ timeout_add_sec(&nd6_timer_ch, nd6_prune);
nd6_rs_init();
}
@@ -437,7 +440,6 @@ nd6_timer_work(void *null)
NET_LOCK(s);
- timeout_set(&nd6_timer_ch, nd6_timer, NULL);
timeout_add_sec(&nd6_timer_ch, nd6_prune);
/* expire default router list */
Index: netinet6/nd6.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.h,v
retrieving revision 1.65
diff -u -p -r1.65 nd6.h
--- netinet6/nd6.h 28 Nov 2016 13:59:51 - 1.65
+++ netinet6/nd6.h 23 Dec 2016 17:57:38 -
@@ -223,8 +223,6 @@ extern int nd6_debug;
#define nd6log(x) do { if (nd6_debug) log x; } while (0)
-extern struct timeout nd6_timer_ch;
-
union nd_opts {
struct nd_opt_hdr *nd_opt_array[9];
struct {
@@ -260,7 +258,6 @@ int nd6_options(union nd_opts *);
struct rtentry *nd6_lookup(struct in6_addr *, int, struct ifnet *, u_int);
void nd6_setmtu(struct ifnet *);
void nd6_llinfo_settimer(struct llinfo_nd6 *, int);
-void nd6_timer(void *);
void nd6_purge(struct ifnet *);
void nd6_nud_hint(struct rtentry *);
void nd6_rtrequest(struct ifnet *, int, struct rtentry *);
Fix clang warning in usb code
This one is similar to the athn(4) fix I committed a couple of days
ago. The compiler complains, because the argument to the macro might
be signed. An explicit cast does the trick here as well.
ok?
Index: dev/usb/uhcireg.h
===
RCS file: /cvs/src/sys/dev/usb/uhcireg.h,v
retrieving revision 1.15
diff -u -p -r1.15 uhcireg.h
--- dev/usb/uhcireg.h 15 Apr 2013 09:23:02 - 1.15
+++ dev/usb/uhcireg.h 23 Dec 2016 18:46:31 -
@@ -163,7 +163,7 @@ struct uhci_td {
#define UHCI_TD_GET_ENDPT(s) (((s) >> 15) & 0xf)
#define UHCI_TD_SET_DT(t) ((t) << 19)
#define UHCI_TD_GET_DT(s) (((s) >> 19) & 1)
-#define UHCI_TD_SET_MAXLEN(l) (((l)-1) << 21)
+#define UHCI_TD_SET_MAXLEN(l) (((uint32_t)(l)-1) << 21)
#define UHCI_TD_GET_MAXLEN(s) s) >> 21) + 1) & 0x7ff)
#define UHCI_TD_MAXLEN_MASK0xffe0
u_int32_t td_buffer;
Re: ld.so: -fno-builtin?
On Fri, Dec 23, 2016 at 9:13 AM, Joerg Sonnenberger wrote:
> On Thu, Dec 22, 2016 at 10:35:25PM -0800, Philip Guenther wrote:
>> I'm assuming clang handles asm names like gcc, such that declaring
>>void *memcpy(void *__restrict, const void *__restrict, __size_t)
>> __dso_hidden __asm("_dl_memcpy");
>>
>> will make even internally generated calls go to _dl_memcpy instead.
>
> No. The backend normally has no idea about assembler names. What problem
> are you trying to solve, really?
This is a form we use inside _libc_ so that calls to those functions
generated by gcc will be redirected to aliases with hidden visibility
and thus be local calls, without using the PLT. If that won't work
with clang, then we'll want to figure out some other way to get the
calls to those functions generated by the compiler to be local calls
inside libc.
Reusing for ld.so whatever we make work for libc seems like a good idea.
As kettenis@ notes, the renaming isn't _necessary_ now that we have an
explicit symbol export list for ld.so, but it would still be nice to
be able to tell the compiler to use direct calls, as the generated
code is better than what the linker alone can optimize it to for archs
like i386.
Philip Guenther
Re: ld.so: -fno-builtin?
On Fri, Dec 23, 2016 at 06:43:42PM +0100, Mark Kettenis wrote:
> > Date: Fri, 23 Dec 2016 18:13:45 +0100
> > From: Joerg Sonnenberger
> >
> > On Thu, Dec 22, 2016 at 10:35:25PM -0800, Philip Guenther wrote:
> > > I'm assuming clang handles asm names like gcc, such that declaring
> > >void *memcpy(void *__restrict, const void *__restrict, __size_t)
> > > __dso_hidden __asm("_dl_memcpy");
> > >
> > > will make even internally generated calls go to _dl_memcpy instead.
> >
> > No. The backend normally has no idea about assembler names. What problem
> > are you trying to solve, really?
>
> Right. The solution gere is probably to rename _dl_memcpy back to
> memcpy. One of the reasons why _dl_memcpy exists is that we wanted to
> prevent exporting ld.so's memcpy implementation such that nothing else
> would accidentally pick it up. But now that we explicitly control
> which symbols we export from ld.so, that risk doesn't exist anymore.
Correct. x86 is more forgiving here than others as it doesn't tend to
pull in anything from libgcc/compiler-rt/whatever, but the same issue
will exist e.g. for the division helper if you care about armv6 etc.
Joerg
acpials(4) fix
Missing sentinel. We've been lucky so far with gcc, but clang does
lay out the data in a different way and we crash.
ok?
Index: dev/acpi/acpials.c
===
RCS file: /cvs/src/sys/dev/acpi/acpials.c,v
retrieving revision 1.1
diff -u -p -r1.1 acpials.c
--- dev/acpi/acpials.c 30 Jul 2016 16:25:04 - 1.1
+++ dev/acpi/acpials.c 23 Dec 2016 19:41:45 -
@@ -71,6 +71,7 @@ struct cfdriver acpials_cd = {
const char *acpials_hids[] = {
"ACPI0008",
+ NULL
};
int
Re: acpials(4) fix
On Fri, Dec 23, 2016 at 08:44:07PM +0100, Mark Kettenis wrote:
> Missing sentinel. We've been lucky so far with gcc, but clang does
> lay out the data in a different way and we crash.
>
> ok?
OK
>
>
> Index: dev/acpi/acpials.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpials.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 acpials.c
> --- dev/acpi/acpials.c30 Jul 2016 16:25:04 - 1.1
> +++ dev/acpi/acpials.c23 Dec 2016 19:41:45 -
> @@ -71,6 +71,7 @@ struct cfdriver acpials_cd = {
>
> const char *acpials_hids[] = {
> "ACPI0008",
> + NULL
> };
>
> int
Re: ld.so: -fno-builtin?
On Fri, Dec 23, 2016 at 11:27:15AM -0800, Philip Guenther wrote: > This is a form we use inside _libc_ so that calls to those functions > generated by gcc will be redirected to aliases with hidden visibility > and thus be local calls, without using the PLT. If that won't work > with clang, then we'll want to figure out some other way to get the > calls to those functions generated by the compiler to be local calls > inside libc. I'm not sure about all possible platforms, but for all I can think of, it is enough if the target is hidden. In that case ld should be using a direct jump/call to the destination without creating a PLT entry. Joerg
Re: splassert: ip_output: want 1 have 0
> Date: Thu, 22 Dec 2016 14:56:43 +0100
> From: Martin Pieuchot
>
> On 22/12/16(Thu) 10:45, Martin Pieuchot wrote:
> > On 22/12/16(Thu) 00:32, Mark Kettenis wrote:
> > > splassert: ip_output: want 1 have 0
> > > Starting stack trace...
> > > ip_output() at ip_output+0x7d
> > > ipsp_process_done() at ipsp_process_done+0x2ad
> > > esp_output_cb() at esp_output_cb+0x135
> > > taskq_thread() at taskq_thread+0x6c
> > > end trace frame: 0x0, count: 253
> > > End of stack trace.
> > >
> > > This makes no sense to me since esp_output_cb() calls
> > > ipsp_process_done() while at splsoftnet. What am I missing?
> >
> > It's a missing NET_LOCK(), right now we're abusing splassert() to find
> > code paths where the lock is not held and should be.
>
> This should fix it, do you confirm?
Looked closer at the diff now, and it looks correct to me.
> Index: netinet/ip_ah.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_ah.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 ip_ah.c
> --- netinet/ip_ah.c 19 Sep 2016 18:09:22 - 1.123
> +++ netinet/ip_ah.c 22 Dec 2016 13:55:02 -
> @@ -1219,7 +1219,7 @@ ah_output_cb(struct cryptop *crp)
> return (EINVAL);
> }
>
> - s = splsoftnet();
> + NET_LOCK(s);
>
> tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto);
> if (tdb == NULL) {
> @@ -1236,7 +1236,7 @@ ah_output_cb(struct cryptop *crp)
> /* Reset the session ID */
> if (tdb->tdb_cryptoid != 0)
> tdb->tdb_cryptoid = crp->crp_sid;
> - splx(s);
> + NET_UNLOCK(s);
> return crypto_dispatch(crp);
> }
> free(tc, M_XDATA, 0);
> @@ -1258,11 +1258,11 @@ ah_output_cb(struct cryptop *crp)
> crypto_freereq(crp);
>
> err = ipsp_process_done(m, tdb);
> - splx(s);
> + NET_UNLOCK(s);
> return err;
>
> baddone:
> - splx(s);
> + NET_UNLOCK(s);
>
> m_freem(m);
>
> Index: netinet/ip_esp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_esp.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 ip_esp.c
> --- netinet/ip_esp.c 19 Sep 2016 18:09:22 - 1.141
> +++ netinet/ip_esp.c 22 Dec 2016 13:55:30 -
> @@ -1064,7 +1064,7 @@ esp_output_cb(struct cryptop *crp)
> }
>
>
> - s = splsoftnet();
> + NET_LOCK(s);
>
> tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto);
> if (tdb == NULL) {
> @@ -1081,7 +1081,7 @@ esp_output_cb(struct cryptop *crp)
> /* Reset the session ID */
> if (tdb->tdb_cryptoid != 0)
> tdb->tdb_cryptoid = crp->crp_sid;
> - splx(s);
> + NET_UNLOCK(s);
> return crypto_dispatch(crp);
> }
> free(tc, M_XDATA, 0);
> @@ -1098,11 +1098,11 @@ esp_output_cb(struct cryptop *crp)
>
> /* Call the IPsec input callback. */
> error = ipsp_process_done(m, tdb);
> - splx(s);
> + NET_UNLOCK(s);
> return error;
>
> baddone:
> - splx(s);
> + NET_UNLOCK(s);
>
> m_freem(m);
>
> Index: netinet/ip_ipcomp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_ipcomp.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 ip_ipcomp.c
> --- netinet/ip_ipcomp.c 24 Sep 2016 14:51:37 - 1.48
> +++ netinet/ip_ipcomp.c 22 Dec 2016 13:54:18 -
> @@ -554,7 +554,7 @@ ipcomp_output_cb(struct cryptop *crp)
> return (EINVAL);
> }
>
> - s = splsoftnet();
> + NET_LOCK(s);
>
> tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto);
> if (tdb == NULL) {
> @@ -571,7 +571,7 @@ ipcomp_output_cb(struct cryptop *crp)
> /* Reset the session ID */
> if (tdb->tdb_cryptoid != 0)
> tdb->tdb_cryptoid = crp->crp_sid;
> - splx(s);
> + NET_UNLOCK(s);
> return crypto_dispatch(crp);
> }
> free(tc, M_XDATA, 0);
> @@ -588,7 +588,7 @@ ipcomp_output_cb(struct cryptop *crp)
> /* Compression was useless, we have lost time. */
> crypto_freereq(crp);
> error = ipsp_process_done(m, tdb);
> - splx(s);
> + NET_UNLOCK(s);
> return error;
> }
>
> @@ -638,11 +638,11 @@ ipcomp_output_cb(struct cryptop *crp)
> crypto_freereq(crp);
>
> error = ipsp_process_done(m, tdb);
> - splx(s);
> + NET_UNLOCK(s);
> return error;
>
> baddone:
> - splx(s);
> + NET_UNLOCK(s);
>
> m_freem(m);
>
>
Fix clang warning in ath(4)
Here clang complains about an implicit enum conversion.
Diff below fixes this by simply using the appropriate ieee80211 enum
in the HAL_OPMODE typedef and defining HAL_M_XXX as aliases for
IEEE80211_M_XXX. This matches what we already do for HAL_LED_STATE.
ok?
Index: dev/ic/ar5xxx.h
===
RCS file: /cvs/src/sys/dev/ic/ar5xxx.h,v
retrieving revision 1.57
diff -u -p -r1.57 ar5xxx.h
--- dev/ic/ar5xxx.h 18 Dec 2016 14:34:20 - 1.57
+++ dev/ic/ar5xxx.h 23 Dec 2016 20:35:53 -
@@ -100,12 +100,12 @@ typedef enum {
HAL_ANT_MAX = 3,
} HAL_ANT_SETTING;
-typedef enum {
- HAL_M_STA = 1,
- HAL_M_IBSS = 0,
- HAL_M_HOSTAP = 6,
- HAL_M_MONITOR = 8,
-} HAL_OPMODE;
+typedef enum ieee80211_opmode HAL_OPMODE;
+
+#defineHAL_M_STA IEEE80211_M_STA
+#define HAL_M_IBSS IEEE80211_M_IBSS
+#define HAL_M_HOSTAP IEEE80211_M_HOSTAP
+#define HAL_M_MONITOR IEEE80211_M_MONITOR
typedef int HAL_STATUS;
Re: ld.so: -fno-builtin?
On Fri, Dec 23, 2016 at 12:11 PM, Joerg Sonnenberger wrote: > On Fri, Dec 23, 2016 at 11:27:15AM -0800, Philip Guenther wrote: >> This is a form we use inside _libc_ so that calls to those functions >> generated by gcc will be redirected to aliases with hidden visibility >> and thus be local calls, without using the PLT. If that won't work >> with clang, then we'll want to figure out some other way to get the >> calls to those functions generated by the compiler to be local calls >> inside libc. > > I'm not sure about all possible platforms, but for all I can think of, > it is enough if the target is hidden. Making memcpy hidden is fine for ld.so, but not for libc, thus the alias and asm rename dance we're currently using. > In that case ld should be using > a direct jump/call to the destination without creating a PLT entry. The linker can eliminate the PLT entry and double-jump, but it currently can't reoptimize the calling functions when %ebx is no longer needed for holding the GOT offset for the PLT call. LTO is getting better, I guess, but it was nice getting the Right Thing without requiring it. Philip Guenther
Re: Fix clang warning in ath(4)
On Fri, Dec 23, 2016 at 09:41:07PM +0100, Mark Kettenis wrote:
> Here clang complains about an implicit enum conversion.
>
> Diff below fixes this by simply using the appropriate ieee80211 enum
> in the HAL_OPMODE typedef and defining HAL_M_XXX as aliases for
> IEEE80211_M_XXX. This matches what we already do for HAL_LED_STATE.
>
> ok?
>
ok stsp@
> Index: dev/ic/ar5xxx.h
> ===
> RCS file: /cvs/src/sys/dev/ic/ar5xxx.h,v
> retrieving revision 1.57
> diff -u -p -r1.57 ar5xxx.h
> --- dev/ic/ar5xxx.h 18 Dec 2016 14:34:20 - 1.57
> +++ dev/ic/ar5xxx.h 23 Dec 2016 20:35:53 -
> @@ -100,12 +100,12 @@ typedef enum {
> HAL_ANT_MAX = 3,
> } HAL_ANT_SETTING;
>
> -typedef enum {
> - HAL_M_STA = 1,
> - HAL_M_IBSS = 0,
> - HAL_M_HOSTAP = 6,
> - HAL_M_MONITOR = 8,
> -} HAL_OPMODE;
> +typedef enum ieee80211_opmode HAL_OPMODE;
> +
> +#define HAL_M_STA IEEE80211_M_STA
> +#define HAL_M_IBSS IEEE80211_M_IBSS
> +#define HAL_M_HOSTAP IEEE80211_M_HOSTAP
> +#define HAL_M_MONITORIEEE80211_M_MONITOR
>
> typedef int HAL_STATUS;
>
>
Re: ospfd - add metric and type to print_redistribute
Claudio Jeker writes:
> On Sat, Nov 19, 2016 at 11:38:56AM +, Stuart Henderson wrote:
>> On 2016/11/19 10:06, Remi Locherer wrote:
>> > Hi,
>> >
>> > In the output of ospfd -nv I miss metric and type for the redistribute
>> > statement. The below patch adds this.
>>
>> OK with me. This prints the values when they're at defaults as well,
>> but I don't think that is a problem.
>>
>
> Same here. I'm OK with the diff.
Same diff for ospf6d, ok?
Index: printconf.c
===
RCS file: /d/cvs/src/usr.sbin/ospf6d/printconf.c,v
retrieving revision 1.4
diff -u -p -r1.4 printconf.c
--- printconf.c 22 Aug 2010 21:15:25 - 1.4
+++ printconf.c 23 Dec 2016 22:04:31 -
@@ -72,24 +72,27 @@ print_redistribute(struct ospfd_conf *co
SIMPLEQ_FOREACH(r, &conf->redist_list, entry) {
switch (r->type & ~REDIST_NO) {
case REDIST_STATIC:
- printf("%sredistribute static\n", print_no(r->type));
+ printf("%sredistribute static ", print_no(r->type));
break;
case REDIST_CONNECTED:
- printf("%sredistribute connected\n", print_no(r->type));
+ printf("%sredistribute connected ", print_no(r->type));
break;
case REDIST_LABEL:
- printf("%sredistribute rtlabel %s\n",
+ printf("%sredistribute rtlabel %s ",
print_no(r->type), rtlabel_id2name(r->label));
break;
case REDIST_ADDR:
- printf("%sredistribute %s/%d\n",
+ printf("%sredistribute %s/%d ",
print_no(r->type), log_in6addr(&r->addr),
r->prefixlen);
break;
case REDIST_DEFAULT:
- printf("%sredistribute default\n", print_no(r->type));
+ printf("%sredistribute default ", print_no(r->type));
break;
}
+ printf("set { metric %d type %d }\n",
+ (r->metric & LSA_METRIC_MASK),
+ ((r->metric & LSA_ASEXT_E_FLAG) == 0 ? 1 : 2));
}
}
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
syslogd klog read size
Hi, When the kernel message buffer overflows, a message is printed by syslogd and the buffer is overwritten. The log file looks like this: Dec 23 22:20:54 q70 /bsd: klog: dropped 5687 bytes, message buffer full Dec 23 22:20:54 q70 /bsd: rch, if=vio1: TCP wire: (0) fdd7:e83e:66bc:210::17[27871] fdd7:e83e:66bc:213::72[7] But after a full message buffer is read, we get a split line. Dec 23 22:20:55 q70 /bsd: pf: key search, if=vio1: ICMPv6 wire: (0) fdd7:e83e: Dec 23 22:20:55 q70 /bsd: 66bc:211::17[24240] fdd7:e83e:66bc:213::72[128] This happens when syslogd does a partial read which ends within a line. To avoid the latter, syslogd has to reserve space for the kernel message buffer plus the buffer full message. ok? bluhm Index: usr.sbin/syslogd/syslogd.c === RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v retrieving revision 1.223 diff -u -p -r1.223 syslogd.c --- usr.sbin/syslogd/syslogd.c 30 Nov 2016 07:59:04 - 1.223 +++ usr.sbin/syslogd/syslogd.c 23 Dec 2016 22:04:17 - @@ -489,7 +489,8 @@ main(int argc, char *argv[]) } else LocalDomain = ""; - linesize = getmsgbufsize(); + /* Reserve space for kernel message buffer plus buffer full message. */ + linesize = getmsgbufsize() + 64; if (linesize < MAXLINE) linesize = MAXLINE; linesize++;
Re: syslogd klog read size
Nice find, that explains the reports of broken lines. OK millert@ - todd
syslogd chdir
Hi,
When syslogd is started with a relative path, the reexec in the
parent process fails. The chdir(2) should be done after execvp(3)
in the parrent so that the same executable is found. Note that the
child always does a chdir(2) after chroot(2).
This allows to start ./syslogd which is useful for debugging.
ok?
bluhm
Index: usr.sbin/syslogd/privsep.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.64
diff -u -p -r1.64 privsep.c
--- usr.sbin/syslogd/privsep.c 16 Oct 2016 22:12:50 - 1.64
+++ usr.sbin/syslogd/privsep.c 23 Dec 2016 22:23:05 -
@@ -168,6 +168,8 @@ priv_exec(char *conf, int numeric, int c
struct addrinfo hints, *res0;
struct sigaction sa;
+ chdir("/");
+
if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
NULL) == -1)
err(1, "pledge priv");
Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.223
diff -u -p -r1.223 syslogd.c
--- usr.sbin/syslogd/syslogd.c 30 Nov 2016 07:59:04 - 1.223
+++ usr.sbin/syslogd/syslogd.c 23 Dec 2016 22:22:18 -
@@ -684,8 +684,6 @@ main(int argc, char *argv[])
logdebug("off & running\n");
- chdir("/");
-
tzset();
if (!Debug && !Foreground) {
Build kernels with -ffreestanding?
We already do this on some architectures, but not on amd64 for
example. The main reason is that this disables memcpy() optimizations
that have a measurable impact on the network stack performance.
We can get those optimizations back by doing:
#define memcpy(d, s, n) __builtin_memcpy((d), (s), (n))
I verified that gcc still does proper bounds checking on
__builtin_memcpy(), so we don't lose that.
The nice thing about this solution is that we can choose explicitly
which optimizations we want. And as you can see the kernel makefile
gets simpler ;).
Of course the real reason why I'm looking into this is that clang
makes it really hard to build kernels without -ffreestanding.
The diff below implements this strategy, and enabled the optimizations
for memcpy() and memset(). We can add others if we think there is a
benefit. I've tested the diff on amd64. We may need to put an #undef
memcpy somewhere for platforms that use the generic C code for memcpy.
Thoughts?
Index: sys/systm.h
===
RCS file: /cvs/src/sys/sys/systm.h,v
retrieving revision 1.120
diff -u -p -r1.120 systm.h
--- sys/systm.h 19 Dec 2016 08:36:50 - 1.120
+++ sys/systm.h 23 Dec 2016 22:53:15 -
@@ -330,6 +330,9 @@ extern int (*mountroot)(void);
#include
+#define memcpy(d, s, n) __builtin_memcpy((d), (s), (n))
+#define memset(b, c, n) __builtin_memset((b), (c), (n))
+
#if defined(DDB) || defined(KGDB)
/* debugger entry points */
void Debugger(void); /* in DDB only */
Index: arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.74
diff -u -p -r1.74 Makefile.amd64
--- arch/amd64/conf/Makefile.amd64 29 Nov 2016 09:08:34 - 1.74
+++ arch/amd64/conf/Makefile.amd64 23 Dec 2016 22:53:15 -
@@ -29,9 +29,7 @@ CWARNFLAGS= -Werror -Wall -Wimplicit-fun
CMACHFLAGS=-mcmodel=kernel -mno-red-zone -mno-sse2 -mno-sse -mno-3dnow \
-mno-mmx -msoft-float -fno-omit-frame-pointer
-CMACHFLAGS+= -fno-builtin-printf -fno-builtin-snprintf \
- -fno-builtin-vsnprintf -fno-builtin-log \
- -fno-builtin-log2 -fno-builtin-malloc ${NOPIE_FLAGS}
+CMACHFLAGS+= -ffreestanding ${NOPIE_FLAGS}
.if ${IDENT:M-DNO_PROPOLICE}
CMACHFLAGS+= -fno-stack-protector
.endif
pf route-to rtisvalid
Hi,
I think it is better to check for a valid route than for an existing
route in pf route-to. So call rtisvalid() now. I want to have
pf_route() and pf_route6() as simmilar as possible so I can merge
them some day. As rtalloc() has to stay after embeding the v6
scope, I have moved it down in the v4 code. In the v6 code I always
do the valid route check now. The duplicate route lookup in
pf_refragment6() will be fixed later.
ok?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1006
diff -u -p -r1.1006 pf.c
--- net/pf.c23 Dec 2016 20:49:41 - 1.1006
+++ net/pf.c23 Dec 2016 22:53:07 -
@@ -5832,12 +5832,6 @@ pf_route(struct pf_pdesc *pd, struct pf_
if (ifp == NULL)
goto bad;
- rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
- if (rt == NULL) {
- ipstat_inc(ips_noroute);
- goto bad;
- }
-
if (pd->kif->pfik_ifp != ifp) {
if (pf_test(AF_INET, PF_OUT, ifp, &m0) != PF_PASS)
goto bad;
@@ -5853,6 +5847,12 @@ pf_route(struct pf_pdesc *pd, struct pf_
in_proto_cksum_out(m0, ifp);
+ rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
+ if (!rtisvalid(rt)) {
+ ipstat_inc(ips_noroute);
+ goto bad;
+ }
+
if (ntohs(ip->ip_len) <= ifp->if_mtu) {
ip->ip_sum = 0;
if (ifp->if_capabilities & IFCAP_CSUM_IPv4)
@@ -5991,6 +5991,12 @@ pf_route6(struct pf_pdesc *pd, struct pf
if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
+ rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
+ if (!rtisvalid(rt)) {
+ ip6stat.ip6s_noroute++;
+ goto bad;
+ }
+
/*
* If packet has been reassembled by PF earlier, we have to
* use pf_refragment6() here to turn it back to fragments.
@@ -5998,13 +6004,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
if ((mtag = m_tag_find(m0, PACKET_TAG_PF_REASSEMBLED, NULL))) {
(void) pf_refragment6(&m0, mtag, dst, ifp);
} else if ((u_long)m0->m_pkthdr.len <= ifp->if_mtu) {
- rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
- if (rt == NULL) {
- ip6stat.ip6s_noroute++;
- goto bad;
- }
ifp->if_output(ifp, m0, sin6tosa(dst), rt);
- rtfree(rt);
} else {
icmp6_error(m0, ICMP6_PACKET_TOO_BIG, 0, ifp->if_mtu);
}
@@ -6012,6 +6012,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
done:
if (r->rt != PF_DUPTO)
pd->m = NULL;
+ rtfree(rt);
return;
bad:
Mistake in flex man page
Afternoon! The attached flexdiff changes flex.1 to be accurate about how flex currently works: -lfl does not provide yywrap() by default. Attached also are two lex files I used to find out that the man page wasn't correct. The man page says one can either use %option noyywrap or link with -lfl but only the former solution works. The two lex files intend to do the same thing: to be the simplest lex program, and to produce an a.out that acts like cat. This works: lex optnoyy.l cc -lfl lex.yy.c This does not work: lex noopt.l cc -lfl lex.yy.c The compiler finds undefined references to yywrap, which shows that -lfl does not provide yywrap(), unlike what is said on the man page. I did this testing on 6.0 release, but looking at cvsweb I don't think it'll be any different on -current. Thanks for reading. :D flexdiff Description: Binary data optnoyy.l Description: Binary data noopt.l Description: Binary data
Re: Mistake in flex man page
On Fri, Dec 23, 2016 at 7:26 PM, Andras Farkas wrote: > The attached flexdiff changes flex.1 to be accurate about how flex > currently works: -lfl does not provide yywrap() by default. > > Attached also are two lex files I used to find out that the man page > wasn't correct. > The man page says one can either use %option noyywrap or link with > -lfl but only the former solution works. > The two lex files intend to do the same thing: to be the simplest lex > program, and to produce an a.out that acts like cat. > > This works: > lex optnoyy.l > cc -lfl lex.yy.c > > This does not work: > lex noopt.l > cc -lfl lex.yy.c The error here isn't in flex or your usage of it, but in your usage of cc. Many options to cc are position sensitive, including the -l option. By default, a library for which there is only a static (.a) version like libfl will only have objects from it pulled in if there is *already* an undefined reference to them in the link line. When cc (really 'ld' as invoked by cc) sees the -lfl option, the only undefined reference is 'main'. libfl.a doesn't define that, so none of its objects are pulled in and ld moves on to the next argument. If you flip the command line around as cc lex.yy.c -lfl then it does link, pulling yywrap from libfl.a Rule of thumb: put all the -l options at the end of the command line. If there are multiple -l options, order them by dependencies so that if libA references libB, then put -lA before -lB. E.g., -ltls -lssl -lcrypto should be in that order. Hope that explains why it was failing for you! Philip Guenther
Re: syslogd chdir
>When syslogd is started with a relative path, the reexec in the
>parent process fails. The chdir(2) should be done after execvp(3)
>in the parrent so that the same executable is found. Note that the
>child always does a chdir(2) after chroot(2).
>
>This allows to start ./syslogd which is useful for debugging.
Interesting. I am surprised we haven't hit this in more privsep
programs. Oh wait, you are reusing the same path!
This is why sshd has to be started with an absolute path, to
avoid this problem. Path games like this worried us.
By removing this, you could be adding some subtle risk...
>Index: usr.sbin/syslogd/privsep.c
>===
>RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
>retrieving revision 1.64
>diff -u -p -r1.64 privsep.c
>--- usr.sbin/syslogd/privsep.c 16 Oct 2016 22:12:50 - 1.64
>+++ usr.sbin/syslogd/privsep.c 23 Dec 2016 22:23:05 -
>@@ -168,6 +168,8 @@ priv_exec(char *conf, int numeric, int c
> struct addrinfo hints, *res0;
> struct sigaction sa;
>
>+ chdir("/");
>+
> if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
> NULL) == -1)
> err(1, "pledge priv");
>Index: usr.sbin/syslogd/syslogd.c
>===
>RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
>retrieving revision 1.223
>diff -u -p -r1.223 syslogd.c
>--- usr.sbin/syslogd/syslogd.c 30 Nov 2016 07:59:04 - 1.223
>+++ usr.sbin/syslogd/syslogd.c 23 Dec 2016 22:22:18 -
>@@ -684,8 +684,6 @@ main(int argc, char *argv[])
>
> logdebug("off & running\n");
>
>- chdir("/");
>-
> tzset();
>
> if (!Debug && !Foreground) {
>
>
Re: Mistake in flex man page
On Fri, Dec 23, 2016 at 11:14 PM, Philip Guenther wrote: > Many options to cc are position sensitive, including the -l > option. Oh wow, I see. You're absolutely right. Thank you!
