Messages in this thread |  | | From | Guangshuo Li <> | | Date | Tue, 5 May 2026 16:48:27 +0800 | | Subject | Re: [PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register() |
| |
Hi Danilo, Russ,
Thanks for your reviewing.
On Tue, 5 May 2026 at 03:18, Danilo Krummrich <dakr@kernel.org> wrote: > > On Sat Apr 18, 2026 at 9:02 AM CEST, Guangshuo Li wrote: > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > > index f59a7856934c..a6dab34b22d8 100644 > > --- a/drivers/base/firmware_loader/sysfs_upload.c > > +++ b/drivers/base/firmware_loader/sysfs_upload.c > > @@ -351,7 +351,8 @@ firmware_upload_register(struct module *module, struct device *parent, > > if (ret != 0) { > > if (ret > 0) > > ret = -EINVAL; > > - goto free_fw_sysfs; > > + put_device(fw_dev); > > + goto exit_module_put; > > } > > fw_priv->is_paged_buf = true; > > fw_sysfs->fw_priv = fw_priv; > > I think this assignment comes too late, by calling put_device() we eventually > end up in fw_upload_free() which expects this to be assigned already. > > I didn't think it through entirely, but can we just move the > > fw_sysfs->fw_upload_priv = fw_upload_priv; > > assignment to after alloc_lookup_fw_priv(), so fw_dev_release() does not enter > this path in the first place?
After manual re-review, I confirmed that the issue Danilo pointed out does exist. In the current v2 patch, fw_sysfs->fw_upload_priv has already been assigned before alloc_lookup_fw_priv() succeeds, while fw_sysfs->fw_priv has not been assigned yet. Therefore, calling put_device() on this failure path may eventually enter fw_upload_free(), which expects fw_sysfs->fw_priv to be valid.
I will address this in a v3 patch.
Thanks, Guangshuo
|  |