Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config

[Suman Anna <s-anna@ti.com>]

On 9/3/20 3:26 AM, Marc Zyngier wrote:
> On 2020-08-27 21:32, Suman Anna wrote:
>> Hi Marc,
>>
>> + Mark Brown
>>
>> On 8/27/20 3:06 PM, Marc Zyngier wrote:
>>> Hi Suman,
>>>
>>> On 2020-08-27 19:28, Suman Anna wrote:
>>>> Hi Marc,
>>>>
>>>> On 8/27/20 9:46 AM, Marc Zyngier wrote:
> 
> [...]
> 
>>>>> This patch triggers some illegal memory accesses when debugfs is
>>>>> enabled, as regmap does rely on config->name to be persistent
>>>>> when the debugfs registration is deferred via regmap_debugfs_early_list
>>>>> (__regmap_init() -> regmap_attach_dev() -> regmap_debugfs_init()...),
>>>>> leading to a KASAN splat on demand.
>>>>>
>>>>
>>>> Thanks, I missed the subtlety around the debugfs registration.
>>>>
>>>>> I came up with the following patch that solves the issue for me.
>>>>>
>>>>> Thanks,
>>>>>
>>>>>         M.
>>>>>
>>>>> From fd3f5f2bf72df53be18d13914fe349a34f81f16b Mon Sep 17 00:00:00 2001
>>>>> From: Marc Zyngier <maz@kernel.org>
>>>>> Date: Thu, 27 Aug 2020 14:45:34 +0100
>>>>> Subject: [PATCH] mfd: syscon: Don't free allocated name for regmap_config
>>>>>
>>>>> The name allocated for the regmap_config structure is freed
>>>>> pretty early, right after the registration of the MMIO region.
>>>>>
>>>>> Unfortunately, that doesn't follow the life cycle that debugfs
>>>>> expects, as it can access the name field long after the free
>>>>> has occured.
>>>>>
>>>>> Move the free on the error path, and keep it forever otherwise.
>>>>
>>>> Hmm, this is exactly what I was trying to avoid. The regmap_init does duplicate
>>>> the name into map->name if config->name is given, and the regmap debugfs makes
>>>> another copy of its own into debugfs_name when actually registered. If the
>>>> rules
>>>> for regmap_init is that the config->name should be persistent, then I guess we
>>>> have no choice but to go with the below fix.
>>>>
>>>> Does something like below help?
>>>>
>>>> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
>>>> index e93700af7e6e..96d8a0161c89 100644
>>>> --- a/drivers/base/regmap/regmap.c
>>>> +++ b/drivers/base/regmap/regmap.c
>>>> @@ -1137,7 +1137,7 @@ struct regmap *__regmap_init(struct device *dev,
>>>>                 if (ret != 0)
>>>>                         goto err_regcache;
>>>>         } else {
>>>> -               regmap_debugfs_init(map, config->name);
>>>> +               regmap_debugfs_init(map, map->name);
>>>>
>>>> But there are couple of other places in regmap code that uses config->name, but
>>>> those won't be exercised with the syscon code.
>>>
>>> Is config->name always the same as map->name? If so, why don't you just
>>> pass map once and for all? Is the lifetime of map->name the same as
>>> that of config->name?
>>
>> map->name is created (kstrdup_const) from config->name if not NULL, so above
>> replacement should be exactly equivalent, map is filled in _regmap_init. But it
>> does make the regmap_debugfs_init callsites in the file look dissimilar.
>>
>>>
>>> My worry with this approach is that we start changing stuff in a rush,
>>> and this would IMHO deserve a thorough investigation of whether this
>>> change is actually safe.
>>>
>>> I'd rather take the safe approach of either keeping the memory around
>>> until we clearly understand what the implications are (and probably
>>> this should involve the regmap maintainer), or to revert this patch
>>> until we figure out the actual life cycle of the various names.
>>
>> Yeah, agreed. Let's see what Mark suggests.
>>
>> Mark,
>> Can you clarify the lifecycle expectations on the config->name and do you have
>> any suggestions here?
> 
> Have we reached a conclusion here? Can we get a fix in mainline?

Marc, we can go with your patch based on Mark's response.

regards
Suman