These structure members are read in pinctrl_handler() in a separare thread.
This is unsafe: when IDL is re-connected or some IDL objects are freed
after svc_monitors list with svc_monitor structures, which point to
sbrec_service_monitor is initialized, sb_svc_mon structure property can
point to wrong address, which then leads to segmentation fault in
svc_monitor_send_tcp_health_check__() and
svc_monitor_send_udp_health_check() on accessing svc_mon->sb_svc_mon.
Imagine situation:
Main ovn-controller thread:
1. Connects to SB and fills IDL with DB contents.
2. run pinctrl_run() with pinctrl mutex and sync_svc_monitors() as a part
of it.
3. Release mutex.
...
4. Loss of OVNSB connection for any reason (network outage/inactivity probe
timeout/etc), start new main-loop iteration, re-initialize IDL in
ovsdb_idl_run() (which probably will destroy previous IDL objects).
...
pinctrl thread:
5. Awake from poll_block().
... run new iteration in its main loop ...
6. Aqcuire mutex lock.
7. Run svc_monitors_run(), run svc_monitor_send_tcp_health_check__() or
svc_monitor_send_udp_health_check(), which try to access IDL objects
with outdated pointers.
8. Segmentation fault:
Stack trace of thread 212406:
>> __GI_strlen (libc.so.6)
>> inet_pton (libc.so.6)
>> ip_parse (ovn-controller)
>> svc_monitor_send_tcp_health_check__.part.37 (ovn-controller)
>> svc_monitor_send_health_check (ovn-controller)
>> pinctrl_handler (ovn-controller)
>> ovsthread_wrapper (ovn-controller)
>> start_thread (libpthread.so.0)
>> __clone (libc.so.6)
This patch removes unsafe access to IDL objects from pinctrl thread.
New svc_monitor structure members are used to store needed data.
CC: Numan Siddique <num...@ovn.org>
Fixes: 8be01f4a5329 ("Send service monitor health checks")
Signed-off-by: Vladislav Odintsov <odiv...@gmail.com>
---
controller/pinctrl.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 6a2c3dc68..b843edb35 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -7307,6 +7307,9 @@ struct svc_monitor {
long long int timestamp;
bool is_ip6;
+ struct eth_addr src_mac;
+ struct in6_addr src_ip;
+
long long int wait_time;
long long int next_send_time;
@@ -7501,6 +7504,15 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
svc_mon->n_success = 0;
svc_mon->n_failures = 0;
+ eth_addr_from_string(sb_svc_mon->src_mac, &svc_mon->src_mac);
+ if (is_ipv4) {
+ ovs_be32 ip4_src;
+ ip_parse(sb_svc_mon->src_ip, &ip4_src);
+ svc_mon->src_ip = in6_addr_mapped_ipv4(ip4_src);
+ } else {
+ ipv6_parse(sb_svc_mon->src_ip, &svc_mon->src_ip);
+ }
+
hmap_insert(&svc_monitors_map, &svc_mon->hmap_node, hash);
ovs_list_push_back(&svc_monitors, &svc_mon->list_node);
changed = true;
@@ -8259,19 +8271,14 @@ svc_monitor_send_tcp_health_check__(struct rconn
*swconn,
struct dp_packet packet;
dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
- struct eth_addr eth_src;
- eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, ð_src);
if (svc_mon->is_ip6) {
- struct in6_addr ip6_src;
- ipv6_parse(svc_mon->sb_svc_mon->src_ip, &ip6_src);
- pinctrl_compose_ipv6(&packet, eth_src, svc_mon->ea,
- &ip6_src, &svc_mon->ip, IPPROTO_TCP,
+ pinctrl_compose_ipv6(&packet, svc_mon->src_mac, svc_mon->ea,
+ &svc_mon->src_ip, &svc_mon->ip, IPPROTO_TCP,
63, TCP_HEADER_LEN);
} else {
- ovs_be32 ip4_src;
- ip_parse(svc_mon->sb_svc_mon->src_ip, &ip4_src);
- pinctrl_compose_ipv4(&packet, eth_src, svc_mon->ea,
- ip4_src, in6_addr_get_mapped_ipv4(&svc_mon->ip),
+ pinctrl_compose_ipv4(&packet, svc_mon->src_mac, svc_mon->ea,
+ in6_addr_get_mapped_ipv4(&svc_mon->src_ip),
+ in6_addr_get_mapped_ipv4(&svc_mon->ip),
IPPROTO_TCP, 63, TCP_HEADER_LEN);
}
@@ -8327,24 +8334,18 @@ svc_monitor_send_udp_health_check(struct rconn *swconn,
struct svc_monitor *svc_mon,
ovs_be16 udp_src)
{
- struct eth_addr eth_src;
- eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, ð_src);
-
uint64_t packet_stub[128 / 8];
struct dp_packet packet;
dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
if (svc_mon->is_ip6) {
- struct in6_addr ip6_src;
- ipv6_parse(svc_mon->sb_svc_mon->src_ip, &ip6_src);
- pinctrl_compose_ipv6(&packet, eth_src, svc_mon->ea,
- &ip6_src, &svc_mon->ip, IPPROTO_UDP,
+ pinctrl_compose_ipv6(&packet, svc_mon->src_mac, svc_mon->ea,
+ &svc_mon->src_ip, &svc_mon->ip, IPPROTO_UDP,
63, UDP_HEADER_LEN + 8);
} else {
- ovs_be32 ip4_src;
- ip_parse(svc_mon->sb_svc_mon->src_ip, &ip4_src);
- pinctrl_compose_ipv4(&packet, eth_src, svc_mon->ea,
- ip4_src, in6_addr_get_mapped_ipv4(&svc_mon->ip),
+ pinctrl_compose_ipv4(&packet, svc_mon->src_mac, svc_mon->ea,
+ in6_addr_get_mapped_ipv4(&svc_mon->src_ip),
+ in6_addr_get_mapped_ipv4(&svc_mon->ip),
IPPROTO_UDP, 63, UDP_HEADER_LEN + 8);
}
--
2.44.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
