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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241024154414.qm6qfzhr7cqzciel@4VRSMR2-DT.corp.robot.car>
Date: Thu, 24 Oct 2024 08:44:14 -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 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@...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().

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
> > 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ