[PATCH 0/2] pinctrl: fix group/function table leaks on consumer re-probe

[PATCH 0/2] pinctrl: fix group/function table leaks on consumer re-probe

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Pinctrl drivers that create their groups/functions lazily from the
consumer's device-tree node in .dt_node_to_map() leak the per-mapping
tables when the same node is mapped again. The core deduplicates
groups/functions by name and keeps the first instance for the controller's
lifetime, so it silently drops the copies built on every subsequent
mapping - e.g. on each re-probe of the consumer device. kmemleak does not
flag them because they stay reachable from the controller's devres list.

Reproducer (patch 1, pinctrl-single) - any board where a device referencing
a pinctrl-single state is repeatedly re-probed; here an I2C TPM whose node
has a "default" pinctrl state:

  while modprobe tpm_tis_i2c; do rmmod tpm_tis_i2c; done

The tables land in a generic kmalloc-N cache (kmalloc-192 on arm64, where
ARCH_KMALLOC_MINALIGN pads every devres allocation; the bucket differs by
config), so watch it cache-independently by diffing /proc/slabinfo after
dropping the reclaimable slab:

  sync; echo 3 > /proc/sys/vm/drop_caches
  cat /proc/slabinfo | awk 'NR>2{print $1,$2}' | sort > /tmp/a
  <run N cycles>
  sync; echo 3 > /proc/sys/vm/drop_caches
  cat /proc/slabinfo | awk 'NR>2{print $1,$2}' | sort > /tmp/b
  join /tmp/a /tmp/b | awk '$3>$2{print $1,$3-$2}'

Patch 1 (pinctrl-single) was found and fixed with the above test on real
hardware. Patch 2 (ti-iodelay) has the same bug pattern - plus a missing
.dt_free_map that leaks the mapping table as well - but was found by code
review only and is compile-tested only; I have no such hardware to
reproduce it.

Alexander Sverdlin (2):
  pinctrl: single: don't leak group/function tables on consumer re-probe
  pinctrl: ti-iodelay: fix memory leaks on consumer re-probe

 drivers/pinctrl/pinctrl-single.c        | 21 +++++++++++++++++++++
 drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 24 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

-- 
2.55.0

[PATCH 1/2] pinctrl: single: don't leak group/function tables on consumer re-probe

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

For every mapping pcs_parse_{one,bits}_in_pinctrl_entry() builds a
pcs_function, a value table, a pin array and a pingroup-name array and
registers them via pcs_add_function()/pinctrl_generic_add_group(). The
core deduplicates groups and functions by name and keeps the first
instance for the controller's lifetime, so the tables built for any later
mapping of the same node - e.g. each consumer re-probe - are dropped by
the core and leaked.

Free the redundant copies once the function turns out to be a duplicate;
the group is deduplicated together with it. Don't remove the deduplicated
instance from the core: its selector is a plain counter (num_groups /
num_functions), so removing any but the last entry corrupts the selector
space.

kmemleak stays silent because the leaked tables remain reachable from the
controller's devres list.

Fixes: 8b8b091bf07f ("pinctrl: Add one-register-per-pin type device tree based pinctrl driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/pinctrl/pinctrl-single.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index e0eb7240a9859..e75e32723100d 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1097,6 +1097,19 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	} else {
 		*num_maps = 1;
 	}
+
+	/*
+	 * The core deduplicates groups/functions by name and keeps the first
+	 * for the controller's lifetime; drop our now-redundant copies when the
+	 * node is mapped again (consumer re-probe). The group is deduplicated
+	 * together with the function, so testing the latter is enough.
+	 */
+	if (pinmux_generic_get_function(pcs->pctl, fsel)->data != function) {
+		devm_kfree(pcs->dev, pins);
+		devm_kfree(pcs->dev, vals);
+		devm_kfree(pcs->dev, function);
+		devm_kfree(pcs->dev, pgnames);
+	}
 	mutex_unlock(&pcs->mutex);
 
 	return 0;
@@ -1235,6 +1248,14 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 	(*map)->data.mux.function = np->name;
 
 	*num_maps = 1;
+
+	/* See pcs_parse_one_pinctrl_entry() for the deduplication rationale. */
+	if (pinmux_generic_get_function(pcs->pctl, fsel)->data != function) {
+		devm_kfree(pcs->dev, pins);
+		devm_kfree(pcs->dev, vals);
+		devm_kfree(pcs->dev, function);
+		devm_kfree(pcs->dev, pgnames);
+	}
 	mutex_unlock(&pcs->mutex);
 
 	return 0;
-- 
2.55.0

[PATCH 2/2] pinctrl: ti-iodelay: fix memory leaks on consumer re-probe

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

ti_iodelay_dt_node_to_map() allocates a pinctrl_map, a group, a pin array
and a config array per mapping with devres on the controller, and registers
the group via pinctrl_generic_add_group(). The core deduplicates groups by
name and keeps the first for the controller's lifetime, so on a repeated
mapping of the same node (consumer re-probe) the freshly built group, pins
and configs are dropped and leaked. The pinctrl_map leaked too: the driver
never implemented pinctrl_ops::dt_free_map, so nothing released it.

Reuse the deduplicated group and free the redundant copy (repointing the
map at the retained group), and add dt_free_map() to release the map.

Fixes: 003910ebc83b ("pinctrl: Introduce TI IOdelay configuration driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
index 019b302db2b00..9a8865ce0bbab 100644
--- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
+++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
@@ -497,6 +497,7 @@ static int ti_iodelay_dt_node_to_map(struct pinctrl_dev *pctldev,
 	struct ti_iodelay_device *iod;
 	struct ti_iodelay_cfg *cfg;
 	struct ti_iodelay_pingroup *g;
+	struct group_desc *grp;
 	const char *name = "pinctrl-pin-array";
 	int rows, *pins, error = -EINVAL, i;
 
@@ -553,6 +554,19 @@ static int ti_iodelay_dt_node_to_map(struct pinctrl_dev *pctldev,
 	if (error < 0)
 		goto free_data;
 
+	/*
+	 * The core deduplicates groups by name and keeps the first for the
+	 * controller's lifetime; on a repeated mapping (consumer re-probe)
+	 * reuse it and free our now-redundant copy.
+	 */
+	grp = pinctrl_generic_get_group(iod->pctl, error);
+	if (grp->data != g) {
+		devm_kfree(iod->dev, cfg);
+		devm_kfree(iod->dev, pins);
+		devm_kfree(iod->dev, g);
+		g = grp->data;
+	}
+
 	(*map)->type = PIN_MAP_TYPE_CONFIGS_GROUP;
 	(*map)->data.configs.group_or_pin = np->name;
 	(*map)->data.configs.configs = &g->config;
@@ -722,6 +736,15 @@ static void ti_iodelay_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 }
 #endif
 
+static void ti_iodelay_dt_free_map(struct pinctrl_dev *pctldev,
+				   struct pinctrl_map *map, unsigned int num_maps)
+{
+	struct ti_iodelay_device *iod;
+
+	iod = pinctrl_dev_get_drvdata(pctldev);
+	devm_kfree(iod->dev, map);
+}
+
 static const struct pinctrl_ops ti_iodelay_pinctrl_ops = {
 	.get_groups_count = pinctrl_generic_get_group_count,
 	.get_group_name = pinctrl_generic_get_group_name,
@@ -730,6 +753,7 @@ static const struct pinctrl_ops ti_iodelay_pinctrl_ops = {
 	.pin_dbg_show = ti_iodelay_pin_dbg_show,
 #endif
 	.dt_node_to_map = ti_iodelay_dt_node_to_map,
+	.dt_free_map = ti_iodelay_dt_free_map,
 };
 
 static const struct pinconf_ops ti_iodelay_pinctrl_pinconf_ops = {
-- 
2.55.0