The following error message is seen when booting an imx6q-sabresd:
debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent 'regmap' already present!
The reason for the duplicate name is that map->debugfs_name is never freed,
which can cause a directory to be created with the same name used in the
previous debugfs entry allocation.
Fix this problem by freeing map->debugfs_name and setting it to NULL
after its usage.
Signed-off-by: Fabio Estevam <[email protected]>
---
Changes since v1:
- Avoid use after free (Mark).
drivers/base/regmap/regmap-debugfs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index ad684d37c2da..e1017ca65be0 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -605,6 +605,9 @@ void regmap_debugfs_init(struct regmap *map)
map->debugfs = debugfs_create_dir(name, regmap_debugfs_root);
+ kfree(map->debugfs_name);
+ map->debugfs_name = NULL;
+
debugfs_create_file("name", 0400, map->debugfs,
map, ®map_name_fops);
--
2.25.1
On Thu, Jan 06, 2022 at 02:50:19PM -0300, Fabio Estevam wrote:
> The reason for the duplicate name is that map->debugfs_name is never freed,
> which can cause a directory to be created with the same name used in the
> previous debugfs entry allocation.
> Fix this problem by freeing map->debugfs_name and setting it to NULL
> after its usage.
OK, but what's the logic here? The name is getting thrown away here but
clearly there is a file still so I'm not seeing how anything is going to
clean that file up. That means that if the device gets freed we'll end
up with the old debugfs file hanging around pointing at nothing. Like I
said (originally in response to Matthias' patch but pasted in this
thread as well):
| (we should probably clean up the one with no device but that's not what
| your commit does). I think what you need to look at here is that we
The use after free extends beyond just the filename, we're also loosing
track of the already created file, which does seem to be an existing
bug. To be more explicit this means we need a call to regmap_debugfs_exit()
which will clean up all the existing debugfs stuff before we loose
references to it.
Hi Mark,
On Thu, Jan 6, 2022 at 3:33 PM Mark Brown <[email protected]> wrote:
> OK, but what's the logic here? The name is getting thrown away here but
I did more debugging and this is what I found:
The 'debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent
'regmap' already present!' message
happens since commit cffa4b2122f5 ("regmap: debugfs: Fix a memory leak
when calling regmap_attach_dev").
Prior to this commit map->debugfs_name would always be allocated via
kasprintf().
After this commit, the allocation only happens when !map->debugfs_name.
This matches with my observations:
- The first allocation for dummy-iomuxc-gpr@20e0000 works as
map->debugfs_name is NULL.
- The second time map->debugfs_name is not NULL, so there is no allocation
via kasprintf(), and the map->debugfs_name uses the 'old' entry from
the previous buffer.
This causes the directory name to be duplicated and fails to be created.
That's why clearing map->debugfs_name causes a new kasprintf()
allocation and restores the correct behavior.
Prior to cffa4b2122f5 ("regmap: debugfs: Fix a memory leak when
calling regmap_attach_dev"):
# mount -t debugfs none /sys/kernel/debug/
# cat /sys/kernel/debug/regmap/dummy-iomuxc-gpr@20e0000/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 0000000e
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4
# cat /sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 00000007
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4
After commit cffa4b2122f5 ("regmap: debugfs: Fix a memory leak when
calling regmap_attach_dev):
The 'debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent
'regmap' already present!' message is seen.
# cat /sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers
cat: can't open
'/sys/kernel/debug/regmap/20e0000.pinctrl-gpr/registers': No such file
or directory
# cat /sys/kernel/debug/regmap/dummy-iomuxc-gpr@20e0000/registers
00: 00000000
04: 48611005
08: 0000000c
0c: 01e00000
10: f00000cf
14: 00000009
18: 007f007f
1c: 007f007f
20: fffd4000
24: 00000000
28: 00003800
2c: 00000000
30: 0f004490
34: 0593e4a4
> clearly there is a file still so I'm not seeing how anything is going to
> clean that file up. That means that if the device gets freed we'll end
> up with the old debugfs file hanging around pointing at nothing. Like I
> said (originally in response to Matthias' patch but pasted in this
> thread as well):
>
> | (we should probably clean up the one with no device but that's not what
> | your commit does). I think what you need to look at here is that we
>
> The use after free extends beyond just the filename, we're also loosing
> track of the already created file, which does seem to be an existing
> bug. To be more explicit this means we need a call to regmap_debugfs_exit()
> which will clean up all the existing debugfs stuff before we loose
> references to it.
As shown above, I don't see the '
/sys/kernel/debug/regmap/20e0000.pinctrl-gpr' directory being created,
so there is nothing to clean.
Where exactly would you like me to call regmap_debugfs_exit()?
Thanks
On Thu, Jan 06, 2022 at 04:27:32PM -0300, Fabio Estevam wrote:
> On Thu, Jan 6, 2022 at 3:33 PM Mark Brown <[email protected]> wrote:
> > OK, but what's the logic here? The name is getting thrown away here but
> I did more debugging and this is what I found:
> The 'debugfs: Directory 'dummy-iomuxc-gpr@20e0000' with parent
> 'regmap' already present!' message
> happens since commit cffa4b2122f5 ("regmap: debugfs: Fix a memory leak
> when calling regmap_attach_dev").
Sure, but that just means that the bug with not cleaning up the
directory predates that.
> # mount -t debugfs none /sys/kernel/debug/
> # cat /sys/kernel/debug/regmap/dummy-iomuxc-gpr@20e0000/registers
> 00: 00000000
> 04: 48611005
What happens if the underlying regmap gets freed for some reason? It
now only remembers the new name, not this old one, so it'll only know to
clean up the old name.
> As shown above, I don't see the '
> /sys/kernel/debug/regmap/20e0000.pinctrl-gpr' directory being created,
> so there is nothing to clean.
Creating that file is the behaviour you are demanding...
> Where exactly would you like me to call regmap_debugfs_exit()?
Before we try to reinitialise debugfs for the new name seems like the
obvious place.
Hi Mark,
On Thu, Jan 6, 2022 at 5:05 PM Mark Brown <[email protected]> wrote:
> > Where exactly would you like me to call regmap_debugfs_exit()?
>
> Before we try to reinitialise debugfs for the new name seems like the
> obvious place.
I am afraid I am not enough familiar with regmap to fix this problem.
If you could please submit a patch, I will be glad to test it.
Thanks
Hi Mark,
On Thu, Jan 6, 2022 at 6:13 PM Fabio Estevam <[email protected]> wrote:
>
> Hi Mark,
>
> On Thu, Jan 6, 2022 at 5:05 PM Mark Brown <[email protected]> wrote:
>
> > > Where exactly would you like me to call regmap_debugfs_exit()?
> >
> > Before we try to reinitialise debugfs for the new name seems like the
> > obvious place.
>
> I am afraid I am not enough familiar with regmap to fix this problem.
>
> If you could please submit a patch, I will be glad to test it.
I tried this change:
diff --git a/drivers/base/regmap/regmap-debugfs.c
b/drivers/base/regmap/regmap-debugfs.c
index ad684d37c2da..fa8821ecc06a 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -581,6 +581,8 @@ void regmap_debugfs_init(struct regmap *map)
if (map->dev)
devname = dev_name(map->dev);
+ regmap_debugfs_exit(map);
+
if (name) {
if (!map->debugfs_name) {
map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
It does avoid the 'already present' error and I see that
/sys/kernel/debug/regmap/20e0000.pinctrl-gpr
is present, but /sys/kernel/debug/regmap/20e0000.pinctrl-gpr is not.
Not sure if this is the desired behavior.
Cheers
On Thu, Jan 06, 2022 at 11:22:32PM -0300, Fabio Estevam wrote:
> On Thu, Jan 6, 2022 at 6:13 PM Fabio Estevam <[email protected]> wrote:
> > > Before we try to reinitialise debugfs for the new name seems like the
> > > obvious place.
> > I am afraid I am not enough familiar with regmap to fix this problem.
> > If you could please submit a patch, I will be glad to test it.
> I tried this change:
> @@ -581,6 +581,8 @@ void regmap_debugfs_init(struct regmap *map)
> if (map->dev)
> devname = dev_name(map->dev);
>
> + regmap_debugfs_exit(map);
> +
I would have expected this to be prior to the call to _init() rather
than actually in the call to _init() but OTOH this should work fine so
meh.
> It does avoid the 'already present' error and I see that
> /sys/kernel/debug/regmap/20e0000.pinctrl-gpr
> is present, but /sys/kernel/debug/regmap/20e0000.pinctrl-gpr is not.
> Not sure if this is the desired behavior.
Yes, that's what we're looking for (assuming the second of those names
should be the dummy name) - that means that the old debugfs file is not
hanging around and so won't be stuck there pointing at a deallocated
regmap if the regmap gets freed for some reason.
On Fri, Jan 7, 2022 at 10:27 AM Mark Brown <[email protected]> wrote:
> I would have expected this to be prior to the call to _init() rather
> than actually in the call to _init() but OTOH this should work fine so
> meh.
Yes, I can call it prior to _init() as you suggested:
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 21a0c2562ec0..f7811641ed5a 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -647,6 +647,7 @@ int regmap_attach_dev(struct device *dev, struct
regmap *map,
if (ret)
return ret;
+ regmap_debugfs_exit(map);
regmap_debugfs_init(map);
/* Add a devres resource for dev_get_regmap() */
Will send a patch shortly.
Thanks
On Fri, Jan 07, 2022 at 11:48:59AM -0300, Fabio Estevam wrote:
> Will send a patch shortly.
Great!