[dpdk-dev] [PATCH] vfio: use right index when tracking devices in a vfio group

[Alejandro Lucero]

Hi Anatoly,

On Tue, May 9, 2017 at 4:18 PM, Burakov, Anatoly <anatoly.burakov at intel.com>
wrote:

> Hi Alejandro,
>
> > From: Alejandro Lucero [mailto:alejandro.lucero at netronome.com]
> > Sent: Monday, May 8, 2017 6:44 PM
> > To: dev at dpdk.org
> > Cc: Burakov, Anatoly <anatoly.burakov at intel.com>;
> > jerin.jacob at caviumnetworks.com; thomas at monjalon.net
> > Subject: [PATCH] vfio: use right index when tracking devices in a vfio
> group
> >
> > Previous fix for properly handling devices from the same VFIO group
> > introduced another bug where the file descriptor of a kernel vfio group
> is
> > used as the index for tracking number of devices of a vfio group struct
> > handled by dpdk vfio code. Instead of the file descriptor itself, the
> vfio group
> > object that file descriptor is registered with has to be used.
> >
> > This patch introduces specific functions for incrementing or decrementing
> > the device counter for a specific vfio group using the vfio file
> descriptor as a
> > parameter. Note the code is not optimized as the vfio group is found
> > sequentially going through the vfio group array but this should not be a
> > problem as this is not related to packet handling at all.
> >
> > Fixes: a9c349e3a100 ("vfio: fix device unplug when several devices per
> > group")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero at netronome.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_vfio.c | 60
> > +++++++++++++++++++++++++++-------
> >  1 file changed, 49 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > index d3eae20..21d126f 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > @@ -172,6 +172,44 @@
> >       return -1;
> >  }
> >
> > +
> > +static int
> > +get_vfio_group_idx(int vfio_group_fd)
> > +{
> > +     int i;
> > +     for (i = 0; i < VFIO_MAX_GROUPS; i++)
> > +             if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd)
> > +                     return i;
> > +     return -1;
> > +}
> > +
> > +static void
> > +vfio_group_device_get(int vfio_group_fd) {
> > +     int i;
> > +
> > +     i = get_vfio_group_idx(vfio_group_fd);
> > +     vfio_cfg.vfio_groups[i].devices++;
>
> Maybe add a check for I < 0?
>
>
Right. I doubted about adding such a check, since being < 0 or >
VFIO_MAX_GROUPS are bad news. But better to avoid using a wrong index which
could led to a random write elsewhere. I will send another patch version
with that check and a RTE_LOG(ERR ... call.

And same for the other functions.

Thanks


> > +}
> > +
> > +static void
> > +vfio_group_device_put(int vfio_group_fd) {
> > +     int i;
> > +
> > +     i = get_vfio_group_idx(vfio_group_fd);
> > +     vfio_cfg.vfio_groups[i].devices--;
>
> Same here.
>
> > +}
> > +
> > +static int
> > +vfio_group_device_count(int vfio_group_fd) {
> > +     int i;
> > +
> > +     i = get_vfio_group_idx(vfio_group_fd);
> > +     return vfio_cfg.vfio_groups[i].devices; }
>
> And here.
>
> > +
> >  int
> >  clear_group(int vfio_group_fd)
> >  {
> > @@ -180,15 +218,14 @@
> >
> >       if (internal_config.process_type == RTE_PROC_PRIMARY) {
> >
> > -             for (i = 0; i < VFIO_MAX_GROUPS; i++)
> > -                     if (vfio_cfg.vfio_groups[i].fd == vfio_group_fd) {
> > -                             vfio_cfg.vfio_groups[i].group_no = -1;
> > -                             vfio_cfg.vfio_groups[i].fd = -1;
> > -                             vfio_cfg.vfio_groups[i].devices = 0;
> > -                             vfio_cfg.vfio_active_groups--;
> > -                             return 0;
> > -                     }
> > -             return -1;
> > +             i = get_vfio_group_idx(vfio_group_fd);
> > +             if ( i < 0)
> > +                     return -1;
> > +             vfio_cfg.vfio_groups[i].group_no = -1;
> > +             vfio_cfg.vfio_groups[i].fd = -1;
> > +             vfio_cfg.vfio_groups[i].devices = 0;
> > +             vfio_cfg.vfio_active_groups--;
> > +             return 0;
> >       }
> >
> >       /* This is just for SECONDARY processes */ @@ -358,7 +395,7 @@
> >               clear_group(vfio_group_fd);
> >               return -1;
> >       }
> > -     vfio_cfg.vfio_groups[vfio_group_fd].devices++;
> > +     vfio_group_device_get(vfio_group_fd);
> >
> >       return 0;
> >  }
> > @@ -406,7 +443,8 @@
> >       /* An VFIO group can have several devices attached. Just when there
> > is
> >        * no devices remaining should the group be closed.
> >        */
> > -     if (--vfio_cfg.vfio_groups[vfio_group_fd].devices == 0) {
> > +     vfio_group_device_put(vfio_group_fd);
> > +     if (!vfio_group_device_count(vfio_group_fd)) {
> >
> >               if (close(vfio_group_fd) < 0) {
> >                       RTE_LOG(INFO, EAL, "Error when closing
> > vfio_group_fd for %s\n",
> > --
> > 1.9.1
>
>