Re: [PATCH net-next v1 8/8] net: dsa: realtek: rtl8365mb: offload DCB DSCP-to-priority

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rtl8365mb_port_del_dscp_prio()
  (drivers/net/dsa/realtek/rtl8365mb_dcb.c:410-434) decides whether to…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but setting
  ds->dscp_prio_mapping_is_global in rtl8365mb_setup() makes it…

--

net: dsa: realtek: rtl8365mb: offload DCB DSCP-to-priority

This patch wires up the port_get_dscp_prio, port_add_dscp_prio and
port_del_dscp_prio DSA callbacks, so dcbnl can program the switch-wide
DSCP-to-priority table. It also sets ds->dscp_prio_mapping_is_global. The
table starts with the IETF default mapping, and del reverts an entry to
that default only when the hardware still holds the priority being removed.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_dcb.c b/drivers/net/dsa/realtek/rtl8365mb_dcb.c
> index 4afcae6e85a1e..091dc8189e738 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_dcb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_dcb.c

[ ... ]

> @@ -324,6 +364,75 @@ int rtl8365mb_port_set_default_prio(struct dsa_switch *ds, int port, u8 prio)

[ ... ]

> +int rtl8365mb_port_del_dscp_prio(struct dsa_switch *ds, int port, u8 dscp,
> +				 u8 prio)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	int tt, ret;
> +
> +	if (dscp >= RTL8365MB_DSCP_MAX)
> +		return -EINVAL;
> +
> +	/* dcbnl replaces an entry by adding the new one before deleting the
> +	 * old, so only revert if the table still holds the removed priority.
> +	 */
> +	ret = rtl8365mb_port_get_dscp_prio(ds, port, dscp);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != prio)
> +		return 0;
> +
> +	/* Revert to the standard IETF default mapping for this DSCP. */
> +	tt = ietf_dscp_to_ieee8021q_tt(dscp);
> +	if (tt < 0)
> +		return tt;
> +
> +	return rtl8365mb_qos_set_dscp_prio(priv, dscp, tt);
> +}

[Severity: Medium]
Can the hardware DSCP table get out of sync with dcbnl when the same DSCP
has more than one APP entry?

The DSA core removes the APP entry first. It then calls the driver and
passes only the removed priority:

net/dsa/user.c:dsa_user_dcbnl_del_dscp_prio() {
    ...
	err = dcb_ieee_delapp(dev, app);
	if (err)
		return err;

	err = ds->ops->port_del_dscp_prio(ds, port, dscp, app->priority);
    ...
}

dcb_app_add() inserts at the head of the list, and dcb_ieee_getapp_mask()
reports only the first match. After a delete, the effective software
mapping is the newest entry that is left. Nothing writes that entry to the
hardware.

For example, dsa_user_dcbnl_init() first sets DSCP 0 to PCP 0. Then:

  dcb app add dev swpX dscp-prio 0:3    APP {3,0}, HW 3
  dcb app add dev swpX dscp-prio 0:5    APP {5,3,0}, HW 5
  dcb app del dev swpX dscp-prio 0:5    APP {3,0}, HW reverted to 0

Every step returns success, and the global setdel copies the APP state to
all user ports. Those ports then report DSCP 0 -> 3, while the shared
table classifies DSCP-trusting traffic as priority 0.

The rollback in dsa_user_dcbnl_add_dscp_prio() seems to have the same
problem:

	err = dsa_user_dcbnl_ieee_global_dscp_setdel(dev, app, false);
	if (err) {
		if (ds->ops->port_del_dscp_prio)
			ds->ops->port_del_dscp_prio(ds, port, dscp, new_prio);
		dcb_ieee_delapp(dev, app);
		return err;
	}

If copying to a peer port fails, the hardware gets the IETF default
instead of the previous user mapping, which stays in the APP table.

A plain dcb app replace (add the new entry, then delete the old one) does
work, because the delete takes the ret != prio early return.

Could the delete path instead program whatever dcb_ieee_getapp_mask()
reports for dsa_to_port(ds, port)->user? The core has already updated that
table. It would then fall back to ietf_dscp_to_ieee8021q_tt() only when no
entry remains, similar to dsa_user_dcbnl_del_default_prio(). ocelot, KSZ
and yt921x use the same heuristic as this patch.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 77f971abf783f..9fe87c2957959 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -3283,6 +3283,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  	/* Establish a defined QoS baseline: program the priority-to-queue map
>  	 * for the chip's queue count and trust only the port default priority.
>  	 */
> +	ds->dscp_prio_mapping_is_global = true;
>  	ret = rtl8365mb_dcb_init(ds);
>  	if (ret)
>  		goto out_teardown_irq;

[Severity: High]
This isn't a bug introduced by this patch, but setting this flag makes an
existing race in the DSA core reachable on rtl8365mb.

dsa_user_dcbnl_ieee_global_dscp_setdel() calls dcb_ieee_setapp() or
dcb_ieee_delapp() on any peer netdev that is not NULL. It does not check
whether the peer is registered, and it holds no reference:

	dsa_switch_for_each_user_port(other_dp, ds) {
		struct net_device *user = other_dp->user;

		if (!user || user == dev)
			continue;

		err = setdel(user, app);

dsa_user_create() sets port->user before it takes RTNL. If PHY setup
fails, it frees the netdev without holding RTNL:

net/dsa/user.c:dsa_user_create() {
    ...
	port->user = user_dev;
	dsa_user_setup_tagger(user_dev);

	netif_carrier_off(user_dev);

	ret = dsa_user_phy_setup(user_dev);
	if (ret) {
		...
		goto out_gcells;
	}

	rtnl_lock();
    ...
out_gcells:
	gro_cells_destroy(&p->gcells);
out_free:
	free_netdev(user_dev);
	port->user = NULL;
    ...
}

User ports are created one at a time, and RTNL is released after each port
is registered. So this sequence looks possible:

CPU1 (RTM_SETDCB on registered port N, under RTNL)
dsa_user_dcbnl_add_dscp_prio()
  dsa_user_dcbnl_ieee_global_dscp_setdel()
    user = other_dp->user;          /* port N+1, not yet registered */
                                    CPU2
                                    dsa_user_create() for port N+1
                                      dsa_user_phy_setup() fails
                                      free_netdev(user_dev);
    dcb_ieee_setapp(user, app)      /* user is freed */

free_netdev() frees a NETREG_UNINITIALIZED device right away. Could this
be a use-after-free of the peer net_device?

Even when PHY setup succeeds, wouldn't the global setdel add APP entries
keyed on the unregistered device's ifindex 0? Those entries would be left
orphaned in dcb_app_list.

KSZ (ksz8, ksz9477, lan937x) and yt921x set the same flag, so they are
already exposed to this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002115850.962676-1-o.rempel%40pengutronix.de