Re: [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 25, 2022 at 08:31:14PM +0100, Joao Martins wrote:
> iova_bitmap_set() doesn't consider the end of the page boundary when the
> first bitmap page offset isn't zero, and wrongly changes the consecutive
> page right after. Consequently this leads to missing dirty pages from
> reported by the device as seen from the VMM.
> 
> The current logic iterates over a given number of base pages and clamps it
> to the remaining indexes to iterate in the last page.  Instead of having to
> consider extra pages to pin (e.g. first and extra pages), just handle the
> first page as its own range and let the rest of the bitmap be handled as if
> it was base page aligned.
> 
> This is done by changing iova_bitmap_mapped_remaining() to return PAGE_SIZE
> - pgoff (on the first bitmap page), and leads to pgoff being set to 0 on
> following iterations.
> 
> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support")
> Reported-by: Avihai Horon <avihaih@nvidia.com>
> Tested-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> I have a small test suite that I have been using for functional and some
> performance tests; I try to cover all the edge cases. Though I happened to
> miss having a test case (which is also fixed) ... leading to this bug.
> I wonder if this test suite is something of interest to have in
> tree?

Yes, when we move this to iommufd the test suite should be included,
either as integrated using the mock domain and the selftests or
otherwise.

This is really a problem in iova_bitmap_set(), it isn't doing the math
right.


> +	/* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */

Why isn't it just 

 bitmap->mapped.npages * PAGE_SIZE - bitmap->mapped.pgoff

And fix the bug in iova_bitmap_set:

void iova_bitmap_set(struct iova_bitmap *bitmap,
		     unsigned long iova, size_t length)
{
	struct iova_bitmap_map *mapped = &bitmap->mapped;
	unsigned cur_bit =
		((iova - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8;
	unsigned long last_bit =
		(((iova + length - 1) - mapped->iova) >> mapped->pgshift) +
		mapped->pgoff * 8;

	do {
		unsigned int page_idx = cur_bit / BITS_PER_PAGE;
		unsigned int nbits =
			min(BITS_PER_PAGE - cur_bit, last_bit - cur_bit + 1);
		void *kaddr;

		kaddr = kmap_local_page(mapped->pages[page_idx]);
		bitmap_set(kaddr, cur_bit % BITS_PER_PAGE, nbits);
		kunmap_local(kaddr);
		cur_bit += nbits;
	} while (cur_bit <= last_bit);
}
EXPORT_SYMBOL_GPL(iova_bitmap_set);

Jason