Message-ID: <20200804212421.e2lztrrg4evuk6zd@skbuf>
Date:   Wed, 5 Aug 2020 00:24:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "Gaube, Marvin (THSE-TL1)" <Marvin.Gaube@...at.de>,
        Florian Fainelli <f.fainelli@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: PROBLEM: (DSA/Microchip): 802.1Q-Header lost on KSZ9477-DSA
 ingress without bridge

On Tue, Aug 04, 2020 at 01:36:56PM -0700, Eric Dumazet wrote:
> On Tue, Aug 4, 2020 at 12:43 PM Vladimir Oltean <olteanv@...il.com> wrote:
> >
> > On Tue, Aug 04, 2020 at 12:40:24PM -0700, Eric Dumazet wrote:
> > > On Tue, Aug 4, 2020 at 12:29 PM Vladimir Oltean <olteanv@...il.com> wrote:
> > > >
> > > > On Tue, Aug 04, 2020 at 07:54:18AM -0700, Eric Dumazet wrote:
> > > > >
> > > > > My 2013 commit was a bug fix, and hinted that in the future (eg in
> > > > > net-next tree) the stop-the-bleed could be refined.
> > > > >
> > > > > +               /* Note: we might in the future use prio bits
> > > > > +                * and set skb->priority like in vlan_do_receive()
> > > > > +                * For the time being, just ignore Priority Code Point
> > > > > +                */
> > > > > +               skb->vlan_tci = 0;
> > > > >
> > > > > If you believe this can be done, this is great.
> > > >
> > > > Do you have a reproducer for that bug? I am willing to spend some time
> > > > understand what is going on. This has nothing to do with priority. You
> > > > vaguely described a problem with 802.1p (VLAN 0) and used that as an
> > > > excuse to clear the entire vlan hwaccel tag regardless of VLAN ID. I'm
> > > > curious because we also now have commit 36b2f61a42c2 ("net: handle
> > > > 802.1P vlan 0 packets properly") in that general area, and I simply want
> > > > to know if your patch still serves a valid purpose or not.
> > > >
> > >
> > > I do not have a repro, the patch seemed to help at that time,
> > > according to the reporter.
> >
> > Do you mind if I respectfully revert then? It's clear that the patch has
> > loopholes already (it clears the vlan if it's hwaccel, but leaves it
> > alone if it isn't) and that the proper solution should be different
> > anyway.
> 
> Clearly the situation before the patch was not good, it seems well
> explained in the changelog.
> 
> If you want to revert, you will need to convince the bug has been
> solved in another way.
> 
> So it seems you might have to repro the initial problem.

What bug? What repro? You just said you don't have any.

Maybe I'm dumb, but the changelog is vague to me. It isn't clear what
kind of routing it is, what type of traffic was the router being
subjected to, from what direction was the VLAN traffic coming, was it
just VLAN 0 that was problematic, what drivers those were, what kernel
was used, what has any of that have to do with the referenced commit
48cc32d38a52 ("vlan: don't deliver frames for unknown vlans to
protocols") which is about macvlan returning RX_HANDLER_PASS instead of
RX_HANDLER_ANOTHER, were there other sub-interfaces as well?
And there are also obvious mistakes in the commit description: "if the
vlan id is set and we could find a vlan device for this particular id."
-> "couldn't" should be instead of "could".

This is ridiculous. Not only will I not waste my time as long as there
is nothing actionable (I could test stuff until the cows come home and I
would never know if I'm under your scenario or not), but I do feel that
it's fundamentally your job to prove that there's a bug for which this
is the right fix, rather than me to disprove it.

I'm sorry, I want to give a helping hand, but it doesn't look like I
can.

Powered by blists - more mailing lists