On 06/09/18 11:05, Will Deacon wrote:
Hi Robin,
On Thu, Aug 23, 2018 at 01:14:59PM +0100, Robin Murphy wrote:
In removing the pagetable-wide lock, we gained the possibility of the
vanishingly unlikely case where we have a race between two concurrent
unmappers splitting the same block entry. The logic to handle this is
fairly straightforward - whoever loses the race frees their partial
next-level table and instead dereferences the winner's newly-installed
entry in order to fall back to a regular unmap, which intentionally
echoes the pre-existing case of recursively splitting a 1GB block down
to 4KB pages by installing a full table of 2MB blocks first.
Unfortunately, the chump who implemented that logic failed to update the
condition check for that fallback, meaning that if said race occurs at
the last level (where the loser's unmap_idx is valid) then the unmap
won't actually happen. Fix that to properly account for both the race
and recursive cases.
Fixes: 2c3d273eabe8 ("iommu/io-pgtable-arm: Support lockless operation")
Signed-off-by: Robin Murphy <robin.mur...@arm.com>
---
drivers/iommu/io-pgtable-arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Well spotted! Did you just find this by inspection?
Indeed - I just kept seeing this bit in the context of the other patches
and thinking "hang on, how do we actually use that new tablep after the
deref?".
IIRC I actually implemented the new logic for v7s first, so clearly made
the blunder in adding the recursive case when porting the change to LPAE.
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 010a254305dd..93b4833cef73 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -575,7 +575,7 @@ static size_t arm_lpae_split_blk_unmap(struct
arm_lpae_io_pgtable *data,
tablep = iopte_deref(pte, data);
}
- if (unmap_idx < 0)
+ if (unmap_idx < 0 || pte != blk_pte)
return __arm_lpae_unmap(data, iova, size, lvl, tablep);
Can we tidy up the control flow a bit here to avoid re-checking the status
of the cmpxchg? See below.
Sure, I just made the minimal change for correctness so as not to
overcomplicate matters, but I'm pretty sure that diff looks correct.
Feel free to squash it in if you're planning on taking this (yes it's
nominally a fix, but it's absolutely non-urgent on account of nobody
will ever realistically hit this).
Robin.
Will
--->8
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 88641b4560bc..2f79efd16a05 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -574,13 +574,12 @@ static size_t arm_lpae_split_blk_unmap(struct
arm_lpae_io_pgtable *data,
return 0;
tablep = iopte_deref(pte, data);
+ } else if (unmap_idx >= 0) {
+ io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
+ return size;
}
- if (unmap_idx < 0)
- return __arm_lpae_unmap(data, iova, size, lvl, tablep);
-
- io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
- return size;
+ return __arm_lpae_unmap(data, iova, size, lvl, tablep);
}
static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
