Re: [RFC PATCH net-next 01/13] net: knod: add uapi and core headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Broken lock-free API: `spsc_push` and `spsc_pop` introduce data races.
- [High] Use-After-Free in `knod_napi_kick()` due to lifecycle mismatch with standard NAPI teardown.
- [High] NULL pointer dereference in inline helpers when no accelerator is attached.
- [High] `spsc_acquire()` lacks a Write Memory Barrier (`wmb()`) before publishing the advanced cursor, risking data corruption on WC memory.
- [Medium] The `attach` Netlink operation violates uAPI design guidelines by missing reply attributes.
- [Medium] Severe false sharing in `struct spsc_ring` due to unpadded producer and consumer cursors.
- [Low] Missing `n` parameter in `for_each_xdev_safe` macro.
--

--- Patch [1]: [RFC PATCH net-next 01/13] net: knod: add uapi and core headers ---
commit 518fe6d810fd2f47140b3bb011b370cf924724ed
Author: Taehee Yoo <ap420073@gmail.com>

net: knod: add uapi and core headers

Add the uAPI and core headers for KNOD, an in-kernel network offload
device that connects a NIC RX path to an accelerator (e.g. a GPU) for
zero-copy packet processing.

> diff --git a/Documentation/netlink/specs/knod.yaml b/Documentation/netlink/specs/knod.yaml
> new file mode 100644
> index 0000000000000..7c64df4676349
> --- /dev/null
> +++ b/Documentation/netlink/specs/knod.yaml
[ ... ]
> @@ -0,0 +1,176 @@
> +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> +---
> +name: knod
[ ... ]
> +    -
> +      name: attach
> +      doc: Attach a NIC netdevice to an accelerator.
> +      attribute-set: dev
> +      flags: [admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - nic-ifindex
> +            - accel-id
> +        reply:
> +          attributes: []

[Severity: Medium]
UAPI design rules state that NEW/ADD operations must reply with a full
message describing the new object. By leaving attributes empty, does this
permanently freeze a broken ABI for userspace applications relying on generic
netlink?

> +    -
> +      name: detach
> +      doc: Detach a NIC netdevice from its accelerator.
> +      attribute-set: dev
[ ... ]
> diff --git a/include/net/knod.h b/include/net/knod.h
> new file mode 100644
> index 0000000000000..50333b0e028b7
> --- /dev/null
> +++ b/include/net/knod.h
[ ... ]
> @@ -0,0 +1,467 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __NET_KNOD_H
> +#define __NET_KNOD_H
[ ... ]
> +struct knod_work_priv {
> +	struct dma_buf *dmabuf;
> +	netmem_ref *netmems;
> +	unsigned int *data_lens;
> +	int *data_offs;
> +	int cnt;
> +	int index;
> +	struct napi_struct *napi;
> +	struct spsc_ring spsc_bds;
> +	void *spsc_pool_priv;	/* accel driver priv for spsc pool memory */
> +	u64 spsc_pool_gaddr;	/* device-visible address of spsc pool */
> +	/* framework-owned delivery pool */
> +	struct page_pool *pass_pool;
> +	/* provider ctx (owner storage) */
> +	struct page_pool_hostmem pass_hm;
> +	/* d2h: SDMA-issued, awaiting drain */
> +	struct spsc_ring pass_pending;
> +} ____cacheline_aligned_in_smp;
> +
> +static inline void knod_napi_kick(struct knod_work_priv *wpriv)
> +{
> +	struct napi_struct *napi;
> +
> +	rcu_read_lock();
> +	napi = READ_ONCE(wpriv->napi);
> +	if (napi)
> +		napi_schedule(napi);
> +	rcu_read_unlock();
> +}

[Severity: High]
Does this RCU lock actually protect the NAPI structure from being freed?
Standard NIC drivers often dismantle and free their queue arrays synchronously
after napi_disable(), without an RCU grace period. Could this lead to a
use-after-free when calling napi_schedule()?

> +
> +#define KNOD_DEFAULT_PASS_SLOTS	64
> +
> +/*
[ ... ]
> +struct knod_dev {
> +	struct list_head list;
> +	struct knod_netdev *knetdev;
> +	struct knod_accel *accel;
[ ... ]
> +};
> +
> +static inline bool knod_dev_active(struct knod_dev *knodev)
> +{
> +	return !!knodev->accel->xdp.xdp_hw.prog;
> +}
> +
> +static inline struct bpf_prog *
> +knod_dev_offloaded(struct knod_dev *knodev)
> +{
> +	return knodev->accel->xdp.bpf_offloaded;
> +}

[Severity: High]
Can knodev->accel be NULL when a NIC is registered but detached from an
accelerator? If so, will these inline helpers unconditionally dereference a
NULL pointer and cause a kernel panic?

> +
> +static inline void knod_dev_offload(struct knod_dev *knodev,
> +					   struct bpf_prog *bpf_offloaded)
[ ... ]
> +extern struct list_head knod_accel_list;
> +
> +#define for_each_xdev(d)         \
> +		list_for_each_entry(d, &knod_dev_list, list)
> +#define for_each_xdev_safe(d)         \
> +		list_for_each_entry_safe(d, n, &knod_dev_list, list)

[Severity: Low]
Is the n parameter missing from the macro signature? Any file using this
macro without a pre-declared local variable n will likely fail to compile.

> +#define for_each_nodev(d)         \
> +		list_for_each_entry(d, &knod_netdev_list, list)
[ ... ]
> diff --git a/include/net/spsc_ring.h b/include/net/spsc_ring.h
> new file mode 100644
> index 0000000000000..1eccadc927d62
> --- /dev/null
> +++ b/include/net/spsc_ring.h
[ ... ]
> @@ -0,0 +1,645 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
[ ... ]
> +struct spsc_ring {
> +	void		**slots;	/* pointer-per-slot into pool */
> +	unsigned int	mask;		/* capacity - 1 */
> +
> +	/* Producer side */
> +	unsigned int	head;		/* next slot to publish */
> +
> +	/* Consumer side */
> +	unsigned int	tail;		/* oldest unconsumed slot */
> +	unsigned int	acquired;	/* sliding window read cursor
> +					 *   tail <= acquired <= head
> +					 */
> +
> +	/* Element pool */
> +	struct page	*pool_page;	/* compound page backing elements */

[Severity: Medium]
Does placing the heavily contended producer cursor (head) and consumer
cursors (tail, acquired) in the same cacheline without padding cause severe
false sharing? This could significantly impact throughput due to continuous
cacheline bouncing.

> +	unsigned int	pool_order;	/* page order */
> +	unsigned int	elem_stride;	/* cacheline-aligned element size */
> +} ____cacheline_aligned_in_smp;
> +
> +/* ================================================================== */
[ ... ]
> +static inline int spsc_acquire(struct spsc_ring *r, void **out,
> +			       unsigned int max, unsigned int *cnt)
> +{
> +	/* acquire the producer's head; slots it published are now visible */
> +	unsigned int head = smp_load_acquire(&r->head);
> +	unsigned int acq = r->acquired;
> +	unsigned int avail;
> +	unsigned int i;
[ ... ]
> +	if (out) {
> +		for (i = 0; i < avail; i++)
> +			out[i] = r->slots[(acq + i) & r->mask];
> +	}
> +
> +	/* Publish the window to the releasing consumer (a different CPU than
> +	 * this acquirer): pair with the smp_load_acquire() in spsc_release()
> +	 * so it cannot observe the advanced cursor before the element stores
> +	 * (e.g. an accel verdict) those slots now point at.
> +	 */
> +	smp_store_release(&r->acquired, acq + avail);

[Severity: High]
On architectures like x86, smp_store_release is only a compiler barrier and
does not flush Write-Combining buffers. Is a wmb() needed here before
publishing the advanced cursor, similar to spsc_produce_commit, to prevent a
concurrent releaser from reading stale memory before the WC stores land?

> +
> +	if (cnt)
> +		*cnt = avail;
> +	return 0;
> +}
> +
> +/**
[ ... ]
> +static inline int spsc_push(struct spsc_ring *r, void **out)
> +{
> +	int ret;
> +
> +	ret = spsc_produce(r, out);
> +	if (ret)
> +		return ret;
> +
> +	spsc_produce_commit(r);
> +	return 0;
> +}

[Severity: High]
Doesn't spsc_produce_commit publish the slot to the consumer immediately? If
the slot is published before the caller writes the data into the pointer
returned in out, could a concurrent consumer read uninitialized data?

> +
> +/**
> + * spsc_pop - dequeue one element
> + * @r:   ring buffer
> + * @out: receives pointer to the consumed element
> + *
> + * The element pointer remains valid until the next spsc_push() or
> + * spsc_produce() reuses that slot.
> + *
> + * Returns 0 on success, -ENOENT if empty.
> + */
> +static inline int spsc_pop(struct spsc_ring *r, void **out)
> +{
> +	/* acquire the producer's head; slots it published are now visible */
> +	unsigned int head = smp_load_acquire(&r->head);
> +	unsigned int tail = r->tail;
> +
> +	if (tail == head)
> +		return -ENOENT;
> +
> +	*out = r->slots[tail & r->mask];
> +
> +	r->acquired = tail + 1;
> +	/* release: hand the consumed slot back to the producer */
> +	smp_store_release(&r->tail, tail + 1);
> +
> +	return 0;
> +}

[Severity: High]
By advancing the tail cursor before returning the element pointer to the
consumer, is the slot made available to the producer too early? A concurrent
producer might overwrite the data while the consumer is actively reading it.

> +
> +/**
> + * spsc_pop_n - dequeue up to @n elements
[ ... ]

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