| Age | Commit message (Collapse) | Author |
|---|
|
amd_iommu_pdom_id_alloc() returns an int: a domain ID on success, or the
negative errno from ida_alloc_range() when the ID space is exhausted or
memory is short. amd_iommu_alloc_domain_nested() stores that return value
in gdom_info->hdom_id, which is a u32, and only then tests it:
gdom_info->hdom_id = amd_iommu_pdom_id_alloc();
if (gdom_info->hdom_id <= 0) {
The assignment discards the sign, so -ENOSPC becomes 0xffffffe4 and the
test never fires. The nested domain is then set up with a host domain ID
that was never allocated, instead of the allocation failing with -ENOSPC.
Keep the value in an int, test it there, and store it only once it is
known to be valid, which is what the other amd_iommu_pdom_id_alloc()
callers already do.
Fixes: 757d2b1fdf5b ("iommu/amd: Introduce gDomID-to-hDomID Mapping and handle parent domain invalidation")
Signed-off-by: Hemanth Selam <hemanth.selam@gmail.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
|
|
A couple of runs of different AI tools have generated something like the
following bug report:
In nested_domain_free(), when refcount_dec_and_test() returns false
(other nested domains still reference the same gdom_info), the function
returns without calling kfree(ndom), leaking the nested_domain
structure. This problem wasn't introduced by this patch, but exists in
the code from commit 757d2b1fdf5b that the patch modifies. Each
nested_domain (ndom) is allocated individually in
amd_iommu_alloc_domain_nested() via kzalloc_obj(*ndom). The .free
callback is the sole point responsible for freeing this domain. When
the refcount is > 0, only the xa_unlock_irqrestore is performed and the
function returns, leaving ndom permanently allocated. This leak occurs
every time a nested domain sharing a gDomID is destroyed while other
domains still use that gDomID.
There is a similar leak later in this function in the WARN_ON() test when
the mapping is already NULL. Switch to a RAII-based cleanup for ndom, since
it should always be freed in this function.
Fixes: 757d2b1fdf5b ("iommu/amd: Introduce gDomID-to-hDomID Mapping and handle parent domain invalidation")
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Ankit Soni <Ankit.Soni@amd.com>
Signed-off-by: Will Deacon <will@kernel.org>
|
|
Lockdep complains:
[ 259.410489] =====================================================
[ 259.417287] WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected
[ 259.424667] 7.0.0-g51db1d8d2113 #54 Not tainted
[ 259.429718] -----------------------------------------------------
[ 259.436516] qemu-system-x86/10143 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
[ 259.444670] ff3b2b1c60305170 (&xa->xa_lock#25){+.+.}-{3:3}, at: __domain_flush_pages+0x17c/0x4b0
[ 259.454485]
and this task is already holding:
[ 259.460991] ff3b2b1c98504cc0 (&domain->lock){-.-.}-{3:3}, at: amd_iommu_iotlb_sync+0x25/0x60
[ 259.470408] which would create a new lock dependency:
[ 259.476041] (&domain->lock){-.-.}-{3:3} -> (&xa->xa_lock#25){+.+.}-{3:3}
[ 259.483615]
but this new dependency connects a HARDIRQ-irq-safe lock:
[ 259.492447] (&domain->lock){-.-.}-{3:3}
[ 259.492449]
... which became HARDIRQ-irq-safe at:
[ 259.503705] lock_acquire+0xb6/0x2e0
[ 259.507790] _raw_spin_lock_irqsave+0x3e/0x60
[ 259.512748] amd_iommu_flush_iotlb_all+0x20/0x50
[ 259.517996] iommu_dma_free_iova.isra.0+0x1b8/0x1e0
[ 259.523534] __iommu_dma_unmap+0xc2/0x140
[ 259.528100] iommu_dma_unmap_phys+0x55/0xc0
[ 259.532863] dma_unmap_phys+0x274/0x2e0
[ 259.537238] dma_unmap_page_attrs+0x17/0x30
[ 259.542000] nvme_unmap_data+0x13e/0x280
[ 259.546473] nvme_pci_complete_batch+0x45/0x70
[ 259.551524] nvme_irq+0x83/0x90
[ 259.555123] __handle_irq_event_percpu+0x92/0x360
[ 259.560466] handle_irq_event+0x39/0x80
[ 259.564841] handle_edge_irq+0xb2/0x1a0
[ 259.569214] __common_interrupt+0x4e/0x130
[ 259.573882] common_interrupt+0x88/0xa0
[ 259.578256] asm_common_interrupt+0x27/0x40
[ 259.583019] cpuidle_enter_state+0x119/0x5d0
[ 259.587877] cpuidle_enter+0x2e/0x50
[ 259.591962] do_idle+0x153/0x2c0
[ 259.595657] cpu_startup_entry+0x29/0x30
[ 259.600128] start_secondary+0x118/0x150
[ 259.604601] common_startup_64+0x13e/0x141
[ 259.609266]
to a HARDIRQ-irq-unsafe lock:
[ 259.615384] (&xa->xa_lock#25){+.+.}-{3:3}
[ 259.615386]
... which became HARDIRQ-irq-unsafe at:
[ 259.627039] ...
[ 259.627039] lock_acquire+0xb6/0x2e0
[ 259.633071] _raw_spin_lock+0x2f/0x50
[ 259.637250] amd_iommu_alloc_domain_nested+0x140/0x3c0
[ 259.643078] iommufd_hwpt_alloc+0x272/0x800 [iommufd]
[ 259.648813] iommufd_fops_ioctl+0x14e/0x200 [iommufd]
[ 259.654547] __x64_sys_ioctl+0x9d/0xf0
...
Since amd_iommu_domain_flush_pages() necessarily holds domain->lock to do the
flush, switch the allocation side in gdom_info_load_or_alloc_locked() to
HARDIRQ-safe allocation. The IOMMU_DESTROY->free path has the same issue,
so switch that path to HARDIRQ-safe locking as well.
Fixes: 757d2b1fdf5b ("iommu/amd: Introduce gDomID-to-hDomID Mapping and handle parent domain invalidation")
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Ankit Soni <Ankit.Soni@amd.com>
Signed-off-by: Will Deacon <will@kernel.org>
|
|
This was done entirely with mindless brute force, using
git grep -l '\<k[vmz]*alloc_objs*(.*, GFP_KERNEL)' |
xargs sed -i 's/\(alloc_objs*(.*\), GFP_KERNEL)/\1)/'
to convert the new alloc_obj() users that had a simple GFP_KERNEL
argument to just drop that argument.
Note that due to the extreme simplicity of the scripting, any slightly
more complex cases spread over multiple lines would not be triggered:
they definitely exist, but this covers the vast bulk of the cases, and
the resulting diff is also then easier to check automatically.
For the same reason the 'flex' versions will be done as a separate
conversion.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
|
|
This is the result of running the Coccinelle script from
scripts/coccinelle/api/kmalloc_objs.cocci. The script is designed to
avoid scalar types (which need careful case-by-case checking), and
instead replace kmalloc-family calls that allocate struct or union
object instances:
Single allocations: kmalloc(sizeof(TYPE), ...)
are replaced with: kmalloc_obj(TYPE, ...)
Array allocations: kmalloc_array(COUNT, sizeof(TYPE), ...)
are replaced with: kmalloc_objs(TYPE, COUNT, ...)
Flex array allocations: kmalloc(struct_size(PTR, FAM, COUNT), ...)
are replaced with: kmalloc_flex(*PTR, FAM, COUNT, ...)
(where TYPE may also be *VAR)
The resulting allocations no longer return "void *", instead returning
"TYPE *".
Signed-off-by: Kees Cook <kees@kernel.org>
|
|
Introduce set_dte_nested() to program guest translation settings in
the host DTE when attaches the nested domain to a device.
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
|
|
invalidation
Each nested domain is assigned guest domain ID (gDomID), which guest OS
programs into guest Device Table Entry (gDTE). For each gDomID, the driver
assigns a corresponding host domain ID (hDomID), which will be programmed
into the host Device Table Entry (hDTE).
The hDomID is allocated during amd_iommu_alloc_domain_nested(),
and free during nested_domain_free(). The gDomID-to-hDomID mapping info
(struct guest_domain_mapping_info) is stored in a per-viommu xarray
(struct amd_iommu_viommu.gdomid_array), which is indexed by gDomID.
Note also that parent domain can be shared among struct iommufd_viommu.
Therefore, when hypervisor invalidates the nest parent domain, the AMD
IOMMU command INVALIDATE_IOMMU_PAGES must be issued for each hDomID in
the gdomid_array. This is handled by the iommu_flush_pages_v1_hdom_ids(),
where it iterates through struct protection_domain.viommu_list.
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
|
|
The nested domain is allocated with IOMMU_DOMAIN_NESTED type to store
stage-1 translation (i.e. GVA->GPA). This includes the GCR3 root pointer
table along with guest page tables. The struct iommu_hwpt_amd_guest
contains this information, and is passed from user-space as a parameter
of the struct iommu_ops.domain_alloc_nested().
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
|
