pimd: RPF change to unreachable was leaving a stale entry in the jp-agg list

This was causing pimd to crash later; call-stack -
(gdb) bt
    context=<optimized out>) at lib/sigevent.c:254
    group=group@entry=0x7ffffa9797e0) at pimd/pim_rp.c:207
    grp=grp@entry=0x7ffffa9799fe, sgs=sgs@entry=0x560ac069edb0, size=52)
    at pimd/pim_msg.c:200
    groups=<optimized out>) at pimd/pim_join.c:562
    at pimd/pim_neighbor.c:288
    at lib/thread.c:1599
    at lib/libfrr.c:1024
    envp=<optimized out>) at pimd/pim_main.c:162
(gdb) fr 4
    group=group@entry=0x7ffffa9797e0) at pimd/pim_rp.c:207
207     pimd/pim_rp.c: No such file or directory.
(gdb) fr 6
    grp=grp@entry=0x7ffffa9799fe, sgs=sgs@entry=0x560ac069edb0, size=52)
    at pimd/pim_msg.c:200
200     pimd/pim_msg.c: No such file or directory.
(gdb) p source->up->sg_str
$1 = '\000' <repeats 31 times>, <incomplete sequence \361>
(gdb)

This problem can manifest in the following event sequence -
1. upstream RPF neighbor is resolved
2. upstream RPF neighbor becomes unresolved (but upstream entry
   stays on the jp-agg list)
3. upstream entry is removed
on the next old-neighbor jp-agg-list processing the stale entry is
accessed resulting in the crash.

Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>

5 files changed, 22 insertions, 16 deletions

diff --git a/pimd/pim_jp_agg.c b/pimd/pim_jp_agg.c
index 243316b433..e1473bfe3b 100644
--- a/pimd/pim_jp_agg.c
+++ b/pimd/pim_jp_agg.c
@@ -323,7 +323,8 @@ void pim_jp_agg_switch_interface(struct pim_rpf *orpf, struct pim_rpf *nrpf,
 		pim_jp_agg_add_group(opius->us, up, false);
 
 	/* send Join(S,G) to the current upstream neighbor */
-	pim_jp_agg_add_group(npius->us, up, true);
+	if (npius)
+		pim_jp_agg_add_group(npius->us, up, true);
 }
 
 
diff --git a/pimd/pim_nht.c b/pimd/pim_nht.c
index 2fbde20075..bd8424b3e4 100644
--- a/pimd/pim_nht.c
+++ b/pimd/pim_nht.c
@@ -447,15 +447,16 @@ static int pim_update_upstream_nh_helper(struct hash_bucket *bucket, void *arg)
 
 	old.source_nexthop.interface = up->rpf.source_nexthop.interface;
 	rpf_result = pim_rpf_update(pim, up, &old, __func__);
-	if (rpf_result == PIM_RPF_FAILURE) {
-		pim_upstream_rpf_clear(pim, up);
-		return HASHWALK_CONTINUE;
-	}
 
-	/* update kernel multicast forwarding cache (MFC) */
-	pim_upstream_mroute_iif_update(up->channel_oil, __func__);
+	/* update kernel multicast forwarding cache (MFC); if the
+	 * RPF nbr is now unreachable the MFC has already been updated
+	 * by pim_rpf_clear
+	 */
+	if (rpf_result != PIM_RPF_FAILURE)
+		pim_upstream_mroute_iif_update(up->channel_oil, __func__);
 
-	if (rpf_result == PIM_RPF_CHANGED)
+	if (rpf_result == PIM_RPF_CHANGED ||
+		(rpf_result == PIM_RPF_FAILURE && old.source_nexthop.interface))
 		pim_zebra_upstream_rpf_changed(pim, up, &old);
 
 
@@ -464,7 +465,8 @@ static int pim_update_upstream_nh_helper(struct hash_bucket *bucket, void *arg)
 			__PRETTY_FUNCTION__, up->sg_str, pim->vrf->name,
 			old.source_nexthop.interface
 			? old.source_nexthop.interface->name : "Unknown",
-			up->rpf.source_nexthop.interface->name);
+			up->rpf.source_nexthop.interface
+			? up->rpf.source_nexthop.interface->name : "Unknown");
 	}
 
 	return HASHWALK_CONTINUE;
diff --git a/pimd/pim_rp.c b/pimd/pim_rp.c
index 5eb035f98c..bf42d03604 100644
--- a/pimd/pim_rp.c
+++ b/pimd/pim_rp.c
@@ -390,7 +390,9 @@ void pim_upstream_update(struct pim_instance *pim, struct pim_upstream *up)
 	if (up->rpf.source_nexthop.interface && up->channel_oil)
 		pim_upstream_mroute_iif_update(up->channel_oil, __func__);
 
-	if (rpf_result == PIM_RPF_CHANGED)
+	if (rpf_result == PIM_RPF_CHANGED ||
+			(rpf_result == PIM_RPF_FAILURE &&
+			 old_rpf.source_nexthop.interface))
 		pim_zebra_upstream_rpf_changed(pim, up, &old_rpf);
 
 	pim_zebra_update_all_interfaces(pim);
diff --git a/pimd/pim_rpf.c b/pimd/pim_rpf.c
index 1472d69f56..1eb5006b94 100644
--- a/pimd/pim_rpf.c
+++ b/pimd/pim_rpf.c
@@ -215,6 +215,10 @@ enum pim_rpf_result pim_rpf_update(struct pim_instance *pim,
 
 	saved.source_nexthop = rpf->source_nexthop;
 	saved.rpf_addr = rpf->rpf_addr;
+	if (old) {
+		old->source_nexthop = saved.source_nexthop;
+		old->rpf_addr = saved.rpf_addr;
+	}
 
 	nht_p.family = AF_INET;
 	nht_p.prefixlen = IPV4_MAX_BITLEN;
@@ -287,11 +291,6 @@ enum pim_rpf_result pim_rpf_update(struct pim_instance *pim,
 	    || saved.source_nexthop
 			       .interface != rpf->source_nexthop.interface) {
 
-		/* return old rpf to caller ? */
-		if (old) {
-			old->source_nexthop = saved.source_nexthop;
-			old->rpf_addr = saved.rpf_addr;
-		}
 		return PIM_RPF_CHANGED;
 	}
 
diff --git a/pimd/pim_upstream.c b/pimd/pim_upstream.c
index b28af54a30..53da58e8fe 100644
--- a/pimd/pim_upstream.c
+++ b/pimd/pim_upstream.c
@@ -1772,7 +1772,9 @@ void pim_upstream_find_new_rpf(struct pim_instance *pim)
 					__PRETTY_FUNCTION__, up->sg_str);
 			old.source_nexthop.interface = up->rpf.source_nexthop.interface;
 			rpf_result = pim_rpf_update(pim, up, &old, __func__);
-			if (rpf_result == PIM_RPF_CHANGED)
+			if (rpf_result == PIM_RPF_CHANGED ||
+					(rpf_result == PIM_RPF_FAILURE &&
+					 old.source_nexthop.interface))
 				pim_zebra_upstream_rpf_changed(pim, up, &old);
 			/* update kernel multicast forwarding cache (MFC) */
 			pim_upstream_mroute_iif_update(up->channel_oil, __func__);