From: Guillaume Boutry <[email protected]> Neighbor synchronization ignores errors from the Netlink dump and can process an incomplete neighbor table as a successful result. This has been present since ad3a0a55b added Netlink neighbor synchronization.
Collect the complete dump before processing observations or changing kernel entries. For unfiltered dumps, check the family and interface header before parsing attributes so unrelated non-Ethernet neighbors cannot cause a false parse error. Keep malformed in-scope records fatal. Cover allocation failures, unrelated records, malformed input and late dump errors, including preservation of caller observations and prevention of kernel writes. Include a successful publication control. Scripted failure tests wrap only the test executable's Netlink transport. Normal commands use the real functions. Production binaries do not use these wrappers. Assisted-By: Codex (gpt-6.1-sol) Signed-off-by: Guillaume Boutry <[email protected]> --- Neighbor synchronization consumes an incomplete neighbor table when its Netlink dump fails. ne_table_dump_one_ifindex() ignores nl_dump_done()'s return value and processes observations before checking completion. The caller can then publish partial observations or reconcile kernel entries using an incomplete view. A failed dump should return its error without changing caller observations or kernel neighbors. Linux documents that interrupted dumps can miss entries and must be retried: https://docs.kernel.org/userspace-api/netlink/intro.html#dump-consistency Reproducer ---------- The patch adds a real EMFILE case and scripted late failures after valid entries, with a nonempty desired set and a sentinel observation: make check-kernel TESTSUITEFLAGS='-d -k netlink-dump-error' To exercise the old behavior, keep the test additions and use the original production implementation. The patch buffers observations until the dump succeeds. It also skips unrelated family/interface records before Ethernet parsing, while rejecting malformed records within the requested scope. Validation ---------- OVN 26.09.90, upstream base: 1c8d7419a73743f79ff54102c6b66f6c8061b756 The Werror build and Linux Netlink selection passed (10 tests, zero skips) on source tree 91b48f512. The standalone export applies to the base above; it was not rebuilt separately. The tests cover partial-dump rollback, malformed input and a successful add/delete/learn control. No full-suite or older-kernel/IPoIB hardware run was performed. controller/neighbor-exchange-netlink.c | 85 +++++++-- tests/automake.mk | 8 + tests/system-ovn-netlink.at | 29 +++ tests/test-ovn-netlink.c | 233 +++++++++++++++++++++++++ 4 files changed, 339 insertions(+), 16 deletions(-) diff --git a/controller/neighbor-exchange-netlink.c b/controller/neighbor-exchange-netlink.c index c6b21e7a2..9cd50037f 100644 --- a/controller/neighbor-exchange-netlink.c +++ b/controller/neighbor-exchange-netlink.c @@ -14,6 +14,7 @@ */ #include <config.h> +#include <errno.h> #include <stdbool.h> #include <linux/if_ether.h> #include <linux/rtnetlink.h> @@ -48,10 +49,13 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); /* Inspired from route_table_dump_one_table() in OVS. */ typedef void ne_table_handle_msg_callback(const struct ne_table_msg *, void *aux); -static void ne_table_dump_one_ifindex(unsigned char address_family, +static int ne_table_dump_one_ifindex(unsigned char address_family, int32_t if_index, ne_table_handle_msg_callback *, void *aux); +static int ne_nl_dump_neigh(uint8_t family, int32_t if_index, + struct vector *neighbors); + struct ne_msg_handle_data { /* Stores 'struct advertise_neighbor_entry'. */ struct hmapx *neighbors_to_advertise; @@ -111,7 +115,21 @@ ne_nl_sync_neigh(uint8_t family, int32_t if_index, .stale_neighbors = &stale_neighbors, .neighbors = neighbors, }; - ne_table_dump_one_ifindex(family, if_index, handle_ne_msg, &data); + struct vector snapshot = + VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh); + ret = ne_nl_dump_neigh(family, if_index, &snapshot); + if (ret) { + goto out; + } + + struct ne_nl_received_neigh *ne; + VECTOR_FOR_EACH_PTR (&snapshot, ne) { + struct ne_table_msg msg = { + .nlmsg_type = RTM_NEWNEIGH, + .nd = *ne, + }; + handle_ne_msg(&msg, &data); + } /* Add any remaining neighbors in the neighbors_to_advertise hmapx to the * system table. */ @@ -141,7 +159,6 @@ ne_nl_sync_neigh(uint8_t family, int32_t if_index, } /* Remove any stale neighbors from the system table. */ - struct ne_nl_received_neigh *ne; VECTOR_FOR_EACH_PTR (&stale_neighbors, ne) { int err = ne_nl_del_neigh(ne->if_index, ne->family, &ne->lladdr, &ne->addr, @@ -163,6 +180,8 @@ ne_nl_sync_neigh(uint8_t family, int32_t if_index, } } +out: + vector_destroy(&snapshot); hmapx_destroy(&neighbors_to_advertise); vector_destroy(&stale_neighbors); return ret; @@ -220,6 +239,31 @@ ne_is_valid_static_arp(struct ne_nl_received_neigh *ne) } static void +collect_ne_msg(const struct ne_table_msg *msg, void *data) +{ + vector_push(data, &msg->nd); +} + +/* Appends a complete, read-only snapshot of neighbors to 'neighbors'. On + * failure, leaves 'neighbors' unchanged and returns an errno value. */ +static int +ne_nl_dump_neigh(uint8_t family, int32_t if_index, struct vector *neighbors) +{ + struct vector snapshot = + VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh); + int error = ne_table_dump_one_ifindex(family, if_index, collect_ne_msg, + &snapshot); + if (!error) { + const struct ne_nl_received_neigh *ne; + VECTOR_FOR_EACH_PTR (&snapshot, ne) { + vector_push(neighbors, ne); + } + } + vector_destroy(&snapshot); + return error; +} + +static int ne_table_dump_one_ifindex(unsigned char address_family, int32_t if_index, ne_table_handle_msg_callback *handle_msg_cb, void *aux) @@ -228,6 +272,7 @@ ne_table_dump_one_ifindex(unsigned char address_family, int32_t if_index, struct ofpbuf request, reply, buf; struct ndmsg *rq_msg; struct nl_dump dump; + int error = 0; uint8_t request_stub[NETNL_REQ_BUFFER_SIZE]; ofpbuf_use_stub(&request, request_stub, sizeof request_stub); @@ -244,24 +289,32 @@ ne_table_dump_one_ifindex(unsigned char address_family, int32_t if_index, ofpbuf_use_stub(&buf, reply_stub, sizeof reply_stub); while (nl_dump_next(&dump, &reply, &buf)) { - struct ne_table_msg msg; + const struct nlmsghdr *nlmsghdr = ofpbuf_at(&reply, 0, NLMSG_HDRLEN); + const struct ndmsg *nd = ofpbuf_at(&reply, NLMSG_HDRLEN, sizeof *nd); + if (!nlmsghdr || !nd) { + error = EPROTO; + break; + } - if (ne_table_parse(&reply, &msg)) { - struct nlmsghdr *nlmsghdr = nl_msg_nlmsghdr(&reply); + /* Some kernels return unfiltered dumps. Check the header before + * parsing attributes so unrelated non-Ethernet neighbors cannot + * abort synchronization of the requested interface. */ + if (!(nlmsghdr->nlmsg_flags & NLM_F_DUMP_FILTERED) + && (nd->ndm_family != address_family + || (if_index && nd->ndm_ifindex != if_index))) { + continue; + } - /* Older kernels do not support filtering. If so, we - * filter ourselves. */ - if (!(nlmsghdr->nlmsg_flags & NLM_F_DUMP_FILTERED)) { - if (msg.nd.family != address_family - || (if_index && msg.nd.if_index != if_index)) { - continue; - } - } - handle_msg_cb(&msg, aux); + struct ne_table_msg msg; + if (!ne_table_parse(&reply, &msg)) { + error = EPROTO; + break; } + handle_msg_cb(&msg, aux); } ofpbuf_uninit(&buf); - nl_dump_done(&dump); + int dump_error = nl_dump_done(&dump); + return dump_error ? dump_error : error; } static int diff --git a/tests/automake.mk b/tests/automake.mk index 8f7a8c4fa..4579bd914 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -291,6 +291,14 @@ tests_ovstest_SOURCES = \ northd/test-ipam.c if HAVE_NETLINK +# Deterministic dump failures exercise the production synchronization path. +# Only ovstest uses these wrappers; inactive scripts call the real transport. +tests_ovstest_LDFLAGS = $(AM_LDFLAGS) \ + -Wl,--wrap=nl_dump_start \ + -Wl,--wrap=nl_dump_next \ + -Wl,--wrap=nl_dump_done \ + -Wl,--wrap=nl_transact + tests_ovstest_SOURCES += \ controller/host-if-monitor.c \ controller/host-if-monitor.h \ diff --git a/tests/system-ovn-netlink.at b/tests/system-ovn-netlink.at index 69c25d6e9..78727785e 100644 --- a/tests/system-ovn-netlink.at +++ b/tests/system-ovn-netlink.at @@ -1,5 +1,34 @@ AT_BANNER([system-ovn-netlink]) +AT_SETUP([sync netlink neighbors - failed dump]) +AT_KEYWORDS([netlink-neighbors netlink-dump-error]) +AT_CHECK([ovstest test-ovn-netlink neighbor-sync-error], [0], [], [ignore]) +AT_CLEANUP + +AT_SETUP([sync netlink neighbors - filter unrelated dump records]) +AT_KEYWORDS([netlink-neighbors netlink-dump-error]) +check ovstest test-ovn-netlink neighbor-dump-script other-interface +check ovstest test-ovn-netlink neighbor-dump-script other-family +AT_CLEANUP + +AT_SETUP([sync netlink neighbors - malformed dump records]) +AT_KEYWORDS([netlink-neighbors netlink-dump-error]) +check ovstest test-ovn-netlink neighbor-dump-script malformed +check ovstest test-ovn-netlink neighbor-dump-script filtered-malformed +check ovstest test-ovn-netlink neighbor-dump-script truncated +AT_CLEANUP + +AT_SETUP([sync netlink neighbors - partial dump rollback]) +AT_KEYWORDS([netlink-neighbors netlink-dump-error]) +check ovstest test-ovn-netlink neighbor-dump-script late-error +check ovstest test-ovn-netlink neighbor-dump-script parse-and-dump-error +AT_CLEANUP + +AT_SETUP([sync netlink neighbors - complete dump publication]) +AT_KEYWORDS([netlink-neighbors netlink-dump-error]) +check ovstest test-ovn-netlink neighbor-dump-script complete +AT_CLEANUP + AT_SETUP([sync netlink neighbors - learn VXLAN VTEP neighbors]) AT_KEYWORDS([netlink-neighbors]) diff --git a/tests/test-ovn-netlink.c b/tests/test-ovn-netlink.c index f75a6f295..cc9a33a3a 100644 --- a/tests/test-ovn-netlink.c +++ b/tests/test-ovn-netlink.c @@ -14,7 +14,12 @@ */ #include <config.h> +#include <errno.h> +#include <linux/rtnetlink.h> +#include <sys/resource.h> +#include "netlink.h" +#include "netlink-socket.h" #include "openvswitch/hmap.h" #include "route-table.h" #include "packets.h" @@ -29,6 +34,231 @@ #include "controller/route.h" #include "controller/route-exchange-netlink.h" +/* Script only the transport used by neighbor synchronization. The normal + * ovstest commands delegate to the real Netlink functions. Production + * binaries do not use these linker wrappers. */ +static struct { + bool active; + bool started; + bool finished; + struct ofpbuf messages[3]; + size_t n_messages; + size_t next; + int error; + unsigned int adds; + unsigned int deletes; +} neighbor_dump; + +void __real_nl_dump_start(struct nl_dump *, int, const struct ofpbuf *); +bool __real_nl_dump_next(struct nl_dump *, struct ofpbuf *, struct ofpbuf *); +int __real_nl_dump_done(struct nl_dump *); +int __real_nl_transact(int, const struct ofpbuf *, struct ofpbuf **); +void __wrap_nl_dump_start(struct nl_dump *, int, const struct ofpbuf *); +bool __wrap_nl_dump_next(struct nl_dump *, struct ofpbuf *, struct ofpbuf *); +int __wrap_nl_dump_done(struct nl_dump *); +int __wrap_nl_transact(int, const struct ofpbuf *, struct ofpbuf **); + +void +__wrap_nl_dump_start(struct nl_dump *dump, int protocol, + const struct ofpbuf *request) +{ + if (!neighbor_dump.active) { + __real_nl_dump_start(dump, protocol, request); + return; + } + ovs_assert(protocol == NETLINK_ROUTE); + ovs_assert(nl_msg_nlmsghdr(request)->nlmsg_type == RTM_GETNEIGH); + ovs_assert(!neighbor_dump.started); + neighbor_dump.started = true; +} + +bool +__wrap_nl_dump_next(struct nl_dump *dump, struct ofpbuf *reply, + struct ofpbuf *buf) +{ + if (!neighbor_dump.active) { + return __real_nl_dump_next(dump, reply, buf); + } + ovs_assert(neighbor_dump.started && !neighbor_dump.finished); + if (neighbor_dump.next == neighbor_dump.n_messages) { + return false; + } + *reply = neighbor_dump.messages[neighbor_dump.next++]; + return true; +} + +int +__wrap_nl_dump_done(struct nl_dump *dump) +{ + if (!neighbor_dump.active) { + return __real_nl_dump_done(dump); + } + ovs_assert(neighbor_dump.started && !neighbor_dump.finished); + neighbor_dump.finished = true; + return neighbor_dump.error; +} + +int +__wrap_nl_transact(int protocol, const struct ofpbuf *request, + struct ofpbuf **reply) +{ + if (!neighbor_dump.active) { + return __real_nl_transact(protocol, request, reply); + } + ovs_assert(protocol == NETLINK_ROUTE && !reply); + const struct ndmsg *nd = ofpbuf_at(request, NLMSG_HDRLEN, sizeof *nd); + ovs_assert(nd && nd->ndm_family == AF_INET && nd->ndm_ifindex == 42); + switch (nl_msg_nlmsghdr(request)->nlmsg_type) { + case RTM_NEWNEIGH: + neighbor_dump.adds++; + break; + case RTM_DELNEIGH: + neighbor_dump.deletes++; + break; + default: + OVS_NOT_REACHED(); + } + return 0; +} + +static struct ofpbuf * +neighbor_dump_message(void) +{ + ovs_assert(neighbor_dump.n_messages < ARRAY_SIZE(neighbor_dump.messages)); + struct ofpbuf *message = + &neighbor_dump.messages[neighbor_dump.n_messages++]; + ofpbuf_init(message, 128); + nl_msg_put_nlmsghdr(message, 0, RTM_NEWNEIGH, 0); + return message; +} + +static void +neighbor_dump_add(uint8_t family, int32_t if_index, uint16_t state, + uint16_t flags, size_t lladdr_len, uint8_t last) +{ + struct ofpbuf *message = neighbor_dump_message(); + struct ndmsg *nd = ofpbuf_put_zeros(message, sizeof *nd); + nd->ndm_family = family; + nd->ndm_ifindex = if_index; + nd->ndm_state = state; + ovs_be32 ip = htonl(0xc0000200 | last); + if (family == AF_INET) { + nl_msg_put_be32(message, NDA_DST, ip); + } else { + struct in6_addr ip6 = in6_addr_mapped_ipv4(ip); + nl_msg_put_in6_addr(message, NDA_DST, &ip6); + } + uint8_t mac[20] = { 0x02, 0, 0, 0, 0, last }; + ovs_assert(lladdr_len <= sizeof mac); + nl_msg_put_unspec(message, NDA_LLADDR, mac, lladdr_len); + struct nlmsghdr *header = nl_msg_nlmsghdr(message); + header->nlmsg_flags = flags; + header->nlmsg_len = message->size; +} + +static void +test_neighbor_dump_script(struct ovs_cmdl_context *ctx) +{ + const char *scenario = ctx->argv[1]; + bool complete = !strcmp(scenario, "complete"); + bool other_interface = !strcmp(scenario, "other-interface"); + bool other_family = !strcmp(scenario, "other-family"); + bool malformed = !strcmp(scenario, "malformed"); + bool filtered = !strcmp(scenario, "filtered-malformed"); + bool truncated = !strcmp(scenario, "truncated"); + bool late_error = !strcmp(scenario, "late-error"); + bool both_errors = !strcmp(scenario, "parse-and-dump-error"); + ovs_assert(complete || other_interface || other_family || malformed + || filtered || truncated || late_error || both_errors); + + struct hmap neighbors = HMAP_INITIALIZER(&neighbors); + struct advertise_neighbor_entry desired = { + .lladdr = ETH_ADDR_C(02,00,00,00,00,03), + .addr = in6_addr_mapped_ipv4(htonl(0xc0000203)), + }; + hmap_insert(&neighbors, &desired.node, + advertise_neigh_hash(&desired.lladdr, &desired.addr)); + struct vector learned = + VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh); + struct ne_nl_received_neigh sentinel = { .if_index = 17 }; + vector_push(&learned, &sentinel); + + memset(&neighbor_dump, 0, sizeof neighbor_dump); + neighbor_dump.active = true; + /* A learned row and an obsolete OVN-owned row precede the fault. A + * successful sync must publish the former, add the desired row and + * delete the latter; a failed dump must do none of those operations. */ + neighbor_dump_add(AF_INET, 42, NUD_REACHABLE, 0, ETH_ADDR_LEN, 1); + neighbor_dump_add(AF_INET, 42, NUD_NOARP, 0, ETH_ADDR_LEN, 2); + if (other_interface || other_family || malformed || filtered + || both_errors) { + neighbor_dump_add(other_family ? AF_INET6 : AF_INET, + other_interface ? 43 : 42, NUD_REACHABLE, + filtered ? NLM_F_DUMP_FILTERED : 0, 20, 4); + } else if (truncated) { + struct ofpbuf *message = neighbor_dump_message(); + ofpbuf_put_zeros(message, sizeof(struct ndmsg) - 1); + nl_msg_nlmsghdr(message)->nlmsg_len = message->size; + } + neighbor_dump.error = late_error || both_errors ? EINTR : 0; + + int error = ne_nl_sync_neigh(AF_INET, 42, &neighbors, &learned); + ovs_assert(neighbor_dump.started && neighbor_dump.finished); + neighbor_dump.active = false; + bool success = complete || other_interface || other_family; + int expected_error = success ? 0 : EPROTO; + if (late_error || both_errors) { + expected_error = EINTR; + } + ovs_assert(error == expected_error); + ovs_assert(neighbor_dump.adds == (success ? 1 : 0)); + ovs_assert(neighbor_dump.deletes == (success ? 1 : 0)); + ovs_assert(hmap_count(&neighbors) == 1); + ovs_assert(learned.len == (success ? 2 : 1)); + const struct ne_nl_received_neigh *rows = learned.buffer; + ovs_assert(!memcmp(&rows[0], &sentinel, sizeof sentinel)); + if (success) { + struct eth_addr mac = ETH_ADDR_C(02,00,00,00,00,01); + struct in6_addr ip = in6_addr_mapped_ipv4(htonl(0xc0000201)); + ovs_assert(rows[1].if_index == 42 && rows[1].family == AF_INET); + ovs_assert(rows[1].state == NUD_REACHABLE); + ovs_assert(eth_addr_equals(rows[1].lladdr, mac)); + ovs_assert(ipv6_addr_equals(&rows[1].addr, &ip)); + } + for (size_t i = 0; i < neighbor_dump.n_messages; i++) { + ofpbuf_uninit(&neighbor_dump.messages[i]); + } + vector_destroy(&learned); + hmap_destroy(&neighbors); +} + +/* A failed dump must not be reported as successful synchronization. */ +static void +test_neighbor_sync_error(struct ovs_cmdl_context *ctx OVS_UNUSED) +{ + struct hmap neighbors = HMAP_INITIALIZER(&neighbors); + struct vector learned = + VECTOR_EMPTY_INITIALIZER(struct ne_nl_received_neigh); + struct ne_nl_received_neigh sentinel = { .if_index = 42 }; + vector_push(&learned, &sentinel); + + /* This command starts without pooled Netlink sockets. Exhaust its file + * descriptor allowance so the real dump fails before receiving data. */ + struct rlimit original; + ovs_assert(!getrlimit(RLIMIT_NOFILE, &original)); + struct rlimit limited = original; + limited.rlim_cur = 0; + ovs_assert(!setrlimit(RLIMIT_NOFILE, &limited)); + int error = ne_nl_sync_neigh(AF_INET, 0, &neighbors, &learned); + ovs_assert(!setrlimit(RLIMIT_NOFILE, &original)); + + ovs_assert(error == EMFILE); + ovs_assert(learned.len == 1); + ovs_assert(!memcmp(learned.buffer, &sentinel, sizeof sentinel)); + vector_destroy(&learned); + hmap_destroy(&neighbors); +} + static void test_neighbor_sync(struct ovs_cmdl_context *ctx) { @@ -488,6 +718,9 @@ test_ovn_netlink(int argc, char *argv[]) { set_program_name(argv[0]); static const struct ovs_cmdl_command commands[] = { + {"neighbor-dump-script", NULL, 1, 1, + test_neighbor_dump_script, OVS_RO}, + {"neighbor-sync-error", NULL, 0, 0, test_neighbor_sync_error, OVS_RO}, {"neighbor-sync", NULL, 2, INT_MAX, test_neighbor_sync, OVS_RO}, {"neighbor-table-notify", NULL, 1, 1, test_neighbor_table_notify, OVS_RO}, _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
