[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240719181725.1446021-1-jacob.jun.pan@linux.intel.com>
Date: Fri, 19 Jul 2024 11:17:25 -0700
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: iommu@...ts.linux.dev,
LKML <linux-kernel@...r.kernel.org>,
"Lu Baolu" <baolu.lu@...ux.intel.com>
Cc: Yi Liu <yi.l.liu@...el.com>,
"Tian, Kevin" <kevin.tian@...el.com>,
tina.zhang@...el.com,
Sanjay K Kumar <sanjay.k.kumar@...el.com>,
Jacob Pan <jacob.jun.pan@...ux.intel.com>
Subject: [PATCH] iommu/vt-d: Fix potential soft lockup due to reclaim
From: Sanjay K Kumar <sanjay.k.kumar@...el.com>
If qi_submit_sync() is invoked with 0 invalidation descriptors (for
instance, for DMA draining purposes), we can run into a bug where a
submitting thread fails to detect the completion of invalidation_wait.
Subsequently, this led to a soft lockup.
Suppose thread T1 invokes qi_submit_sync() with non-zero descriptors, while
concurrently, thread T2 calls qi_submit_sync() with zero descriptors. Both
threads then enter a while loop, waiting for their respective descriptors
to complete. T1 detects its completion (i.e., T1's invalidation_wait status
changes to QI_DONE by HW) and proceeds to call reclaim_free_desc() to
reclaim all descriptors, potentially including adjacent ones of other
threads that are also marked as QI_DONE.
During this time, while T2 is waiting to acquire the qi->q_lock, the IOMMU
hardware may complete the invalidation for T2, setting its status to
QI_DONE. However, if T1's execution of reclaim_free_desc() frees T2's
invalidation_wait descriptor and changes its status to QI_FREE, T2 will
not observe the QI_DONE status for its invalidation_wait and will
indefinitely remain stuck.
This soft lockup does not occur when only non-zero descriptors are
submitted.In such cases, invalidation descriptors are interspersed among
wait descriptors with the status QI_IN_USE, acting as barriers. These
barriers prevent the reclaim code from mistakenly freeing descriptors
belonging to other submitters.
Considered the following example timeline:
T1 T2
========================================
ID1
WD1
while(WD1!=QI_DONE)
unlock
lock
WD1=QI_DONE* WD2
while(WD2!=QI_DONE)
unlock
lock
WD1==QI_DONE?
ID1=QI_DONE WD2=DONE*
reclaim()
ID1=FREE
WD1=FREE
WD2=FREE
unlock
soft lockup! T2 never sees QI_DONE in WD2
Where:
ID = invalidation descriptor
WD = wait descriptor
* Written by hardware
The root of the problem is that the descriptor status QI_DONE flag is used
for two conflicting purposes:
1. signal a descriptor is ready for reclaim (to be freed)
2. signal by the hardware that a wait descriptor is complete
The solution (in this patch) is state separation by introducing a new flag
for the descriptors called QI_TO_BE_FREED.
Once a thread's invalidation descriptors are complete, their status would
be set to QI_TO_BE_FREED. The reclaim_free_desc() function would then only
free descriptors marked as QI_TO_BE_FREED instead of those marked as
QI_DONE. This change ensures that T2 (from the previous example) will
correctly observe the completion of its invalidation_wait (marked as
QI_DONE).
Currently, there is no impact by this bug on the existing users because no
callers are submitting invalidations with 0 descriptors.
Signed-off-by: Sanjay K Kumar <sanjay.k.kumar@...el.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
---
drivers/iommu/intel/dmar.c | 13 +++++++++----
drivers/iommu/intel/iommu.h | 3 ++-
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 304e84949ca7..00e0f5f801c5 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1204,8 +1204,7 @@ static void free_iommu(struct intel_iommu *iommu)
*/
static inline void reclaim_free_desc(struct q_inval *qi)
{
- while (qi->desc_status[qi->free_tail] == QI_DONE ||
- qi->desc_status[qi->free_tail] == QI_ABORT) {
+ while (qi->desc_status[qi->free_tail] == QI_TO_BE_FREED) {
qi->desc_status[qi->free_tail] = QI_FREE;
qi->free_tail = (qi->free_tail + 1) % QI_LENGTH;
qi->free_cnt++;
@@ -1463,8 +1462,14 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
raw_spin_lock(&qi->q_lock);
}
- for (i = 0; i < count; i++)
- qi->desc_status[(index + i) % QI_LENGTH] = QI_DONE;
+ /*
+ * The reclaim code can free descriptors from multiple submissions
+ * starting from the tail of the queue. When count == 0, the
+ * status of the standalone wait descriptor at the tail of the queue
+ * must be set to QI_TO_BE_FREED to allow the reclaim code to proceed.
+ */
+ for (i = 0; i <= count; i++)
+ qi->desc_status[(index + i) % QI_LENGTH] = QI_TO_BE_FREED;
reclaim_free_desc(qi);
raw_spin_unlock_irqrestore(&qi->q_lock, flags);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index eaf015b4353b..1ab39f9145f2 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -382,7 +382,8 @@ enum {
QI_FREE,
QI_IN_USE,
QI_DONE,
- QI_ABORT
+ QI_ABORT,
+ QI_TO_BE_FREED
};
#define QI_CC_TYPE 0x1
--
2.25.1
Powered by blists - more mailing lists