> > 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?
>
> > Signed-off-by: Wei Fang <[email protected]>
>
> [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.
>
> > diff --git a/drivers/net/dsa/netc/netc_main.c
> b/drivers/net/dsa/netc/netc_main.c
> > index c590931a4e1b..082ed9405868 100644
> > --- a/drivers/net/dsa/netc/netc_main.c
> > +++ b/drivers/net/dsa/netc/netc_main.c
>
> [ ... ]
>
> > @@ -1761,32 +1755,31 @@ static int netc_port_add_host_flood_rule(struct
> netc_port *np,
> > 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);
>
> [ ... ]
>
> > @@ -1803,19 +1801,24 @@ static void netc_port_set_host_flood(struct
> dsa_switch *ds, int port,
> > 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.
>
> [ ... ]
>
> > @@ -2015,6 +2024,11 @@ static int netc_port_bridge_join(struct
> dsa_switch *ds, int port,
> > 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.
