Re: [PATCH v2 2/4] firmware_loader: Move module refcounts to allow unloading

[Russ Weight]

On Fri, Nov 01, 2024 at 06:15:27PM +0000, Dionna Glaze wrote:
> If a kernel module registers a firmware upload API ops set, then it's
> unable to be moved due to effectively a cyclic reference that the module
> depends on the upload which depends on the module.
> 
> Instead, only require the try_module_get when an upload is requested to
> disallow unloading a module only while the upload is in progress.
> 
> Fixes: 97730bbb242c ("firmware_loader: Add firmware-upload support")
> 
> CC: Sean Christopherson <seanjc@xxxxxxxxxx>
> CC: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CC: Ingo Molnar <mingo@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> CC: Ashish Kalra <ashish.kalra@xxxxxxx>
> CC: Tom Lendacky <thomas.lendacky@xxxxxxx>
> CC: John Allen <john.allen@xxxxxxx>
> CC: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> CC: "David S. Miller" <davem@xxxxxxxxxxxxx>
> CC: Michael Roth <michael.roth@xxxxxxx>
> CC: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> CC: Russ Weight <russ.weight@xxxxxxxxx>
> CC: Danilo Krummrich <dakr@xxxxxxxxxx>
> CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> CC: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> CC: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> 
> Tested-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> Signed-off-by: Dionna Glaze <dionnaglaze@xxxxxxxxxx>

Reviewed-by: Russ Weight <russ.weight@xxxxxxxxx>

> ---
>  drivers/base/firmware_loader/sysfs_upload.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index 829270067d163..7d9c6aef7720a 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -204,6 +204,7 @@ static void fw_upload_main(struct work_struct *work)
>  		fwlp->ops->cleanup(fwl);
>  
>  putdev_exit:
> +	module_put(fwlp->module);
>  	put_device(fw_dev->parent);
>  
>  	/*
> @@ -239,6 +240,9 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs)
>  	}
>  
>  	fwlp = fw_sysfs->fw_upload_priv;
> +	if (!try_module_get(fwlp->module)) /* released in fw_upload_main */
> +		return -EFAULT;
> +
>  	mutex_lock(&fwlp->lock);
>  
>  	/* Do not interfere with an on-going fw_upload */
> @@ -310,13 +314,10 @@ firmware_upload_register(struct module *module, struct device *parent,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if (!try_module_get(module))
> -		return ERR_PTR(-EFAULT);
> -
>  	fw_upload = kzalloc(sizeof(*fw_upload), GFP_KERNEL);
>  	if (!fw_upload) {
>  		ret = -ENOMEM;
> -		goto exit_module_put;
> +		goto exit_err;
>  	}
>  
>  	fw_upload_priv = kzalloc(sizeof(*fw_upload_priv), GFP_KERNEL);
> @@ -358,7 +359,7 @@ firmware_upload_register(struct module *module, struct device *parent,
>  	if (ret) {
>  		dev_err(fw_dev, "%s: device_register failed\n", __func__);
>  		put_device(fw_dev);
> -		goto exit_module_put;
> +		goto exit_err;
>  	}
>  
>  	return fw_upload;
> @@ -372,8 +373,7 @@ firmware_upload_register(struct module *module, struct device *parent,
>  free_fw_upload:
>  	kfree(fw_upload);
>  
> -exit_module_put:
> -	module_put(module);
> +exit_err:
>  
>  	return ERR_PTR(ret);
>  }
> @@ -387,7 +387,6 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
>  {
>  	struct fw_sysfs *fw_sysfs = fw_upload->priv;
>  	struct fw_upload_priv *fw_upload_priv = fw_sysfs->fw_upload_priv;
> -	struct module *module = fw_upload_priv->module;
>  
>  	mutex_lock(&fw_upload_priv->lock);
>  	if (fw_upload_priv->progress == FW_UPLOAD_PROG_IDLE) {
> @@ -403,6 +402,5 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
>  
>  unregister:
>  	device_unregister(&fw_sysfs->dev);
> -	module_put(module);
>  }
>  EXPORT_SYMBOL_GPL(firmware_upload_unregister);
> -- 
> 2.47.0.163.g1226f6d8fa-goog
>