Date:   Wed, 3 Aug 2022 17:49:46 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun.Ramadoss@...rochip.com
Cc:     andrew@...n.ch, linux-kernel@...r.kernel.org,
        UNGLinuxDriver@...rochip.com, vivien.didelot@...il.com,
        linux@...linux.org.uk, f.fainelli@...il.com, kuba@...nel.org,
        edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org,
        Woojung.Huh@...rochip.com, davem@...emloft.net
Subject: Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid
 for bridge_vlan_unwaware

On Tue, Aug 02, 2022 at 02:40:09PM +0000, Arun.Ramadoss@...rochip.com wrote:
> On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > > b/drivers/net/dsa/microchip/ksz_common.c
> > > index 516fb9d35c87..8a5583b1f2f4 100644
> > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
> > > {
> > >       .vlan_filtering = ksz8_port_vlan_filtering,
> > >       .vlan_add = ksz8_port_vlan_add,
> > >       .vlan_del = ksz8_port_vlan_del,
> > > +     .drop_untagged = ksz8_port_enable_pvid,
> > 
> > You'll have to explain this one. What impact does PVID insertion on KSZ8
> > have upon dropping/not dropping untagged packets? This patch is saying
> > that when untagged packets should be dropped, PVID insertion should be
> > enabled, and when untagged packets should be accepted, PVID insertion
> > should be disabled. How come?
> 
> Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
> register for the dropping the untagged packet. If that is the case,
> shall I remove the dropping of untagged packet feature from the ksz8
> switches?

You'll have to see how KSZ8 behaves when the ingress port is configured
with a PVID (through REG_PORT_CTRL_VID) which isn't present in the VLAN
table. If untagged packets are dropped, that's your "drop untagged"
setting. Some other switches will not do this, and accept untagged
packets even if the VLAN table doesn't contain an entry for the PVID
(or doesn't have this port as a member of that VLAN), but have a
separate knob for dropping untagged traffic.

> > This is better in the sense that it resolves the need for the
> > configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
> > bridge ports still share the same PVID. Even more so, standalone ports
> > have address learning enabled, which will poison the address database of
> > VLAN-unaware bridge ports (and of other standalone ports):
> > 
> https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
> > 
> > Are you going to do further work in this area?
> 
> For now, I thought I can fix the issue for bridge vlan unaware port. I
> have few other patch series to be submitted like gPTP, tc commands. If
> standalone port fix also needed for your patch series I can work on it
> otherwise I can take up later stage.

I think the most imperative thing for you to do is to make sure you are
not introducing regressions with the port default VID change - this can
be done by running the bridge related selftests (and making them pass).

Something which I forgot to mention is that normally, I'd expect a
change of VLAN-unaware PVID to also need a change in the way that
VLAN-unaware FDB entries are added (other drivers need to remap vid=0
from port_fdb_add() to the PVID that they use for that VLAN-unaware
bridge, in your case 4095, for those FDB entries to continue matching
properly).

However, I see that currently, ksz9477_fdb_add() sets the "USE FID" bit
only for VLAN-aware FDB entries (vid != 0), which leaves me with more
questions than answers.

It isn't very well explained what it means to not use FID: let's say
there are 2 entries in the static address table, one has "USE FID"=false,
and the other has "USE FID"=true and FID=127, and a packet is received
which is classified to FID 127. On which entry will this packet match?

The bridge driver gives you all FDB entries at once (VLAN-aware and
VLAN-unaware), so if the USE_FID=false entries that the ksz9477 driver
uses for VLAN-unaware mode will shadow the VLAN-aware FDB entries, this
is going to be a problem.

Also, the way in which the ksz9477 driver translates a 12-bit VID into a
7-bit FID also has me incredibly confused (FID is vlan->vid & VLAN_FID_M,
or otherwise said, a simple truncation). This means that your
VLAN-unaware PVID of 4095 uses a FID of 127, which is also the same FID
as VLANs 127, 255, 383 etc, right? So there is potentially still full
address database leakage between VLAN-unaware and VLAN-aware bridges.

I think this phrase from the documentation is under-appreciated in
understanding how the hardware works:

| Table 4-8 details the forwarding and discarding actions that are taken
| for the various VLAN scenarios. The first entry in the table is
| explained by the fact that VLAN Table lookup is enabled even when 802.1Q
| VLAN is not enabled.

The last part ("VLAN Table lookup is enabled even when 802.1Q VLAN is
not enabled") is what makes it so that the PVID of the port must be
present in the VLAN table or otherwise you get packet drops. In turn,
if the VLAN table is being looked up, it means that regardless of
whether the switch is VLAN-unaware or not, the VID will be transformed
into a 7-bit FID.

I want that the FID that is being used for standalone ports and
VLAN-unaware bridges (127) to be a fully conscious decision, with the
implications understood, and not just something done for me to shut up.
There is a risk here that you may think things are fine and work on
other features, but things are not fine at all. And in this area,
standalone ports/bridge VLANs/ FDB entries/FIDs are very inter-related
things. When you change one, you may find that the entire scheme needs
to be re-thought.

Powered by blists - more mailing lists