Re: [PATCH net-next v3 09/12] dpaa2-switch: offload FDBs added on an upper bond device
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-06 02:10:56
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- dpaa2-switch: offload FDBs added on an upper bond device This patch adds support for offloading FDB entries added on upper bond devices.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c index fb754dae80697..fadebf6a758d1 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -616,6 +640,91 @@ static int dpaa2_switch_port_fdb_del(struct ethsw_port_priv *port_priv, return dpaa2_switch_port_fdb_del_mc(port_priv, addr); } +static struct dpaa2_mac_addr * +dpaa2_switch_mac_addr_find(struct list_head *addr_list, + const unsigned char *addr, u16 vid) +{ + struct dpaa2_mac_addr *a; + + list_for_each_entry(a, addr_list, list) + if (ether_addr_equal(a->addr, addr) && a->vid == vid) + return a; + + return NULL; +} + +static int dpaa2_switch_lag_fdb_add(struct dpaa2_switch_lag *lag, + const unsigned char *addr, u16 vid) +{ + struct ethsw_port_priv *port_priv; + struct dpaa2_mac_addr *a; + int err = 0; + + mutex_lock(&lag->fdb_lock); + + a = dpaa2_switch_mac_addr_find(&lag->fdbs, addr, vid); + if (a) { + refcount_inc(&a->refcount);
[Severity: High] If a port leaves the LAG, the fan-out for the FDB delete event will only reach the remaining ports. Could this result in the refcount decrementing fewer times than it incremented, preventing it from reaching zero and permanently leaking the struct dpaa2_mac_addr?
+ goto out; + } + + port_priv = dpaa2_switch_lag_get_primary(lag);
[Severity: High] The FDB entry is programmed into hardware using this primary port's physical interface ID. If this primary port leaves the LAG, is there a mechanism to migrate the existing FDB entries to one of the remaining ports? Without migration, could traffic be misrouted, and would subsequent deletions fail because they target the new primary port's ID?
+ if (!port_priv) {
+ err = -ENOENT;
+ goto out;
+ }
+
+ a = kzalloc(sizeof(*a), GFP_KERNEL);[ ... ]
quoted hunk ↗ jump to hunk
@@ -2856,32 +2996,46 @@ struct ethsw_switchdev_event_work { struct work_struct work; struct switchdev_notifier_fdb_info fdb_info; struct net_device *dev; + struct net_device *orig_dev; unsigned long event; + u16 vid; }; static void dpaa2_switch_event_work(struct work_struct *work) { struct ethsw_switchdev_event_work *switchdev_work = container_of(work, struct ethsw_switchdev_event_work, work); + struct net_device *orig_dev = switchdev_work->orig_dev; struct net_device *dev = switchdev_work->dev; + struct ethsw_port_priv *port_priv = netdev_priv(dev); struct switchdev_notifier_fdb_info *fdb_info; + struct dpaa2_switch_lag *lag; int err; rtnl_lock(); fdb_info = &switchdev_work->fdb_info; + lag = rtnl_dereference(port_priv->lag); switch (switchdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: - err = dpaa2_switch_port_fdb_add(netdev_priv(dev), - fdb_info->addr); + if (lag) + err = dpaa2_switch_lag_fdb_add(lag, fdb_info->addr, + switchdev_work->vid); + else + err = dpaa2_switch_port_fdb_add(netdev_priv(dev), + fdb_info->addr); if (err) break; fdb_info->offloaded = true; - call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev, + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, orig_dev, &fdb_info->info, NULL); break; case SWITCHDEV_FDB_DEL_TO_DEVICE: - dpaa2_switch_port_fdb_del(netdev_priv(dev), fdb_info->addr); + if (lag) + dpaa2_switch_lag_fdb_del(lag, fdb_info->addr, + switchdev_work->vid); + else + dpaa2_switch_port_fdb_del(port_priv, fdb_info->addr);
[Severity: High] Is it possible for a port to be removed from a bond between the queuing of this event and its execution? If port_priv->lag becomes NULL during this window, the worker would route the deletion to the standalone path instead of the LAG path. Could this bypass the LAG refcount decrement and leak the MAC address?
break; }
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3903,8 +4073,10 @@ static void dpaa2_switch_remove(struct fsl_mc_device *sw_dev) for (i = 0; i < DPAA2_SWITCH_RX_NUM_FQS; i++) netif_napi_del(ðsw->fq[i].napi); - for (i = 0; i < ethsw->sw_attr.num_ifs; i++) + for (i = 0; i < ethsw->sw_attr.num_ifs; i++) { dpaa2_switch_remove_port(ethsw, i); + mutex_destroy(ðsw->lags[i].fdb_lock); + }
[Severity: High] Are the dynamically allocated struct dpaa2_mac_addr entries in the lag->fdbs list freed during driver teardown? It appears ethsw->lags is freed shortly after this without iterating through the fdbs lists, which might cause a memory leak of all active LAG FDB entries. Could this also happen if all ports leave a LAG and it becomes unused, inheriting stale entries upon reuse?
kfree(ethsw->fdbs); kfree(ethsw->filter_blocks);