lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 29 Jun 2023 22:18:08 +0200
From: Paweł Dembicki <paweldembicki@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>, 
	Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@...il.com> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote:
> > This patch implement vlan filtering for vsc73xx driver.
> >
> > After vlan filtering start, switch is reconfigured from QinQ to simple
> > vlan aware mode. It's required, because VSC73XX chips haven't support
> > for inner vlan tag filter.
> >
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> > Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> > ---
> > v2:
> >   - no changes done
> >
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 457eb7fddf4c..c946464489ab 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> >       return ret;
> >  }
> >
> > +static int
> > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> > +                         bool vlan_filtering, struct netlink_ext_ack *extack)
> > +{
> > +     int ret, i;
> > +
> > +     if (vlan_filtering) {
> > +             vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
> > +     } else {
> > +             if (port == CPU_PORT)
> > +                     vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> > +             else
> > +                     vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
> > +     }
>
> Why do you need ports to be double VLAN aware when vlan_filtering=0?
> Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> incoming packets, and set up the PVIDs of user ports as egress-tagged on
> the CPU port?
>

Because I want to forward tagged and untagged frames when
vlan_filtering is off.  If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
all (tagged and untagged) traffic into the outer vlan, called by the
datasheet as "MAN space". In QinQ mode, it is possible to ignore what
goes from a particular port but it is possible to separate traffic
from different ports.

> > +
> > +     for (i = 0; i <= 3072; i++) {
> > +             ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> What is the purpose of this?

I want to be sure that the table is cleared when vlan awareness is changed.

>
> > +
> > +     return ret;
> > +}
> > +
> >  static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> >                                    bool port_vlan)
> >  {
> > @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> >       return 0;
> >  }
> >
> > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan,
> > +                              struct netlink_ext_ack *extack)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > +     int ret;
> > +
> > +     /* Be sure to deny alterations to the configuration done by tag_8021q.
> > +      */
> > +     if (vid_is_dsa_8021q(vlan->vid)) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Range 3072-4095 reserved for dsa_8021q operation");
> > +             return -EBUSY;
> > +     }
> > +
> > +     if (untagged && port != CPU_PORT) {
> > +             ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     if (pvid && port != CPU_PORT) {
>
> Missing logic to change hardware PVID only while VLAN-aware, and to
> restore the tag_8021q PVID when the bridge VLAN awareness gets disabled.
> DSA does not resolve the conflicts on resources between .port_vlan_add()
> and .tag_8021q_vlan_add(), the driver must do that.
>
> > +             ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
> > +
> > +     return ret;
>
> Style: return vsc73xx_port_update_vlan_table(...)
>
> > +}
> > +
> > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 vlan_no;
> > +     int ret;
> > +     u32 val;
> > +
> > +     ret =
> > +         vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
>
> Style: single line
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > +
> > +     if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> > +                          VSC73XX_TXUPDCFG, &val);
> > +             vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > +                       VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > +             if (vlan_no == vlan->vid) {
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> > +                                         0);
> > +                     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                         VSC73XX_TXUPDCFG,
> > +                                         VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> > +             }
> > +     }
> > +
> > +     vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > +     vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > +     if (vlan_no && vlan_no == vlan->vid) {
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > +                                 VSC73XX_CAT_PORT_VLAN,
> > +                                 VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
>
> As documented in Documentation/networking/switchdev.rst:
>
> When the bridge has VLAN filtering enabled and a PVID is not configured on the
> ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
> has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
> priority-tagged packets must be accepted and forwarded according to the
> bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
> disabled, the presence/lack of a PVID should not influence the packet
> forwarding decision.
>
> Setting the hardware PVID to 0 when the bridge PVID is deleted sounds
> like it accomplishes none of those.
>

My bad. I should just set VSC73XX_CAT_DROP_UNTAGGED_ENA here.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> >  {
> >       int i;
> > @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .port_change_mtu = vsc73xx_change_mtu,
> >       .port_max_mtu = vsc73xx_get_max_mtu,
> >       .port_stp_state_set = vsc73xx_port_stp_state_set,
> > +     .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> > +     .port_vlan_add = vsc73xx_port_vlan_add,
> > +     .port_vlan_del = vsc73xx_port_vlan_del,
> >       .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> >       .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
> >  };
> > --
> > 2.34.1
> >
>

Thank you for such detailed responses and clarifying for me many issues.

--
Pawel Dembicki

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ