Hi Marc, On 8/27/20 9:46 AM, Marc Zyngier wrote: > Hi all, > > On 2020-07-27 22:10, Suman Anna wrote: >> The DT node full name is currently being used in regmap_config >> which in turn is used to create the regmap debugfs directories. >> This name however is not guaranteed to be unique and the regmap >> debugfs registration can fail in the cases where the syscon nodes >> have the same unit-address but are present in different DT node >> hierarchies. Replace this logic using the syscon reg resource >> address instead (inspired from logic used while creating platform >> devices) to ensure a unique name is given for each syscon. >> >> Signed-off-by: Suman Anna <s-anna@xxxxxx> >> --- >> Hi Arnd, >> Lee is looking for your review on this patch. Can you please >> review and provide your comments. >> >> This is a resend of the patch that was posted previously, rebased >> now onto latest kernel. >> >> v2: https://patchwork.kernel.org/patch/11353355/ >> - Fix build warning reported by kbuild test bot >> v1: https://patchwork.kernel.org/patch/11346363/ >> >> drivers/mfd/syscon.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c >> index 3a97816d0cba..75859e492984 100644 >> --- a/drivers/mfd/syscon.c >> +++ b/drivers/mfd/syscon.c >> @@ -101,12 +101,14 @@ static struct syscon *of_syscon_register(struct >> device_node *np, bool check_clk) >> } >> } >> >> - syscon_config.name = of_node_full_name(np); >> + syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np, >> + (u64)res.start); >> syscon_config.reg_stride = reg_io_width; >> syscon_config.val_bits = reg_io_width * 8; >> syscon_config.max_register = resource_size(&res) - reg_io_width; >> >> regmap = regmap_init_mmio(NULL, base, &syscon_config); >> + kfree(syscon_config.name); >> if (IS_ERR(regmap)) { >> pr_err("regmap init failed\n"); >> ret = PTR_ERR(regmap); > > 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@xxxxxxxxxx> > 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. regards Suman > > Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with regmap_config") > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> > --- > drivers/mfd/syscon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index 75859e492984..7a660411c562 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -108,7 +108,6 @@ static struct syscon *of_syscon_register(struct device_node > *np, bool check_clk) > syscon_config.max_register = resource_size(&res) - reg_io_width; > > regmap = regmap_init_mmio(NULL, base, &syscon_config); > - kfree(syscon_config.name); > if (IS_ERR(regmap)) { > pr_err("regmap init failed\n"); > ret = PTR_ERR(regmap); > @@ -145,6 +144,7 @@ static struct syscon *of_syscon_register(struct device_node > *np, bool check_clk) > regmap_exit(regmap); > err_regmap: > iounmap(base); > + kfree(syscon_config.name); > err_map: > kfree(syscon); > return ERR_PTR(ret);
