Will Deacon
2021-Oct-04 11:38 UTC
[PATCH 5/5] iommu/iova: Avoid double-negatives in magazine helpers
On Fri, Sep 24, 2021 at 06:01:57PM +0800, John Garry wrote:> A similar crash to the following could be observed if initial CPU rcache > magazine allocations fail in init_iova_rcaches(): > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 > Mem abort info: > > free_iova_fast+0xfc/0x280 > iommu_dma_free_iova+0x64/0x70 > __iommu_dma_unmap+0x9c/0xf8 > iommu_dma_unmap_sg+0xa8/0xc8 > dma_unmap_sg_attrs+0x28/0x50 > cq_thread_v3_hw+0x2dc/0x528 > irq_thread_fn+0x2c/0xa0 > irq_thread+0x130/0x1e0 > kthread+0x154/0x158 > ret_from_fork+0x10/0x34 > > The issue is that expression !iova_magazine_full(NULL) evaluates true; this > falls over in __iova_rcache_insert() when we attempt to cache a mag and > cpu_rcache->loaded == NULL: > > if (!iova_magazine_full(cpu_rcache->loaded)) { > can_insert = true; > ... > > if (can_insert) > iova_magazine_push(cpu_rcache->loaded, iova_pfn); > > As above, can_insert is evaluated true, which it shouldn't be, and we try > to insert pfns in a NULL mag, which is not safe. > > To avoid this, stop using double-negatives, like !iova_magazine_full() and > !iova_magazine_empty(), and use positive tests, like > iova_magazine_has_space() and iova_magazine_has_pfns(), respectively; these > can safely deal with cpu_rcache->{loaded, prev} = NULL.I don't understand why you're saying that things like !iova_magazine_empty() are double-negatives. What about e.g. !list_empty() elsewhre in the kernel? The crux of the fix seems to be:> @@ -783,8 +787,9 @@ static bool __iova_rcache_insert(struct iova_caching_domain *rcached, > if (new_mag) { > spin_lock(&rcache->lock); > if (rcache->depot_size < MAX_GLOBAL_MAGS) { > - rcache->depot[rcache->depot_size++] > - cpu_rcache->loaded; > + if (cpu_rcache->loaded) > + rcache->depot[rcache->depot_size++] > + cpu_rcache->loaded;Which could be independent of the renaming? Will