Re: [RFC PATCH net-next 07/13] drm/amdkfd: add knod provider core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 22 potential issue(s) to consider:
- [Critical] Reliance on a user-space binary (/bin/sleep) to provide a process context for the GPU network offload core.
- [Critical] Vulnerability to PID recycling in `stop_umh()`, leading to potential arbitrary process termination.
- [High] Hardcoded VFS path lookup from kernel space to open DRM render nodes makes the core vulnerable to namespace spoofing.
- [High] AMDGPU ring buffer write pointers are defined as 32-bit signed integers instead of `u64`, causing sign-extension corruption.
- [High] Global `accels` array is overwritten when multiple AMD GPUs are initialized.
- [High] Array bounds overflow for queue structures due to `NR_CPUS` vs `KNOD_MAX_QUEUE_CNT` mismatch.
- [High] GPU memory allocated by `__knod_alloc_mem` is silently leaked upon free.
- [High] GPU virtual addresses are passed as `__user` pointers to KFD queue creation.
- [High] The KFD process and MMU notifier are leaked on initialization error paths in `knod_alloc_ctx_init` and `knod_alloc_ctx`.
- [High] Hardcoded access to `pdds[0]` causes index mismatches on multi-GPU systems.
- [High] SDMA queue GPU memory is not explicitly zeroed, causing the hardware engine to execute garbage commands.
- [High] `knod_exit()` tears down knod contexts without stopping the background worker thread, leading to a Use-After-Free.
- [High] Concurrent modification of a single SDMA ring buffer across multiple CPUs.
- [High] Modifying `accel_ops.xdp_ops` during per-device feature activation overrides features for all devices system-wide.
- [High] Unprotected concurrent modifications to the global `ctx_list`.
- [High] Data race between MMU notifier cleanup and KNOD's explicit queue teardown.
- [High] Concurrent feature lifecycle operations can lead to a data race on `knod->worker`, causing multiple threads to simultaneously stop the same kthread.
- [High] Hardware packets and descriptors are written to GPU memory using host native endianness instead of little-endian.
- [High] A memory barrier is missing before writing to the AQL doorbell, which can cause the GPU to observe a stale dispatch ID.
- [Medium] Lack of locking in `knod_accel_xdp_register()` allows it to race with `knod_attach()`, causing missed XDP initialization.
- [Medium] `knod_init` returns 0 (success) on allocation failure, causing silent failures during accelerator initialization.
- [Low] A typo in the endianness conditional macro for `knod_aql_rsrc1` causes the struct to be empty on big-endian architectures.
--

--- Patch [7]: [RFC PATCH net-next 07/13] drm/amdkfd: add knod provider core ---
commit 3676145304477ec735999c96871ed634c55e8b47
Author: Taehee Yoo <ap420073@gmail.com>

drm/amdkfd: add knod provider core

This patch introduces the knod accelerator provider into amdgpu. It allocates
and drives GPU AQL/SDMA queues, manages GPU memory for the RX data path, and
registers a knod accelerator that binds to a NIC via the knod core.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_knod.c b/drivers/gpu/drm/amd/amdkfd/kfd_knod.c
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_knod.c
> @@ -0,0 +1,2202 @@

[ ... ]

> +static int launch_and_get_pid(void)
> +{
> +	static const char * const argv[] = { "/bin/sleep", "2147483647", NULL };

[Severity: Critical]
Is it safe to rely on a user-space binary to provide the process context?

The core uses call_usermodehelper_exec() to launch /bin/sleep to bind the
KFD GPU context. If the /bin/sleep process is killed by an administrator, the
OOM killer, or exits unexpectedly, the KFD MMU notifier will asynchronously
tear down the GPU resources locklessly.

Can this lead to a severe crash in the network driver?

[ ... ]

> +static void stop_umh(pid_t umh_pid)
> +{
> +	struct pid *p = find_get_pid(umh_pid);
> +
> +	if (!p)
> +		return;
> +
> +	kill_pid(p, SIGKILL, 1);
> +	put_pid(p);
> +}

[Severity: Critical]
Is there a risk of terminating unrelated processes due to PID recycling?

When the KNOD context is released, stop_umh() looks up the user mode helper
process by its numeric PID and unconditionally sends a SIGKILL.

Because knod_alloc_ctx() drops the safe struct pid reference immediately
after creation, if the original helper process exited prematurely, its PID
might have been reassigned. Could detaching the network device
unintentionally kill an innocent process?

[ ... ]

> +struct knod_mem *__knod_alloc_mem(struct knod *knod, size_t size,
> +				  int flags)
> +{
[ ... ]
> +	err = amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(kdev->adev, mem->gaddr,
> +						      size, pdd->drm_priv,
> +						      &mem->mem, NULL, flags,
> +						      false);

[Severity: High]
Does this code leak GPU memory when the packets are freed?

Looking at __knod_alloc_mem(), it uses
amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu() directly which bypasses the KFD
tracking list.

However, knod_free_mem() uses kfd_process_free_gpuvm() which expects the
memory to be in the process tracking list. If it is not found, will the GPU
memory remain permanently leaked?

[ ... ]

> +void knod_sdma_copy(struct knod *knod, u64 dst_offset,
> +		    u64 src_offset, int idx, int size)
> +{
> +	struct knod_sdma *sdma = &knod->sdma[idx];
> +	u32 ring_mask = (sdma->sdma->size / 4) - 1;
> +	u64 *wptr = (u64 *)sdma->queue->kaddr + 1;
> +	u32 *ptr = sdma->sdma->kaddr;
> +
> +	ptr[sdma->idx++ & ring_mask] = SDMA_PKT_HEADER_OP(SDMA_OP_COPY) |
> +		SDMA_PKT_HEADER_SUB_OP(SDMA_SUBOP_COPY_LINEAR) |
> +		SDMA_PKT_COPY_LINEAR_HEADER_TMZ((0));

[Severity: High]
Can this sequence cause data races and ring buffer corruption?

This function is called from NAPI context by multiple RX queues concurrently,
but it increments the software write cursor (sdma->idx++) and writes to the
ring buffer without locks or atomic operations.

[Severity: High]
Are these descriptors written to the GPU in the correct byte order?

AMDGPU hardware typically expects ring buffer packets to be formatted in
little-endian. Writing multi-byte fields directly using native host
endianness might cause the hardware to interpret garbage commands on
big-endian systems. Should helpers like cpu_to_le32() be used here?

[ ... ]

> +static void knod_destroy_one_queue(struct knod *knod, int idx)
> +{
[ ... ]
> +	if (knod->aql_queue_created[idx]) {
> +		int ret;
> +
> +		ret = knod_destroy_queue(knod->process,
> +					 knod->aql_queue_id[idx]);

[Severity: High]
Does this manual queue teardown race with the lockless MMU notifier cleanup?

If the dummy /bin/sleep process is killed, mmu_notifier_release() will
call pqm_uninit() which locklessly iterates over and frees pqm->queues.

If the KNOD framework concurrently initiates cleanup via
knod_destroy_queue(), which locks p->mutex and deletes queues from the exact
same list, could this lead to list corruption and double-freeing of
process_queue_node structures?

[ ... ]

> +static int knod_alloc_one_queue(struct knod *knod, int idx,
> +				struct kfd_topology_device *topo_dev,
> +				struct kfd_process_device *pdd, void *ptr)
> +{
[ ... ]
> +	qp.write_ptr =
> +		(void __user *)((u64)knod->kaql[idx].queue->gaddr + 0x38);
> +	qp.read_ptr = (void __user *)((u64)knod->kaql[idx].queue->gaddr + 0x80);

[Severity: High]
Can casting a GPU virtual address to a user-space CPU address cause faults?

KFD queue management uses get_user() and put_user() on these pointers. Since
the raw GPU virtual address has no CPU mapping in the user process, won't
this trigger an -EFAULT page fault during queue creation or sync operations?

[ ... ]

> +static void knod_stop_worker(struct knod *knod)
> +{
> +	if (!knod->worker)
> +		return;
> +
> +	if (knod->flush_fn)
> +		knod->flush_fn(knod->worker_ctx);
> +
> +	kthread_stop(knod->worker);
> +	put_task_struct(knod->worker);

[Severity: High]
Is there a data race here that could lead to a double-free of the task struct?

knod_stop_worker() checks and modifies knod->worker without holding any
locks. If feature transitions and module unload invoke this concurrently, two
threads could simultaneously pass the if (!knod->worker) check.

Both would then call kthread_stop() and put_task_struct() on the same task
structure, potentially dropping the refcount to zero prematurely.

[ ... ]

> +static int knod_alloc_ctx_init(struct knod *knod, int id, void **doorbell,
> +			       struct kfd_topology_device **out_topo_dev,
> +			       struct kfd_process_device **out_pdd)
> +{
> +	struct kfd_topology_device *topo_dev;
> +	struct kfd_process_device *pdd;
> +	struct file *drm_file;
> +	size_t mem_size;
> +	char path[64];
> +	int err;
> +
> +	sprintf(path, "/dev/dri/renderD%d", id);
> +
> +	drm_file = filp_open(path, O_RDWR, 0);

[Severity: High]
Does this hardcoded VFS path lookup introduce a namespace spoofing risk?

The initialization path uses filp_open() from kernel space to obtain a struct
file for the DRM device, which resolves in the user-space mount namespace.

If an unprivileged user in a user namespace bind-mounts a malicious file
over /dev/dri/renderD0, could this lead to arbitrary kernel memory read/write
when AMDGPU casts filp->private_data without validating the inode?

[ ... ]

> +	pdd = knod->process->pdds[0];

[Severity: High]
Will unconditionally accessing pdds[0] fail on multi-GPU systems?

This hardcodes access to the first AMD GPU, regardless of the renderD node
passed to knod_alloc_ctx_init().

On a system with multiple GPUs, would this result in initializing the VM of
GPU 0 with the DRM file of a different GPU?

[ ... ]

> +err_unref_process:
> +	kfd_unref_process(knod->process);
> +err_filp_close:
> +	fput(knod->drm_file);
> +	return err;
> +}

[Severity: High]
Are the KFD process and MMU notifier leaked on this error path?

The setup code earlier takes two references to the KFD process, but
kfd_unref_process() is only called once here.

Additionally, kfd_process_notifier_release_internal() is omitted. Will this
leave the process permanently embedded in KFD's hash tables and mmu_notifier
tracking?

[ ... ]

> +struct knod *knod_alloc_ctx(struct knod_dev *knodev, int queue_cnt, int id,
> +			    int channels)
> +{
[ ... ]
> +	for (idx = 0; idx < knod->sdma_cnt; idx++) {
[ ... ]
> +		knod->sdma[idx].sdma = knod_alloc_mem(knod, PAGE_SIZE << 4,
> +			KFD_IOC_ALLOC_MEM_FLAGS_GTT |
> +			KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE |
> +			KFD_IOC_ALLOC_MEM_FLAGS_COHERENT |
> +			KFD_IOC_ALLOC_MEM_FLAGS_UNCACHED |
> +			KFD_IOC_ALLOC_MEM_FLAGS_EXECUTABLE);

[Severity: High]
Can the hardware engine execute garbage commands here?

Unlike the AQL queues which use memset() to clear the memory, the SDMA queue
memory (sdma, queue, and queue_signal) allocated here is not explicitly
zeroed.

When this queue is submitted to the hardware, could the command processor
read uninitialized read/write pointers and execute random garbage memory?

[ ... ]

> +	list_add_tail(&knod->list, &ctx_list);

[Severity: High]
Is it safe to modify the global ctx_list without holding a lock?

knod_alloc_ctx() adds the newly allocated context to the global ctx_list
via list_add_tail(), and knod_release_ctx() removes it via list_del().

Neither of these operations appears to be protected by spinlocks or mutexes,
which could lead to list corruption if multiple interfaces are attached or
detached concurrently.

[ ... ]

> +void knod_release_ctx(struct knod *knod)
> +{
> +	int idx;
> +
> +	list_del(&knod->list);
[ ... ]
> +	kfree(knod);
> +}

[Severity: High]
Does tearing down the context here leave the worker thread running?

Neither knod_exit() nor knod_release_ctx() calls knod_stop_worker().
If the module is unloaded while a knod context is still active, the active
knod->worker kthread might be left running in the background and could
immediately dereference the freed knod context, causing a Use-After-Free.

[ ... ]

> +static int knod_feature_activate(struct knod *knod)
> +{
[ ... ]
> +		/*
> +		 * Publish xdp_ops, then phase B: register the offload dev so
> +		 * user XDP progs/maps can bind (their install/free route
> +		 * through accel_ops.xdp_ops->xdp_install).
> +		 */
> +		WRITE_ONCE(accel_ops.xdp_ops, registered_xdp_ops);

[Severity: High]
Will this override the features for all devices system-wide?

accel_ops is a single static global instance shared among all registered
knod_accel contexts. Modifying accel_ops.xdp_ops during a per-device
feature activation will immediately alter the operations for all other
devices concurrently.

[ ... ]

> +static u32 knod_accel_d2h_submit(struct knod_dev *knodev, u64 dst, int queue,
> +				 u32 page_idx, u16 off, u32 len)
> +{
> +	struct knod *knod = knodev->accel->priv;
> +	struct knod_sdma_copy_desc c;
> +
> +	/* The netmem page_pool dma_addr is the NIC address; SDMA needs
> +	 * the GPU VM address, so derive the source from buf[queue].
> +	 */
> +	c.src = knod->buf[queue]->gaddr + ((u64)page_idx << PAGE_SHIFT) + off;
> +	c.dst = dst;
> +	c.len = len;
> +	return knod_sdma_submit(knod, 0, &c, 1);

[Severity: High]
Does hardcoding the SDMA queue index to 0 cause concurrent modifications?

knod_accel_d2h_submit() passes 0 for the SDMA queue index to
knod_sdma_submit().

Because this is called from NAPI context by multiple RX queues concurrently,
it will result in data races and corrupted SDMA ring buffers across multiple
CPUs.

[ ... ]

> +void knod_accel_xdp_register(struct knod_accel_xdp_ops *xdp_ops)
> +{
> +	int i;
> +
> +	/*
> +	 * Module load advertises the feature and sets up the permanent
> +	 * per-attach state (bpf_offload_dev) on already-attached accels;
> +	 * GPU compute resources wait for ->activate() on feature select.
> +	 */
> +	WRITE_ONCE(registered_xdp_ops, xdp_ops);

[Severity: Medium]
Is there a race condition with knod_attach() here due to a lack of locking?

knod_accel_xdp_register() writes to registered_xdp_ops and iterates over
accels[i]->knodev without synchronization.

If a device is attaching simultaneously, knod_attach() might read
registered_xdp_ops as NULL, while knod_accel_xdp_register() sees
accel->knodev before it is fully initialized, potentially missing the
initialization or crashing.

[ ... ]

> +int knod_init(struct amdgpu_device *adev)
> +{
> +	int base_id = adev_to_drm(adev)->render->index * KNOD_MAX_AQL;
> +	struct knod_accel *accel;
> +	int i, n;
> +
> +	n = KNOD_MAX_AQL;
> +
> +	for (i = 0; i < n; i++) {
> +		accel = kzalloc_obj(struct knod_accel, GFP_KERNEL);
> +		if (!accel) {
> +			pr_err("knod: Failed to allocate accel node %d\n", i);
> +			goto err;
> +		}
> +
> +		INIT_LIST_HEAD(&accel->list);
> +		accel->type = KNOD_TYPE_GPU;
> +		accel->accel_ops = &accel_ops;
> +		accel->owner = THIS_MODULE;
> +		accel->id = base_id + i;
> +		snprintf(accel->name, sizeof(accel->name), "amdgpu-%d", i);
> +		knod_accel_register(accel);
> +		accels[i] = accel;

[Severity: High]
Does this unintentionally overwrite the global accels array for system GPUs?

The global accels array is populated based on index i (which loops 0 to
n - 1, where n is KNOD_MAX_AQL which is 1). Since knod_init() is called
per-device, it will unconditionally store the allocated context at accels[0].

On systems with multiple AMD GPUs, this overwrites accels[0], leaking the
previous GPU's context and preventing its teardown later.

> +	}
> +	nr_accels = n;
> +
> +	return 0;
> +
> +err:
> +	while (i-- > 0) {
> +		knod_accel_unregister(accels[i]);
> +		kfree(accels[i]);
> +		accels[i] = NULL;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Should the error path return a negative error code instead of 0?

If kzalloc_obj() fails, the err label cleans up the allocations but
immediately returns 0. This masks the allocation failure from the upstream
caller, leading to silent failures and likely null-pointer dereferences
downstream.

> diff --git a/drivers/gpu/drm/amd/amdkfd/knod/kfd_knod.h b/drivers/gpu/drm/amd/amdkfd/knod/kfd_knod.h
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdkfd/knod/kfd_knod.h
> @@ -0,0 +1,270 @@
[ ... ]
> +union knod_aql_rsrc1 {
> +	struct {
> +#if defined(__LITTLE_ENDIAN)
> +		unsigned int base_address_hi : 16;
> +		unsigned int stride : 14;
> +		unsigned int cache_swizzle : 1;
> +		unsigned int swizzle_enable : 1;
> +#elif defined(__LITTLE_ENDIAN)

[Severity: Low]
Is the second elif directive a typo for big-endian architectures?

The conditional macro uses elif defined(__LITTLE_ENDIAN) again instead of
elif defined(__BIG_ENDIAN).

On a big-endian system, neither condition is met, leaving the struct
entirely empty and causing compilation errors.

[ ... ]

> +struct knod_aql {
> +	struct knod_mem *aql;
[ ... ]
> +	u64 *doorbell;
> +	int idx;
> +};

[Severity: High]
Can this signed 32-bit integer write pointer cause sign-extension corruption?

The AQL write pointer idx is declared as a signed int instead of a u64.
After enough operations, this pointer will overflow to a negative value.

In knod_setup_header(), this negative value will undergo sign-extension to
64 bits when written to the hardware doorbell via writeq(), which breaks the
hardware's monotonic expectation and violates AMDGPU subsystem rules.

[ ... ]

> +struct knod {
> +	struct list_head list;
[ ... ]
> +	struct knod_aql kaql[NR_CPUS];

[Severity: High]
Could this array sizing cause a heap buffer overflow?

The array fields for queue tracking are sized using NR_CPUS. However, in
knod_alloc_ctx(), the loop iterates up to queue_cnt, which can be up to
KNOD_MAX_QUEUE_CNT (32).

On kernel configurations where NR_CPUS is less than 32, this loop will
write out-of-bounds.

[ ... ]

> +static inline void
> +knod_setup_header(struct knod *knod,
> +		  const struct knod_dispatch_params *p, int q_idx)
> +{
> +	struct amd_queue *amd_queue = (struct amd_queue *)knod->kaql[q_idx].amd_queue->kaddr;
> +	int curr_idx = knod->kaql[q_idx].idx;
> +	int next_idx = curr_idx + 1;
> +	u64 *ptr = knod->kaql[q_idx].doorbell;
> +
> +	knod_setup_invalidate(knod, next_idx % knod->nr_aql_ring, q_idx);
> +	knod_setup_dispatch(knod, curr_idx % knod->nr_aql_ring, p, q_idx);
> +	WRITE_ONCE(amd_queue->write_dispatch_id, curr_idx);
> +	writeq(curr_idx, ptr);

[Severity: High]
Is a memory barrier needed before writing to the AQL doorbell?

While WRITE_ONCE prevents compiler reordering, it does not prevent CPU
reordering. The CPU can reorder the doorbell write so it becomes visible to
the hardware before the write_dispatch_id store reaches memory.

Should there be a wmb() before the writeq() to ensure the GPU does not
observe a stale dispatch ID?

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