netdev - Re: [PATCH net-next] net: dsa: microchip: fix VLAN filtering operation

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: <c4cb3da1-498c-0805-f134-e076e417bfa7@gmail.com>
Date:   Tue, 18 Dec 2018 10:12:26 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tristram.Ha@...rochip.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Pavel Machek <pavel@....cz>,
        Marek Vasut <marex@...x.de>
Cc:     vivien.didelot@...oirfairelinux.com, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: microchip: fix VLAN filtering
 operation

Hi Tristram,

On 12/17/18 7:31 PM, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Fix VLAN filtering operation in kernel 4.15 and later.

A better commit message should be added to this patch, something like:

When VLAN filtering on a bridge port is disabled, it is necessary to
create a VLAN entry with VID = 0 for that port in order for untagged
traffic to reach the bridge. Conversely, when turning on VLAN filtering,
a VLAN entry with the desired PVID value (typically VID = 1) must be
created.

Something along those lines.

> 
> Fixes: b987e98e50ab90e5 ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 44 ++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 57a146a..28e3fa5 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -496,19 +496,33 @@ static int ksz9477_port_vlan_filtering(struct dsa_switch *ds, int port,
>  				       bool flag)
>  {
>  	struct ksz_device *dev = ds->priv;
> +	u32 vlan_table[3];
>  
>  	if (flag) {
> +		vlan_table[0] = VLAN_VALID | 0;
> +		vlan_table[1] = 0;
> +		vlan_table[2] = dev->port_mask;

Is not that breaking separation between bridged ports and non bridged
ports? Don't you want to just include the CPU and the port as specified
as argument to that function here?

> +		if (ksz9477_set_vlan_table(dev, 0, vlan_table)) {
> +			dev_dbg(dev->dev, "Failed to set vlan table\n");
> +			return 0;
> +		}
>  		ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> -			     PORT_VLAN_LOOKUP_VID_0, true);
> -		ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> -			      true);
> +			     (PORT_VLAN_LOOKUP_VID_0 | PORT_INGRESS_FILTER),
> +			     true);
>  		ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, true);
>  	} else {
> +		/* VLAN 1 entry is required to run properly. */
> +		vlan_table[0] = VLAN_VALID | 0;
> +		vlan_table[1] = 0;
> +		vlan_table[2] = dev->port_mask;

Likewise here

> +		if (ksz9477_set_vlan_table(dev, 1, vlan_table)) {
> +			dev_dbg(dev->dev, "Failed to set vlan table\n");
> +			return 0;
> +		}

You are assuming that VID 1 is always used, but this may not be true, I
would read whichever was the last PVID value configured, which could
have been either at bridge creation time through the default_pvid
property, or later on through:

bridge vlan add vid <num> dev <interface> pvid untagged

and use that value as the VLAN entry to create.

>  		ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, false);
> -		ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> -			      false);
>  		ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> -			     PORT_VLAN_LOOKUP_VID_0, false);
> +			     (PORT_VLAN_LOOKUP_VID_0 | PORT_INGRESS_FILTER),

parenthesis should not be needed

> +			     false);
>  	}
>  
>  	return 0;
> @@ -521,6 +535,7 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  	u32 vlan_table[3];
>  	u16 vid;
>  	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
>  
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		if (ksz9477_get_vlan_table(dev, vid, vlan_table)) {
> @@ -533,7 +548,12 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  			vlan_table[1] |= BIT(port);
>  		else
>  			vlan_table[1] &= ~BIT(port);
> -		vlan_table[1] &= ~(BIT(dev->cpu_port));
> +
> +		/* Keep host port untagged when setting pvid. */
> +		if (untagged && pvid)
> +			vlan_table[1] |= BIT(dev->cpu_port);
> +		else
> +			vlan_table[1] &= ~(BIT(dev->cpu_port));
>  
>  		vlan_table[2] |= BIT(port) | BIT(dev->cpu_port);
>  
> @@ -543,7 +563,7 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  		}
>  
>  		/* change PVID */
> -		if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
> +		if (pvid)
>  			ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vid);
>  	}
>  }
> @@ -568,6 +588,10 @@ static int ksz9477_port_vlan_del(struct dsa_switch *ds, int port,
>  
>  		vlan_table[2] &= ~BIT(port);
>  
> +		/* Remove VLAN entry if no longer used. */
> +		if (vid > 1 && !(vlan_table[2] & ~dev->host_mask))
> +			vlan_table[0] &= ~VLAN_VALID;
> +
>  		if (pvid == vid)
>  			pvid = 1;
>  
> @@ -1130,6 +1154,10 @@ static int ksz9477_setup(struct dsa_switch *ds)
>  		return ret;
>  	}
>  
> +	/* Required for port partitioning. */
> +	ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> +		      true);
> +
>  	/* accept packet up to 2000bytes */
>  	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_LEGAL_PACKET_DISABLE, true);
>  
> 


-- 
Florian