regmap: follow-up for ("regmap: allow to disable all locking mechanisms")
Subsystems (2)
cover regmap: follow-up for ("regmap: allow to disable all locking mechanisms")
Hi Mark,
this series is a follow-up for my previous patch. The first commit renames the empty locking routine introduced in it - I agree with Andy that it just looks better.
The second patch disables the debugfs entries when locking is disabled as suggested by Lars-Peter.
The last patch is optional - feel free to drop it if you think it's not necessary. I just noticed that most frameworks duplicate any name strings, if it's possible that an unaware user gives us a pointer to a temporary buffer. While I haven't found any such instance in the current kernel, I believe it would make regmap safer for the future.
Bartosz Golaszewski (3):
regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none()
regmap: don't create the debugfs entries if locking is disabled
regmap: duplicate the name string stored in regmap
drivers/base/regmap/internal.h | 2 ++ drivers/base/regmap/regmap-debugfs.c | 5 +++++ drivers/base/regmap/regmap.c | 15 ++++++++++++--- 3 files changed, 19 insertions(+), 3 deletions(-)
Signature · 1 line
2.15.1
PATCH 1/3 regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none()
Minor naming convention tweak.
--- drivers/base/regmap/regmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/base/regmap/regmap.c +2 -2
@@ -457,7 +457,7 @@ static void regmap_unlock_hwlock_irqrestore(void *__map)hwspin_unlock_irqrestore (map->hwlock, &map->spinlock_flags); } -static void regmap_lock_unlock_empty (void *__map) +static void regmap_lock_unlock_none (void *__map) { }
@@ -673,7 +673,7 @@ struct regmap *__regmap_init(struct device *dev, }if (config->disable_locking) { - map->lock = map->unlock = regmap_lock_unlock_empty; + map->lock = map->unlock = regmap_lock_unlock_none; }else if (config->lock && config->unlock) { map->lock = config->lock; map->unlock = config->unlock;
Signature · 1 line
2.15.1
PATCH 2/3 regmap: don't create the debugfs entries if locking is disabled
In reply to Bartosz GolaszewskiUser space can initiate concurrent access to regmap over debugfs and, if the locking is disabled, we can't protect it. Don't create the debugfs entries at all in this case.
--- drivers/base/regmap/internal.h | 2 ++ drivers/base/regmap/regmap-debugfs.c | 5 +++++ drivers/base/regmap/regmap.c | 1 + 3 files changed, 8 insertions(+)
drivers/base/regmap/internal.h +2 -0
@@ -60,6 +60,8 @@ struct regmap {regmap_lock lock;regmap_unlock unlock;void *lock_arg;/* This is passed to lock/unlock functions */ +bool locking_disabled; +gfp_t alloc_flags;struct device *dev;/* Device we do I/O on */
drivers/base/regmap/regmap-debugfs.c +5 -0
@@ -529,6 +529,11 @@ void regmap_debugfs_init(struct regmap *map, const char *name)struct regmap_range_node *range_node;const char *devname ="dummy" ; +if (map->locking_disabled) { +dev_dbg (map->dev,"locking disabled - not creating debugfs entries\n" ); +return ; + } +/* If we don't have the debugfs root yet, postpone init */ if (!regmap_debugfs_root) {struct regmap_debugfs_node *node;
drivers/base/regmap/regmap.c +1 -0
@@ -673,6 +673,7 @@ struct regmap *__regmap_init(struct device *dev, }if (config->disable_locking) { + map->locking_disabled = true; map->lock = map->unlock = regmap_lock_unlock_none; }else if (config->lock && config->unlock) { map->lock = config->lock;
Signature · 1 line
2.15.1
Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
Quoted 8 lines · Bartosz Golaszewski
User space can initiate concurrent access to regmap over debugfs and, if the locking is disabled, we can't protect it. Don't create the debugfs entries at all in this case.
Suggested-by Lars-Peter Clausen <[email protected]>Signed-off-by Bartosz Golaszewski <[email protected]>+
dev_dbg(map->dev, "locking disabled - not creating debugfs entries\n");
I dunno about level (OK debug seems good because we are still talking about debug), though message might be slightly confusing (if driver itself is using debugfs for its own purposes), so, I would suggest to add
"regmap: locking...".
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 15:54 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 16 lines · Bartosz Golaszewski
Quoted 8 lines
User space can initiate concurrent access to regmap over debugfs and, if the locking is disabled, we can't protect it. Don't create the debugfs entries at all in this case.
Suggested-by Lars-Peter Clausen <[email protected]>Signed-off-by Bartosz Golaszewski <[email protected]>+
dev_dbg(map->dev, "locking disabled - not creating debugfs entries\n");I dunno about level (OK debug seems good because we are still talking about debug), though message might be slightly confusing (if driver itself is using debugfs for its own purposes), so, I would suggest to add
"regmap: locking...".
Quoted 3 lines
--
With Best Regards,
Andy Shevchenko
Good point, I think "regmap locking disabled..." would be even better in this case.
Thanks,
Bartosz
Mark Brown
maintainerRe: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
In reply to Bartosz GolaszewskiQuoted 3 lines · Bartosz Golaszewski
User space can initiate concurrent access to regmap over debugfs and, if the locking is disabled, we can't protect it. Don't create the debugfs entries at all in this case.
I (probably unsurprisingly) prefer my version that I posted yesterday as it gives us the trivial memory saving of not having the flag if debugfs is disabled but more importantly from my point of view keeps the interface to debugfs as function calls in the core.
Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 16:39 GMT+01:00 Mark Brown <[email protected]>:
Quoted 9 lines · Bartosz Golaszewski
Quoted 3 lines
User space can initiate concurrent access to regmap over debugfs and, if the locking is disabled, we can't protect it. Don't create the debugfs entries at all in this case.
I (probably unsurprisingly) prefer my version that I posted yesterday as it gives us the trivial memory saving of not having the flag if debugfs is disabled but more importantly from my point of view keeps the interface to debugfs as function calls in the core.
It went into spam in my mailbox for some reason, that's why I didn't see it.
Yes, it looks better. I'll drop it from the series.
Thanks,
Bartosz
Mark Brown
maintainerRe: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
Quoted 10 lines · Bartosz Golaszewski
2017-12-13 16:39 GMT+01:00 Mark Brown <[email protected]>:
Quoted 4 lines
I (probably unsurprisingly) prefer my version that I posted yesterday as it gives us the trivial memory saving of not having the flag if debugfs is disabled but more importantly from my point of view keeps the interface to debugfs as function calls in the core.
It went into spam in my mailbox for some reason, that's why I didn't see it.
Yes, it looks better. I'll drop it from the series.
OK, great.
PATCH 3/3 regmap: duplicate the name string stored in regmap
In reply to Bartosz GolaszewskiCurrently we just copy over the pointer passed to regmap_init() in the regmap config struct. To be on the safe side: duplicate the string so that if an unaware user passes an address to a stack-allocated buffer, we won't crash.
--- drivers/base/regmap/regmap.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/base/regmap/regmap.c +9 -1
@@ -672,6 +672,14 @@ struct regmap *__regmap_init(struct device *dev, goto err; } +if (config->name) { + map->name =kstrdup (config->name,GFP_KERNEL ); +if (!map->name) { + ret = -ENOMEM ; + goto err_map; + } + } +if (config->disable_locking) { map->locking_disabled = true; map->lock = map->unlock = regmap_lock_unlock_none;
@@ -763,7 +771,6 @@ struct regmap *__regmap_init(struct device *dev, map->volatile_reg = config->volatile_reg; map->precious_reg = config->precious_reg; map->cache_type = config->cache_type; - map->name = config->name;spin_lock_init (&map->async_lock);INIT_LIST_HEAD (&map->async_list);
@@ -1308,6 +1315,7 @@ void regmap_exit(struct regmap *map) }if (map->hwlock)hwspin_lock_free (map->hwlock); +kfree (map->name);kfree (map); }EXPORT_SYMBOL_GPL (regmap_exit);
Signature · 1 line
2.15.1
Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap
Quoted 15 lines · Bartosz Golaszewski
Currently we just copy over the pointer passed to
regmap_init()in the regmap config struct. To be on the safe side: duplicate the string so that if an unaware user passes an address to a stack-allocated buffer, we won't crash.Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/base/regmap/regmap.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)drivers/base/regmap/regmap.c +0
-0
drivers/base/regmap/regmap.c struct regmap *__regmap_init(struct device *dev, · line 676
Quoted 5 lines
drivers/base/regmap/regmap.c +2
-0@@ -672,6 +672,14 @@ struct regmap *__regmap_init(struct device *dev, goto err; } +if (config->name) { + map->name =kstrdup (config->name,GFP_KERNEL );
To get the best of both worlds: kstrdup_const(), this will not make a copy when it is in a read-only section (like most strings will be). Needs to be matched with kfree_const().
Quoted 26 lines
drivers/base/regmap/regmap.c +7
-1+if (!map->name) { + ret = -ENOMEM ; + goto err_map; + } + } +if (config->disable_locking) { map->locking_disabled = true; map->lock = map->unlock = regmap_lock_unlock_none;@@ -763,7 +771,6 @@ struct regmap *__regmap_init(struct device *dev, map->volatile_reg = config->volatile_reg; map->precious_reg = config->precious_reg; map->cache_type = config->cache_type; - map->name = config->name;spin_lock_init (&map->async_lock);INIT_LIST_HEAD (&map->async_list);@@ -1308,6 +1315,7 @@ void regmap_exit(struct regmap *map) }if (map->hwlock)hwspin_lock_free (map->hwlock); +kfree (map->name);kfree (map); }EXPORT_SYMBOL_GPL (regmap_exit);
Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap
2017-12-13 10:32 GMT+01:00 Lars-Peter Clausen <[email protected]>:
Quoted 19 lines · Bartosz Golaszewski
Quoted 18 lines
Currently we just copy over the pointer passed to
regmap_init()in the regmap config struct. To be on the safe side: duplicate the string so that if an unaware user passes an address to a stack-allocated buffer, we won't crash.Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/base/regmap/regmap.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)drivers/base/regmap/regmap.c +0
-0@@ -672,6 +672,14 @@ struct regmap *__regmap_init(struct device *dev, goto err; }
Quoted 7 lines
drivers/base/regmap/regmap.c struct regmap *__regmap_init(struct device *dev, · line 676
Quoted 2 lines
drivers/base/regmap/regmap.c +2
-0+if (config->name) { + map->name =kstrdup (config->name,GFP_KERNEL );To get the best of both worlds:
kstrdup_const(), this will not make a copy when it is in a read-only section (like most strings will be). Needs to be matched withkfree_const().
Nice! Another useful kernel interface I wasn't aware of. I'll wait some more for any potential comments from others and include that in v2.
Thanks,
Bartosz
Quoted 27 lines
Quoted 26 lines
drivers/base/regmap/regmap.c +7
-1+if (!map->name) { + ret = -ENOMEM ; + goto err_map; + } + } +if (config->disable_locking) { map->locking_disabled = true; map->lock = map->unlock = regmap_lock_unlock_none;@@ -763,7 +771,6 @@ struct regmap *__regmap_init(struct device *dev, map->volatile_reg = config->volatile_reg; map->precious_reg = config->precious_reg; map->cache_type = config->cache_type; - map->name = config->name;spin_lock_init (&map->async_lock);INIT_LIST_HEAD (&map->async_list);@@ -1308,6 +1315,7 @@ void regmap_exit(struct regmap *map) }if (map->hwlock)hwspin_lock_free (map->hwlock); +kfree (map->name);kfree (map); }EXPORT_SYMBOL_GPL (regmap_exit);