linux-kernel - Re: Duplicate calls to regmap_debugfs_init() through regmap_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <75670688fbec759e8ab8b42356a16cca465dc430.camel@ew.tq-group.com>
Date:   Fri, 23 Jul 2021 09:34:26 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Lee Jones <lee.jones@...aro.org>, Arnd Bergmann <arnd@...db.de>
Cc:     linux-kernel@...r.kernel.org, Dong Aisheng <aisheng.dong@....com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: Duplicate calls to regmap_debugfs_init() through
 regmap_attach_dev()

On Mon, 2021-07-19 at 15:53 +0200, Matthias Schiffer wrote:
> Hi everyone,
> 
> I hope I got the right list of maintainers for this issue, which seems
> to be rooted in the interaction between regmap, syscon and pinctrl-imx.
> 
> With recent kernels (observed on v5.10.y, but the code doesn't look
> significantly different on master/next) I've seen the following message
> on boot on i.MX6UL SoCs:
> 
> > debugfs: Directory 'dummy-iomuxc-gpr@...4000' with parent 'regmap' already present!
> 
> I've tracked this down to this piece of code in the pinctrl-imx driver:
> 
> > 		gpr = syscon_regmap_lookup_by_compatible(info->gpr_compatible);
> > 		if (!IS_ERR(gpr))
> > 			regmap_attach_dev(&pdev->dev, gpr, &config);
> 
> __regmap_init() (called by syscon_regmap_lookup_by_compatible()) has:
> 
> > 	if (dev) {
> > 		ret = regmap_attach_dev(dev, map, config);
> > 		if (ret != 0)
> > 			goto err_regcache;
> > 	} else {
> > 		regmap_debugfs_init(map);
> > 	}
> 
> As dev is NULL in this call, regmap_debugfs_init() will be called.
> 
> pinctrl-imx then calls regmap_attach_dev(), which calls
> regmap_debugfs_init() again. Unless I'm missing something, this is very
> problematic: regmap_debugfs_init() does a lot more than just adding
> debugfs files - it also initializes list heads and mutices in the
> regmap structure.
> 
> It seems to me that there is no correct way to use regmap_attach_dev()
> from outside of __regmap_init(). In particular on a syscon regmap that
> may be shared between different drivers, setting map->dev looks wrong
> to me.
> 
> The total number of drivers that call regmap_attach_dev() is very low
> (I count 5), but all of them use it on a syscon regmap. Some of them
> perform further operations on the regmap as if they owned it, like
> modifying the cache configuration.
> 
> While not directly related, could anyone tell me why the locking around
> syscon_list in the syscon driver is correct (or if it is in fact
> incorrect)? It looks to me like two tasks might call
> device_node_get_regmap() at the same time, leading to two concurrent
> constructions of the same syscon regmap.
> 
> Kind regards,
> Matthias


Another question regarding the syscon driver: Does the syscon platform
device still have any use after bdb0066df96e ("mfd: syscon: Decouple
syscon interface from platform devices")? All exported syscon functions
use the regmaps stored in the global "syscon_list", which are
compeletely independent of the devices handled by syscon_probe().

As the syscon platform_driver doesn't do anything, it seems to me like
that part could just be removed, leaving only the handling of shared
regmaps. Maybe that code should live under drivers/base/regmap instead
of drivers/mfd?