Re: [PATCH net] net: dsa: Discard frames from unused ports
On Thu, Apr 05, 2018 at 03:20:14PM -0700, Florian Fainelli wrote: > On 04/04/2018 07:17 PM, Andrew Lunn wrote: > > On Wed, Apr 04, 2018 at 05:49:10PM -0700, Florian Fainelli wrote: > >> On 04/04/2018 04:56 PM, Andrew Lunn wrote: > >>> The Marvell switches under some conditions will pass a frame to the > >>> host with the port being the CPU port. Such frames are invalid, and > >>> should be dropped. Not dropping them can result in a crash when > >>> incrementing the receive statistics for an invalid port. > >>> > >>> Reported-by: Chris Healy> >>> Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics") > >> > >> Are you sure this is the commit that introduced the problem? > > > > Hi Florian > > > > Well, the problem is it crashes when trying to update the > > statistics. The CPU port is not allocated a p->stats64, only slave > > ports get those. So before this patch, there was no crash and the > > frame would be delivered to the master interface. This in itself is > > probably not correct, but also not fatal. Talking to Chris, it seems > > this behaviour has existing for a long while. I needed to use lldpd to > > trigger the issue, because i assume the Marvell switch sees these as > > special frames and forwards them to the CPU. The other thing is, the > > code got refactored recently. So this fix will not rebase to too many > > earlier versions. It needs a fix per tagging protocol for before the > > common dsa_master_find_slave() was added. > > Yes what you are explaining makes sense, but does not that mean we would > just be accessing a garbage memory location before as well? Humm, yes. I actually picked the wrong patch. It took two attempts to get the stats64 working. I should of picked the first one. Before stats64, we just used skb->dev. I need to look back at older code, but skb->dev is valid in the versions i tested. It points to the master device. So we don't crash. However, i agree, we should fix this for the LTS kernels. Andrew
Re: [PATCH net] net: dsa: Discard frames from unused ports
From: Andrew LunnDate: Thu, 5 Apr 2018 01:56:44 +0200 > The Marvell switches under some conditions will pass a frame to the > host with the port being the CPU port. Such frames are invalid, and > should be dropped. Not dropping them can result in a crash when > incrementing the receive statistics for an invalid port. > > Reported-by: Chris Healy > Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics") > Signed-off-by: Andrew Lunn ... > + slave_port = >ports[port]; > + > + if (slave_port->type != DSA_PORT_TYPE_USER) > + return NULL; Look like we need a Fixes: update and an adjustment to use unlikely() here based upon Florian's feedback.
Re: [PATCH net] net: dsa: Discard frames from unused ports
On 04/04/2018 07:17 PM, Andrew Lunn wrote: > On Wed, Apr 04, 2018 at 05:49:10PM -0700, Florian Fainelli wrote: >> On 04/04/2018 04:56 PM, Andrew Lunn wrote: >>> The Marvell switches under some conditions will pass a frame to the >>> host with the port being the CPU port. Such frames are invalid, and >>> should be dropped. Not dropping them can result in a crash when >>> incrementing the receive statistics for an invalid port. >>> >>> Reported-by: Chris Healy>>> Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics") >> >> Are you sure this is the commit that introduced the problem? > > Hi Florian > > Well, the problem is it crashes when trying to update the > statistics. The CPU port is not allocated a p->stats64, only slave > ports get those. So before this patch, there was no crash and the > frame would be delivered to the master interface. This in itself is > probably not correct, but also not fatal. Talking to Chris, it seems > this behaviour has existing for a long while. I needed to use lldpd to > trigger the issue, because i assume the Marvell switch sees these as > special frames and forwards them to the CPU. The other thing is, the > code got refactored recently. So this fix will not rebase to too many > earlier versions. It needs a fix per tagging protocol for before the > common dsa_master_find_slave() was added. Yes what you are explaining makes sense, but does not that mean we would just be accessing a garbage memory location before as well? It seems to me like this problem dates back from long before that particular changeset, and this changeset just made it more obvious. Would that be accurate? > >>> - return ds->ports[port].slave; >>> + slave_port = >ports[port]; >>> + >>> + if (slave_port->type != DSA_PORT_TYPE_USER) >> >> Can we optimize this with an unlikely()? > > Yes, that would make sense. > > Andrew > -- Florian
Re: [PATCH net] net: dsa: Discard frames from unused ports
On Wed, Apr 04, 2018 at 05:49:10PM -0700, Florian Fainelli wrote: > On 04/04/2018 04:56 PM, Andrew Lunn wrote: > > The Marvell switches under some conditions will pass a frame to the > > host with the port being the CPU port. Such frames are invalid, and > > should be dropped. Not dropping them can result in a crash when > > incrementing the receive statistics for an invalid port. > > > > Reported-by: Chris Healy> > Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics") > > Are you sure this is the commit that introduced the problem? Hi Florian Well, the problem is it crashes when trying to update the statistics. The CPU port is not allocated a p->stats64, only slave ports get those. So before this patch, there was no crash and the frame would be delivered to the master interface. This in itself is probably not correct, but also not fatal. Talking to Chris, it seems this behaviour has existing for a long while. I needed to use lldpd to trigger the issue, because i assume the Marvell switch sees these as special frames and forwards them to the CPU. The other thing is, the code got refactored recently. So this fix will not rebase to too many earlier versions. It needs a fix per tagging protocol for before the common dsa_master_find_slave() was added. > > - return ds->ports[port].slave; > > + slave_port = >ports[port]; > > + > > + if (slave_port->type != DSA_PORT_TYPE_USER) > > Can we optimize this with an unlikely()? Yes, that would make sense. Andrew
Re: [PATCH net] net: dsa: Discard frames from unused ports
On 04/04/2018 04:56 PM, Andrew Lunn wrote: > The Marvell switches under some conditions will pass a frame to the > host with the port being the CPU port. Such frames are invalid, and > should be dropped. Not dropping them can result in a crash when > incrementing the receive statistics for an invalid port. > > Reported-by: Chris Healy> Fixes: 5f6b4e14cada ("net: dsa: User per-cpu 64-bit statistics") Are you sure this is the commit that introduced the problem? > Signed-off-by: Andrew Lunn > --- > net/dsa/dsa_priv.h | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 70de7895e5b8..6c1bf3d9f652 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -126,6 +126,7 @@ static inline struct net_device > *dsa_master_find_slave(struct net_device *dev, > struct dsa_port *cpu_dp = dev->dsa_ptr; > struct dsa_switch_tree *dst = cpu_dp->dst; > struct dsa_switch *ds; > + struct dsa_port *slave_port; > > if (device < 0 || device >= DSA_MAX_SWITCHES) > return NULL; > @@ -137,7 +138,12 @@ static inline struct net_device > *dsa_master_find_slave(struct net_device *dev, > if (port < 0 || port >= ds->num_ports) > return NULL; > > - return ds->ports[port].slave; > + slave_port = >ports[port]; > + > + if (slave_port->type != DSA_PORT_TYPE_USER) Can we optimize this with an unlikely()? > + return NULL; > + > + return slave_port->slave; > } > > /* port.c */ > -- Florian