Skip to content

Commit

Permalink
controller: Store src_mac, src_ip in svc_monitor struct.
Browse files Browse the repository at this point in the history 
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. Acquire 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.

Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413198.html
CC: Numan Siddique <numans@ovn.org>
Acked-by: Ales Musil <amusil@redhat.com>
Fixes: 8be01f4 ("Send service monitor health checks")
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
  • Loading branch information
@odivlad @numansiddique
odivlad authored and numansiddique committed Jun 12, 2024
1 parent 4e3115f commit a0a5dd8
Showing 1 changed file with 16 additions and 21 deletions.
37 changes: 16 additions & 21 deletions controller/pinctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7275,6 +7275,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;

Expand Down Expand Up @@ -7469,6 +7472,9 @@ 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);
ip46_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;
Expand Down Expand Up @@ -8227,19 +8233,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, &eth_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);
}

Expand Down Expand Up @@ -8295,24 +8296,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, &eth_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);
}

Expand Down

0 comments on commit a0a5dd8

Please sign in to comment.