linux-kernel - RE: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM8PR12MB54000ADBE1A0717362433C1FABC19@DM8PR12MB5400.namprd12.prod.outlook.com>
Date:   Mon, 16 Jan 2023 07:13:22 +0000
From:   Eli Cohen <elic@...dia.com>
To:     Eugenio Pérez <eperezma@...hat.com>,
        "mst@...hat.com" <mst@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Parav Pandit <parav@...dia.com>,
        "lulu@...hat.com" <lulu@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "sgarzare@...hat.com" <sgarzare@...hat.com>,
        "si-wei.liu@...cle.com" <si-wei.liu@...cle.com>
Subject: RE: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb

> From: Eugenio Pérez <eperezma@...hat.com>
> Sent: Thursday, 12 January 2023 16:22
> To: mst@...hat.com; Eli Cohen <elic@...dia.com>
> Cc: linux-kernel@...r.kernel.org; Parav Pandit <parav@...dia.com>;
> lulu@...hat.com; jasowang@...hat.com; virtualization@...ts.linux-
> foundation.org; sgarzare@...hat.com; si-wei.liu@...cle.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 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 <eperezma@...hat.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 = 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