From 4d60d9e2b4690bc0f1c84dd68b8b25c5c523e39a Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Thu, 7 Dec 2023 15:26:47 +0100 Subject: [PATCH] zebra: enqueue NHG_DEL in rib_nhg meta queue The NHG_DEL operation is done directly from ZAPI call, whereas the NHG_ADD operation is done in the rib_nhg meta queue. This may be problematic when ADD is followed by DEL. Imagine a scenarion with two protocol NHIDs. depends of and . The deletion of at the protocol level will trigger 2 messages to ZEBRA: NHG_ADD() and NHG_DEL(). Those operations are properly enqueued in ZAPI, but in the end, the NHG_DEL is executed first. This causes NHG_ADD to unlink an already freed NHG. Fix this by consistently enqueuing NHG_DEL and NHG_ADD operations. Signed-off-by: Philippe Guibert --- zebra/rib.h | 1 + zebra/zapi_msg.c | 25 +++++++++---------- zebra/zebra_rib.c | 62 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 65 insertions(+), 23 deletions(-) diff --git a/zebra/rib.h b/zebra/rib.h index 7ce6fbe609..61d9953d81 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -430,6 +430,7 @@ extern int rib_queue_nhg_ctx_add(struct nhg_ctx *ctx); /* Enqueue incoming nhg from proto daemon for processing */ extern int rib_queue_nhe_add(struct nhg_hash_entry *nhe); +extern int rib_queue_nhe_del(struct nhg_hash_entry *nhe); /* Enqueue evpn route for processing */ int zebra_rib_queue_evpn_route_add(vrf_id_t vrf_id, const struct ethaddr *rmac, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 2d1614ca54..761eafeb13 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1959,20 +1959,19 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS) return; } - /* - * Delete the received nhg id - */ - nhe = zebra_nhg_proto_del(api_nhg.id, api_nhg.proto); + /* Create a temporary nhe */ + nhe = zebra_nhg_alloc(); + nhe->id = api_nhg.id; + nhe->type = api_nhg.proto; + nhe->zapi_instance = client->instance; + nhe->zapi_session = client->session_id; + + /* Sanity check - Empty nexthop and group */ + nhe->nhg.nexthop = NULL; + + /* Enqueue to workqueue for processing */ + rib_queue_nhe_del(nhe); - if (nhe) { - zebra_nhg_decrement_ref(nhe); - zsend_nhg_notify(api_nhg.proto, client->instance, - client->session_id, api_nhg.id, - ZAPI_NHG_REMOVED); - } else - zsend_nhg_notify(api_nhg.proto, client->instance, - client->session_id, api_nhg.id, - ZAPI_NHG_REMOVE_FAIL); /* Stats */ client->nhg_del_cnt++; } diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 0bcb21fd74..54e0a34922 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -186,6 +186,7 @@ struct wq_nhg_wrapper { struct nhg_ctx *ctx; struct nhg_hash_entry *nhe; } u; + bool deletion; }; #define WQ_NHG_WRAPPER_TYPE_CTX 0x01 @@ -2531,7 +2532,7 @@ static void process_subq_evpn(struct listnode *lnode) static void process_subq_nhg(struct listnode *lnode) { struct nhg_ctx *ctx; - struct nhg_hash_entry *nhe, *newnhe; + struct nhg_hash_entry *nhe, *newnhe, *oldnhe; struct wq_nhg_wrapper *w; uint8_t qindex = META_QUEUE_NHG; @@ -2563,15 +2564,33 @@ static void process_subq_nhg(struct listnode *lnode) subqueue2str(qindex)); /* Process incoming nhg update, probably from a proto daemon */ - newnhe = zebra_nhg_proto_add(nhe->id, nhe->type, - nhe->zapi_instance, - nhe->zapi_session, &nhe->nhg, 0); + if (w->deletion) { + /* + * Delete the received nhg id + */ + oldnhe = zebra_nhg_proto_del(nhe->id, nhe->type); + if (oldnhe) { + zsend_nhg_notify(nhe->type, nhe->zapi_instance, + nhe->zapi_session, nhe->id, + ZAPI_NHG_REMOVED); + zebra_nhg_decrement_ref(oldnhe); + } else + zsend_nhg_notify(nhe->type, nhe->zapi_instance, + nhe->zapi_session, nhe->id, + ZAPI_NHG_REMOVE_FAIL); - /* Report error to daemon via ZAPI */ - if (newnhe == NULL) - zsend_nhg_notify(nhe->type, nhe->zapi_instance, - nhe->zapi_session, nhe->id, - ZAPI_NHG_FAIL_INSTALL); + } else { + newnhe = zebra_nhg_proto_add(nhe->id, nhe->type, + nhe->zapi_instance, + nhe->zapi_session, + &nhe->nhg, 0); + + /* Report error to daemon via ZAPI */ + if (newnhe == NULL) + zsend_nhg_notify(nhe->type, nhe->zapi_instance, + nhe->zapi_session, nhe->id, + ZAPI_NHG_FAIL_INSTALL); + } /* Free temp nhe - we own that memory. */ zebra_nhg_free(nhe); @@ -3339,7 +3358,8 @@ static int rib_meta_queue_nhg_ctx_add(struct meta_queue *mq, void *data) return 0; } -static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data) +static int rib_meta_queue_nhg_process(struct meta_queue *mq, void *data, + bool deletion) { struct nhg_hash_entry *nhe = NULL; uint8_t qindex = META_QUEUE_NHG; @@ -3354,6 +3374,7 @@ static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data) w->type = WQ_NHG_WRAPPER_TYPE_NHG; w->u.nhe = nhe; + w->deletion = deletion; listnode_add(mq->subq[qindex], w); mq->size++; @@ -3365,6 +3386,16 @@ static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data) return 0; } +static int rib_meta_queue_nhg_add(struct meta_queue *mq, void *data) +{ + return rib_meta_queue_nhg_process(mq, data, false); +} + +static int rib_meta_queue_nhg_del(struct meta_queue *mq, void *data) +{ + return rib_meta_queue_nhg_process(mq, data, true); +} + static int rib_meta_queue_evpn_add(struct meta_queue *mq, void *data) { listnode_add(mq->subq[META_QUEUE_EVPN], data); @@ -3472,6 +3503,17 @@ int rib_queue_nhe_add(struct nhg_hash_entry *nhe) return mq_add_handler(nhe, rib_meta_queue_nhg_add); } +/* + * Enqueue incoming nhg from proto daemon for processing + */ +int rib_queue_nhe_del(struct nhg_hash_entry *nhe) +{ + if (nhe == NULL) + return -1; + + return mq_add_handler(nhe, rib_meta_queue_nhg_del); +} + /* * Enqueue evpn route for processing */