| 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: <20080805140327.16494fb0.akpm@linux-foundation.org>
Date: Tue, 5 Aug 2008 14:03:27 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Jaswinder Singh <jaswinder@...radead.org>
Cc: linux-kernel@...r.kernel.org, dwmw2@...radead.org
Subject: Re: [PATCH] firmware: avoiding multiple replication for same
firmware file
On Fri, 01 Aug 2008 11:30:59 +0530
Jaswinder Singh <jaswinder@...radead.org> wrote:
> Now when request_firmware will be called it will check whether firmware
> is already allocated or not. If already allocated then it provide handle
> of old firmware handle and increase count of firmware.
>
> release_firmware will decrease count of firmware, if count is one only then
> firmware will be release.
>
> Added release_firmware_all() can be called from driver cleanup or exit
> to release firmware forcefully.
>
> Signed-off-by: Jaswinder Singh <jaswinder@...radead.org>
> ---
> drivers/base/firmware_class.c | 112 ++++++++++++++++++++++++++++++++++++-----
> include/linux/firmware.h | 5 ++
> 2 files changed, 104 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b0be1d1..9743a3d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -49,6 +49,15 @@ struct firmware_priv {
> struct timer_list timeout;
> };
>
> +struct firmware_list {
> + const struct firmware *fw;
> + char *name;
> + int count;
> + struct list_head list;
> +};
OK, that's a per-item data structure.
> +static struct firmware_list firmwarelist;
No, we shouldn't declare an entire static `struct firmware_list' just
for storage of all the dynamically allocated ones. Instead simply do:
static LIST_HEAD(firmwarelist);
and use that.
Please also add a comment indicating which lock protects that global
list. If it is indeed protected. If not, fix it. If no fix is
needed, add a comment explaining why this list doesn't need locking.
> #ifdef CONFIG_FW_LOADER
> extern struct builtin_fw __start_builtin_fw[];
> extern struct builtin_fw __end_builtin_fw[];
> @@ -400,11 +409,21 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> struct firmware_priv *fw_priv;
> struct firmware *firmware;
> struct builtin_fw *builtin;
> + struct firmware_list *tmp;
> int retval;
>
> if (!firmware_p)
> return -EINVAL;
>
> + /* Return firmware pointer from firmware list if already allocated */
> + list_for_each_entry(tmp, &firmwarelist.list, list) {
And this becomes
list_for_each_entry(tmp, &firmwarelist, list)
> + if (strcmp(name, tmp->name) == 0) {
> + tmp->count++;
> + *firmware_p = tmp->fw;
> + break;
> + }
> + }
> +
> *firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
> if (!firmware) {
> printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
I don't think that existing printk was needed - the page allocator (or
the oom-killer) will dump information for us in the unlikely event that
this allocation failed.
> @@ -413,6 +432,27 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> goto out;
> }
>
> + tmp = kzalloc(sizeof(struct firmware_list), GFP_KERNEL);
> + if (!tmp) {
> + printk(KERN_ERR "%s: kmalloc(struct firmware_list) failed\n",
> + __func__);
So the newly-added ones don't have much value.
> + retval = -ENOMEM;
> + goto error_kfree_fw;
> + }
> +
> + tmp->name = kzalloc(strlen(name), GFP_KERNEL);
Could have been kmalloc(), but see below.
> + if (!tmp->name) {
> + printk(KERN_ERR "%s: kmalloc firmware_list->name failed\n",
> + __func__);
Unneeeded printk.
> + retval = -ENOMEM;
> + goto error_kfree_fw_list;
> + }
> +
> + tmp->fw = *firmware_p;
> + tmp->count = 1;
> + strcpy(tmp->name, name);
Please replace the kmalloc+strcpy with a call to kstrdup().
> + list_add(&(tmp->list), &(firmwarelist.list));
That is excessively parenthesised.
There is no locking for that list.
> for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
> builtin++) {
> if (strcmp(name, builtin->name))
> @@ -429,7 +469,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
> retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
> if (retval)
> - goto error_kfree_fw;
> + goto error_kfree_fw_name;
>
> fw_priv = dev_get_drvdata(f_dev);
>
> ...
>
> -/**
> +static void __release_firmware(const struct firmware *fw,
> + struct firmware_list *tmp)
> +{
> + struct builtin_fw *builtin;
> +
> + for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
> + builtin++) {
> + if (fw->data == builtin->data)
> + goto free_fw;
> + }
> + vfree(fw->data);
> +free_fw:
> + kfree(fw);
> + list_del(&tmp->list);
locking.
> + kfree(tmp->name);
> + kfree(tmp);
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists