From a72ac295b91efde6bf2cf72c99ee2e8b9804d4e0 Mon Sep 17 00:00:00 2001 From: SumitAgarwal123 Date: Thu, 19 Sep 2019 03:04:48 -0700 Subject: [PATCH 1/6] 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 1f1568f511..89a097fed8 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1434,7 +1434,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 d6dc32f6f6a7b0aa01dddf0a15f8188048b2e23c Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Thu, 10 Oct 2019 09:07:21 +0200 Subject: [PATCH 2/6] 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 89a097fed8..aa6aba962f 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 90104e23f282af8b8e7891231d5eb66fba8ceb2b Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 11:15:56 -0300 Subject: [PATCH 3/6] 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 aa6aba962f..7e57bcbc71 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1701,6 +1701,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 2d0d00ce4661970602d2197b80f32e55126eb4f7 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 13:12:26 -0300 Subject: [PATCH 4/6] 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 7e57bcbc71..75af2baf62 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 f1062401307adf5f495a450a2b1da84ae6c1fb3b Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 16:13:24 -0300 Subject: [PATCH 5/6] 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 75af2baf62..a7ca85aa98 100644 --- a/bfdd/bfd.c +++ b/bfdd/bfd.c @@ -1219,19 +1219,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 10aeb3e52c..a817014c75 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 3e2ace6ea6..707d3e581c 100644 --- a/bfdd/ptm_adapter.c +++ b/bfdd/ptm_adapter.c @@ -576,8 +576,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; @@ -602,10 +600,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; @@ -613,7 +607,6 @@ static void bfdd_sessions_disable_interface(struct interface *ifp) if (bs->sock == -1) continue; - /* Try to enable it. */ bfd_session_disable(bs); } @@ -650,8 +643,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)) @@ -660,7 +651,6 @@ void bfdd_sessions_disable_vrf(struct vrf *vrf) if (bs->sock == -1) continue; - /* Try to enable it. */ bfd_session_disable(bs); } } @@ -716,9 +706,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 80420ce34d3a69cce48058e834ebf48085d6f8c2 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Fri, 11 Oct 2019 16:54:51 -0300 Subject: [PATCH 6/6] 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 | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/bfdd/bfdd_northbound.c b/bfdd/bfdd_northbound.c index 7cd2fb6b9a..6bcc5d66bb 100644 --- a/bfdd/bfdd_northbound.c +++ b/bfdd/bfdd_northbound.c @@ -58,10 +58,39 @@ 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 in6_addr i6a; 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. + * + * `memset` `i6a` in case address is IPv4 or non + * link-local IPv6, it should also avoid static + * analyzer warning about unset memory read. + */ + memset(&i6a, 0, sizeof(i6a)); + yang_dnode_get_ipv6(&i6a, 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 (IN6_IS_ADDR_LINKLOCAL(&i6a) && 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: