* [PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms")
@ 2017-12-13 9:28 Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none() Bartosz Golaszewski
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 9:28 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: linux-kernel, Lars-Peter Clausen, Andy Shevchenko, Bartosz Golaszewski
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(-)
--
2.15.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none()
2017-12-13 9:28 [PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms") Bartosz Golaszewski
@ 2017-12-13 9:28 ` Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 3/3] regmap: duplicate the name string stored in regmap Bartosz Golaszewski
2 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 9:28 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: linux-kernel, Lars-Peter Clausen, Andy Shevchenko, Bartosz Golaszewski
Minor naming convention tweak.
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
drivers/base/regmap/regmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 1da2a9fc40b0..16a66ba84aef 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -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;
--
2.15.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 9:28 [PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms") Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none() Bartosz Golaszewski
@ 2017-12-13 9:28 ` Bartosz Golaszewski
2017-12-13 14:54 ` Andy Shevchenko
2017-12-13 15:39 ` Mark Brown
2017-12-13 9:28 ` [PATCH 3/3] regmap: duplicate the name string stored in regmap Bartosz Golaszewski
2 siblings, 2 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 9:28 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: linux-kernel, Lars-Peter Clausen, Andy Shevchenko, 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 <lars@metafoo.de>
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
drivers/base/regmap/internal.h | 2 ++
drivers/base/regmap/regmap-debugfs.c | 5 +++++
drivers/base/regmap/regmap.c | 1 +
3 files changed, 8 insertions(+)
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 8641183cac2f..e40a8e499d35 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -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 */
diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 36ce3511c733..5a45b95ec00a 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -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;
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 16a66ba84aef..a2a02ce58824 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -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;
--
2.15.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] regmap: duplicate the name string stored in regmap
2017-12-13 9:28 [PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms") Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none() Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled Bartosz Golaszewski
@ 2017-12-13 9:28 ` Bartosz Golaszewski
2017-12-13 9:32 ` Lars-Peter Clausen
2 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 9:28 UTC (permalink / raw)
To: Mark Brown, Greg Kroah-Hartman
Cc: linux-kernel, Lars-Peter Clausen, Andy Shevchenko, 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 <brgl@bgdev.pl>
---
drivers/base/regmap/regmap.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index a2a02ce58824..3952e5d7638a 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -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);
--
2.15.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap
2017-12-13 9:28 ` [PATCH 3/3] regmap: duplicate the name string stored in regmap Bartosz Golaszewski
@ 2017-12-13 9:32 ` Lars-Peter Clausen
2017-12-13 9:56 ` Bartosz Golaszewski
0 siblings, 1 reply; 11+ messages in thread
From: Lars-Peter Clausen @ 2017-12-13 9:32 UTC (permalink / raw)
To: Bartosz Golaszewski, Mark Brown, Greg Kroah-Hartman
Cc: linux-kernel, Andy Shevchenko
On 12/13/2017 10:28 AM, Bartosz Golaszewski wrote:
> 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 <brgl@bgdev.pl>
> ---
> drivers/base/regmap/regmap.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index a2a02ce58824..3952e5d7638a 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -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().
> + 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);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap
2017-12-13 9:32 ` Lars-Peter Clausen
@ 2017-12-13 9:56 ` Bartosz Golaszewski
0 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 9:56 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Mark Brown, Greg Kroah-Hartman, Linux Kernel Mailing List,
Andy Shevchenko
2017-12-13 10:32 GMT+01:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 12/13/2017 10:28 AM, Bartosz Golaszewski wrote:
>> 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 <brgl@bgdev.pl>
>> ---
>> drivers/base/regmap/regmap.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
>> index a2a02ce58824..3952e5d7638a 100644
>> --- a/drivers/base/regmap/regmap.c
>> +++ b/drivers/base/regmap/regmap.c
>> @@ -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().
>
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
>> + 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);
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 9:28 ` [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled Bartosz Golaszewski
@ 2017-12-13 14:54 ` Andy Shevchenko
2017-12-13 14:59 ` Bartosz Golaszewski
2017-12-13 15:39 ` Mark Brown
1 sibling, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2017-12-13 14:54 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Mark Brown, Greg Kroah-Hartman, linux-kernel, Lars-Peter Clausen
On Wed, Dec 13, 2017 at 11:28 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 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 <lars@metafoo.de>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> + 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...".
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 14:54 ` Andy Shevchenko
@ 2017-12-13 14:59 ` Bartosz Golaszewski
0 siblings, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 14:59 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mark Brown, Greg Kroah-Hartman, linux-kernel, Lars-Peter Clausen
2017-12-13 15:54 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Wed, Dec 13, 2017 at 11:28 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> 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 <lars@metafoo.de>
>> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>
>> + 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...".
>
> --
> With Best Regards,
> Andy Shevchenko
Good point, I think "regmap locking disabled..." would be even better
in this case.
Thanks,
Bartosz
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 9:28 ` [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled Bartosz Golaszewski
2017-12-13 14:54 ` Andy Shevchenko
@ 2017-12-13 15:39 ` Mark Brown
2017-12-13 15:46 ` Bartosz Golaszewski
1 sibling, 1 reply; 11+ messages in thread
From: Mark Brown @ 2017-12-13 15:39 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Greg Kroah-Hartman, linux-kernel, Lars-Peter Clausen, Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]
On Wed, Dec 13, 2017 at 10:28:11AM +0100, Bartosz Golaszewski wrote:
> 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 15:39 ` Mark Brown
@ 2017-12-13 15:46 ` Bartosz Golaszewski
2017-12-13 16:00 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2017-12-13 15:46 UTC (permalink / raw)
To: Mark Brown
Cc: Greg Kroah-Hartman, Linux Kernel Mailing List,
Lars-Peter Clausen, Andy Shevchenko
2017-12-13 16:39 GMT+01:00 Mark Brown <broonie@kernel.org>:
> On Wed, Dec 13, 2017 at 10:28:11AM +0100, Bartosz Golaszewski wrote:
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled
2017-12-13 15:46 ` Bartosz Golaszewski
@ 2017-12-13 16:00 ` Mark Brown
0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2017-12-13 16:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Greg Kroah-Hartman, Linux Kernel Mailing List,
Lars-Peter Clausen, Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
On Wed, Dec 13, 2017 at 04:46:10PM +0100, Bartosz Golaszewski wrote:
> 2017-12-13 16:39 GMT+01:00 Mark Brown <broonie@kernel.org>:
> > 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-13 16:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 9:28 [PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms") Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none() Bartosz Golaszewski
2017-12-13 9:28 ` [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled Bartosz Golaszewski
2017-12-13 14:54 ` Andy Shevchenko
2017-12-13 14:59 ` Bartosz Golaszewski
2017-12-13 15:39 ` Mark Brown
2017-12-13 15:46 ` Bartosz Golaszewski
2017-12-13 16:00 ` Mark Brown
2017-12-13 9:28 ` [PATCH 3/3] regmap: duplicate the name string stored in regmap Bartosz Golaszewski
2017-12-13 9:32 ` Lars-Peter Clausen
2017-12-13 9:56 ` Bartosz Golaszewski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.