[PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms")

[PATCH 0/3] regmap: follow-up for ("regmap: allow to disable all locking mechanisms")

[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

[PATCH 1/3] regmap: rename regmap_lock_unlock_empty() to regmap_lock_unlock_none()

[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

[PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[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

Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[Andy Shevchenko]

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

Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[Bartosz Golaszewski]

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

Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[Mark Brown]

[-- 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 --]

Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[Bartosz Golaszewski]

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

Re: [PATCH 2/3] regmap: don't create the debugfs entries if locking is disabled

[Mark Brown]

[-- 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 --]

[PATCH 3/3] regmap: duplicate the name string stored in regmap

[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

Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap

[Lars-Peter Clausen]

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);
> 

Re: [PATCH 3/3] regmap: duplicate the name string stored in regmap

[Bartosz Golaszewski]

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);
>>
>