[PATCH v1 0/2] vfio/iova_bitmap: bug fixes

[PATCH v1 0/2] vfio/iova_bitmap: bug fixes

[Joao Martins]

Hey,

This small series addresses one small improvement and one bug fix:

1) Avoid obscurity into how I use slab helpers yet I was
never include it directly. And it wasn't detected thanks to a unrelated
commit in v5.18 that lead linux/mm.h to indirectly include slab.h.

2) A bug reported by Avihai reported during migration testing where
we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead
to miss dirty marked correctly on some IOVAs.

Comments appreciated

Thanks,
	Joao

Joao Martins (2):
  vfio/iova_bitmap: Explicitly include linux/slab.h
  vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps

 drivers/vfio/iova_bitmap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.17.2

[PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h

[Joao Martins]

kzalloc/kzfree are used so include `slab.h`. While it happens to work
without it, due to commit 8b9f3ac5b01d ("fs: introduce alloc_inode_sb() to
allocate filesystems specific inode") which indirectly includes via:

. ./include/linux/mm.h
.. ./include/linux/huge_mm.h
... ./include/linux/fs.h
.... ./include/linux/slab.h

Make it explicit should any of its indirect dependencies be dropped/changed
for entirely different reasons as it was the cause prior to commit above
recently (i.e. <= v5.18).

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/vfio/iova_bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c
index c20ffce1a0fd..389f36cae355 100644
--- a/drivers/vfio/iova_bitmap.c
+++ b/drivers/vfio/iova_bitmap.c
@@ -5,6 +5,7 @@
  */
 #include <linux/iova_bitmap.h>
 #include <linux/mm.h>
+#include <linux/slab.h>
 #include <linux/highmem.h>
 
 #define BITS_PER_PAGE (PAGE_SIZE * BITS_PER_BYTE)
-- 
2.17.2

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

[Joao Martins]

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?
---
 drivers/vfio/iova_bitmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c
index 389f36cae355..40463c51da31 100644
--- a/drivers/vfio/iova_bitmap.c
+++ b/drivers/vfio/iova_bitmap.c
@@ -296,11 +296,15 @@ void iova_bitmap_free(struct iova_bitmap *bitmap)
  */
 static unsigned long iova_bitmap_mapped_remaining(struct iova_bitmap *bitmap)
 {
-	unsigned long remaining;
+	unsigned long remaining, bytes;
+
+	/* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */
+	bytes = !bitmap->mapped.pgoff ? bitmap->mapped.npages << PAGE_SHIFT :
+					PAGE_SIZE - bitmap->mapped.pgoff;
 
 	remaining = bitmap->mapped_total_index - bitmap->mapped_base_index;
 	remaining = min_t(unsigned long, remaining,
-	      (bitmap->mapped.npages << PAGE_SHIFT) / sizeof(*bitmap->bitmap));
+			  bytes / sizeof(*bitmap->bitmap));
 
 	return remaining;
 }
-- 
2.17.2

Re: [PATCH v1 0/2] vfio/iova_bitmap: bug fixes

[Joao Martins]

On 25/10/2022 20:31, Joao Martins wrote:
> Hey,
> 
> This small series addresses one small improvement and one bug fix:
> 
> 1) Avoid obscurity into how I use slab helpers yet I was
> never include it directly. And it wasn't detected thanks to a unrelated
> commit in v5.18 that lead linux/mm.h to indirectly include slab.h.
> 
> 2) A bug reported by Avihai reported during migration testing where
> we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead
> to miss dirty marked correctly on some IOVAs.
> 
> Comments appreciated
> 
Ping?

> Thanks,
> 	Joao
> 
> Joao Martins (2):
>   vfio/iova_bitmap: Explicitly include linux/slab.h
>   vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps
> 
>  drivers/vfio/iova_bitmap.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

Re: [PATCH v1 0/2] vfio/iova_bitmap: bug fixes

[Alex Williamson]

On Tue, 25 Oct 2022 20:31:12 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> Hey,
> 
> This small series addresses one small improvement and one bug fix:
> 
> 1) Avoid obscurity into how I use slab helpers yet I was
> never include it directly. And it wasn't detected thanks to a unrelated
> commit in v5.18 that lead linux/mm.h to indirectly include slab.h.
> 
> 2) A bug reported by Avihai reported during migration testing where
> we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead
> to miss dirty marked correctly on some IOVAs.
> 
> Comments appreciated
> 
> Thanks,
> 	Joao
> 
> Joao Martins (2):
>   vfio/iova_bitmap: Explicitly include linux/slab.h
>   vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps
> 
>  drivers/vfio/iova_bitmap.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

Applied to vfio next branch for v6.2.  Thanks,

Alex

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

[Jason Gunthorpe]

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

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

[Joao Martins]

On 24/11/2022 18:20, Jason Gunthorpe wrote:
> 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.
> 
So in iommufd counterpart I have already tests which exercise this. But not as
extensive.

This testsuite is a bit more focused on all the functions in iova_bitmap and
making sure I iterate the ranges right with small and huge bitmaps (up to 4T of
IOVA space bitmaps), and testing that all the bits are set as expected, etc.
I'll see if I merge both of these, there's probably no need for two separate
units in the testsuite. The only test infra need for standalone iova-bitmap
tests was the fact that I use GUP, and so I couldn't but this enterily in userspace.

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

Ack

> 
>> +	/* 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
> 
This would return a higher than necessary number of indexes.

But in fact the logic currently is correct and if we instead fix
iova_bitmap_set() this new capping I was doing isn't needed at all.

> And fix the bug in iova_bitmap_set:
> 

This is right.

I'm not handling cross page boundaries right 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);
> 
Let me test this and respin.

Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new
version, or whether I should explicitly revert the already commited one and
apply a new version. I'll do a v2 of this patch and wait for Alex direction on
where to go on the v2 patch in case I need to go via the latter.

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

[Jason Gunthorpe]

On Fri, Nov 25, 2022 at 10:37:39AM +0000, Joao Martins wrote:

> > 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.
>
> So in iommufd counterpart I have already tests which exercise this. But not as
> extensive.

We are getting to the point where we should start posting the iommufd
dirty tracking stuff. Do you have time to work on it for the next
cycle? Meaning get it largely sorted out in the next 3 weeks for review?

> > 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);

min(BITS_PER_PAGE - (cur_bit % BITS_PER_PAGE), ...)

> Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new
> version, 

It isn't, you should just post a new patch on top of Alex's current
tree "rework iova_bitmap_et to handle all page crossings" and along
the way revert the first bit

Jason

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

[Joao Martins]

On 25/11/2022 12:42, Jason Gunthorpe wrote:
> On Fri, Nov 25, 2022 at 10:37:39AM +0000, Joao Martins wrote:
> 
>>> 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.
>>
>> So in iommufd counterpart I have already tests which exercise this. But not as
>> extensive.
> 
> We are getting to the point where we should start posting the iommufd
> dirty tracking stuff. Do you have time to work on it for the next
> cycle? Meaning get it largely sorted out in the next 3 weeks for review?
>

I'll post it for the next cycle -- It has been a bit on crazy on my end this
past month or so.

>>> 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);
> 
> min(BITS_PER_PAGE - (cur_bit % BITS_PER_PAGE), ...)
> 

I actually had this already in my changeset :) as the earlier snip wasn't
passing my tests. Plus I need to account for less indexes with pgoff, contrary
to what I said earlier in the remaining() function calculation.

>> Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new
>> version, 
> 
> It isn't, you should just post a new patch on top of Alex's current
> tree "rework iova_bitmap_et to handle all page crossings" and along
> the way revert the first bit

OK -- makes sense. The other fix wasn't incorrect either (as we need to account
for pgoff on the same function), this one though fixes the real issue of
iova_bitmap_set().

Also, I'll add your Signed-off-by+Co-developed-by -- let me know otherwise if I
should not.

	Joao