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 <cphe...@gmail.com> > >>> 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