From 13044b6f6fe34f7f3cbe83ec810bdf24bef35b96 Mon Sep 17 00:00:00 2001 From: SumitAgarwal123 Date: Thu, 19 Sep 2019 03:04:48 -0700 Subject: [PATCH 1/7] bfdd: Fixing coredump in log Param missing in debug log, leading to coredump Signed-off-by: Sayed Mohd Saquib --- bfdd/bfd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bfdd/bfd.c b/bfdd/bfd.c index 245f2dc5ee..b884cd03da 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1432,7 +1432,7 @@ struct bfd_session *bfd_key_lookup(struct bfd_key key) if (ctx.result) { bsp = ctx.result; log_debug(" peer %s found, but ifp" - " and/or loc-addr params ignored"); + " and/or loc-addr params ignored", peer_buf); } return bsp; } From c7666ae7a18f28c42b18b5c185c1bbc7ec89f5ce Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Thu, 10 Oct 2019 09:07:21 +0200 Subject: [PATCH 2/7] bfdd: upon vrf disable, unlink bfd session with vrf bfd session has a vrf pointer that needs to be reset, when vrf is disabled. Signed-off-by: Philippe Guibert --- bfdd/bfd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bfdd/bfd.c b/bfdd/bfd.c index b884cd03da..0ff638350b 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -216,6 +216,7 @@ void bfd_session_disable(struct bfd_session *bs) bfd_echo_recvtimer_delete(bs); bfd_xmttimer_delete(bs); bfd_echo_xmttimer_delete(bs); + bs->vrf = NULL; } static uint32_t ptm_bfd_gen_ID(void) From 507d75d4532c9ed3be5b4f232ae1a0f434f97d83 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 11:15:56 -0300 Subject: [PATCH 3/7] bfdd: disable sockets polling before closing it Otherwise the `thread_read` will keep waking us up to handle closing sockets which are never unregistered. Signed-off-by: Rafael Zalamena --- bfdd/bfd.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/bfdd/bfd.c b/bfdd/bfd.c index 0ff638350b..c15391c654 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1709,6 +1709,15 @@ static int bfd_vrf_disable(struct vrf *vrf) } log_debug("VRF disable %s id %d", vrf->name, vrf->vrf_id); + + /* Disable read/write poll triggering. */ + THREAD_OFF(bvrf->bg_ev[0]); + THREAD_OFF(bvrf->bg_ev[1]); + THREAD_OFF(bvrf->bg_ev[2]); + THREAD_OFF(bvrf->bg_ev[3]); + THREAD_OFF(bvrf->bg_ev[4]); + THREAD_OFF(bvrf->bg_ev[5]); + /* Close all descriptors. */ socket_close(&bvrf->bg_echo); socket_close(&bvrf->bg_shop); From 8ee0862e80da186dcae93311c22e17783cdd723c Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 13:12:26 -0300 Subject: [PATCH 4/7] bfdd: set session down after disabling it If a session is no longer able to send/receive packets, it is very likely it will be down in a few milliseconds so lets speed up the process and correctly mark it as down. Signed-off-by: Rafael Zalamena --- bfdd/bfd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/bfdd/bfd.c b/bfdd/bfd.c index c15391c654..072f44dc3b 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -213,10 +213,13 @@ void bfd_session_disable(struct bfd_session *bs) /* Disable all timers. */ bfd_recvtimer_delete(bs); - bfd_echo_recvtimer_delete(bs); bfd_xmttimer_delete(bs); - bfd_echo_xmttimer_delete(bs); + ptm_bfd_echo_stop(bs); bs->vrf = NULL; + bs->ifp = NULL; + + /* Set session down so it doesn't report UP and disabled. */ + ptm_bfd_sess_dn(bs, BD_PATH_DOWN); } static uint32_t ptm_bfd_gen_ID(void) @@ -329,7 +332,14 @@ void ptm_bfd_sess_dn(struct bfd_session *bfd, uint8_t diag) bfd->demand_mode = 0; monotime(&bfd->downtime); - ptm_bfd_snd(bfd, 0); + /* + * Only attempt to send if we have a valid socket: + * this function might be called by session disablers and in + * this case we won't have a valid socket (i.e. interface was + * removed or VRF doesn't exist anymore). + */ + if (bfd->sock != -1) + ptm_bfd_snd(bfd, 0); /* Slow down the control packets, the connection is down. */ bs_set_slow_timers(bfd); From ced291deb79f04ba7f6e17a0bbbc88fd95c47598 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 16:13:24 -0300 Subject: [PATCH 5/7] bfdd: simplify session observers code Don't be selective about what to observe, always observe all possible aspects of the session that may change on run-time (i.e. bind address, interface and VRF existence). Signed-off-by: Rafael Zalamena --- bfdd/bfd.c | 15 +++------------ bfdd/bfd.h | 8 ++------ bfdd/ptm_adapter.c | 13 ------------- 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/bfdd/bfd.c b/bfdd/bfd.c index 072f44dc3b..90287114fc 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1217,19 +1217,10 @@ int bs_observer_add(struct bfd_session *bs) struct bfd_session_observer *bso; bso = XCALLOC(MTYPE_BFDD_SESSION_OBSERVER, sizeof(*bso)); - bso->bso_isaddress = false; bso->bso_bs = bs; - bso->bso_isinterface = !BFD_CHECK_FLAG(bs->flags, BFD_SESS_FLAG_MH); - if (bso->bso_isinterface) - strlcpy(bso->bso_entryname, bs->key.ifname, - sizeof(bso->bso_entryname)); - /* Handle socket binding failures caused by missing local addresses. */ - if (bs->sock == -1) { - bso->bso_isaddress = true; - bso->bso_addr.family = bs->key.family; - memcpy(&bso->bso_addr.u.prefix, &bs->key.local, - sizeof(bs->key.local)); - } + bso->bso_addr.family = bs->key.family; + memcpy(&bso->bso_addr.u.prefix, &bs->key.local, + sizeof(bs->key.local)); TAILQ_INSERT_TAIL(&bglobal.bg_obslist, bso, bso_entry); diff --git a/bfdd/bfd.h b/bfdd/bfd.h index cdec78d122..a9c8bd183a 100644 --- a/bfdd/bfd.h +++ b/bfdd/bfd.h @@ -274,12 +274,8 @@ struct bfd_state_str_list { struct bfd_session_observer { struct bfd_session *bso_bs; - bool bso_isinterface; - bool bso_isaddress; - union { - char bso_entryname[MAXNAMELEN]; - struct prefix bso_addr; - }; + char bso_entryname[MAXNAMELEN]; + struct prefix bso_addr; TAILQ_ENTRY(bfd_session_observer) bso_entry; }; diff --git a/bfdd/ptm_adapter.c b/bfdd/ptm_adapter.c index 1b3219c235..df48bc2af0 100644 --- a/bfdd/ptm_adapter.c +++ b/bfdd/ptm_adapter.c @@ -579,8 +579,6 @@ static void bfdd_sessions_enable_interface(struct interface *ifp) TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) { bs = bso->bso_bs; - if (bso->bso_isinterface == false) - continue; /* Interface name mismatch. */ if (strcmp(ifp->name, bs->key.ifname)) continue; @@ -605,10 +603,6 @@ static void bfdd_sessions_disable_interface(struct interface *ifp) struct bfd_session *bs; TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) { - if (bso->bso_isinterface == false) - continue; - - /* Interface name mismatch. */ bs = bso->bso_bs; if (strcmp(ifp->name, bs->key.ifname)) continue; @@ -616,7 +610,6 @@ static void bfdd_sessions_disable_interface(struct interface *ifp) if (bs->sock == -1) continue; - /* Try to enable it. */ bfd_session_disable(bs); } @@ -658,8 +651,6 @@ void bfdd_sessions_disable_vrf(struct vrf *vrf) struct bfd_session *bs; TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) { - if (bso->bso_isinterface) - continue; bs = bso->bso_bs; if (bs->key.vrfname[0] && strcmp(vrf->name, bs->key.vrfname)) @@ -668,7 +659,6 @@ void bfdd_sessions_disable_vrf(struct vrf *vrf) if (bs->sock == -1) continue; - /* Try to enable it. */ bfd_session_disable(bs); } } @@ -701,9 +691,6 @@ static void bfdd_sessions_enable_address(struct connected *ifc) struct prefix prefix; TAILQ_FOREACH(bso, &bglobal.bg_obslist, bso_entry) { - if (bso->bso_isaddress == false) - continue; - /* Skip enabled sessions. */ bs = bso->bso_bs; if (bs->sock != -1) From 38e9efd85f68c115f2250d3a6e2249967b4b35b9 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 16:54:51 -0300 Subject: [PATCH 6/7] bfdd: don't allow link-local without interface When using link-local addresses we must provide scope-id to the operating system so it knows where to send packets. Spotted by Pavel Ivashchenko (@zays26). Signed-off-by: Rafael Zalamena --- bfdd/bfdd_northbound.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/bfdd/bfdd_northbound.c b/bfdd/bfdd_northbound.c index 7cd2fb6b9a..975fc7b31f 100644 --- a/bfdd/bfdd_northbound.c +++ b/bfdd/bfdd_northbound.c @@ -58,10 +58,36 @@ static int bfd_session_create(enum nb_event event, const struct lyd_node *dnode, union nb_resource *resource, bool mhop) { struct bfd_session *bs; + const char *ifname; struct bfd_key bk; + struct prefix p; switch (event) { case NB_EV_VALIDATE: + /* + * When `dest-addr` is IPv6 and link-local we must + * require interface name, otherwise we can't figure + * which interface to use to send the packets. + */ + yang_dnode_get_prefix(&p, dnode, "./dest-addr"); + + /* + * To support old FRR versions we must allow empty + * interface to be specified, however that should + * change in the future. + */ + if (yang_dnode_exists(dnode, "./interface")) + ifname = yang_dnode_get_string(dnode, "./interface"); + else + ifname = ""; + + if (p.family == AF_INET6 + && IN6_IS_ADDR_LINKLOCAL(&p.u.prefix6) + && strlen(ifname) == 0) { + zlog_warn("%s: when using link-local you must specify " + "an interface.", __func__); + return NB_ERR_VALIDATION; + } break; case NB_EV_PREPARE: From 46fcb2df90409e401223d29c6162753bf73fca64 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 20:15:46 -0300 Subject: [PATCH 7/7] lib: use `prefix` for yang get prefix wrapper This change fixes a static analyzer warning and should also make us safer when using this function. At the moment the code that triggered the warning is the only one that uses this function. Passing anything other than `struct prefix` to `str2prefix` function is dangerous, because the structure might be smaller than expected and we might have an buffer overflow. Signed-off-by: Rafael Zalamena --- lib/yang_wrappers.c | 10 ++++++++-- lib/yang_wrappers.h | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/yang_wrappers.c b/lib/yang_wrappers.c index 50225f35a0..a308b18b73 100644 --- a/lib/yang_wrappers.c +++ b/lib/yang_wrappers.c @@ -799,7 +799,7 @@ struct yang_data *yang_data_new_prefix(const char *xpath, return yang_data_new(xpath, value_str); } -void yang_dnode_get_prefix(union prefixptr prefix, const struct lyd_node *dnode, +void yang_dnode_get_prefix(struct prefix *prefix, const struct lyd_node *dnode, const char *xpath_fmt, ...) { const struct lyd_node_leaf_list *dleaf; @@ -816,9 +816,15 @@ void yang_dnode_get_prefix(union prefixptr prefix, const struct lyd_node *dnode, YANG_DNODE_GET_ASSERT(dnode, xpath); } + /* + * Initialize prefix to avoid static analyzer complaints about + * uninitialized memory. + */ + memset(prefix, 0, sizeof(*prefix)); + dleaf = (const struct lyd_node_leaf_list *)dnode; assert(dleaf->value_type == LY_TYPE_STRING); - (void)str2prefix(dleaf->value_str, prefix.p); + (void)str2prefix(dleaf->value_str, prefix); } void yang_get_default_prefix(union prefixptr var, const char *xpath_fmt, ...) diff --git a/lib/yang_wrappers.h b/lib/yang_wrappers.h index 1a30ff3686..10d1ea314f 100644 --- a/lib/yang_wrappers.h +++ b/lib/yang_wrappers.h @@ -118,7 +118,7 @@ extern void yang_get_default_string_buf(char *buf, size_t size, extern void yang_str2prefix(const char *value, union prefixptr prefix); extern struct yang_data *yang_data_new_prefix(const char *xpath, union prefixconstptr prefix); -extern void yang_dnode_get_prefix(union prefixptr prefix, +extern void yang_dnode_get_prefix(struct prefix *prefix, const struct lyd_node *dnode, const char *xpath_fmt, ...); extern void yang_get_default_prefix(union prefixptr var, const char *xpath_fmt,