[PATCH 1/2] interconnect: Add character pointer initialization

[PATCH 1/2] interconnect: Add character pointer initialization

[Yibin Ding]

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

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);
-- 
2.25.1

Re: [PATCH 1/2] interconnect: Add character pointer initialization

[Greg KH]

On Fri, Aug 30, 2024 at 06:22:44PM +0800, Yibin Ding wrote:
> 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(-)
> 
> 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

Why is this needed?  Why not just use the size of the structure?

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

Wow, how did this ever work at all?

What commit id does this fix?

And where are you freeing this memory you just allocated?

thanks,

greg k-h

Re: [PATCH 1/2] interconnect: Add character pointer initialization

[Greg KH]

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

Re: [PATCH 1/2] interconnect: Add character pointer initialization

[Greg KH]

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