Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] misc: fastrpc: session leak in fastrpc_rpmsg_probe() on race with open --
commit 51e0d53d78d8245f1572af01da3a96d0f550fc6f Author: Vinayak Katoch <[email protected]> misc: fastrpc: move ADSP duplicate session creation to the driver This commit removes the qcom,nsessions device tree property read and per-CB session duplication from fastrpc_cb_init(), and instead appends FASTRPC_DUP_SESSIONS copies of the last initialized session in fastrpc_rpmsg_probe() for the ADSP domain. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 7441e9b8940d8..b4be0ee2bc181 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2369,16 +2372,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) { > @@ -2681,6 +2674,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); > + } [Severity: High] As reported by Sashiko on v2 [1], could a concurrent open() claim last_sess between fastrpc_cb_init() dropping cctx->lock and fastrpc_rpmsg_probe() duplicating the session, permanently leaking all 4 dup_sess entries? In fastrpc_rpmsg_probe(), fastrpc_device_register() calls misc_register() before fastrpc_cb_devices_create() and before the ADSP duplicate session block: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... switch (domain_id) { case ADSP_DOMAIN_ID: ... 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 change, fastrpc_cb_init() sets sess->valid = true, increments cctx->sesscount, and drops cctx->lock before fastrpc_rpmsg_probe() re-acquires data->lock to copy last_sess. If userspace daemons open the character device node during this window, fastrpc_device_open() -> fastrpc_session_alloc() can claim last_sess and set last_sess->used = true (or fail with -EBUSY if all raw CB sessions are consumed before the duplicate sessions are appended): 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]; ... break; } } } When fastrpc_rpmsg_probe() then runs memcpy(dup_sess, last_sess, sizeof(*dup_sess)), used = true is copied into all 4 dup_sess slots without resetting dup_sess->used = false. Because no fastrpc_user instance owns those duplicated slots, fastrpc_session_free() will never be called on them, leaving all 4 extra ADSP sessions permanently unavailable. Also, data->sesscount is checked outside data->lock before entering the block. [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
