[PATCH v3] regmap: debugfs: Fix name collision without atomic operations

[PATCH v3] regmap: debugfs: Fix name collision without atomic operations

[Zxyan Zhu]

The `dummy_index` global variable caused debugfs file name conflicts
during re-entry, leading to creation failures. Use atomic operations
to ensure safe and unique debugfs `dummy%d` naming.

Signed-off-by: Zxyan Zhu <zxyan20@163.com>
---

Changes since v2:
- Remove atomic_dec operation if kasprintf() fails

Changes since v1:
- Replaced atomic_read + atomic_inc with atomic_inc_return.
- Added atomic_dec in the error path to maintain index consistency.
- Updated the commit message to clarify the fix.

At 2025-03-05 00:23:02, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Tue, Mar 04, 2025 at 10:24:52PM +0800, Zxyan Zhu wrote:
>>  	if (!strcmp(name, "dummy")) {
>>  		kfree(map->debugfs_name);
>> -		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d",
>> -						dummy_index);
>> -		if (!map->debugfs_name)
>> +		index = atomic_inc_return(&dummy_index);
>> +		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d", index);
>> +		if (!map->debugfs_name) {
>> +			atomic_dec(&dummy_index);
>>  			return;
>> +		}
>>  		name = map->debugfs_name;
>> -		dummy_index++;
>
>Shouldn't you just use an idr here if there is a race condition?
>There's a lock built into it that should solve all of these issues.
>
>thanks,
>
>greg k-h

Thanks for the feedback! I think using atomic_t is a better fit here
than idr because:
1. It's lighter and simpler for our basic indexing needs.
2. No extra resource management is needed. If we use idr, we'd have to
 handle resource cleanup and add locks for debugfs_init/debugfs_exit
 concurrency.
3. As Mark mentioned, id continuity isn't a hard requirement, so we
 can even skip the atomic_dec on kasprintf failure.

If you still think idr has clear advantages here, I'm happy to adjust
the code, but it'd be great to understand the specific benefits in
this context.

Cheers,

Zxyan Zhu

 drivers/base/regmap/regmap-debugfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index fb84cda92a75..8811c42dccb7 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -20,7 +20,7 @@ struct regmap_debugfs_node {
 	struct list_head link;
 };
 
-static unsigned int dummy_index;
+static atomic_t dummy_index = ATOMIC_INIT(0);
 static struct dentry *regmap_debugfs_root;
 static LIST_HEAD(regmap_debugfs_early_list);
 static DEFINE_MUTEX(regmap_debugfs_early_lock);
@@ -549,6 +549,7 @@ void regmap_debugfs_init(struct regmap *map)
 	struct regmap_range_node *range_node;
 	const char *devname = "dummy";
 	const char *name = map->name;
+	int index;
 
 	/*
 	 * Userspace can initiate reads from the hardware over debugfs.
@@ -595,12 +596,11 @@ void regmap_debugfs_init(struct regmap *map)
 
 	if (!strcmp(name, "dummy")) {
 		kfree(map->debugfs_name);
-		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d",
-						dummy_index);
+		index = atomic_inc_return(&dummy_index);
+		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d", index);
 		if (!map->debugfs_name)
 			return;
 		name = map->debugfs_name;
-		dummy_index++;
 	}
 
 	map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
-- 
2.34.1

Re: [PATCH v3] regmap: debugfs: Fix name collision without atomic operations

[Greg KH]

On Wed, Mar 05, 2025 at 09:32:52PM +0800, Zxyan Zhu wrote:
> Thanks for the feedback! I think using atomic_t is a better fit here
> than idr because:
> 1. It's lighter and simpler for our basic indexing needs.

No, idr, and ida (don't remember which is which for this use case, but
one of them is the right one) was explicitly written for this exact type
of functionality.

> 2. No extra resource management is needed. If we use idr, we'd have to
>  handle resource cleanup and add locks for debugfs_init/debugfs_exit
>  concurrency.

The idr has a lock in it, right?  Use that.

> 3. As Mark mentioned, id continuity isn't a hard requirement, so we
>  can even skip the atomic_dec on kasprintf failure.

Again, use the data structure designed for this very thing.  To ignore
that feels very odd.

Drivers should almost NEVER use atomic values, as there are other apis
to use instead that are correct.  If you find yourself using one, always
verify that you really should be doing that.

thanks,

greg k-h

[PATCH v3] regmap: debugfs: Fix name collision without atomic operations

[Zxyan Zhu]

The `dummy_index` global variable caused debugfs file name conflicts
during re-entry, leading to creation failures. Use atomic operations
to ensure safe and unique debugfs `dummy%d` naming.

Signed-off-by: Zxyan Zhu <zxyan20@163.com>

Changes since v2:
- Remove atomic_dec operation if kasprintf() fails

Changes since v1:
- Replaced atomic_read + atomic_inc with atomic_inc_return.
- Added atomic_dec in the error path to maintain index consistency.
- Updated the commit message to clarify the fix.

At 2025-03-05 00:23:02, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Tue, Mar 04, 2025 at 10:24:52PM +0800, Zxyan Zhu wrote:
>>  	if (!strcmp(name, "dummy")) {
>>  		kfree(map->debugfs_name);
>> -		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d",
>> -						dummy_index);
>> -		if (!map->debugfs_name)
>> +		index = atomic_inc_return(&dummy_index);
>> +		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d", index);
>> +		if (!map->debugfs_name) {
>> +			atomic_dec(&dummy_index);
>>  			return;
>> +		}
>>  		name = map->debugfs_name;
>> -		dummy_index++;
>
>Shouldn't you just use an idr here if there is a race condition?
>There's a lock built into it that should solve all of these issues.
>
>thanks,
>
>greg k-h

Thanks for the feedback! I think using atomic_t is a better fit here
than idr because:
1. It's lighter and simpler for our basic indexing needs.
2. No extra resource management is needed. If we use idr, we'd have to
 handle resource cleanup and add locks for debugfs_init/debugfs_exit
 concurrency.
3. As Mark mentioned, id continuity isn't a hard requirement, so we
 can even skip the atomic_dec on kasprintf failure.

If you still think idr has clear advantages here, I'm happy to adjust
the code, but it'd be great to understand the specific benefits in
this context.

Cheers,

Zxyan Zhu
---
 drivers/base/regmap/regmap-debugfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index fb84cda92a75..8811c42dccb7 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -20,7 +20,7 @@ struct regmap_debugfs_node {
 	struct list_head link;
 };
 
-static unsigned int dummy_index;
+static atomic_t dummy_index = ATOMIC_INIT(0);
 static struct dentry *regmap_debugfs_root;
 static LIST_HEAD(regmap_debugfs_early_list);
 static DEFINE_MUTEX(regmap_debugfs_early_lock);
@@ -549,6 +549,7 @@ void regmap_debugfs_init(struct regmap *map)
 	struct regmap_range_node *range_node;
 	const char *devname = "dummy";
 	const char *name = map->name;
+	int index;
 
 	/*
 	 * Userspace can initiate reads from the hardware over debugfs.
@@ -595,12 +596,11 @@ void regmap_debugfs_init(struct regmap *map)
 
 	if (!strcmp(name, "dummy")) {
 		kfree(map->debugfs_name);
-		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d",
-						dummy_index);
+		index = atomic_inc_return(&dummy_index);
+		map->debugfs_name = kasprintf(GFP_KERNEL, "dummy%d", index);
 		if (!map->debugfs_name)
 			return;
 		name = map->debugfs_name;
-		dummy_index++;
 	}
 
 	map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
-- 
2.34.1

Re: [PATCH v3] regmap: debugfs: Fix name collision without atomic operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Wed, Mar 05, 2025 at 09:27:18PM +0800, Zxyan Zhu wrote:
> The `dummy_index` global variable caused debugfs file name conflicts
> during re-entry, leading to creation failures. Use atomic operations
> to ensure safe and unique debugfs `dummy%d` naming.
> 
> Signed-off-by: Zxyan Zhu <zxyan20@163.com>
> 
> Changes since v2:
> - Remove atomic_dec operation if kasprintf() fails

This should go after the --- as covered in submitting-patches.rst.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

> At 2025-03-05 00:23:02, "Greg KH" <gregkh@linuxfoundation.org> wrote:
> >On Tue, Mar 04, 2025 at 10:24:52PM +0800, Zxyan Zhu wrote:
> >>  	if (!strcmp(name, "dummy")) {

Please don't mix stuff like this into patch postings, the tooling can't
cope with stripping it out automatically.

> Thanks for the feedback! I think using atomic_t is a better fit here
> than idr because:
> 1. It's lighter and simpler for our basic indexing needs.
> 2. No extra resource management is needed. If we use idr, we'd have to
>  handle resource cleanup and add locks for debugfs_init/debugfs_exit
>  concurrency.
> 3. As Mark mentioned, id continuity isn't a hard requirement, so we
>  can even skip the atomic_dec on kasprintf failure.

I don't think we care about cost for allocating new regmaps, if that's a
hot path we've probably got bigger problems.

> If you still think idr has clear advantages here, I'm happy to adjust
> the code, but it'd be great to understand the specific benefits in
> this context.

It's the standard idiom for this application.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]