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.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 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.
> /* special case for 'ext-community rt *' */
> if ((fc->flags >> 8 & 0xff) != COMMUNITY_ANY ||
> @@ -141,7 +141,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 +243,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 +402,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 +421,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 +565,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 +581,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 +672,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 +918,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;
>
