On Wed, May 25, 2022 at 04:27:46PM +0200, Claudio Jeker wrote:
> On Wed, May 25, 2022 at 03:50:38PM +0200, Theo Buehler wrote:
> > On Wed, May 25, 2022 at 02:46:55PM +0200, Claudio Jeker wrote:
> > > Noticed this morning working on something else. The non-transitive
> > > extended community support is fairly messed up. In my case (set
> > > ext-community rt 1:1) it causes the community to be skipped.
> > >
> > > First of all the check in non_transitive_community() is messed up. It uses
> > > the wrong data field, does a unneccessary ntohl and the logic seems also
> > > to be reversed.
> > >
> > > While looking at this I decided to fix the various switch statements on
> > > type to mask out the two high bits with this extending ext community types
> > > should be a bit easier.
> > >
> > > While there be strict from where bgpd accepts non-transitive
> > > ext-communities. Now non-transitive ext communities are filtered in and
> > > out
> > > for ebgp sessions. So non-transitive ext communities only pass on ibgp
> > > sessions.
> > >
> > > For the rde_attr_add() the system defaults to ibgp since this is for
> > > network announcements.
> > >
> > > There is also a trivial fix to the bgpd unittests that I excluded here.
> > >
> > > With this ext-communities are properly sent on updates.
> > > OK?
> >
> > Makes sense, one comment below.
> >
> > > --
> > > :wq Claudio
> > >
> > > Index: rde_community.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
> > > retrieving revision 1.4
> > > diff -u -p -r1.4 rde_community.c
> > > --- rde_community.c 6 Feb 2022 09:51:19 -0000 1.4
> > > +++ rde_community.c 25 May 2022 12:30:44 -0000
> > > @@ -99,7 +99,7 @@ fc2c(struct community *fc, struct rde_pe
> > > subtype = fc->data3 & 0xff;
> > >
> > > c->data3 = type << 8 | subtype;
> > > - switch (type) {
> > > + switch (type & EXT_COMMUNITY_VALUE) {
> > > case -1:
> >
> > Should that '-1' not be changed to 'EXT_COMMUNITY_VALUE'? Since you now
> > mask out the high two bits, it is no longer possible to reach this case.
>
> Good point. I decided to refactor the code a but and move the -1 case into
> its own if statement above the switch. I think this is the best solution
> here. I also did tidy up some types.
parseextcommunity(), parseextvalue() and parsesubtype() still use an int
for type and log_ext_subtype() uses a short. Not sure if you want to
adjust them as well.
The diff reads ok now.
>
> --
> :wq Claudio
>
> Index: bgpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.423
> diff -u -p -r1.423 bgpd.h
> --- bgpd.h 23 May 2022 13:40:11 -0000 1.423
> +++ bgpd.h 25 May 2022 14:14:24 -0000
> @@ -989,7 +989,7 @@ struct filter_peers {
> #define EXT_COMMUNITY_FLAG_VALID 0x01
>
> struct ext_comm_pairs {
> - short type;
> + uint8_t type;
> uint8_t subtype;
> const char *subname;
> };
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.545
> diff -u -p -r1.545 rde.c
> --- rde.c 6 May 2022 15:51:09 -0000 1.545
> +++ rde.c 25 May 2022 12:30:44 -0000
> @@ -1917,8 +1917,8 @@ bad_flags:
> if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE,
> ATTR_PARTIAL))
> goto bad_flags;
> - if (community_ext_add(&state->communities, flags, p,
> - attr_len) == -1) {
> + if (community_ext_add(&state->communities, flags,
> + peer->conf.ebgp, p, attr_len) == -1) {
> /*
> * mark update as bad and withdraw all routes as per
> * RFC 7606
> @@ -2062,8 +2062,8 @@ rde_attr_add(struct filterstate *state,
> return community_large_add(&state->communities, flags, p,
> attr_len);
> case ATTR_EXT_COMMUNITIES:
> - return community_ext_add(&state->communities, flags, p,
> - attr_len);
> + return community_ext_add(&state->communities, flags, 0,
> + p, attr_len);
> }
>
> if (attr_optadd(&state->aspath, flags, type, p, attr_len) == -1)
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.251
> diff -u -p -r1.251 rde.h
> --- rde.h 22 Mar 2022 10:53:08 -0000 1.251
> +++ rde.h 25 May 2022 12:30:44 -0000
> @@ -459,7 +459,7 @@ void community_delete(struct rde_communi
>
> int community_add(struct rde_community *, int, void *, size_t);
> int community_large_add(struct rde_community *, int, void *, size_t);
> -int community_ext_add(struct rde_community *, int, void *, size_t);
> +int community_ext_add(struct rde_community *, int, int, void *, size_t);
>
> int community_write(struct rde_community *, void *, uint16_t);
> int community_large_write(struct rde_community *, void *, uint16_t);
> Index: rde_community.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 rde_community.c
> --- rde_community.c 6 Feb 2022 09:51:19 -0000 1.4
> +++ rde_community.c 25 May 2022 14:19:39 -0000
> @@ -61,7 +61,7 @@ static int
> fc2c(struct community *fc, struct rde_peer *peer, struct community *c,
> struct community *m)
> {
> - short type;
> + int type;
> uint8_t subtype;
>
> memset(c, 0, sizeof(*c));
> @@ -98,9 +98,7 @@ fc2c(struct community *fc, struct rde_pe
> type = (int32_t)fc->data3 >> 8;
> subtype = fc->data3 & 0xff;
>
> - c->data3 = type << 8 | subtype;
> - switch (type) {
> - case -1:
> + if (type == -1) {
> /* special case for 'ext-community rt *' */
> if ((fc->flags >> 8 & 0xff) != COMMUNITY_ANY ||
> m == NULL)
> @@ -110,6 +108,10 @@ fc2c(struct community *fc, struct rde_pe
> m->data2 = 0;
> m->data3 = 0xff;
> return 0;
> + }
> +
> + c->data3 = type << 8 | subtype;
> + switch (type & EXT_COMMUNITY_VALUE) {
> case EXT_COMMUNITY_TRANS_TWO_AS:
> if ((fc->flags >> 8 & 0xff) == COMMUNITY_ANY)
> break;
> @@ -141,7 +143,6 @@ fc2c(struct community *fc, struct rde_pe
> return 0;
> case EXT_COMMUNITY_TRANS_OPAQUE:
> case EXT_COMMUNITY_TRANS_EVPN:
> - case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> if ((fc->flags >> 8 & 0xff) == COMMUNITY_ANY)
> break;
>
> @@ -244,10 +245,9 @@ insert_community(struct rde_community *c
> }
>
> static int
> -non_transitive_community(struct community *c)
> +non_transitive_ext_community(struct community *c)
> {
> - if ((uint8_t)c->flags == COMMUNITY_TYPE_EXT &&
> - !((ntohl(c->data1) >> 24) & EXT_COMMUNITY_NON_TRANSITIVE))
> + if ((c->data3 >> 8) & EXT_COMMUNITY_NON_TRANSITIVE)
> return 1;
> return 0;
> }
> @@ -404,7 +404,8 @@ community_large_add(struct rde_community
> }
>
> int
> -community_ext_add(struct rde_community *comm, int flags, void *buf, size_t
> len)
> +community_ext_add(struct rde_community *comm, int flags, int ebgp,
> + void *buf, size_t len)
> {
> struct community set = { .flags = COMMUNITY_TYPE_EXT };
> uint8_t *b = buf, type;
> @@ -422,11 +423,13 @@ community_ext_add(struct rde_community *
>
> c = be64toh(c);
> type = c >> 56;
> - switch (type) {
> + /* filter out non-transitive ext communuties from ebgp peers */
> + if (ebgp && (type & EXT_COMMUNITY_NON_TRANSITIVE))
> + continue;
> + switch (type & EXT_COMMUNITY_VALUE) {
> case EXT_COMMUNITY_TRANS_TWO_AS:
> case EXT_COMMUNITY_TRANS_OPAQUE:
> case EXT_COMMUNITY_TRANS_EVPN:
> - case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> set.data1 = c >> 32 & 0xffff;
> set.data2 = c;
> break;
> @@ -564,7 +567,7 @@ community_ext_write(struct rde_community
> for (l = 0; l < comm->nentries; l++)
> if ((uint8_t)comm->communities[l].flags ==
> COMMUNITY_TYPE_EXT && !(ebgp &&
> - non_transitive_community(&comm->communities[l])))
> + non_transitive_ext_community(&comm->communities[l])))
> n++;
>
> if (n == 0)
> @@ -580,13 +583,12 @@ community_ext_write(struct rde_community
> for (l = 0; l < comm->nentries; l++) {
> cp = comm->communities + l;
> if ((uint8_t)cp->flags == COMMUNITY_TYPE_EXT && !(ebgp &&
> - non_transitive_community(cp))) {
> + non_transitive_ext_community(cp))) {
> ext = (uint64_t)cp->data3 << 48;
> - switch (cp->data3 >> 8) {
> + switch ((cp->data3 >> 8) & EXT_COMMUNITY_VALUE) {
> case EXT_COMMUNITY_TRANS_TWO_AS:
> case EXT_COMMUNITY_TRANS_OPAQUE:
> case EXT_COMMUNITY_TRANS_EVPN:
> - case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> ext |= ((uint64_t)cp->data1 & 0xffff) << 32;
> ext |= (uint64_t)cp->data2;
> break;
> @@ -672,11 +674,10 @@ community_writebuf(struct ibuf *buf, str
> continue;
>
> ext = (uint64_t)cp->data3 << 48;
> - switch (cp->data3 >> 8) {
> + switch ((cp->data3 >> 8) & EXT_COMMUNITY_VALUE) {
> case EXT_COMMUNITY_TRANS_TWO_AS:
> case EXT_COMMUNITY_TRANS_OPAQUE:
> case EXT_COMMUNITY_TRANS_EVPN:
> - case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> ext |= ((uint64_t)cp->data1 & 0xffff) << 32;
> ext |= (uint64_t)cp->data2;
> break;
> @@ -919,7 +920,7 @@ community_to_rd(struct community *fc, ui
> if (fc2c(fc, NULL, &c, NULL) == -1)
> return -1;
>
> - switch (c.data3 >> 8) {
> + switch ((c.data3 >> 8) & EXT_COMMUNITY_VALUE) {
> case EXT_COMMUNITY_TRANS_TWO_AS:
> rd = (0ULL << 48);
> rd |= ((uint64_t)c.data1 & 0xffff) << 32;
