netdev - Re: [PATCH net-next 3/6 v7] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement

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] [day] [month] [year] [list]

Message-Id: <20210927044242.760492-1-dqfext@gmail.com>
Date:   Mon, 27 Sep 2021 12:42:42 +0800
From:   DENG Qingfang <dqfext@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Mauri Sandberg <sandberg@...lfence.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH net-next 3/6 v7] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement

On Sun, Sep 26, 2021 at 12:59:26AM +0200, Linus Walleij wrote:
> Implement the correct callback: we have two registers dealing
> with filtering on the RTL9366RB, so we implement an ASIC-specific

Typo

> callback and implement filering using the register bit that makes
> the switch drop frames if the port is not in the VLAN member set.
> 
> The DSA documentation Documentation/networking/switchdev.rst states:
> 
>   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.
> 
> To comply with this, we add two arrays of bool in the RTL8366RB
> state that keeps track of if filtering and PVID is enabled or

You can use dsa_port_is_vlan_filtering() instead.

>  static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
> @@ -933,11 +952,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> -	/* Discard VLAN tagged packets if the port is not a member of
> -	 * the VLAN with which the packets is associated.
> -	 */
> +	/* Accept all packets by default, we enable filtering on-demand */
> +	ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
> +			   0);
> +	if (ret)
> +		return ret;
>  	ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
> -			   RTL8366RB_PORT_ALL);
> +			   0);
>  	if (ret)
>  		return ret;
>  
> @@ -1209,6 +1230,53 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
>  			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
>  }
>  
> +/**
> + * rtl8366rb_drop_untagged() - make the switch drop untagged and C-tagged frames
> + * @smi: SMI state container
> + * @port: the port to drop untagged and C-tagged frames on
> + * @drop: whether to drop or pass untagged and C-tagged frames
> + */
> +static int rtl8366rb_drop_untagged(struct realtek_smi *smi, int port, bool drop)
> +{
> +	return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
> +				  RTL8366RB_VLAN_INGRESS_CTRL1_DROP(port),
> +				  drop ? RTL8366RB_VLAN_INGRESS_CTRL1_DROP(port) : 0);
> +}
> +
> +static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
> +				    bool vlan_filtering,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	struct rtl8366rb *rb;
> +	int ret;

Can standalone ports or VLAN-unaware bridges send/receive VLAN-tagged frames as
is? If not, please take a look at rtl8368s_setAsicVlanKeepCtagFormat function
in the vendor driver. Set the registers to all 1's in setup function, and
change them here.

> +
> +	rb = smi->chip_data;
> +
> +	dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port,
> +		vlan_filtering ? "enable" : "disable");
> +
> +	/* If the port is not in the member set, the frame will be dropped */
> +	ret = regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
> +				 BIT(port), vlan_filtering ? BIT(port) : 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep track if filtering is enabled on each port */
> +	rb->vlan_filtering[port] = vlan_filtering;
> +
> +	/* If VLAN filtering is enabled and PVID is also enabled, we must
> +	 * not drop any untagged or C-tagged frames. If we turn off VLAN
> +	 * filtering on a port, we need ti accept any frames.

Typo

> +	 */
> +	if (vlan_filtering)
> +		ret = rtl8366rb_drop_untagged(smi, port, !rb->pvid_enabled[port]);
> +	else
> +		ret = rtl8366rb_drop_untagged(smi, port, false);

Can be simplified as
rtl8366rb_drop_untagged(smi, port, vlan_filtering && !rb->pvid_enabled[port]);

> +
> +	return ret;
> +}