Message ID | 20240522121913.609332-1-odivlad@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v2] controller: Store src_mac, src_ip in svc_monitor struct. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Again adding forgotten tag: Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413198.html regards, Vladislav Odintsov On 22.05.2024, 15:19, "Vladislav Odintsov" <odivlad@gmail.com> wrote: 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. CC: Numan Siddique <numans@ovn.org> Acked-by: Ales Musil <amusil@redhat.com> Fixes: 8be01f4a5329 ("Send service monitor health checks") Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- v1 -> v2: - Addressed Ales's comment: replaced ip_parse() & ipv6_parse() with ip46_parse(). --- controller/pinctrl.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 6a2c3dc68..0178ac6cc 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,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; @@ -8259,19 +8265,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 +8328,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
On Wed, May 22, 2024 at 8:34 AM Vladislav Odintsov <odivlad@gmail.com> wrote: > > Again adding forgotten tag: > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413198.html > > regards, Thanks for fixing this. I applied this patch to main and will backport to other branches once the CI passes for those branches in my github repo. Numan > > Vladislav Odintsov > > On 22.05.2024, 15:19, "Vladislav Odintsov" <odivlad@gmail.com> wrote: > > 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. > > CC: Numan Siddique <numans@ovn.org> > Acked-by: Ales Musil <amusil@redhat.com> > Fixes: 8be01f4a5329 ("Send service monitor health checks") > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > > --- > v1 -> v2: > - Addressed Ales's comment: replaced ip_parse() & ipv6_parse() with > ip46_parse(). > --- > controller/pinctrl.c | 37 ++++++++++++++++--------------------- > 1 file changed, 16 insertions(+), 21 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 6a2c3dc68..0178ac6cc 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,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; > @@ -8259,19 +8265,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 +8328,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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 6a2c3dc68..0178ac6cc 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,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; @@ -8259,19 +8265,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 +8328,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); }