diff mbox series

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

Message ID 20240514192525.524142-1-odivlad@gmail.com
State Changes Requested, archived
Headers show
Series [ovs-dev] 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 fail github build: failed

Commit Message

Vladislav Odintsov May 14, 2024, 7:25 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. 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 <numans@ovn.org>
Fixes: 8be01f4a5329 ("Send service monitor health checks")
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
---
 controller/pinctrl.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

Comments

Vladislav Odintsov May 14, 2024, 7:56 p.m. UTC | #1 
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413198.html

> On 14 May 2024, at 22:25, 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. 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 <numans@ovn.org>
> Fixes: 8be01f4a5329 ("Send service monitor health checks")
> Signed-off-by: Vladislav Odintsov <odivlad@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, &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 +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, &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
> 


Regards,
Vladislav Odintsov
Ales Musil May 22, 2024, 7:59 a.m. UTC | #2 
On Tue, May 14, 2024 at 9:25 PM 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. 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 <numans@ovn.org>
> Fixes: 8be01f4a5329 ("Send service monitor health checks")
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
> ---
>

Hi Vladislav,

thank you for the fix. I have one comment down below.


>  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);
> +            }
> +
>

The whole if else block can be replaced with ip46_parse().

             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, &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 +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, &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
>
>
Other than that it looks good, feel free to add "Acked-by: Ales Musil <
amusil@redhat.com>" to v2.

Thanks,
Ales
Vladislav Odintsov May 22, 2024, 12:22 p.m. UTC | #3 
Hi Ales!

Thanks for the review.
I've addressed requested changes in v2 with your Acked-by added:

https://patchwork.ozlabs.org/project/ovn/patch/20240522121913.609332-1-odivlad@gmail.com/

regards,
 
Vladislav Odintsov

On 22.05.2024, 10:59, "dev on behalf of Ales Musil" <ovs-dev-bounces@openvswitch.org on behalf of amusil@redhat.com> wrote:

    On Tue, May 14, 2024 at 9:25 PM 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. 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 <numans@ovn.org>
    > Fixes: 8be01f4a5329 ("Send service monitor health checks")
    > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
    > ---
    >

    Hi Vladislav,

    thank you for the fix. I have one comment down below.


    >  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);
    > +            }
    > +
    >

    The whole if else block can be replaced with ip46_parse().

                 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, &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 +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, &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
    >
    >
    Other than that it looks good, feel free to add "Acked-by: Ales Musil <
    amusil@redhat.com>" to v2.

    Thanks,
    Ales

    -- 

    Ales Musil

    Senior Software Engineer - OVN Core

    Red Hat EMEA <https://www.redhat.com>

    amusil@redhat.com
    <https://red.ht/sig>
    _______________________________________________
    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..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, &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 +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, &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);
     }