Re: rpki-client: TZ=UTC + localtime -> gmtime?
Claudio Jeker(cje...@diehard.n-r-g.com) on 2022.04.20 15:12:57 +0200: > On Wed, Apr 20, 2022 at 03:00:15PM +0200, Theo Buehler wrote: > > Found this when looking at the timezone issue a couple of weeks back and > > then forgot about it: > > > > This setenv() + localtime() looks like a hack to me and I don't really > > understand why it should be preferable over using gmtime() directly. The > > only change in output that I can see is that it causes %Z to print GMT > > instead of UTC, so I modified the format string accordingly (but I have > > no strong opinion on this). > > UTC is the proper timezone here. Not sure why gmtime results in %Z to > return GMT since gmtime converts into UTC. On the other hand timezones > in struct tm are strange so it probably just defaults to GMT for a 0 > offset time. well, no. UTC is not a timezone. UTC is a time standard. GMT is the correct timezone.
Re: refcount btrace
I still think it is worth to have refcount debugging in generic
kernel dt(4). Having tools is easier than first adding printf to
hunt a bug. I see no downside.
ok?
bluhm
Index: dev/dt/dt_prov_static.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dt_prov_static.c,v
retrieving revision 1.13
diff -u -p -r1.13 dt_prov_static.c
--- dev/dt/dt_prov_static.c 17 Mar 2022 14:53:59 - 1.13
+++ dev/dt/dt_prov_static.c 21 Apr 2022 21:06:03 -
@@ -87,6 +87,12 @@ DT_STATIC_PROBE1(smr, barrier_exit, "int
DT_STATIC_PROBE0(smr, wakeup);
DT_STATIC_PROBE2(smr, thread, "uint64_t", "uint64_t");
+/*
+ * reference counting
+ */
+DT_STATIC_PROBE0(refcnt, none);
+DT_STATIC_PROBE3(refcnt, inpcb, "void *", "int", "int");
+DT_STATIC_PROBE3(refcnt, tdb, "void *", "int", "int");
/*
* List of all static probes
@@ -127,15 +133,24 @@ struct dt_probe *const dtps_static[] = {
&_DT_STATIC_P(smr, barrier_exit),
&_DT_STATIC_P(smr, wakeup),
&_DT_STATIC_P(smr, thread),
+ /* refcnt */
+ &_DT_STATIC_P(refcnt, none),
+ &_DT_STATIC_P(refcnt, inpcb),
+ &_DT_STATIC_P(refcnt, tdb),
};
+struct dt_probe *const *dtps_index_refcnt;
+
int
dt_prov_static_init(void)
{
int i;
- for (i = 0; i < nitems(dtps_static); i++)
+ for (i = 0; i < nitems(dtps_static); i++) {
+ if (dtps_static[i] == &_DT_STATIC_P(refcnt, none))
+ dtps_index_refcnt = _static[i];
dt_dev_register_probe(dtps_static[i]);
+ }
return i;
}
Index: dev/dt/dtvar.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dtvar.h,v
retrieving revision 1.13
diff -u -p -r1.13 dtvar.h
--- dev/dt/dtvar.h 27 Feb 2022 10:14:01 - 1.13
+++ dev/dt/dtvar.h 21 Apr 2022 21:06:03 -
@@ -313,11 +313,30 @@ extern volatile uint32_t dt_tracing; /*
#defineDT_STATIC_ENTER(func, name, args...) do {
\
extern struct dt_probe _DT_STATIC_P(func, name);\
struct dt_probe *dtp = &_DT_STATIC_P(func, name); \
- struct dt_provider *dtpv = dtp->dtp_prov; \
\
if (__predict_false(dt_tracing) && \
__predict_false(dtp->dtp_recording)) { \
+ struct dt_provider *dtpv = dtp->dtp_prov; \
+ \
dtpv->dtpv_enter(dtpv, dtp, args); \
+ } \
+} while (0)
+
+#define _DT_INDEX_P(func) (dtps_index_##func)
+
+#define DT_INDEX_ENTER(func, index, args...) do { \
+ extern struct dt_probe **_DT_INDEX_P(func); \
+ \
+ if (__predict_false(dt_tracing) && \
+ __predict_false(index > 0) && \
+ __predict_true(_DT_INDEX_P(func) != NULL)) {\
+ struct dt_probe *dtp = _DT_INDEX_P(func)[index];\
+ \
+ if(__predict_false(dtp->dtp_recording)) { \
+ struct dt_provider *dtpv = dtp->dtp_prov; \
+ \
+ dtpv->dtpv_enter(dtpv, dtp, args); \
+ } \
} \
} while (0)
Index: kern/kern_synch.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.185
diff -u -p -r1.185 kern_synch.c
--- kern/kern_synch.c 18 Mar 2022 15:32:06 - 1.185
+++ kern/kern_synch.c 21 Apr 2022 21:06:03 -
@@ -804,7 +804,15 @@ sys___thrwakeup(struct proc *p, void *v,
void
refcnt_init(struct refcnt *r)
{
+ refcnt_init_trace(r, 0);
+}
+
+void
+refcnt_init_trace(struct refcnt *r, int idx)
+{
+ r->r_traceidx = idx;
atomic_store_int(>r_refs, 1);
+ TRACEINDEX(refcnt, r->r_traceidx, r, 0, +1);
}
void
@@ -814,6 +822,7 @@ refcnt_take(struct refcnt *r)
refs = atomic_inc_int_nv(>r_refs);
KASSERT(refs != 0);
+ TRACEINDEX(refcnt, r->r_traceidx, r, refs - 1, +1);
(void)refs;
}
@@ -824,6 +833,7 @@ refcnt_rele(struct refcnt *r)
refs = atomic_dec_int_nv(>r_refs);
KASSERT(refs != ~0);
+ TRACEINDEX(refcnt, r->r_traceidx, r, refs + 1, -1);
Re: Provide memory barriers in refcnt_rele() and refcnt_finalize()
> Date: Thu, 21 Apr 2022 22:17:31 +0200 > From: Alexander Bluhm > > On Mon, Apr 18, 2022 at 08:33:06AM +, Visa Hankala wrote: > > I think the sanest solution is to add the release and acquire barriers > > in refcnt_rele(). > > Getting memory barriers right is too complicated for developers > doing MP stuff. The existing locking and refcount primitives have > to implement that functionality. I am on visa@'s side and would > prefer a memory barrier in refcount API instead of searching for > races in MP code. > > Better waste some CPU cycles in some cases than having strange > behavior due to missing barries in other cases. I don't disagree with that. The refcount API is at the same level as the mutex API and explicit memory barriers should not be needed to use it safely. It's just that it isn't obvious what the right memory ordering semantics are for a refcount API. For some reason this doesn't seem to be something that's widely discussed. The Linux include/linux/refcount.h file has the following comment at the start: * Memory ordering * === * * Memory ordering rules are slightly relaxed wrt regular atomic_t functions * and provide only what is strictly required for refcounts. * * The increments are fully relaxed; these will not provide ordering. The * rationale is that whatever is used to obtain the object we're increasing the * reference count on will provide the ordering. For locked data structures, * its the lock acquire, for RCU/lockless data structures its the dependent * load. * * Do note that inc_not_zero() provides a control dependency which will order * future stores against the inc, this ensures we'll never modify the object * if we did not in fact acquire a reference. * * The decrements will provide release order, such that all the prior loads and * stores will be issued before, it also provides a control dependency, which * will order us against the subsequent free(). * * The control dependency is against the load of the cmpxchg (ll/sc) that * succeeded. This means the stores aren't fully ordered, but this is fine * because the 1->0 transition indicates no concurrency. * * Note that the allocator is responsible for ordering things between free() * and alloc(). * * The decrements dec_and_test() and sub_and_test() also provide acquire * ordering on success. That is still a bit murky, but I think it matches what visa@'s diff does for refcnt_rele(9). And doing what Linux does in its refcount API is probably a good thing. But I don't think it covers the membar_sync() added to the end of refcnt_finalize(9), so I'm not confident about that bit, and would like to understand that better. The other issue I have with the diff is that it documentations the memory ordering in terms of acquire and release which is not what we do in other places such as the membar_enter(9) man page. Maybe this should explicitly call out the memory ordering like what the Linux comment does.
Re: Provide memory barriers in refcnt_rele() and refcnt_finalize()
On Mon, Apr 18, 2022 at 08:33:06AM +, Visa Hankala wrote: > I think the sanest solution is to add the release and acquire barriers > in refcnt_rele(). Getting memory barriers right is too complicated for developers doing MP stuff. The existing locking and refcount primitives have to implement that functionality. I am on visa@'s side and would prefer a memory barrier in refcount API instead of searching for races in MP code. Better waste some CPU cycles in some cases than having strange behavior due to missing barries in other cases. bluhm
Re: pf igmp icmp6 multicast router alert
On 2022-04-21 21:10 +02, Alexander Bluhm wrote: > On Thu, Apr 21, 2022 at 08:56:07PM +0200, Otto Moerbeek wrote: >> > Currently it allows all options. Should I make it specific to >> > router alert with IGMP or ICMP6? >> >> To me it looks like the icmp6 case already is limited to MLD? > > The question is the other way around. My current diff allows any > option with ICMP6 MLD. Do we want to restict the option to router > alert? > > In our ip6.h we have: > #define IP6OPT_JUMBO0xC2/* 11 0 00010 = 194 */ > #define IP6OPT_NSAP_ADDR0xC3/* 11 0 00011 */ > #define IP6OPT_TUNNEL_LIMIT 0x04/* 00 0 00100 */ > #define IP6OPT_ROUTER_ALERT 0x05/* 00 0 00101 (RFC3542, recommended) > */ > > And who knows what other options have been designed. > > In ip.h I see these: > #define IPOPT_RR7 /* record packet route */ > #define IPOPT_TS68 /* timestamp */ > #define IPOPT_SECURITY 130 /* provide s,c,h,tcc */ > #define IPOPT_LSRR 131 /* loose source route */ > #define IPOPT_SATID 136 /* satnet id */ > #define IPOPT_SSRR 137 /* strict source route */ > #define IPOPT_RA148 /* router alert */ > > The option I have ever seen in the wild is router alert. So it may > be better to allow IGMP and ICMP6 multicast if router alert is the > only option in the packet. That's my gut feeling as well. But I don't have any hard evidence that that is actually correct. However, since we learned that router-alert is an actual problem it is also a good incremental step. > > bluhm > -- I'm not entirely sure you are real.
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 21, 2022 at 08:56:07PM +0200, Otto Moerbeek wrote: > > Currently it allows all options. Should I make it specific to > > router alert with IGMP or ICMP6? > > To me it looks like the icmp6 case already is limited to MLD? The question is the other way around. My current diff allows any option with ICMP6 MLD. Do we want to restict the option to router alert? In our ip6.h we have: #define IP6OPT_JUMBO0xC2/* 11 0 00010 = 194 */ #define IP6OPT_NSAP_ADDR0xC3/* 11 0 00011 */ #define IP6OPT_TUNNEL_LIMIT 0x04/* 00 0 00100 */ #define IP6OPT_ROUTER_ALERT 0x05/* 00 0 00101 (RFC3542, recommended) */ And who knows what other options have been designed. In ip.h I see these: #define IPOPT_RR7 /* record packet route */ #define IPOPT_TS68 /* timestamp */ #define IPOPT_SECURITY 130 /* provide s,c,h,tcc */ #define IPOPT_LSRR 131 /* loose source route */ #define IPOPT_SATID 136 /* satnet id */ #define IPOPT_SSRR 137 /* strict source route */ #define IPOPT_RA148 /* router alert */ The option I have ever seen in the wild is router alert. So it may be better to allow IGMP and ICMP6 multicast if router alert is the only option in the packet. bluhm
Re: pf igmp icmp6 multicast router alert
On Thu, Apr 21, 2022 at 07:08:38PM +0200, Alexander Bluhm wrote:
> Hi,
>
> IGMP and ICMP6 for multicast packets have router alert options.
> Per default pf drops all IP packets with options. Usually people
> ask what is wrong until someone points out that they have to use a
> pf rule with allow-opts.
>
> As this is normal behavior and our kernel generates such packets,
> the pf default is bad.
>
> Diff is untested, but otto@ and florian@ could try it.
Tested this with success on two hosts that hasd issues because their
outgoing icmp6 messages were blcoked.
> Currently it allows all options. Should I make it specific to
> router alert with IGMP or ICMP6?
To me it looks like the icmp6 case already is limited to MLD?
-Otto
>
> bluhm
>
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1126
> diff -u -p -r1.1126 pf.c
> --- net/pf.c 17 Mar 2022 18:27:55 - 1.1126
> +++ net/pf.c 21 Apr 2022 16:30:18 -
> @@ -6398,6 +6398,9 @@ pf_walk_header(struct pf_pdesc *pd, stru
> end = pd->off + ntohs(h->ip_len);
> pd->off += hlen;
> pd->proto = h->ip_p;
> + /* IGMP packets have router alert options, allow them */
> + if (pd->proto == IPPROTO_IGMP)
> + pd->badopts = 0;
> /* stop walking over non initial fragments */
> if ((h->ip_off & htons(IP_OFFMASK)) != 0)
> return (PF_PASS);
> @@ -6494,6 +6497,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
> {
> struct ip6_frag frag;
> struct ip6_ext ext;
> + struct icmp6_hdr icmp6;
> struct ip6_rthdr rthdr;
> u_int32_tend;
> int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
> @@ -6607,9 +6611,23 @@ pf_walk_header6(struct pf_pdesc *pd, str
> pd->off += (ext.ip6e_len + 1) * 8;
> pd->proto = ext.ip6e_nxt;
> break;
> + case IPPROTO_ICMPV6:
> + if (!pf_pull_hdr(pd->m, pd->off, , sizeof(icmp6),
> + NULL, reason, AF_INET6)) {
> + DPFPRINTF(LOG_NOTICE, "IPv6 short icmp6hdr");
> + return (PF_DROP);
> + }
> + /* ICMP multicast packets have router alert options */
> + switch (icmp6.icmp6_type) {
> + case MLD_LISTENER_QUERY:
> + case MLD_LISTENER_REPORT:
> + case MLD_LISTENER_DONE:
> + pd->badopts = 0;
> + break;
> + }
> + /* FALLTHROUGH */
> case IPPROTO_TCP:
> case IPPROTO_UDP:
> - case IPPROTO_ICMPV6:
> /* fragments may be short, ignore inner header then */
> if (pd->fragoff != 0 && end < pd->off +
> (pd->proto == IPPROTO_TCP ? sizeof(struct tcphdr) :
>
Re: obscure vi crash, fix
On Thu, Apr 21, 2022 at 11:07:50AM -0600, Todd C. Miller wrote: > On Thu, 21 Apr 2022 09:35:44 -0700, Jeremy Mates wrote: > > > The cause is an unguarded use of the NULL output pointer. I am pretty > > sure an .exrc cannot cause this condition (map rhs requires > > something, not nothing) only recompiling with a NULL output string > > for some command. > > > > One fix is to guard the "init_nomap = !e_memcmp(qp->output," line in > > common/key.c with something like > > > > if (qp->output) > > init_nomap = !e_memcmp(qp->output, >i_event[gp->i_next], qp->ilen); > > That seems reasonable to me since the other users of qp->output do > check for NULL. We don't need to worry about init_nomap being unset > due to the "goto retry" that happens a few lines down. This makes sense. As QREM() modifies gp->inext, we can't simply move this assignment down. ok tb
pf igmp icmp6 multicast router alert
Hi,
IGMP and ICMP6 for multicast packets have router alert options.
Per default pf drops all IP packets with options. Usually people
ask what is wrong until someone points out that they have to use a
pf rule with allow-opts.
As this is normal behavior and our kernel generates such packets,
the pf default is bad.
Diff is untested, but otto@ and florian@ could try it.
Currently it allows all options. Should I make it specific to
router alert with IGMP or ICMP6?
bluhm
Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1126
diff -u -p -r1.1126 pf.c
--- net/pf.c17 Mar 2022 18:27:55 - 1.1126
+++ net/pf.c21 Apr 2022 16:30:18 -
@@ -6398,6 +6398,9 @@ pf_walk_header(struct pf_pdesc *pd, stru
end = pd->off + ntohs(h->ip_len);
pd->off += hlen;
pd->proto = h->ip_p;
+ /* IGMP packets have router alert options, allow them */
+ if (pd->proto == IPPROTO_IGMP)
+ pd->badopts = 0;
/* stop walking over non initial fragments */
if ((h->ip_off & htons(IP_OFFMASK)) != 0)
return (PF_PASS);
@@ -6494,6 +6497,7 @@ pf_walk_header6(struct pf_pdesc *pd, str
{
struct ip6_frag frag;
struct ip6_ext ext;
+ struct icmp6_hdr icmp6;
struct ip6_rthdr rthdr;
u_int32_tend;
int hdr_cnt, fraghdr_cnt = 0, rthdr_cnt = 0;
@@ -6607,9 +6611,23 @@ pf_walk_header6(struct pf_pdesc *pd, str
pd->off += (ext.ip6e_len + 1) * 8;
pd->proto = ext.ip6e_nxt;
break;
+ case IPPROTO_ICMPV6:
+ if (!pf_pull_hdr(pd->m, pd->off, , sizeof(icmp6),
+ NULL, reason, AF_INET6)) {
+ DPFPRINTF(LOG_NOTICE, "IPv6 short icmp6hdr");
+ return (PF_DROP);
+ }
+ /* ICMP multicast packets have router alert options */
+ switch (icmp6.icmp6_type) {
+ case MLD_LISTENER_QUERY:
+ case MLD_LISTENER_REPORT:
+ case MLD_LISTENER_DONE:
+ pd->badopts = 0;
+ break;
+ }
+ /* FALLTHROUGH */
case IPPROTO_TCP:
case IPPROTO_UDP:
- case IPPROTO_ICMPV6:
/* fragments may be short, ignore inner header then */
if (pd->fragoff != 0 && end < pd->off +
(pd->proto == IPPROTO_TCP ? sizeof(struct tcphdr) :
Re: obscure vi crash, fix
On Thu, 21 Apr 2022 09:35:44 -0700, Jeremy Mates wrote:
> The cause is an unguarded use of the NULL output pointer. I am pretty
> sure an .exrc cannot cause this condition (map rhs requires
> something, not nothing) only recompiling with a NULL output string
> for some command.
>
> One fix is to guard the "init_nomap = !e_memcmp(qp->output," line in
> common/key.c with something like
>
> if (qp->output)
> init_nomap = !e_memcmp(qp->output, >i_event[gp->i_next], qp->ilen);
That seems reasonable to me since the other users of qp->output do
check for NULL. We don't need to worry about init_nomap being unset
due to the "goto retry" that happens a few lines down.
- todd
Index: usr.bin/vi/common/key.c
===
RCS file: /cvs/src/usr.bin/vi/common/key.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 key.c
--- usr.bin/vi/common/key.c 18 Apr 2017 01:45:35 - 1.19
+++ usr.bin/vi/common/key.c 21 Apr 2022 17:03:26 -
@@ -650,7 +650,10 @@ not_digit: argp->e_c = CH_NOT_DIGIT;
}
/* Find out if the initial segments are identical. */
- init_nomap = !e_memcmp(qp->output, >i_event[gp->i_next], qp->ilen);
+ if (qp->output != NULL) {
+ init_nomap =
+ !e_memcmp(qp->output, >i_event[gp->i_next], qp->ilen);
+ }
/* Delete the mapped characters from the queue. */
QREM(qp->ilen);
obscure vi crash, fix
One may wish to disable the arrow keys in vi(1), because it is cold and the jacket will sometimes brush up against said keys, causing unexpected cursor motions. This may be done by modifying cl/cl_term.c to set "kcud1" and friends to use NULL for the output string, plus some obvious tkp->output string length changes just below that. This is according to the seq_set() comment: * An input string must always be present. The output string * can be NULL, when set internally, that's how we throw away * input. However, after recompiling, vi crashes when the arrow keys are pressed. The cause is an unguarded use of the NULL output pointer. I am pretty sure an .exrc cannot cause this condition (map rhs requires something, not nothing) only recompiling with a NULL output string for some command. One fix is to guard the "init_nomap = !e_memcmp(qp->output," line in common/key.c with something like if (qp->output) init_nomap = !e_memcmp(qp->output, >i_event[gp->i_next], qp->ilen);
Re: router timer mutex
On Wed, Apr 20, 2022 at 08:12:51PM +0200, Alexander Bluhm wrote:
> mvs@ reminded me of a crash I have seen in December. Route timers
> are not MP safe, but I think this can be fixed with a mutex. The
> idea is to protect the global lists with a mutex and move the rttimer
> into a temporary list. Then the callback and pool put can be called
> later without mutex.
I have a tiny update to the diff.
- Global locks are documented with capital letter, so use [T].
- rt_timer_add() grabbed the mutex twice, first remove then add.
Better exchange in one critical section. pool_get before and
pool_put after.
ok?
Index: net/route.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
retrieving revision 1.406
diff -u -p -r1.406 route.c
--- net/route.c 20 Apr 2022 17:58:22 - 1.406
+++ net/route.c 21 Apr 2022 13:31:52 -
@@ -1361,7 +1361,8 @@ rt_ifa_purge_walker(struct rtentry *rt,
* for multiple queues for efficiency's sake...
*/
-LIST_HEAD(, rttimer_queue) rttimer_queue_head;
+struct mutex rttimer_mtx;
+LIST_HEAD(, rttimer_queue) rttimer_queue_head; /* [T] */
#define RTTIMER_CALLOUT(r) { \
if (r->rtt_func != NULL) { \
@@ -1393,6 +1394,7 @@ rt_timer_init(void)
pool_init(_queue_pool, sizeof(struct rttimer_queue), 0,
IPL_MPFLOOR, 0, "rttmrq", NULL);
+ mtx_init(_mtx, IPL_MPFLOOR);
LIST_INIT(_queue_head);
timeout_set_proc(_timer_timeout, rt_timer_timer, _timer_timeout);
timeout_add_sec(_timer_timeout, 1);
@@ -1408,7 +1410,10 @@ rt_timer_queue_create(int timeout)
rtq->rtq_timeout = timeout;
rtq->rtq_count = 0;
TAILQ_INIT(>rtq_head);
+
+ mtx_enter(_mtx);
LIST_INSERT_HEAD(_queue_head, rtq, rtq_link);
+ mtx_leave(_mtx);
return (rtq);
}
@@ -1416,28 +1421,36 @@ rt_timer_queue_create(int timeout)
void
rt_timer_queue_change(struct rttimer_queue *rtq, int timeout)
{
+ mtx_enter(_mtx);
rtq->rtq_timeout = timeout;
+ mtx_leave(_mtx);
}
void
rt_timer_queue_destroy(struct rttimer_queue *rtq)
{
- struct rttimer *r;
+ struct rttimer *r;
+ TAILQ_HEAD(, rttimer)rttlist;
NET_ASSERT_LOCKED();
+ TAILQ_INIT();
+ mtx_enter(_mtx);
while ((r = TAILQ_FIRST(>rtq_head)) != NULL) {
LIST_REMOVE(r, rtt_link);
TAILQ_REMOVE(>rtq_head, r, rtt_next);
+ TAILQ_INSERT_TAIL(, r, rtt_next);
+ KASSERT(rtq->rtq_count > 0);
+ rtq->rtq_count--;
+ }
+ LIST_REMOVE(rtq, rtq_link);
+ mtx_leave(_mtx);
+
+ while ((r = TAILQ_FIRST()) != NULL) {
+ TAILQ_REMOVE(, r, rtt_next);
RTTIMER_CALLOUT(r);
pool_put(_pool, r);
- if (rtq->rtq_count > 0)
- rtq->rtq_count--;
- else
- printf("rt_timer_queue_destroy: rtq_count reached 0\n");
}
-
- LIST_REMOVE(rtq, rtq_link);
pool_put(_queue_pool, rtq);
}
@@ -1450,15 +1463,22 @@ rt_timer_queue_count(struct rttimer_queu
void
rt_timer_remove_all(struct rtentry *rt)
{
- struct rttimer *r;
+ struct rttimer *r;
+ TAILQ_HEAD(, rttimer)rttlist;
+ TAILQ_INIT();
+ mtx_enter(_mtx);
while ((r = LIST_FIRST(>rt_timer)) != NULL) {
LIST_REMOVE(r, rtt_link);
TAILQ_REMOVE(>rtt_queue->rtq_head, r, rtt_next);
- if (r->rtt_queue->rtq_count > 0)
- r->rtt_queue->rtq_count--;
- else
- printf("rt_timer_remove_all: rtq_count reached 0\n");
+ TAILQ_INSERT_TAIL(, r, rtt_next);
+ KASSERT(r->rtt_queue->rtq_count > 0);
+ r->rtt_queue->rtq_count--;
+ }
+ mtx_leave(_mtx);
+
+ while ((r = TAILQ_FIRST()) != NULL) {
+ TAILQ_REMOVE(, r, rtt_next);
pool_put(_pool, r);
}
}
@@ -1467,12 +1487,23 @@ int
rt_timer_add(struct rtentry *rt, void (*func)(struct rtentry *,
struct rttimer *), struct rttimer_queue *queue, u_int rtableid)
{
- struct rttimer *r;
+ struct rttimer *r, *rnew;
time_t current_time;
+ rnew = pool_get(_pool, PR_NOWAIT | PR_ZERO);
+ if (rnew == NULL)
+ return (ENOBUFS);
+
current_time = getuptime();
- rt->rt_expire = current_time + queue->rtq_timeout;
+ rnew->rtt_rt = rt;
+ rnew->rtt_time = current_time;
+ rnew->rtt_func = func;
+ rnew->rtt_queue = queue;
+ rnew->rtt_tableid = rtableid;
+
+ mtx_enter(_mtx);
+ rt->rt_expire = current_time + queue->rtq_timeout;
/*
* If there's already a timer with this action, destroy it
Re: [External] : parallel IP forwarding
On Fri, Apr 08, 2022 at 12:56:12PM +0200, Alexander Bluhm wrote: > Hi, > > I now the right time to commit the parallel forwarding diff? > > Known limitiations are: > - Hrvoje has seen a crash with both pfsync and ipsec on his production > machine. But he cannot reproduce it in his lab. > - TCP processing gets slower as we have an additional queue between > IP and protocol layer. > - Protocol layer may starve as 1 exclusive lock is fightig with 4 > shared locks. This happens only when forwardig a lot. > > The advantage of commiting is that we see how relevant these things > are in real world. But the most important thing is that we learn > how all the locks behave under MP pressure. You can add a lot of > locking, but only when you run in parallel, you see if it is correct. > > An alternative could be to commit it with NET_TASKQ 1. With only > one softnet thread I would expect to see less bugs, but there is > also less to learn. NET_TASKQ 1 could be a safe point where we > could easily switch back. > > bluhm > I think it's worth to give it a try. May be I would just split it to three commits/diffs: diff which adds task queues for forwarding diff which adds a KERNEL_LOCK to deal with arp diff which deals with local delivery to socket it's up to you. all changes look good to me. You have my OK, in case you want to proceed in one big step. OK sashan
Re: router timer kernel lock
On Thu, Apr 21, 2022 at 03:25:03PM +0200, Alexander Bluhm wrote:
> Hi,
>
> As claudio@ wants to refactor router timer before making them MP
> safe, I would like to protect them with kernel lock. It should fix
> this panic.
>
> https://marc.info/?l=openbsd-tech=164038527425440=2
>
> I hope this is the final step before running IP forwarding in
> parallel.
>
> ok?
I prefer the other diff, I don't like that other diff but this one is worse.
> bluhm
>
> Index: netinet/ip_icmp.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.188
> diff -u -p -r1.188 ip_icmp.c
> --- netinet/ip_icmp.c 20 Apr 2022 09:38:26 - 1.188
> +++ netinet/ip_icmp.c 21 Apr 2022 12:45:40 -
> @@ -634,8 +634,10 @@ reflect:
> rtredirect(sintosa(), sintosa(),
> sintosa(), , m->m_pkthdr.ph_rtableid);
> if (newrt != NULL && icmp_redirtimeout > 0) {
> + KERNEL_LOCK();
> rt_timer_add(newrt, icmp_redirect_timeout,
> icmp_redirect_timeout_q, m->m_pkthdr.ph_rtableid);
> + KERNEL_UNLOCK();
> }
> rtfree(newrt);
> pfctlinput(PRC_REDIRECT_HOST, sintosa());
> @@ -884,8 +886,10 @@ icmp_sysctl(int *name, u_int namelen, vo
> NET_LOCK();
> error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> _redirtimeout, 0, INT_MAX);
> + KERNEL_LOCK();
> rt_timer_queue_change(icmp_redirect_timeout_q,
> icmp_redirtimeout);
> + KERNEL_UNLOCK();
> NET_UNLOCK();
> break;
>
> @@ -975,8 +979,10 @@ icmp_mtudisc_clone(struct in_addr dst, u
> rt = nrt;
> rtm_send(rt, RTM_ADD, 0, rtableid);
> }
> + KERNEL_LOCK();
> error = rt_timer_add(rt, icmp_mtudisc_timeout, ip_mtudisc_timeout_q,
> rtableid);
> + KERNEL_UNLOCK();
> if (error)
> goto bad;
>
> Index: netinet/ip_input.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.367
> diff -u -p -r1.367 ip_input.c
> --- netinet/ip_input.c20 Apr 2022 09:38:26 - 1.367
> +++ netinet/ip_input.c21 Apr 2022 13:00:33 -
> @@ -1616,9 +1616,11 @@ ip_sysctl(int *name, u_int namelen, void
> NET_LOCK();
> error = sysctl_int(oldp, oldlenp, newp, newlen, _mtudisc);
> if (ip_mtudisc == 0) {
> + KERNEL_LOCK();
> rt_timer_queue_destroy(ip_mtudisc_timeout_q);
> ip_mtudisc_timeout_q =
> rt_timer_queue_create(ip_mtudisc_timeout);
> + KERNEL_UNLOCK();
> }
> NET_UNLOCK();
> return error;
> @@ -1626,8 +1628,10 @@ ip_sysctl(int *name, u_int namelen, void
> NET_LOCK();
> error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> _mtudisc_timeout, 0, INT_MAX);
> + KERNEL_LOCK();
> rt_timer_queue_change(ip_mtudisc_timeout_q,
> ip_mtudisc_timeout);
> + KERNEL_UNLOCK();
> NET_UNLOCK();
> return (error);
> #ifdef IPSEC
> Index: netinet/ip_mroute.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 ip_mroute.c
> --- netinet/ip_mroute.c 15 Dec 2021 17:21:08 - 1.131
> +++ netinet/ip_mroute.c 21 Apr 2022 13:02:43 -
> @@ -520,7 +520,9 @@ ip_mrouter_init(struct socket *so, struc
> return (EADDRINUSE);
>
> ip_mrouter[rtableid] = so;
> + KERNEL_LOCK();
> mrouterq[rtableid] = rt_timer_queue_create(MCAST_EXPIRE_FREQUENCY);
> + KERNEL_UNLOCK();
>
> return (0);
> }
> @@ -572,7 +574,9 @@ ip_mrouter_done(struct socket *so)
>
> mrt_api_config = 0;
>
> + KERNEL_LOCK();
> rt_timer_queue_destroy(mrouterq[rtableid]);
> + KERNEL_UNLOCK();
> mrouterq[rtableid] = NULL;
> ip_mrouter[rtableid] = NULL;
> mrt_count[rtableid] = 0;
> @@ -799,8 +803,10 @@ mfc_expire_route(struct rtentry *rt, str
> /* Not expired, add it back to the queue. */
> if (mfc->mfc_expire == 0) {
> mfc->mfc_expire = 1;
> + KERNEL_LOCK();
> rt_timer_add(rt, mfc_expire_route, mrouterq[rtableid],
> rtableid);
> + KERNEL_UNLOCK();
> return;
> }
>
> @@ -834,8 +840,10 @@ mfc_add_route(struct ifnet *ifp, struct
>
> rt->rt_llinfo = (caddr_t)mfc;
>
> + KERNEL_LOCK();
> rt_timer_add(rt, mfc_expire_route,
router timer kernel lock
Hi,
As claudio@ wants to refactor router timer before making them MP
safe, I would like to protect them with kernel lock. It should fix
this panic.
https://marc.info/?l=openbsd-tech=164038527425440=2
I hope this is the final step before running IP forwarding in
parallel.
ok?
bluhm
Index: netinet/ip_icmp.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.188
diff -u -p -r1.188 ip_icmp.c
--- netinet/ip_icmp.c 20 Apr 2022 09:38:26 - 1.188
+++ netinet/ip_icmp.c 21 Apr 2022 12:45:40 -
@@ -634,8 +634,10 @@ reflect:
rtredirect(sintosa(), sintosa(),
sintosa(), , m->m_pkthdr.ph_rtableid);
if (newrt != NULL && icmp_redirtimeout > 0) {
+ KERNEL_LOCK();
rt_timer_add(newrt, icmp_redirect_timeout,
icmp_redirect_timeout_q, m->m_pkthdr.ph_rtableid);
+ KERNEL_UNLOCK();
}
rtfree(newrt);
pfctlinput(PRC_REDIRECT_HOST, sintosa());
@@ -884,8 +886,10 @@ icmp_sysctl(int *name, u_int namelen, vo
NET_LOCK();
error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
_redirtimeout, 0, INT_MAX);
+ KERNEL_LOCK();
rt_timer_queue_change(icmp_redirect_timeout_q,
icmp_redirtimeout);
+ KERNEL_UNLOCK();
NET_UNLOCK();
break;
@@ -975,8 +979,10 @@ icmp_mtudisc_clone(struct in_addr dst, u
rt = nrt;
rtm_send(rt, RTM_ADD, 0, rtableid);
}
+ KERNEL_LOCK();
error = rt_timer_add(rt, icmp_mtudisc_timeout, ip_mtudisc_timeout_q,
rtableid);
+ KERNEL_UNLOCK();
if (error)
goto bad;
Index: netinet/ip_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.367
diff -u -p -r1.367 ip_input.c
--- netinet/ip_input.c 20 Apr 2022 09:38:26 - 1.367
+++ netinet/ip_input.c 21 Apr 2022 13:00:33 -
@@ -1616,9 +1616,11 @@ ip_sysctl(int *name, u_int namelen, void
NET_LOCK();
error = sysctl_int(oldp, oldlenp, newp, newlen, _mtudisc);
if (ip_mtudisc == 0) {
+ KERNEL_LOCK();
rt_timer_queue_destroy(ip_mtudisc_timeout_q);
ip_mtudisc_timeout_q =
rt_timer_queue_create(ip_mtudisc_timeout);
+ KERNEL_UNLOCK();
}
NET_UNLOCK();
return error;
@@ -1626,8 +1628,10 @@ ip_sysctl(int *name, u_int namelen, void
NET_LOCK();
error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
_mtudisc_timeout, 0, INT_MAX);
+ KERNEL_LOCK();
rt_timer_queue_change(ip_mtudisc_timeout_q,
ip_mtudisc_timeout);
+ KERNEL_UNLOCK();
NET_UNLOCK();
return (error);
#ifdef IPSEC
Index: netinet/ip_mroute.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_mroute.c,v
retrieving revision 1.131
diff -u -p -r1.131 ip_mroute.c
--- netinet/ip_mroute.c 15 Dec 2021 17:21:08 - 1.131
+++ netinet/ip_mroute.c 21 Apr 2022 13:02:43 -
@@ -520,7 +520,9 @@ ip_mrouter_init(struct socket *so, struc
return (EADDRINUSE);
ip_mrouter[rtableid] = so;
+ KERNEL_LOCK();
mrouterq[rtableid] = rt_timer_queue_create(MCAST_EXPIRE_FREQUENCY);
+ KERNEL_UNLOCK();
return (0);
}
@@ -572,7 +574,9 @@ ip_mrouter_done(struct socket *so)
mrt_api_config = 0;
+ KERNEL_LOCK();
rt_timer_queue_destroy(mrouterq[rtableid]);
+ KERNEL_UNLOCK();
mrouterq[rtableid] = NULL;
ip_mrouter[rtableid] = NULL;
mrt_count[rtableid] = 0;
@@ -799,8 +803,10 @@ mfc_expire_route(struct rtentry *rt, str
/* Not expired, add it back to the queue. */
if (mfc->mfc_expire == 0) {
mfc->mfc_expire = 1;
+ KERNEL_LOCK();
rt_timer_add(rt, mfc_expire_route, mrouterq[rtableid],
rtableid);
+ KERNEL_UNLOCK();
return;
}
@@ -834,8 +840,10 @@ mfc_add_route(struct ifnet *ifp, struct
rt->rt_llinfo = (caddr_t)mfc;
+ KERNEL_LOCK();
rt_timer_add(rt, mfc_expire_route, mrouterq[rtableid],
rtableid);
+ KERNEL_UNLOCK();
mfc->mfc_parent = mfccp->mfcc_parent;
mfc->mfc_pkt_cnt = 0;
@@ -1342,7 +1350,9 @@ mrt_mcast_del(struct rtentry *rt, unsign
int error;
/* Remove all
Re: [External] : Re: pfsync(4) snapshot lists must have dedicated link element
Hello, On Thu, Apr 21, 2022 at 02:42:45PM +0200, Alexander Bluhm wrote: > On Wed, Apr 20, 2022 at 11:22:27PM +0200, Alexandr Nedvedicky wrote: > > updated diff is below > > OK bluhm@ > > You have to merge again, as I removed #ifdef PFSYNC_DEBUG and added > a #ifdef DIAGNOSTIC. Sorry. > thanks for reminder. will merge and commit. regards sashan
Re: [External] : Re: pfsync(4) snapshot lists must have dedicated link element
On Wed, Apr 20, 2022 at 11:22:27PM +0200, Alexandr Nedvedicky wrote:
> updated diff is below
OK bluhm@
You have to merge again, as I removed #ifdef PFSYNC_DEBUG and added
a #ifdef DIAGNOSTIC. Sorry.
> 8<---8<---8<--8<
> diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
> index fc6843b541f..3061318cec9 100644
> --- a/sys/net/if_pfsync.c
> +++ b/sys/net/if_pfsync.c
> @@ -181,6 +181,7 @@ void pfsync_q_del(struct pf_state *);
>
> struct pfsync_upd_req_item {
> TAILQ_ENTRY(pfsync_upd_req_item)ur_entry;
> + TAILQ_ENTRY(pfsync_upd_req_item)ur_snap;
> struct pfsync_upd_req ur_msg;
> };
> TAILQ_HEAD(pfsync_upd_reqs, pfsync_upd_req_item);
> @@ -295,7 +296,7 @@ void pfsync_bulk_update(void *);
> void pfsync_bulk_fail(void *);
>
> void pfsync_grab_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
> -void pfsync_drop_snapshot(struct pfsync_snapshot *, struct pfsync_softc *);
> +void pfsync_drop_snapshot(struct pfsync_snapshot *);
>
> void pfsync_send_dispatch(void *);
> void pfsync_send_pkt(struct mbuf *);
> @@ -422,8 +423,7 @@ pfsync_clone_destroy(struct ifnet *ifp)
> sc->sc_deferred = 0;
> mtx_leave(>sc_deferrals_mtx);
>
> - while (!TAILQ_EMPTY()) {
> - pd = TAILQ_FIRST();
> + while ((pd = TAILQ_FIRST()) != NULL) {
> TAILQ_REMOVE(, pd, pd_entry);
> pfsync_undefer(pd, 0);
> }
> @@ -1574,6 +1574,9 @@ void
> pfsync_grab_snapshot(struct pfsync_snapshot *sn, struct pfsync_softc *sc)
> {
> int q;
> + struct pf_state *st;
> + struct pfsync_upd_req_item *ur;
> + struct tdb *tdb;
>
> sn->sn_sc = sc;
>
> @@ -1583,14 +1586,31 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn,
> struct pfsync_softc *sc)
>
> for (q = 0; q < PFSYNC_S_COUNT; q++) {
> TAILQ_INIT(>sn_qs[q]);
> - TAILQ_CONCAT(>sn_qs[q], >sc_qs[q], sync_list);
> +
> + while ((st = TAILQ_FIRST(>sc_qs[q])) != NULL) {
> + KASSERT(st->snapped == 0);
> + TAILQ_REMOVE(>sc_qs[q], st, sync_list);
> + TAILQ_INSERT_TAIL(>sn_qs[q], st, sync_snap);
> + st->snapped = 1;
> + }
> }
>
> TAILQ_INIT(>sn_upd_req_list);
> - TAILQ_CONCAT(>sn_upd_req_list, >sc_upd_req_list, ur_entry);
> + while ((ur = TAILQ_FIRST(>sc_upd_req_list)) != NULL) {
> + TAILQ_REMOVE(>sc_upd_req_list, ur, ur_entry);
> + TAILQ_INSERT_TAIL(>sn_upd_req_list, ur, ur_snap);
> + }
>
> TAILQ_INIT(>sn_tdb_q);
> - TAILQ_CONCAT(>sn_tdb_q, >sc_tdb_q, tdb_sync_entry);
> + while ((tdb = TAILQ_FIRST(>sc_tdb_q)) != NULL) {
> + TAILQ_REMOVE(>sc_tdb_q, tdb, tdb_sync_entry);
> + TAILQ_INSERT_TAIL(>sn_tdb_q, tdb, tdb_sync_snap);
> +
> + mtx_enter(>tdb_mtx);
> + KASSERT(!ISSET(tdb->tdb_flags, TDBF_PFSYNC_SNAPPED));
> + SET(tdb->tdb_flags, TDBF_PFSYNC_SNAPPED);
> + mtx_leave(>tdb_mtx);
> + }
>
> sn->sn_len = sc->sc_len;
> sc->sc_len = PFSYNC_MINPKT;
> @@ -1606,41 +1626,40 @@ pfsync_grab_snapshot(struct pfsync_snapshot *sn,
> struct pfsync_softc *sc)
> }
>
> void
> -pfsync_drop_snapshot(struct pfsync_snapshot *sn, struct pfsync_softc * sc)
> +pfsync_drop_snapshot(struct pfsync_snapshot *sn)
> {
> struct pf_state *st;
> struct pfsync_upd_req_item *ur;
> struct tdb *t;
> int q;
>
> -
> for (q = 0; q < PFSYNC_S_COUNT; q++) {
> if (TAILQ_EMPTY(>sn_qs[q]))
> continue;
>
> while ((st = TAILQ_FIRST(>sn_qs[q])) != NULL) {
> - TAILQ_REMOVE(>sn_qs[q], st, sync_list);
> -#ifdef PFSYNC_DEBUG
> KASSERT(st->sync_state == q);
> -#endif
> + KASSERT(st->snapped == 1);
> + TAILQ_REMOVE(>sn_qs[q], st, sync_snap);
> st->sync_state = PFSYNC_S_NONE;
> + st->snapped = 0;
> pf_state_unref(st);
> }
> }
>
> while ((ur = TAILQ_FIRST(>sn_upd_req_list)) != NULL) {
> - TAILQ_REMOVE(>sn_upd_req_list, ur, ur_entry);
> + TAILQ_REMOVE(>sn_upd_req_list, ur, ur_snap);
> pool_put(>sn_sc->sc_pool, ur);
> }
>
> - mtx_enter(>sc_tdb_mtx);
> while ((t = TAILQ_FIRST(>sn_tdb_q)) != NULL) {
> - TAILQ_REMOVE(>sn_tdb_q, t, tdb_sync_entry);
> + TAILQ_REMOVE(>sn_tdb_q, t, tdb_sync_snap);
> mtx_enter(>tdb_mtx);
> + KASSERT(ISSET(t->tdb_flags, TDBF_PFSYNC_SNAPPED));
> + CLR(t->tdb_flags, TDBF_PFSYNC_SNAPPED);
> CLR(t->tdb_flags, TDBF_PFSYNC);
> mtx_leave(>tdb_mtx);
> }
> -
Re: rpki-client more refactoring
On Thu, Apr 21, 2022 at 02:15:14PM +0200, Claudio Jeker wrote:
> On Thu, Apr 21, 2022 at 02:08:01PM +0200, Theo Buehler wrote:
> > On Thu, Apr 21, 2022 at 01:14:31PM +0200, Claudio Jeker wrote:
> > > So here is the cleanup of filemode.c and also a bit of cleanup in parse.c
> > > This should also fix a few bugs in parse_load_certchain() (mainly
> > > memleaks).
> >
> > A couple of suggestions for parse_load_certchain() below.
> >
>
> Always good to have extra eyes on such changes. Here updated diff with
> your suggested changes.
Sure.
> /* TA found play back the stack and add all certs */
> - for (failed = 0; i >= 0; i--) {
> + for (; i >= 0; i--) {
> cert = stack[i];
> uri = filestack[i];
>
> - if (failed)
> - cert_free(cert);
> - else if (proc_parser_cert_validate(uri, cert) == NULL)
> - failed = 1;
> + crl = crl_get(, a);
> + if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> + !valid_cert(uri, a, cert))
> + goto fail;
> + cert->talid = a->cert->talid;
> + a = auth_insert(, cert, a);
> + stack[i] = NULL;
> }
I'd add a
return;
Then it's ok.
> +
> +fail:
> + for (i = 0; i < MAX_CERT_DEPTH; i++)
> + cert_free(stack[i]);
> }
Re: rpki-client more refactoring
On Thu, Apr 21, 2022 at 02:08:01PM +0200, Theo Buehler wrote:
> On Thu, Apr 21, 2022 at 01:14:31PM +0200, Claudio Jeker wrote:
> > So here is the cleanup of filemode.c and also a bit of cleanup in parse.c
> > This should also fix a few bugs in parse_load_certchain() (mainly
> > memleaks).
>
> A couple of suggestions for parse_load_certchain() below.
>
Always good to have extra eyes on such changes. Here updated diff with
your suggested changes.
--
:wq Claudio
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.70
diff -u -p -r1.70 cert.c
--- cert.c 21 Apr 2022 09:53:07 - 1.70
+++ cert.c 21 Apr 2022 10:45:17 -
@@ -999,6 +999,9 @@ out:
struct cert *
cert_parse(const char *fn, struct cert *p)
{
+ if (p == NULL)
+ return NULL;
+
if (p->aki == NULL) {
warnx("%s: RFC 6487 section 8.4.2: "
"non-trust anchor missing AKI", fn);
@@ -1032,6 +1035,9 @@ ta_parse(const char *fn, struct cert *p,
ASN1_TIME *notBefore, *notAfter;
EVP_PKEY*pk, *opk;
+ if (p == NULL)
+ return NULL;
+
/* first check pubkey against the one from the TAL */
pk = d2i_PUBKEY(NULL, , pkeysz);
if (pk == NULL) {
@@ -1207,7 +1213,7 @@ auth_find(struct auth_tree *auths, const
return RB_FIND(auth_tree, auths, );
}
-void
+struct auth *
auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *parent)
{
struct auth *na;
@@ -1221,6 +1227,8 @@ auth_insert(struct auth_tree *auths, str
if (RB_INSERT(auth_tree, auths, na) != NULL)
err(1, "auth tree corrupted");
+
+ return na;
}
static void
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.131
diff -u -p -r1.131 extern.h
--- extern.h21 Apr 2022 09:53:07 - 1.131
+++ extern.h21 Apr 2022 10:46:09 -
@@ -308,7 +308,7 @@ struct auth {
RB_HEAD(auth_tree, auth);
struct auth*auth_find(struct auth_tree *, const char *);
-voidauth_insert(struct auth_tree *, struct cert *, struct auth *);
+struct auth*auth_insert(struct auth_tree *, struct cert *, struct auth *);
enum http_result {
HTTP_FAILED,/* anything else */
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.1
diff -u -p -r1.1 filemode.c
--- filemode.c 21 Apr 2022 09:53:07 - 1.1
+++ filemode.c 21 Apr 2022 12:12:06 -
@@ -46,80 +46,6 @@ static struct crl_treecrlt = RB_INITIA
struct tal *talobj[TALSZ_MAX];
/*
- * Validate a certificate, if invalid free the resouces and return NULL.
- */
-static struct cert *
-proc_parser_cert_validate(char *file, struct cert *cert)
-{
- struct auth *a;
- struct crl *crl;
-
- a = valid_ski_aki(file, , cert->ski, cert->aki);
- crl = crl_get(, a);
-
- if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
- cert_free(cert);
- return NULL;
- }
-
- cert->talid = a->cert->talid;
-
- /* Validate the cert */
- if (!valid_cert(file, a, cert)) {
- cert_free(cert);
- return NULL;
- }
-
- /*
-* Add validated CA certs to the RPKI auth tree.
-*/
- if (cert->purpose == CERT_PURPOSE_CA)
- auth_insert(, cert, a);
-
- return cert;
-}
-
-/*
- * Root certificates come from TALs (has a pkey and is self-signed).
- * Parse the certificate, ensure that its public key matches the
- * known public key from the TAL, and then validate the RPKI
- * content.
- *
- * This returns a certificate (which must not be freed) or NULL on
- * parse failure.
- */
-static struct cert *
-proc_parser_root_cert(char *file, const unsigned char *der, size_t len,
-unsigned char *pkey, size_t pkeysz, int talid)
-{
- struct cert *cert;
-
- /* Extract certificate data. */
-
- cert = cert_parse_pre(file, der, len);
- if (cert == NULL)
- return NULL;
- cert = ta_parse(file, cert, pkey, pkeysz);
- if (cert == NULL)
- return NULL;
-
- if (!valid_ta(file, , cert)) {
- warnx("%s: certificate not a valid ta", file);
- cert_free(cert);
- return NULL;
- }
-
- cert->talid = talid;
-
- /*
-* Add valid roots to the RPKI auth tree.
-*/
- auth_insert(, cert, NULL);
-
- return cert;
-}
-
-/*
* Use the X509 CRL Distribution Points to locate the CRL needed for
* verification.
*/
@@ -207,25 +133,25 @@ parse_load_cert(char *uri)
static void
parse_load_certchain(char *uri)
{
- struct cert
Re: rpki-client more refactoring
On Thu, Apr 21, 2022 at 01:14:31PM +0200, Claudio Jeker wrote:
> So here is the cleanup of filemode.c and also a bit of cleanup in parse.c
> This should also fix a few bugs in parse_load_certchain() (mainly
> memleaks).
A couple of suggestions for parse_load_certchain() below.
ok
>
> --
> :wq Claudio
>
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 cert.c
> --- cert.c21 Apr 2022 09:53:07 - 1.70
> +++ cert.c21 Apr 2022 10:45:17 -
> @@ -999,6 +999,9 @@ out:
> struct cert *
> cert_parse(const char *fn, struct cert *p)
> {
> + if (p == NULL)
> + return NULL;
> +
> if (p->aki == NULL) {
> warnx("%s: RFC 6487 section 8.4.2: "
> "non-trust anchor missing AKI", fn);
> @@ -1032,6 +1035,9 @@ ta_parse(const char *fn, struct cert *p,
> ASN1_TIME *notBefore, *notAfter;
> EVP_PKEY*pk, *opk;
>
> + if (p == NULL)
> + return NULL;
> +
> /* first check pubkey against the one from the TAL */
> pk = d2i_PUBKEY(NULL, , pkeysz);
> if (pk == NULL) {
> @@ -1207,7 +1213,7 @@ auth_find(struct auth_tree *auths, const
> return RB_FIND(auth_tree, auths, );
> }
>
> -void
> +struct auth *
> auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *parent)
> {
> struct auth *na;
> @@ -1221,6 +1227,8 @@ auth_insert(struct auth_tree *auths, str
>
> if (RB_INSERT(auth_tree, auths, na) != NULL)
> err(1, "auth tree corrupted");
> +
> + return na;
> }
>
> static void
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.131
> diff -u -p -r1.131 extern.h
> --- extern.h 21 Apr 2022 09:53:07 - 1.131
> +++ extern.h 21 Apr 2022 10:46:09 -
> @@ -308,7 +308,7 @@ struct auth {
> RB_HEAD(auth_tree, auth);
>
> struct auth *auth_find(struct auth_tree *, const char *);
> -void auth_insert(struct auth_tree *, struct cert *, struct auth *);
> +struct auth *auth_insert(struct auth_tree *, struct cert *, struct auth *);
>
> enum http_result {
> HTTP_FAILED,/* anything else */
> Index: filemode.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 filemode.c
> --- filemode.c21 Apr 2022 09:53:07 - 1.1
> +++ filemode.c21 Apr 2022 10:47:24 -
> @@ -46,80 +46,6 @@ static struct crl_tree crlt = RB_INITIA
> struct tal *talobj[TALSZ_MAX];
>
> /*
> - * Validate a certificate, if invalid free the resouces and return NULL.
> - */
> -static struct cert *
> -proc_parser_cert_validate(char *file, struct cert *cert)
> -{
> - struct auth *a;
> - struct crl *crl;
> -
> - a = valid_ski_aki(file, , cert->ski, cert->aki);
> - crl = crl_get(, a);
> -
> - if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
> - cert_free(cert);
> - return NULL;
> - }
> -
> - cert->talid = a->cert->talid;
> -
> - /* Validate the cert */
> - if (!valid_cert(file, a, cert)) {
> - cert_free(cert);
> - return NULL;
> - }
> -
> - /*
> - * Add validated CA certs to the RPKI auth tree.
> - */
> - if (cert->purpose == CERT_PURPOSE_CA)
> - auth_insert(, cert, a);
> -
> - return cert;
> -}
> -
> -/*
> - * Root certificates come from TALs (has a pkey and is self-signed).
> - * Parse the certificate, ensure that its public key matches the
> - * known public key from the TAL, and then validate the RPKI
> - * content.
> - *
> - * This returns a certificate (which must not be freed) or NULL on
> - * parse failure.
> - */
> -static struct cert *
> -proc_parser_root_cert(char *file, const unsigned char *der, size_t len,
> -unsigned char *pkey, size_t pkeysz, int talid)
> -{
> - struct cert *cert;
> -
> - /* Extract certificate data. */
> -
> - cert = cert_parse_pre(file, der, len);
> - if (cert == NULL)
> - return NULL;
> - cert = ta_parse(file, cert, pkey, pkeysz);
> - if (cert == NULL)
> - return NULL;
> -
> - if (!valid_ta(file, , cert)) {
> - warnx("%s: certificate not a valid ta", file);
> - cert_free(cert);
> - return NULL;
> - }
> -
> - cert->talid = talid;
> -
> - /*
> - * Add valid roots to the RPKI auth tree.
> - */
> - auth_insert(, cert, NULL);
> -
> - return cert;
> -}
> -
> -/*
> * Use the X509 CRL Distribution Points to locate the CRL needed for
> * verification.
> */
> @@ -207,25 +133,25 @@ parse_load_cert(char *uri)
> static void
> parse_load_certchain(char *uri)
> {
> - struct
rpki-client more refactoring
So here is the cleanup of filemode.c and also a bit of cleanup in parse.c
This should also fix a few bugs in parse_load_certchain() (mainly
memleaks).
--
:wq Claudio
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.70
diff -u -p -r1.70 cert.c
--- cert.c 21 Apr 2022 09:53:07 - 1.70
+++ cert.c 21 Apr 2022 10:45:17 -
@@ -999,6 +999,9 @@ out:
struct cert *
cert_parse(const char *fn, struct cert *p)
{
+ if (p == NULL)
+ return NULL;
+
if (p->aki == NULL) {
warnx("%s: RFC 6487 section 8.4.2: "
"non-trust anchor missing AKI", fn);
@@ -1032,6 +1035,9 @@ ta_parse(const char *fn, struct cert *p,
ASN1_TIME *notBefore, *notAfter;
EVP_PKEY*pk, *opk;
+ if (p == NULL)
+ return NULL;
+
/* first check pubkey against the one from the TAL */
pk = d2i_PUBKEY(NULL, , pkeysz);
if (pk == NULL) {
@@ -1207,7 +1213,7 @@ auth_find(struct auth_tree *auths, const
return RB_FIND(auth_tree, auths, );
}
-void
+struct auth *
auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *parent)
{
struct auth *na;
@@ -1221,6 +1227,8 @@ auth_insert(struct auth_tree *auths, str
if (RB_INSERT(auth_tree, auths, na) != NULL)
err(1, "auth tree corrupted");
+
+ return na;
}
static void
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.131
diff -u -p -r1.131 extern.h
--- extern.h21 Apr 2022 09:53:07 - 1.131
+++ extern.h21 Apr 2022 10:46:09 -
@@ -308,7 +308,7 @@ struct auth {
RB_HEAD(auth_tree, auth);
struct auth*auth_find(struct auth_tree *, const char *);
-voidauth_insert(struct auth_tree *, struct cert *, struct auth *);
+struct auth*auth_insert(struct auth_tree *, struct cert *, struct auth *);
enum http_result {
HTTP_FAILED,/* anything else */
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.1
diff -u -p -r1.1 filemode.c
--- filemode.c 21 Apr 2022 09:53:07 - 1.1
+++ filemode.c 21 Apr 2022 10:47:24 -
@@ -46,80 +46,6 @@ static struct crl_treecrlt = RB_INITIA
struct tal *talobj[TALSZ_MAX];
/*
- * Validate a certificate, if invalid free the resouces and return NULL.
- */
-static struct cert *
-proc_parser_cert_validate(char *file, struct cert *cert)
-{
- struct auth *a;
- struct crl *crl;
-
- a = valid_ski_aki(file, , cert->ski, cert->aki);
- crl = crl_get(, a);
-
- if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
- cert_free(cert);
- return NULL;
- }
-
- cert->talid = a->cert->talid;
-
- /* Validate the cert */
- if (!valid_cert(file, a, cert)) {
- cert_free(cert);
- return NULL;
- }
-
- /*
-* Add validated CA certs to the RPKI auth tree.
-*/
- if (cert->purpose == CERT_PURPOSE_CA)
- auth_insert(, cert, a);
-
- return cert;
-}
-
-/*
- * Root certificates come from TALs (has a pkey and is self-signed).
- * Parse the certificate, ensure that its public key matches the
- * known public key from the TAL, and then validate the RPKI
- * content.
- *
- * This returns a certificate (which must not be freed) or NULL on
- * parse failure.
- */
-static struct cert *
-proc_parser_root_cert(char *file, const unsigned char *der, size_t len,
-unsigned char *pkey, size_t pkeysz, int talid)
-{
- struct cert *cert;
-
- /* Extract certificate data. */
-
- cert = cert_parse_pre(file, der, len);
- if (cert == NULL)
- return NULL;
- cert = ta_parse(file, cert, pkey, pkeysz);
- if (cert == NULL)
- return NULL;
-
- if (!valid_ta(file, , cert)) {
- warnx("%s: certificate not a valid ta", file);
- cert_free(cert);
- return NULL;
- }
-
- cert->talid = talid;
-
- /*
-* Add valid roots to the RPKI auth tree.
-*/
- auth_insert(, cert, NULL);
-
- return cert;
-}
-
-/*
* Use the X509 CRL Distribution Points to locate the CRL needed for
* verification.
*/
@@ -207,25 +133,25 @@ parse_load_cert(char *uri)
static void
parse_load_certchain(char *uri)
{
- struct cert *stack[MAX_CERT_DEPTH];
+ struct cert *stack[MAX_CERT_DEPTH] = { 0 };
char *filestack[MAX_CERT_DEPTH];
struct cert *cert;
- int i, failed;
+ struct crl *crl;
+ struct auth *a;
+ int i;
for (i = 0; i < MAX_CERT_DEPTH; i++) {
- cert =
Re: router timer mutex
On Wed, Apr 20, 2022 at 08:12:51PM +0200, Alexander Bluhm wrote:
> Hi,
>
> mvs@ reminded me of a crash I have seen in December. Route timers
> are not MP safe, but I think this can be fixed with a mutex. The
> idea is to protect the global lists with a mutex and move the rttimer
> into a temporary list. Then the callback and pool put can be called
> later without mutex.
>
> It survived a full regress with witness.
>
> Hrvoje: Can you put this on your test machine together with parallel
> IP forwarding?
>
> ok?
I would have prefer if the code was refactored first. The removal of a
rt_timer is copied over in 4 or 5 places with almost no difference.
In the rt_timer_queue_destroy() case you can use TAILQ_CONCAT and kill one
loop. Also I would use TAILQ_HEAD_INITIALIZER() instead of TAILQ_INIT().
To be honest most of this code should be directly replaced with the
timeout API. I bet the timeout API is more efficent.
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.406
> diff -u -p -r1.406 route.c
> --- net/route.c 20 Apr 2022 17:58:22 - 1.406
> +++ net/route.c 20 Apr 2022 18:00:39 -
> @@ -1361,7 +1361,8 @@ rt_ifa_purge_walker(struct rtentry *rt,
> * for multiple queues for efficiency's sake...
> */
>
> -LIST_HEAD(, rttimer_queue) rttimer_queue_head;
> +struct mutex rttimer_mtx;
> +LIST_HEAD(, rttimer_queue) rttimer_queue_head; /* [t] */
>
> #define RTTIMER_CALLOUT(r) { \
> if (r->rtt_func != NULL) { \
> @@ -1393,6 +1394,7 @@ rt_timer_init(void)
> pool_init(_queue_pool, sizeof(struct rttimer_queue), 0,
> IPL_MPFLOOR, 0, "rttmrq", NULL);
>
> + mtx_init(_mtx, IPL_MPFLOOR);
> LIST_INIT(_queue_head);
> timeout_set_proc(_timer_timeout, rt_timer_timer, _timer_timeout);
> timeout_add_sec(_timer_timeout, 1);
> @@ -1408,7 +1410,10 @@ rt_timer_queue_create(int timeout)
> rtq->rtq_timeout = timeout;
> rtq->rtq_count = 0;
> TAILQ_INIT(>rtq_head);
> +
> + mtx_enter(_mtx);
> LIST_INSERT_HEAD(_queue_head, rtq, rtq_link);
> + mtx_leave(_mtx);
>
> return (rtq);
> }
> @@ -1416,28 +1421,36 @@ rt_timer_queue_create(int timeout)
> void
> rt_timer_queue_change(struct rttimer_queue *rtq, int timeout)
> {
> + mtx_enter(_mtx);
> rtq->rtq_timeout = timeout;
> + mtx_leave(_mtx);
> }
>
> void
> rt_timer_queue_destroy(struct rttimer_queue *rtq)
> {
> - struct rttimer *r;
> + struct rttimer *r;
> + TAILQ_HEAD(, rttimer)rttlist;
>
> NET_ASSERT_LOCKED();
>
> + TAILQ_INIT();
> + mtx_enter(_mtx);
> while ((r = TAILQ_FIRST(>rtq_head)) != NULL) {
> LIST_REMOVE(r, rtt_link);
> TAILQ_REMOVE(>rtq_head, r, rtt_next);
> + TAILQ_INSERT_TAIL(, r, rtt_next);
> + KASSERT(rtq->rtq_count > 0);
> + rtq->rtq_count--;
> + }
> + LIST_REMOVE(rtq, rtq_link);
> + mtx_leave(_mtx);
> +
> + while ((r = TAILQ_FIRST()) != NULL) {
> + TAILQ_REMOVE(, r, rtt_next);
> RTTIMER_CALLOUT(r);
> pool_put(_pool, r);
> - if (rtq->rtq_count > 0)
> - rtq->rtq_count--;
> - else
> - printf("rt_timer_queue_destroy: rtq_count reached 0\n");
> }
> -
> - LIST_REMOVE(rtq, rtq_link);
> pool_put(_queue_pool, rtq);
> }
>
> @@ -1450,15 +1463,22 @@ rt_timer_queue_count(struct rttimer_queu
> void
> rt_timer_remove_all(struct rtentry *rt)
> {
> - struct rttimer *r;
> + struct rttimer *r;
> + TAILQ_HEAD(, rttimer)rttlist;
>
> + TAILQ_INIT();
> + mtx_enter(_mtx);
> while ((r = LIST_FIRST(>rt_timer)) != NULL) {
> LIST_REMOVE(r, rtt_link);
> TAILQ_REMOVE(>rtt_queue->rtq_head, r, rtt_next);
> - if (r->rtt_queue->rtq_count > 0)
> - r->rtt_queue->rtq_count--;
> - else
> - printf("rt_timer_remove_all: rtq_count reached 0\n");
> + TAILQ_INSERT_TAIL(, r, rtt_next);
> + KASSERT(r->rtt_queue->rtq_count > 0);
> + r->rtt_queue->rtq_count--;
> + }
> + mtx_leave(_mtx);
> +
> + while ((r = TAILQ_FIRST()) != NULL) {
> + TAILQ_REMOVE(, r, rtt_next);
> pool_put(_pool, r);
> }
> }
> @@ -1471,8 +1491,9 @@ rt_timer_add(struct rtentry *rt, void (*
> time_t current_time;
>
> current_time = getuptime();
> - rt->rt_expire = current_time + queue->rtq_timeout;
>
> + mtx_enter(_mtx);
> + rt->rt_expire = current_time + queue->rtq_timeout;
> /*
>* If there's already a timer with this action, destroy it before
>* we add a new one.
> @@ -1481,27 +1502,31
Re: [External] : Re: pfsync(4) snapshot lists must have dedicated link element
On 20.4.2022. 23:22, Alexandr Nedvedicky wrote: > Hello, > > On Wed, Apr 20, 2022 at 03:43:06PM +0200, Alexander Bluhm wrote: >> On Sat, Apr 09, 2022 at 01:51:05AM +0200, Alexandr Nedvedicky wrote: >>> updated diff is below. >> I am not sure what Hrvoje actually did test and what not. My >> impression was, that he got a panic with the previous version of >> this diff, but the machine was stable with the code in current. >> >> But maybe I got it wrong and we need this code to run pfsync with >> IPsec in parallel. > that's correct. Hrvoje was testing several diffs stacked > on each other: > diff which enables parallel forwarding > diff which fixes tunnel descriptor block handling (tdb) for ipsec > diff which fixes pfsync Yes, panics that we exchange privately was on production and I couldn't reproduce them in lab. In production I don't have ipsec, only simple pfsync setup. It seems to me that pfsync mpfloor diff solved panics that I had in production. At the same time I was running sasyncd setup with 5 site-to-site ipsec tunnels with same diffs and for now it seems stable ..
Re: router timer mutex
On 20.4.2022. 20:12, Alexander Bluhm wrote: > Hi, > > mvs@ reminded me of a crash I have seen in December. Route timers > are not MP safe, but I think this can be fixed with a mutex. The > idea is to protect the global lists with a mutex and move the rttimer > into a temporary list. Then the callback and pool put can be called > later without mutex. > > It survived a full regress with witness. > > Hrvoje: Can you put this on your test machine together with parallel > IP forwarding? > > ok? No problem, I'm running this and parallel forwarding diff in lab and production.
Re: xenodm and login_fbtab
> Date: Thu, 21 Apr 2022 11:49:45 +1000 > From: Jonathan Gray > > On Wed, Apr 20, 2022 at 05:17:58PM -0500, joshua stein wrote: > > xenodm supports login_fbtab(3) to chown devices but it currently > > doesn't do anything because /etc/fbtab does not list /dev/ttyC4. It > > uses the GiveConsole and TakeConsole scripts in /etc/X11/xenodm/ to > > do this manually, but the file lists are not complete. > > > > I would like to remove all of the custom chown/chmod calls in the > > GiveConsole and TakeConsole scripts and move this into /etc/fbtab by > > adding /dev/ttyC4 and all of the wskbd* and wsmouse* devices so that > > wsconsctl from within X11 works. (These wildcards require the > > just-committed changes to libutil.) > > > > The current fbtab lists many of these devices for /dev/ttyC0 which > > seems only relevant for running OpenGL applications from the console > > (is this even possible?) or running X11 as an unprivileged user > > which we don't support anymore. > > using startx still works with the modesetting xorg driver > when the pci bus does not need to be probed > > does your diff no longer change ownership of the devices for > startx? Right this proposal breaks startx. Maybe the installer could change /etc/fbtab when it enables xenodm? > > revision 1.7 > date: 2019/09/15 12:25:40; author: kettenis; state: Exp; lines: +1 -1; > commitid: zDNBYfUsKpdfByaf; > Add ttyC4 to lost of devices to change when logging in on ttyC0 (and in > some cases also the serial console) such that X can use it as its VT > when running without root privileges. > > ok jsg@, matthieu@ > > > > > > Is there any particular reason to keep these around for /dev/ttyC0? > > > > This has bit me before when I am logged into X11 as my normal user, > > I login to ttyC0 as root to check something which chowns all the > > devices to root, then later in X11 I notice no GL-using apps like > > Firefox work anymore because they can't open /dev/dri nodes. > > > > Once these file lists are ironed out, I will make diffs for all the > > other arches. > > > > > > diff --git etc/etc.amd64/fbtab etc/etc.amd64/fbtab > > index aec447931a7..df5a7a29cdd 100644 > > --- etc/etc.amd64/fbtab > > +++ etc/etc.amd64/fbtab > > @@ -1 +1,2 @@ > > -/dev/ttyC0 0600 > > /dev/console:/dev/wskbd:/dev/wskbd0:/dev/wsmouse:/dev/wsmouse0:/dev/ttyCcfg:/dev/ttyC4:/dev/dri/card0:/dev/dri/renderD128 > > +/dev/ttyC0 0600/dev/console:/dev/wskbd*:/dev/wsmouse*:/dev/ttyCcfg > > +/dev/ttyC4 0600 > > /dev/console:/dev/wskbd*:/dev/wsmouse*:/dev/ttyCcfg:/dev/ttyC4:/dev/dri/* > > > > > >
