[PATCH] vfio/type1: fixed rollback in vfio_dma_bitmap_alloc_all()

[PATCH] vfio/type1: fixed rollback in vfio_dma_bitmap_alloc_all()

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The vfio dma bitmap of p should be freed, not n

Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0ac5607..ba5d91e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -293,7 +293,7 @@ static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
 			struct rb_node *p;
 
 			for (p = rb_prev(n); p; p = rb_prev(p)) {
-				struct vfio_dma *dma = rb_entry(n,
+				struct vfio_dma *dma = rb_entry(p,
 							struct vfio_dma, node);
 
 				vfio_dma_bitmap_free(dma);
-- 
2.9.4

Re: [PATCH] vfio/type1: fixed rollback in vfio_dma_bitmap_alloc_all()

[Alex Williamson]

On Wed, 21 May 2025 11:46:47 +0800
lirongqing <lirongqing@baidu.com> wrote:

> From: Li RongQing <lirongqing@baidu.com>
> 
> The vfio dma bitmap of p should be freed, not n
> 
> Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages tracking")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 0ac5607..ba5d91e 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -293,7 +293,7 @@ static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
>  			struct rb_node *p;
>  
>  			for (p = rb_prev(n); p; p = rb_prev(p)) {
> -				struct vfio_dma *dma = rb_entry(n,
> +				struct vfio_dma *dma = rb_entry(p,
>  							struct vfio_dma, node);
>  
>  				vfio_dma_bitmap_free(dma);

Good find.  The change looks correct to me.  For the benefit of stable
backports, let's venture towards being overly verbose in the subject and
commit log.  My suggestion would be:

    vfio/type1: Fix error unwind in migration dirty bitmap allocation

    When setting up dirty page tracking at the vfio IOMMU backend for
    device migration, if an error is encountered allocating a tracking
    bitmap, the unwind loop fails to free previously allocated tracking
    bitmaps.  This occurs because the wrong loop index is used to
    generate the tracking object.  This results in unintended memory
    usage for the life of the current DMA mappings where bitmaps were
    successfully allocated.

    Use the correct loop index to derive the tracking object for
    freeing during unwind.

This gives us some context relative to when we might encounter this
issue (pretty rare) and the scope of the issue (bound to the lifetime
of the vfio_dma object).  If you approve I can incorporate this to v1
or feel free to send a v2 with these updates.  Thanks,

Alex

答复: [????] Re: [PATCH] vfio/type1: fixed rollback in vfio_dma_bitmap_alloc_all()

[Li,Rongqing]

>     vfio/type1: Fix error unwind in migration dirty bitmap allocation
> 
>     When setting up dirty page tracking at the vfio IOMMU backend for
>     device migration, if an error is encountered allocating a tracking
>     bitmap, the unwind loop fails to free previously allocated tracking
>     bitmaps.  This occurs because the wrong loop index is used to
>     generate the tracking object.  This results in unintended memory
>     usage for the life of the current DMA mappings where bitmaps were
>     successfully allocated.
> 
>     Use the correct loop index to derive the tracking object for
>     freeing during unwind.
> 

Your changelog is extremely detailed and highly accurate.

Please directly incorporate this patch with your changelog

Thanks

-Li

Re: [????] Re: [PATCH] vfio/type1: fixed rollback in vfio_dma_bitmap_alloc_all()

[Alex Williamson]

On Thu, 22 May 2025 01:53:48 +0000
"Li,Rongqing" <lirongqing@baidu.com> wrote:

> >     vfio/type1: Fix error unwind in migration dirty bitmap allocation
> > 
> >     When setting up dirty page tracking at the vfio IOMMU backend for
> >     device migration, if an error is encountered allocating a tracking
> >     bitmap, the unwind loop fails to free previously allocated tracking
> >     bitmaps.  This occurs because the wrong loop index is used to
> >     generate the tracking object.  This results in unintended memory
> >     usage for the life of the current DMA mappings where bitmaps were
> >     successfully allocated.
> > 
> >     Use the correct loop index to derive the tracking object for
> >     freeing during unwind.
> >   
> 
> Your changelog is extremely detailed and highly accurate.
> 
> Please directly incorporate this patch with your changelog

Applied to vfio next branch for v6.16 with updated changelog.  Thanks,

Alex