This series implements mitigations for lack of DMA access control on
systems without an IOMMU, which could result in the DMA accessing the
system memory at unexpected times and/or unexpected addresses, possibly
leading to data leakage or corruption.
For example, we plan to use the PCI-e bus for Wi-Fi and that PCI-e bus is
not behind an IOMMU. As PCI-e, by design, gives the device full access to
system memory, a vulnerability in the Wi-Fi firmware could easily escalate
to a full system exploit (remote wifi exploits: [1a], [1b] that shows a
full chain of exploits; [2], [3]).
To mitigate the security concerns, we introduce restricted DMA. Restricted
DMA utilizes the existing swiotlb to bounce streaming DMA in and out of a
specially allocated region and does memory allocation from the same region.
The feature on its own provides a basic level of protection against the DMA
overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system needs
to provide a way to restrict the DMA to a predefined memory region (this is
usually done at firmware level, e.g. MPU in ATF on some ARM platforms [4]).
[1a] https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_4.html
[1b] https://googleprojectzero.blogspot.com/2017/04/over-air-exploiting-broadcoms-wi-fi_11.html
[2] https://blade.tencent.com/en/advisories/qualpwn/
[3] https://www.bleepingcomputer.com/news/security/vulnerabilities-found-in-highly-popular-firmware-for-wifi-chips/
[4] https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132
v14:
- Move set_memory_decrypted before swiotlb_init_io_tlb_mem (patch 01/12, 10,12)
- Add Stefano's Acked-by tag from v13
v13:
- Fix xen-swiotlb issues
- memset in patch 01/12
- is_swiotlb_force_bounce in patch 06/12
- Fix the dts example typo in reserved-memory.txt
- Add Stefano and Will's Tested-by tag from v12
https://lore.kernel.org/patchwork/cover/1448001/
v12:
Split is_dev_swiotlb_force into is_swiotlb_force_bounce (patch 06/12) and
is_swiotlb_for_alloc (patch 09/12)
https://lore.kernel.org/patchwork/cover/1447254/
v11:
- Rebase against swiotlb devel/for-linus-5.14
- s/mempry/memory/g
- exchange the order of patch 09/12 and 10/12
https://lore.kernel.org/patchwork/cover/1447216/
v10:
Address the comments in v9 to
- fix the dev->dma_io_tlb_mem assignment
- propagate swiotlb_force setting into io_tlb_default_mem->force
- move set_memory_decrypted out of swiotlb_init_io_tlb_mem
- move debugfs_dir declaration into the main CONFIG_DEBUG_FS block
- add swiotlb_ prefix to find_slots and release_slots
- merge the 3 alloc/free related patches
- move the CONFIG_DMA_RESTRICTED_POOL later
https://lore.kernel.org/patchwork/cover/1446882/
v9:
Address the comments in v7 to
- set swiotlb active pool to dev->dma_io_tlb_mem
- get rid of get_io_tlb_mem
- dig out the device struct for is_swiotlb_active
- move debugfs_create_dir out of swiotlb_create_debugfs
- do set_memory_decrypted conditionally in swiotlb_init_io_tlb_mem
- use IS_ENABLED in kernel/dma/direct.c
- fix redefinition of 'of_dma_set_restricted_buffer'
https://lore.kernel.org/patchwork/cover/1445081/
v8:
- Fix reserved-memory.txt and add the reg property in example.
- Fix sizeof for of_property_count_elems_of_size in
drivers/of/address.c#of_dma_set_restricted_buffer.
- Apply Will's suggestion to try the OF node having DMA configuration in
drivers/of/address.c#of_dma_set_restricted_buffer.
- Fix typo in the comment of drivers/of/address.c#of_dma_set_restricted_buffer.
- Add error message for PageHighMem in
kernel/dma/swiotlb.c#rmem_swiotlb_device_init and move it to
rmem_swiotlb_setup.
- Fix the message string in rmem_swiotlb_setup.
https://lore.kernel.org/patchwork/cover/1437112/
v7:
Fix debugfs, PageHighMem and comment style in rmem_swiotlb_device_init
https://lore.kernel.org/patchwork/cover/1431031/
v6:
Address the comments in v5
https://lore.kernel.org/patchwork/cover/1423201/
v5:
Rebase on latest linux-next
https://lore.kernel.org/patchwork/cover/1416899/
v4:
- Fix spinlock bad magic
- Use rmem->name for debugfs entry
- Address the comments in v3
https://lore.kernel.org/patchwork/cover/1378113/
v3:
Using only one reserved memory region for both streaming DMA and memory
allocation.
https://lore.kernel.org/patchwork/cover/1360992/
v2:
Building on top of swiotlb.
https://lore.kernel.org/patchwork/cover/1280705/
v1:
Using dma_map_ops.
https://lore.kernel.org/patchwork/cover/1271660/
Claire Chang (12):
swiotlb: Refactor swiotlb init functions
swiotlb: Refactor swiotlb_create_debugfs
swiotlb: Set dev->dma_io_tlb_mem to the swiotlb pool used
swiotlb: Update is_swiotlb_buffer to add a struct device argument
swiotlb: Update is_swiotlb_active to add a struct device argument
swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
swiotlb: Move alloc_size to swiotlb_find_slots
swiotlb: Refactor swiotlb_tbl_unmap_single
swiotlb: Add restricted DMA alloc/free support
swiotlb: Add restricted DMA pool initialization
dt-bindings: of: Add restricted DMA pool
of: Add plumbing for restricted DMA pool
.../reserved-memory/reserved-memory.txt | 36 ++-
drivers/base/core.c | 4 +
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 +-
drivers/iommu/dma-iommu.c | 12 +-
drivers/of/address.c | 33 +++
drivers/of/device.c | 3 +
drivers/of/of_private.h | 6 +
drivers/pci/xen-pcifront.c | 2 +-
drivers/xen/swiotlb-xen.c | 4 +-
include/linux/device.h | 4 +
include/linux/swiotlb.h | 51 +++-
kernel/dma/Kconfig | 14 +
kernel/dma/direct.c | 59 +++--
kernel/dma/direct.h | 8 +-
kernel/dma/swiotlb.c | 250 +++++++++++++-----
16 files changed, 387 insertions(+), 103 deletions(-)
--
2.32.0.288.g62a8d224e6-goog
Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
initialization to make the code reusable.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
kernel/dma/swiotlb.c | 50 ++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 25 deletions(-)
@@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)if(!mem)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-mem->nslabs=nslabs;-mem->start=__pa(tlb);-mem->end=mem->start+bytes;-mem->index=0;-spin_lock_init(&mem->lock);-for(i=0;i<mem->nslabs;i++){-mem->slots[i].list=IO_TLB_SEGSIZE-io_tlb_offset(i);-mem->slots[i].orig_addr=INVALID_PHYS_ADDR;-mem->slots[i].alloc_size=0;-}++swiotlb_init_io_tlb_mem(mem,__pa(tlb),nslabs,false);io_tlb_default_mem=mem;if(verbose)
Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
use it to determine whether to bounce the data or not. This will be
useful later to allow for different pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
drivers/xen/swiotlb-xen.c | 2 +-
include/linux/swiotlb.h | 11 +++++++++++
kernel/dma/direct.c | 2 +-
kernel/dma/direct.h | 2 +-
kernel/dma/swiotlb.c | 4 ++++
5 files changed, 18 insertions(+), 3 deletions(-)
@@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev){/* If SWIOTLB is active, use its maximum mapping size */if(is_swiotlb_active(dev)&&-(dma_addressing_limited(dev)||swiotlb_force==SWIOTLB_FORCE))+(dma_addressing_limited(dev)||is_swiotlb_force_bounce(dev)))returnswiotlb_max_mapping_size(dev);returnSIZE_MAX;}
Add a new function, swiotlb_release_slots, to make the code reusable for
supporting different bounce buffer pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
kernel/dma/swiotlb.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
Rename find_slots to swiotlb_find_slots and move the maintenance of
alloc_size to it for better code reusability later.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
kernel/dma/swiotlb.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Always have the pointer to the swiotlb pool used in struct device. This
could help simplify the code for other pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
drivers/base/core.c | 4 ++++
include/linux/device.h | 4 ++++
kernel/dma/swiotlb.c | 8 ++++----
3 files changed, 12 insertions(+), 4 deletions(-)
@@ -495,7 +495,7 @@ int dma_direct_supported(struct device *dev, u64 mask)size_tdma_direct_max_mapping_size(structdevice*dev){/* If SWIOTLB is active, use its maximum mapping size */-if(is_swiotlb_active()&&+if(is_swiotlb_active(dev)&&(dma_addressing_limited(dev)||swiotlb_force==SWIOTLB_FORCE))returnswiotlb_max_mapping_size(dev);returnSIZE_MAX;
Add the functions, swiotlb_{alloc,free} and is_swiotlb_for_alloc to
support the memory allocation from restricted DMA pool.
The restricted DMA pool is preferred if available.
Note that since coherent allocation needs remapping, one must set up
another device coherent pool by shared-dma-pool and use
dma_alloc_from_dev_coherent instead for atomic coherent allocation.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
include/linux/swiotlb.h | 26 ++++++++++++++++++++++
kernel/dma/direct.c | 49 +++++++++++++++++++++++++++++++----------
kernel/dma/swiotlb.c | 38 ++++++++++++++++++++++++++++++--
3 files changed, 99 insertions(+), 14 deletions(-)
@@ -155,18 +174,23 @@ void *dma_direct_alloc(struct device *dev, size_t size,}if(!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED)&&-!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&-!dev_is_dma_coherent(dev))+!IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev)&&+!is_swiotlb_for_alloc(dev))returnarch_dma_alloc(dev,size,dma_handle,gfp,attrs);/**Remappingordecryptingmemorymayblock.Ifeitherisrequiredand*wecan'tblock,allocatethememoryfromtheatomicpools.+*IfrestrictedDMA(i.e.,is_swiotlb_for_alloc)isrequired,onemust+*setupanotherdevicecoherentpoolbyshared-dma-poolanduse+*dma_alloc_from_dev_coherentinstead.*/if(IS_ENABLED(CONFIG_DMA_COHERENT_POOL)&&!gfpflags_allow_blocking(gfp)&&(force_dma_unencrypted(dev)||-(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&!dev_is_dma_coherent(dev))))+(IS_ENABLED(CONFIG_DMA_DIRECT_REMAP)&&+!dev_is_dma_coherent(dev)))&&+!is_swiotlb_for_alloc(dev))returndma_direct_alloc_from_pool(dev,size,dma_handle,gfp);/* we always manually zero the memory once we are done */
Add the initialization function to create restricted DMA pools from
matching reserved-memory nodes.
Regardless of swiotlb setting, the restricted DMA pool is preferred if
available.
The restricted DMA pools provide a basic level of protection against the
DMA overwriting buffer contents at unexpected times. However, to protect
against general data leakage and system memory corruption, the system
needs to provide a way to lock down the memory access, e.g., MPU.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
include/linux/swiotlb.h | 3 +-
kernel/dma/Kconfig | 14 ++++++++
kernel/dma/swiotlb.c | 76 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 92 insertions(+), 1 deletion(-)
@@ -80,6 +80,20 @@ config SWIOTLBboolselectNEED_DMA_MAP_STATE+configDMA_RESTRICTED_POOL+bool"DMA Restricted Pool"+depends onOF&&OF_RESERVED_MEM+selectSWIOTLB+help+ThisenablessupportforrestrictedDMApoolswhichprovidealevelof+DMAmemoryprotectiononsystemswithlimitedhardwareprotection+capabilities,suchasthoselackinganIOMMU.++Formoreinformationsee+<Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt>+and<kernel/dma/swiotlb.c>.+Ifunsure,say"n".+## Should be selected if we can mmap non-coherent mappings to userspace.# The only thing that is really required is a way to set an uncached bit
@@ -736,4 +743,73 @@ bool swiotlb_free(struct device *dev, struct page *page, size_t size)returntrue;}+staticintrmem_swiotlb_device_init(structreserved_mem*rmem,+structdevice*dev)+{+structio_tlb_mem*mem=rmem->priv;+unsignedlongnslabs=rmem->size>>IO_TLB_SHIFT;++/*+*Sincemultipledevicescansharethesamepool,theprivatedata,+*io_tlb_memstruct,willbeinitializedbythefirstdeviceattached+*toit.+*/+if(!mem){+mem=kzalloc(struct_size(mem,slots,nslabs),GFP_KERNEL);+if(!mem)+return-ENOMEM;++set_memory_decrypted((unsignedlong)phys_to_virt(rmem->base),+rmem->size>>PAGE_SHIFT);+swiotlb_init_io_tlb_mem(mem,rmem->base,nslabs,false);+mem->force_bounce=true;+mem->for_alloc=true;++rmem->priv=mem;++if(IS_ENABLED(CONFIG_DEBUG_FS)){+mem->debugfs=+debugfs_create_dir(rmem->name,debugfs_dir);+swiotlb_create_debugfs_files(mem);+}+}++dev->dma_io_tlb_mem=mem;++return0;+}++staticvoidrmem_swiotlb_device_release(structreserved_mem*rmem,+structdevice*dev)+{+dev->dma_io_tlb_mem=io_tlb_default_mem;+}++staticconststructreserved_mem_opsrmem_swiotlb_ops={+.device_init=rmem_swiotlb_device_init,+.device_release=rmem_swiotlb_device_release,+};++staticint__initrmem_swiotlb_setup(structreserved_mem*rmem)+{+unsignedlongnode=rmem->fdt_node;++if(of_get_flat_dt_prop(node,"reusable",NULL)||+of_get_flat_dt_prop(node,"linux,cma-default",NULL)||+of_get_flat_dt_prop(node,"linux,dma-default",NULL)||+of_get_flat_dt_prop(node,"no-map",NULL))+return-EINVAL;++if(PageHighMem(pfn_to_page(PHYS_PFN(rmem->base)))){+pr_err("Restricted DMA pool must be accessible within the linear mapping.");+return-EINVAL;+}++rmem->ops=&rmem_swiotlb_ops;+pr_info("Reserved memory: created restricted DMA pool at %pa, size %ld MiB\n",+&rmem->base,(unsignedlong)rmem->size/SZ_1M);+return0;+}++RESERVEDMEM_OF_DECLARE(dma,"restricted-dma-pool",rmem_swiotlb_setup);#endif /* CONFIG_DMA_RESTRICTED_POOL */
Introduce the new compatible string, restricted-dma-pool, for restricted
DMA. One can specify the address and length of the restricted DMA memory
region by restricted-dma-pool in the reserved-memory node.
Signed-off-by: Claire Chang <redacted>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
.../reserved-memory/reserved-memory.txt | 36 +++++++++++++++++--
1 file changed, 33 insertions(+), 3 deletions(-)
@@ -51,6 +51,23 @@ compatible (optional) - standard definition used as a shared pool of DMA buffers for a set of devices. It can be used by an operating system to instantiate the necessary pool management subsystem if necessary.+ - restricted-dma-pool: This indicates a region of memory meant to be+ used as a pool of restricted DMA buffers for a set of devices. The+ memory region would be the only region accessible to those devices.+ When using this, the no-map and reusable properties must not be set,+ so the operating system can create a virtual mapping that will be used+ for synchronization. The main purpose for restricted DMA is to+ mitigate the lack of DMA access control on systems without an IOMMU,+ which could result in the DMA accessing the system memory at+ unexpected times and/or unexpected addresses, possibly leading to data+ leakage or corruption. The feature on its own provides a basic level+ of protection against the DMA overwriting buffer contents at+ unexpected times. However, to protect against general data leakage and+ system memory corruption, the system needs to provide way to lock down+ the memory access, e.g., MPU. Note that since coherent allocation+ needs remapping, one must set up another device coherent pool by+ shared-dma-pool and use dma_alloc_from_dev_coherent instead for atomic+ coherent allocation. - vendor specific string in the form <vendor>,[<device>-]<usage> no-map (optional) - empty property - Indicates the operating system must not create a virtual mapping
@@ -85,10 +102,11 @@ memory-region-names (optional) - a list of names, one for each corresponding Example --------This example defines 3 contiguous regions are defined for Linux kernel:+This example defines 4 contiguous regions for Linux kernel: one default of all device drivers (named linux,cma@72000000 and 64MiB in size),-one dedicated to the framebuffer device (named framebuffer@78000000, 8MiB), and-one for multimedia processing (named multimedia-memory@77000000, 64MiB).+one dedicated to the framebuffer device (named framebuffer@78000000, 8MiB),+one for multimedia processing (named multimedia-memory@77000000, 64MiB), and+one for restricted dma pool (named restricted_dma_reserved@0x50000000, 64MiB). / { #address-cells = <1>;
If a device is not behind an IOMMU, we look up the device node and set
up the restricted DMA when the restricted-dma-pool is presented.
Signed-off-by: Claire Chang <redacted>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
---
drivers/of/address.c | 33 +++++++++++++++++++++++++++++++++
drivers/of/device.c | 3 +++
drivers/of/of_private.h | 6 ++++++
3 files changed, 42 insertions(+)
Add a new function, swiotlb_init_io_tlb_mem, for the io_tlb_mem struct
initialization to make the code reusable.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
@@ -186,16 +205,8 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)if(!mem)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-mem->nslabs=nslabs;-mem->start=__pa(tlb);-mem->end=mem->start+bytes;-mem->index=0;-spin_lock_init(&mem->lock);-for(i=0;i<mem->nslabs;i++){-mem->slots[i].list=IO_TLB_SEGSIZE-io_tlb_offset(i);-mem->slots[i].orig_addr=INVALID_PHYS_ADDR;-mem->slots[i].alloc_size=0;-}++swiotlb_init_io_tlb_mem(mem,__pa(tlb),nslabs,false);io_tlb_default_mem=mem;if(verbose)
Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
use it to determine whether to bounce the data or not. This will be
useful later to allow for different pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
@@ -496,7 +496,7 @@ size_t dma_direct_max_mapping_size(struct device *dev){/* If SWIOTLB is active, use its maximum mapping size */if(is_swiotlb_active(dev)&&-(dma_addressing_limited(dev)||swiotlb_force==SWIOTLB_FORCE))+(dma_addressing_limited(dev)||is_swiotlb_force_bounce(dev)))returnswiotlb_max_mapping_size(dev);returnSIZE_MAX;}
From: Will Deacon <will@kernel.org> Date: 2021-06-23 18:37:51
On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote:
On 6/18/2021 11:40 PM, Claire Chang wrote:
quoted
Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
use it to determine whether to bounce the data or not. This will be
useful later to allow for different pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next.
Hmm, so that makes patch 7 the suspicious one, right?
Looking at that one more closely, it looks like swiotlb_find_slots() takes
'alloc_size + offset' as its 'alloc_size' parameter from
swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based
on 'alloc_size + offset', which looks like a change in behaviour from the
old code, which didn't include the offset there.
swiotlb_release_slots() then adds the offset back on afaict, so we end up
accounting for it twice and possibly unmap more than we're supposed to?
Will
On Wed, Jun 23, 2021 at 12:39:29PM -0400, Qian Cai wrote:
quoted
On 6/18/2021 11:40 PM, Claire Chang wrote:
quoted
Propagate the swiotlb_force into io_tlb_default_mem->force_bounce and
use it to determine whether to bounce the data or not. This will be
useful later to allow for different pools.
Signed-off-by: Claire Chang <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Will Deacon <will@kernel.org>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reverting the rest of the series up to this patch fixed a boot crash with NVMe on today's linux-next.
Hmm, so that makes patch 7 the suspicious one, right?
Will, no. It is rather patch #6 (this patch). Only the patch from #6 to #12 were reverted to fix the issue. Also, looking at this offset of the crash,
pc : dma_direct_map_sg+0x304/0x8f0
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
Looking at that one more closely, it looks like swiotlb_find_slots() takes
'alloc_size + offset' as its 'alloc_size' parameter from
swiotlb_tbl_map_single() and initialises 'mem->slots[i].alloc_size' based
on 'alloc_size + offset', which looks like a change in behaviour from the
old code, which didn't include the offset there.
swiotlb_release_slots() then adds the offset back on afaict, so we end up
accounting for it twice and possibly unmap more than we're supposed to?
Will
From: Christoph Hellwig <hch@lst.de> Date: 2021-06-24 05:43:24
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig [off-list ref] wrote:
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
quoted
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-06-24 11:14:55
On 2021-06-24 07:05, Claire Chang wrote:
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig [off-list ref] wrote:
quoted
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
quoted
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
What also looks odd is that the base "address" 0xdfff800000000000 is held in a couple of registers, but the offset 0xe looks too small to match up to any relevant structure member in that dereference chain :/
Robin.
From: Will Deacon <will@kernel.org> Date: 2021-06-24 11:19:10
On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote:
On 2021-06-24 07:05, Claire Chang wrote:
quoted
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig [off-list ref] wrote:
quoted
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
quoted
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
What also looks odd is that the base "address" 0xdfff800000000000 is held in
a couple of registers, but the offset 0xe looks too small to match up to any
relevant structure member in that dereference chain :/
FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't
been initialised but we dereference 'dev->dma_io_tlb_mem', so I think
Christoph's suggestion is needed regardless. But I agree that it won't help
with the issue reported by Qian Cai.
Qian Cai: please can you share your .config and your command line?
Thanks,
Will
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-06-24 11:34:24
On 2021-06-24 12:18, Will Deacon wrote:
On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote:
quoted
On 2021-06-24 07:05, Claire Chang wrote:
quoted
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig [off-list ref] wrote:
quoted
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
quoted
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
What also looks odd is that the base "address" 0xdfff800000000000 is held in
a couple of registers, but the offset 0xe looks too small to match up to any
relevant structure member in that dereference chain :/
FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't
been initialised but we dereference 'dev->dma_io_tlb_mem', so I think
Christoph's suggestion is needed regardless.
Ack to that - for SWIOTLB_NO_FORCE, io_tlb_default_mem will remain NULL. The massive jump in KernelCI baseline failures as of yesterday looks like every arm64 machine with less than 4GB of RAM blowing up...
Robin.
But I agree that it won't help
with the issue reported by Qian Cai.
Qian Cai: please can you share your .config and your command line?
Thanks,
Will
From: Will Deacon <will@kernel.org> Date: 2021-06-24 11:48:44
On Thu, Jun 24, 2021 at 12:34:09PM +0100, Robin Murphy wrote:
On 2021-06-24 12:18, Will Deacon wrote:
quoted
On Thu, Jun 24, 2021 at 12:14:39PM +0100, Robin Murphy wrote:
quoted
On 2021-06-24 07:05, Claire Chang wrote:
quoted
On Thu, Jun 24, 2021 at 1:43 PM Christoph Hellwig [off-list ref] wrote:
quoted
On Wed, Jun 23, 2021 at 02:44:34PM -0400, Qian Cai wrote:
quoted
is_swiotlb_force_bounce at /usr/src/linux-next/./include/linux/swiotlb.h:119
is_swiotlb_force_bounce() was the new function introduced in this patch here.
+static inline bool is_swiotlb_force_bounce(struct device *dev)
+{
+ return dev->dma_io_tlb_mem->force_bounce;
+}
To me the crash looks like dev->dma_io_tlb_mem is NULL. Can you
turn this into :
return dev->dma_io_tlb_mem && dev->dma_io_tlb_mem->force_bounce;
for a quick debug check?
I just realized that dma_io_tlb_mem might be NULL like Christoph
pointed out since swiotlb might not get initialized.
However, `Unable to handle kernel paging request at virtual address
dfff80000000000e` looks more like the address is garbage rather than
NULL?
I wonder if that's because dev->dma_io_tlb_mem is not assigned
properly (which means device_initialize is not called?).
What also looks odd is that the base "address" 0xdfff800000000000 is held in
a couple of registers, but the offset 0xe looks too small to match up to any
relevant structure member in that dereference chain :/
FWIW, I've managed to trigger a NULL dereference locally when swiotlb hasn't
been initialised but we dereference 'dev->dma_io_tlb_mem', so I think
Christoph's suggestion is needed regardless.
Ack to that - for SWIOTLB_NO_FORCE, io_tlb_default_mem will remain NULL. The
massive jump in KernelCI baseline failures as of yesterday looks like every
arm64 machine with less than 4GB of RAM blowing up...
Ok, diff below which attempts to tackle the offset issue I mentioned as
well. Qian Cai -- please can you try with these changes?
Will
--->8
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-06-24 15:57:18
On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote:
On 6/24/2021 7:48 AM, Will Deacon wrote:
quoted
Ok, diff below which attempts to tackle the offset issue I mentioned as
well. Qian Cai -- please can you try with these changes?
This works fine.
Cool. Let me squash this patch in #6 and rebase the rest of them.
Claire, could you check the devel/for-linus-5.14 say by end of today to
double check that I didn't mess anything up please?
Will,
Thank you for generating the fix! I am going to run it on x86 and Xen
to make sure all is good (granted last time I ran devel/for-linus-5.14
on that setup I didn't see any errors so I need to double check
I didn't do something silly like run a wrong kernel).
On Thu, Jun 24, 2021 at 11:56 PM Konrad Rzeszutek Wilk
[off-list ref] wrote:
On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote:
quoted
On 6/24/2021 7:48 AM, Will Deacon wrote:
quoted
Ok, diff below which attempts to tackle the offset issue I mentioned as
well. Qian Cai -- please can you try with these changes?
This works fine.
Cool. Let me squash this patch in #6 and rebase the rest of them.
Claire, could you check the devel/for-linus-5.14 say by end of today to
double check that I didn't mess anything up please?
Will,
Thank you for generating the fix! I am going to run it on x86 and Xen
to make sure all is good (granted last time I ran devel/for-linus-5.14
on that setup I didn't see any errors so I need to double check
I didn't do something silly like run a wrong kernel).
From: Konrad Rzeszutek Wilk <hidden> Date: 2021-06-24 19:22:24
On Thu, Jun 24, 2021 at 11:58:57PM +0800, Claire Chang wrote:
On Thu, Jun 24, 2021 at 11:56 PM Konrad Rzeszutek Wilk
[off-list ref] wrote:
quoted
On Thu, Jun 24, 2021 at 10:10:51AM -0400, Qian Cai wrote:
quoted
On 6/24/2021 7:48 AM, Will Deacon wrote:
quoted
Ok, diff below which attempts to tackle the offset issue I mentioned as
well. Qian Cai -- please can you try with these changes?
This works fine.
Cool. Let me squash this patch in #6 and rebase the rest of them.
Claire, could you check the devel/for-linus-5.14 say by end of today to
double check that I didn't mess anything up please?