netdev - Re: [PATCH v4 net-next 10/10] net: dsa: microchip: add support for vlan operations

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]

Message-ID: <6c97e0771204b492f31b3d003a5fd97d789920ef.camel@microchip.com>
Date:   Mon, 11 Oct 2021 23:13:36 +0530
From:   Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     <andrew@...n.ch>, <netdev@...r.kernel.org>, <robh+dt@...nel.org>,
        <UNGLinuxDriver@...rochip.com>, <Woojung.Huh@...rochip.com>,
        <hkallweit1@...il.com>, <linux@...linux.org.uk>,
        <davem@...emloft.net>, <kuba@...nel.org>,
        <linux-kernel@...r.kernel.org>, <vivien.didelot@...il.com>,
        <f.fainelli@...il.com>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 net-next 10/10] net: dsa: microchip: add support for
 vlan operations

On Thu, 2021-10-07 at 23:17 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +static int lan937x_port_vlan_filtering(struct dsa_switch *ds, int port,
> > +                                    bool flag,
> > +                                    struct netlink_ext_ack *extack)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     int ret;
> > +
> > +     ret = lan937x_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE,
> > +                       flag);
> 
> If you're going to resend anyway, can you please check the entire
> submission for this pattern, where you can eliminate the intermediary
> "ret" variable and just return the function call directly?
> 
>         return lan937x_cfg(...)
Sure

> Do you have an explanation for what SW_VLAN_ENABLE does exactly?
Enabling the VLAN mode and then act as per the VLAN table.
Do you want me to add this explanation as a comment? or?



> > 
> > 
> > +static int lan937x_port_vlan_del(struct dsa_switch *ds, int port,
> > +                              const struct switchdev_obj_port_vlan *vlan)
> > +{
> > +     bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > +     struct ksz_device *dev = ds->priv;
> > +     struct lan937x_vlan vlan_entry;
> > +     u16 pvid;
> > +     int ret;
> > +
> > +     lan937x_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
> > +     pvid &= 0xFFF;
> > +
> > +     ret = lan937x_get_vlan_table(dev, vlan->vid, &vlan_entry);
> > +     if (ret < 0) {
> > +             dev_err(dev->dev, "Failed to get vlan table\n");
> > +             return ret;
> > +     }
> > +     /* clear port fwd map */
> > +     vlan_entry.fwd_map &= ~BIT(port);
> > +
> > +     if (untagged)
> > +             vlan_entry.untag_prtmap &= ~BIT(port);
> 
> This is bogus.
> The user can add a VLAN entry using:
> 
> bridge vlan add dev lan0 vid 100 pvid untagged
> 
> and remove it using
> 
> bridge vlan del dev lan0 vid 100
> 
> so BRIDGE_VLAN_INFO_UNTAGGED is not set on removal.
> 
> Considering the fact that it doesn't matter whether the port is
> egress-tagged or not when it isn't in the fwd_map in the first place,
> I suggest you completely drop this condition.
Sure, i agree with you.

> 
> > +
> > +     ret = lan937x_set_vlan_table(dev, vlan->vid, &vlan_entry);
> > +     if (ret < 0) {
> > +             dev_err(dev->dev, "Failed to set vlan table\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = lan937x_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);
> 
> What is the point of reading the pvid and writing it back unmodified?
> Is the AND-ing with 0xFFF supposed to do anything? Because when you
> write to REG_PORT_DEFAULT_VID, you write it with nothing in the upper
> bits, so I expect there to be nothing in the upper bits when you read it
> back either.
I had a feedback for not to reset the PVID as per the switchdev documentation
during vlan del. As part of the fix , i just removed PVID reset code alone but
missed these. Read/write PVID to be completely removed. I had a test case to
make sure that the PVID is not reset during vlan del. Since this is
reading/writing back the same values, could not catch them. I will clean up in
the next patch.


> 
> > +     if (ret < 0) {
> > +             dev_err(dev->dev, "Failed to set pvid\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> 
> Also, consider the following set of commands:
> 
Step (0)
> ip link add br0 type bridge vlan_filtering 1
> ip link set lan0 master br0
> bridge vlan add dev lan0 vid 100 pvid untagged
Step (1)
> bridge vlan del dev lan0 vid 100
Step (2)
> ip link set br0 type bridge vlan_filtering 0
> 
> The expectation is that the switch, being VLAN-unaware as it is currently
> configured, receives and sends any packet regardless of VLAN ID.
> If you put an IP on br0 in this state, are you able to ping an outside host?

I have numbered the commands above.
Results are,
Before Step (0). Am able to ping outside.
After Step (0). Am not able to ping outside because the vlan table is set
After Step (1). Am not able to ping outside
After Step (2). Am able to ping outside because of vlan unaware mode.