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@xxxxxxxxxx>
> Tested-by: Avihai Horon <avihaih@xxxxxxxxxx>
> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> ---
> 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
