On Tue, Oct 25, 2022 at 03:12:17PM -0300, Jason Gunthorpe wrote:
> +/**
> + * iopt_pages_unfill_xarray() - Update the xarry after removing an access
> + * @pages: The pages to act on
> + * @start_index: Starting PFN index
> + * @last_index: Last PFN index
> + *
> + * Called when an iopt_pages_access is removed, removes pages from the itree.
> + * The access should already be removed from the access_itree.
> + */
> +void iopt_pages_unfill_xarray(struct iopt_pages *pages,
> + unsigned long start_index,
> + unsigned long last_index)
> +{
> + struct interval_tree_double_span_iter span;
> + u64 backup[BATCH_BACKUP_SIZE];
> + struct pfn_batch batch;
> + bool batch_inited = false;
> +
> + lockdep_assert_held(&pages->mutex);
> +
> + interval_tree_for_each_double_span(&span, &pages->access_itree,
> + &pages->domains_itree, start_index,
> + last_index) {
> + if (!span.is_used) {
> + if (!batch_inited) {
> + batch_init_backup(&batch,
> + last_index - start_index + 1,
> + backup, sizeof(backup));
> + batch_inited = true;
> + }
> + batch_from_xarray_clear(&batch, &pages->pinned_pfns,
> + span.start_hole,
> + span.last_hole);
> + batch_unpin(&batch, pages, 0, batch.total_pfns);
> + batch_clear(&batch);
Syzkaller, and then the nth test suite found this - the required loop
around the 'batch_from' is missing. The normal test suite should have
found this, but it turns out it is using huge pages, and not enough of
them to overfill even the limited batch.
Testing also showed that batch overflow accidently zerod an xarray
entry that was not actually stored. Thus this:
diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index da95b28b41772c..3640f83f57069b 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -479,10 +479,11 @@ static void batch_from_xarray_clear(struct pfn_batch *batch, struct xarray *xa,
entry = xas_next(&xas);
if (xas_retry(&xas, entry))
continue;
- xas_store(&xas, NULL);
WARN_ON(!xa_is_value(entry));
- if (!batch_add_pfn(batch, xa_to_value(entry)) ||
- start_index == last_index)
+ if (!batch_add_pfn(batch, xa_to_value(entry)))
+ break;
+ xas_store(&xas, NULL);
+ if (start_index == last_index)
break;
start_index++;
}
@@ -1407,6 +1408,20 @@ void iopt_area_unfill_domains(struct iopt_area *area, struct iopt_pages *pages)
mutex_unlock(&pages->mutex);
}
+static void iopt_pages_unpin_xarray(struct pfn_batch *batch,
+ struct iopt_pages *pages,
+ unsigned long start_index,
+ unsigned long end_index)
+{
+ while (start_index <= end_index) {
+ batch_from_xarray_clear(batch, &pages->pinned_pfns, start_index,
+ end_index);
+ batch_unpin(batch, pages, 0, batch->total_pfns);
+ start_index += batch->total_pfns;
+ batch_clear(batch);
+ }
+}
+
/**
* iopt_pages_unfill_xarray() - Update the xarry after removing an access
* @pages: The pages to act on
@@ -1437,11 +1452,8 @@ void iopt_pages_unfill_xarray(struct iopt_pages *pages,
backup, sizeof(backup));
batch_inited = true;
}
- batch_from_xarray_clear(&batch, &pages->pinned_pfns,
- span.start_hole,
+ iopt_pages_unpin_xarray(&batch, pages, span.start_hole,
span.last_hole);
- batch_unpin(&batch, pages, 0, batch.total_pfns);
- batch_clear(&batch);
} else if (span.is_used == 2) {
/* Covered by a domain */
clear_xarray(&pages->pinned_pfns, span.start_used,
