[PATCH v6 0/3] misc: fastrpc: fix ADSP duplicate session creation

[PATCH v6 0/3] misc: fastrpc: fix ADSP duplicate session creation

[Vinayak Katoch]

For ADSP, only a limited number of FastRPC context banks (CBs) are
available. Each CB supports a single session, which means only a few
processes can run on ADSP simultaneously. If all sessions are consumed
by fastrpc daemons, no session remains available when a user application
starts, causing the application to fail.

To work around this, some DT set:
  qcom,nsessions = <5>;
which duplicated sessions inline during context bank initialisation.
Upstream feedback indicated that this policy does not belong in DT and
should be handled at the driver level instead.

This series iterates over CB child nodes directly and synchronously,
replacing of_platform_populate() and of_platform_depopulate(), and moves
the ADSP session duplication to the driver. The qcom,nsessions binding
is deprecated in the same series.

Signed-off-by: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>
---
Changes in v6:
- Drop unused dev_set_drvdata() from fastrpc_cb_init()
- Use device_for_each_child_reverse() to destroy CB devices
- Return -ENODEV when of_platform_device_create() fails
- Add a comment for FASTRPC_DUP_SESSIONS
- Link to v5: https://lore.kernel.org/r/20260923-dup-sessions-v5-0-e953133a1827@oss.qualcomm.com

Changes in v5:
- Introduce fastrpc_cb_devices_create() and fastrpc_cb_devices_destroy()
  for symmetric CB device lifecycle management in probe and remove.
- Replace module_init/exit pair with module_rpmsg_driver().
- Link to v4: https://lore.kernel.org/r/20260826-dup-sessions-v4-0-35555d2bfed4@oss.qualcomm.com

Changes in v4:
- Rebased on latest linux-next (a8406e6c0b79, 2026-08-25)
- Link to v3: https://lore.kernel.org/r/20260824-dup-sessions-v3-0-4b019def4e0b@oss.qualcomm.com

Changes in v3:
- Add patch to fix async probe race with of_platform_populate().
- Rename fastrpc_cb_probe() to fastrpc_cb_init().
- Reorder series: sync fix, nsessions, binding.
- Collect Reviewed-by tags.
- Link to v2: https://lore.kernel.org/r/20260708-dup-sessions-v2-0-da40f9c98a2b@oss.qualcomm.com

Changes in v2:
- Added patch to deprecate the qcom,nsessions dt-binding.
- Kept the logic unchanged; only split into two patches.
- Link to v1: https://lore.kernel.org/r/20260609-dup-sessions-v1-1-26934abb9fa3@oss.qualcomm.com

---
Vinayak Katoch (3):
      misc: fastrpc: iterate CB nodes manually instead of of_platform_populate
      misc: fastrpc: move ADSP duplicate session creation to the driver
      dt-bindings: misc: qcom,fastrpc: deprecate qcom,nsessions

 .../devicetree/bindings/misc/qcom,fastrpc.yaml     |   2 +
 drivers/misc/fastrpc.c                             | 128 ++++++++++-----------
 2 files changed, 64 insertions(+), 66 deletions(-)
---
base-commit: 8f7df005a7564bd7ffa75da9c3085d5b996f1e16
change-id: 20260609-dup-sessions-ea2acaac1994

Best regards,
-- 
Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

[PATCH v6 1/3] misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

[Vinayak Katoch]

of_platform_populate() only guarantees that child devices are registered,
not that their probes have completed before it returns. This creates a
window where fastrpc_cb_init() may not have run for all context bank
nodes, leaving the channel context partially initialised.

Introduce fastrpc_cb_devices_create() to iterate over child DT nodes
directly and call fastrpc_cb_init() synchronously for each
qcom,fastrpc-compute-cb node. This ensures all context banks are fully
initialised before fastrpc_rpmsg_probe() returns.

Introduce fastrpc_cb_devices_destroy() as the symmetric counterpart.
Before destroying the CB platform devices, invalidate all sessions under
the channel lock so that any fastrpc_user still holding a reference to
the channel context cannot acquire a new session backed by a destroyed
device.

Since fastrpc_cb_driver is no longer needed as an independent platform
driver, remove it along with its match table and remove callback. Use
module_rpmsg_driver() now that only a single driver registration remains.

Signed-off-by: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 95 ++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 54 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index af18ff1992ee..caa479f304c1 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2333,7 +2333,7 @@ static const struct file_operations fastrpc_fops = {
 	.compat_ioctl = fastrpc_device_ioctl,
 };
 
-static int fastrpc_cb_probe(struct platform_device *pdev)
+static int fastrpc_cb_init(struct platform_device *pdev)
 {
 	struct fastrpc_channel_ctx *cctx;
 	struct fastrpc_session_ctx *sess;
@@ -2355,7 +2355,7 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 	spin_lock_irqsave(&cctx->lock, flags);
 	if (cctx->sesscount >= FASTRPC_MAX_SESSIONS) {
 		spin_unlock_irqrestore(&cctx->lock, flags);
-		dev_err(&pdev->dev, "too many sessions\n");
+		dev_err(dev, "too many sessions\n");
 		return -ENOSPC;
 	}
 	dma_bits = cctx->soc_data->dma_addr_bits_default;
@@ -2363,7 +2363,6 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 	sess->used = false;
 	sess->valid = true;
 	sess->dev = dev;
-	dev_set_drvdata(dev, sess);
 	sess->sid = sid;
 
 	if (cctx->domain_id == CDSP_DOMAIN_ID)
@@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void fastrpc_cb_remove(struct platform_device *pdev)
+static void fastrpc_cb_devices_destroy(struct rpmsg_device *rpdev)
 {
-	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(pdev->dev.parent);
-	struct fastrpc_session_ctx *sess = dev_get_drvdata(&pdev->dev);
+	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
+	struct device *rdev = &rpdev->dev;
 	unsigned long flags;
 	int i;
 
 	spin_lock_irqsave(&cctx->lock, flags);
-	for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
-		if (cctx->session[i].sid == sess->sid) {
-			cctx->session[i].valid = false;
-			cctx->sesscount--;
-		}
-	}
+	for (i = 0; i < cctx->sesscount; i++)
+		cctx->session[i].valid = false;
 	spin_unlock_irqrestore(&cctx->lock, flags);
+
+	device_for_each_child_reverse(rdev, NULL, of_platform_device_destroy);
 }
 
-static const struct of_device_id fastrpc_match_table[] = {
-	{ .compatible = "qcom,fastrpc-compute-cb" },
-	{ }
-};
+static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev)
+{
+	struct device *rdev = &rpdev->dev;
+	struct platform_device *pdev;
+	struct device_node *np;
+	int err;
 
-static struct platform_driver fastrpc_cb_driver = {
-	.probe = fastrpc_cb_probe,
-	.remove = fastrpc_cb_remove,
-	.driver = {
-		.name = "qcom,fastrpc-cb",
-		.of_match_table = fastrpc_match_table,
-		.suppress_bind_attrs = true,
-	},
-};
+	for_each_available_child_of_node(rdev->of_node, np) {
+		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
+			of_node_put(np);
+			continue;
+		}
+
+		pdev = of_platform_device_create(np, NULL, rdev);
+		if (!pdev) {
+			of_node_put(np);
+			fastrpc_cb_devices_destroy(rpdev);
+			return -ENODEV;
+		}
+
+		err = fastrpc_cb_init(pdev);
+		if (err) {
+			of_node_put(np);
+			fastrpc_cb_devices_destroy(rpdev);
+			return err;
+		}
+	}
+
+	return 0;
+}
 
 static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ctx *cctx,
 				   bool is_secured, const char *domain)
@@ -2639,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	data->rpdev = rpdev;
 	dev_set_drvdata(&rpdev->dev, data);
 
-	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
+	err = fastrpc_cb_devices_create(rpdev);
 	if (err)
 		goto err_deregister_fdev;
 
@@ -2711,7 +2724,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 				&cctx->remote_heap_addr, cctx->remote_heap_size, err);
 	}
 
-	of_platform_depopulate(&rpdev->dev);
+	fastrpc_cb_devices_destroy(rpdev);
 
 	fastrpc_channel_ctx_put(cctx);
 }
@@ -2798,33 +2811,7 @@ static struct rpmsg_driver fastrpc_driver = {
 	},
 };
 
-static int fastrpc_init(void)
-{
-	int ret;
-
-	ret = platform_driver_register(&fastrpc_cb_driver);
-	if (ret < 0) {
-		pr_err("fastrpc: failed to register cb driver\n");
-		return ret;
-	}
-
-	ret = register_rpmsg_driver(&fastrpc_driver);
-	if (ret < 0) {
-		pr_err("fastrpc: failed to register rpmsg driver\n");
-		platform_driver_unregister(&fastrpc_cb_driver);
-		return ret;
-	}
-
-	return 0;
-}
-module_init(fastrpc_init);
-
-static void fastrpc_exit(void)
-{
-	platform_driver_unregister(&fastrpc_cb_driver);
-	unregister_rpmsg_driver(&fastrpc_driver);
-}
-module_exit(fastrpc_exit);
+module_rpmsg_driver(fastrpc_driver);
 
 MODULE_DESCRIPTION("Qualcomm FastRPC");
 MODULE_LICENSE("GPL v2");

-- 
2.34.1

[PATCH v6 2/3] misc: fastrpc: move ADSP duplicate session creation to the driver

[Vinayak Katoch]

For ADSP, only a limited number of FastRPC context banks (CBs) are
available. Each CB supports a single session, which means only a few
processes can run on ADSP simultaneously. If all sessions are consumed
by fastrpc daemons, no session remains available when a user application
starts, causing the application to fail.

To work around this, qcom,nsessions = <5> was set in DT to duplicate
sessions inline during fastrpc_cb_init(). This policy does not belong
in DT and should be handled at the driver level instead.

Remove the qcom,nsessions DT property read and the per-CB duplication
logic from fastrpc_cb_init(). After all context banks have been
initialised in fastrpc_rpmsg_probe(), append FASTRPC_DUP_SESSIONS (4)
copies of the last session for the ADSP domain.

Signed-off-by: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index caa479f304c1..5dd1990d3e2a 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -34,6 +34,11 @@
 #define CDSP_DOMAIN_ID (3)
 #define GDSP_DOMAIN_ID (4)
 #define FASTRPC_MAX_SESSIONS	14
+/*
+ * Extra ADSP sessions duplicated from the last CB, as each CB supports
+ * only one session.
+ */
+#define FASTRPC_DUP_SESSIONS	4
 #define FASTRPC_MAX_VMIDS	16
 #define FASTRPC_ALIGN		128
 #define FASTRPC_MAX_FDLIST	16
@@ -2338,7 +2343,6 @@ static int fastrpc_cb_init(struct platform_device *pdev)
 	struct fastrpc_channel_ctx *cctx;
 	struct fastrpc_session_ctx *sess;
 	struct device *dev = &pdev->dev;
-	int i, sessions = 0;
 	unsigned long flags;
 	u32 dma_bits;
 	u32 sid = 0;
@@ -2348,7 +2352,6 @@ static int fastrpc_cb_init(struct platform_device *pdev)
 	if (!cctx)
 		return -EINVAL;
 
-	of_property_read_u32(dev->of_node, "qcom,nsessions", &sessions);
 	if (of_property_read_u32(dev->of_node, "reg", &sid))
 		dev_info(dev, "FastRPC Session ID not specified in DT\n");
 
@@ -2368,16 +2371,6 @@ static int fastrpc_cb_init(struct platform_device *pdev)
 	if (cctx->domain_id == CDSP_DOMAIN_ID)
 		dma_bits = cctx->soc_data->dma_addr_bits_cdsp;
 
-	if (sessions > 0) {
-		struct fastrpc_session_ctx *dup_sess;
-
-		for (i = 1; i < sessions; i++) {
-			if (cctx->sesscount >= FASTRPC_MAX_SESSIONS)
-				break;
-			dup_sess = &cctx->session[cctx->sesscount++];
-			memcpy(dup_sess, sess, sizeof(*dup_sess));
-		}
-	}
 	spin_unlock_irqrestore(&cctx->lock, flags);
 	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
 	if (rc) {
@@ -2656,6 +2649,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	if (err)
 		goto err_deregister_fdev;
 
+	if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) {
+		struct fastrpc_session_ctx *last_sess;
+		struct fastrpc_session_ctx *dup_sess;
+		unsigned long flags;
+
+		spin_lock_irqsave(&data->lock, flags);
+		last_sess = &data->session[data->sesscount - 1];
+		for (i = 0; i < FASTRPC_DUP_SESSIONS; i++) {
+			if (data->sesscount >= FASTRPC_MAX_SESSIONS)
+				break;
+			dup_sess = &data->session[data->sesscount++];
+			memcpy(dup_sess, last_sess, sizeof(*dup_sess));
+		}
+		spin_unlock_irqrestore(&data->lock, flags);
+	}
+
 	return 0;
 
 err_deregister_fdev:

-- 
2.34.1

[PATCH v6 3/3] dt-bindings: misc: qcom,fastrpc: deprecate qcom,nsessions

[Vinayak Katoch]

The qcom,nsessions property was used to duplicate FastRPC sessions
inline during context bank initialisation. Session duplication is now
handled at the driver level, making this DT property redundant. Mark
it deprecated.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Reviewed-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index 49d2a5e8ba39..e945c8ba75e3 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -101,8 +101,10 @@ patternProperties:
       qcom,nsessions:
         $ref: /schemas/types.yaml#/definitions/uint32
         default: 1
+        deprecated: true
         description: >
           A value indicating how many sessions can share this context bank.
+          Session duplication is now handled by the driver.
 
     required:
       - compatible

-- 
2.34.1

Re: [PATCH v6 3/3] dt-bindings: misc: qcom,fastrpc: deprecate qcom,nsessions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] misc: fastrpc: double of_node_put() on continue in fastrpc_cb_devices_create()
- [Medium] misc: fastrpc: session leak in fastrpc_rpmsg_probe() when duplicating active session
--

commit 9b30a7a26899dd48c168049c0e649bd641947cdc
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

This patch series (through commit c3dc726c9e7a, "dt-bindings: misc:
qcom,fastrpc: deprecate qcom,nsessions") replaces of_platform_populate()
with manual iteration of context bank child DT nodes in
fastrpc_cb_devices_create() so that context banks are initialized
synchronously during fastrpc_rpmsg_probe().

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index af18ff1992eed..caa479f304c1c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>  	return 0;
>  }

[ ... ]

> +static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev)
> +{
> +	struct device *rdev = &rpdev->dev;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int err;
> +
> +	for_each_available_child_of_node(rdev->of_node, np) {
> +		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
> +			of_node_put(np);
> +			continue;
> +		}

[Severity: High]
Doesn't calling of_node_put(np) before continue in
fastrpc_cb_devices_create() drop the reference on np twice when an
available child node of rdev->of_node does not match
"qcom,fastrpc-compute-cb"?

In include/linux/of.h, for_each_available_child_of_node() advances to the
next node on each loop iteration by passing the current child node as prev
to of_get_next_available_child():

    #define for_each_available_child_of_node(parent, child) \
        for (child = of_get_next_available_child(parent, NULL); child != NULL; \
             child = of_get_next_available_child(parent, child))

And in drivers/of/base.c, of_get_next_available_child() delegates to
of_get_next_status_child(), which already releases the reference on prev:

    for (; next; next = next->sibling) {
        if (!checker(next))
            continue;
        if (of_node_get(next))
            break;
    }
    of_node_put(prev);

When fastrpc_rpmsg_probe() -> fastrpc_cb_devices_create() encounters a
non-matching available child node, the explicit of_node_put(np) followed by
continue causes a reference count underflow on struct device_node and a
potential use-after-free. Should the explicit of_node_put(np) before
continue be removed (while keeping the of_node_put(np) calls on the early
return error paths)?

[ ... ]

commit 810d79d90cb45359bb71f6c3439fb0921ee5af9c
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: move ADSP duplicate session creation to the driver

This commit removes the qcom,nsessions property handling from
fastrpc_cb_init() and instead duplicates the last initialized session
FASTRPC_DUP_SESSIONS times in fastrpc_rpmsg_probe() for the ADSP domain.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index caa479f304c1c..5dd1990d3e2a9 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -2368,16 +2371,6 @@ static int fastrpc_cb_init(struct platform_device *pdev)
>  	if (cctx->domain_id == CDSP_DOMAIN_ID)
>  		dma_bits = cctx->soc_data->dma_addr_bits_cdsp;
>  
> -	if (sessions > 0) {
> -		struct fastrpc_session_ctx *dup_sess;
> -
> -		for (i = 1; i < sessions; i++) {
> -			if (cctx->sesscount >= FASTRPC_MAX_SESSIONS)
> -				break;
> -			dup_sess = &cctx->session[cctx->sesscount++];
> -			memcpy(dup_sess, sess, sizeof(*dup_sess));
> -		}
> -	}
>  	spin_unlock_irqrestore(&cctx->lock, flags);
>  	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
>  	if (rc) {
> @@ -2656,6 +2649,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (err)
>  		goto err_deregister_fdev;
>  
> +	if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) {
> +		struct fastrpc_session_ctx *last_sess;
> +		struct fastrpc_session_ctx *dup_sess;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&data->lock, flags);
> +		last_sess = &data->session[data->sesscount - 1];
> +		for (i = 0; i < FASTRPC_DUP_SESSIONS; i++) {
> +			if (data->sesscount >= FASTRPC_MAX_SESSIONS)
> +				break;
> +			dup_sess = &data->session[data->sesscount++];
> +			memcpy(dup_sess, last_sess, sizeof(*dup_sess));
> +		}
> +		spin_unlock_irqrestore(&data->lock, flags);
> +	}
> +
>  	return 0;

[Severity: Medium]
Can moving the session duplication out of fastrpc_cb_init() and into a
separate lock critical section in fastrpc_rpmsg_probe() leak the duplicated
dup_sess entries if userspace opens the device concurrently during probe?

In fastrpc_rpmsg_probe(), fastrpc_device_register() registers the miscdevice
before fastrpc_cb_devices_create() and the ADSP session duplication block
run. Previously, session duplication happened inside fastrpc_cb_probe()
under the same cctx->lock acquisition that initialized sess->used = false.

Now, fastrpc_cb_init() initializes the session and drops cctx->lock:

    sess = &cctx->session[cctx->sesscount++];
    sess->used = false;
    sess->valid = true;
    sess->dev = dev;
    sess->sid = sid;

    if (cctx->domain_id == CDSP_DOMAIN_ID)
        dma_bits = cctx->soc_data->dma_addr_bits_cdsp;

    spin_unlock_irqrestore(&cctx->lock, flags);

Because the /dev/fastrpc-adsp miscdevice is already live, a concurrent open
from userspace (such as udev or a fastrpc daemon) calling
fastrpc_device_open() -> fastrpc_session_alloc() can allocate the last
session and set last_sess->used = true before fastrpc_rpmsg_probe()
acquires data->lock:

    spin_lock_irqsave(&cctx->lock, flags);
    for (i = 0; i < cctx->sesscount; i++) {
        if (!cctx->session[i].used && cctx->session[i].valid) {
            cctx->session[i].used = true;
            session = &cctx->session[i];
            ...
            break;
        }
    }
    spin_unlock_irqrestore(&cctx->lock, flags);

When fastrpc_rpmsg_probe() then copies last_sess via memcpy(dup_sess,
last_sess, sizeof(*dup_sess)) without resetting dup_sess->used = false, all
FASTRPC_DUP_SESSIONS duplicated entries are created with used = true and no
owning fastrpc_user to release them, permanently exhausting those sessions
for the lifetime of the channel context and causing later allocations in
fastrpc_session_alloc() to fail with -EBUSY.

Should dup_sess->used = false be set explicitly after the memcpy(), or
should fastrpc_device_register() be called after all sessions and context
banks are initialized?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-dup-sessions-v6-0-578ab52ba66c@oss.qualcomm.com?part=3

Re: [PATCH v6 2/3] misc: fastrpc: move ADSP duplicate session creation to the driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] misc: fastrpc: session leak in fastrpc_rpmsg_probe() on race with open
- [High] misc: fastrpc: double of_node_put() in fastrpc_cb_devices_create()
- [High] misc: fastrpc: use-after-free on fastrpc_cb_devices_create() failure in probe
--

commit 810d79d90cb45359bb71f6c3439fb0921ee5af9c
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: move ADSP duplicate session creation to the driver

This commit removes the qcom,nsessions device tree property read and per-bank
session duplication from fastrpc_cb_init(), and instead appends four copies of
the last session in fastrpc_rpmsg_probe() for the adsp domain.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index caa479f304c1c..5dd1990d3e2a9 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -2368,16 +2371,6 @@ static int fastrpc_cb_init(struct platform_device *pdev)
>  	if (cctx->domain_id == CDSP_DOMAIN_ID)
>  		dma_bits = cctx->soc_data->dma_addr_bits_cdsp;
>  
> -	if (sessions > 0) {
> -		struct fastrpc_session_ctx *dup_sess;
> -
> -		for (i = 1; i < sessions; i++) {
> -			if (cctx->sesscount >= FASTRPC_MAX_SESSIONS)
> -				break;
> -			dup_sess = &cctx->session[cctx->sesscount++];
> -			memcpy(dup_sess, sess, sizeof(*dup_sess));
> -		}
> -	}
>  	spin_unlock_irqrestore(&cctx->lock, flags);
>  	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
>  	if (rc) {
> @@ -2656,6 +2649,22 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	if (err)
>  		goto err_deregister_fdev;

[Severity: High]
Could calling of_node_put(np) before continue in fastrpc_cb_devices_create()
cause a double release of the device_node reference on np?

Right before this check in drivers/misc/fastrpc.c, fastrpc_rpmsg_probe() calls
fastrpc_cb_devices_create(rpdev) (introduced earlier in this series by commit
"misc: fastrpc: iterate CB nodes manually instead of of_platform_populate"):

drivers/misc/fastrpc.c:fastrpc_cb_devices_create() {
	for_each_available_child_of_node(rdev->of_node, np) {
		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
			of_node_put(np);
			continue;
		}
		...
}

In include/linux/of.h, for_each_available_child_of_node() advances the loop
with of_get_next_available_child(parent, child), which already drops the
reference on the previous child node:

#define for_each_available_child_of_node(parent, child) \
	for (child = of_get_next_available_child(parent, NULL); child != NULL; \
	     child = of_get_next_available_child(parent, child))

When an available child node under rdev->of_node is not compatible with
"qcom,fastrpc-compute-cb", calling of_node_put(np) before continue drops the
reference on np a first time, and the loop increment drops it a second time,
leading to a reference count underflow and potential use-after-free on np.

>  
> +	if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) {
> +		struct fastrpc_session_ctx *last_sess;
> +		struct fastrpc_session_ctx *dup_sess;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&data->lock, flags);
> +		last_sess = &data->session[data->sesscount - 1];
> +		for (i = 0; i < FASTRPC_DUP_SESSIONS; i++) {
> +			if (data->sesscount >= FASTRPC_MAX_SESSIONS)
> +				break;
> +			dup_sess = &data->session[data->sesscount++];
> +			memcpy(dup_sess, last_sess, sizeof(*dup_sess));
> +		}
> +		spin_unlock_irqrestore(&data->lock, flags);
> +	}

[Severity: High]
What prevents a race with fastrpc_device_open() from permanently leaking the
duplicated dup_sess entries in data->session[] here?

In drivers/misc/fastrpc.c, fastrpc_rpmsg_probe() registers the character
device via fastrpc_device_register() -> misc_register() before calling
fastrpc_cb_devices_create(rpdev):

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
	...
	case ADSP_DOMAIN_ID:
	case MDSP_DOMAIN_ID:
	case SDSP_DOMAIN_ID:
		data->unsigned_support = false;
		err = fastrpc_device_register(rdev, data, secure_dsp, domain);
		...
	err = fastrpc_cb_devices_create(rpdev);
}

Previously, fastrpc_cb_init() duplicated the sessions inside the same
cctx->lock critical section that initialized sess->used = false. With this
regression, fastrpc_cb_init() sets sess->used = false and sess->valid = true,
increments cctx->sesscount, and drops cctx->lock before calling
dma_set_mask():

drivers/misc/fastrpc.c:fastrpc_cb_init() {
	...
	sess = &cctx->session[cctx->sesscount++];
	sess->used = false;
	sess->valid = true;
	sess->dev = dev;
	sess->sid = sid;

	if (cctx->domain_id == CDSP_DOMAIN_ID)
		dma_bits = cctx->soc_data->dma_addr_bits_cdsp;

	spin_unlock_irqrestore(&cctx->lock, flags);
	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
}

If userspace (such as fastrpc daemons triggered when misc_register() exposes
the device node) opens the device during this unlocked window,
fastrpc_device_open() -> fastrpc_session_alloc() can allocate up to last_sess
and set last_sess->used = true:

drivers/misc/fastrpc.c:fastrpc_session_alloc() {
	...
	for (i = 0; i < cctx->sesscount; i++) {
		if (!cctx->session[i].used && cctx->session[i].valid) {
			cctx->session[i].used = true;
			session = &cctx->session[i];
			fl->client_id = i + 1;
			break;
		}
	}
}

When fastrpc_rpmsg_probe() then checks data->sesscount > 0 (outside
data->lock), acquires data->lock, and copies last_sess into each dup_sess via
memcpy(dup_sess, last_sess, sizeof(*dup_sess)), used = true is copied into all
four dup_sess slots without resetting dup_sess->used = false.

Because no fastrpc_user instance owns those dup_sess pointers,
fastrpc_session_free() is never called on them, leaving all four duplicated
sessions permanently marked used = true and unallocatable.

> +
>  	return 0;
>  
>  err_deregister_fdev:

[Severity: High]
If fastrpc_cb_devices_create() fails after a session has been marked valid,
can kfree(data) in the err_deregister_fdev path cause a use-after-free on
data and the destroyed child platform devices?

Premature registration of the miscdevice prior to child population and
kfree(data) on the error path existed prior to this commit, and commit
9b30a7a26899 ("misc: fastrpc: iterate CB nodes manually instead of
of_platform_populate") in this series replaced of_platform_populate() with
fastrpc_cb_devices_create() while this patch modifies fastrpc_cb_init() and
fastrpc_rpmsg_probe().

During fastrpc_rpmsg_probe() -> fastrpc_cb_devices_create() ->
fastrpc_cb_init() in drivers/misc/fastrpc.c, once the first context bank sets
sess->valid = true and drops cctx->lock, a concurrent open() on the registered
miscdevice can succeed in fastrpc_device_open():

drivers/misc/fastrpc.c:fastrpc_device_open() {
	...
	/* Released in fastrpc_device_release() */
	fastrpc_channel_ctx_get(cctx);
	...
	fl->sctx = fastrpc_session_alloc(fl);
	...
	spin_lock_irqsave(&cctx->lock, flags);
	list_add_tail(&fl->user, &cctx->users);
	spin_unlock_irqrestore(&cctx->lock, flags);
}

If dma_set_mask() then fails in fastrpc_cb_init(), or a subsequent child node
fails in of_platform_device_create() or fastrpc_cb_init(),
fastrpc_cb_devices_create() destroys the child platform devices via
fastrpc_cb_devices_destroy(rpdev) and returns an error:

drivers/misc/fastrpc.c:fastrpc_cb_devices_create() {
	...
		err = fastrpc_cb_init(pdev);
		if (err) {
			of_node_put(np);
			fastrpc_cb_devices_destroy(rpdev);
			return err;
		}
}

fastrpc_rpmsg_probe() then jumps to err_deregister_fdev, calls
misc_deregister() (which does not close existing open file descriptors), and
unconditionally frees data with kfree(data) instead of releasing the initial
reference via fastrpc_channel_ctx_put(data):

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
	...
err_deregister_fdev:
	if (data->fdevice)
		misc_deregister(&data->fdevice->miscdev);
	if (data->secure_fdevice)
		misc_deregister(&data->secure_fdevice->miscdev);

err_free_data:
	kfree(data);
	return err;
}

When the open file descriptor later performs an ioctl or closes the file in
fastrpc_device_release() -> fastrpc_user_put(), it dereferences the freed
cctx (data) to lock cctx->lock, remove fl->user from cctx->users, clear
fl->sctx->used, and call fastrpc_channel_ctx_put(fl->cctx).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-dup-sessions-v6-0-578ab52ba66c@oss.qualcomm.com?part=2

Re: [PATCH v6 1/3] misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] misc: fastrpc: double of_node_put() in fastrpc_cb_devices_create()
- [High] misc: fastrpc: missing DMA and IOMMU setup in fastrpc_cb_devices_create()
- [High] misc: fastrpc: premature misc_register() before fastrpc_cb_devices_create()
--

commit 17aff65cbac844472a8706f83a0e300c3444f667
Author: Vinayak Katoch <vinayak.katoch@oss.qualcomm.com>

misc: fastrpc: iterate CB nodes manually instead of of_platform_populate

This commit replaces of_platform_populate() and the separate
fastrpc_cb_driver platform driver with manual iteration over child DT nodes
in fastrpc_cb_devices_create() and cleanup in fastrpc_cb_devices_destroy()
so context banks are initialized synchronously during fastrpc_rpmsg_probe().

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index af18ff1992eed..caa479f304c1c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -2389,37 +2388,51 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static void fastrpc_cb_remove(struct platform_device *pdev)
> +static void fastrpc_cb_devices_destroy(struct rpmsg_device *rpdev)
>  {
> -	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(pdev->dev.parent);
> -	struct fastrpc_session_ctx *sess = dev_get_drvdata(&pdev->dev);
> +	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
> +	struct device *rdev = &rpdev->dev;
>  	unsigned long flags;
>  	int i;
>  
>  	spin_lock_irqsave(&cctx->lock, flags);
> -	for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
> -		if (cctx->session[i].sid == sess->sid) {
> -			cctx->session[i].valid = false;
> -			cctx->sesscount--;
> -		}
> -	}
> +	for (i = 0; i < cctx->sesscount; i++)
> +		cctx->session[i].valid = false;
>  	spin_unlock_irqrestore(&cctx->lock, flags);
> +
> +	device_for_each_child_reverse(rdev, NULL, of_platform_device_destroy);
>  }
>  
> -static const struct of_device_id fastrpc_match_table[] = {
> -	{ .compatible = "qcom,fastrpc-compute-cb" },
> -	{ }
> -};
> +static int fastrpc_cb_devices_create(struct rpmsg_device *rpdev)
> +{
> +	struct device *rdev = &rpdev->dev;
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	int err;
>  
> -static struct platform_driver fastrpc_cb_driver = {
> -	.probe = fastrpc_cb_probe,
> -	.remove = fastrpc_cb_remove,
> -	.driver = {
> -		.name = "qcom,fastrpc-cb",
> -		.of_match_table = fastrpc_match_table,
> -		.suppress_bind_attrs = true,
> -	},
> -};
> +	for_each_available_child_of_node(rdev->of_node, np) {
> +		if (!of_device_is_compatible(np, "qcom,fastrpc-compute-cb")) {
> +			of_node_put(np);
> +			continue;
> +		}

[Severity: High]
Won't calling of_node_put(np) before continue in fastrpc_cb_devices_create()
cause a double-put on np when an available child node is not compatible with
"qcom,fastrpc-compute-cb"?

for_each_available_child_of_node() advances by calling
of_get_next_available_child() -> of_get_next_status_child() in
drivers/of/base.c:

	of_node_put(prev);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
	return next;

Because the loop increment expression dereferences prev->sibling and
automatically calls of_node_put(prev) on each step, dropping the reference on
np explicitly before continue underflows the struct device_node reference
count and can lead to a use-after-free.

> +
> +		pdev = of_platform_device_create(np, NULL, rdev);
> +		if (!pdev) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return -ENODEV;
> +		}
> +
> +		err = fastrpc_cb_init(pdev);
> +		if (err) {
> +			of_node_put(np);
> +			fastrpc_cb_devices_destroy(rpdev);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
With fastrpc_cb_driver removed, how are DMA and IOMMU configuration and
probe deferral handled for the context bank platform devices created in
fastrpc_cb_devices_create()?

Previously, binding each qcom,fastrpc-compute-cb device to fastrpc_cb_driver
went through really_probe() in drivers/base/dd.c, which checked supplier
links via device_links_check_suppliers(dev), set dev->driver, and called
platform_dma_configure() in drivers/base/platform.c:

	if (is_of_node(fwnode)) {
		ret = of_dma_configure(dev, to_of_node(fwnode), true);
	} else if (is_acpi_device_node(fwnode)) {
		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
		ret = acpi_dma_configure(dev, attr);
	}
	/* @dev->driver may not be valid when we're called from the IOMMU layer */
	if (ret || !drv || to_platform_driver(drv)->driver_managed_dma)
		return ret;

	ret = iommu_device_use_default_domain(dev);

Without a bound platform_driver or an explicit of_dma_configure() call in
fastrpc_cb_devices_create() or fastrpc_cb_init(), pdev->dev.driver is always
NULL:

1. Because the iommus property resides on the child compute-cb DT nodes
   rather than the parent rpmsg node, fw_devlink only creates proxy
   sync_state links on rdev. During of_platform_device_create() ->
   device_add(), iommu_init_device() in drivers/iommu/iommu.c calls
   dev->bus->dma_configure(dev) with dev->driver == NULL and ignores its
   return value. If the SMMU has not probed yet and of_iommu_configure()
   returns -EPROBE_DEFER in of_dma_configure_id() (drivers/of/device.c), it
   returns early before arch_setup_dma_ops(dev, coherent), yet
   fastrpc_cb_init() still succeeds and marks sess->valid = true with
   unconfigured DMA ops and no IOMMU translation. A subsequent
   bus_iommu_probe() replay of of_dma_configure_id() also overwrites
   *dev->dma_mask with the 32-bit coherent_dma_mask, clobbering the 34-bit
   mask set via dma_set_mask() in fastrpc_cb_init().

2. When CONFIG_IOMMU_API is disabled, iommu_bus_notifier is not registered
   and of_dma_configure() is never called on pdev->dev at all.

3. Even when the SMMU has already probed, platform_dma_configure() returns
   before calling iommu_device_use_default_domain(dev) because dev->driver
   is NULL (leaving group->owner_cnt at 0 while fastrpc performs DMA
   allocations and dma_buf mappings on sess->dev), and managed supplier
   device links to pdev->dev remain unactivated because
   device_links_driver_bound() never runs.

> @@ -2639,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	data->rpdev = rpdev;
>  	dev_set_drvdata(&rpdev->dev, data);
>  
> -	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
> +	err = fastrpc_cb_devices_create(rpdev);
>  	if (err)
>  		goto err_deregister_fdev;
>  

[Severity: High]
Could calling fastrpc_cb_devices_create() after fastrpc_device_register() in
fastrpc_rpmsg_probe() still expose partially initialized sessions to
userspace and trigger a use-after-free on failure?

Earlier in fastrpc_rpmsg_probe(), fastrpc_device_register() calls
misc_register() before data->refcount, data->users, data->lock, data->ctx_idr,
and data->rpdev are initialized, and before fastrpc_cb_devices_create(rpdev)
is called (as well as before ADSP duplicate session creation in "misc:
fastrpc: move ADSP duplicate session creation to the driver"). Inside
fastrpc_cb_init(), sess->valid = true and cctx->sesscount++ are also published
under cctx->lock before dma_set_mask() is executed:

	sess = &cctx->session[cctx->sesscount++];
	sess->used = false;
	sess->valid = true;
	sess->dev = dev;
	sess->sid = sid;
	...
	spin_unlock_irqrestore(&cctx->lock, flags);
	rc = dma_set_mask(dev, DMA_BIT_MASK(dma_bits));
	if (rc) {
		dev_err(dev, "%u-bit DMA enable failed\n", dma_bits);
		return rc;
	}

Because the miscdevices are already registered while
fastrpc_cb_devices_create() iterates over the DT child nodes, a concurrent
fastrpc_device_open() -> fastrpc_session_alloc() can still observe zero or a
partial set of sessions, or acquire an earlier session and take a reference
on data via fastrpc_channel_ctx_get(cctx).

Unlike of_platform_populate() which previously returned 0 even if child
device creation failed, if dma_set_mask() fails or a later CB node fails in
of_platform_device_create() or fastrpc_cb_init(), fastrpc_cb_devices_create()
destroys the CB platform devices and fastrpc_rpmsg_probe() jumps to
err_deregister_fdev -> err_free_data: kfree(data), freeing data while an open
file descriptor still holds references to fl->cctx and fl->sctx->dev. Should
fastrpc_device_register() be called only after cctx and all CB sessions have
been fully initialized?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-dup-sessions-v6-0-578ab52ba66c@oss.qualcomm.com?part=1