diff mbox series

[1/2] interconnect: Add character pointer initialization

Message ID 20240830102244.409058-1-Yibin.Ding@unisoc.com
State New
Headers show
Series Added debugfs node initialization and null pointer detection. | expand

Commit Message

Yibin Ding Aug. 30, 2024, 10:22 a.m. UTC
From: Yibin Ding <Yibin.ding@unisoc.com>

When accessing a node whose data type is a character pointer and has not
been initialized, a crash will occur due to accessing a null pointer. So
it is necessary to add the operation of initializing the character pointer.
Since the debugfs_write_file_str() function performs a kfree() operation
on the node data, memory is allocated to the node pointer during
initialization will be released when data is written to the node.

Signed-off-by: Yibin Ding <Yibin.ding@unisoc.com>
---
 drivers/interconnect/debugfs-client.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

gregkh@linuxfoundation.org Sept. 2, 2024, 6:10 a.m. UTC | #1
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Mon, Sep 02, 2024 at 11:03:29AM +0800, yi'bin ding wrote:
> Thanks in advance,
> 
> Why is this needed?  Why not just use the size of the structure?
> 
> answer:Because the memory allocated here is not actually used, it is just
> to prevent the occurrence of null pointer.In order to prevent memory waste,
> I applied for a small amount of memory as possible. If necessary, I can
> submit another revision to change it to the size of the structure or just
> to "1" without the macro definition.

Make it correct please.

> What commit id does this fix?
> 
> answer:commit id:770c69f037c18cfaa37c3d6c6ef8bd257635513f (interconnect:
> Add debugfs test client)
> This commit creates some debugfs nodes, where the src_node and dst_node
> character pointers are not initialized. This will result in accessing null
> pointers when accessing them directly.

Please put the proper Fixes: tag in the changelog text then.

> And where are you freeing this memory you just allocated?
> 
> answer:The memory allocated here will be released when data is written to
> the node. The write operation will call debugfs_write_file_str() function,
> in which a new piece of memory will be allocated to save the new data, and
> then the old memory will be released.

That happens if you write to the file, but what happens if you never
write to the file?  What happens when you remove the driver/module,
shouldn't you free the memory then as well?

thanks,

greg k-h
gregkh@linuxfoundation.org Sept. 3, 2024, 8 a.m. UTC | #2
Again, please do not top-post, please fix your email client.

On Tue, Sep 03, 2024 at 03:23:13PM +0800, yi'bin ding wrote:
> Thanks in advance,
> 
> That happens if you write to the file, but what happens if you never write
> to the file?  What happens when you remove the driver/module,shouldn't you
> free the memory then as well?
> 
> answer:If no write operation is performed on the node, this part of the
> memory will not be released before shutdown. The initialization operation
> of this module is performed by the swapper process, so this part of the
> memory will be released when the swapper process is terminated during
> shutdown.

What "swapper process" are you talking about here?  Specifics please.

confused,

greg k-h
diff mbox series

Patch

diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
index bc3fd8a7b9eb..d62ba56b7bbe 100644
--- a/drivers/interconnect/debugfs-client.c
+++ b/drivers/interconnect/debugfs-client.c
@@ -16,6 +16,7 @@ 
 #undef INTERCONNECT_ALLOW_WRITE_DEBUGFS
 
 #if defined(INTERCONNECT_ALLOW_WRITE_DEBUGFS) && defined(CONFIG_DEBUG_FS)
+#define INITNODE_SIZE 1
 
 static LIST_HEAD(debugfs_paths);
 static DEFINE_MUTEX(debugfs_lock);
@@ -147,8 +148,13 @@  int icc_debugfs_client_init(struct dentry *icc_dir)
 
 	client_dir = debugfs_create_dir("test_client", icc_dir);
 
-	debugfs_create_str("src_node", 0600, client_dir, &src_node);
-	debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
+	src_node = kzalloc(INITNODE_SIZE, GFP_KERNEL);
+	dst_node = kzalloc(INITNODE_SIZE, GFP_KERNEL);
+
+	if (src_node)
+		debugfs_create_str("src_node", 0600, client_dir, &src_node);
+	if (dst_node)
+		debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
 	debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
 	debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
 	debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);