From: AI Reviewer <redacted>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
octeontx2: offload host FIB updates to switch via AF mailbox

This change wires up an IPv4/IPv6 FIB offload path: the switch PF pushes
fib_entry batches to the RVU AF via a fib_notify mbox, and the AF forwards
them back down to the switch PF via a new af2swdev FIB_CMD.  A pair of
workqueues on both ends batch and post the entries.

Some questions follow inline.

[Critical]
Can this shrink cause an out-of-bounds write in
rvu_sw_l3_offl_rule_push()?  The AF-side handler still accepts up to 16
entries per fib_notify:

    if (req->cnt > 16)
        return -EINVAL;

and the batching guard in sw_l3_offl_work_handler()

    if (lcnt + l3_entry->cnt > 8 && !list_empty(&l3_local_lh))

only splits when the local list is already non-empty, so the first
l3_entry in a batch is always accepted regardless of its cnt.  A single
fib_notify with cnt in [13,16] therefore reaches

    memcpy(dst, entry, sizeof(*entry) * cnt);

writing up to 16 struct fib_entry into req->entry[12], and overflows the
mbox message by 1..4 entries (roughly 60..240 bytes) into adjacent mbox
memory.

Also, req->cnt is a u8 while tot_cnt is int, so any total that batches
past 255 would silently truncate on the assignment

    req->cnt = tot_cnt;

Would it be clearer to either raise entry[] back to 16, cap fib_notify at
12, or clamp/split at the memcpy site with tot_cnt validated against a
u8?

[ ... ]

+static struct workqueue_struct *sw_l3_offl_wq;
+
+struct l3_entry {
+	struct list_head list;
+	struct rvu *rvu;
+	u32 port_id;
+	int cnt;
+	struct fib_entry entry[];
+};
+
+static DEFINE_MUTEX(l3_offl_llock);
+static LIST_HEAD(l3_offl_lh);
+static bool l3_offl_work_running;
+
+static struct workqueue_struct *sw_l3_offl_wq;

[Low]
Is the second declaration of sw_l3_offl_wq intentional?  It appears once
above the struct l3_entry definition and again immediately after, so the
translation unit has two tentative definitions of the same object.

+static void sw_l3_offl_work_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(l3_offl_work, sw_l3_offl_work_handler);
+
+static int rvu_sw_l3_offl_rule_push(struct list_head *lh)
+{

[ ... ]

+	dst = &req->entry[0];
+	while ((l3_entry =
+		list_first_entry_or_null(lh,
+					 struct l3_entry, list)) != NULL) {
+		entry = l3_entry->entry;
+		cnt = l3_entry->cnt;
+		sz = sizeof(*entry) * cnt;
+
+		memcpy(dst, entry, sz);
+		tot_cnt += cnt;
+		dst += cnt;
+
+		list_del_init(&l3_entry->list);
+		kfree(l3_entry);
+	}
+	req->flags = FIB_CMD;
+	req->cnt = tot_cnt;
+
+	rc = otx2_mbox_wait_for_zero(&rvu->afpf_wq_info.mbox_up, swdev_pf);
+	if (rc)
+		otx2_mbox_msg_send_up(&rvu->afpf_wq_info.mbox_up, swdev_pf);
+
+	mutex_unlock(&rvu->mbox_lock);
+	return rc ? 0 : -EFAULT;
+}

[High]
When otx2_mbox_wait_for_zero() returns false (mbox still busy after the
~1ms wait), otx2_mbox_msg_send_up() is skipped but the loop above has
already drained every l3_entry from lh and kfree()d them.

Does this silently drop the batch?  The allocated mbox message is left
dangling in the up-channel, the fib_entry payload has been freed, and
the caller only logs pr_err() without any re-queue, so the switch state
appears to permanently diverge from the host FIB whenever the mbox is
momentarily busy.

+static atomic64_t req_cnt;
+static atomic64_t ack_cnt;
+static atomic64_t req_processed;
+static LIST_HEAD(l3_local_lh);
+static int lcnt;
+
+static void sw_l3_offl_work_handler(struct work_struct *work)
+{
+	struct l3_entry *l3_entry;
+	struct list_head l3lh;
+	u64 req, ack, proc;
+
+	INIT_LIST_HEAD(&l3lh);
+
+	mutex_lock(&l3_offl_llock);
+	while (1) {
+		l3_entry = list_first_entry_or_null(&l3_offl_lh, struct l3_entry, list);
+
+		if (!l3_entry)
+			break;
+
+		if (lcnt + l3_entry->cnt > 8 && !list_empty(&l3_local_lh)) {
+			req = atomic64_read(&req_cnt);
+			atomic64_set(&ack_cnt, req);
+			atomic64_set(&req_processed, req);
+			mutex_unlock(&l3_offl_llock);
+			goto process;
+		}
+
+		lcnt += l3_entry->cnt;
+
+		atomic64_inc(&req_cnt);
+		list_del_init(&l3_entry->list);
+		list_add_tail(&l3_entry->list, &l3_local_lh);
+	}
+	mutex_unlock(&l3_offl_llock);
+
+	req = atomic64_read(&req_cnt);
+	ack = atomic64_read(&ack_cnt);
+
+	if (req > ack) {
+		atomic64_set(&ack_cnt, req);
+		queue_delayed_work(sw_l3_offl_wq, &l3_offl_work,
+				   msecs_to_jiffies(100));
+		return;
+	}
+
+	proc = atomic64_read(&req_processed);
+	if (req == proc) {
+		queue_delayed_work(sw_l3_offl_wq, &l3_offl_work,
+				   msecs_to_jiffies(1000));
+		return;
+	}
+
+	atomic64_set(&req_processed, req);

[Medium]
Do req_cnt, ack_cnt, and req_processed model any real acknowledgement
protocol?  They are only ever written from inside this function and
neither otx2_mbox_wait_for_zero() nor otx2_mbox_msg_send_up() touches
them, so the (req > ack) branch effectively only ever fires on the first
invocation after new entries arrive and then

    atomic64_set(&ack_cnt, req);

immediately equalizes them.  Would this be clearer as a plain "if new
work arrived, re-poll shortly, otherwise poll slowly" without the ack
counters?

[Medium]
Do the two early-return branches leak the entries parked in l3_local_lh?

The while loop above moves items from l3_offl_lh into the module-static
l3_local_lh, and then both

    if (req > ack) { ... return; }
    if (req == proc) { ... return; }

return without splicing l3_local_lh into l3lh for processing.  If
rvu_sw_l3_shutdown() then runs, it only drains l3_offl_lh:

    while (1) {
        entry = list_first_entry_or_null(&l3_offl_lh,
                                         struct l3_entry, list);

so entries parked in l3_local_lh remain allocated.  Would it be safer to
splice l3_local_lh back into l3_offl_lh before returning, or drain both
lists in the shutdown path?

+
+process:
+	lcnt = 0;

[ ... ]

+int rvu_mbox_handler_fib_notify(struct rvu *rvu,
 				struct fib_notify_req *req,
 				struct msg_rsp *rsp)
 {
+	struct l3_entry *l3_entry;
+	int sz;
+
+	if (!(rvu->rswitch.flags & RVU_SWITCH_FLAG_FW_READY))
+		return 0;

[High]
Should this return an error instead of 0 when firmware is not ready?
Returning 0 tells the caller the update was accepted, but nothing is
buffered or resynced later, so any FIB event that arrives during the
pre-ready window is silently dropped.  Because FIB deltas are
edge-triggered, a single missed add/del leaves the offload permanently
divergent from the host FIB with no user-visible signal.

+
+	if (req->cnt > 16)
+		return -EINVAL;
+
+	sz = req->cnt * sizeof(struct fib_entry);
+
+	l3_entry = kcalloc(1, sizeof(*l3_entry) + sz, GFP_KERNEL);
+	if (!l3_entry)
+		return -ENOMEM;
+
+	l3_entry->port_id = rvu_sw_port_id(rvu, req->hdr.pcifunc);
+	l3_entry->rvu = rvu;
+	l3_entry->cnt = req->cnt;
+	INIT_LIST_HEAD(&l3_entry->list);
+	memcpy(l3_entry->entry, req->entry, sz);
+
+	mutex_lock(&l3_offl_llock);
+	list_add_tail(&l3_entry->list, &l3_offl_lh);
+	mutex_unlock(&l3_offl_llock);
+
+	if (!l3_offl_work_running) {
+		sw_l3_offl_wq = alloc_workqueue("sw_af_fib_wq", 0, 0);
+		l3_offl_work_running = true;
+		queue_delayed_work(sw_l3_offl_wq, &l3_offl_work,
+				   msecs_to_jiffies(1000));
+	}
+
 	return 0;
 }

[High]
Is the return value of alloc_workqueue() handled anywhere?  Under memory
pressure it can return NULL and queue_delayed_work() will then
dereference a NULL pool.  Additionally l3_offl_work_running is set to
true unconditionally, so on the next invocation the branch is skipped
and every future fib_notify piles a new l3_entry onto l3_offl_lh with
no worker to drain it.

[High]
Is this check-then-set race-safe?  It runs outside l3_offl_llock, and
mbox handlers can dispatch from separate PF worker contexts.  Two
concurrent invocations can both observe l3_offl_work_running == false,
both call alloc_workqueue(), and one of the pointers is then overwritten
into sw_l3_offl_wq while the other workqueue leaks along with any
delayed work already queued on it.

Related: rvu_sw_l3_shutdown() destroys sw_l3_offl_wq but does not clear
sw_l3_offl_wq to NULL or reset l3_offl_work_running:

+void rvu_sw_l3_shutdown(void)
+{
+	struct l3_entry *entry;
+	LIST_HEAD(tlist);
+
+	if (!sw_l3_offl_wq)
+		return;
+
+	cancel_delayed_work_sync(&l3_offl_work);
+	destroy_workqueue(sw_l3_offl_wq);
+
+	mutex_lock(&l3_offl_llock);
+	while (1) {
+		entry = list_first_entry_or_null(&l3_offl_lh,
+						 struct l3_entry, list);
+		if (!entry)
+			break;
+
+		list_del_init(&entry->list);
+		kfree(entry);
+	}
+	mutex_unlock(&l3_offl_llock);
+}

After shutdown, if rvu_mbox_handler_fib_notify() runs again it sees
l3_offl_work_running still true and calls queue_delayed_work() on the
destroyed workqueue.  Would it help to move the workqueue lifetime to
subsystem init/deinit (like rvu_sw_l2) and always clear both pieces of
state here?

[ ... ]

+static void sw_fib_work_handler(struct work_struct *work)
+{
+	struct sw_fib_list_entry *lentry;
+	LIST_HEAD(tlist);
+
+	spin_lock_bh(&sw_fib_llock);
+	list_splice_init(&sw_fib_lh, &tlist);
+	spin_unlock_bh(&sw_fib_llock);
+
+	while ((lentry =
+		list_first_entry_or_null(&tlist,
+					 struct sw_fib_list_entry, lh)) != NULL) {
+		list_del_init(&lentry->lh);
+		sw_fib_notify(lentry->pf, lentry->cnt, lentry->entry);
+		kfree(lentry->entry);
+		kfree(lentry);
+	}

[High]
Should the return value of sw_fib_notify() be checked?  It can fail via
cnt > 16 returning -EINVAL, mbox alloc returning -ENOMEM, or
otx2_sync_mbox_msg() transport failure, and all three paths simply drop
the FIB delta with no retry and no diagnostic.  Because FIB updates are
edge-triggered, a single lost message leaves the switch state
permanently out of sync.

+
+	spin_lock_bh(&sw_fib_llock);
+	if (!list_empty(&sw_fib_lh))
+		queue_delayed_work(sw_fib_wq, &sw_fib_work,
+				   msecs_to_jiffies(10));
+	spin_unlock_bh(&sw_fib_llock);
+}
+
+int sw_fib_add_to_list(struct net_device *dev,
+		       struct fib_entry *entry, int cnt)
+{
+	struct otx2_nic *pf = netdev_priv(dev);
+	struct sw_fib_list_entry *lentry;
+
+	lentry = kcalloc(1, sizeof(*lentry), GFP_ATOMIC);
+	if (!lentry)
+		return -ENOMEM;

[High]
Does this leak the caller's entry on ENOMEM?  Every call site in this
patch (sw_nb_v4_netdev_event, sw_nb_v4_inetaddr_event,
sw_nb_v4_fib_event x2, sw_nb_net_v4_neigh_update, sw_nb_v6_netdev_event,
sw_nb_v6_fib_event, sw_nb_net_v6_neigh_update, sw_nb_v6_inetaddr_event)
hands ownership of the entry to this function and drops the reference:

    -   kfree(entry);
    +   sw_fib_add_to_list(pf_dev, entry, 1);

The return value is ignored in every caller, so on GFP_ATOMIC failure
the fib_entry buffer is stranded.  Would it make sense to kfree(entry)
on the error path (or take ownership only after the lentry allocation
succeeds)?

[Medium]
Is there a cnt upper bound?  sw_fib_notify() rejects cnt > 16 but
sw_fib_add_to_list() does not, and sw_nb_v4_fib_event() computes

    cnt = iter - entries;

bounded by fi->fib_nhs.  For multipath configurations with more than 16
next-hops the entry is happily queued and then silently discarded inside
the worker.  Should the notifier either split the batch or refuse early
so the sender can adjust?

+
+	lentry->pf = pf;
+	lentry->cnt = cnt;
+	lentry->entry = entry;
+	INIT_LIST_HEAD(&lentry->lh);
+
+	spin_lock(&sw_fib_llock);
+	list_add_tail(&lentry->lh, &sw_fib_lh);
+	queue_delayed_work(sw_fib_wq, &sw_fib_work,
+			   msecs_to_jiffies(10));
+	spin_unlock(&sw_fib_llock);
+
+	return 0;
+}

[High]
Is sw_fib_llock protected against softirq reentry here?  This function
uses plain spin_lock()/spin_unlock() but sw_fib_work_handler() and
sw_fib_deinit() both acquire the same lock with spin_lock_bh().

sw_nb_net_v4_neigh_update() and sw_nb_net_v6_neigh_update() are called
from the netevent chain, which the neighbour subsystem can invoke in
softirq context (e.g., from __neigh_update() paths driven by RX
processing).  If CPU X is holding sw_fib_llock via spin_lock() in
process context and a softirq on the same CPU reaches
sw_fib_add_to_list() as well, the softirq spins waiting on a lock that
only its preempted process context can release.

Would spin_lock_bh() be more appropriate here?

[ ... ]

+void sw_fib_deinit(void)
 {
+	struct sw_fib_list_entry *lentry;
+	LIST_HEAD(tlist);
+
+	cancel_delayed_work_sync(&sw_fib_work);
+	destroy_workqueue(sw_fib_wq);
+
+	spin_lock_bh(&sw_fib_llock);
+	list_splice_init(&sw_fib_lh, &tlist);
+	spin_unlock_bh(&sw_fib_llock);
+
+	while ((lentry =
+		list_first_entry_or_null(&tlist,
+					 struct sw_fib_list_entry, lh)) != NULL) {
+		list_del_init(&lentry->lh);
+		kfree(lentry->entry);
+		kfree(lentry);
+	}
 }

[High]
Is there anything in this module that prevents sw_fib_add_to_list() from
being called during or after sw_fib_deinit()?  The current caller order
in sw_nb_unregister() unregisters notifiers first, but sw_fib.c has no
self-contained shutdown flag or nulling of sw_fib_wq.  A stray notifier
callback that reaches sw_fib_add_to_list() after destroy_workqueue()
would call queue_delayed_work() on a freed workqueue pointer.  Would
setting sw_fib_wq to NULL under the lock and checking it in
sw_fib_add_to_list() be robust against future ordering changes?

[High]
Does this drop the guard on the wrong caller?  The other callers of the
v6 helpers in this file (register/unregister, sw_nb_v6_fib_event,
sw_nb_v6_inetaddr_event dispatch, netevent v6 dispatch) still sit under
their own IS_ENABLED(CONFIG_IPV6) blocks, and drivers/net/ethernet/
marvell/octeontx2/nic/Makefile only compiles sw_nb_v6.o under

    ifdef CONFIG_IPV6

so sw_nb_v6.c's body is wrapped in the same #if.  On
CONFIG_OCTEONTX_SWITCH=y with CONFIG_IPV6=n, Kconfig does not select or
depend on IPV6, so the reference to sw_nb_v6_netdev_event() would remain
in the object file with no definition to link against.  Should this
guard be restored, or should sw_nb_v6.h grow a static inline stub for
!CONFIG_IPV6?

[High]
Not introduced by this patch, but the switch to sw_fib_add_to_list()
lengthens the exposure of a pre-existing kcalloc(GFP_ATOMIC) that is
never NULL-checked:

    entry = kcalloc(1, sizeof(*entry), GFP_ATOMIC);
    entry->cmd = sw_nb_inetaddr_event_to_otx2_event(event, dev);

Previously a NULL entry crashed here at the immediate dereference;
now the pointer also gets stored in a work item.  The same pattern
exists in sw_nb_net_v4_neigh_update(), sw_nb_v6_netdev_event()
(GFP_KERNEL), sw_nb_net_v6_neigh_update(), and
sw_nb_v6_inetaddr_event().  Could the NULL checks be added while
touching these paths?
-- 
This is an AI-generated review.