Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock at
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.
Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Eugenio Pérez <[email protected]>
---
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 = 0, last = ULLONG_MAX;
- int err;
+ int err = 0;
+
+ spin_lock(&mvdev->cvq.iommu_lock);
vhost_iotlb_reset(mvdev->cvq.iotlb);
if (!src) {
err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last, start, VHOST_ACCESS_RW);
- return err;
+ goto out;
}
for (map = 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 = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start, 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
> From: Eugenio Pérez <[email protected]>
> Sent: Thursday, 12 January 2023 16:22
> To: [email protected]; Eli Cohen <[email protected]>
> Cc: [email protected]; Parav Pandit <[email protected]>;
> [email protected]; [email protected]; [email protected]
> foundation.org; [email protected]; [email protected]
> Subject: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
>
> Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock at
> 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 reference an iotlb.
Question if it make sense that the iotlb could change while it is being referenced.
Can you identify a specific case for this?
> Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC
> setting")
> Signed-off-by: Eugenio Pérez <[email protected]>
> ---
> 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 = 0, last = ULLONG_MAX;
> - int err;
> + int err = 0;
> +
> + spin_lock(&mvdev->cvq.iommu_lock);
>
> vhost_iotlb_reset(mvdev->cvq.iotlb);
>
> if (!src) {
> err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> start, VHOST_ACCESS_RW);
> - return err;
> + goto out;
> }
>
> for (map = 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 = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start,
> 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
On Mon, Jan 16, 2023 at 8:13 AM Eli Cohen <[email protected]> wrote:
>
> > From: Eugenio Pérez <[email protected]>
> > Sent: Thursday, 12 January 2023 16:22
> > To: [email protected]; Eli Cohen <[email protected]>
> > Cc: [email protected]; Parav Pandit <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > foundation.org; [email protected]; [email protected]
> > Subject: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
> >
> > Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock at
> > 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 reference an iotlb.
vring.c:iotlb_translate takes it.
> Question if it make sense that the iotlb could change while it is being referenced.
> 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érez <[email protected]>
> > ---
> > 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 = 0, last = ULLONG_MAX;
> > - int err;
> > + int err = 0;
> > +
> > + spin_lock(&mvdev->cvq.iommu_lock);
> >
> > vhost_iotlb_reset(mvdev->cvq.iotlb);
> >
> > if (!src) {
> > err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> > start, VHOST_ACCESS_RW);
> > - return err;
> > + goto out;
> > }
> >
> > for (map = 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 = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start,
> > 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
>