diff mbox series

[ovs-dev,v2] controller: Store src_mac, src_ip in svc_monitor struct.

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

Checks

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

Commit Message

Vladislav Odintsov May 22, 2024, 12:19 p.m. UTC 
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(-)

Comments

Vladislav Odintsov May 22, 2024, 12:33 p.m. UTC | #1 
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, &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);
         }

    @@ -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, &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);
         }

    -- 
    2.44.0
Numan Siddique June 12, 2024, 2:26 p.m. UTC | #2 
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, &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);
>          }
>
>     @@ -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, &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);
>          }
>
>     --
>     2.44.0
>
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

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, &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);
     }
 
@@ -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, &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);
     }