[PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[Pengpeng Hou]

meson_msr_probe() creates a debugfs tree with entries that reference
devm-managed measurement table entries and the driver's private regmap
state. The driver has no remove callback, so unbinding the device can
leave those debugfs entries behind after the private data is released.

Store the debugfs root and remove the subtree from a new remove callback.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/soc/amlogic/meson-clk-measure.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
index d862e30a244e..7ca43bcb622a 100644
--- a/drivers/soc/amlogic/meson-clk-measure.c
+++ b/drivers/soc/amlogic/meson-clk-measure.c
@@ -7,6 +7,7 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/bitfield.h>
+#include <linux/err.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 #include <linux/regmap.h>
@@ -50,6 +51,7 @@ struct meson_msr_data {
 struct meson_msr {
 	struct regmap *regmap;
 	struct meson_msr_data data;
+	struct dentry *debugfs_root;
 };
 
 #define CLK_MSR_ID(__id, __name) \
@@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
 	       sizeof(struct msr_reg_offset));
 
 	root = debugfs_create_dir("meson-clk-msr", NULL);
+	priv->debugfs_root = root;
 	clks = debugfs_create_dir("clks", root);
 
 	debugfs_create_file("measure_summary", 0444, root,
@@ -967,9 +970,19 @@ static int meson_msr_probe(struct platform_device *pdev)
 				    &priv->data.msr_table[i], &clk_msr_fops);
 	}
 
+	platform_set_drvdata(pdev, priv);
+
 	return 0;
 }
 
+static void meson_msr_remove(struct platform_device *pdev)
+{
+	struct meson_msr *priv = platform_get_drvdata(pdev);
+
+	if (!IS_ERR_OR_NULL(priv->debugfs_root))
+		debugfs_remove_recursive(priv->debugfs_root);
+}
+
 static const struct msr_reg_offset msr_reg_offset = {
 	.duty_val = 0x0,
 	.freq_ctrl = 0x4,
@@ -1065,6 +1078,7 @@ MODULE_DEVICE_TABLE(of, meson_msr_match_table);
 
 static struct platform_driver meson_msr_driver = {
 	.probe	= meson_msr_probe,
+	.remove = meson_msr_remove,
 	.driver = {
 		.name		= "meson_msr",
 		.of_match_table	= meson_msr_match_table,
-- 
2.50.1 (Apple Git-155)

[PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[Pengpeng Hou]

meson_msr_probe() creates a debugfs tree with entries that reference
devm-managed measurement table entries and the driver's private regmap
state. The driver has no remove callback, so unbinding the device can
leave those debugfs entries behind after the private data is released.

Store the debugfs root and remove the subtree from a new remove callback.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/soc/amlogic/meson-clk-measure.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
index d862e30a244e..7ca43bcb622a 100644
--- a/drivers/soc/amlogic/meson-clk-measure.c
+++ b/drivers/soc/amlogic/meson-clk-measure.c
@@ -7,6 +7,7 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/bitfield.h>
+#include <linux/err.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
 #include <linux/regmap.h>
@@ -50,6 +51,7 @@ struct meson_msr_data {
 struct meson_msr {
 	struct regmap *regmap;
 	struct meson_msr_data data;
+	struct dentry *debugfs_root;
 };
 
 #define CLK_MSR_ID(__id, __name) \
@@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
 	       sizeof(struct msr_reg_offset));
 
 	root = debugfs_create_dir("meson-clk-msr", NULL);
+	priv->debugfs_root = root;
 	clks = debugfs_create_dir("clks", root);
 
 	debugfs_create_file("measure_summary", 0444, root,
@@ -967,9 +970,19 @@ static int meson_msr_probe(struct platform_device *pdev)
 				    &priv->data.msr_table[i], &clk_msr_fops);
 	}
 
+	platform_set_drvdata(pdev, priv);
+
 	return 0;
 }
 
+static void meson_msr_remove(struct platform_device *pdev)
+{
+	struct meson_msr *priv = platform_get_drvdata(pdev);
+
+	if (!IS_ERR_OR_NULL(priv->debugfs_root))
+		debugfs_remove_recursive(priv->debugfs_root);
+}
+
 static const struct msr_reg_offset msr_reg_offset = {
 	.duty_val = 0x0,
 	.freq_ctrl = 0x4,
@@ -1065,6 +1078,7 @@ MODULE_DEVICE_TABLE(of, meson_msr_match_table);
 
 static struct platform_driver meson_msr_driver = {
 	.probe	= meson_msr_probe,
+	.remove = meson_msr_remove,
 	.driver = {
 		.name		= "meson_msr",
 		.of_match_table	= meson_msr_match_table,
-- 
2.50.1 (Apple Git-155)


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Exposing the `measure_summary` debugfs file before initializing the `priv` pointers in the `msr_table` leads to a potential NULL pointer dereference.
--

commit cf66aebca628dec91393028bc843130a21da66ad
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: soc: amlogic: meson-clk-measure: remove debugfs tree

This commit adds a remove callback to the driver to properly clean up the
debugfs tree created during probe. By storing the debugfs root and removing
it on driver unbind, it prevents leftover entries from referencing freed
private data.

> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
> index d862e30a244ed..7ca43bcb622a2 100644
> --- a/drivers/soc/amlogic/meson-clk-measure.c
> +++ b/drivers/soc/amlogic/meson-clk-measure.c
[ ... ]
> @@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
>  	       sizeof(struct msr_reg_offset));
>  
>  	root = debugfs_create_dir("meson-clk-msr", NULL);
> +	priv->debugfs_root = root;
>  	clks = debugfs_create_dir("clks", root);
>  
>  	debugfs_create_file("measure_summary", 0444, root,
>  			    priv->data.msr_table, &clk_msr_summary_fops);
>  
>  	for (i = 0 ; i < priv->data.msr_count ; ++i) {
>  		if (!priv->data.msr_table[i].name)
>  			continue;
>  
>  		priv->data.msr_table[i].priv = priv;

[Severity: High]
This is a pre-existing issue, but does creating the measure_summary debugfs
file here expose it to userspace before the priv pointers in msr_table are
initialized?

If a user-space process reads measure_summary concurrently while the loop
below is still executing, clk_msr_summary_show() could dereference a NULL
pointer via msr_table->priv:

clk_msr_summary_show() {
    struct meson_msr_id *msr_table = s->private;
    unsigned int msr_count = msr_table->priv->data.msr_count;
    ...
}

Could the measure_summary debugfs file be created after the priv fields are
fully initialized in the loop to avoid this race?

>  		debugfs_create_file(priv->data.msr_table[i].name, 0444, clks,
>  				    &priv->data.msr_table[i], &clk_msr_fops);
>  	}
>  
> +	platform_set_drvdata(pdev, priv);
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615091531.21964-1-pengpeng@iscas.ac.cn?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[neil.armstrong]

On 6/15/26 11:24, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Exposing the `measure_summary` debugfs file before initializing the `priv` pointers in the `msr_table` leads to a potential NULL pointer dereference.
> --
> 
> commit cf66aebca628dec91393028bc843130a21da66ad
> Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
> Subject: soc: amlogic: meson-clk-measure: remove debugfs tree
> 
> This commit adds a remove callback to the driver to properly clean up the
> debugfs tree created during probe. By storing the debugfs root and removing
> it on driver unbind, it prevents leftover entries from referencing freed
> private data.
> 
>> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
>> index d862e30a244ed..7ca43bcb622a2 100644
>> --- a/drivers/soc/amlogic/meson-clk-measure.c
>> +++ b/drivers/soc/amlogic/meson-clk-measure.c
> [ ... ]
>> @@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
>>   	       sizeof(struct msr_reg_offset));
>>   
>>   	root = debugfs_create_dir("meson-clk-msr", NULL);
>> +	priv->debugfs_root = root;
>>   	clks = debugfs_create_dir("clks", root);
>>   
>>   	debugfs_create_file("measure_summary", 0444, root,
>>   			    priv->data.msr_table, &clk_msr_summary_fops);
>>   
>>   	for (i = 0 ; i < priv->data.msr_count ; ++i) {
>>   		if (!priv->data.msr_table[i].name)
>>   			continue;
>>   
>>   		priv->data.msr_table[i].priv = priv;
> 
> [Severity: High]
> This is a pre-existing issue, but does creating the measure_summary debugfs
> file here expose it to userspace before the priv pointers in msr_table are
> initialized?
> 
> If a user-space process reads measure_summary concurrently while the loop
> below is still executing, clk_msr_summary_show() could dereference a NULL
> pointer via msr_table->priv:
> 
> clk_msr_summary_show() {
>      struct meson_msr_id *msr_table = s->private;
>      unsigned int msr_count = msr_table->priv->data.msr_count;
>      ...
> }
> 
> Could the measure_summary debugfs file be created after the priv fields are
> fully initialized in the loop to avoid this race?

Impossible the driver is initialized way before userspace starts.

> 
>>   		debugfs_create_file(priv->data.msr_table[i].name, 0444, clks,
>>   				    &priv->data.msr_table[i], &clk_msr_fops);
>>   	}
>>   
>> +	platform_set_drvdata(pdev, priv);
>> +
>>   	return 0;
>>   }
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[Neil Armstrong]

On 6/15/26 11:15, Pengpeng Hou wrote:
> meson_msr_probe() creates a debugfs tree with entries that reference
> devm-managed measurement table entries and the driver's private regmap
> state. The driver has no remove callback, so unbinding the device can
> leave those debugfs entries behind after the private data is released.
> 
> Store the debugfs root and remove the subtree from a new remove callback.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>   drivers/soc/amlogic/meson-clk-measure.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
> index d862e30a244e..7ca43bcb622a 100644
> --- a/drivers/soc/amlogic/meson-clk-measure.c
> +++ b/drivers/soc/amlogic/meson-clk-measure.c
> @@ -7,6 +7,7 @@
>   #include <linux/of_address.h>
>   #include <linux/platform_device.h>
>   #include <linux/bitfield.h>
> +#include <linux/err.h>
>   #include <linux/seq_file.h>
>   #include <linux/debugfs.h>
>   #include <linux/regmap.h>
> @@ -50,6 +51,7 @@ struct meson_msr_data {
>   struct meson_msr {
>   	struct regmap *regmap;
>   	struct meson_msr_data data;
> +	struct dentry *debugfs_root;
>   };
>   
>   #define CLK_MSR_ID(__id, __name) \
> @@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
>   	       sizeof(struct msr_reg_offset));
>   
>   	root = debugfs_create_dir("meson-clk-msr", NULL);
> +	priv->debugfs_root = root;
>   	clks = debugfs_create_dir("clks", root);
>   
>   	debugfs_create_file("measure_summary", 0444, root,
> @@ -967,9 +970,19 @@ static int meson_msr_probe(struct platform_device *pdev)
>   				    &priv->data.msr_table[i], &clk_msr_fops);
>   	}
>   
> +	platform_set_drvdata(pdev, priv);
> +
>   	return 0;
>   }
>   
> +static void meson_msr_remove(struct platform_device *pdev)
> +{
> +	struct meson_msr *priv = platform_get_drvdata(pdev);
> +
> +	if (!IS_ERR_OR_NULL(priv->debugfs_root))
> +		debugfs_remove_recursive(priv->debugfs_root);
> +}
> +
>   static const struct msr_reg_offset msr_reg_offset = {
>   	.duty_val = 0x0,
>   	.freq_ctrl = 0x4,
> @@ -1065,6 +1078,7 @@ MODULE_DEVICE_TABLE(of, meson_msr_match_table);
>   
>   static struct platform_driver meson_msr_driver = {
>   	.probe	= meson_msr_probe,
> +	.remove = meson_msr_remove,
>   	.driver = {
>   		.name		= "meson_msr",
>   		.of_match_table	= meson_msr_match_table,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

Re: [PATCH] soc: amlogic: meson-clk-measure: remove debugfs tree

[Neil Armstrong]

On 6/15/26 11:15, Pengpeng Hou wrote:
> meson_msr_probe() creates a debugfs tree with entries that reference
> devm-managed measurement table entries and the driver's private regmap
> state. The driver has no remove callback, so unbinding the device can
> leave those debugfs entries behind after the private data is released.
> 
> Store the debugfs root and remove the subtree from a new remove callback.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>   drivers/soc/amlogic/meson-clk-measure.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/soc/amlogic/meson-clk-measure.c b/drivers/soc/amlogic/meson-clk-measure.c
> index d862e30a244e..7ca43bcb622a 100644
> --- a/drivers/soc/amlogic/meson-clk-measure.c
> +++ b/drivers/soc/amlogic/meson-clk-measure.c
> @@ -7,6 +7,7 @@
>   #include <linux/of_address.h>
>   #include <linux/platform_device.h>
>   #include <linux/bitfield.h>
> +#include <linux/err.h>
>   #include <linux/seq_file.h>
>   #include <linux/debugfs.h>
>   #include <linux/regmap.h>
> @@ -50,6 +51,7 @@ struct meson_msr_data {
>   struct meson_msr {
>   	struct regmap *regmap;
>   	struct meson_msr_data data;
> +	struct dentry *debugfs_root;
>   };
>   
>   #define CLK_MSR_ID(__id, __name) \
> @@ -952,6 +954,7 @@ static int meson_msr_probe(struct platform_device *pdev)
>   	       sizeof(struct msr_reg_offset));
>   
>   	root = debugfs_create_dir("meson-clk-msr", NULL);
> +	priv->debugfs_root = root;
>   	clks = debugfs_create_dir("clks", root);
>   
>   	debugfs_create_file("measure_summary", 0444, root,
> @@ -967,9 +970,19 @@ static int meson_msr_probe(struct platform_device *pdev)
>   				    &priv->data.msr_table[i], &clk_msr_fops);
>   	}
>   
> +	platform_set_drvdata(pdev, priv);
> +
>   	return 0;
>   }
>   
> +static void meson_msr_remove(struct platform_device *pdev)
> +{
> +	struct meson_msr *priv = platform_get_drvdata(pdev);
> +
> +	if (!IS_ERR_OR_NULL(priv->debugfs_root))
> +		debugfs_remove_recursive(priv->debugfs_root);
> +}
> +
>   static const struct msr_reg_offset msr_reg_offset = {
>   	.duty_val = 0x0,
>   	.freq_ctrl = 0x4,
> @@ -1065,6 +1078,7 @@ MODULE_DEVICE_TABLE(of, meson_msr_match_table);
>   
>   static struct platform_driver meson_msr_driver = {
>   	.probe	= meson_msr_probe,
> +	.remove = meson_msr_remove,
>   	.driver = {
>   		.name		= "meson_msr",
>   		.of_match_table	= meson_msr_match_table,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Thanks,
Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic