| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241023201654.pjz67e5cv7kbki5t@4VRSMR2-DT.corp.robot.car>
Date: Wed, 23 Oct 2024 13:16:54 -0700
From: Russ Weight <russ.weight@...ux.dev>
To: Dionna Glaze <dionnaglaze@...gle.com>
Cc: linux-kernel@...r.kernel.org, Ashish.Kalra@....com,
Luis Chamberlain <mcgrof@...nel.org>,
Danilo Krummrich <dakr@...hat.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 1/1] firmware_loader: Move module refcounts to allow
unloading
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@...nel.org>
> CC: Russ Weight <russ.weight@...ux.dev>
> CC: Danilo Krummrich <dakr@...hat.com>
> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> CC: "Rafael J. Wysocki" <rafael@...nel.org>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.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
>
Powered by blists - more mailing lists