[PATCH vfio 0/2] Fix several unwind issues in the mlx5/vfio driver

[PATCH vfio 0/2] Fix several unwind issues in the mlx5/vfio driver

[Yishai Hadas]

This series fixes several unwind issues in the mlx5/vfio driver.

Further details are provided in the commit logs.

Yishai

Yishai Hadas (2):
  vfio/mlx5: Fix an unwind issue in mlx5vf_add_migration_pages()
  vfio/mlx5: Fix unwind flows in mlx5vf_pci_save/resume_device_data()

 drivers/vfio/pci/mlx5/cmd.c  |  6 +++++-
 drivers/vfio/pci/mlx5/main.c | 35 +++++++++++++++++------------------
 2 files changed, 22 insertions(+), 19 deletions(-)

-- 
2.18.1

[PATCH vfio 1/2] vfio/mlx5: Fix an unwind issue in mlx5vf_add_migration_pages()

[Yishai Hadas]

Fix an unwind issue in mlx5vf_add_migration_pages().

If a set of pages is allocated but fails to be added to the SG table,
they need to be freed to prevent a memory leak.

Any pages successfully added to the SG table will be freed as part of
mlx5vf_free_data_buffer().

Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 drivers/vfio/pci/mlx5/cmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
index 41a4b0cf4297..7527e277c898 100644
--- a/drivers/vfio/pci/mlx5/cmd.c
+++ b/drivers/vfio/pci/mlx5/cmd.c
@@ -423,6 +423,7 @@ static int mlx5vf_add_migration_pages(struct mlx5_vhca_data_buffer *buf,
 	unsigned long filled;
 	unsigned int to_fill;
 	int ret;
+	int i;
 
 	to_fill = min_t(unsigned int, npages, PAGE_SIZE / sizeof(*page_list));
 	page_list = kvzalloc(to_fill * sizeof(*page_list), GFP_KERNEL_ACCOUNT);
@@ -443,7 +444,7 @@ static int mlx5vf_add_migration_pages(struct mlx5_vhca_data_buffer *buf,
 			GFP_KERNEL_ACCOUNT);
 
 		if (ret)
-			goto err;
+			goto err_append;
 		buf->allocated_length += filled * PAGE_SIZE;
 		/* clean input for another bulk allocation */
 		memset(page_list, 0, filled * sizeof(*page_list));
@@ -454,6 +455,9 @@ static int mlx5vf_add_migration_pages(struct mlx5_vhca_data_buffer *buf,
 	kvfree(page_list);
 	return 0;
 
+err_append:
+	for (i = filled - 1; i >= 0; i--)
+		__free_page(page_list[i]);
 err:
 	kvfree(page_list);
 	return ret;
-- 
2.18.1

Re: [PATCH vfio 1/2] vfio/mlx5: Fix an unwind issue in mlx5vf_add_migration_pages()

[Jason Gunthorpe]

On Thu, Nov 14, 2024 at 11:53:17AM +0200, Yishai Hadas wrote:
> Fix an unwind issue in mlx5vf_add_migration_pages().
> 
> If a set of pages is allocated but fails to be added to the SG table,
> they need to be freed to prevent a memory leak.
> 
> Any pages successfully added to the SG table will be freed as part of
> mlx5vf_free_data_buffer().
> 
> Fixes: 6fadb021266d ("vfio/mlx5: Implement vfio_pci driver for mlx5 devices")
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/mlx5/cmd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH vfio 2/2] vfio/mlx5: Fix unwind flows in mlx5vf_pci_save/resume_device_data()

[Yishai Hadas]

Fix unwind flows in mlx5vf_pci_save_device_data() and
mlx5vf_pci_resume_device_data() to avoid freeing the migf pointer at the
'end' label, as this will be handled by fput(migf->filp) through
mlx5vf_release_file().

To ensure mlx5vf_release_file() functions correctly, move the
initialization of migf fields (such as migf->lock) to occur before any
potential unwind flow, as these fields may be accessed within
mlx5vf_release_file().

Fixes: 9945a67ea4b3 ("vfio/mlx5: Refactor PD usage")
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 drivers/vfio/pci/mlx5/main.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
index 242c23eef452..8833e60d42f5 100644
--- a/drivers/vfio/pci/mlx5/main.c
+++ b/drivers/vfio/pci/mlx5/main.c
@@ -640,14 +640,11 @@ mlx5vf_pci_save_device_data(struct mlx5vf_pci_core_device *mvdev, bool track)
 					O_RDONLY);
 	if (IS_ERR(migf->filp)) {
 		ret = PTR_ERR(migf->filp);
-		goto end;
+		kfree(migf);
+		return ERR_PTR(ret);
 	}
 
 	migf->mvdev = mvdev;
-	ret = mlx5vf_cmd_alloc_pd(migf);
-	if (ret)
-		goto out_free;
-
 	stream_open(migf->filp->f_inode, migf->filp);
 	mutex_init(&migf->lock);
 	init_waitqueue_head(&migf->poll_wait);
@@ -663,6 +660,11 @@ mlx5vf_pci_save_device_data(struct mlx5vf_pci_core_device *mvdev, bool track)
 	INIT_LIST_HEAD(&migf->buf_list);
 	INIT_LIST_HEAD(&migf->avail_list);
 	spin_lock_init(&migf->list_lock);
+
+	ret = mlx5vf_cmd_alloc_pd(migf);
+	if (ret)
+		goto out;
+
 	ret = mlx5vf_cmd_query_vhca_migration_state(mvdev, &length, &full_size, 0);
 	if (ret)
 		goto out_pd;
@@ -692,10 +694,8 @@ mlx5vf_pci_save_device_data(struct mlx5vf_pci_core_device *mvdev, bool track)
 	mlx5vf_free_data_buffer(buf);
 out_pd:
 	mlx5fv_cmd_clean_migf_resources(migf);
-out_free:
+out:
 	fput(migf->filp);
-end:
-	kfree(migf);
 	return ERR_PTR(ret);
 }
 
@@ -1016,13 +1016,19 @@ mlx5vf_pci_resume_device_data(struct mlx5vf_pci_core_device *mvdev)
 					O_WRONLY);
 	if (IS_ERR(migf->filp)) {
 		ret = PTR_ERR(migf->filp);
-		goto end;
+		kfree(migf);
+		return ERR_PTR(ret);
 	}
 
+	stream_open(migf->filp->f_inode, migf->filp);
+	mutex_init(&migf->lock);
+	INIT_LIST_HEAD(&migf->buf_list);
+	INIT_LIST_HEAD(&migf->avail_list);
+	spin_lock_init(&migf->list_lock);
 	migf->mvdev = mvdev;
 	ret = mlx5vf_cmd_alloc_pd(migf);
 	if (ret)
-		goto out_free;
+		goto out;
 
 	buf = mlx5vf_alloc_data_buffer(migf, 0, DMA_TO_DEVICE);
 	if (IS_ERR(buf)) {
@@ -1041,20 +1047,13 @@ mlx5vf_pci_resume_device_data(struct mlx5vf_pci_core_device *mvdev)
 	migf->buf_header[0] = buf;
 	migf->load_state = MLX5_VF_LOAD_STATE_READ_HEADER;
 
-	stream_open(migf->filp->f_inode, migf->filp);
-	mutex_init(&migf->lock);
-	INIT_LIST_HEAD(&migf->buf_list);
-	INIT_LIST_HEAD(&migf->avail_list);
-	spin_lock_init(&migf->list_lock);
 	return migf;
 out_buf:
 	mlx5vf_free_data_buffer(migf->buf[0]);
 out_pd:
 	mlx5vf_cmd_dealloc_pd(migf);
-out_free:
+out:
 	fput(migf->filp);
-end:
-	kfree(migf);
 	return ERR_PTR(ret);
 }
 
-- 
2.18.1

Re: [PATCH vfio 2/2] vfio/mlx5: Fix unwind flows in mlx5vf_pci_save/resume_device_data()

[Jason Gunthorpe]

On Thu, Nov 14, 2024 at 11:53:18AM +0200, Yishai Hadas wrote:
> Fix unwind flows in mlx5vf_pci_save_device_data() and
> mlx5vf_pci_resume_device_data() to avoid freeing the migf pointer at the
> 'end' label, as this will be handled by fput(migf->filp) through
> mlx5vf_release_file().
> 
> To ensure mlx5vf_release_file() functions correctly, move the
> initialization of migf fields (such as migf->lock) to occur before any
> potential unwind flow, as these fields may be accessed within
> mlx5vf_release_file().
> 
> Fixes: 9945a67ea4b3 ("vfio/mlx5: Refactor PD usage")
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/mlx5/main.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)

Tricky

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH vfio 0/2] Fix several unwind issues in the mlx5/vfio driver

[Alex Williamson]

On Thu, 14 Nov 2024 11:53:16 +0200
Yishai Hadas <yishaih@nvidia.com> wrote:

> This series fixes several unwind issues in the mlx5/vfio driver.
> 
> Further details are provided in the commit logs.
> 
> Yishai
> 
> Yishai Hadas (2):
>   vfio/mlx5: Fix an unwind issue in mlx5vf_add_migration_pages()
>   vfio/mlx5: Fix unwind flows in mlx5vf_pci_save/resume_device_data()
> 
>  drivers/vfio/pci/mlx5/cmd.c  |  6 +++++-
>  drivers/vfio/pci/mlx5/main.c | 35 +++++++++++++++++------------------
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 

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

Alex