From: Andrew Lunn <andrew@lunn.ch>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: David Miller <davem@davemloft.net>,
netdev <netdev@vger.kernel.org>,
Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Subject: Re: [PATCH net] net: dsa: Discard frames from unused ports
Date: Fri, 6 Apr 2018 16:25:31 +0200 [thread overview]
Message-ID: <20180406142531.GJ17495@lunn.ch> (raw)
In-Reply-To: <9421e733-ed1a-0e2e-b587-4f32dad6d8cb@gmail.com>
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 <cphealy@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
next prev parent reply other threads:[~2018-04-06 14:25 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-04 23:56 [PATCH net] net: dsa: Discard frames from unused ports Andrew Lunn
2018-04-05 0:49 ` Florian Fainelli
2018-04-05 2:17 ` Andrew Lunn
2018-04-05 22:20 ` Florian Fainelli
2018-04-06 14:25 ` Andrew Lunn [this message]
2018-04-06 2:04 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180406142531.GJ17495@lunn.ch \
--to=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=vivien.didelot@savoirfairelinux.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).