On Thu, Dec 11, 2025 at 8:21 PM Dumitru Ceara <[email protected]> wrote:
> On 12/11/25 4:24 PM, Ales Musil wrote:
> > The struct route_data includes an ovs_list, by copying it into vector
> > we might be copying soon to be invalid pointers, in other words
> > pointers to stack variables. This wasn't an issue until now because
> > we didn't reference anything within the list, however it has become
> > an issue with the nexthop reference added later on.
> >
> > Fixes: 13d1a5cbae68 ("controller: Postpone the route deletion after the
> dump is done.")
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
> > v3: New patch in the series.
> > ---
>
> Hi Ales,
>
> Thanks for the patch!
>
> > controller/route-exchange-netlink.c | 72 ++++++++++++++---------------
> > controller/route-exchange-netlink.h | 7 ++-
> > controller/route.c | 12 +++++
> > controller/route.h | 3 ++
> > 4 files changed, 54 insertions(+), 40 deletions(-)
> >
> > diff --git a/controller/route-exchange-netlink.c
> b/controller/route-exchange-netlink.c
> > index 60bae4781..d6358343d 100644
> > --- a/controller/route-exchange-netlink.c
> > +++ b/controller/route-exchange-netlink.c
> > @@ -97,11 +97,10 @@ re_nl_delete_vrf(const char *ifname)
> >
> > static int
> > modify_route(uint32_t type, uint32_t flags_arg, uint32_t table_id,
> > - const struct in6_addr *dst, unsigned int plen,
> > - unsigned int priority)
> > + const struct advertise_route_entry *re)
> > {
> > uint32_t flags = NLM_F_REQUEST | NLM_F_ACK;
> > - bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(dst);
> > + bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&re->addr);
> > struct rtmsg *rt;
> > int err;
> >
> > @@ -119,15 +118,16 @@ modify_route(uint32_t type, uint32_t flags_arg,
> uint32_t table_id,
> > rt->rtm_protocol = RTPROT_OVN;
> > rt->rtm_type = RTN_BLACKHOLE;
> > rt->rtm_scope = RT_SCOPE_UNIVERSE;
> > - rt->rtm_dst_len = plen;
> > + rt->rtm_dst_len = re->plen;
> >
> > nl_msg_put_u32(&request, RTA_TABLE, table_id);
> > - nl_msg_put_u32(&request, RTA_PRIORITY, priority);
> > + nl_msg_put_u32(&request, RTA_PRIORITY, re->priority);
> >
> > if (is_ipv4) {
> > - nl_msg_put_be32(&request, RTA_DST,
> in6_addr_get_mapped_ipv4(dst));
> > + nl_msg_put_be32(&request, RTA_DST,
> > + in6_addr_get_mapped_ipv4(&re->addr));
> > } else {
> > - nl_msg_put_in6_addr(&request, RTA_DST, dst);
> > + nl_msg_put_in6_addr(&request, RTA_DST, &re->addr);
> > }
> >
> > if (VLOG_IS_DBG_ENABLED()) {
> > @@ -140,13 +140,13 @@ modify_route(uint32_t type, uint32_t flags_arg,
> uint32_t table_id,
> > }
> >
> > ds_put_format(&msg, "table %"PRIu32 " for prefix ", table_id);
> > - if (IN6_IS_ADDR_V4MAPPED(dst)) {
> > + if (IN6_IS_ADDR_V4MAPPED(&re->addr)) {
>
> We're changing this line anyway, I guess we could also just do:
>
> if (is_ipv4) {
>
> > ds_put_format(&msg, IP_FMT,
> > - IP_ARGS(in6_addr_get_mapped_ipv4(dst)));
> > + IP_ARGS(in6_addr_get_mapped_ipv4(&re->addr)));
> > } else {
> > - ipv6_format_addr(dst, &msg);
> > + ipv6_format_addr(&re->addr, &msg);
> > }
> > - ds_put_format(&msg, "/%u", plen);
> > + ds_put_format(&msg, "/%u", re->plen);
> >
> > VLOG_DBG("%s", ds_cstr(&msg));
> > ds_destroy(&msg);
> > @@ -159,8 +159,7 @@ modify_route(uint32_t type, uint32_t flags_arg,
> uint32_t table_id,
> > }
> >
> > int
> > -re_nl_add_route(uint32_t table_id, const struct in6_addr *dst,
> > - unsigned int plen, unsigned int priority)
> > +re_nl_add_route(uint32_t table_id, const struct advertise_route_entry
> *re)
> > {
> > if (!TABLE_ID_VALID(table_id)) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> > @@ -170,13 +169,11 @@ re_nl_add_route(uint32_t table_id, const struct
> in6_addr *dst,
> > return EINVAL;
> > }
> >
> > - return modify_route(RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL,
> table_id,
> > - dst, plen, priority);
> > + return modify_route(RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL,
> table_id, re);
> > }
> >
> > int
> > -re_nl_delete_route(uint32_t table_id, const struct in6_addr *dst,
> > - unsigned int plen, unsigned int priority)
> > +re_nl_delete_route(uint32_t table_id, const struct
> advertise_route_entry *re)
> > {
> > if (!TABLE_ID_VALID(table_id)) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> > @@ -186,7 +183,7 @@ re_nl_delete_route(uint32_t table_id, const struct
> in6_addr *dst,
> > return EINVAL;
> > }
> >
> > - return modify_route(RTM_DELROUTE, 0, table_id, dst, plen, priority);
> > + return modify_route(RTM_DELROUTE, 0, table_id, re);
> > }
> >
> > struct route_msg_handle_data {
> > @@ -248,12 +245,14 @@ handle_route_msg(const struct route_table_msg
> *msg, void *data)
> > return;
> > }
> >
> > + const struct advertise_route_entry re =
> > + advertise_route_from_route_data(rd);
> > if (handle_data->routes_to_advertise) {
> > - uint32_t hash = advertise_route_hash(&rd->rta_dst,
> rd->rtm_dst_len);
> > + uint32_t hash = advertise_route_hash(&re.addr, re.plen);
> > HMAP_FOR_EACH_WITH_HASH (ar, node, hash, handle_data->routes) {
> > - if (ipv6_addr_equals(&ar->addr, &rd->rta_dst)
> > - && ar->plen == rd->rtm_dst_len
> > - && ar->priority == rd->rta_priority) {
> > + if (ipv6_addr_equals(&ar->addr, &re.addr)
> > + && ar->plen == re.plen
> > + && ar->priority == re.priority) {
> > hmapx_find_and_delete(handle_data->routes_to_advertise,
> ar);
> > return;
> > }
> > @@ -261,27 +260,26 @@ handle_route_msg(const struct route_table_msg
> *msg, void *data)
> > }
> >
> > if (handle_data->stale_routes) {
> > - vector_push(handle_data->stale_routes, rd);
> > + vector_push(handle_data->stale_routes, &re);
> > }
> > }
> >
> > static int
> > -re_nl_delete_stale_routes(const struct vector *stale_routes)
> > +re_nl_delete_stale_routes(uint32_t table_id, const struct vector
> *stale_routes)
> > {
> > int ret = 0;
> >
> > - const struct route_data *rd;
> > - VECTOR_FOR_EACH_PTR (stale_routes, rd) {
> > - int err = re_nl_delete_route(rd->rta_table_id, &rd->rta_dst,
> > - rd->rtm_dst_len, rd->rta_priority);
> > + const struct advertise_route_entry *re;
> > + VECTOR_FOR_EACH_PTR (stale_routes, re) {
> > + int err = re_nl_delete_route(table_id, re);
> > if (err) {
> > char addr_s[INET6_ADDRSTRLEN + 1];
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 20);
> > VLOG_WARN_RL(&rl, "Delete route table_id=%"PRIu32" dst=%s
> plen=%d "
> > - "failed: %s", rd->rta_table_id,
> > + "failed: %s", table_id,
> > ipv6_string_mapped(
> > - addr_s, &rd->rta_dst) ? addr_s :
> "(invalid)",
> > - rd->rtm_dst_len,
> > + addr_s, &re->addr) ? addr_s : "(invalid)",
> > + re->plen,
> > ovs_strerror(err));
> > if (!ret) {
> > ret = err;
> > @@ -298,7 +296,8 @@ re_nl_sync_routes(uint32_t table_id, const struct
> hmap *routes,
> > const struct sbrec_datapath_binding *db)
> > {
> > struct hmapx routes_to_advertise =
> HMAPX_INITIALIZER(&routes_to_advertise);
> > - struct vector stale_routes = VECTOR_EMPTY_INITIALIZER(struct
> route_data);
> > + struct vector stale_routes =
> > + VECTOR_EMPTY_INITIALIZER(struct advertise_route_entry);
> > struct advertise_route_entry *ar;
> >
> > HMAP_FOR_EACH (ar, node, routes) {
> > @@ -318,14 +317,14 @@ re_nl_sync_routes(uint32_t table_id, const struct
> hmap *routes,
> > };
> > route_table_dump_one_table(table_id, handle_route_msg, &data);
> >
> > - int ret = re_nl_delete_stale_routes(&stale_routes);
> > + int ret = re_nl_delete_stale_routes(table_id, &stale_routes);
> >
> > /* Add any remaining routes in the routes_to_advertise hmapx to the
> > * system routing table. */
> > struct hmapx_node *hn;
> > HMAPX_FOR_EACH (hn, &routes_to_advertise) {
> > ar = hn->data;
> > - int err = re_nl_add_route(table_id, &ar->addr, ar->plen,
> ar->priority);
> > + int err = re_nl_add_route(table_id, ar);
> > if (err) {
> > char addr_s[INET6_ADDRSTRLEN + 1];
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 20);
> > @@ -352,7 +351,8 @@ re_nl_sync_routes(uint32_t table_id, const struct
> hmap *routes,
> > int
> > re_nl_cleanup_routes(uint32_t table_id)
> > {
> > - struct vector stale_routes = VECTOR_EMPTY_INITIALIZER(struct
> route_data);
> > + struct vector stale_routes =
> > + VECTOR_EMPTY_INITIALIZER(struct advertise_route_entry);
> > /* Remove routes from the system that are not in the host_routes
> hmap and
> > * remove entries from host_routes hmap that match routes already
> installed
> > * in the system. */
> > @@ -364,7 +364,7 @@ re_nl_cleanup_routes(uint32_t table_id)
> > };
> > route_table_dump_one_table(table_id, handle_route_msg, &data);
> >
> > - int ret = re_nl_delete_stale_routes(&stale_routes);
> > + int ret = re_nl_delete_stale_routes(table_id, &stale_routes);
> > vector_destroy(&stale_routes);
> >
> > return ret;
> > diff --git a/controller/route-exchange-netlink.h
> b/controller/route-exchange-netlink.h
> > index 1741f761d..ad1f0783b 100644
> > --- a/controller/route-exchange-netlink.h
> > +++ b/controller/route-exchange-netlink.h
> > @@ -52,10 +52,9 @@ struct re_nl_received_route_node {
> > int re_nl_create_vrf(const char *ifname, uint32_t table_id);
> > int re_nl_delete_vrf(const char *ifname);
> >
> > -int re_nl_add_route(uint32_t table_id, const struct in6_addr *dst,
> > - unsigned int plen, unsigned int priority);
> > -int re_nl_delete_route(uint32_t table_id, const struct in6_addr *dst,
> > - unsigned int plen, unsigned int priority);
> > +int re_nl_add_route(uint32_t table_id, const struct
> advertise_route_entry *);
> > +int re_nl_delete_route(uint32_t table_id,
> > + const struct advertise_route_entry *);
> >
> > int re_nl_sync_routes(uint32_t table_id, const struct hmap *routes,
> > struct vector *learned_routes,
> > diff --git a/controller/route.c b/controller/route.c
> > index 093306c5b..dcf1984ab 100644
> > --- a/controller/route.c
> > +++ b/controller/route.c
> > @@ -31,6 +31,8 @@
> > #include "local_data.h"
> > #include "route.h"
> >
> > +#include "route-table.h"
> > +
> > VLOG_DEFINE_THIS_MODULE(exchange);
> >
> > #define PRIORITY_DEFAULT 1000
> > @@ -49,6 +51,16 @@ advertise_route_hash(const struct in6_addr *dst,
> unsigned int plen)
> > return hash_int(plen, hash);
> > }
> >
> > +struct advertise_route_entry
> > +advertise_route_from_route_data(const struct route_data *rd)
> > +{
> > + return (struct advertise_route_entry) {
> > + .addr = rd->rta_dst,
> > + .plen = rd->rtm_dst_len,
> > + .priority = rd->rta_priority,
> > + };
> > +}
> > +
> > const struct sbrec_port_binding*
> > route_exchange_find_port(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> > const struct sbrec_chassis *chassis,
> > diff --git a/controller/route.h b/controller/route.h
> > index 5c800f4ab..a1dd61d53 100644
> > --- a/controller/route.h
> > +++ b/controller/route.h
> > @@ -27,6 +27,7 @@
> >
> > struct hmap;
> > struct ovsdb_idl_index;
> > +struct route_data;
> > struct sbrec_chassis;
> > struct sbrec_port_binding;
> > struct sbrec_datapath_binding;
> > @@ -89,6 +90,8 @@ const struct sbrec_port_binding
> *route_exchange_find_port(
> > const struct sbrec_port_binding *pb,
> > const char **dynamic_routing_port_name);
> > uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned int
> plen);
> > +struct advertise_route_entry
> > +advertise_route_from_route_data(const struct route_data *);
> > void route_run(struct route_ctx_in *, struct route_ctx_out *);
> > void route_cleanup(struct hmap *announce_routes);
> > uint32_t route_get_table_id(const struct sbrec_datapath_binding *);
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara <[email protected]>
>
> Regards,
> Dumitru
>
>
>
Thank you Dumitru,
I went ahead, addressed the comment, merged this into main and backported
down to 25.09.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
