| Message ID | 20221125172956.19975-1-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/iova_bitmap: refactor iova_bitmap_set() to better handle page boundaries | expand |
On Fri, Nov 25, 2022 at 05:29:56PM +0000, Joao Martins wrote: > Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > had fixed the unaligned bitmaps by capping the remaining iterable set at > the start of the bitmap. Although, that mistakenly worked around > iova_bitmap_set() incorrectly setting bits across page boundary. > > Fix this by reworking the loop inside iova_bitmap_set() to iterate over a > range of bits to set (cur_bit .. last_bit) which may span different pinned > pages, thus updating @page_idx and @offset as it sets the bits. The > previous cap to the first page is now adjusted to be always accounted > rather than when there's only a non-zero pgoff. > > While at it, make @page_idx , @offset and @nbits to be unsigned int given > that it won't be more than 512 and 4096 respectively (even a bigger > PAGE_SIZE or a smaller struct page size won't make this bigger than the > above 32-bit max). Also, delete the stale kdoc on Return type. > > Cc: Avihai Horon <avihaih@nvidia.com> > Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > It passes my tests but to be extra sure: Avihai could you take this > patch a spin in your rig/tests as well? Thanks! > --- > drivers/vfio/iova_bitmap.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Fri, 25 Nov 2022 17:29:56 +0000 Joao Martins <joao.m.martins@oracle.com> wrote: > Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > had fixed the unaligned bitmaps by capping the remaining iterable set at > the start of the bitmap. Although, that mistakenly worked around > iova_bitmap_set() incorrectly setting bits across page boundary. > > Fix this by reworking the loop inside iova_bitmap_set() to iterate over a > range of bits to set (cur_bit .. last_bit) which may span different pinned > pages, thus updating @page_idx and @offset as it sets the bits. The > previous cap to the first page is now adjusted to be always accounted > rather than when there's only a non-zero pgoff. > > While at it, make @page_idx , @offset and @nbits to be unsigned int given > that it won't be more than 512 and 4096 respectively (even a bigger > PAGE_SIZE or a smaller struct page size won't make this bigger than the > above 32-bit max). Also, delete the stale kdoc on Return type. > > Cc: Avihai Horon <avihaih@nvidia.com> > Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Should this have: Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") ? Thanks, Alex
On 28/11/2022 19:12, Alex Williamson wrote: > On Fri, 25 Nov 2022 17:29:56 +0000 > Joao Martins <joao.m.martins@oracle.com> wrote: > >> Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") >> had fixed the unaligned bitmaps by capping the remaining iterable set at >> the start of the bitmap. Although, that mistakenly worked around >> iova_bitmap_set() incorrectly setting bits across page boundary. >> >> Fix this by reworking the loop inside iova_bitmap_set() to iterate over a >> range of bits to set (cur_bit .. last_bit) which may span different pinned >> pages, thus updating @page_idx and @offset as it sets the bits. The >> previous cap to the first page is now adjusted to be always accounted >> rather than when there's only a non-zero pgoff. >> >> While at it, make @page_idx , @offset and @nbits to be unsigned int given >> that it won't be more than 512 and 4096 respectively (even a bigger >> PAGE_SIZE or a smaller struct page size won't make this bigger than the >> above 32-bit max). Also, delete the stale kdoc on Return type. >> >> Cc: Avihai Horon <avihaih@nvidia.com> >> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > > Should this have: > > Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > > ? I was at two minds with the Fixes tag. The commit you referenced above is still a fix (or workaround), this patch is a better fix that superseeds as opposed to fixing a bug that commit f38044e5ef58 introduced. So perhaps the right one ought to be: Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") Joao
On Mon, 28 Nov 2022 19:22:10 +0000 Joao Martins <joao.m.martins@oracle.com> wrote: > On 28/11/2022 19:12, Alex Williamson wrote: > > On Fri, 25 Nov 2022 17:29:56 +0000 > > Joao Martins <joao.m.martins@oracle.com> wrote: > > > >> Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > >> had fixed the unaligned bitmaps by capping the remaining iterable set at > >> the start of the bitmap. Although, that mistakenly worked around ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> iova_bitmap_set() incorrectly setting bits across page boundary. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> > >> Fix this by reworking the loop inside iova_bitmap_set() to iterate over a ^^^^^^^^^^^... > >> range of bits to set (cur_bit .. last_bit) which may span different pinned > >> pages, thus updating @page_idx and @offset as it sets the bits. The > >> previous cap to the first page is now adjusted to be always accounted > >> rather than when there's only a non-zero pgoff. > >> > >> While at it, make @page_idx , @offset and @nbits to be unsigned int given > >> that it won't be more than 512 and 4096 respectively (even a bigger > >> PAGE_SIZE or a smaller struct page size won't make this bigger than the > >> above 32-bit max). Also, delete the stale kdoc on Return type. > >> > >> Cc: Avihai Horon <avihaih@nvidia.com> > >> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> > >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > > > > Should this have: > > > > Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > > > > ? > > I was at two minds with the Fixes tag. > > The commit you referenced above is still a fix (or workaround), this patch is a > better fix that superseeds as opposed to fixing a bug that commit f38044e5ef58 > introduced. So perhaps the right one ought to be: > > Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") The above highlighted text certainly suggests that there's a fix to f38044e5ef58 here. We might still be iterating on a problem that was originally introduced in 58ccf0190d19, but this more directly addresses the version of that problem that exists after f38044e5ef58. I think it's more helpful for backporters to see this progression rather than two patches claiming to fix the same commit with one depending on another. If you'd rather that stable have a different backport that short circuits the interim fix in f38044e5ef58, that could be posted separately, but imo it's better to follow the mainline progression. Thanks, Alex
On 28/11/2022 22:48, Alex Williamson wrote: > On Mon, 28 Nov 2022 19:22:10 +0000 > Joao Martins <joao.m.martins@oracle.com> wrote: > >> On 28/11/2022 19:12, Alex Williamson wrote: >>> On Fri, 25 Nov 2022 17:29:56 +0000 >>> Joao Martins <joao.m.martins@oracle.com> wrote: >>> >>>> Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") >>>> had fixed the unaligned bitmaps by capping the remaining iterable set at >>>> the start of the bitmap. Although, that mistakenly worked around > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> iova_bitmap_set() incorrectly setting bits across page boundary. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> >>>> Fix this by reworking the loop inside iova_bitmap_set() to iterate over a > ^^^^^^^^^^^... >>>> range of bits to set (cur_bit .. last_bit) which may span different pinned >>>> pages, thus updating @page_idx and @offset as it sets the bits. The >>>> previous cap to the first page is now adjusted to be always accounted >>>> rather than when there's only a non-zero pgoff. >>>> >>>> While at it, make @page_idx , @offset and @nbits to be unsigned int given >>>> that it won't be more than 512 and 4096 respectively (even a bigger >>>> PAGE_SIZE or a smaller struct page size won't make this bigger than the >>>> above 32-bit max). Also, delete the stale kdoc on Return type. >>>> >>>> Cc: Avihai Horon <avihaih@nvidia.com> >>>> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> >>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>> >>> Should this have: >>> >>> Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") >>> >>> ? >> >> I was at two minds with the Fixes tag. >> >> The commit you referenced above is still a fix (or workaround), this patch is a >> better fix that superseeds as opposed to fixing a bug that commit f38044e5ef58 >> introduced. So perhaps the right one ought to be: >> >> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") > > The above highlighted text certainly suggests that there's a fix to > f38044e5ef58 here. We might still be iterating on a problem that was > originally introduced in 58ccf0190d19, but this more directly addresses > the version of that problem that exists after f38044e5ef58. I think > it's more helpful for backporters to see this progression rather than > two patches claiming to fix the same commit with one depending on > another. If you'd rather that stable have a different backport that > short circuits the interim fix in f38044e5ef58, that could be posted > separately, but imo it's better to follow the mainline progression. OK, thanks for the explanation -- lets then use f38044e5ef58 as the Fixes tag.
On 29/11/2022 11:28, Joao Martins wrote: > On 28/11/2022 22:48, Alex Williamson wrote: >> On Mon, 28 Nov 2022 19:22:10 +0000 >> Joao Martins <joao.m.martins@oracle.com> wrote: >> >>> On 28/11/2022 19:12, Alex Williamson wrote: >>>> On Fri, 25 Nov 2022 17:29:56 +0000 >>>> Joao Martins <joao.m.martins@oracle.com> wrote: >>>> >>>>> Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") >>>>> had fixed the unaligned bitmaps by capping the remaining iterable set at >>>>> the start of the bitmap. Although, that mistakenly worked around >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> iova_bitmap_set() incorrectly setting bits across page boundary. >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> >>>>> Fix this by reworking the loop inside iova_bitmap_set() to iterate over a >> ^^^^^^^^^^^... >>>>> range of bits to set (cur_bit .. last_bit) which may span different pinned >>>>> pages, thus updating @page_idx and @offset as it sets the bits. The >>>>> previous cap to the first page is now adjusted to be always accounted >>>>> rather than when there's only a non-zero pgoff. >>>>> >>>>> While at it, make @page_idx , @offset and @nbits to be unsigned int given >>>>> that it won't be more than 512 and 4096 respectively (even a bigger >>>>> PAGE_SIZE or a smaller struct page size won't make this bigger than the >>>>> above 32-bit max). Also, delete the stale kdoc on Return type. >>>>> >>>>> Cc: Avihai Horon <avihaih@nvidia.com> >>>>> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> >>>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >>>> >>>> Should this have: >>>> >>>> Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") >>>> >>>> ? >>> >>> I was at two minds with the Fixes tag. >>> >>> The commit you referenced above is still a fix (or workaround), this patch is a >>> better fix that superseeds as opposed to fixing a bug that commit f38044e5ef58 >>> introduced. So perhaps the right one ought to be: >>> >>> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") >> >> The above highlighted text certainly suggests that there's a fix to >> f38044e5ef58 here. We might still be iterating on a problem that was >> originally introduced in 58ccf0190d19, but this more directly addresses >> the version of that problem that exists after f38044e5ef58. I think >> it's more helpful for backporters to see this progression rather than >> two patches claiming to fix the same commit with one depending on >> another. If you'd rather that stable have a different backport that >> short circuits the interim fix in f38044e5ef58, that could be posted >> separately, but imo it's better to follow the mainline progression. > > OK, thanks for the explanation -- lets then use f38044e5ef58 as the Fixes tag. Meanwhile I've sent v2 with the tags I got here, as I'm going to be out the rest of the week.
diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c index de6d6ea5c496..0848f920efb7 100644 --- a/drivers/vfio/iova_bitmap.c +++ b/drivers/vfio/iova_bitmap.c @@ -298,9 +298,7 @@ static unsigned long iova_bitmap_mapped_remaining(struct iova_bitmap *bitmap) { 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; + bytes = (bitmap->mapped.npages << PAGE_SHIFT) - bitmap->mapped.pgoff; remaining = bitmap->mapped_total_index - bitmap->mapped_base_index; remaining = min_t(unsigned long, remaining, @@ -399,29 +397,27 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, * Set the bits corresponding to the range [iova .. iova+length-1] in * the user bitmap. * - * Return: The number of bits set. */ void iova_bitmap_set(struct iova_bitmap *bitmap, unsigned long iova, size_t length) { struct iova_bitmap_map *mapped = &bitmap->mapped; - unsigned long offset = (iova - mapped->iova) >> mapped->pgshift; - unsigned long nbits = max_t(unsigned long, 1, length >> mapped->pgshift); - unsigned long page_idx = offset / BITS_PER_PAGE; - unsigned long page_offset = mapped->pgoff; - void *kaddr; - - offset = offset % BITS_PER_PAGE; + unsigned long cur_bit = ((iova - mapped->iova) >> + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; + unsigned long last_bit = (((iova + length - 1) - mapped->iova) >> + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; do { - unsigned long size = min(BITS_PER_PAGE - offset, nbits); + unsigned int page_idx = cur_bit / BITS_PER_PAGE; + unsigned int offset = cur_bit % BITS_PER_PAGE; + unsigned int nbits = min(BITS_PER_PAGE - offset, + last_bit - cur_bit + 1); + void *kaddr; kaddr = kmap_local_page(mapped->pages[page_idx]); - bitmap_set(kaddr + page_offset, offset, size); + bitmap_set(kaddr, offset, nbits); kunmap_local(kaddr); - page_offset = offset = 0; - nbits -= size; - page_idx++; - } while (nbits > 0); + cur_bit += nbits; + } while (cur_bit <= last_bit); } EXPORT_SYMBOL_GPL(iova_bitmap_set);
Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") had fixed the unaligned bitmaps by capping the remaining iterable set at the start of the bitmap. Although, that mistakenly worked around iova_bitmap_set() incorrectly setting bits across page boundary. Fix this by reworking the loop inside iova_bitmap_set() to iterate over a range of bits to set (cur_bit .. last_bit) which may span different pinned pages, thus updating @page_idx and @offset as it sets the bits. The previous cap to the first page is now adjusted to be always accounted rather than when there's only a non-zero pgoff. While at it, make @page_idx , @offset and @nbits to be unsigned int given that it won't be more than 512 and 4096 respectively (even a bigger PAGE_SIZE or a smaller struct page size won't make this bigger than the above 32-bit max). Also, delete the stale kdoc on Return type. Cc: Avihai Horon <avihaih@nvidia.com> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- It passes my tests but to be extra sure: Avihai could you take this patch a spin in your rig/tests as well? Thanks! --- drivers/vfio/iova_bitmap.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)