[PATCH 0/9] of: fix compatible-child-node lookups

[PATCH 0/9] of: fix compatible-child-node lookups

[Johan Hovold]

Several drivers currently use of_find_compatible_node() to lookup child
nodes while failing to notice that the of_find_ functions search the
entire tree depth-first and therefore can match unrelated (non-child)
nodes.

The fact that these functions also drop a reference to the node they
start searching from (e.g. the parent node) is typically also
overlooked, something which can lead to use-after-free bugs (e.g. after
probe deferrals).

This series adds a new helper, similar to of_get_child_by_name(), 
that can be used to lookup compatible child nodes, and uses the new
helper to fix child-node lookups throughout the tree.

This is related to the fixes I posted about a year ago, which addressed
a similar anti-pattern when looking up child nodes by name. Since it
took me more than a year to get all those fixes into Linus' tree (one
fix is still pending), and as these fixes depend on the new helper, I'm
suggesting that these all go in through Rob's or Greg's trees.

Alternatively, the helper could go into to -rc2, and I'll be pinging
submaintainers for coming year as well. ;)

Johan


Johan Hovold (9):
  of: add helper to lookup compatible child node
  drm/mediatek: fix OF sibling-node lookup
  drm/msm: fix OF child-node lookup
  mmc: meson-mx-sdio: fix OF child-node lookup
  mtd: nand: atmel: fix OF child-node lookup
  net: bcmgenet: fix OF child-node lookup
  net: stmmac: dwmac-sun8i: fix OF child-node lookup
  NFC: nfcmrvl_uart: fix OF child-node lookup
  power: supply: twl4030-charger: fix OF sibling-node lookup

 drivers/gpu/drm/mediatek/mtk_hdmi.c           |  5 ++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.c       |  5 ++--
 drivers/mmc/host/meson-mx-sdio.c              |  8 ++++--
 drivers/mtd/nand/raw/atmel/nand-controller.c  | 11 +++++---
 drivers/net/ethernet/broadcom/genet/bcmmii.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 12 +++++++--
 drivers/nfc/nfcmrvl/uart.c                    |  5 ++--
 drivers/of/base.c                             | 25 +++++++++++++++++++
 drivers/power/supply/twl4030_charger.c        |  5 ++--
 include/linux/of.h                            |  8 ++++++
 10 files changed, 68 insertions(+), 18 deletions(-)

-- 
2.18.0

[PATCH 1/9] of: add helper to lookup compatible child node

[Johan Hovold]

Add of_get_compatible_child() helper that can be used to lookup
compatible child nodes.

Several drivers currently use of_find_compatible_node() to lookup child
nodes while failing to notice that the of_find_ functions search the
entire tree depth-first and therefore can match unrelated (non-child)
nodes. The fact that these functions also drop a reference to the node
they start searching from (e.g. the parent node) is typically also
overlooked, something which can lead to use-after-free bugs.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/of/base.c  | 25 +++++++++++++++++++++++++
 include/linux/of.h |  8 ++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 466e3c8582f0..bc420d2aa5f5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -719,6 +719,31 @@ struct device_node *of_get_next_available_child(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_next_available_child);
 
+/**
+ * of_get_compatible_child - Find compatible child node
+ * @parent:	parent node
+ * @compatible:	compatible string
+ *
+ * Lookup child node whose compatible property contains the given compatible
+ * string.
+ *
+ * Returns a node pointer with refcount incremented, use of_node_put() on it
+ * when done; or NULL if not found.
+ */
+struct device_node *of_get_compatible_child(const struct device_node *parent,
+				const char *compatible)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(parent, child) {
+		if (of_device_is_compatible(child, compatible))
+			break;
+	}
+
+	return child;
+}
+EXPORT_SYMBOL(of_get_compatible_child);
+
 /**
  *	of_get_child_by_name - Find the child node by name for a given parent
  *	@node:	parent node
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f952d9..c22982fc1bff 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -290,6 +290,8 @@ extern struct device_node *of_get_next_child(const struct device_node *node,
 extern struct device_node *of_get_next_available_child(
 	const struct device_node *node, struct device_node *prev);
 
+extern struct device_node *of_get_compatible_child(const struct device_node *parent,
+					const char *compatible);
 extern struct device_node *of_get_child_by_name(const struct device_node *node,
 					const char *name);
 
@@ -632,6 +634,12 @@ static inline bool of_have_populated_dt(void)
 	return false;
 }
 
+static struct device_node *of_get_compatible_child(const struct device_node *parent,
+					const char *compatible)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_child_by_name(
 					const struct device_node *node,
 					const char *name)
-- 
2.18.0

[PATCH 2/9] drm/mediatek: fix OF sibling-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the sibling
instead of using of_find_compatible_node(), which searches the entire
tree and thus can return an unrelated (i.e. non-sibling) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the parent device node).

While at it, also fix the related cec-node reference leak.

Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
Cc: stable <stable@vger.kernel.org>     # 4.8
Cc: Jie Qiu <jie.qiu@mediatek.com>
Cc: Junzhi Zhao <junzhi.zhao@mediatek.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: CK Hu <ck.hu@mediatek.com>
Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 2d45d1dd9554..643f5edd68fe 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1446,8 +1446,7 @@ static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
 	}
 
 	/* The CEC module handles HDMI hotplug detection */
-	cec_np = of_find_compatible_node(np->parent, NULL,
-					 "mediatek,mt8173-cec");
+	cec_np = of_get_compatible_child(np->parent, "mediatek,mt8173-cec");
 	if (!cec_np) {
 		dev_err(dev, "Failed to find CEC node\n");
 		return -EINVAL;
@@ -1457,8 +1456,10 @@ static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
 	if (!cec_pdev) {
 		dev_err(hdmi->dev, "Waiting for CEC device %pOF\n",
 			cec_np);
+		of_node_put(cec_np);
 		return -EPROBE_DEFER;
 	}
+	of_node_put(cec_np);
 	hdmi->cec_dev = &cec_pdev->dev;
 
 	/*
-- 
2.18.0

[PATCH 3/9] drm/msm: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the legacy
pwrlevels child node instead of using of_find_compatible_node(), which
searches the entire tree and thus can return an unrelated (i.e.
non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the probed device's node).

While at it, also fix the related child-node reference leak.

Fixes: e2af8b6b0ca1 ("drm/msm: gpu: Use OPP tables if we can")
Cc: stable <stable@vger.kernel.org>     # 4.12
Cc: Jordan Crouse <jcrouse@codeaurora.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index da1363a0c54d..93d70f4a2154 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -633,8 +633,7 @@ static int adreno_get_legacy_pwrlevels(struct device *dev)
 	struct device_node *child, *node;
 	int ret;
 
-	node = of_find_compatible_node(dev->of_node, NULL,
-		"qcom,gpu-pwrlevels");
+	node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
 	if (!node) {
 		dev_err(dev, "Could not find the GPU powerlevels\n");
 		return -ENXIO;
@@ -655,6 +654,8 @@ static int adreno_get_legacy_pwrlevels(struct device *dev)
 			dev_pm_opp_add(dev, val, 0);
 	}
 
+	of_node_put(node);
+
 	return 0;
 }
 
-- 
2.18.0

[PATCH 4/9] mmc: meson-mx-sdio: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the slot child
node instead of using of_find_compatible_node(), which searches the
entire tree and thus can return an unrelated (i.e. non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the node of the device being probed).

While at it, also fix up the related slot-node reference leak.

Fixes: ed80a13bb4c4 ("mmc: meson-mx-sdio: Add a driver for the Amlogic Meson8 and Meson8b SoCs")
Cc: stable <stable@vger.kernel.org>     # 4.15
Cc: Carlo Caione <carlo@endlessm.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mmc/host/meson-mx-sdio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/meson-mx-sdio.c b/drivers/mmc/host/meson-mx-sdio.c
index 09cb89645d06..2cfec33178c1 100644
--- a/drivers/mmc/host/meson-mx-sdio.c
+++ b/drivers/mmc/host/meson-mx-sdio.c
@@ -517,19 +517,23 @@ static struct mmc_host_ops meson_mx_mmc_ops = {
 static struct platform_device *meson_mx_mmc_slot_pdev(struct device *parent)
 {
 	struct device_node *slot_node;
+	struct platform_device *pdev;
 
 	/*
 	 * TODO: the MMC core framework currently does not support
 	 * controllers with multiple slots properly. So we only register
 	 * the first slot for now
 	 */
-	slot_node = of_find_compatible_node(parent->of_node, NULL, "mmc-slot");
+	slot_node = of_get_compatible_child(parent->of_node, "mmc-slot");
 	if (!slot_node) {
 		dev_warn(parent, "no 'mmc-slot' sub-node found\n");
 		return ERR_PTR(-ENOENT);
 	}
 
-	return of_platform_device_create(slot_node, NULL, parent);
+	pdev = of_platform_device_create(slot_node, NULL, parent);
+	of_node_put(slot_node);
+
+	return pdev;
 }
 
 static int meson_mx_mmc_add_host(struct meson_mx_mmc_host *host)
-- 
2.18.0

[PATCH 5/9] mtd: nand: atmel: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the nfc child
node instead of using of_find_compatible_node(), which searches the
entire tree and thus can return an unrelated (i.e. non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the node of the device being probed).

While at it, also fix a related nfc-node reference leak.

Fixes: f88fc122cc34 ("mtd: nand: Cleanup/rework the atmel_nand driver")
Cc: stable <stable@vger.kernel.org>     # 4.11
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Josh Wu <rainyfeeling@outlook.com>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
index a068b214ebaa..d3dfe63956ac 100644
--- a/drivers/mtd/nand/raw/atmel/nand-controller.c
+++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
@@ -2061,8 +2061,7 @@ atmel_hsmc_nand_controller_legacy_init(struct atmel_hsmc_nand_controller *nc)
 	int ret;
 
 	nand_np = dev->of_node;
-	nfc_np = of_find_compatible_node(dev->of_node, NULL,
-					 "atmel,sama5d3-nfc");
+	nfc_np = of_get_compatible_child(dev->of_node, "atmel,sama5d3-nfc");
 
 	nc->clk = of_clk_get(nfc_np, 0);
 	if (IS_ERR(nc->clk)) {
@@ -2472,15 +2471,19 @@ static int atmel_nand_controller_probe(struct platform_device *pdev)
 	}
 
 	if (caps->legacy_of_bindings) {
+		struct device_node *nfc_node;
 		u32 ale_offs = 21;
 
 		/*
 		 * If we are parsing legacy DT props and the DT contains a
 		 * valid NFC node, forward the request to the sama5 logic.
 		 */
-		if (of_find_compatible_node(pdev->dev.of_node, NULL,
-					    "atmel,sama5d3-nfc"))
+		nfc_node = of_get_compatible_child(pdev->dev.of_node,
+						   "atmel,sama5d3-nfc");
+		if (nfc_node) {
 			caps = &atmel_sama5_nand_caps;
+			of_node_put(nfc_node);
+		}
 
 		/*
 		 * Even if the compatible says we are dealing with an
-- 
2.18.0

[PATCH 6/9] net: bcmgenet: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the mdio child
node instead of using of_find_compatible_node(), which searches the
entire tree and thus can return an unrelated (i.e. non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the node of the device being probed).

Fixes: aa09677cba42 ("net: bcmgenet: add MDIO routines")
Cc: stable <stable@vger.kernel.org>     # 3.15
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 5333274a283c..87fc65560ceb 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -333,7 +333,7 @@ static struct device_node *bcmgenet_mii_of_find_mdio(struct bcmgenet_priv *priv)
 	if (!compat)
 		return NULL;
 
-	priv->mdio_dn = of_find_compatible_node(dn, NULL, compat);
+	priv->mdio_dn = of_get_compatible_child(dn, compat);
 	kfree(compat);
 	if (!priv->mdio_dn) {
 		dev_err(kdev, "unable to find MDIO bus node\n");
-- 
2.18.0

[PATCH 7/9] net: stmmac: dwmac-sun8i: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the mdio-internal
child node instead of using of_find_compatible_node(), which searches
the entire tree and thus can return an unrelated (i.e. non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the mdio-mux node). Fortunately, this was inadvertently
balanced by a failure to drop the mdio-mux reference after lookup.

While at it, also fix the related mdio-internal- and phy-node reference
leaks.

Fixes: 634db83b8265 ("net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs")
Cc: Corentin Labbe <clabbe.montjoie@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index f9a61f90cfbc..0f660af01a4b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -714,8 +714,9 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
 		return -ENODEV;
 	}
 
-	mdio_internal = of_find_compatible_node(mdio_mux, NULL,
+	mdio_internal = of_get_compatible_child(mdio_mux,
 						"allwinner,sun8i-h3-mdio-internal");
+	of_node_put(mdio_mux);
 	if (!mdio_internal) {
 		dev_err(priv->device, "Cannot get internal_mdio node\n");
 		return -ENODEV;
@@ -729,13 +730,20 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
 		gmac->rst_ephy = of_reset_control_get_exclusive(iphynode, NULL);
 		if (IS_ERR(gmac->rst_ephy)) {
 			ret = PTR_ERR(gmac->rst_ephy);
-			if (ret == -EPROBE_DEFER)
+			if (ret == -EPROBE_DEFER) {
+				of_node_put(iphynode);
+				of_node_put(mdio_internal);
 				return ret;
+			}
 			continue;
 		}
 		dev_info(priv->device, "Found internal PHY node\n");
+		of_node_put(iphynode);
+		of_node_put(mdio_internal);
 		return 0;
 	}
+
+	of_node_put(mdio_internal);
 	return -ENODEV;
 }
 
-- 
2.18.0

[PATCH 8/9] NFC: nfcmrvl_uart: fix OF child-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the nfc child
node instead of using of_find_compatible_node(), which searches the
entire tree and thus can return an unrelated (i.e. non-child) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the parent node).

Fixes: e097dc624f78 ("NFC: nfcmrvl: add UART driver")
Fixes: d8e018c0b321 ("NFC: nfcmrvl: update device tree bindings for Marvell NFC")
Cc: stable <stable@vger.kernel.org>     # 4.2
Cc: Vincent Cuissard <cuissard@marvell.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/nfc/nfcmrvl/uart.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c
index 91162f8e0366..9a22056e8d9e 100644
--- a/drivers/nfc/nfcmrvl/uart.c
+++ b/drivers/nfc/nfcmrvl/uart.c
@@ -73,10 +73,9 @@ static int nfcmrvl_uart_parse_dt(struct device_node *node,
 	struct device_node *matched_node;
 	int ret;
 
-	matched_node = of_find_compatible_node(node, NULL, "marvell,nfc-uart");
+	matched_node = of_get_compatible_child(node, "marvell,nfc-uart");
 	if (!matched_node) {
-		matched_node = of_find_compatible_node(node, NULL,
-						       "mrvl,nfc-uart");
+		matched_node = of_get_compatible_child(node, "mrvl,nfc-uart");
 		if (!matched_node)
 			return -ENODEV;
 	}
-- 
2.18.0

[PATCH 9/9] power: supply: twl4030-charger: fix OF sibling-node lookup

[Johan Hovold]

Use the new of_get_compatible_child() helper to lookup the usb sibling
node instead of using of_find_compatible_node(), which searches the
entire tree and thus can return an unrelated (non-sibling) node.

This also addresses a potential use-after-free (e.g. after probe
deferral) as the tree-wide helper drops a reference to its first
argument (i.e. the parent device node).

While at it, also fix the related phy-node reference leak.

Fixes: f5e4edb8c888 ("power: twl4030_charger: find associated phy by more reliable means.")
Cc: stable <stable@vger.kernel.org>     # 4.2
Cc: NeilBrown <neilb@suse.de>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/power/supply/twl4030_charger.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index bbcaee56db9d..b6a7d9f74cf3 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -996,12 +996,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	if (bci->dev->of_node) {
 		struct device_node *phynode;
 
-		phynode = of_find_compatible_node(bci->dev->of_node->parent,
-						  NULL, "ti,twl4030-usb");
+		phynode = of_get_compatible_child(bci->dev->of_node->parent,
+						  "ti,twl4030-usb");
 		if (phynode) {
 			bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			of_node_put(phynode);
 			if (IS_ERR(bci->transceiver)) {
 				ret = PTR_ERR(bci->transceiver);
 				if (ret == -EPROBE_DEFER)
-- 
2.18.0

Re: [PATCH 0/9] of: fix compatible-child-node lookups

[Rob Herring]

On Wed, Aug 22, 2018 at 5:57 AM Johan Hovold <johan@kernel.org> wrote:
>
> Several drivers currently use of_find_compatible_node() to lookup child
> nodes while failing to notice that the of_find_ functions search the
> entire tree depth-first and therefore can match unrelated (non-child)
> nodes.

That is not quite right. It searches all nodes following 'from', so
not the entire tree unless 'from' is NULL. The purpose of 'from' is to
iterate to find all compatible nodes. But you are correct that anyone
calling of_find_compatible_node directly with from != NULL is wrong.

I'd really like to make of_find_compatible_node() function as
searching all of the sub-tree as that should be what all the callers
want (unless they've open coded for_each_compatible_node). Though
maybe 2 functions to search the whole tree and just immediate children
is best.

Also, it would be good to remove the type parameter as device_type is
deprecated (mostly). It looks like most if not all callers setting
type could drop it and just match on compatible. It seems to just
serve as additional validation of the DT.

> The fact that these functions also drop a reference to the node they
> start searching from (e.g. the parent node) is typically also
> overlooked, something which can lead to use-after-free bugs (e.g. after
> probe deferrals).
>
> This series adds a new helper, similar to of_get_child_by_name(),
> that can be used to lookup compatible child nodes, and uses the new
> helper to fix child-node lookups throughout the tree.
>
> This is related to the fixes I posted about a year ago, which addressed
> a similar anti-pattern when looking up child nodes by name. Since it
> took me more than a year to get all those fixes into Linus' tree (one
> fix is still pending), and as these fixes depend on the new helper, I'm
> suggesting that these all go in through Rob's or Greg's trees.

I'm happy to take them or apply the dependency now and then anything
not picked up by sub-maintainers for 4.20.

Rob

Re: [PATCH 3/9] drm/msm: fix OF child-node lookup

[Rob Herring]

On Wed, Aug 22, 2018 at 5:57 AM Johan Hovold <johan@kernel.org> wrote:
>
> Use the new of_get_compatible_child() helper to lookup the legacy
> pwrlevels child node instead of using of_find_compatible_node(), which
> searches the entire tree and thus can return an unrelated (i.e.
> non-child) node.
>
> This also addresses a potential use-after-free (e.g. after probe
> deferral) as the tree-wide helper drops a reference to its first
> argument (i.e. the probed device's node).
>
> While at it, also fix the related child-node reference leak.
>
> Fixes: e2af8b6b0ca1 ("drm/msm: gpu: Use OPP tables if we can")
> Cc: stable <stable@vger.kernel.org>     # 4.12
> Cc: Jordan Crouse <jcrouse@codeaurora.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index da1363a0c54d..93d70f4a2154 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -633,8 +633,7 @@ static int adreno_get_legacy_pwrlevels(struct device *dev)
>         struct device_node *child, *node;
>         int ret;
>
> -       node = of_find_compatible_node(dev->of_node, NULL,
> -               "qcom,gpu-pwrlevels");
> +       node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");

Not really your problem, but this is undocumented and a downstream
binding. There's been OPP support in addition for more than a year
now, we should just remove this code IMO. For some reason though, no
one updated the 8064 dts though.

Rob

Re: [PATCH 0/9] of: fix compatible-child-node lookups

[Johan Hovold]

On Wed, Aug 22, 2018 at 09:32:11AM -0500, Rob Herring wrote:
> On Wed, Aug 22, 2018 at 5:57 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > Several drivers currently use of_find_compatible_node() to lookup child
> > nodes while failing to notice that the of_find_ functions search the
> > entire tree depth-first and therefore can match unrelated (non-child)
> > nodes.
> 
> That is not quite right. It searches all nodes following 'from', so
> not the entire tree unless 'from' is NULL. The purpose of 'from' is to
> iterate to find all compatible nodes. But you are correct that anyone
> calling of_find_compatible_node directly with from != NULL is wrong.

Yeah, sorry, I guess I could have been more specific. I just find
qualifying "searching the entire tree" with "starting from the node
specified in the first argument" to be too cumbersome to write. And it's
sort of implicit as the functions *are* iterators meant for searching
the entire tree (passing NULL as the first argument the first time).

> > This is related to the fixes I posted about a year ago, which addressed
> > a similar anti-pattern when looking up child nodes by name. Since it
> > took me more than a year to get all those fixes into Linus' tree (one
> > fix is still pending), and as these fixes depend on the new helper, I'm
> > suggesting that these all go in through Rob's or Greg's trees.
> 
> I'm happy to take them or apply the dependency now and then anything
> not picked up by sub-maintainers for 4.20.

Thanks, let's see if what the others prefer.

Johan

Re: [PATCH 9/9] power: supply: twl4030-charger: fix OF sibling-node lookup

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi,

On Wed, Aug 22, 2018 at 12:55:47PM +0200, Johan Hovold wrote:
> Use the new of_get_compatible_child() helper to lookup the usb sibling
> node instead of using of_find_compatible_node(), which searches the
> entire tree and thus can return an unrelated (non-sibling) node.
> 
> This also addresses a potential use-after-free (e.g. after probe
> deferral) as the tree-wide helper drops a reference to its first
> argument (i.e. the parent device node).
> 
> While at it, also fix the related phy-node reference leak.
> 
> Fixes: f5e4edb8c888 ("power: twl4030_charger: find associated phy by more reliable means.")
> Cc: stable <stable@vger.kernel.org>     # 4.2
> Cc: NeilBrown <neilb@suse.de>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---

Reviewed-by: Sebastian Reichel <sre@kernel.org>

-- Sebastian

>  drivers/power/supply/twl4030_charger.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index bbcaee56db9d..b6a7d9f74cf3 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -996,12 +996,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  	if (bci->dev->of_node) {
>  		struct device_node *phynode;
>  
> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> -						  NULL, "ti,twl4030-usb");
> +		phynode = of_get_compatible_child(bci->dev->of_node->parent,
> +						  "ti,twl4030-usb");
>  		if (phynode) {
>  			bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
>  			bci->transceiver = devm_usb_get_phy_by_node(
>  				bci->dev, phynode, &bci->usb_nb);
> +			of_node_put(phynode);
>  			if (IS_ERR(bci->transceiver)) {
>  				ret = PTR_ERR(bci->transceiver);
>  				if (ret == -EPROBE_DEFER)
> -- 
> 2.18.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/9] of: add helper to lookup compatible child node

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

Hi Johan,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.18 next-20180822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/of-fix-compatible-child-node-lookups/20180823-074211
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: mips-decstation_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   In file included from include/linux/irqdomain.h:35:0,
                    from arch/mips/include/asm/irq.h:14,
                    from include/linux/irq.h:23,
                    from include/asm-generic/hardirq.h:13,
                    from arch/mips/include/asm/hardirq.h:16,
                    from include/linux/hardirq.h:9,
                    from include/linux/interrupt.h:11,
                    from arch/mips/dec/ecc-berr.c:16:
>> include/linux/of.h:637:28: error: 'of_get_compatible_child' defined but not used [-Werror=unused-function]
    static struct device_node *of_get_compatible_child(const struct device_node *parent,
                               ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors

vim +/of_get_compatible_child +637 include/linux/of.h

   636	
 > 637	static struct device_node *of_get_compatible_child(const struct device_node *parent,
   638						const char *compatible)
   639	{
   640		return NULL;
   641	}
   642	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9832 bytes --]

Re: [PATCH 1/9] of: add helper to lookup compatible child node

[Johan Hovold]

On Thu, Aug 23, 2018 at 11:17:26AM +0800, kbuild test robot wrote:
> Hi Johan,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on v4.18 next-20180822]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/of-fix-compatible-child-node-lookups/20180823-074211
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: mips-decstation_defconfig (attached as .config)
> compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=mips 
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/irqdomain.h:35:0,
>                     from arch/mips/include/asm/irq.h:14,
>                     from include/linux/irq.h:23,
>                     from include/asm-generic/hardirq.h:13,
>                     from arch/mips/include/asm/hardirq.h:16,
>                     from include/linux/hardirq.h:9,
>                     from include/linux/interrupt.h:11,
>                     from arch/mips/dec/ecc-berr.c:16:
> >> include/linux/of.h:637:28: error: 'of_get_compatible_child' defined but not used [-Werror=unused-function]
>     static struct device_node *of_get_compatible_child(const struct device_node *parent,
>                                ^~~~~~~~~~~~~~~~~~~~~~~
>    cc1: all warnings being treated as errors

Bah, I forgot the inline keyword. I'll fix this in a v2, and I can amend
the commit message and mention the start node while at it.

Johan

Re: [PATCH 9/9] power: supply: twl4030-charger: fix OF sibling-node lookup

[Johan Hovold]

On Wed, Aug 22, 2018 at 11:36:58PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Aug 22, 2018 at 12:55:47PM +0200, Johan Hovold wrote:
> > Use the new of_get_compatible_child() helper to lookup the usb sibling
> > node instead of using of_find_compatible_node(), which searches the
> > entire tree and thus can return an unrelated (non-sibling) node.
> > 
> > This also addresses a potential use-after-free (e.g. after probe
> > deferral) as the tree-wide helper drops a reference to its first
> > argument (i.e. the parent device node).
> > 
> > While at it, also fix the related phy-node reference leak.
> > 
> > Fixes: f5e4edb8c888 ("power: twl4030_charger: find associated phy by more reliable means.")
> > Cc: stable <stable@vger.kernel.org>     # 4.2
> > Cc: NeilBrown <neilb@suse.de>
> > Cc: Felipe Balbi <balbi@ti.com>
> > Cc: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> 
> Reviewed-by: Sebastian Reichel <sre@kernel.org>

Thanks for reviewing.

Johan

Re: [PATCH 4/9] mmc: meson-mx-sdio: fix OF child-node lookup

[Martin Blumenstingl]

Hi Johan,

On Wed, Aug 22, 2018 at 12:57 PM Johan Hovold <johan@kernel.org> wrote:
>
> Use the new of_get_compatible_child() helper to lookup the slot child
> node instead of using of_find_compatible_node(), which searches the
> entire tree and thus can return an unrelated (i.e. non-child) node.
that new helper is much appreciated, thank you!

> This also addresses a potential use-after-free (e.g. after probe
> deferral) as the tree-wide helper drops a reference to its first
> argument (i.e. the node of the device being probed).
>
> While at it, also fix up the related slot-node reference leak.
not sure why both issues went unnoticed so far - good that they're now
both fixed. thank you!

> Fixes: ed80a13bb4c4 ("mmc: meson-mx-sdio: Add a driver for the Amlogic Meson8 and Meson8b SoCs")
> Cc: stable <stable@vger.kernel.org>     # 4.15
backporting only works if the patch introducing
of_get_compatible_child is also backported
do we have to inform Greg somehow?

> Cc: Carlo Caione <carlo@endlessm.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Regards
Martin

Re: [PATCH 4/9] mmc: meson-mx-sdio: fix OF child-node lookup

[Johan Hovold]

On Thu, Aug 23, 2018 at 08:50:23PM +0200, Martin Blumenstingl wrote:
> On Wed, Aug 22, 2018 at 12:57 PM Johan Hovold <johan@kernel.org> wrote:

> > Fixes: ed80a13bb4c4 ("mmc: meson-mx-sdio: Add a driver for the Amlogic Meson8 and Meson8b SoCs")
> > Cc: stable <stable@vger.kernel.org>     # 4.15
>
> backporting only works if the patch introducing
> of_get_compatible_child is also backported
> do we have to inform Greg somehow?

Greg is on CC, but he also sends out a notice in case a stable patch
fails to apply or compile.

Note that if the helper had gone in before the rest of the series, I
could have referenced it in the stable tags using its commit id in order
to facilitate the process:

	Cc: stable <stable@vger.kernel.org>     # 4.15: <commit id>

> > Cc: Carlo Caione <carlo@endlessm.com>
> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Thanks for the ack.

Johan