RE: [PATCH v4 net-next 5/8] net: dsa: netc: check return value of ntmp_ipft_delete_entry()

quoted

ntmp_ipft_delete_entry() may fail, but the driver ignored its return
value, so a failed deletion left the IPFT entry untracked and
impossible to clean up later. This is harmless today because the port
also disables its ingress port filter table lookup, so the leftover
entry is never matched.

[Severity: Medium]
Is the "harmless today" statement accurate for the replacement path that
this patch reorders?

The harmless meaning here is that when host flooding is disabled, even if
hardware entries are not deleted, the flooding rules that have not been
deleted will not take effect because IPFT lookup is disabled. I will improve
the commit message to make it clear.

In the pre-patch netc_port_set_host_flood(), the new rule was added
first, which sets np->ipft_hf_eid to the new entry ID and writes
NETC_PIPFCR = PIPFCR_EN, and only afterwards was the old ID deleted:

	old_entry_id = np->ipft_hf_eid;

	if (netc_port_add_host_flood_rule(np, uc, mc)) {
	...
	/* Remove the old host flood entry */
	netc_port_remove_host_flood(np, old_entry_id);

The old helper only disabled the lookup when the deleted ID matched the
tracked one:

	if (np->ipft_hf_eid == entry_id)
		disable_host_flood = true;

Since np->ipft_hf_eid had already been updated to the new entry, that
condition was false here, so NETC_PIPFCR = 0 was never written.  Doesn't
that mean a failed delete left an active, untracked IPFT redirect entry
with the lookup still enabled, rather than an entry that "is never
matched"?

For the promisc to allmulti-only transition (uc=1,mc=1 to uc=0,mc=1) the
stale entry has an empty DMAC mask, so it matches every frame from that
source port and redirects it to the host.  Would the port then stay
effectively promiscuous after userspace disabled promiscuous mode, with
the ternary IPFT entry leaked permanently because its ID is no longer
recorded anywhere?

The delete can fail in practice: ntmp_ipft_delete_entry() ->
ntmp_delete_entry_by_id() -> ntmp_alloc_data_mem() can return -ENOMEM
from dma_alloc_coherent(GFP_KERNEL), and netc_xmit_ntmp_cmd() can
return
-EBUSY, -ETIMEDOUT (read_poll_timeout() on cbdr->regs.cir) or -EIO.

Could the changelog be updated to describe the actual impact?

quoted

Signed-off-by: Wei Fang <wei.fang@nxp.com>

[Severity: Low]
Should this carry a Fixes: tag?  The ignored return value together with
the add-before-delete ordering came in with commit
46d6407692c80b258ecba3af831130a6f6e2feea ("net: dsa: netc: add FDB,
STP, MTU, port setup and host flooding support"), which is already in
the tree, and the effect described above (a port keeping a redirect the
current RX flags no longer request, plus a leaked TCAM entry) is user
visible.

This is only a theoretical issue; we haven't encountered this problem in
practice. I think it's acceptable as an improvement rather than a bug fix.

quoted

diff --git a/drivers/net/dsa/netc/netc_main.c

b/drivers/net/dsa/netc/netc_main.c

quoted

index c590931a4e1b..082ed9405868 100644

--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

quoted

@@ -1761,32 +1755,31 @@ static int netc_port_add_host_flood_rule(struct

netc_port *np,

quoted

 	return err;
 }

-static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
+static int netc_port_remove_host_flood(struct netc_port *np)
 {
 	struct netc_switch *priv = np->switch_priv;
-	bool disable_host_flood = false;
+	u32 entry_id = np->ipft_hf_eid;
+	int err;

 	if (entry_id == NTMP_NULL_ENTRY_ID)
-		return;
+		return 0;

-	if (np->ipft_hf_eid == entry_id)
-		disable_host_flood = true;
+	err = ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
+	if (err)
+		return err;

-	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
+	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
+	np->uc = false;
+	np->mc = false;
+	netc_port_wr(np, NETC_PIPFCR, 0);

[ ... ]

quoted

@@ -1803,19 +1801,24 @@ static void netc_port_set_host_flood(struct

dsa_switch *ds, int port,

quoted

 		return;

 	/* IPFT does not support in-place updates to the KEYE element,
-	 * we need to add a new entry and then delete the old one. So
-	 * save the old entry ID first.
+	 * we need to delete the old one and then add the new rule. If
+	 * the deletion fails, return immediately.
 	 */
-	old_entry_id = np->ipft_hf_eid;
-
-	if (netc_port_add_host_flood_rule(np, uc, mc)) {
-		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
+	if (netc_port_remove_host_flood(np)) {
+		dev_err(ds->dev,
+			"Failed to delete old host flood rule on port %d\n",
 			port);
+
 		return;
 	}

-	/* Remove the old host flood entry */
-	netc_port_remove_host_flood(np, old_entry_id);
+	/* Restoring the previous configuration is pointless because
+	 * .port_set_host_flood() returns void, so the upper layer cannot
+	 * detect the error and the RX flags have changed.
+	 */
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev,
+			"Failed to add host flood rule on port %d\n", port);

[Severity: Medium]
With the new ordering, what happens to host flooding when the add fails?

netc_port_remove_host_flood() has already deleted the entry and cleared
the tracked state:

	np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
	np->uc = false;
	np->mc = false;
	netc_port_wr(np, NETC_PIPFCR, 0);

netc_port_add_host_flood_rule() can then fail on kzalloc_obj() returning
NULL, or on ntmp_ipft_add_entry() returning -ENOMEM/-EBUSY/-ETIMEDOUT/
-EIO from the command BD transaction.  In that case the port ends up
with no host flood entry at all while the netdev still has IFF_PROMISC
or IFF_ALLMULTI set, and only a dev_err() is emitted.

Before this change the add ran first, so an add failure left the
previous working rule installed and np->uc/np->mc untouched.

Since .port_set_host_flood() is void and is only reached from
dsa_user_change_rx_flags() -> dsa_user_manage_host_flood(), nothing
reports or retries the failure.  Does host unicast/multicast flooding
for that port then stay silently off until userspace toggles the RX
flags again?

The comment says restoring the previous configuration is pointless.  For
the common promisc to allmulti transition the old rule is a superset of
what is now requested, so wouldn't keeping it still satisfy the new
request and be better than delivering nothing?

The failure mode you describe is real, but it's a deliberate trade-off:

IPFT does not support in-place updates to the KEYE element, so the old
entry must be deleted before the new one is added. Keeping the "add-first,
delete-later" order would force each port to track two or more host flood
rules when the old deletion fails, which this patch intentionally moves away
from - especially since IPFT will soon serve other features (PTP trapping,
flow policing) where disabling the port's whole lookup is no longer acceptable.

The impact is bounded. An add failure only happens under memory pressure
or a command-BD error (rare), and the effect is that the port's host flooding is
temporarily off until userspace toggles the RX flags again - a recoverable
functional degradation, not a memory-safety issue.

[Severity: Low]
Separately, is the window with no rule installed a concern even when
both operations succeed?  Between the delete and the add the port has no
host flood redirect, and the gap covers a full synchronous NTMP command
BD transaction for the delete (dma_alloc_coherent(GFP_KERNEL) plus
netc_xmit_ntmp_cmd() polling through read_poll_timeout()), a
kzalloc_obj() that may enter direct reclaim, and a second command BD
transaction for the add, which only re-arms the redirect at the end:

	err = ntmp_ipft_add_entry(&priv->ntmp, host_flood);
	if (err)
		goto free_host_flood;
	...
	/* Enable ingress port filter table lookup */
	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);

During that interval, frames that only reach the CPU through the host
flood redirect (multicast under IFF_ALLMULTI when going from
(uc=0,mc=1) to (uc=1,mc=1), or unknown-DA unicast under IFF_PROMISC)
are dropped.  The old add-then-delete ordering kept a matching entry
armed at every instant.

Yes, the window is real, but I suppose it is acceptable when switching the
configuration. As stated above, the "delete-first, add-later" order is a
trade-off. Otherwise, each port may need to track two or more host flood
rules when the old deletion fails.

[ ... ]

quoted

@@ -2015,6 +2024,11 @@ static int netc_port_bridge_join(struct

dsa_switch *ds, int port,

quoted

 disable_mlo:
 	netc_port_set_mlo(np, MLO_DISABLE);

+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev,
+			"Failed to restore host flood rule on port %u\n",
+			port);

[Severity: Low]
This isn't a bug, but port is declared int by the .port_bridge_join
prototype, and the two other messages added by this patch use %d for
the same value ("Failed to delete old host flood rule on port %d" and
"Failed to add host flood rule on port %d").  Should this one use %d as
well?

Yes, this should be improved.