linux-kernel - [PATCH] ipmi: kcs_bmc: Avoid wasting some memory.

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-next>] [day] [month] [year] [list]

Date:   Sun,  4 Sep 2022 15:35:16 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Corey Minyard <minyard@....org>
Cc:     linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        openipmi-developer@...ts.sourceforge.net
Subject: [PATCH] ipmi: kcs_bmc: Avoid wasting some memory.

KCS_MSG_BUFSIZ is 1000.

When using devm_kmalloc(), there is a small memory overhead and, on most
systems, this leads to 40 bytes of extra memory allocation.
So 1040 bytes are expected to be allocated.

The memory allocator works with fixed size hunks of memory. In this case,
it will require 2048 bytes of memory because more than 1024 bytes are
required.

So, when requesting 3 x 1000 bytes, it ends up to 2048 x 3.

In order to avoid wasting 3ko of memory, allocate buffers all at once.
3000+40 bytes will be required and 4ko allocated. This still wastes 1ko,
but it is already better.

Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
---
Looking at this code, I wonder why priv->miscdev.name is not freed in
kcs_bmc_ipmi_remove_device()?

If this make sense, this also mean that KCS_MSG_BUFSIZ can be increased at
no cost.
Or it could be slightly reduce to around 1024-40-1 bytes to keep the logic
which is in place.

Another solution would be to use just kmalloc and add a
devm_add_action_or_reset() call and a function that frees the memory.
If it make sense, KCS_MSG_BUFSIZ could be increased to 1024 and we would
allocate just a little above 3x1024 bytes.
---
 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 486834a962c3..15a4a39a6478 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -485,14 +485,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
 
 	priv->client.dev = kcs_bmc;
 	priv->client.ops = &kcs_bmc_ipmi_client_ops;
-	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
-	priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+	/* Allocate buffers all at once */
+	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ * 3, GFP_KERNEL);
+	priv->data_out = priv->data_in + KCS_MSG_BUFSIZ;
+	priv->kbuffer  = priv->data_in + KCS_MSG_BUFSIZ * 2;
 
 	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
 	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
 					   kcs_bmc->channel);
-	if (!priv->data_in || !priv->data_out || !priv->kbuffer || !priv->miscdev.name)
+	if (!priv->data_in || !priv->miscdev.name)
 		return -EINVAL;
 
 	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
@@ -531,8 +532,6 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
 
 	misc_deregister(&priv->miscdev);
 	kcs_bmc_disable_device(priv->client.dev, &priv->client);
-	devm_kfree(kcs_bmc->dev, priv->kbuffer);
-	devm_kfree(kcs_bmc->dev, priv->data_out);
 	devm_kfree(kcs_bmc->dev, priv->data_in);
 	devm_kfree(kcs_bmc->dev, priv);
 
-- 
2.34.1