Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
On 2022/6/21 13:48, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 21, 2022 12:28 PM
On 2022/6/21 11:46, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 21, 2022 11:39 AM
On 2022/6/21 10:54, Tian, Kevin wrote:
From: Lu Baolu
Sent: Monday, June 20, 2022 4:17 PM
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
dmar_domain *domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu,
domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
2.25.1
It's cleaner to avoid this error at the first place, i.e. only do the
setup when the first device is attached to the pasid table.
The logic that identifies the first device might introduce additional
unnecessary complexity. Devices that share a pasid table are rare. I
even prefer to give up sharing tables so that the code can be
simpler.:-)
It's not that complex if you simply move device_attach_pasid_table()
out of intel_pasid_alloc_table(). Then do the setup if
list_empty(&pasid_table->dev) and then attach device to the
pasid table in domain_add_dev_info().
The pasid table is part of the device, hence a better place to
allocate/free the pasid table is in the device probe/release paths.
Things will become more complicated if we change relationship between
device and it's pasid table when attaching/detaching a domain. That's
the reason why I thought it was additional complexity.
If you do want to follow current route it’s still cleaner to check
whether the pasid entry has pointed to the domain in the individual
setup function instead of blindly returning -EBUSY and then ignoring
it even if a real busy condition occurs. The setup functions can
just return zero for this benign alias case.
Fair enough. Let me improve it.
--
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
> From: Baolu Lu
> Sent: Tuesday, June 21, 2022 12:28 PM
>
> On 2022/6/21 11:46, Tian, Kevin wrote:
> >> From: Baolu Lu
> >> Sent: Tuesday, June 21, 2022 11:39 AM
> >>
> >> On 2022/6/21 10:54, Tian, Kevin wrote:
> From: Lu Baolu
> Sent: Monday, June 20, 2022 4:17 PM
> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> dmar_domain *domain, struct device *dev)
> ret = intel_pasid_setup_second_level(iommu,
> domain,
> dev, PASID_RID2PASID);
> spin_unlock_irqrestore(&iommu->lock, flags);
> -if (ret) {
> +if (ret && ret != -EBUSY) {
> dev_err(dev, "Setup RID2PASID failed\n");
> dmar_remove_one_dev_info(dev);
> return ret;
> --
> 2.25.1
> >>>
> >>> It's cleaner to avoid this error at the first place, i.e. only do the
> >>> setup when the first device is attached to the pasid table.
> >>
> >> The logic that identifies the first device might introduce additional
> >> unnecessary complexity. Devices that share a pasid table are rare. I
> >> even prefer to give up sharing tables so that the code can be
> >> simpler.:-)
> >>
> >
> > It's not that complex if you simply move device_attach_pasid_table()
> > out of intel_pasid_alloc_table(). Then do the setup if
> > list_empty(&pasid_table->dev) and then attach device to the
> > pasid table in domain_add_dev_info().
>
> The pasid table is part of the device, hence a better place to
> allocate/free the pasid table is in the device probe/release paths.
> Things will become more complicated if we change relationship between
> device and it's pasid table when attaching/detaching a domain. That's
> the reason why I thought it was additional complexity.
>
If you do want to follow current route it’s still cleaner to check
whether the pasid entry has pointed to the domain in the individual
setup function instead of blindly returning -EBUSY and then ignoring
it even if a real busy condition occurs. The setup functions can
just return zero for this benign alias case.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
On 2022/6/21 11:46, Tian, Kevin wrote:
From: Baolu Lu
Sent: Tuesday, June 21, 2022 11:39 AM
On 2022/6/21 10:54, Tian, Kevin wrote:
From: Lu Baolu
Sent: Monday, June 20, 2022 4:17 PM
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
dmar_domain *domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu,
domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
2.25.1
It's cleaner to avoid this error at the first place, i.e. only do the
setup when the first device is attached to the pasid table.
The logic that identifies the first device might introduce additional
unnecessary complexity. Devices that share a pasid table are rare. I
even prefer to give up sharing tables so that the code can be
simpler.:-)
It's not that complex if you simply move device_attach_pasid_table()
out of intel_pasid_alloc_table(). Then do the setup if
list_empty(&pasid_table->dev) and then attach device to the
pasid table in domain_add_dev_info().
The pasid table is part of the device, hence a better place to
allocate/free the pasid table is in the device probe/release paths.
Things will become more complicated if we change relationship between
device and it's pasid table when attaching/detaching a domain. That's
the reason why I thought it was additional complexity.
--
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
> From: Baolu Lu
> Sent: Tuesday, June 21, 2022 11:39 AM
>
> On 2022/6/21 10:54, Tian, Kevin wrote:
> >> From: Lu Baolu
> >> Sent: Monday, June 20, 2022 4:17 PM
> >> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> >> dmar_domain *domain, struct device *dev)
> >>ret = intel_pasid_setup_second_level(iommu,
> >> domain,
> >>dev, PASID_RID2PASID);
> >>spin_unlock_irqrestore(&iommu->lock, flags);
> >> - if (ret) {
> >> + if (ret && ret != -EBUSY) {
> >>dev_err(dev, "Setup RID2PASID failed\n");
> >>dmar_remove_one_dev_info(dev);
> >>return ret;
> >> --
> >> 2.25.1
> >
> > It's cleaner to avoid this error at the first place, i.e. only do the
> > setup when the first device is attached to the pasid table.
>
> The logic that identifies the first device might introduce additional
> unnecessary complexity. Devices that share a pasid table are rare. I
> even prefer to give up sharing tables so that the code can be
> simpler.:-)
>
It's not that complex if you simply move device_attach_pasid_table()
out of intel_pasid_alloc_table(). Then do the setup if
list_empty(&pasid_table->dev) and then attach device to the
pasid table in domain_add_dev_info().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
On 2022/6/21 10:54, Tian, Kevin wrote:
From: Lu Baolu
Sent: Monday, June 20, 2022 4:17 PM
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
dmar_domain *domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu,
domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
2.25.1
It's cleaner to avoid this error at the first place, i.e. only do the
setup when the first device is attached to the pasid table.
The logic that identifies the first device might introduce additional
unnecessary complexity. Devices that share a pasid table are rare. I
even prefer to give up sharing tables so that the code can be
simpler.:-)
--
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
> From: Lu Baolu
> Sent: Monday, June 20, 2022 4:17 PM
> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> dmar_domain *domain, struct device *dev)
> ret = intel_pasid_setup_second_level(iommu,
> domain,
> dev, PASID_RID2PASID);
> spin_unlock_irqrestore(&iommu->lock, flags);
> - if (ret) {
> + if (ret && ret != -EBUSY) {
> dev_err(dev, "Setup RID2PASID failed\n");
> dmar_remove_one_dev_info(dev);
> return ret;
> --
> 2.25.1
It's cleaner to avoid this error at the first place, i.e. only do the
setup when the first device is attached to the pasid table.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-direct: take dma-ranges/offsets into account in resource mapping
Folks,
On Fri, Jun 10, 2022 at 11:08:02AM +0300, Serge Semin wrote:
> A basic device-specific linear memory mapping was introduced back in
> commit ("dma: Take into account dma_pfn_offset") as a single-valued offset
> preserved in the device.dma_pfn_offset field, which was initialized for
> instance by means of the "dma-ranges" DT property. Afterwards the
> functionality was extended to support more than one device-specific region
> defined in the device.dma_range_map list of maps. But all of these
> improvements concerned a single pointer, page or sg DMA-mapping methods,
> while the system resource mapping function turned to miss the
> corresponding modification. Thus the dma_direct_map_resource() method now
> just casts the CPU physical address to the device DMA address with no
> dma-ranges-based mapping taking into account, which is obviously wrong.
> Let's fix it by using the phys_to_dma_direct() method to get the
> device-specific bus address from the passed memory resource for the case
> of the directly mapped DMA.
So any comment on the suggest modification? Any notes against or for?
-Sergey
>
> Fixes: 25f1e1887088 ("dma: Take into account dma_pfn_offset")
> Signed-off-by: Serge Semin
>
> ---
>
> After a long discussion with Christoph and Robin regarding this patch
> here:
> https://lore.kernel.org/lkml/20220324014836.19149-4-sergey.se...@baikalelectronics.ru
> and here
> https://lore.kernel.org/linux-pci/20220503225104.12108-2-sergey.se...@baikalelectronics.ru/
> It was decided to consult with wider maintainers audience whether it's ok
> to accept the change as is or a more sophisticated solution needs to be
> found for the non-linear direct MMIO mapping.
>
> Cc: Christoph Hellwig
> Cc: Robin Murphy
> Cc: Manivannan Sadhasivam
>
> file: arch/arm/mach-orion5x/board-dt.c
> Cc: Andrew Lunn
> Cc: Sebastian Hesselbarth
> Cc: Gregory Clement
> Cc: linux-arm-ker...@lists.infradead.org
>
> file: drivers/crypto/marvell/cesa/cesa.c
> Cc: Srujana Challa
> Cc: Arnaud Ebalard
> Cc: Boris Brezillon
> Cc: linux-cry...@vger.kernel.org
>
> file: drivers/dma/{fsl-edma-common.c,pl330.c,sh/rcar-dmac.c}
> Cc: Vinod Koul
> Cc: dmaeng...@vger.kernel.org
>
> file:
> arch/arm/boot/dts/{vfxxx.dtsi,ls1021a.dtsi,imx7ulp.dtsi,fsl-ls1043a.dtsi}
> Cc: Shawn Guo
> Cc: Sascha Hauer
> Cc: Li Yang
> Cc: linux-arm-ker...@lists.infradead.org
>
> file: arch/arm/boot/dts/r8a77*.dtsi, arch/arm64/boot/dts/renesas/r8a77*.dtsi
> Cc: Geert Uytterhoeven
> Cc: Magnus Damm
> Cc: linux-renesas-...@vger.kernel.org
>
> file: drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "Pan, Xinhui"
>
> file: drivers/gpu/drm/virtio/virtgpu_vram.c
> Cc: David Airlie
> Cc: Gerd Hoffmann
>
> file: drivers/media/common/videobuf2/videobuf2-dma-contig.c
> Cc: Tomasz Figa
> Cc: Marek Szyprowski
>
> file: drivers/misc/habanalabs/common/memory.c
> Cc: Oded Gabbay
> Cc: Arnd Bergmann
> Cc: Greg Kroah-Hartman
>
> file: drivers/mtd/nand/raw/qcom_nandc.c
> Cc: Manivannan Sadhasivam
>
> file:
> arch/arm64/boot/dts/qcom/{ipq8074.dtsi,ipq6018.dtsi,qcom-sdx55.dtsi,qcom-ipq4019.dtsi,qcom-ipq8064.dtsi}
> Cc: Andy Gross
> Cc: Bjorn Andersson
> Cc: linux-arm-...@vger.kernel.org
>
> file: drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> Cc: Sunil Goutham
> Cc: Linu Cherian
> Cc: Geetha sowjanya
>
> file: drivers/ntb/ntb_transport.c
> Cc: Jon Mason
> Cc: Dave Jiang
> Cc: n...@lists.linux.dev
> ---
> kernel/dma/direct.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 9743c6ccce1a..bc06db74dfdb 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -497,7 +497,7 @@ int dma_direct_map_sg(struct device *dev, struct
> scatterlist *sgl, int nents,
> dma_addr_t dma_direct_map_resource(struct device *dev, phys_addr_t paddr,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> - dma_addr_t dma_addr = paddr;
> + dma_addr_t dma_addr = phys_to_dma_direct(dev, paddr);
>
> if (unlikely(!dma_capable(dev, dma_addr, size, false))) {
> dev_err_once(dev,
> --
> 2.35.1
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: Tidy up locking
Hi Jean-Philippe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on soc/for-next linus/master v5.19-rc2 next-20220617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/intel-lab-lkp/linux/commits/Jean-Philippe-Brucker/uacce-Tidy-up-locking/20220620-220634
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
0a35780c755ccec097d15c6b4ff8b246a89f1689
config: i386-randconfig-s001
(https://download.01.org/0day-ci/archive/20220621/202206210432.wvkoxvu5-...@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-30-g92122700-dirty
#
https://github.com/intel-lab-lkp/linux/commit/3589b5391f54bea3dc85ed65fe0f036757a4f21c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Jean-Philippe-Brucker/uacce-Tidy-up-locking/20220620-220634
git checkout 3589b5391f54bea3dc85ed65fe0f036757a4f21c
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir
ARCH=i386 SHELL=/bin/bash drivers/misc/uacce/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
sparse warnings: (new ones prefixed by >>)
>> drivers/misc/uacce/uacce.c:291:21: sparse: sparse: incorrect type in
>> assignment (different base types) @@ expected int ret @@ got
>> restricted __poll_t @@
drivers/misc/uacce/uacce.c:291:21: sparse: expected int ret
drivers/misc/uacce/uacce.c:291:21: sparse: got restricted __poll_t
>> drivers/misc/uacce/uacce.c:295:16: sparse: sparse: incorrect type in return
>> expression (different base types) @@ expected restricted __poll_t @@
>> got int ret @@
drivers/misc/uacce/uacce.c:295:16: sparse: expected restricted __poll_t
drivers/misc/uacce/uacce.c:295:16: sparse: got int ret
vim +291 drivers/misc/uacce/uacce.c
277
278 static __poll_t uacce_fops_poll(struct file *file, poll_table *wait)
279 {
280 struct uacce_queue *q = file->private_data;
281 struct uacce_device *uacce = q->uacce;
282 int ret = 0;
283
284 poll_wait(file, &q->wait, wait);
285
286 mutex_lock(&q->mutex);
287 if (!uacce_queue_is_valid(q))
288 goto out_unlock;
289
290 if (uacce->ops->is_q_updated && uacce->ops->is_q_updated(q))
> 291 ret = EPOLLIN | EPOLLRDNORM;
292
293 out_unlock:
294 mutex_unlock(&q->mutex);
> 295 return ret;
296 }
297
--
0-DAY CI Kernel Test Service
https://01.org/lkp
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/2] iommu: arm-smmu-impl: Add 8250 display compatible to the client list.
Required for turning on per-process page tables for the GPU.
Signed-off-by: Emma Anholt
Reviewed-by: Konrad Dybcio
Reviewed-by: Dmitry Baryshkov
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d8e1ef83c01b..bb9220937068 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -233,6 +233,7 @@ static const struct of_device_id
qcom_smmu_client_of_match[] __maybe_unused = {
{ .compatible = "qcom,sc7280-mdss" },
{ .compatible = "qcom,sc7280-mss-pil" },
{ .compatible = "qcom,sc8180x-mdss" },
+ { .compatible = "qcom,sm8250-mdss" },
{ .compatible = "qcom,sdm845-mdss" },
{ .compatible = "qcom,sdm845-mss-pil" },
{ }
--
2.36.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/2] arm64: dts: qcom: sm8250: Enable per-process page tables.
This is an SMMU for the adreno gpu, and adding this compatible lets
the driver use per-fd page tables, which are required for security
between GPU clients.
Signed-off-by: Emma Anholt
Reviewed-by: Dmitry Baryshkov
---
v2: moved qcom,adreno-smmu earlier
arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi
b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index a92230bec1dd..aae7b841b81a 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2513,7 +2513,7 @@ gpucc: clock-controller@3d9 {
};
adreno_smmu: iommu@3da {
- compatible = "qcom,sm8250-smmu-500", "arm,mmu-500";
+ compatible = "qcom,sm8250-smmu-500",
"qcom,adreno-smmu", "arm,mmu-500";
reg = <0 0x03da 0 0x1>;
#iommu-cells = <2>;
#global-interrupts = <2>;
--
2.36.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/2] per-process page tables for qcom 8250
This enable per-process page tables on the Qualcomm RB5 boards I'm setting up for Mesa CI. Has survived a full deqp-vk run. v2: moved qcom,adreno-smmu compatible earlier Emma Anholt (2): iommu: arm-smmu-impl: Add 8250 display compatible to the client list. arm64: dts: qcom: sm8250: Enable per-process page tables. arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 +- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) -- 2.36.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On Mon, Jun 20, 2022 at 02:24:31PM +0100, Jean-Philippe Brucker wrote: > >From c7c2b051ec19285bbb973f8a2a5e58bb5326e00e Mon Sep 17 00:00:00 2001 > From: Jean-Philippe Brucker > Date: Mon, 20 Jun 2022 10:10:41 +0100 > Subject: [PATCH] uacce: Tidy up locking > > The uacce driver must deal with a possible removal of the parent driver > or device at any time. No it should not, if the reference counting logic is properly set up. The parent driver should correctly tear things down here. > At the moment there are several issues that may > result in use-after-free. Tidy up the locking to handle module removal. I don't think you did that, as module removal should never happen if a file descriptor is opened as I previously mentioned. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On Mon, Jun 20, 2022 at 02:24:31PM +0100, Jean-Philippe Brucker wrote: > On Fri, Jun 17, 2022 at 02:05:21PM +0800, Zhangfei Gao wrote: > > > The refcount only ensures that the uacce_device object is not freed as > > > long as there are open fds. But uacce_remove() can run while there are > > > open fds, or fds in the process of being opened. And atfer uacce_remove() > > > runs, the uacce_device object still exists but is mostly unusable. For > > > example once the module is freed, uacce->ops is not valid anymore. But > > > currently uacce_fops_open() may dereference the ops in this case: > > > > > > uacce_fops_open() > > >if (!uacce->parent->driver) > > >/* Still valid, keep going */ > > >...rmmod > > >uacce_remove() > > >... free_module() > > >uacce->ops->get_queue() /* BUG */ > > > > uacce_remove should wait for uacce->queues_lock, until fops_open release the > > lock. > > If open happen just after the uacce_remove: unlock, uacce_bind_queue in open > > should fail. > > Ah yes sorry, I lost sight of what this patch was adding. But we could > have the same issue with the patch, just in a different order, no? > > uacce_fops_open() >uacce = xa_load() >...rmmod Um, how is rmmod called if the file descriptor is open? That should not be possible if the owner of the file descriptor is properly set. Please fix that up. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On Fri, Jun 17, 2022 at 10:23:13PM +0800, Zhangfei Gao wrote:
> @@ -312,12 +345,20 @@ static ssize_t available_instances_show(struct device
> *dev,
> char *buf)
> {
> struct uacce_device *uacce = to_uacce_device(dev);
> + ssize_t ret;
>
> - if (!uacce->ops->get_available_instances)
> - return -ENODEV;
> + mutex_lock(&uacce_mutex);
> + if (!uacce->ops || !uacce->ops->get_available_instances) {
Doesn't the sysfs group go away with uacce_remove()? We shouldn't need
this check
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] uacce: fix concurrency of fops_open and uacce_remove
On Fri, Jun 17, 2022 at 02:05:21PM +0800, Zhangfei Gao wrote: > > The refcount only ensures that the uacce_device object is not freed as > > long as there are open fds. But uacce_remove() can run while there are > > open fds, or fds in the process of being opened. And atfer uacce_remove() > > runs, the uacce_device object still exists but is mostly unusable. For > > example once the module is freed, uacce->ops is not valid anymore. But > > currently uacce_fops_open() may dereference the ops in this case: > > > > uacce_fops_open() > > if (!uacce->parent->driver) > > /* Still valid, keep going */ > > ...rmmod > > uacce_remove() > > ... free_module() > > uacce->ops->get_queue() /* BUG */ > > uacce_remove should wait for uacce->queues_lock, until fops_open release the > lock. > If open happen just after the uacce_remove: unlock, uacce_bind_queue in open > should fail. Ah yes sorry, I lost sight of what this patch was adding. But we could have the same issue with the patch, just in a different order, no? uacce_fops_open() uacce = xa_load() ...rmmod uacce_remove() mutex_lock() mutex_unlock() mutex_lock() if (!uacce->parent->driver) /* Still valid, keep going */ parent->driver = NULL free_module() uacce->ops->get_queue() /* BUG */ > > Accessing uacce->ops after free_module() is a use-after-free. We need all > you men parent release the resources. > > the fops to synchronize with uacce_remove() to ensure they don't use any > > resource of the parent after it's been freed. > After fops_open, currently we are counting on parent driver stop all dma > first, then call uacce_remove, which is assumption. > Like drivers/crypto/hisilicon/zip/zip_main.c: hisi_qm_wait_task_finish, > which will wait uacce_release. > If comments this , there may other issue, > Unable to handle kernel paging request at virtual address 8b700204 > pc : hisi_qm_cache_wb.part.0+0x2c/0xa0 > > > I see uacce_fops_poll() may have the same problem, and should be inside > > uacce_mutex. > Do we need consider this, uacce_remove can happen anytime but not waiting > dma stop? No, the parent driver must stop DMA before calling uacce_remove(), there is no way around that > > Not sure uacce_mutex can do this. > Currently the sequence is > mutex_lock(&uacce->queues_lock); > mutex_lock(&uacce_mutex); We should document why some ops use one lock or the other. I believe it's to avoid circular lock dependency between ioctl and mmap, do you know if there was another reason? > > Or we set all the callbacks of uacce_ops to NULL? That would be cleaner, though we already use the queue state to indicate whether it is usable or not. I think we just need to extend that to all ops. How about the following patch? Unfortunately it still has the lock disparity between ioctl and mmap because of the circular lockking with mmap_sem, I don't know how to make that cleaner. --- 8< --- >From c7c2b051ec19285bbb973f8a2a5e58bb5326e00e Mon Sep 17 00:00:00 2001 From: Jean-Philippe Brucker Date: Mon, 20 Jun 2022 10:10:41 +0100 Subject: [PATCH] uacce: Tidy up locking The uacce driver must deal with a possible removal of the parent driver or device at any time. At the moment there are several issues that may result in use-after-free. Tidy up the locking to handle module removal. When unbinding the parent device from its driver, the driver calls uacce_remove(). This function removes the cdev, ensuring that no new uacce file descriptor will be opened, but existing fds are still open and uacce fops may be called after uacce_remove() completes, when the parent module is gone. Each open fd holds a reference to the uacce device, ensuring that the structure cannot be freed until all fds are closed. But the uacce fops may still access uacce->ops which belonged to the parent module, now freed. To solve this: * use the global uacce_mutex to serialize uacce_fops_open() against uacce_remove(), and q->mutex to serialize all other fops against uacce_remove(). * q->mutex replaces the less scalable uacce->queues_lock. The queues list is now protected by uacce_mutex, and the queue state by q->mutex. Note that scalability is only desirable for poll(), since the other fops are only used during setup. * uacce_queue_is_valid(), checked under q->mutex, denotes whether uacce_remove() has disabled all queues. If that is the case, don't go any further since the parent module may be gone. Any call to uacce->ops must be done while holding q->mutex to ensure the state doesn't change. * Clear uacce->ops i
Re: [PATCH v2 5/5] vfio/iommu_type1: Simplify group attachment
On 2022-06-17 03:53, Tian, Kevin wrote:
From: Nicolin Chen
Sent: Friday, June 17, 2022 6:41 AM
...
- if (resv_msi) {
+ if (resv_msi && !domain->msi_cookie) {
ret = iommu_get_msi_cookie(domain->domain,
resv_msi_base);
if (ret && ret != -ENODEV)
goto out_detach;
+ domain->msi_cookie = true;
}
why not moving to alloc_attach_domain() then no need for the new
domain field? It's required only when a new domain is allocated.
When reusing an existing domain that doesn't have an msi_cookie,
we can do iommu_get_msi_cookie() if resv_msi is found. So it is
not limited to a new domain.
Looks msi_cookie requirement is per platform (currently only
for smmu. see arm_smmu_get_resv_regions()). If there is
no mixed case then above check is not required.
But let's hear whether Robin has a different thought here.
Yes, the cookie should logically be tied to the lifetime of the domain
itself. In the relevant context, "an existing domain that doesn't have
an msi_cookie" should never exist.
Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
On 2022/6/20 16:31, Yi Liu wrote:
Hi Baolu,
On 2022/6/20 16:17, Lu Baolu wrote:
The IOMMU driver shares the pasid table for PCI alias devices. When the
RID2PASID entry of the shared pasid table has been filled by the first
device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
failed" failure as the pasid entry has already been marke as present. As
s/marke/marked/
Updated. Thanks!
the result, the IOMMU probing process will be aborted.
This fixes it by skipping RID2PASID setting if the pasid entry has been
populated. This works because the IOMMU core ensures that only the same
IOMMU domain can be attached to all PCI alias devices at the same time.
nit. this sentence is a little bit to interpret. :-) I guess what you want
to describe is the PCI alias devices should be attached to the same domain
instead of different domain. right?
Yes.
also, does it apply to all domain types? e.g. the SVA domains introduced
in "iommu: SVA and IOPF refactoring"
No. Only about the RID2PASID.
Therefore the subsequent devices just try to setup the RID2PASID entry
with the same domain, which is negligible.
Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID
support")
Reported-by: Chenyi Qiang
Cc: sta...@vger.kernel.org
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..b9966c01a2a2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
dmar_domain *domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu, domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
Hi Baolu,
On 2022/6/20 16:17, Lu Baolu wrote:
The IOMMU driver shares the pasid table for PCI alias devices. When the
RID2PASID entry of the shared pasid table has been filled by the first
device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
failed" failure as the pasid entry has already been marke as present. As
s/marke/marked/
the result, the IOMMU probing process will be aborted.
This fixes it by skipping RID2PASID setting if the pasid entry has been
populated. This works because the IOMMU core ensures that only the same
IOMMU domain can be attached to all PCI alias devices at the same time.
nit. this sentence is a little bit to interpret. :-) I guess what you want
to describe is the PCI alias devices should be attached to the same domain
instead of different domain. right?
also, does it apply to all domain types? e.g. the SVA domains introduced in
"iommu: SVA and IOPF refactoring"
Therefore the subsequent devices just try to setup the RID2PASID entry
with the same domain, which is negligible.
Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID support")
Reported-by: Chenyi Qiang
Cc: sta...@vger.kernel.org
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..b9966c01a2a2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct dmar_domain
*domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu, domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure
The IOMMU driver shares the pasid table for PCI alias devices. When the
RID2PASID entry of the shared pasid table has been filled by the first
device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
failed" failure as the pasid entry has already been marke as present. As
the result, the IOMMU probing process will be aborted.
This fixes it by skipping RID2PASID setting if the pasid entry has been
populated. This works because the IOMMU core ensures that only the same
IOMMU domain can be attached to all PCI alias devices at the same time.
Therefore the subsequent devices just try to setup the RID2PASID entry
with the same domain, which is negligible.
Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID support")
Reported-by: Chenyi Qiang
Cc: sta...@vger.kernel.org
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..b9966c01a2a2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct dmar_domain
*domain, struct device *dev)
ret = intel_pasid_setup_second_level(iommu, domain,
dev, PASID_RID2PASID);
spin_unlock_irqrestore(&iommu->lock, flags);
- if (ret) {
+ if (ret && ret != -EBUSY) {
dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(dev);
return ret;
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
