* [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config @ 2020-07-27 21:10 Suman Anna 2020-07-28 7:44 ` Arnd Bergmann ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Suman Anna @ 2020-07-27 21:10 UTC (permalink / raw) To: Lee Jones, Arnd Bergmann Cc: Grzegorz Jaszczyk, David Lechner, Tony Lindgren, Roger Quadros, linux-kernel, linux-arm-kernel, linux-omap, Suman Anna 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@ti.com> --- 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); -- 2.26.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-07-27 21:10 [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna @ 2020-07-28 7:44 ` Arnd Bergmann 2020-07-28 15:34 ` Suman Anna 2020-07-28 8:01 ` Lee Jones 2020-08-27 14:46 ` Marc Zyngier 2 siblings, 1 reply; 12+ messages in thread From: Arnd Bergmann @ 2020-07-28 7:44 UTC (permalink / raw) To: Suman Anna Cc: Lee Jones, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, Roger Quadros, linux-kernel, Linux ARM, linux-omap On Mon, Jul 27, 2020 at 11:10 PM Suman Anna <s-anna@ti.com> 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@ti.com> > --- > Hi Arnd, > Lee is looking for your review on this patch. Can you please > review and provide your comments. Sorry for missing this earlier. I think this makes sense, and I don't expect the name change to cause problems, so Reviewed-by: Arnd Bergmann <arnd@arndb.de> > --- 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); Note that you could avoid the cast by using "%pOFn@%pa", and passing res.start by reference. Not important though, the result should be similar, and you might not like the '0x' that this adds. Arnd ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-07-28 7:44 ` Arnd Bergmann @ 2020-07-28 15:34 ` Suman Anna 0 siblings, 0 replies; 12+ messages in thread From: Suman Anna @ 2020-07-28 15:34 UTC (permalink / raw) To: Arnd Bergmann Cc: Lee Jones, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, Roger Quadros, linux-kernel, Linux ARM, linux-omap On 7/28/20 2:44 AM, Arnd Bergmann wrote: > On Mon, Jul 27, 2020 at 11:10 PM Suman Anna <s-anna@ti.com> 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@ti.com> >> --- >> Hi Arnd, >> Lee is looking for your review on this patch. Can you please >> review and provide your comments. > > Sorry for missing this earlier. I think this makes sense, and I don't > expect the name change to cause problems, so > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> Thanks Arnd. > >> --- 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); > > Note that you could avoid the cast by using "%pOFn@%pa", and > passing res.start by reference. Not important though, the result should > be similar, and you might not like the '0x' that this adds. Yeah, preference was not to add the leading 0x or any leading 0s. We did discuss about this on the original v2 submission [1]. regards Suman [1] https://patchwork.kernel.org/comment/23129393/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-07-27 21:10 [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna 2020-07-28 7:44 ` Arnd Bergmann @ 2020-07-28 8:01 ` Lee Jones 2020-08-27 14:46 ` Marc Zyngier 2 siblings, 0 replies; 12+ messages in thread From: Lee Jones @ 2020-07-28 8:01 UTC (permalink / raw) To: Suman Anna Cc: Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, Roger Quadros, linux-kernel, linux-arm-kernel, linux-omap On Mon, 27 Jul 2020, 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@ti.com> > --- > 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(-) Applied, thanks. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-07-27 21:10 [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna 2020-07-28 7:44 ` Arnd Bergmann 2020-07-28 8:01 ` Lee Jones @ 2020-08-27 14:46 ` Marc Zyngier 2020-08-27 18:28 ` Suman Anna 2 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2020-08-27 14:46 UTC (permalink / raw) To: Suman Anna Cc: Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team 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@ti.com> > --- > 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. 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. Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with regmap_config") Signed-off-by: Marc Zyngier <maz@kernel.org> --- 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); -- 2.27.0 -- Jazz is not dead. It just smells funny... ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-08-27 14:46 ` Marc Zyngier @ 2020-08-27 18:28 ` Suman Anna 2020-08-27 20:06 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Suman Anna @ 2020-08-27 18:28 UTC (permalink / raw) To: Marc Zyngier Cc: Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team 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@ti.com> >> --- >> 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@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. regards Suman > > Fixes: e15d7f2b81d2 ("mfd: syscon: Use a unique name with regmap_config") > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > 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); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-08-27 18:28 ` Suman Anna @ 2020-08-27 20:06 ` Marc Zyngier 2020-08-27 20:32 ` Suman Anna 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2020-08-27 20:06 UTC (permalink / raw) To: Suman Anna Cc: Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team Hi Suman, On 2020-08-27 19:28, Suman Anna wrote: > 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@ti.com> >>> --- >>> 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@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? 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. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-08-27 20:06 ` Marc Zyngier @ 2020-08-27 20:32 ` Suman Anna 2020-08-28 10:40 ` Mark Brown 2020-09-03 8:26 ` Marc Zyngier 0 siblings, 2 replies; 12+ messages in thread From: Suman Anna @ 2020-08-27 20:32 UTC (permalink / raw) To: Marc Zyngier, Mark Brown Cc: Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team 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: >>> 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@ti.com> >>>> --- >>>> 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@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? regards Suman ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-08-27 20:32 ` Suman Anna @ 2020-08-28 10:40 ` Mark Brown 2020-09-03 8:26 ` Marc Zyngier 1 sibling, 0 replies; 12+ messages in thread From: Mark Brown @ 2020-08-28 10:40 UTC (permalink / raw) To: Suman Anna Cc: Marc Zyngier, Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team [-- Attachment #1: Type: text/plain, Size: 306 bytes --] On Thu, Aug 27, 2020 at 03:32:05PM -0500, Suman Anna wrote: > Can you clarify the lifecycle expectations on the config->name and do you have > any suggestions here? The regmap name is expected to be managed by the caller and should be live as long as the regmap is live, it is almost always static data. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-08-27 20:32 ` Suman Anna 2020-08-28 10:40 ` Mark Brown @ 2020-09-03 8:26 ` Marc Zyngier 2020-09-03 13:25 ` Suman Anna 1 sibling, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2020-09-03 8:26 UTC (permalink / raw) To: Suman Anna Cc: Mark Brown, Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team 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? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-09-03 8:26 ` Marc Zyngier @ 2020-09-03 13:25 ` Suman Anna 2020-09-03 16:04 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Suman Anna @ 2020-09-03 13:25 UTC (permalink / raw) To: Marc Zyngier Cc: Mark Brown, Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config 2020-09-03 13:25 ` Suman Anna @ 2020-09-03 16:04 ` Marc Zyngier 0 siblings, 0 replies; 12+ messages in thread From: Marc Zyngier @ 2020-09-03 16:04 UTC (permalink / raw) To: Suman Anna Cc: Mark Brown, Lee Jones, Arnd Bergmann, Grzegorz Jaszczyk, David Lechner, Tony Lindgren, linux-kernel, linux-omap, linux-arm-kernel, Roger Quadros, kernel-team On 2020-09-03 14:25, Suman Anna wrote: > On 9/3/20 3:26 AM, Marc Zyngier wrote: [...] >> 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. Patch resent[1]. M. [1] https://lore.kernel.org/r/20200903160237.932818-1-maz@kernel.org -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-03 16:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-27 21:10 [RESEND PATCH v2] mfd: syscon: Use a unique name with regmap_config Suman Anna 2020-07-28 7:44 ` Arnd Bergmann 2020-07-28 15:34 ` Suman Anna 2020-07-28 8:01 ` Lee Jones 2020-08-27 14:46 ` Marc Zyngier 2020-08-27 18:28 ` Suman Anna 2020-08-27 20:06 ` Marc Zyngier 2020-08-27 20:32 ` Suman Anna 2020-08-28 10:40 ` Mark Brown 2020-09-03 8:26 ` Marc Zyngier 2020-09-03 13:25 ` Suman Anna 2020-09-03 16:04 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).