| Message ID | 20250326183504.16724-1-v.shevtsov@mt-integration.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: dsa: felix: check felix_cpu_port_for_conduit() for failure | expand |
Hello Vitaliy, On Wed, Mar 26, 2025 at 11:34:57PM +0500, Vitaliy Shevtsov wrote: > felix_cpu_port_for_conduit() can return a negative value in case of failure > and then it will be used as a port index causing buffer underflow. This can > happen if a bonding interface in 802.1Q mode has no ports. This is unlikely > to happen because the underlying driver handles IFF_TEAM, IFF_MASTER, > IFF_BONDING bits and ports populating correctly, it is still better to > check this for correctness if somehow it fails. If the bonding interface has no ports, it is not a DSA conduit. See the logic in dsa_conduit_changeupper() which, starting from "dev" which is known to be a DSA conduit, it looks at info->upper_dev which is a LAG device, and calls dsa_conduit_lag_join() when it is linking with it. Thus, the LAG device (info->upper_dev) has at least one port: dev. Also see this comment and walk through the dsa_conduit_lag_leave() path: /* If the LAG DSA conduit has no ports left, migrate back all * user ports to the first physical CPU port */ Given the justification provided thus far, I don't see a reason to merge this patch. The "somehow it fails" needs to be a bit more clear.
On Wed, 26 Mar 2025 21:22:59 +0200, Vladimir Oltean wrote: > Hello Vitaliy, > > If the bonding interface has no ports, it is not a DSA conduit. > > See the logic in dsa_conduit_changeupper() which, starting from "dev" > which is known to be a DSA conduit, it looks at info->upper_dev which is > a LAG device, and calls dsa_conduit_lag_join() when it is linking with > it. Thus, the LAG device (info->upper_dev) has at least one port: dev. > > Also see this comment and walk through the dsa_conduit_lag_leave() path: > > /* If the LAG DSA conduit has no ports left, migrate back all > * user ports to the first physical CPU port > */ > > Given the justification provided thus far, I don't see a reason to merge > this patch. The "somehow it fails" needs to be a bit more clear. Hello, Vladimir. Okay then. Pretty clear, thanks.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 0a4e682a55ef..1495f8e21f90 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -523,6 +523,7 @@ static int felix_tag_npi_change_conduit(struct dsa_switch *ds, int port, { struct dsa_port *dp = dsa_to_port(ds, port), *other_dp; struct ocelot *ocelot = ds->priv; + int cpu; if (netif_is_lag_master(conduit)) { NL_SET_ERR_MSG_MOD(extack, @@ -546,7 +547,12 @@ static int felix_tag_npi_change_conduit(struct dsa_switch *ds, int port, } felix_npi_port_deinit(ocelot, ocelot->npi); - felix_npi_port_init(ocelot, felix_cpu_port_for_conduit(ds, conduit)); + cpu = felix_cpu_port_for_conduit(ds, conduit); + if (cpu < 0) { + dev_err(ds->dev, "Cpu port for conduit not found\n"); + return cpu; + } + felix_npi_port_init(ocelot, cpu); return 0; } @@ -658,6 +664,11 @@ static int felix_tag_8021q_change_conduit(struct dsa_switch *ds, int port, int cpu = felix_cpu_port_for_conduit(ds, conduit); struct ocelot *ocelot = ds->priv; + if (cpu < 0) { + dev_err(ds->dev, "Cpu port for conduit not found\n"); + return cpu; + } + ocelot_port_unassign_dsa_8021q_cpu(ocelot, port); ocelot_port_assign_dsa_8021q_cpu(ocelot, port, cpu);
felix_cpu_port_for_conduit() can return a negative value in case of failure and then it will be used as a port index causing buffer underflow. This can happen if a bonding interface in 802.1Q mode has no ports. This is unlikely to happen because the underlying driver handles IFF_TEAM, IFF_MASTER, IFF_BONDING bits and ports populating correctly, it is still better to check this for correctness if somehow it fails. Check if cpu_port is non-negative before using it as an index. Errors from change_conduit() are already handled and no additional changes are required. Found by Linux Verification Center (linuxtesting.org) with Svace. Signed-off-by: Vitaliy Shevtsov <v.shevtsov@mt-integration.ru> --- v2: return the real cpu value instead of -EINVAL as per Andrew Lunn observation. drivers/net/dsa/ocelot/felix.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)