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

[David Marchand <david.marchand@redhat.com>]

On Mon, Oct 14, 2024 at 10:01 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> 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.

Yes, this was an option mentionned when we discussed the issue in Montréal.
For now, a spinlock seems enough.


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

I can add something when applying, like:

@@ -1400,6 +1400,7 @@ static int eth_dev_telemetry_do(const char *cmd,
const char *params,
 {
        int ret;
        telemetry_cb fn = arg;
+       /* Protect against port removal while invoking callback,
calling ethdev API. */
        rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
        ret = fn(cmd, params, d);
        rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());



-- 
David Marchand