[PATCH 19.11] service: fix early move to inactive status

[PATCH 19.11] service: fix early move to inactive status

[Erik Gabriel Carrillo]

[ upstream commit 329280c53e6d09002b67e4d052fe27a952bd19cf ]

Assume thread T2 is a service lcore that is in the middle of executing
a service function.  Also, assume thread T1 concurrently calls
rte_service_lcore_stop(), which will set the "service_active_on_lcore"
state to false.  If thread T1 then calls rte_service_may_be_active(),
it can return zero even though T2 is still running the service function.
If T1 then proceeds to free data being used by T2, a crash can ensue.

Move the logic that clears the "service_active_on_lcore" state from the
rte_service_lcore_stop() function to the service_runner_func() to
ensure that we:
- don't let the "service_active_on_lcore" state linger as 1
- don't clear the state early

Fixes: 3d3bcdcee4 ("service: fix lingering active status")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/librte_eal/common/rte_service.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
index d11027fcb..11cd5a538 100644
--- a/lib/librte_eal/common/rte_service.c
+++ b/lib/librte_eal/common/rte_service.c
@@ -447,6 +447,12 @@ service_runner_func(void *arg)
 		rte_smp_rmb();
 	}
 
+	/* Switch off this core for all services, to ensure that future
+	 * calls to may_be_active() know this core is switched off.
+	 */
+	for (i = 0; i < RTE_SERVICE_NUM_MAX; i++)
+		cs->service_active_on_lcore[i] = 0;
+
 	return 0;
 }
 
@@ -717,11 +723,6 @@ rte_service_lcore_stop(uint32_t lcore)
 		int32_t only_core = (1 ==
 			rte_atomic32_read(&rte_services[i].num_mapped_cores));
 
-		/* Switch off this core for all services, to ensure that future
-		 * calls to may_be_active() know this core is switched off.
-		 */
-		cs->service_active_on_lcore[i] = 0;
-
 		/* if the core is mapped, and the service is running, and this
 		 * is the only core that is mapped, the service would cease to
 		 * run if this core stopped, so fail instead.
-- 
2.23.0

Re: [PATCH 19.11] service: fix early move to inactive status

[Christian Ehrhardt]

On Wed, Nov 16, 2022 at 7:34 PM Erik Gabriel Carrillo
<erik.g.carrillo@intel.com> wrote:
>
> [ upstream commit 329280c53e6d09002b67e4d052fe27a952bd19cf ]

Applied, thanks!

> Assume thread T2 is a service lcore that is in the middle of executing
> a service function.  Also, assume thread T1 concurrently calls
> rte_service_lcore_stop(), which will set the "service_active_on_lcore"
> state to false.  If thread T1 then calls rte_service_may_be_active(),
> it can return zero even though T2 is still running the service function.
> If T1 then proceeds to free data being used by T2, a crash can ensue.
>
> Move the logic that clears the "service_active_on_lcore" state from the
> rte_service_lcore_stop() function to the service_runner_func() to
> ensure that we:
> - don't let the "service_active_on_lcore" state linger as 1
> - don't clear the state early
>
> Fixes: 3d3bcdcee4 ("service: fix lingering active status")
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  lib/librte_eal/common/rte_service.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c
> index d11027fcb..11cd5a538 100644
> --- a/lib/librte_eal/common/rte_service.c
> +++ b/lib/librte_eal/common/rte_service.c
> @@ -447,6 +447,12 @@ service_runner_func(void *arg)
>                 rte_smp_rmb();
>         }
>
> +       /* Switch off this core for all services, to ensure that future
> +        * calls to may_be_active() know this core is switched off.
> +        */
> +       for (i = 0; i < RTE_SERVICE_NUM_MAX; i++)
> +               cs->service_active_on_lcore[i] = 0;
> +
>         return 0;
>  }
>
> @@ -717,11 +723,6 @@ rte_service_lcore_stop(uint32_t lcore)
>                 int32_t only_core = (1 ==
>                         rte_atomic32_read(&rte_services[i].num_mapped_cores));
>
> -               /* Switch off this core for all services, to ensure that future
> -                * calls to may_be_active() know this core is switched off.
> -                */
> -               cs->service_active_on_lcore[i] = 0;
> -
>                 /* if the core is mapped, and the service is running, and this
>                  * is the only core that is mapped, the service would cease to
>                  * run if this core stopped, so fail instead.
> --
> 2.23.0
>


-- 
Christian Ehrhardt
Senior Staff Engineer, Ubuntu Server
Canonical Ltd