Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Bharat Bhushan <bbhushan2@marvell.com>
To:     Ankur Arora <ankur.a.arora@oracle.com>
CC:     "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        "terminus@gmail.com" <terminus@gmail.com>
Subject: RE: [EXT] Re: vfio-pci: protect remap_pfn_range() from simultaneous
 calls
Thread-Topic: [EXT] Re: vfio-pci: protect remap_pfn_range() from simultaneous
 calls
Thread-Index: AdbkRnC96D9dwqxzR0eR3ijlJSqy4gAEULUAAnocGfAAW82ggAcKnP6AALubABA=
Date:   Tue, 2 Mar 2021 12:47:53 +0000
Message-ID: <DM6PR18MB2698B4CB0114D7D1FCE74B4BE3999@DM6PR18MB2698.namprd18.prod.outlook.com>
References: <20210121043907.76037-1-ankur.a.arora@oracle.com>
 <20210226005333.1576246-1-ankur.a.arora@oracle.com>
In-Reply-To: <20210226005333.1576246-1-ankur.a.arora@oracle.com>
Accept-Language: en-IN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f1608bfa-762b-477b-a197-08d8dd796573
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Mar 2021 12:47:53.4683
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CryX2SebubFSzJpNtQtE5iw94Eb1zALwPM+D/EiFMKOmxZn68bPRCpDI2n+Ief4IsHGDE/Rv7FMAdd1ErInUmA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR18MB2874
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761
 definitions=2021-03-02_06:2021-03-01,2021-03-02 signatures=0
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ankur,

> -----Original Message-----
> From: Ankur Arora <ankur.a.arora@oracle.com>
> Sent: Friday, February 26, 2021 6:24 AM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: alex.williamson@redhat.com; ankur.a.arora@oracle.com; linux-
> kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>;
> terminus@gmail.com
> Subject: [EXT] Re: vfio-pci: protect remap_pfn_range() from simultaneous =
calls
>=20
> External Email
>=20
> ----------------------------------------------------------------------
> Hi Bharat,
>=20
> Can you test the patch below to see if it works for you?

Sorry for late reply, I actually missed this email.

Reproducibility of the issue was low in my test scenario, one out of ~15 ru=
ns. I run it multiple times, overnight and observed no issues.

>=20
> Also could you add some more detail to your earlier description of
> the bug?

Our test case is running ODP multi-threaded application, where parent proce=
ss maps (yes it uses MAP_DMA) the region and then child processes access sa=
me.  As a workaround we tried accessing the region once by parent process b=
efore creating other accessor threads and it worked as expected.

Thanks
-Bharat

> In particular, AFAICS you are using ODP (-DPDK?) with multiple
> threads touching this region. From your stack, it looks like the
> fault was user-space generated, and I'm guessing you were not
> using the VFIO_IOMMU_MAP_DMA.
>=20
> Ankur
>=20
> -- >8 --
>=20
> Subject: [PATCH] vfio-pci: protect io_remap_pfn_range() from simultaneous=
 calls
>=20
> vfio_pci_mmap_fault() maps the complete VMA on fault. With concurrent
> faults, this would result in multiple calls to io_remap_pfn_range(),
> where it would hit a BUG_ON(!pte_none(*pte)) in remap_pte_range().
> (It would also link the same VMA multiple times in vdev->vma_list
> but given the BUG_ON that is less serious.)
>=20
> Normally, however, this won't happen -- at least with vfio_iommu_type1 --
> the VFIO_IOMMU_MAP_DMA path is protected by iommu->lock.
>=20
> If, however, we are using some kind of parallelization mechanism like
> this one with ktask under discussion [1], we would hit this.
> Even if we were doing this serially, given that vfio-pci remaps a larger
> extent than strictly necessary it should internally enforce coherence of
> its data structures.
>=20
> Handle this by using the VMA's presence in the vdev->vma_list as
> indicative of a fully mapped VMA and returning success early to
> all but the first VMA fault. Note that this is clearly optimstic given
> that the mapping is ongoing, and might mean that the caller sees
> more faults until the remap is done.
>=20
> [1] https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__lore.kernel.or=
g_linux-
> 2Dmm_20181105145141.6f9937f6-
> 40w520.home_&d=3DDwIDAg&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3DPAAlWswPe7d8gHl
> GbCLmy2YezyK7O3Hv_t2heGnouBw&m=3D3ZDXqnn9xNUCjgXwN9mHIKT7oyXu55P
> U7yV2j0b-5hw&s=3DhiICkNtrcH4AbAWRrbkvMUylp7Bv0YHFCjxNGC6CGOk&e=3D
>=20
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 65e7e6b44578..b9f509863db1 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1573,6 +1573,11 @@ static int __vfio_pci_add_vma(struct vfio_pci_devi=
ce
> *vdev,
>  {
>  	struct vfio_pci_mmap_vma *mmap_vma;
>=20
> +	list_for_each_entry(mmap_vma, &vdev->vma_list, vma_next) {
> +		if (mmap_vma->vma =3D=3D vma)
> +			return 1;
> +	}
> +
>  	mmap_vma =3D kmalloc(sizeof(*mmap_vma), GFP_KERNEL);
>  	if (!mmap_vma)
>  		return -ENOMEM;
> @@ -1613,6 +1618,7 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fau=
lt
> *vmf)
>  	struct vm_area_struct *vma =3D vmf->vma;
>  	struct vfio_pci_device *vdev =3D vma->vm_private_data;
>  	vm_fault_t ret =3D VM_FAULT_NOPAGE;
> +	int vma_present;
>=20
>  	mutex_lock(&vdev->vma_lock);
>  	down_read(&vdev->memory_lock);
> @@ -1623,7 +1629,21 @@ static vm_fault_t vfio_pci_mmap_fault(struct
> vm_fault *vmf)
>  		goto up_out;
>  	}
>=20
> -	if (__vfio_pci_add_vma(vdev, vma)) {
> +	/*
> +	 * __vfio_pci_add_vma() either adds the vma to the vdev->vma_list
> +	 * (vma_present =3D=3D 0), or indicates that the vma is already present
> +	 * on the list (vma_present =3D=3D 1).
> +	 *
> +	 * Overload the meaning of this flag to also imply that the vma is
> +	 * fully mapped. This allows us to serialize the mapping -- ensuring
> +	 * that simultaneous faults will not both try to call
> +	 * io_remap_pfn_range().
> +	 *
> +	 * However, this might mean that callers to which we returned success
> +	 * optimistically will see more faults until the remap is complete.
> +	 */
> +	vma_present =3D __vfio_pci_add_vma(vdev, vma);
> +	if (vma_present < 0) {
>  		ret =3D VM_FAULT_OOM;
>  		mutex_unlock(&vdev->vma_lock);
>  		goto up_out;
> @@ -1631,6 +1651,9 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fau=
lt
> *vmf)
>=20
>  	mutex_unlock(&vdev->vma_lock);
>=20
> +	if (vma_present)
> +		goto up_out;
> +
>  	if (io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>  			       vma->vm_end - vma->vm_start, vma-
> >vm_page_prot))
>  		ret =3D VM_FAULT_SIGBUS;
> --
> 2.29.2