Messages in this thread | | | From | Fabio Estevam <> | Date | Thu, 6 Jan 2022 16:27:32 -0300 | Subject | Re: [PATCH v2] regmap: debugfs: Free debugfs_name buffer after usage |
| |
Hi Mark,
On Thu, Jan 6, 2022 at 3:33 PM Mark Brown <broonie@kernel.org> 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
| |