On Wed, Jan 18, 2023 at 05:53:10PM +0100, Theo Buehler wrote:
> On Wed, Jan 18, 2023 at 05:37:37PM +0100, Claudio Jeker wrote:
> > On Wed, Jan 18, 2023 at 05:18:58PM +0100, Theo Buehler wrote:
> > > On Wed, Jan 18, 2023 at 02:46:19PM +0100, Claudio Jeker wrote:
> > > > This is the next step in vstate cleanup.
> > > > Since the vstate is now part of struct filterstate use that information
> > > > instead of passing an explicit vstate to the various update functions.
> > >
> > > It took me a moment to understand that rde_filterstate_clean() does not
> > > zero the vstate, so the changes in rde_update.c preserve behavior.
> > > That's not super intuitive to me, but if that's bound to stay this way,
> >
> > Uhm, now I'm a bit confused. Where is the vstate used after calling
> > rde_filterstate_clean()? I did not spot that issue in rde_update.c
> > I think we could zero everyting in the filterstate during clean. Just to
> > be sure.
>
> It is super confusing. For example in up_generate_addpath() it starts
> with:
>
> rde_filterstate_prep(&state, new);
>
> After this, state.vstate == prefix_roa_vstate(new).
>
> if (rde_filter(rules, peer, prefix_peer(new), &addr,
> prefixlen, &state) == ACTION_DENY) {
> rde_filterstate_clean(&state);
> continue;
> }
>
> if (up_enforce_open_policy(peer, &state)) {
> rde_filterstate_clean(&state);
> continue;
> }
>
> /* from here on we know this is an update */
> p = prefix_adjout_get(peer, new->path_id_tx, &addr, prefixlen);
>
> up_prep_adjout(peer, &state, addr.aid);
>
> None of the above code touches state.vstate.
>
> prefix_adjout_update(p, peer, &state, &addr,
> prefixlen, new->path_id_tx);
>
> and here you dropped the prefix_roa_vstate(new) argument that is now
> taken from state, which is correct.
Agreed.
>
> In up_generate_update() I think there's something wrong.
>
> Again, the vstate is copied from new at the start
>
> rde_filterstate_prep(&state, new);
>
> then new may or may not change over the next few lines, and the two
> rde_filterstate_clean(&state) before prefix_adjout_update() don't
> change state's vstate, and state.vstate may or may not match
> prefix_adjout_update(new).
But those calls have either a break or continue, so either the loop is
exited or restarted (depending on PEERFLAG_EVALUATE_ALL).
So there should be no way to go from a rde_filterstate_clean(&state) to
prefix_adjout_update(new).
There is a missing rde_filterstate_clean(&state) in up_generate_update().
if (up_enforce_open_policy(peer, &state)) {
rde_filterstate_clean(&state);
if (peer->flags & PEERFLAG_EVALUATE_ALL) {
new = TAILQ_NEXT(new, entry.list.rib);
if (new != NULL && prefix_eligible(new))
continue;
}
break;
}
/* check if this was actually a withdraw */
if (need_withdraw)
>>>>>>>> here the filterstate is leaked, which is bad.
break;
/* from here on we know this is an update */
up_prep_adjout(peer, &state, addr.aid);
prefix_adjout_update(p, peer, &state, &addr,
new->pt->prefixlen, new->path_id_tx);
rde_filterstate_clean(&state);
Below a diff that fixes the extra leak.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.588
diff -u -p -r1.588 rde.c
--- rde.c 18 Jan 2023 13:20:00 -0000 1.588
+++ rde.c 18 Jan 2023 17:20:10 -0000
@@ -1744,7 +1744,7 @@ rde_update_update(struct rde_peer *peer,
/* add original path to the Adj-RIB-In */
if (prefix_update(rib_byid(RIB_ADJ_IN), peer, path_id, path_id_tx,
- in, prefix, prefixlen, in->vstate) == 1)
+ in, prefix, prefixlen) == 1)
peer->prefix_cnt++;
/* max prefix checker */
@@ -1772,7 +1772,7 @@ rde_update_update(struct rde_peer *peer,
&state.nexthop->exit_nexthop, prefix,
prefixlen);
prefix_update(rib, peer, path_id, path_id_tx, &state,
- prefix, prefixlen, in->vstate);
+ prefix, prefixlen);
} else if (prefix_withdraw(rib, peer, path_id, prefix,
prefixlen)) {
rde_update_log(wmsg, i, peer,
@@ -3847,8 +3847,7 @@ rde_softreconfig_in(struct rib_entry *re
/* update Local-RIB */
prefix_update(rib, peer, p->path_id,
p->path_id_tx, &state,
- &prefix, pt->prefixlen,
- prefix_roa_vstate(p));
+ &prefix, pt->prefixlen);
} else if (action == ACTION_DENY) {
/* remove from Local-RIB */
prefix_withdraw(rib, peer, p->path_id, &prefix,
@@ -3986,8 +3985,7 @@ rde_roa_softreload(struct rib_entry *re,
/* update Local-RIB */
prefix_update(rib, peer, p->path_id,
p->path_id_tx, &state,
- &prefix, pt->prefixlen,
- prefix_roa_vstate(p));
+ &prefix, pt->prefixlen);
} else if (action == ACTION_DENY) {
/* remove from Local-RIB */
prefix_withdraw(rib, peer, p->path_id, &prefix,
@@ -4187,7 +4185,6 @@ network_add(struct network_config *nc, s
struct filter_set_head *vpnset = NULL;
struct in_addr prefix4;
struct in6_addr prefix6;
- uint8_t vstate;
uint16_t i;
uint32_t path_id_tx;
@@ -4249,14 +4246,16 @@ network_add(struct network_config *nc, s
rde_apply_set(vpnset, peerself, peerself, state,
nc->prefix.aid);
+ path_id_tx = pathid_assign(peerself, 0, &nc->prefix, nc->prefixlen);
+
#if NOTYET
- state.aspath.aspa_state = ASPA_NEVER_KNOWN;
+ state->aspath.aspa_state = ASPA_NEVER_KNOWN;
#endif
- vstate = rde_roa_validity(&rde_roa, &nc->prefix,
+ state->vstate = rde_roa_validity(&rde_roa, &nc->prefix,
nc->prefixlen, aspath_origin(state->aspath.aspath));
- path_id_tx = pathid_assign(peerself, 0, &nc->prefix, nc->prefixlen);
+
if (prefix_update(rib_byid(RIB_ADJ_IN), peerself, 0, path_id_tx,
- state, &nc->prefix, nc->prefixlen, vstate) == 1)
+ state, &nc->prefix, nc->prefixlen) == 1)
peerself->prefix_cnt++;
for (i = RIB_LOC_START; i < rib_size; i++) {
struct rib *rib = rib_byid(i);
@@ -4266,7 +4265,7 @@ network_add(struct network_config *nc, s
state->nexthop ? &state->nexthop->exit_nexthop : NULL,
&nc->prefix, nc->prefixlen);
prefix_update(rib, peerself, 0, path_id_tx, state, &nc->prefix,
- nc->prefixlen, vstate);
+ nc->prefixlen);
}
filterset_free(&nc->attrset);
}
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.279
diff -u -p -r1.279 rde.h
--- rde.h 17 Jan 2023 16:09:01 -0000 1.279
+++ rde.h 17 Jan 2023 16:09:50 -0000
@@ -624,14 +624,12 @@ struct prefix *prefix_adjout_lookup(stru
int);
struct prefix *prefix_adjout_next(struct rde_peer *, struct prefix *);
int prefix_update(struct rib *, struct rde_peer *, uint32_t,
- uint32_t, struct filterstate *, struct bgpd_addr *,
- int, uint8_t);
+ uint32_t, struct filterstate *, struct bgpd_addr *, int);
int prefix_withdraw(struct rib *, struct rde_peer *, uint32_t,
struct bgpd_addr *, int);
void prefix_add_eor(struct rde_peer *, uint8_t);
void prefix_adjout_update(struct prefix *, struct rde_peer *,
- struct filterstate *, struct bgpd_addr *, int,
- uint32_t, uint8_t);
+ struct filterstate *, struct bgpd_addr *, int, uint32_t);
void prefix_adjout_withdraw(struct prefix *);
void prefix_adjout_destroy(struct prefix *);
void prefix_adjout_dump(struct rde_peer *, void *,
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.251
diff -u -p -r1.251 rde_rib.c
--- rde_rib.c 28 Dec 2022 21:30:16 -0000 1.251
+++ rde_rib.c 12 Jan 2023 17:37:15 -0000
@@ -955,7 +955,7 @@ prefix_adjout_match(struct rde_peer *pee
int
prefix_update(struct rib *rib, struct rde_peer *peer, uint32_t path_id,
uint32_t path_id_tx, struct filterstate *state, struct bgpd_addr *prefix,
- int prefixlen, uint8_t vstate)
+ int prefixlen)
{
struct rde_aspath *asp, *nasp = &state->aspath;
struct rde_community *comm, *ncomm = &state->communities;
@@ -973,7 +973,7 @@ prefix_update(struct rib *rib, struct rd
path_compare(nasp, prefix_aspath(p)) == 0) {
/* no change, update last change */
p->lastchange = getmonotime();
- p->validation_state = vstate;
+ p->validation_state = state->vstate;
return (0);
}
}
@@ -997,11 +997,11 @@ prefix_update(struct rib *rib, struct rd
/* If the prefix was found move it else add it to the RIB. */
if (p != NULL)
return (prefix_move(p, peer, asp, comm, state->nexthop,
- state->nhflags, vstate));
+ state->nhflags, state->vstate));
else
return (prefix_add(prefix, prefixlen, rib, peer, path_id,
path_id_tx, asp, comm, state->nexthop, state->nhflags,
- vstate));
+ state->vstate));
}
/*
@@ -1123,7 +1123,7 @@ prefix_add_eor(struct rde_peer *peer, ui
void
prefix_adjout_update(struct prefix *p, struct rde_peer *peer,
struct filterstate *state, struct bgpd_addr *prefix, int prefixlen,
- uint32_t path_id_tx, uint8_t vstate)
+ uint32_t path_id_tx)
{
struct rde_aspath *asp;
struct rde_community *comm;
@@ -1160,7 +1160,7 @@ prefix_adjout_update(struct prefix *p, s
prefix_communities(p)) &&
path_compare(&state->aspath, prefix_aspath(p)) == 0) {
/* nothing changed */
- p->validation_state = vstate;
+ p->validation_state = state->vstate;
p->lastchange = getmonotime();
p->flags &= ~PREFIX_FLAG_STALE;
return;
@@ -1205,7 +1205,7 @@ prefix_adjout_update(struct prefix *p, s
}
prefix_link(p, NULL, p->pt, peer, 0, p->path_id_tx, asp, comm,
- state->nexthop, state->nhflags, vstate);
+ state->nexthop, state->nhflags, state->vstate);
peer->prefix_out_cnt++;
if (p->flags & PREFIX_FLAG_MASK)
Index: rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
retrieving revision 1.151
diff -u -p -r1.151 rde_update.c
--- rde_update.c 12 Jan 2023 17:35:51 -0000 1.151
+++ rde_update.c 18 Jan 2023 16:39:58 -0000
@@ -199,15 +199,16 @@ up_generate_updates(struct filter_head *
}
/* check if this was actually a withdraw */
- if (need_withdraw)
+ if (need_withdraw) {
+ rde_filterstate_clean(&state);
break;
+ }
/* from here on we know this is an update */
up_prep_adjout(peer, &state, addr.aid);
prefix_adjout_update(p, peer, &state, &addr,
- new->pt->prefixlen, new->path_id_tx,
- prefix_roa_vstate(new));
+ new->pt->prefixlen, new->path_id_tx);
rde_filterstate_clean(&state);
/* max prefix checker outbound */
@@ -337,8 +338,7 @@ up_generate_addpath(struct filter_head *
up_prep_adjout(peer, &state, addr.aid);
prefix_adjout_update(p, peer, &state, &addr,
- new->pt->prefixlen, new->path_id_tx,
- prefix_roa_vstate(new));
+ new->pt->prefixlen, new->path_id_tx);
rde_filterstate_clean(&state);
/* max prefix checker outbound */
@@ -441,7 +441,7 @@ up_generate_addpath_all(struct filter_he
up_prep_adjout(peer, &state, addr.aid);
prefix_adjout_update(p, peer, &state, &addr,
- prefixlen, new->path_id_tx, prefix_roa_vstate(new));
+ prefixlen, new->path_id_tx);
rde_filterstate_clean(&state);
/* max prefix checker outbound */
@@ -509,7 +509,7 @@ up_generate_default(struct filter_head *
}
up_prep_adjout(peer, &state, addr.aid);
- prefix_adjout_update(p, peer, &state, &addr, 0, 0, ROA_NOTFOUND);
+ prefix_adjout_update(p, peer, &state, &addr, 0, 0);
rde_filterstate_clean(&state);
/* max prefix checker outbound */
@@ -1028,7 +1028,6 @@ up_dump_attrnlri(u_char *buf, int len, s
goto done;
rde_filterstate_prep(&state, p);
-
r = up_generate_attr(buf + 2, len - 2, peer, &state, AID_INET);
rde_filterstate_clean(&state);
if (r == -1) {
