[GIT PULL] dma-mapping fixes for Linux 5.18
The following changes since commit 4fe87e818ea492ade079cc01a31d088e445f8539: dma-mapping: move pgprot_decrypted out of dma_pgprot (2022-04-01 06:46:51 +0200) are available in the Git repository at: git://git.infradead.org/users/hch/dma-mapping.git tags/dma-mapping-5.18-2 for you to fetch changes up to 9e02977bfad006af328add9434c8bffa40e053bb: dma-direct: avoid redundant memory sync for swiotlb (2022-04-14 06:30:39 +0200) dma-mapping fixes for Linux 5.18 - avoid a double memory copy for swiotlb (Chao Gao) Chao Gao (1): dma-direct: avoid redundant memory sync for swiotlb kernel/dma/direct.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Align size in __arm_smmu_tlb_inv_range
On Thu, Apr 14, 2022 at 11:32:38AM +0100, Robin Murphy wrote:
> > By looking at the call trace within arm_smmu_* functions:
> >__arm_smmu_tlb_inv_range
> >arm_smmu_tlb_inv_range_asid
> >arm_smmu_mm_invalidate_range
> >{from mm_notifier_* functions}
> >
> > There's no address alignment check. Although I do think we
> > should fix the source who passes down the non-page-aligned
> > parameter, the SMMU driver shouldn't silently dead loop if
> > a set of unaligned inputs are given, IMHO.
>
> Oh, sure, I'm not saying we definitely don't need to fix anything, I'd
> just like to get a better understanding of *what* we're fixing. I'd have
> (naively) expected the mm layer to give us page-aligned quantities even
> in the SVA notifier case, so if we've got a clear off-by-one somewhere
> in that path we should fix that before just blindly over-invalidating to
> paper over it; if we still also want to be robust at the SMMU driver end
> just in case, something like "if (WARN_ON(num_pages == 0)) num_pages =
> 1;" might be more appropriate. However if it turns out that we *can*
> actually end up with unsanitised input from some userspace unmap
> interface getting this far, then a silent fixup is the best option, but
> if so I'd still like to confirm that we're rounding in the same
> direction as whoever touched the pagetables (since it can't have been us).
I got some details:
[ 1008.868735] mmap: ---__do_munmap: range [a4fd, a4fe] len
1
[ 1008.869183] ---arm_smmu_mm_invalidate_range: range [a4fd,
a4fe] len 10001
[ 1009.056127] [ cut here ]
[ 1009.345791] WARNING: CPU: 0 PID: 131 at
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c:189
arm_smmu_mm_invalidate_range+0x4c/0xa8
[ 1009.605439] Modules linked in: nvidia(O)
[ 1009.692799] CPU: 0 PID: 131 Comm: dmaTest Tainted: GW O
5.15.0-tegra #30
[ 1009.865535] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[ 1010.015871] pstate: 4045 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 1010.168191] pc : arm_smmu_mm_invalidate_range+0x4c/0xa8
[ 1010.283136] lr : arm_smmu_mm_invalidate_range+0x48/0xa8
[ 1010.397119] sp : 80001436fa60
[ 1010.469568] x29: 80001436fa60 x28: 1840be80 x27: 07b3fff0
[ 1010.629631] x26: 00e8589f0f43 x25: 1aa20288 x24:
[ 1010.786432] x23: 138c1000 x22: 1783aa00 x21: 1c021380
[ 1010.98] x20: a4fd x19: 00010001 x18:
[ 1011.101568] x17: 8e4b x16: 80001001 x15: 81a13a744e89
[ 1011.259839] x14: 00ce x13: 00ce x12:
[ 1011.415616] x11: 0010 x10: 09c0 x9 : 80001436f7f0
[ 1011.575552] x8 : 13563420 x7 : 1feb9180 x6 : 35aa
[ 1011.731775] x5 : x4 : 1feb29e0 x3 : 1feb5a78
[ 1011.887615] x2 : 66f9034381513000 x1 : x0 : 0051
[ 1012.042944] Call trace:
[ 1012.097919] arm_smmu_mm_invalidate_range+0x4c/0xa8
[ 1012.204480] __mmu_notifier_invalidate_range+0x68/0xb0
[ 1012.318208] unmap_page_range+0x730/0x740
[ 1012.405951] unmap_single_vma+0x4c/0xb0
[ 1012.521920] unmap_vmas+0x70/0xf0
[ 1012.633727] unmap_region+0xb0/0x110
[ 1012.753856] __do_munmap+0x36c/0x460
[ 1012.855168] __vm_munmap+0x70/0xd0
[ 1012.929791] __arm64_sys_munmap+0x34/0x50
[ 1013.018944] invoke_syscall.constprop.0+0x4c/0xe0
[ 1013.122047] do_el0_svc+0x50/0x150
[ 1013.196415] el0_svc+0x28/0xc0
[ 1013.262848] el0t_64_sync_handler+0xb0/0xc0
[ 1013.355584] el0t_64_sync+0x1a0/0x1a4
[ 1013.435903] ---[ end trace c95eb7dc909f29ba ]---
We can see from call trace and logs that the invalidation range
comes from __do_munmap() with end address = 0xa4fe.
The problem seems to be the difference between how mm and iommu
cores express their end addresses: mm core calculates end using
start + size, while iommu core subtracts 1 from that. So that
end address 0xa4fe should be 0xa4fd in iommu's
way.
Perhaps we should simply do something like the following?
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index d816759a6bcf..e280568bb513 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -183,7 +183,7 @@ static void arm_smmu_mm_invalidate_range(struct
mmu_notifier *mn,
{
struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn);
struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
- size_t size = end - start + 1;
+ size_t size = end - start;
if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
arm_smmu_tlb_inv_range_asid(start, size, smmu_mn->cd->asid,
Thanks
Nic
___
iommu mailing list
iommu@lists.linux-foundation.org
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2022/4/16 上午5:00, Jacob Pan wrote:
Hi zhangfei@foxmail.com,
On Fri, 15 Apr 2022 19:52:03 +0800, "zhangfei@foxmail.com"
wrote:
A PASID might be still used even though it is freed on mm exit.
process A:
sva_bind();
ioasid_alloc() = N; // Get PASID N for the mm
fork(): // spawn process B
exit();
ioasid_free(N);
process B:
device uses PASID N -> failure
sva_unbind();
Dave Hansen suggests to take a refcount on the mm whenever binding the
PASID to a device and drop the refcount on unbinding. The mm won't be
dropped if the PASID is still bound to it.
Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID
allocation and free it on mm exit")
Is process A's mm intended to be used by process B? Or you really should
use PASID N on process B's mm? If the latter, it may work for a while until
B changes mapping.
It seems you are just extending the life of a defunct mm?
From nginx code, the master process init resources, then fork daemon
process to take over,
then master process exit by itself.
src/core/nginx.c
main
ngx_ssl_init(log); -> openssl engine -> bind_fn -> sva_bind()
ngx_daemon(cycle->log)
src/os/unix/ngx_daemon.c
ngx_daemon(ngx_log_t *log)
{
int fd;
switch (fork()) {
case -1:
ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "fork() failed");
return NGX_ERROR;
case 0:
// the fork daemon process
break;
default:
// master process directly exit, and release mm as well as ioasid
exit(0);
}
// only daemon process
Thanks
Thanks,
Jacob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2022/4/15 下午8:37, Fenghua Yu wrote: Hi, Zhangfei, On Fri, Apr 15, 2022 at 07:52:03PM +0800, zhangfei@foxmail.com wrote: On 2022/4/15 下午6:50, Fenghua Yu wrote: Hi, Zhangfei, On Fri, Apr 15, 2022 at 06:14:09PM +0800, zhangfei@foxmail.com wrote: I download this patch from: https://lore.kernel.org/lkml/ylladl6umolll...@fyu1.sc.intel.com/raw git am to either v5.18-rc2 or the latest upstream without any issue. It is my copy paste issue. I have tested, nginx woks well. Great! Other than the following issue, Each time /sbin/nginx will alloc ioasid but not free. which I think it maybe nginx issue or the mis-usage, will ask there. Which nginx/openssl function is supposed to call kernel sva_unbind? I couldn't find the function in nginx tree. If nginx doesn't free ioasid, it will cause ioasid leak and memory leak. Yes In my case, sva_bind/unbind is from openssl_engine, bind_fn, not in nginx itself nginx will use openssl -> openssl engine. nginx: src/core/nginx.c main ngx_ssl_init(log); OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL) openssl_engine IMPLEMENT_DYNAMIC_BIND_FN(bind_fn) bind_fn sva_bind destroy sva_unbind But destroy seems not called in sbin/nginx -s quit. Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/13] iommu: Move bus setup to IOMMU device registration
On 2022/4/14 20:42, Robin Murphy wrote:
@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus)
*/
int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
{
- int err;
-
- if (ops == NULL) {
- bus->iommu_ops = NULL;
- return 0;
- }
-
- if (bus->iommu_ops != NULL)
+ if (bus->iommu_ops && ops && bus->iommu_ops != ops)
return -EBUSY;
bus->iommu_ops = ops;
Do we still need to keep above lines in bus_set_iommu()?
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/2] PCI: ACPI: Support Microsoft's "DmaProperty"
On Thu, Apr 14, 2022 at 04:15:47PM -0700, Rajat Jain via iommu wrote: > On Thu, Apr 7, 2022 at 12:17 PM Bjorn Helgaas wrote: > > On Fri, Mar 25, 2022 at 11:46:08AM -0700, Rajat Jain wrote: > > > Support the "DmaProperty" with the same semantics. This is useful for > > > internal PCI devices that do not hang off a PCIe rootport, but offer > > > an attack surface for DMA attacks (e.g. internal network devices). > > > > Same semantics as what? > > Er, I meant the same semantics as the "DmaProperty". Please also see below. "Support the 'DmaProperty' with the same semantics as 'DmaProperty'" doesn't help much, so there must be a little more to the story :) > > The MS description of "ExternalFacingPort" says: > > > > This ACPI object enables the operating system to identify externally > > exposed PCIe hierarchies, such as Thunderbolt. > > No, my patch doesn't have to do with this one. I know, but it's similar, and I'm just hoping we can deal with them consistently. > > and "DmaProperty" says: > > > > This ACPI object enables the operating system to identify internal > > PCIe hierarchies that are easily accessible by users (such as, > > Laptop M.2 PCIe slots accessible by way of a latch) and require > > protection by the OS Kernel DMA Protection mechanism. > > Yes, this is the property that my patch uses. Microsoft has agreed to > update this documentation (in a sideband thread that I also copied you > on), with the updated semantics that this property can be used to > identify any PCI devices that require Kernel DMA protection. i.e. the > property is not restricted to identify "internal PCIe hierarchies" > (starting at root port), but to "any PCI device". > > > I don't really understand why they called out "laptop M.2 PCIe slots" > > here. Is the idea that those are more accessible than a standard > > internal PCIe slot? Seems like a pretty small distinction to me. > > > > I can understand your example of internal network devices adding an > > attack surface. But I don't see how "DmaProperty" helps identify > > those. Wouldn't a NIC in a standard internal PCIe slot add the same > > attack surface? > > Yes it would. The attack surface is the same. They probably only > thought of devices external to the SoC (starting from a root port) > when designing this property and thus called out internal M.2 PCI > slots. But nowhave realized that this could be opened to any PCI > device. > > > + * Property also used by Microsoft Windows for same purpose, > > > + * (to implement DMA protection from a device, using the IOMMU). > > > + */ > > > + if (device_property_read_u8(>dev, "DmaProperty", )) > > > > The MS web page says a _DSD with this property must be implemented in > > the Root Port device scope, but we don't enforce that here. We *do* > > enforce it in pci_acpi_set_untrusted(). Shouldn't we do the same > > here? > > No, the whole point of doing this (please refer to the discussion on > the previous versions of this patch) was that we want to have a > property that is NOT limited to the root ports only. And we have > reached an agreement with Microsoft about that. We need to either mention the fact that we're going beyond what the MS web page says or (ideally, as you are doing) get the page updated with the semantics you need. > > But IIUC, device_property_read_u8() works for either ACPI or DT > > properties, and maybe there is interest in using this for DT systems. > > None of these appear in any in-tree DTs, but maybe it is important to > > handle these in DTs? > > > > If that's the case, this code would no longer be specific to ACPI and > > should be moved to somewhere that's compiled even when CONFIG_ACPI > > isn't set. > > I think unifying ACPI and GPIO systems to use the same code / function > to read the properties might be more work/investigation, because > reading the properties for ACPI system happens much later than DT > systems (For acpi systems, it happens in pci_acpi_setup() which is > called much later). Given that no one wants to use this for DT > systems, I'd prefer for this to be ACPI specific for now, and then we > can solve it for DT once someone needs it. I think it's OK to make it ACPI-specific for now. Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi zhangfei@foxmail.com,
On Fri, 15 Apr 2022 19:52:03 +0800, "zhangfei@foxmail.com"
wrote:
> >>> A PASID might be still used even though it is freed on mm exit.
> >>>
> >>> process A:
> >>> sva_bind();
> >>> ioasid_alloc() = N; // Get PASID N for the mm
> >>> fork(): // spawn process B
> >>> exit();
> >>> ioasid_free(N);
> >>>
> >>> process B:
> >>> device uses PASID N -> failure
> >>> sva_unbind();
> >>>
> >>> Dave Hansen suggests to take a refcount on the mm whenever binding the
> >>> PASID to a device and drop the refcount on unbinding. The mm won't be
> >>> dropped if the PASID is still bound to it.
> >>>
> >>> Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID
> >>> allocation and free it on mm exit")
> >>>
Is process A's mm intended to be used by process B? Or you really should
use PASID N on process B's mm? If the latter, it may work for a while until
B changes mapping.
It seems you are just extending the life of a defunct mm?
Thanks,
Jacob
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Zhangfei, On Fri, Apr 15, 2022 at 07:52:03PM +0800, zhangfei@foxmail.com wrote: > > On my X86 machine, nginx doesn't trigger the kernel sva binding function > > to allocate ioasid. I tried pre- nstalled nginx/openssl and also tried my > > built > > a few versions of nginx/openssl. nginx does call OPENSSL_init_ssl() but > > doesn't go to the binding function. Don't know if it's my configuration > > issue. > > Maybe you can give me some advice? > I am using openssl engine, which use crypto driver and using sva via uacce. > nginx -> openssl -> openssl engine -> sva related. uacce is not used on X86. That's why I cannot test IOASID/PASID by nginx on X86. I only can test the RFC patch by other test tools via IDXD driver which uses PASID on X86. Thanks. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail #forregzbot
TWIMC: this mail is primarily send for documentation purposes and for
regzbot, my Linux kernel regression tracking bot. These mails usually
contain '#forregzbot' in the subject, to make them easy to spot and filter.
#regzbot fixed-by: 4fe87e818ea492ade079cc0
On 31.03.22 08:51, Thorsten Leemhuis wrote:
> [TLDR: I'm adding the regression report below to regzbot, the Linux
> kernel regression tracking bot; all text you find below is compiled from
> a few templates paragraphs you might have encountered already already
> from similar mails.]
>
> Hi, this is your Linux kernel regression tracker. Sending this just to
> the lists, as it's already handled.
>
> On 30.03.22 19:51, Alex Xu (Hello71) wrote:
>>
>> After a recent kernel update, booting one of my machines causes it to
>> hang on a black screen. Pressing Lock keys on the USB keyboard does not
>> turn on the indicators, and the machine does not appear on the Ethernet
>> network. I don't have a serial port on this machine. I didn't try
>> netconsole, but I suspect it won't work.
>>
>> Setting mem_encrypt=0 seems to resolve the issue. Reverting f5ff79fddf0e
>> ("dma-mapping: remove CONFIG_DMA_REMAP") also appears to resolve the
>> issue.
>>
>> The machine in question has an AMD Ryzen 5 1600 and ASRock B450 Pro4.
>
> To be sure below issue doesn't fall through the cracks unnoticed, I'm
> adding it to regzbot, my Linux kernel regression tracking bot:
>
> #regzbot ^introduced f5ff79fddf0e
> #regzbot title dma: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD
> SME boot fail
> #regzbot ignore-activity
>
> If it turns out this isn't a regression, free free to remove it from the
> tracking by sending a reply to this thread containing a paragraph like
> "#regzbot invalid: reason why this is invalid" (without the quotes).
>
> Ciao, Thorsten
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 06/13] iommu/dart: Clean up bus_set_iommu()
On Thu, Apr 14, 2022, at 14:42, Robin Murphy wrote: > Stop calling bus_set_iommu() since it's now unnecessary, and simplify > the probe failure path accordingly. > > Signed-off-by: Robin Murphy Tested-by: Sven Peter Reviewed-by: Sven Peter Can't wait until that saga is completed :) Sven ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Zhangfei, On Fri, Apr 15, 2022 at 07:52:03PM +0800, zhangfei@foxmail.com wrote: > > > On 2022/4/15 下午6:50, Fenghua Yu wrote: > > Hi, Zhangfei, > > > > On Fri, Apr 15, 2022 at 06:14:09PM +0800, zhangfei@foxmail.com wrote: > > I download this patch from: > > https://lore.kernel.org/lkml/ylladl6umolll...@fyu1.sc.intel.com/raw > > git am to either v5.18-rc2 or the latest upstream without any issue. > It is my copy paste issue. > > I have tested, nginx woks well. Great! > > Other than the following issue, > Each time /sbin/nginx will alloc ioasid but not free. > which I think it maybe nginx issue or the mis-usage, will ask there. Which nginx/openssl function is supposed to call kernel sva_unbind? I couldn't find the function in nginx tree. If nginx doesn't free ioasid, it will cause ioasid leak and memory leak. > > Tested-by: Zhangfei Gao Thank you for your testing! > > > > > > It should work for arm. > > > > > > In fact I have a similar patch at hand but pending since I found an issue. > > > > > > I start & stop nginx via this cmd. > > > //start > > > sudo sbin/nginx // this alloc an ioasid=1 > > > //stop > > > sudo sbin/nginx -s quit // this does not free ioasid=1, but still alloc > > > ioasid=2. > > > So ioasid will keep allocated but not freed if continue start/stop nginx, > > > though not impact the nginx function. > > > > > > stop nginx with -s quit still calls > > > src/core/nginx.c > > > main -> ngx_ssl_init -> openssl engine: bind_fn -> ... -> alloc asid > > > But openssl engine: ENGINE_free is not called > > > > > > Still in checking nginx code. > > > > > > Or do you test with nginx? > > On my X86 machine, nginx doesn't trigger the kernel sva binding function > > to allocate ioasid. I tried pre- nstalled nginx/openssl and also tried my > > built > > a few versions of nginx/openssl. nginx does call OPENSSL_init_ssl() but > > doesn't go to the binding function. Don't know if it's my configuration > > issue. > > Maybe you can give me some advice? > I am using openssl engine, which use crypto driver and using sva via uacce. > nginx -> openssl -> openssl engine -> sva related. I'll do more nginx experiments. > > > > > I test the patch with a few internal test tools and observe mmget()/mmput() > > works fine in various cases. > OK, thanks Thank you very much! -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2022/4/15 下午6:50, Fenghua Yu wrote:
Hi, Zhangfei,
On Fri, Apr 15, 2022 at 06:14:09PM +0800, zhangfei@foxmail.com wrote:
On 2022/4/15 下午5:51, Fenghua Yu wrote:
On Thu, Apr 14, 2022 at 06:08:09PM +0800, zhangfei@foxmail.com wrote:
On 2022/4/12 下午11:35, zhangfei@foxmail.com wrote:
Hi, Fenghua
On 2022/4/12 下午9:41, Fenghua Yu wrote:
From a6444e1e5bd8076f5e5c5e950d3192de327f0c9c Mon Sep 17 00:00:00 2001
From: Fenghua Yu
Date: Fri, 15 Apr 2022 00:51:33 -0700
Subject: [RFC PATCH] iommu/sva: Fix PASID use-after-free issue
A PASID might be still used even though it is freed on mm exit.
process A:
sva_bind();
ioasid_alloc() = N; // Get PASID N for the mm
fork(): // spawn process B
exit();
ioasid_free(N);
process B:
device uses PASID N -> failure
sva_unbind();
Dave Hansen suggests to take a refcount on the mm whenever binding the
PASID to a device and drop the refcount on unbinding. The mm won't be
dropped if the PASID is still bound to it.
Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID allocation and free
it on mm exit")
Reported-by: Zhangfei Gao
Suggested-by: Dave Hansen"
Signed-off-by: Fenghua Yu
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++
drivers/iommu/intel/svm.c | 4
2 files changed, 10 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 22ddd05bbdcd..3fcb842a0df0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include "arm-smmu-v3.h"
#include "../../iommu-sva-lib.h"
@@ -363,6 +364,9 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
void *drvdata)
mutex_lock(_lock);
handle = __arm_smmu_sva_bind(dev, mm);
+ /* Take an mm refcount on a successful bind. */
+ if (!IS_ERR(handle))
+ mmget(mm);
mutex_unlock(_lock);
return handle;
}
@@ -372,6 +376,8 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
struct arm_smmu_bond *bond = sva_to_bond(handle);
mutex_lock(_lock);
+ /* Drop an mm refcount. */
+ mmput(bond->mm);
if (refcount_dec_and_test(>refs)) {
list_del(>list);
arm_smmu_mmu_notifier_put(bond->smmu_mn);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a38763c1d1..345a0d5d7922 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -403,6 +403,8 @@ static struct iommu_sva *intel_svm_bind_mm(struct
intel_iommu *iommu,
goto free_sdev;
list_add_rcu(>list, >devs);
+ /* Take an mm refcount on binding mm. */
+ mmget(mm);
success:
return >sva;
@@ -465,6 +467,8 @@ static int intel_svm_unbind_mm(struct device *dev, u32
pasid)
kfree(svm);
}
}
+ /* Drop an mm reference on unbinding mm. */
+ mmput(mm);
}
out:
return ret;
This patch can not be applied on 5.18-rc2 for intel part.
What error do you see? Could you please send to me errors?
I download this patch from:
https://lore.kernel.org/lkml/ylladl6umolll...@fyu1.sc.intel.com/raw
git am to either v5.18-rc2 or the latest upstream without any issue.
It is my copy paste issue.
I have tested, nginx woks well.
Other than the following issue,
Each time /sbin/nginx will alloc ioasid but not free.
which I think it maybe nginx issue or the mis-usage, will ask there.
Tested-by: Zhangfei Gao
It should work for arm.
In fact I have a similar patch at hand but pending since I found an issue.
I start & stop nginx via this cmd.
//start
sudo sbin/nginx // this alloc an ioasid=1
//stop
sudo sbin/nginx -s quit // this does not free ioasid=1, but still alloc
ioasid=2.
So ioasid will keep allocated but not freed if continue start/stop nginx,
though not impact the nginx function.
stop nginx with -s quit still calls
src/core/nginx.c
main -> ngx_ssl_init -> openssl engine: bind_fn -> ... -> alloc asid
But openssl engine: ENGINE_free is not called
Still in checking nginx code.
Or do you test with nginx?
On my X86 machine, nginx doesn't trigger the kernel sva binding function
to allocate ioasid. I tried pre- nstalled nginx/openssl and also tried my built
a few versions of nginx/openssl. nginx does call OPENSSL_init_ssl() but
doesn't go to the binding function. Don't know if it's my configuration issue.
Maybe you can give me some advice?
I am using openssl engine, which use crypto driver and using sva via uacce.
nginx -> openssl -> openssl engine -> sva related.
I test the patch with a few internal test tools and observe mmget()/mmput()
works fine in various cases.
OK, thanks
___
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Zhangfei,
On Fri, Apr 15, 2022 at 06:14:09PM +0800, zhangfei@foxmail.com wrote:
>
>
> On 2022/4/15 下午5:51, Fenghua Yu wrote:
> > On Thu, Apr 14, 2022 at 06:08:09PM +0800, zhangfei@foxmail.com wrote:
> > > On 2022/4/12 下午11:35, zhangfei@foxmail.com wrote:
> > > > Hi, Fenghua
> > > >
> > > > On 2022/4/12 下午9:41, Fenghua Yu wrote:
> > From a6444e1e5bd8076f5e5c5e950d3192de327f0c9c Mon Sep 17 00:00:00 2001
> > From: Fenghua Yu
> > Date: Fri, 15 Apr 2022 00:51:33 -0700
> > Subject: [RFC PATCH] iommu/sva: Fix PASID use-after-free issue
> >
> > A PASID might be still used even though it is freed on mm exit.
> >
> > process A:
> > sva_bind();
> > ioasid_alloc() = N; // Get PASID N for the mm
> > fork(): // spawn process B
> > exit();
> > ioasid_free(N);
> >
> > process B:
> > device uses PASID N -> failure
> > sva_unbind();
> >
> > Dave Hansen suggests to take a refcount on the mm whenever binding the
> > PASID to a device and drop the refcount on unbinding. The mm won't be
> > dropped if the PASID is still bound to it.
> >
> > Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID allocation
> > and free it on mm exit")
> >
> > Reported-by: Zhangfei Gao
> > Suggested-by: Dave Hansen"
> > Signed-off-by: Fenghua Yu
> > ---
> > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++
> > drivers/iommu/intel/svm.c | 4
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > index 22ddd05bbdcd..3fcb842a0df0 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > @@ -7,6 +7,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "arm-smmu-v3.h"
> > #include "../../iommu-sva-lib.h"
> > @@ -363,6 +364,9 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct
> > *mm, void *drvdata)
> > mutex_lock(_lock);
> > handle = __arm_smmu_sva_bind(dev, mm);
> > + /* Take an mm refcount on a successful bind. */
> > + if (!IS_ERR(handle))
> > + mmget(mm);
> > mutex_unlock(_lock);
> > return handle;
> > }
> > @@ -372,6 +376,8 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
> > struct arm_smmu_bond *bond = sva_to_bond(handle);
> > mutex_lock(_lock);
> > + /* Drop an mm refcount. */
> > + mmput(bond->mm);
> > if (refcount_dec_and_test(>refs)) {
> > list_del(>list);
> > arm_smmu_mmu_notifier_put(bond->smmu_mn);
> > diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> > index 23a38763c1d1..345a0d5d7922 100644
> > --- a/drivers/iommu/intel/svm.c
> > +++ b/drivers/iommu/intel/svm.c
> > @@ -403,6 +403,8 @@ static struct iommu_sva *intel_svm_bind_mm(struct
> > intel_iommu *iommu,
> > goto free_sdev;
> > list_add_rcu(>list, >devs);
> > + /* Take an mm refcount on binding mm. */
> > + mmget(mm);
> > success:
> > return >sva;
> > @@ -465,6 +467,8 @@ static int intel_svm_unbind_mm(struct device *dev, u32
> > pasid)
> > kfree(svm);
> > }
> > }
> > + /* Drop an mm reference on unbinding mm. */
> > + mmput(mm);
> > }
> > out:
> > return ret;
> This patch can not be applied on 5.18-rc2 for intel part.
What error do you see? Could you please send to me errors?
I download this patch from:
https://lore.kernel.org/lkml/ylladl6umolll...@fyu1.sc.intel.com/raw
git am to either v5.18-rc2 or the latest upstream without any issue.
> It should work for arm.
>
> In fact I have a similar patch at hand but pending since I found an issue.
>
> I start & stop nginx via this cmd.
> //start
> sudo sbin/nginx // this alloc an ioasid=1
> //stop
> sudo sbin/nginx -s quit // this does not free ioasid=1, but still alloc
> ioasid=2.
> So ioasid will keep allocated but not freed if continue start/stop nginx,
> though not impact the nginx function.
>
> stop nginx with -s quit still calls
> src/core/nginx.c
> main -> ngx_ssl_init -> openssl engine: bind_fn -> ... -> alloc asid
> But openssl engine: ENGINE_free is not called
>
> Still in checking nginx code.
>
> Or do you test with nginx?
On my X86 machine, nginx doesn't trigger the kernel sva binding function
to allocate ioasid. I tried pre- nstalled nginx/openssl and also tried my built
a few versions of nginx/openssl. nginx does call OPENSSL_init_ssl() but
doesn't go to the binding function. Don't know if it's my configuration issue.
Maybe you can give me some advice?
I test the patch with a few internal test tools and observe mmget()/mmput()
works fine in various cases.
Thanks.
-Fenghua
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On 2022/4/15 下午5:51, Fenghua Yu wrote:
On Thu, Apr 14, 2022 at 06:08:09PM +0800, zhangfei@foxmail.com wrote:
On 2022/4/12 下午11:35, zhangfei@foxmail.com wrote:
Hi, Fenghua
On 2022/4/12 下午9:41, Fenghua Yu wrote:
Hi, Zhangfei,
On Tue, Apr 12, 2022 at 03:04:09PM +0800, zhangfei@foxmail.com
wrote:
On 2022/4/11 下午10:52, Dave Hansen wrote:
On 4/11/22 07:44, zhangfei@foxmail.com wrote:
On 2022/4/11 下午10:36, Dave Hansen wrote:
On 4/11/22 07:20, zhangfei@foxmail.com wrote:
Agree with Dave, I think user space should not be broken.
Thanks
Any plan about this regression?
Currently I need this patch to workaround the issue.
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 22ddd05bbdcd..2d74ac53d11c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -4,6 +4,7 @@
*/
#include
+#include
#include
#include
#include
@@ -363,6 +364,7 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct
*mm, void *drvdata)
mutex_lock(_lock);
handle = __arm_smmu_sva_bind(dev, mm);
+ mmget(mm);
mutex_unlock(_lock);
return handle;
}
@@ -377,6 +379,7 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
arm_smmu_mmu_notifier_put(bond->smmu_mn);
kfree(bond);
}
+ mmput(bond->mm);
mutex_unlock(_lock);
}
Could you please review and/or test this patch? It's supposed to fix
the PASID issue on both ARM and X86.
From a6444e1e5bd8076f5e5c5e950d3192de327f0c9c Mon Sep 17 00:00:00 2001
From: Fenghua Yu
Date: Fri, 15 Apr 2022 00:51:33 -0700
Subject: [RFC PATCH] iommu/sva: Fix PASID use-after-free issue
A PASID might be still used even though it is freed on mm exit.
process A:
sva_bind();
ioasid_alloc() = N; // Get PASID N for the mm
fork(): // spawn process B
exit();
ioasid_free(N);
process B:
device uses PASID N -> failure
sva_unbind();
Dave Hansen suggests to take a refcount on the mm whenever binding the
PASID to a device and drop the refcount on unbinding. The mm won't be
dropped if the PASID is still bound to it.
Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID allocation and free
it on mm exit")
Reported-by: Zhangfei Gao
Suggested-by: Dave Hansen"
Signed-off-by: Fenghua Yu
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++
drivers/iommu/intel/svm.c | 4
2 files changed, 10 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 22ddd05bbdcd..3fcb842a0df0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include "arm-smmu-v3.h"
#include "../../iommu-sva-lib.h"
@@ -363,6 +364,9 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
void *drvdata)
mutex_lock(_lock);
handle = __arm_smmu_sva_bind(dev, mm);
+ /* Take an mm refcount on a successful bind. */
+ if (!IS_ERR(handle))
+ mmget(mm);
mutex_unlock(_lock);
return handle;
}
@@ -372,6 +376,8 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
struct arm_smmu_bond *bond = sva_to_bond(handle);
mutex_lock(_lock);
+ /* Drop an mm refcount. */
+ mmput(bond->mm);
if (refcount_dec_and_test(>refs)) {
list_del(>list);
arm_smmu_mmu_notifier_put(bond->smmu_mn);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a38763c1d1..345a0d5d7922 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -403,6 +403,8 @@ static struct iommu_sva *intel_svm_bind_mm(struct
intel_iommu *iommu,
goto free_sdev;
list_add_rcu(>list, >devs);
+ /* Take an mm refcount on binding mm. */
+ mmget(mm);
success:
return >sva;
@@ -465,6 +467,8 @@ static int intel_svm_unbind_mm(struct device *dev, u32 pasid)
kfree(svm);
}
}
+ /* Drop an mm reference on unbinding mm. */
+ mmput(mm);
}
out:
return ret;
This patch can not be applied on 5.18-rc2 for intel part.
It should work for arm.
In fact I have a similar patch at hand but pending since I found an issue.
I start & stop nginx via this cmd.
//start
sudo sbin/nginx // this alloc an ioasid=1
//stop
sudo sbin/nginx -s quit // this does not free ioasid=1, but still
alloc ioasid=2.
So ioasid will keep allocated but not freed if continue start/stop
nginx, though not impact the nginx function.
stop nginx with -s quit still calls
src/core/nginx.c
main -> ngx_ssl_init -> openssl engine: bind_fn -> ... -> alloc
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
Hi, Dave, On Tue, Apr 12, 2022 at 07:39:10AM -0700, Dave Hansen wrote: > On 4/12/22 06:41, Fenghua Yu wrote: > >> master process quit, mmput -> mm_pasid_drop->ioasid_free > >> But this ignore driver's iommu_sva_unbind_device function, > >> iommu_sva_bind_device and iommu_sva_unbind_device are not pair, So driver > >> does not know ioasid is freed. > >> > >> Any suggestion? > > ioasid is per process or per mm. A daemon process shouldn't share the same > > ioasid with any other process with even its parent process. Its parent gets > > an ioasid and frees it on exit. The ioasid is gone and shouldn't be used > > by its child process. > > > > Each daemon process should call driver -> iommu_sva_bind_device -> > > ioasid_alloc > > to get its own ioasid/PASID. On daemon quit, the ioasid is freed. > > > > That means nqnix needs to be changed. > > Fenghua, please step back for a second and look at what you are saying. > Your patch caused userspace to break. Now, you're telling someone that > they need to go change that userspace to work around something that your > patch. How, exactly, are you suggesting that nginx could change to fix > this? What, specifically, was it doing with *fork()* that was wrong? > > It sounds to me like you're saying that it's OK to break userspace. You are right. The patch should not break userspace. I follow your suggestion to fix the issue by mmget() in binding and mmput() in unbinding. The RFC patch was sent out in another thread. Please review it. Thank you very much for your advice. -Fenghua ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit
On Thu, Apr 14, 2022 at 06:08:09PM +0800, zhangfei@foxmail.com wrote:
>
> On 2022/4/12 下午11:35, zhangfei@foxmail.com wrote:
> > Hi, Fenghua
> >
> > On 2022/4/12 下午9:41, Fenghua Yu wrote:
> > > Hi, Zhangfei,
> > >
> > > On Tue, Apr 12, 2022 at 03:04:09PM +0800, zhangfei@foxmail.com
> > > wrote:
> > > >
> > > > On 2022/4/11 下午10:52, Dave Hansen wrote:
> > > > > On 4/11/22 07:44, zhangfei@foxmail.com wrote:
> > > > > > On 2022/4/11 下午10:36, Dave Hansen wrote:
> > > > > > > On 4/11/22 07:20, zhangfei@foxmail.com wrote:
> > Agree with Dave, I think user space should not be broken.
> >
> > Thanks
>
> Any plan about this regression?
> Currently I need this patch to workaround the issue.
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 22ddd05bbdcd..2d74ac53d11c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -4,6 +4,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -363,6 +364,7 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct
> *mm, void *drvdata)
>
> mutex_lock(_lock);
> handle = __arm_smmu_sva_bind(dev, mm);
> + mmget(mm);
> mutex_unlock(_lock);
> return handle;
> }
> @@ -377,6 +379,7 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
> arm_smmu_mmu_notifier_put(bond->smmu_mn);
> kfree(bond);
> }
> + mmput(bond->mm);
> mutex_unlock(_lock);
> }
Could you please review and/or test this patch? It's supposed to fix
the PASID issue on both ARM and X86.
From a6444e1e5bd8076f5e5c5e950d3192de327f0c9c Mon Sep 17 00:00:00 2001
From: Fenghua Yu
Date: Fri, 15 Apr 2022 00:51:33 -0700
Subject: [RFC PATCH] iommu/sva: Fix PASID use-after-free issue
A PASID might be still used even though it is freed on mm exit.
process A:
sva_bind();
ioasid_alloc() = N; // Get PASID N for the mm
fork(): // spawn process B
exit();
ioasid_free(N);
process B:
device uses PASID N -> failure
sva_unbind();
Dave Hansen suggests to take a refcount on the mm whenever binding the
PASID to a device and drop the refcount on unbinding. The mm won't be
dropped if the PASID is still bound to it.
Fixes: 701fac40384f ("iommu/sva: Assign a PASID to mm on PASID allocation and
free it on mm exit")
Reported-by: Zhangfei Gao
Suggested-by: Dave Hansen"
Signed-off-by: Fenghua Yu
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++
drivers/iommu/intel/svm.c | 4
2 files changed, 10 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 22ddd05bbdcd..3fcb842a0df0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include "arm-smmu-v3.h"
#include "../../iommu-sva-lib.h"
@@ -363,6 +364,9 @@ arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
void *drvdata)
mutex_lock(_lock);
handle = __arm_smmu_sva_bind(dev, mm);
+ /* Take an mm refcount on a successful bind. */
+ if (!IS_ERR(handle))
+ mmget(mm);
mutex_unlock(_lock);
return handle;
}
@@ -372,6 +376,8 @@ void arm_smmu_sva_unbind(struct iommu_sva *handle)
struct arm_smmu_bond *bond = sva_to_bond(handle);
mutex_lock(_lock);
+ /* Drop an mm refcount. */
+ mmput(bond->mm);
if (refcount_dec_and_test(>refs)) {
list_del(>list);
arm_smmu_mmu_notifier_put(bond->smmu_mn);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 23a38763c1d1..345a0d5d7922 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -403,6 +403,8 @@ static struct iommu_sva *intel_svm_bind_mm(struct
intel_iommu *iommu,
goto free_sdev;
list_add_rcu(>list, >devs);
+ /* Take an mm refcount on binding mm. */
+ mmget(mm);
success:
return >sva;
@@ -465,6 +467,8 @@ static int intel_svm_unbind_mm(struct device *dev, u32
pasid)
kfree(svm);
}
}
+ /* Drop an mm reference on unbinding mm. */
+ mmput(mm);
}
out:
return ret;
--
2.32.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] crypto: qat - stop using iommu_present()
On Tue, Apr 05, 2022 at 01:25:11PM +0100, Robin Murphy wrote: > Even if an IOMMU might be present for some PCI segment in the system, > that doesn't necessarily mean it provides translation for the device > we care about. Replace iommu_present() with a more appropriate check. > > Signed-off-by: Robin Murphy > --- > drivers/crypto/qat/qat_common/adf_sriov.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
