Re: [PATCH v2 net-next 6/9] octeontx2-pf: register switch notifiers for eswitch offload

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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-pf: register switch notifiers for eswitch offload

This change registers switchdev, FIB, netevent, inetaddr, inet6addr,
and netdevice notifiers when devlink switch mode is enabled, adds
sw_nb_v4.c/sw_nb_v6.c for family-specific handlers, and extends
sw_nb.c with device filtering for Cavium ports behind bridges and
VLANs.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 257a2ae6a53e..e4c01ac87477 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -399,6 +400,9 @@ static void rvu_rep_get_stats64(struct net_device *dev,
>  
>  static int rvu_eswitch_config(struct otx2_nic *priv, u8 ena)
>  {
> +#if IS_ENABLED(CONFIG_OCTEONTX_SWITCH)
> +	struct net_device *netdev = priv->netdev;
> +#endif
>  	struct devlink_port_attrs attrs = {};
>  	struct esw_cfg_req *req;
>  
> @@ -414,6 +418,11 @@ static int rvu_eswitch_config(struct otx2_nic *priv, u8 ena)
>  	memcpy(req->switch_id, attrs.switch_id.id, attrs.switch_id.id_len);
>  	otx2_sync_mbox_msg(&priv->mbox);
>  	mutex_unlock(&priv->mbox.lock);
> +
> +#if IS_ENABLED(CONFIG_OCTEONTX_SWITCH)
> +	ena ? sw_nb_register(netdev) : sw_nb_unregister(netdev);
> +#endif
> +
>  	return 0;
>  }

[Medium]
Is the return value from sw_nb_register() and sw_nb_unregister()
intentionally discarded here?

sw_nb_register() has six register_*_notifier() calls and a full
err1..err5 unwind path, but the ternary drops any error and the caller
returns 0.  The mbox esw_cfg_req has already been committed to the AF
before this line runs, so on notifier registration failure the AF sees
switch mode enabled while the PF has no observation infrastructure,
and devlink still reports success.

[Medium]
The notifier_block instances (sw_nb_fdb, sw_nb_fib, sw_nb_netevent,
sw_nb_v4_inetaddr, sw_nb_v6_inetaddr, sw_nb_netdev) and sw_nb_pf_netdev
are file-scope singletons in sw_nb.c.  When devlink switch mode is
enabled on a second octeontx2 PF (multi-port CN10K parts have a devlink
instance per PF), sw_nb_register() re-registers the same static
notifier_block pointers.

For notifier chains that keep a single linked list of nodes, adding the
same node twice either fails with -EEXIST (silently, per the previous
comment) or corrupts the chain.  sw_nb_pf_netdev is also unconditionally
overwritten so any netdev_info/netdev_dbg(sw_nb_pf_netdev, ...) is
attributed to the wrong PF, and once the first PF's netdev is freed
the pointer becomes a use-after-free for any callback that still
dereferences it.

Should these notifier_blocks live in a per-PF structure instead?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb.c b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb.c
> index 2d14a0590c5d..fb183265a9aa 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb.c

[ ... ]

> +bool sw_nb_is_valid_dev(struct net_device *netdev)
> +{
> +	struct netdev_nested_priv priv;
> +	struct net_device *br;
> +	int cnt = 0;
> +
> +	priv.flags = true;
> +	priv.data = &cnt;
> +
> +	if (netif_is_bridge_master(netdev) || is_vlan_dev(netdev)) {
> +		/* sw_nb_is_valid_dev() is invoked in notifier callback context, so
> +		 * rtnl_lcok is already acquired
> +		 */
> +		netdev_walk_all_lower_dev(netdev, sw_nb_check_slaves, &priv);
> +		return priv.flags && !!*(int *)priv.data;
> +	}
> +
> +	if (netif_is_bridge_port(netdev)) {
> +		br = netdev_master_upper_dev_get_rcu(netdev);
> +		if (!br)
> +			return false;
> +
> +		netdev_walk_all_lower_dev(br, sw_nb_check_slaves, &priv);
> +		return priv.flags && !!*(int *)priv.data;
> +	}
> +
> +	return sw_nb_is_cavium_dev(netdev);
> +}

[High]
Is the assumption in the comment ("rtnl_lcok is already acquired")
correct for all callers?

sw_nb_is_valid_dev() is invoked from:

  sw_nb_net_event()      - netevent chain, atomic_notifier
  sw_nb_v4_fib_event()   - FIB chain, atomic_notifier
  sw_nb_v6_fib_event()   - FIB chain, atomic_notifier
  sw_nb_fdb_event()      - switchdev, atomic
  sw_nb_netdev_event()   - netdevice chain, rtnl-held

register_netevent_notifier() and register_fib_notifier() install into
atomic notifier chains (see net/core/netevent.c and
net/core/fib_notifier.c), so those callbacks are not guaranteed to run
with rtnl held.

netdev_walk_all_lower_dev() (the non-RCU variant) walks
dev->adj_list.lower without rcu_dereference, and
netdev_master_upper_dev_get_rcu() dereferences adj_list.upper without
an outer rcu_read_lock.  Concurrent adjacency changes under rtnl can
then race the walk.  Also, note the typo "rtnl_lcok" in the comment.

[ ... ]

> +static int sw_nb_netdev_event(struct notifier_block *unused,
> +			      unsigned long event, void *ptr)
> +{
[ ... ]
> +}
> +
> +static struct notifier_block sw_nb_netdev = {
> +	.notifier_call = sw_nb_netdev_event,
> +};
> +
> +int sw_nb_unregister(struct net_device *netdev)
> +{
[ ... ]
> +}
> +EXPORT_SYMBOL(sw_nb_unregister);
> +
> +int sw_nb_register(struct net_device *netdev)
> +{
[ ... ]
> +}
> +EXPORT_SYMBOL(sw_nb_register);

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v4.c b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v4.c
> new file mode 100644
> index 000000000000..947dafe586a0
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v4.c

[ ... ]

> +int sw_nb_v4_inetaddr_event(struct notifier_block *nb,
> +			    unsigned long event, void *ptr)
> +{
> +	struct in_ifaddr *ifa = (struct in_ifaddr *)ptr;
> +	struct net_device *dev = ifa->ifa_dev->dev;
> +	struct net_device *lower, *pf_dev;
> +	struct netdev_hw_addr *dev_addr;
> +	struct fib_entry *entry;
> +	struct in_device *idev;
> +	struct list_head *iter;
> +	struct otx2_nic *pf;
> +
> +	if (event != NETDEV_CHANGE &&
> +	    event != NETDEV_UP &&
> +	    event != NETDEV_DOWN) {
> +		return NOTIFY_DONE;
> +	}
> +
> +	idev = __in_dev_get_rtnl(dev);
> +	if (!idev || !idev->ifa_list)
> +		return NOTIFY_DONE;
> +
> +	entry = kcalloc(1, sizeof(*entry), GFP_ATOMIC);
> +	entry->cmd = sw_nb_inetaddr_event_to_otx2_event(event, dev);

[High]
Can kcalloc() return NULL here?  The very next line dereferences entry
without a NULL check.  The same pattern is present in
sw_nb_net_v4_neigh_update(), sw_nb_v6_netdev_event(),
sw_nb_v6_inetaddr_event(), and sw_nb_net_v6_neigh_update().  Two other
handlers in this same patch (sw_nb_v4_netdev_event and
sw_nb_v4_fib_event) do have the NULL check.

These callbacks run from atomic notifier chains (netevent, inet6addr)
during frequent events such as ARP/ND updates and address changes,
which are triggerable by an unprivileged party sending traffic on the
interface.

[Medium]
For the bridge-master branch below:

> +	pf_dev = dev;
> +	if (netif_is_bridge_master(dev))  {
> +		entry->bridge = 1;
> +		netdev_for_each_lower_dev(dev, lower, iter) {
> +			pf_dev = lower;
> +			break;
> +		}
> +	} else if (is_vlan_dev(dev)) {
> +		entry->vlan_valid = 1;
> +		pf_dev = vlan_dev_real_dev(dev);
> +		entry->vlan_tag = vlan_dev_vlan_id(dev);
> +	}
> +
> +	pf = netdev_priv(pf_dev);
> +	entry->port_id = pf->pcifunc;

What happens when the bridge has zero slaves?  pf_dev stays equal to
the bridge netdev itself, and netdev_priv(bridge) returns
struct net_bridge, not struct otx2_nic.  Reading pcifunc off that
pointer produces an out-of-type read.

Unlike sw_nb_v4_fib_event() and sw_nb_net_v4_neigh_update(), this
handler is not gated by sw_nb_is_valid_dev(), so an empty bridge with
no ports still reaches this code.

[ ... ]

> +int sw_nb_v4_fib_event(struct notifier_block *nb,
> +		       unsigned long event, void *ptr)
> +{
[ ... ]
> +	entries = kcalloc(fi->fib_nhs, sizeof(*entries), GFP_ATOMIC);
> +	if (!entries)
> +		return NOTIFY_DONE;
> +
> +	haddr = kcalloc(fi->fib_nhs, sizeof(u32), GFP_ATOMIC);
> +	if (!haddr) {
> +		kfree(entries);
> +		return NOTIFY_DONE;
> +	}
[ ... ]
> +	cnt = iter - entries;
> +	if (!cnt)
> +		return NOTIFY_DONE;
> +
> +	netdev_dbg(pf_dev, "pf_dev is %s cnt=%d\n", pf_dev->name, cnt);
> +	kfree(entries);
> +
> +	if (!hcnt)
> +		return NOTIFY_DONE;
> +
> +	entries = kcalloc(hcnt, sizeof(*entries), GFP_ATOMIC);
> +	if (!entries)
> +		return NOTIFY_DONE;

[High]
Does this leak entries and haddr on the early-return paths?

The if (!cnt) return path returns before freeing entries or haddr.

The if (!hcnt) return path frees entries just above but then returns
without freeing haddr.

The second kcalloc failure path returns having freed only the first
entries allocation, leaving haddr allocated.

FIB events fire on every route change; a system where
sw_nb_is_valid_dev() rejects every nexthop will leak on every event.
This appears to be addressed in the follow-up commit
7f7c9aba2d92 ("octeontx2: offload host FIB updates to switch via AF
mailbox") which adds kfree() calls on each branch, but the leak is
present in this commit as-is.

[ ... ]

> +int sw_nb_net_v4_neigh_update(struct notifier_block *nb,
> +			      unsigned long event, void *ptr)
> +{
[ ... ]
> +	entry = kcalloc(1, sizeof(*entry), GFP_ATOMIC);
> +	entry->cmd = OTX2_NEIGH_UPDATE;
> +	entry->dst = (__force u32)htonl(*(u32 *)n->primary_key);
> +	entry->dst_len = n->tbl->key_len * 8;
> +	entry->mac_valid = 1;
> +	entry->nud_state = n->nud_state;
> +	neigh_ha_snapshot(entry->mac, n, n->dev);
> +	ether_addr_copy(entry->mac, n->ha);

[Medium]
Why does this call neigh_ha_snapshot() followed immediately by a
plain ether_addr_copy() on n->ha?

neigh_ha_snapshot() uses read_seqbegin/read_seqretry against
n->ha_lock precisely because n->ha updates from neigh_update() take
write_seqlock(&n->ha_lock) and are not atomic word-writes on all
architectures.  The subsequent ether_addr_copy() is an unsynchronized
read of the same field and overwrites the snapshot with a possibly
torn value.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v6.c b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v6.c
> new file mode 100644
> index 000000000000..cc908f565d24
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/switch/sw_nb_v6.c

[ ... ]

> +int sw_nb_v6_netdev_event(struct notifier_block *unused,
> +			  unsigned long event, void *ptr)
> +{
[ ... ]
> +	i6dev = __in6_dev_get(dev);
> +
> +	rcu_read_lock();
> +	ifp = list_first_entry_or_null(&i6dev->addr_list,
> +				       struct inet6_ifaddr,  if_list);
> +	if (!ifp) {
> +		rcu_read_unlock();
> +		return NOTIFY_DONE;
> +	}
> +
> +	if (ipv6_addr_type(&ifp->addr) & IPV6_ADDR_LINKLOCAL) {
> +		rcu_read_unlock();
> +		return NOTIFY_DONE;
> +	}
> +
> +	pf = netdev_priv(dev);
> +
> +	entry = kcalloc(1, sizeof(*entry), GFP_KERNEL);
> +	entry->cmd = sw_nb_inetaddr_event_to_otx2_event(event, dev);

[High]
Is GFP_KERNEL safe under rcu_read_lock()?  GFP_KERNEL implies
__GFP_DIRECT_RECLAIM and may sleep, which is illegal inside an RCU
read-side critical section and will trip
"BUG: sleeping function called from invalid context" with
CONFIG_DEBUG_ATOMIC_SLEEP.

Would GFP_ATOMIC, or moving the allocation outside the
rcu_read_lock() region, be more appropriate?

[High]
Is there a missing NULL check on this kcalloc()?  The next statement
dereferences entry unconditionally.

[ ... ]

> +int sw_nb_v6_inetaddr_event(struct notifier_block *nb,
> +			    unsigned long event, void *ptr)
> +{
[ ... ]
> +	entry = kcalloc(1, sizeof(*entry), GFP_ATOMIC);
> +	entry->cmd = sw_nb_inetaddr_event_to_otx2_event(event, dev);

[High]
Same pattern as noted above - can this dereference NULL when kcalloc()
fails under memory pressure?

> +int sw_nb_net_v6_neigh_update(struct notifier_block *nb,
> +			      unsigned long event, void *ptr)
> +{
[ ... ]
> +	entry = kcalloc(1, sizeof(*entry), GFP_ATOMIC);
> +	entry->cmd = OTX2_NEIGH_UPDATE;

[High]
Same as above - is a NULL check missing before dereferencing entry?
-- 
This is an AI-generated review.