Re: [PATCH v4 04/25] accel/qda: Assign a context bank to each session

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel/qda: dangling cb_dev pointer in struct qda_iommu_device after unpopulate
- [High] accel/qda: devm lifetime mismatch and premature teardown of qdev->iommu_mgr
--

commit 274f69c69f358c698c8f7dd37051492c7c060cc1
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

accel/qda: Assign a context bank to each session

This commit introduces a context bank registry (struct qda_memory_manager) to
track a QDA device's compute context banks and exclusively assign one refcounted
struct qda_iommu_device per open session.

> diff --git a/drivers/accel/qda/qda_cb.c b/drivers/accel/qda/qda_cb.c
> index afb0bb71a1f44..6f1cff1133f9d 100644
> --- a/drivers/accel/qda/qda_cb.c
> +++ b/drivers/accel/qda/qda_cb.c
> @@ -7,8 +7,44 @@
>  #include <drm/drm_print.h>
>  #include "qda_cb.h"
>  #include "qda_drv.h"
> +#include "qda_memory_manager.h"
>  #include "qda_soc.h"
>  
> +static int qda_cb_setup_device(struct qda_dev *qdev, struct device *cb_dev)
> +{
> +	struct qda_iommu_device *iommu_dev;
> +	int ret;
> +
> +	iommu_dev = kzalloc_obj(*iommu_dev);
> +	if (!iommu_dev)
> +		return -ENOMEM;
> +
> +	iommu_dev->dev = cb_dev;
> +	iommu_dev->qdev = qdev;
> +
> +	ret = qda_memory_manager_register_device(qdev->iommu_mgr, iommu_dev);
> +	if (ret) {
> +		drm_err(&qdev->drm_dev, "Failed to register context bank: %d\n", ret);
> +		kfree(iommu_dev);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(cb_dev, iommu_dev);
> +
> +	return 0;
> +}
> +
> +static void qda_cb_teardown_device(struct device *cb_dev)
> +{
> +	struct qda_iommu_device *iommu_dev = dev_get_drvdata(cb_dev);
> +
> +	if (!iommu_dev)
> +		return;
> +
> +	qda_memory_manager_unregister_device(iommu_dev->qdev->iommu_mgr, iommu_dev);
> +	dev_set_drvdata(cb_dev, NULL);
> +}

[ ... ]

> @@ -70,6 +112,7 @@ void qda_cb_unpopulate(struct qda_dev *qdev)
>  
>  	list_for_each_entry_safe(entry, tmp, &qdev->cb_devs, node) {
>  		list_del(&entry->node);
> +		qda_cb_teardown_device(&entry->pdev->dev);
>  		of_platform_device_destroy(&entry->pdev->dev, NULL);

[Severity: High]
Can this leave a dangling pointer in iommu_dev->dev when the rpmsg channel
goes away while a session is still open?

In qda_cb_setup_device(), iommu_dev->dev stores the raw cb_dev pointer
(&pdev->dev) without taking a reference via get_device(cb_dev), and
iommu_device_free() frees struct qda_iommu_device without calling
put_device().

When a session owns a context bank via qda_memory_manager_assign_device(),
assign_available_device() takes a second reference on iommu_dev->ref so that
struct qda_iommu_device stays alive until qda_postclose() ->
qda_session_free() calls qda_memory_manager_release_device().

If qda_rpmsg_remove() runs while a DRM file is open:

  qda_rpmsg_remove()
    qda_cb_unpopulate()
      qda_cb_teardown_device(&entry->pdev->dev)
        qda_memory_manager_unregister_device()
          kref_put(&iommu_dev->ref, iommu_device_free)  /* ref drops 2 -> 1 */
      of_platform_device_destroy(&entry->pdev->dev, NULL)

of_platform_device_destroy() unregisters and frees entry->pdev (and its
embedded struct device), while session->assigned_iommu_dev remains alive with
iommu_dev->dev still pointing to the freed struct device for the remainder of
the open session's lifetime.

Should qda_cb_setup_device() and iommu_device_free() manage a device
reference with get_device() and put_device(), or otherwise invalidate
iommu_dev->dev under synchronization when the context bank is torn down?

> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c
> index 6ecb6a966f686..a99e23933f6ba 100644
> --- a/drivers/accel/qda/qda_drv.c
> +++ b/drivers/accel/qda/qda_drv.c

[ ... ]

> @@ -70,6 +76,26 @@ struct qda_dev *qda_alloc_device(struct device *dev)
>  	return qdev;
>  }
>  
> +int qda_init_device(struct qda_dev *qdev, int num_cbs)
> +{
> +	int ret;
> +
> +	qdev->iommu_mgr = devm_kzalloc(qdev->dev, sizeof(*qdev->iommu_mgr), GFP_KERNEL);
> +	if (!qdev->iommu_mgr)
> +		return -ENOMEM;
> +
> +	ret = qda_memory_manager_init(qdev->iommu_mgr, num_cbs);
> +	if (ret)
> +		drm_err(&qdev->drm_dev, "Failed to initialize memory manager: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +void qda_deinit_device(struct qda_dev *qdev)
> +{
> +	qda_memory_manager_exit(qdev->iommu_mgr);
> +}

[Severity: High]
Does allocating qdev->iommu_mgr with devm_kzalloc(qdev->dev, ...) and tearing
it down in qda_rpmsg_remove() -> qda_deinit_device() mismatch the lifetime of
struct qda_dev?

qda_alloc_device() allocates struct qda_dev with devm_drm_dev_alloc(), which
keeps qdev alive until the last drm_device reference is dropped when all open
drm_file descriptors are closed. In contrast, devm_kzalloc(qdev->dev, ...)
ties qdev->iommu_mgr to the rpmsg_device driver binding.

When qda_rpmsg_remove() runs while a file is open:

  qda_rpmsg_remove()
    drm_dev_unplug(&qdev->drm_dev)
    qda_cb_unpopulate(qdev)
    qda_deinit_device(qdev)
      qda_memory_manager_exit(qdev->iommu_mgr)
        kfree(mem_mgr->devices)
        mutex_destroy(&mem_mgr->assignment_lock)

When qda_rpmsg_remove() returns, devres releases qdev->iommu_mgr, leaving
qdev->iommu_mgr as a non-NULL dangling pointer to freed memory and a destroyed
mutex while qdev and open qda_session instances are still alive. Moreover,
because drm_ioctl() only performs a point-in-time drm_dev_is_unplugged() check
at entry before calling the ioctl handler, any in-flight operation that passed
that check before drm_dev_unplug() and calls
qda_memory_manager_assign_device(qdev->iommu_mgr, session) can race with
qda_memory_manager_exit() and devres teardown.

Should struct qda_memory_manager be embedded in struct qda_dev (or managed via
drmm) so its lifetime and mutex match qdev->drm_dev?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=4