Re: [PATCH 1/2] vfio: Publish the container after all fallible initialization

[Alex Williamson]

On Fri, 30 Oct 2026 21:25:26 -0700
Kyle Zeng <kylebot@xxxxxxxxxx> wrote:

> vfio_group_init() publishes /dev/vfio/vfio before vfio_init() has
> finished. The optional no-IOMMU backend is also registered after the
> misc device. A concurrent open can therefore obtain a container while
> the module is still COMING, even if a later initialization step fails.
> 
> The failed-module-init path frees the module without waiting for these
> open files. misc_deregister() prevents new opens, but does not revoke
> existing files, leaving their vfio_fops and callbacks in freed module
> memory. An unprivileged first-autoload race can trigger this by exhausting
> the global task limit before the IRQFD workqueue creates its rescuer.
> A subsequent VFIO_GET_API_VERSION ioctl reports:
> 
>   BUG: KASAN: vmalloc-out-of-bounds in __se_sys_ioctl+0xaf/0x120
>   Read of size 8 at addr ffffffffc089df70 by task vfio_race/1484
> 
> Move container initialization out of vfio_group_init() and make it the
> last fallible step in vfio_init(). Register the no-IOMMU backend before
> the misc device, and unwind both resources in reverse order. Once an
> open can reach vfio_fops, module initialization can no longer fail.
> 
> Fixes: cba3345cc494 ("vfio: VFIO core")
> Signed-off-by: Kyle Zeng <kylebot@xxxxxxxxxx>
> ---
>  drivers/vfio/container.c | 22 +++++++++++++---------
>  drivers/vfio/group.c     |  9 +--------
>  drivers/vfio/vfio_main.c |  8 ++++++++
>  3 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
> index 003281dbf8bc..bd245e23a55d 100644
> --- a/drivers/vfio/container.c
> +++ b/drivers/vfio/container.c
> @@ -577,29 +577,33 @@ int __init vfio_container_init(void)
>  	mutex_init(&vfio.iommu_drivers_lock);
>  	INIT_LIST_HEAD(&vfio.iommu_drivers_list);
>  
> +	if (IS_ENABLED(CONFIG_VFIO_NOIOMMU)) {
> +		ret = vfio_register_iommu_driver(&vfio_noiommu_ops);
> +		if (ret)
> +			goto err_mutex;
> +	}
> +
>  	ret = misc_register(&vfio_dev);
>  	if (ret) {
>  		pr_err("vfio: misc device register failed\n");
> -		return ret;
> +		goto err_noiommu;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_VFIO_NOIOMMU)) {
> -		ret = vfio_register_iommu_driver(&vfio_noiommu_ops);
> -		if (ret)
> -			goto err_misc;
> -	}

This looks wrong to me, an error loading the vfio-noiommu IOMMU backend
shouldn't cause the subsystem to fail.

I think this is fallout from a13b1e472b93 ("vfio: check
vfio_register_iommu_driver() return value") that had no other
justification for inducing this failure than the fact the registration
function can return an error.

The error return is necessary for the module init use cases, not here.

Unwrapping that as an unchecked error avoids the re-ordering here and
in vfio_container_cleanup() below, and this unnecessary unwind
collateral.

This should be a separate patch, and use the correct lists per the
MAINTAINERS file (mising kvm@vger for vfio).

>  	return 0;
>  
> -err_misc:
> -	misc_deregister(&vfio_dev);
> +err_noiommu:
> +	if (IS_ENABLED(CONFIG_VFIO_NOIOMMU))
> +		vfio_unregister_iommu_driver(&vfio_noiommu_ops);
> +err_mutex:
> +	mutex_destroy(&vfio.iommu_drivers_lock);
>  	return ret;
>  }
>  
>  void vfio_container_cleanup(void)
>  {
> +	misc_deregister(&vfio_dev);
>  	if (IS_ENABLED(CONFIG_VFIO_NOIOMMU))
>  		vfio_unregister_iommu_driver(&vfio_noiommu_ops);
> -	misc_deregister(&vfio_dev);
>  	mutex_destroy(&vfio.iommu_drivers_lock);
>  }
>  
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index b2299e5bc6df..dc8f8a38723f 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -898,14 +898,10 @@ int __init vfio_group_init(void)
>  	mutex_init(&vfio.group_lock);
>  	INIT_LIST_HEAD(&vfio.group_list);
>  
> -	ret = vfio_container_init();
> -	if (ret)
> -		return ret;
> -
>  	/* /dev/vfio/$GROUP */
>  	ret = class_register(&vfio_class);
>  	if (ret)
> -		goto err_group_class;
> +		return ret;
>  
>  	ret = alloc_chrdev_region(&vfio.group_devt, 0, MINORMASK + 1, "vfio");
>  	if (ret)
> @@ -914,8 +910,6 @@ int __init vfio_group_init(void)
>  
>  err_alloc_chrdev:
>  	class_unregister(&vfio_class);
> -err_group_class:
> -	vfio_container_cleanup();
>  	return ret;
>  }
>  
> @@ -925,5 +919,4 @@ void vfio_group_cleanup(void)
>  	ida_destroy(&vfio.group_ida);
>  	unregister_chrdev_region(vfio.group_devt, MINORMASK + 1);
>  	class_unregister(&vfio_class);
> -	vfio_container_cleanup();
>  }
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 423ead48aafe..dfc48a9e0fe6 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -1852,10 +1852,17 @@ static int __init vfio_init(void)
>  	if (ret)
>  		goto err_alloc_dev_chrdev;
>  
> +	/* Publish /dev/vfio/vfio only after all other fallible setup. */
> +	ret = vfio_container_init();
> +	if (ret)
> +		goto err_container;
> +
>  	vfio_debugfs_create_root();
>  	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>  	return 0;
>  
> +err_container:
> +	vfio_cdev_cleanup();
>  err_alloc_dev_chrdev:
>  	class_unregister(&vfio_device_class);
>  err_dev_class:
> @@ -1868,6 +1875,7 @@ static int __init vfio_init(void)
>  static void __exit vfio_cleanup(void)
>  {
>  	vfio_debugfs_remove_root();
> +	vfio_container_cleanup();
>  	ida_destroy(&vfio.device_ida);
>  	vfio_cdev_cleanup();
>  	class_unregister(&vfio_device_class);
> 
> base-commit: 70dd4ee72eb2d5793c62e71c14e3aba78e5a4e56

Jason additionally asked[1] whether the try_module_get() via misc_open()
-> fops_get() should be the real barrier, preventing open until the
module has fully initialized.  That seems a valid question here as well.
Thanks,

Alex

[1]https://lore.kernel.org/all/20261030120845.GA3889730@xxxxxxxx/