Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230112142218.725622-1-eperezma@redhat.com> <20230112142218.725622-4-eperezma@redhat.com>
 <DM8PR12MB54000ADBE1A0717362433C1FABC19@DM8PR12MB5400.namprd12.prod.outlook.com>
In-Reply-To: <DM8PR12MB54000ADBE1A0717362433C1FABC19@DM8PR12MB5400.namprd12.prod.outlook.com>
From:   Eugenio Perez Martin <eperezma@redhat.com>
Date:   Mon, 16 Jan 2023 11:39:47 +0100
Message-ID: <CAJaqyWdrg1nkBSdOmZU=+Nns8UEqoyX7C+wqyk+dOPtv8UW22Q@mail.gmail.com>
Subject: Re: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
To:     Eli Cohen <elic@nvidia.com>
Cc:     "mst@redhat.com" <mst@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Parav Pandit <parav@nvidia.com>,
        "lulu@redhat.com" <lulu@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "virtualization@lists.linux-foundation.org" 
        <virtualization@lists.linux-foundation.org>,
        "sgarzare@redhat.com" <sgarzare@redhat.com>,
        "si-wei.liu@oracle.com" <si-wei.liu@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jan 16, 2023 at 8:13 AM Eli Cohen <elic@nvidia.com> wrote:
>
> > From: Eugenio P=C3=A9rez <eperezma@redhat.com>
> > Sent: Thursday, 12 January 2023 16:22
> > To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> > Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> > lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> > foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> > Subject: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
> >
> > Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock a=
t
> > this moment, but:
> > * These iotlb changes / queries are not in the fast data path.
> > * reslock belongs to netdev, while dup_iotlb seems generic.
> > * It's located in a different file than the lock it needs to hold
> >
> > Justifies the lock acquisition.
> >
>
> Following this reasoning we should take the spinlock wherever we referenc=
e an iotlb.

vring.c:iotlb_translate takes it.

> Question if it make sense that the iotlb could change while it is being r=
eferenced.
> Can you identify a specific case for this?
>

Not at this moment, because both are protected by
mlx5_vdpa_net->reslock before access or change iotlb. So this would
require changes to be exploitable, that's true.

However, to take the lock is the expected usage for vringh, so future
changes to either mlx or vringh could miss it.

Thanks!

> > Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC
> > setting")
> > Signed-off-by: Eugenio P=C3=A9rez <eperezma@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/core/mr.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index 878ee94efa78..e9c8a7f8ee1d 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -454,13 +454,15 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *src)
> >  {
> >       struct vhost_iotlb_map *map;
> >       u64 start =3D 0, last =3D ULLONG_MAX;
> > -     int err;
> > +     int err =3D 0;
> > +
> > +     spin_lock(&mvdev->cvq.iommu_lock);
> >
> >       vhost_iotlb_reset(mvdev->cvq.iotlb);
> >
> >       if (!src) {
> >               err =3D vhost_iotlb_add_range(mvdev->cvq.iotlb, start, la=
st,
> > start, VHOST_ACCESS_RW);
> > -             return err;
> > +             goto out;
> >       }
> >
> >       for (map =3D vhost_iotlb_itree_first(src, start, last); map;
> > @@ -468,9 +470,12 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *src)
> >               err =3D vhost_iotlb_add_range(mvdev->cvq.iotlb, map->star=
t,
> > map->last,
> >                                           map->addr, map->perm);
> >               if (err)
> > -                     return err;
> > +                     goto out;
> >       }
> > -     return 0;
> > +
> > +out:
> > +     spin_unlock(&mvdev->cvq.iommu_lock);
> > +     return err;
> >  }
> >
> >  static void prune_iotlb(struct mlx5_vdpa_dev *mvdev)
> > --
> > 2.31.1
>