[PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[Dionna Glaze]

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.

CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Russ Weight <russ.weight@linux.dev>
CC: Danilo Krummrich <dakr@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: "Rafael J. Wysocki" <rafael@kernel.org>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/base/firmware_loader/sysfs_upload.c | 28 ++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index 829270067d16..97b0ae855b5f 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -103,6 +103,10 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
 	if (fwlp->progress == FW_UPLOAD_PROG_IDLE)
 		ret = -ENODEV;
 
+	/*
+	 * Not idle, so fw_upload_start already called try_module_get.
+	 * No need to get/put around cancel.
+	 */
 	fwlp->ops->cancel(fwlp->fw_upload);
 	mutex_unlock(&fwlp->lock);
 
@@ -164,11 +168,13 @@ static void fw_upload_main(struct work_struct *work)
 	enum fw_upload_err ret;
 	struct device *fw_dev;
 	struct fw_upload *fwl;
+	struct module *module;
 
 	fwlp = container_of(work, struct fw_upload_priv, work);
 	fwl = fwlp->fw_upload;
 	fw_sysfs = (struct fw_sysfs *)fwl->priv;
 	fw_dev = &fw_sysfs->dev;
+	module = fwlp->module;
 
 	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
 	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
@@ -204,6 +210,7 @@ static void fw_upload_main(struct work_struct *work)
 		fwlp->ops->cleanup(fwl);
 
 putdev_exit:
+	module_put(module);
 	put_device(fw_dev->parent);
 
 	/*
@@ -238,7 +245,11 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs)
 		return 0;
 	}
 
+
 	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 +321,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 +366,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 +380,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 +394,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) {
@@ -395,6 +401,11 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
 		goto unregister;
 	}
 
+	/*
+	 * No need to try_module_get/module_put around the op since only the
+	 * module itself will call unregister, usually when the refcount has
+	 * dropped to zero and it's cleaning up dependencies to destroy itself.
+	 */
 	fw_upload_priv->ops->cancel(fw_upload);
 	mutex_unlock(&fw_upload_priv->lock);
 
@@ -403,6 +414,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.rc1.288.g06298d1525-goog

Re: [PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[Russ Weight]

On Tue, Oct 15, 2024 at 08:14:24PM +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.

Generally, the parent driver that registers for firmware_upload would
want the module to be present until it unregisters.

Is there a case where this change is needed?

> 
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> CC: Danilo Krummrich <dakr@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@kernel.org>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  drivers/base/firmware_loader/sysfs_upload.c | 28 ++++++++++++++-------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index 829270067d16..97b0ae855b5f 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -103,6 +103,10 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
>  	if (fwlp->progress == FW_UPLOAD_PROG_IDLE)
>  		ret = -ENODEV;
>  
> +	/*
> +	 * Not idle, so fw_upload_start already called try_module_get.
> +	 * No need to get/put around cancel.
> +	 */

This comment isn't necessary. Cancel is intended to signal the
parent driver to abort the upload by returning an error condition
from other ops in progress. It shouldn't attempt to unload the
module.

>  	fwlp->ops->cancel(fwlp->fw_upload);
>  	mutex_unlock(&fwlp->lock);
>  
> @@ -164,11 +168,13 @@ static void fw_upload_main(struct work_struct *work)
>  	enum fw_upload_err ret;
>  	struct device *fw_dev;
>  	struct fw_upload *fwl;
> +	struct module *module;
>  
>  	fwlp = container_of(work, struct fw_upload_priv, work);
>  	fwl = fwlp->fw_upload;
>  	fw_sysfs = (struct fw_sysfs *)fwl->priv;
>  	fw_dev = &fw_sysfs->dev;
> +	module = fwlp->module;
>  
>  	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
>  	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
> @@ -204,6 +210,7 @@ static void fw_upload_main(struct work_struct *work)
>  		fwlp->ops->cleanup(fwl);
>  
>  putdev_exit:
> +	module_put(module);

Skip the local variable: module_put(fwlp->module)

>  	put_device(fw_dev->parent);
>  
>  	/*
> @@ -238,7 +245,11 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs)
>  		return 0;
>  	}
>  
> +
>  	fwlp = fw_sysfs->fw_upload_priv;
> +	if (!try_module_get(fwlp->module)) /* released in fw_upload_main */

Isn't it too late to ensure that the module is present? The
fw_upload_start() function itself resides within the
syfs_upload module. If the module isn't present, then this function
cannot be called. try_module_get() would need to be called before
the call to fw_upload_start().

> +		return -EFAULT;
> +
>  	mutex_lock(&fwlp->lock);
>  
>  	/* Do not interfere with an on-going fw_upload */
> @@ -310,13 +321,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 +366,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 +380,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 +394,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) {
> @@ -395,6 +401,11 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
>  		goto unregister;
>  	}
>  
> +	/*
> +	 * No need to try_module_get/module_put around the op since only the
> +	 * module itself will call unregister, usually when the refcount has
> +	 * dropped to zero and it's cleaning up dependencies to destroy itself.
> +	 */

This comment is unnecessary.

Thanks,
- Russ

>  	fw_upload_priv->ops->cancel(fw_upload);
>  	mutex_unlock(&fw_upload_priv->lock);
>  
> @@ -403,6 +414,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.rc1.288.g06298d1525-goog
> 

Re: [PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[Kalra, Ashish]

On 10/23/2024 3:16 PM, Russ Weight wrote:
> 
> On Tue, Oct 15, 2024 at 08:14:24PM +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.
> 
> Generally, the parent driver that registers for firmware_upload would
> want the module to be present until it unregisters.
> 
> Is there a case where this change is needed?

We are using the firmware_upload_register() API interface for SEV firmware loader/update
with the AMD Crypto CCP driver.

Now, when we call firmware_upload_register() it does a module_get() and bumps the module refcnt and 
then we do the firmware_upload_unregister() as part of the CCP module's exit() callback, but the
CCP module's exit() callback is never invoked as it's refcnt is non-zero, so it is like a catch 22
situation, we want the module's exit() callback to be invoked to call firmware_upload_unregister()
to do a module_put() and decrement module's refcnt, but the callback is never invoked as it's
refcnt is non-zero.

Isn't the firmware_upload_register() API interface intended to be used by standalone drivers ?

Thanks, 
Ashish

Re: [PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[Russ Weight]

On Thu, Oct 24, 2024 at 04:35:20AM -0500, Kalra, Ashish wrote:
> 
> 
> On 10/23/2024 3:16 PM, Russ Weight wrote:
> > 
> > On Tue, Oct 15, 2024 at 08:14:24PM +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.
> > 
> > Generally, the parent driver that registers for firmware_upload would
> > want the module to be present until it unregisters.
> > 
> > Is there a case where this change is needed?
> 
> We are using the firmware_upload_register() API interface for SEV firmware loader/update
> with the AMD Crypto CCP driver.
> 
> Now, when we call firmware_upload_register() it does a module_get() and bumps the module refcnt and 
> then we do the firmware_upload_unregister() as part of the CCP module's exit() callback, but the
> CCP module's exit() callback is never invoked as it's refcnt is non-zero, so it is like a catch 22
> situation, we want the module's exit() callback to be invoked to call firmware_upload_unregister()
> to do a module_put() and decrement module's refcnt, but the callback is never invoked as it's
> refcnt is non-zero.

That makes sense. Thanks for the explanation.

> 
> Isn't the firmware_upload_register() API interface intended to be used by standalone drivers ?

Yes, it is.

Thanks,
- Russ

Re: [PATCH 1/1] firmware_loader: Move module refcounts to allow unloading

[Russ Weight]

On Wed, Oct 23, 2024 at 01:17:07PM -0700, Russ Weight wrote:
> 
> On Tue, Oct 15, 2024 at 08:14:24PM +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.
> 
> Generally, the parent driver that registers for firmware_upload would
> want the module to be present until it unregisters.
> 
> Is there a case where this change is needed?
> 
> > 
> > CC: Luis Chamberlain <mcgrof@kernel.org>
> > CC: Russ Weight <russ.weight@linux.dev>
> > CC: Danilo Krummrich <dakr@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > CC: "Rafael J. Wysocki" <rafael@kernel.org>
> > 
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > ---
> >  drivers/base/firmware_loader/sysfs_upload.c | 28 ++++++++++++++-------
> >  1 file changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> > index 829270067d16..97b0ae855b5f 100644
> > --- a/drivers/base/firmware_loader/sysfs_upload.c
> > +++ b/drivers/base/firmware_loader/sysfs_upload.c
> > @@ -103,6 +103,10 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
> >  	if (fwlp->progress == FW_UPLOAD_PROG_IDLE)
> >  		ret = -ENODEV;
> >  
> > +	/*
> > +	 * Not idle, so fw_upload_start already called try_module_get.
> > +	 * No need to get/put around cancel.
> > +	 */
> 
> This comment isn't necessary. Cancel is intended to signal the
> parent driver to abort the upload by returning an error condition
> from other ops in progress. It shouldn't attempt to unload the
> module.
> 
> >  	fwlp->ops->cancel(fwlp->fw_upload);
> >  	mutex_unlock(&fwlp->lock);
> >  
> > @@ -164,11 +168,13 @@ static void fw_upload_main(struct work_struct *work)
> >  	enum fw_upload_err ret;
> >  	struct device *fw_dev;
> >  	struct fw_upload *fwl;
> > +	struct module *module;
> >  
> >  	fwlp = container_of(work, struct fw_upload_priv, work);
> >  	fwl = fwlp->fw_upload;
> >  	fw_sysfs = (struct fw_sysfs *)fwl->priv;
> >  	fw_dev = &fw_sysfs->dev;
> > +	module = fwlp->module;
> >  
> >  	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
> >  	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
> > @@ -204,6 +210,7 @@ static void fw_upload_main(struct work_struct *work)
> >  		fwlp->ops->cleanup(fwl);
> >  
> >  putdev_exit:
> > +	module_put(module);
> 
> Skip the local variable: module_put(fwlp->module)
> 
> >  	put_device(fw_dev->parent);
> >  
> >  	/*
> > @@ -238,7 +245,11 @@ int fw_upload_start(struct fw_sysfs *fw_sysfs)
> >  		return 0;
> >  	}
> >  
> > +
> >  	fwlp = fw_sysfs->fw_upload_priv;
> > +	if (!try_module_get(fwlp->module)) /* released in fw_upload_main */
> 
> Isn't it too late to ensure that the module is present? The
> fw_upload_start() function itself resides within the
> syfs_upload module. If the module isn't present, then this function
> cannot be called. try_module_get() would need to be called before
> the call to fw_upload_start().

You can disregard the above comment. I was thinking of THIS module,
instead of the module associated with the driver using the API.
This placement of try_module_get() should be okay.

- Russ

> 
> > +		return -EFAULT;
> > +
> >  	mutex_lock(&fwlp->lock);
> >  
> >  	/* Do not interfere with an on-going fw_upload */
> > @@ -310,13 +321,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 +366,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 +380,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 +394,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) {
> > @@ -395,6 +401,11 @@ void firmware_upload_unregister(struct fw_upload *fw_upload)
> >  		goto unregister;
> >  	}
> >  
> > +	/*
> > +	 * No need to try_module_get/module_put around the op since only the
> > +	 * module itself will call unregister, usually when the refcount has
> > +	 * dropped to zero and it's cleaning up dependencies to destroy itself.
> > +	 */
> 
> This comment is unnecessary.
> 
> Thanks,
> - Russ
> 
> >  	fw_upload_priv->ops->cancel(fw_upload);
> >  	mutex_unlock(&fw_upload_priv->lock);
> >  
> > @@ -403,6 +414,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.rc1.288.g06298d1525-goog
> >