From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH net] net: dsa: Discard frames from unused ports Date: Fri, 6 Apr 2018 16:25:31 +0200 Message-ID: <20180406142531.GJ17495@lunn.ch> References: <1522886204-1545-1-git-send-email-andrew@lunn.ch> <20180405021757.GA1838@lunn.ch> <9421e733-ed1a-0e2e-b587-4f32dad6d8cb@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: David Miller , netdev , Vivien Didelot To: Florian Fainelli Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:52395 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756645AbeDFOZh (ORCPT ); Fri, 6 Apr 2018 10:25:37 -0400 Content-Disposition: inline In-Reply-To: <9421e733-ed1a-0e2e-b587-4f32dad6d8cb@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: 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