Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 16 Feb 2021 08:42:10 +0000
Message-ID: <87pn10pfod.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        kernel-team@android.com, Lee Jones <lee.jones@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>, Suman Anna <s-anna@ti.com>
Subject: Re: [PATCH] mfd: syscon: Don't free allocated name for regmap_config
In-Reply-To: <d35d3960-c2a6-491d-fd1a-cf3aa95ce109@ideasonboard.com>
References: <20200903160237.932818-1-maz@kernel.org>
        <d35d3960-c2a6-491d-fd1a-cf3aa95ce109@ideasonboard.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Tomi,

On Tue, 16 Feb 2021 06:34:50 +0000,
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> 
> Hi Marc,
> 
> On 03/09/2020 19:02, Marc Zyngier wrote:
> > 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);
> > 
> 
> This patch causes lots of kmemleak reports, for example:

It may trigger some kmemleak reports, but at the time it was written,
this patch avoided some very nasty memory corruption with debugfs. The
problem is that the memory management responsibilities between syscon,
regmap and debugfs are rather obscure.

If you can come up with an accurate description of the life cycle of
syscon_config.name across these 3 subsystems, we may be able to fix it
for good.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.