[PATCH] accel/rocket: search every core slot when looking up a scheduler

[PATCH] accel/rocket: search every core slot when looking up a scheduler

[Igor Paunovic]

sched_to_core() walks rdev->cores[] up to rdev->num_cores, and
rocket_remove() decrements num_cores for every core it removes. Unbind a
core that is not the last one and the cores behind it fall outside the
search, so sched_to_core() returns NULL for a core that is still bound and
still running jobs. Neither caller checks the result:

  rocket_job_run():      rocket_fence_create(core), core->dev
  rocket_job_timedout(): dev_err(core->dev, "NPU job timed out")

Unbinding the middle core of the three on an RK3588 while three clients are
submitting to all of them faults twice, once from the surviving core's
job queue and once from its reset work:

  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
  Workqueue: fdad0000.npu drm_sched_run_job_work [gpu_sched]
  pc : rocket_job_run+0x234/0x838 [rocket]
  Call trace:
   rocket_job_run+0x234/0x838 [rocket]
   drm_sched_run_job_work+0x2cc/0xad8 [gpu_sched]
   process_one_work+0x640/0x14f0

  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  Workqueue: rocket-reset-2 drm_sched_job_timedout [gpu_sched]
  pc : rocket_job_timedout+0xf0/0x1e0 [rocket]
  Call trace:
   rocket_job_timedout+0xf0/0x1e0 [rocket]
   drm_sched_job_timedout+0x188/0x6a0 [gpu_sched]

Both are the third core: the workqueue names are its device and its
core->index, and it was left at slot 2 while num_cores had dropped to 2.

Search all the slots that were allocated, the way find_core_for_dev() now
does. A core that is still bound is then found, and the two callers get
the pointer they already assume they have.

This does not make unbinding one core out of several safe. An open client
keeps an entity pointing at the scheduler of the core that went away:
drm_sched reports it as not ready for every job that lands on it, and the
client waits in dma_fence_default_wait for a fence that will never signal.
Stopping the NULL dereference is what belongs in a fix; the rest wants
more thought.

Reported-by: Sidong Yang <sidong.yang@furiosa.ai>
Closes: https://lore.kernel.org/dri-devel/apwUewaRnoTNXHCt@rock-5b-plus/
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 3141f210fcd1b..a6c24dfe0563a 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -283,7 +283,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
 {
 	unsigned int core;
 
-	for (core = 0; core < rdev->num_cores; core++) {
+	for (core = 0; core < rdev->max_cores; core++) {
 		if (&rdev->cores[core].sched == sched)
 			return &rdev->cores[core];
 	}

base-commit: a9f09b5ea0c3db1e2d4c0f8d3ebdd612d8aa0366
prerequisite-patch-id: 519bcdfdde80d902309c8346f749ebc4bb6b29c0
-- 
2.43.0

[PATCH] accel/rocket: search every core slot when looking up a scheduler

[Igor Paunovic]

sched_to_core() walks rdev->cores[] up to rdev->num_cores, and
rocket_remove() decrements num_cores for every core it removes. Unbind a
core that is not the last one and the cores behind it fall outside the
search, so sched_to_core() returns NULL for a core that is still bound and
still running jobs. Neither caller checks the result:

  rocket_job_run():      rocket_fence_create(core), core->dev
  rocket_job_timedout(): dev_err(core->dev, "NPU job timed out")

Unbinding the middle core of the three on an RK3588 while three clients are
submitting to all of them faults twice, once from the surviving core's
job queue and once from its reset work:

  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
  Workqueue: fdad0000.npu drm_sched_run_job_work [gpu_sched]
  pc : rocket_job_run+0x234/0x838 [rocket]
  Call trace:
   rocket_job_run+0x234/0x838 [rocket]
   drm_sched_run_job_work+0x2cc/0xad8 [gpu_sched]
   process_one_work+0x640/0x14f0

  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  Workqueue: rocket-reset-2 drm_sched_job_timedout [gpu_sched]
  pc : rocket_job_timedout+0xf0/0x1e0 [rocket]
  Call trace:
   rocket_job_timedout+0xf0/0x1e0 [rocket]
   drm_sched_job_timedout+0x188/0x6a0 [gpu_sched]

Both are the third core: the workqueue names are its device and its
core->index, and it was left at slot 2 while num_cores had dropped to 2.

Search all the slots that were allocated, the way find_core_for_dev() now
does. A core that is still bound is then found, and the two callers get
the pointer they already assume they have.

This does not make unbinding one core out of several safe. An open client
keeps an entity pointing at the scheduler of the core that went away:
drm_sched reports it as not ready for every job that lands on it, and the
client waits in dma_fence_default_wait for a fence that will never signal.
Stopping the NULL dereference is what belongs in a fix; the rest wants
more thought.

Reported-by: Sidong Yang <sidong.yang@furiosa.ai>
Closes: https://lore.kernel.org/dri-devel/apwUewaRnoTNXHCt@rock-5b-plus/
Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Assisted-by: LLM sparse checkpatch
---
 drivers/accel/rocket/rocket_job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
index 3141f210fcd1b..a6c24dfe0563a 100644
--- a/drivers/accel/rocket/rocket_job.c
+++ b/drivers/accel/rocket/rocket_job.c
@@ -283,7 +283,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
 {
 	unsigned int core;
 
-	for (core = 0; core < rdev->num_cores; core++) {
+	for (core = 0; core < rdev->max_cores; core++) {
 		if (&rdev->cores[core].sched == sched)
 			return &rdev->cores[core];
 	}

base-commit: a9f09b5ea0c3db1e2d4c0f8d3ebdd612d8aa0366
prerequisite-patch-id: 519bcdfdde80d902309c8346f749ebc4bb6b29c0
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] accel/rocket: search every core slot when looking up a scheduler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [Critical] The patch introduces a compilation failure by referencing an undefined struct member `max_cores` in `struct rocket_device`.

Pre-existing issues:
- [Critical] Decrementing `rdev->num_cores` on core removal without array compaction corrupts core tracking, leading to dropped active cores, array overwrites on re-probe, and use-after-free.
- [Critical] Use-after-free of GPU scheduler because reset work is cancelled after scheduler teardown.
- [High] Unbinding a core leaves active clients with dangling scheduler entities, causing jobs to hang forever.
- [High] Missing NULL check for the `scheds` array allocation in `rocket_job_open`.
- [High] Incorrect teardown ordering in `rocket_core_fini` drops IOMMU and PM runtime states while the DRM scheduler and hardware are still active.
- [High] The IOVA allocator allocates up to 40 bits, but the hardware register and UAPI strictly support 32-bit addresses for command buffers.
- [High] The shared IRQ handler reads a hardware register without verifying that the device power domain and clocks are enabled.
- [High] The IRQ handler leaks a reference to the IOMMU group on every completed job.
--

commit 7e2bfdb7bd20f1e79747c6f4d972e6308a291eb7
Author: Igor Paunovic <royalnet026@gmail.com>

accel/rocket: search every core slot when looking up a scheduler

This patch attempts to fix a bug where sched_to_core() fails to find a core
after intermediate cores are unbound. It changes the iteration bound to
search all allocated core slots instead of just up to num_cores.

Closes: https://lore.kernel.org/dri-devel/apwUewaRnoTNXHCt@rock-5b-plus/

> diff --git a/drivers/accel/rocket/rocket_job.c b/drivers/accel/rocket/rocket_job.c
> index 3141f210fcd1b..a6c24dfe0563a 100644
> --- a/drivers/accel/rocket/rocket_job.c
> +++ b/drivers/accel/rocket/rocket_job.c
> @@ -283,7 +283,7 @@ static struct rocket_core *sched_to_core(struct rocket_device *rdev,
>  {
>  	unsigned int core;
>  
> -	for (core = 0; core < rdev->num_cores; core++) {
> +	for (core = 0; core < rdev->max_cores; core++) {

[Severity: Critical]
Does struct rocket_device actually contain the max_cores field? The commit
message mentions this approach, but I can't find max_cores defined in struct
rocket_device. Will this cause a build failure here?

>  		if (&rdev->cores[core].sched == sched)
>  			return &rdev->cores[core];
>  	}

[Severity: Critical]
This isn't a bug introduced by this patch, but when a core is removed in
rocket_remove():

drivers/accel/rocket/rocket_drv.c:rocket_remove() {
    ...
    rdev->num_cores--;
    ...
}

The active core count is decremented blindly without shifting the remaining
active cores into the resulting gap. Does this corrupt the core tracking,
leaving surviving active cores orphaned and vulnerable to being overwritten
if rocket_probe() reuses the shrunk num_cores as an index?

[Severity: High]
This is a pre-existing issue, but rocket_core_fini() initiates the teardown
of the drm scheduler:

drivers/accel/rocket/rocket_core.c:rocket_core_fini() {
    ...
    rocket_job_fini(core);
    ...
}

However, the client retains the scheduler entity pointing to this destroyed
scheduler from when it was initialized:

drivers/accel/rocket/rocket_job.c:rocket_job_open() {
    ...
    ret = drm_sched_entity_init(&rocket_priv->sched_entity,
                                DRM_SCHED_PRIORITY_NORMAL,
                                scheds,
                                rdev->num_cores, NULL);
    ...
}

If a client keeps the DRM file descriptor open while a core is unbound and
submits a job, will those jobs permanently hang waiting for the destroyed
scheduler?

[Severity: High]
This isn't a bug introduced by this patch, but rocket_job_open() allocates
the scheds array:

drivers/accel/rocket/rocket_job.c:rocket_job_open() {
    ...
    struct drm_gpu_scheduler **scheds = kmalloc_objs(*scheds,
                                                     rdev->num_cores);
    unsigned int core;
    int ret;
    
    for (core = 0; core < rdev->num_cores; core++)
        scheds[core] = &rdev->cores[core].sched;
    ...
}

If the allocation fails under memory pressure and returns NULL, won't the
immediate loop dereference lead to a kernel oops?

[Severity: Critical]
This is a pre-existing issue, but when cleaning up a job:

drivers/accel/rocket/rocket_job.c:rocket_job_fini() {
    ...
    drm_sched_fini(&core->sched);
    
    cancel_work_sync(&core->reset.work);
    ...
}

The scheduler is destroyed before its associated asynchronous reset work is
cancelled. If a job times out concurrently and the reset work executes,
could it result in a use-after-free when attempting to operate on the
already freed scheduler?

[Severity: High]
This is a pre-existing issue, but rocket_core_fini() drops IOMMU and PM
runtime states early:

drivers/accel/rocket/rocket_core.c:rocket_core_fini() {
    ...
    pm_runtime_disable(core->dev);
    iommu_group_put(core->iommu_group);
    core->iommu_group = NULL;
    rocket_job_fini(core);
    ...
}

Since rocket_job_fini() waits on or cancels the DRM scheduler, does
releasing the IOMMU group and disabling PM runtime beforehand leave active
hardware DMAing into unmapped memory or accessing disabled PM resources?

[Severity: High]
This isn't a bug introduced by this patch, but rocket_ioctl_create_bo()
allocates up to a 40-bit IOVA:

drivers/accel/rocket/rocket_gem.c:rocket_ioctl_create_bo() {
    ...
    args->dma_address = rkt_obj->mm.start;
    ...
}

Which is later written back to the NPU's hardware register via the 32-bit
regcmd field:

drivers/accel/rocket/rocket_job.c:rocket_job_hw_submit() {
    ...
    rocket_pc_writel(core, BASE_ADDRESS, task->regcmd);
    ...
}

If the buffer IOVA allocation exceeds the 4GB boundary, does this result in
silent address truncation and subsequent IOMMU faults when the hardware
accesses unintended physical memory?

[Severity: High]
This isn't a bug introduced by this patch, but the shared IRQ handler reads
a hardware register unconditionally:

drivers/accel/rocket/rocket_job.c:rocket_job_irq_handler() {
    ...
    u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
    ...
}

If another device sharing the same IRQF_SHARED interrupt line fires while
the NPU is idle and runtime-suspended, will reading this register without
verifying the device power domain cause a synchronous external abort?

[Severity: High]
This is a pre-existing issue, but the IRQ handler gets an IOMMU group
reference:

drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
    ...
    iommu_detach_group(NULL, iommu_group_get(core->dev));
    ...
}

Since iommu_group_put() is never called to balance this, does this leak a
reference to the IOMMU group every time an in-flight job completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905150432.7477-1-royalnet026@gmail.com?part=1

Re: [PATCH] accel/rocket: search every core slot when looking up a scheduler

[Igor Paunovic]

> - [Critical] The patch introduces a compilation failure by referencing an
>   undefined struct member `max_cores` in `struct rocket_device`.

Correct on its own, and my fault for leaving it to a trailer.

max_cores is added by "accel/rocket: search every core slot when a core is
removed", still on the list:

  https://lore.kernel.org/dri-devel/20260904125936.26234-1-royalnet026@gmail.com/

This patch declares that dependency as prerequisite-patch-id, and the id in
it matches git patch-id of that patch exactly. On top of it the build is
clean with W=1. On its own it does not build, as you say - it should have
said so in plain text below the --- as well, not only in the trailer. I
will do that on any patch of mine that depends on a pending one.

The pre-existing issues are useful, and two of them are already in hand: the
global rdev left holding an ERR_PTR, and the devm allocations on drm_dev->dev
that are never freed. The reset work cancelled after drm_sched_fini, and the
iommu_group reference taken in the IRQ handler, I had not seen. I will not
send patches for them until I have reproduced them on hardware - the two bugs
I did send this week both looked different once a KASAN kernel had printed
the trace.

Igor

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] accel/rocket: search every core slot when looking up a scheduler

[Igor Paunovic]

> - [Critical] The patch introduces a compilation failure by referencing an
>   undefined struct member `max_cores` in `struct rocket_device`.

Correct on its own, and my fault for leaving it to a trailer.

max_cores is added by "accel/rocket: search every core slot when a core is
removed", still on the list:

  https://lore.kernel.org/dri-devel/20260904125936.26234-1-royalnet026@gmail.com/

This patch declares that dependency as prerequisite-patch-id, and the id in
it matches git patch-id of that patch exactly. On top of it the build is
clean with W=1. On its own it does not build, as you say - it should have
said so in plain text below the --- as well, not only in the trailer. I
will do that on any patch of mine that depends on a pending one.

The pre-existing issues are useful, and two of them are already in hand: the
global rdev left holding an ERR_PTR, and the devm allocations on drm_dev->dev
that are never freed. The reset work cancelled after drm_sched_fini, and the
iommu_group reference taken in the IRQ handler, I had not seen. I will not
send patches for them until I have reproduced them on hardware - the two bugs
I did send this week both looked different once a KASAN kernel had printed
the trace.

Igor