Re: [ovs-dev] [PATCH ovn] controller: Store src_mac, src_ip in svc_monitor struct.
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-odiv...@gmail.com/
regards,
Vladislav Odintsov
On 22.05.2024, 10:59, "dev on behalf of Ales Musil"
wrote:
On Tue, May 14, 2024 at 9:25 PM Vladislav Odintsov
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
> Fixes: 8be01f4a5329 ("Send service monitor health checks")
> Signed-off-by: Vladislav Odintsov
> ---
>
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, _mon->src_mac);
> +if (is_ipv4) {
> +ovs_be32 ip4_src;
> +ip_parse(sb_svc_mon->src_ip, _src);
> +svc_mon->src_ip = in6_addr_mapped_ipv4(ip4_src);
> +} else {
> +ipv6_parse(sb_svc_mon->src_ip, _mon->src_ip);
> +}
> +
>
The whole if else block can be replaced with ip46_parse().
hmap_insert(_monitors_map, _mon->hmap_node, hash);
> ovs_list_push_back(_monitors, _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_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, _src);
> -pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
> - _src, _mon->ip, IPPROTO_TCP,
> +pinctrl_compose_ipv6(, svc_mon->src_mac, svc_mon->ea,
> + _mon->src_ip, _mon->ip, IPPROTO_TCP,
> 63, TCP_HEADER_LEN);
> } else {
> -ovs_be32 ip4_src;
> -ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
> -pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
> - ip4_src,
> in6_addr_get_mapped_ipv4(_mon->ip),
> +
Re: [ovs-dev] [PATCH ovn] controller: Store src_mac, src_ip in svc_monitor struct.
On Tue, May 14, 2024 at 9:25 PM Vladislav Odintsov
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
> Fixes: 8be01f4a5329 ("Send service monitor health checks")
> Signed-off-by: Vladislav Odintsov
> ---
>
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, _mon->src_mac);
> +if (is_ipv4) {
> +ovs_be32 ip4_src;
> +ip_parse(sb_svc_mon->src_ip, _src);
> +svc_mon->src_ip = in6_addr_mapped_ipv4(ip4_src);
> +} else {
> +ipv6_parse(sb_svc_mon->src_ip, _mon->src_ip);
> +}
> +
>
The whole if else block can be replaced with ip46_parse().
hmap_insert(_monitors_map, _mon->hmap_node, hash);
> ovs_list_push_back(_monitors, _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_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, _src);
> -pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
> - _src, _mon->ip, IPPROTO_TCP,
> +pinctrl_compose_ipv6(, svc_mon->src_mac, svc_mon->ea,
> + _mon->src_ip, _mon->ip, IPPROTO_TCP,
> 63, TCP_HEADER_LEN);
> } else {
> -ovs_be32 ip4_src;
> -ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
> -pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
> - ip4_src,
> in6_addr_get_mapped_ipv4(_mon->ip),
> +pinctrl_compose_ipv4(, svc_mon->src_mac, svc_mon->ea,
> + in6_addr_get_mapped_ipv4(_mon->src_ip),
> + in6_addr_get_mapped_ipv4(_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_stub, sizeof packet_stub);
>
> if
Re: [ovs-dev] [PATCH ovn] controller: Store src_mac, src_ip in svc_monitor struct.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413198.html
> On 14 May 2024, at 22:25, Vladislav Odintsov 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
> Fixes: 8be01f4a5329 ("Send service monitor health checks")
> Signed-off-by: Vladislav Odintsov
> ---
> 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, _mon->src_mac);
> +if (is_ipv4) {
> +ovs_be32 ip4_src;
> +ip_parse(sb_svc_mon->src_ip, _src);
> +svc_mon->src_ip = in6_addr_mapped_ipv4(ip4_src);
> +} else {
> +ipv6_parse(sb_svc_mon->src_ip, _mon->src_ip);
> +}
> +
> hmap_insert(_monitors_map, _mon->hmap_node, hash);
> ovs_list_push_back(_monitors, _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_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, _src);
> -pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
> - _src, _mon->ip, IPPROTO_TCP,
> +pinctrl_compose_ipv6(, svc_mon->src_mac, svc_mon->ea,
> + _mon->src_ip, _mon->ip, IPPROTO_TCP,
> 63, TCP_HEADER_LEN);
> } else {
> -ovs_be32 ip4_src;
> -ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
> -pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
> - ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
> +pinctrl_compose_ipv4(, svc_mon->src_mac, svc_mon->ea,
> + in6_addr_get_mapped_ipv4(_mon->src_ip),
> + in6_addr_get_mapped_ipv4(_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_stub, sizeof packet_stub);
>
> if (svc_mon->is_ip6) {
> -struct in6_addr ip6_src;
> -
