Message-ID: <20210301150852.ejyouycigwu6o5ht@skbuf>
Date:   Mon, 1 Mar 2021 17:08:52 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Michael Walle <michael@...le.cc>
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>,
        Markus Blöchl <Markus.Bloechl@...tronik.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 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.

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