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 <[email protected]> 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/[email protected]?part=1
