Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 14 Oct 2024 21:32:37 +0200
Robin Jarry <rjarry@redhat.com> wrote:

> While invoking telemetry commands (which may happen at any time, out of
> control of the application), an application thread may concurrently
> add/remove ports. The telemetry callbacks may then access partially
> initialized/uninitialised ethdev data.
> 
> Reuse the ethdev lock that protects port allocation/destruction and the
> new telemetry callback register api that takes an additional private
> argument. Pass eth_dev_telemetry_do as the main callback and the actual
> endpoint callbacks as private argument.
> 
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/ethdev/rte_ethdev_telemetry.c | 66 ++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
> index 6b873e7abe68..7599fa2852b6 100644
> --- a/lib/ethdev/rte_ethdev_telemetry.c
> +++ b/lib/ethdev/rte_ethdev_telemetry.c
> @@ -1395,45 +1395,73 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
>  	return ret;
>  }
>  
> +static int eth_dev_telemetry_do(const char *cmd, const char *params,
> +				void *arg, struct rte_tel_data *d)
> +{
> +	int ret;
> +	telemetry_cb fn = arg;
> +	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
> +	ret = fn(cmd, params, d);
> +	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
> +	return ret;
> +}

If this happens often, and the function takes a long time (like doing i/o)
it might be worth changing this to reader/writer in future.

Also, would be best to add a comment here as to what is being protected
if you do another version.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>