Message-ID: <20210301162653.xwfi7qoxdegi66x5@ipetronik.com>
Date:   Mon, 1 Mar 2021 17:26:53 +0100
From:   Markus Blöchl <Markus.Bloechl@...tronik.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Michael Walle <michael@...le.cc>, Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v2 net 5/6] net: enetc: don't disable VLAN filtering in
 IFF_PROMISC mode

On Mon, Mar 01, 2021 at 05:08:52PM +0200, Vladimir Oltean wrote:
> On Mon, Mar 01, 2021 at 03:36:15PM +0100, Michael Walle wrote:
> > Ok, I see, so your proposed behavior is backed by the standards. But
> > OTOH there was a summary by Markus of the behavior of other drivers:
> > https://lore.kernel.org/netdev/20201119153751.ix73o5h4n6dgv4az@ipetronik.com/
> > And a conclusion by Jakub:
> > https://lore.kernel.org/netdev/20201112164457.6af0fbaf@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/#t
> > And a propsed core change to disable vlan filtering with promisc mode.
> > Do I understand you correctly, that this shouldn't be done either?
> >
> > Don't get me wrong, I don't vote against or in favor of this patch.
> > I just want to understand the behavior.
> 
> So you can involuntarily ignore a standard, or you can ignore it
> deliberately. I can't force anyone to not ignore it in the latter case,
> but indeed, now that I tried to look it up, I personally don't think
> that promiscuity should disable VLAN filtering unless somebody comes up
> with a good reason for which Linux should basically disregard IEEE 802.3.
> In particular, Jakub seems to have been convinced in that thread by no
> other argument except that other drivers ignore the standards too, which
> I'm not sure is a convincing enough argument.

Admittedly, I am still not entirely convinced myself.
I don't know why the other drivers do what they do, why they do it and
whether that's correct.

That is one of the reasons (next to quite a few issues I had with patching
net/core) why I ungracefully abandoned the mentioned thread for now
( sorry and shame on me :-/ ).

The main problem here could also just be that almost everybody _thinks_
that promiscuity means receiving all frames and no one is aware of the
standards definition.
In fact, I can't blame them, as the standard is hard to come by and not
enjoyable to read, imho. And all secondary documentation I could find
on the internet explain promiscuous mode as a "mode of operation" in which
"the card accepts every Ethernet packet sent on the network" or similar.
Even libpcap, which I consider the reference on network sniffing, thinks
that "Promiscuous mode [...] sniffs all traffic on the wire."

Thus I still think that this issue is also fixable by proper
documentation of promiscuity.
At least the meaning and guarantees of IFF_PROMISC in this kernel should
be clearly defined - in one way or the other - such that users with
different expectations can be directed there and drivers with different
behavior can be fixed with that definition as justification.

> 
> In my opinion, the fact that some drivers disable VLAN filtering should
> be treated like a marginal condition, similar to how, when you set the
> MTU on an interface to N octets, it might happen that it accepts packets
> larger than N octets, but it isn't guaranteed.
> 
> > I haven't had time to actually test this, but what if you do:
> >  - don't load the 8021q module (or don't enable kernel support)
> >  - enable promisc
> >  (1)
> >  - load 8021q module
> >  (2)
> >  - add a vlan interface
> >  (3)
> >  - add another vlan interface
> >  (4)
> >
> > What frames would you actually receive on the base interface
> > in (1), (2), (3), (4) and what is the user expectation?
> > I'd say its the same every time. (IIRC there is already some
> > discrepancy due to the VLAN filter hardware offloading)
> 
> The default value is:
> ethtool -k eno0 | grep rx-vlan-filter
> rx-vlan-filter: off
> 
> so we receive all VLAN-tagged packets by default in enetc, unless VLAN
> filtering is turned on.
> 
> > > I chose option 2 because it was way simpler and was just as correct.
> >
> > Fair, but it will also put additional burden to the user to also
> > disable the vlan filtering, right?. Otherwise it would just work. And
> > it will waste CPU cycles for unwanted frames.
> > Although your new patch version contains a new "(yet)" ;)
> 
> True, nobody said it's optimal, but you can't make progress if you
> always try to do things optimally the first time (but at least you
> should do something that's not wrong).
> Adding the dev_uc_add, dev_mc_add and vlan_vid_add calls to
> net/sched/cls_flower.c doesn't seem an impossible task (especially since
> all of them are refcounted, it should be pretty simple to avoid strange
> interactions with other layers such as 8021q), but nonetheless, it just
> wasn't (and still isn't) high enough on my list of priorities.

Powered by blists - more mailing lists