Bug: BFD confuses sessions with identical IPv6 link-local addresses

[Sebastian Hahn]

Hi there,

> On 7. Mar 2018, at 15:42, Ondrej Zajicek <santiago at crfreenet.org> wrote:
> On Sun, Mar 04, 2018 at 07:50:13PM +0100, Julian Brost wrote:
>> I just tried to enable BFD for some OSPF and BGP connections and ran
>> into an issue which BFD sessions flapping up and down and Bird logging
>> many messages like these:
>> 
>>  bfd1: Bad packet from fe80::2 - unknown session id (130079069)
>>  bfd1: Bad packet from fe80::2 - unknown session id (3102513000)
>>  bfd1: Bad packet from fe80::2 - unknown session id (3650438750)
>>  bfd1: Bad packet from fe80::2 - unknown session id (1597034259)
>> 
>> This presumably happens due to this piece of code in bfd_rx_hook() in
>> proto/bfd/packets.c:
>> 
>>  s = bfd_find_session_by_addr(p, sk->faddr);
>> 
>> I use fe80::1/fe80::2 as addresses for most of my IPv6 peering
>> connections and sk->faddr is just a 128 bit value, so it contains no
>> interface identifier, which likely leads to Bird confusing multiple BFD
>> sessions.
> Thanks for the bugreport. You are right, BFD sessions are dispatched just based
> on source address regardless of interface, so it is confused by conflicting
> link-local addresses. We will fix that, i would expect the same issue could
> happen when conflicting private IPs are used in different VRFs.

in the meantime I thought I might try my hand at a patch, as attached here.
It seems to work for me in some limited form of testing. I'll be happy to
do any and all kinds of revisions as required of course.

Thanks!
Sebastian



>From 414e8dffe70f15d6e517a795f925728dc8e88e9b Mon Sep 17 00:00:00 2001
From: Sebastian Hahn <sebastian at torproject.org>
Date: Thu, 9 Apr 2020 16:18:20 +0200
Subject: [PATCH] BFD: Session lookup by addr and local iface

Neighbours with overlapping IPv6 link-local addresses weren't working
for BFD before, as the sessions couldn't be correctly looked up.

Also log the iface index in warning messages about bad packets
---
proto/bfd/bfd.c     | 21 +++++++++++----------
proto/bfd/bfd.h     |  2 +-
proto/bfd/packets.c |  4 ++--
3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c
index b4c53754..4d53b056 100644
--- a/proto/bfd/bfd.c
+++ b/proto/bfd/bfd.c
@@ -27,10 +27,11 @@
 * related to the session and two timers (TX timer for periodic packets and hold
 * timer for session timeout). These sessions are allocated from @session_slab
 * and are accessible by two hash tables, @session_hash_id (by session ID) and
- * @session_hash_ip (by IP addresses of neighbors). Slab and both hashes are in
- * the main protocol structure &bfd_proto. The protocol logic related to BFD
- * sessions is implemented in internal functions bfd_session_*(), which are
- * expected to be called from the context of BFD thread, and external functions
+ * @session_hash_ip (by IP addresses of neighbors and the index of the local
+ * interface used to talk to them). Slab and both hashes are in the main
+ * protocol structure &bfd_proto. The protocol logic related to BFD sessions is
+ * implemented in internal functions bfd_session_*(), which are expected to be
+ * called from the context of BFD thread, and external functions
 * bfd_add_session(), bfd_remove_session() and bfd_reconfigure_session(), which
 * form an interface to the BFD core for the rest and are expected to be called
 * from the context of main thread.
@@ -108,10 +109,10 @@
#define HASH_ID_EQ(a,b)		a == b
#define HASH_ID_FN(k)		k

-#define HASH_IP_KEY(n)		n->addr
+#define HASH_IP_KEY(n)		n->addr, n->ifa->iface->index
#define HASH_IP_NEXT(n)		n->next_ip
-#define HASH_IP_EQ(a,b)		ipa_equal(a,b)
-#define HASH_IP_FN(k)		ipa_hash(k)
+#define HASH_IP_EQ(a,b,c,d)	ipa_equal(a,c) && b == d
+#define HASH_IP_FN(k,s)		ipa_hash(k)

static list bfd_proto_list;
static list bfd_wait_list;
@@ -373,9 +374,9 @@ bfd_find_session_by_id(struct bfd_proto *p, u32 id)
}

struct bfd_session *
-bfd_find_session_by_addr(struct bfd_proto *p, ip_addr addr)
+bfd_find_session_by_addr_and_ifidx(struct bfd_proto *p, ip_addr addr, uint ifidx)
{
-  return HASH_FIND(p->session_hash_ip, HASH_IP, addr);
+  return HASH_FIND(p->session_hash_ip, HASH_IP, addr, ifidx);
}

static void
@@ -635,7 +636,7 @@ bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
  if (req->iface ? !cf->accept_direct : !cf->accept_multihop)
    return 0;

-  struct bfd_session *s = bfd_find_session_by_addr(p, req->addr);
+  struct bfd_session *s = bfd_find_session_by_addr_and_ifidx(p, req->addr, req->iface->index);
  u8 state, diag;

  if (!s)
diff --git a/proto/bfd/bfd.h b/proto/bfd/bfd.h
index 5c2054cc..6792b40b 100644
--- a/proto/bfd/bfd.h
+++ b/proto/bfd/bfd.h
@@ -201,7 +201,7 @@ static inline void bfd_unlock_sessions(struct bfd_proto *p) { pthread_spin_unloc

/* bfd.c */
struct bfd_session * bfd_find_session_by_id(struct bfd_proto *p, u32 id);
-struct bfd_session * bfd_find_session_by_addr(struct bfd_proto *p, ip_addr addr);
+struct bfd_session * bfd_find_session_by_addr_and_ifidx(struct bfd_proto *p, ip_addr addr, uint ifidx);
void bfd_session_process_ctl(struct bfd_session *s, u8 flags, u32 old_tx_int, u32 old_rx_int);
void bfd_show_sessions(struct proto *P);

diff --git a/proto/bfd/packets.c b/proto/bfd/packets.c
index 703c6e28..3c8fc286 100644
--- a/proto/bfd/packets.c
+++ b/proto/bfd/packets.c
@@ -366,7 +366,7 @@ bfd_rx_hook(sock *sk, uint len)
    if (ps > BFD_STATE_DOWN)
      DROP("invalid init state", ps);

-    s = bfd_find_session_by_addr(p, sk->faddr);
+    s = bfd_find_session_by_addr_and_ifidx(p, sk->faddr, sk->lifindex);

    /* FIXME: better session matching and message */
    if (!s)
@@ -395,7 +395,7 @@ bfd_rx_hook(sock *sk, uint len)
  return 1;

drop:
-  LOG_PKT("Bad packet from %I - %s (%u)", sk->faddr, err_dsc, err_val);
+  LOG_PKT("Bad packet from %I%%%d - %s (%u)", sk->faddr, sk->lifindex, err_dsc, err_val);
  return 1;
}

-- 
2.26.0