Patch "net: stmmac: don't reject VLANs when IFF_PROMISC is set" has been added to the 6.2-stable tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This is a note to let you know that I've just added the patch titled

    net: stmmac: don't reject VLANs when IFF_PROMISC is set

to the 6.2-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     net-stmmac-don-t-reject-vlans-when-iff_promisc-is-se.patch
and it can be found in the queue-6.2 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.



commit 992054ff7f9c9992af90feb27b7f11b22d6a927b
Author: Vladimir Oltean <vladimir.oltean@xxxxxxx>
Date:   Sat Mar 25 13:28:15 2023 +0200

    net: stmmac: don't reject VLANs when IFF_PROMISC is set
    
    [ Upstream commit a7602e7332b97cfbec7bacb0f1ade99a575fe104 ]
    
    The blamed commit has introduced the following tests to
    dwmac4_add_hw_vlan_rx_fltr(), called from stmmac_vlan_rx_add_vid():
    
            if (hw->promisc) {
                    netdev_err(dev,
                               "Adding VLAN in promisc mode not supported\n");
                    return -EPERM;
            }
    
    "VLAN promiscuous" mode is keyed in this driver to IFF_PROMISC, and so,
    vlan_vid_add() and vlan_vid_del() calls cannot take place in IFF_PROMISC
    mode. I have the following 2 arguments that this restriction is.... hm,
    how shall I put it nicely... unproductive :)
    
    First, take the case of a Linux bridge. If the kernel is compiled with
    CONFIG_BRIDGE_VLAN_FILTERING=y, then this bridge shall have a VLAN
    database. The bridge shall try to call vlan_add_vid() on its bridge
    ports for each VLAN in the VLAN table. It will do this irrespectively of
    whether that port is *currently* VLAN-aware or not. So it will do this
    even when the bridge was created with vlan_filtering 0.
    But the Linux bridge, in VLAN-unaware mode, configures its ports in
    promiscuous (IFF_PROMISC) mode, so that they accept packets with any
    MAC DA (a switch must do this in order to forward those packets which
    are not directly targeted to its MAC address).
    
    As a result, the stmmac driver does not work as a bridge port, when the
    kernel is compiled with CONFIG_BRIDGE_VLAN_FILTERING=y.
    
    $ ip link add br0 type bridge && ip link set br0 up
    $ ip link set eth0 master br0 && ip link set eth0 up
    [ 2333.943296] br0: port 1(eth0) entered blocking state
    [ 2333.943381] br0: port 1(eth0) entered disabled state
    [ 2333.943782] device eth0 entered promiscuous mode
    [ 2333.944080] 4033c000.ethernet eth0: Adding VLAN in promisc mode not supported
    [ 2333.976509] 4033c000.ethernet eth0: failed to initialize vlan filtering on this port
    RTNETLINK answers: Operation not permitted
    
    Secondly, take the case of stmmac as DSA master. Some switch tagging
    protocols are based on 802.1Q VLANs (tag_sja1105.c), and as such,
    tag_8021q.c uses vlan_vid_add() to work with VLAN-filtering DSA masters.
    But also, when a DSA port becomes promiscuous (for example when it joins
    a bridge), the DSA framework also makes the DSA master promiscuous.
    
    Moreover, for every VLAN that a DSA switch sends to the CPU, DSA also
    programs a VLAN filter on the DSA master, because if the the DSA switch
    uses a tail tag, then the hardware frame parser of the DSA master will
    see VLAN as VLAN, and might filter them out, for being unknown.
    
    Due to the above 2 reasons, my belief is that the stmmac driver does not
    get to choose to not accept vlan_vid_add() calls while IFF_PROMISC is
    enabled, because the 2 are completely independent and there are code
    paths in the network stack which directly lead to this situation
    occurring, without the user's direct input.
    
    In fact, my belief is that "VLAN promiscuous" mode should have never
    been keyed on IFF_PROMISC in the first place, but rather, on the
    NETIF_F_HW_VLAN_CTAG_FILTER feature flag which can be toggled by the
    user through ethtool -k, when present in netdev->hw_features.
    
    In the stmmac driver, NETIF_F_HW_VLAN_CTAG_FILTER is only present in
    "features", making this feature "on [fixed]".
    
    I have this belief because I am unaware of any definition of promiscuity
    which implies having an effect on anything other than MAC DA (therefore
    not VLAN). However, I seem to be rather alone in having this opinion,
    looking back at the disagreements from this discussion:
    https://lore.kernel.org/netdev/20201110153958.ci5ekor3o2ekg3ky@xxxxxxxxxxxxx/
    
    In any case, to remove the vlan_vid_add() dependency on !IFF_PROMISC,
    one would need to remove the check and see what fails. I guess the test
    was there because of the way in which dwmac4_vlan_promisc_enable() is
    implemented.
    
    For context, the dwmac4 supports Perfect Filtering for a limited number
    of VLANs - dwmac4_get_num_vlan(), priv->hw->num_vlan, with a fallback on
    Hash Filtering - priv->dma_cap.vlhash - see stmmac_vlan_update(), also
    visible in cat /sys/kernel/debug/stmmaceth/eth0/dma_cap | grep 'VLAN
    Hash Filtering'.
    
    The perfect filtering is based on MAC_VLAN_Tag_Filter/MAC_VLAN_Tag_Data
    registers, accessed in the driver through dwmac4_write_vlan_filter().
    
    The hash filtering is based on the MAC_VLAN_Hash_Table register, named
    GMAC_VLAN_HASH_TABLE in the driver and accessed by dwmac4_update_vlan_hash().
    The control bit for enabling hash filtering is GMAC_VLAN_VTHM
    (MAC_VLAN_Tag_Ctrl bit VTHM: VLAN Tag Hash Table Match Enable).
    
    Now, the description of dwmac4_vlan_promisc_enable() is that it iterates
    through the driver's cache of perfect filter entries (hw->vlan_filter[i],
    added by dwmac4_add_hw_vlan_rx_fltr()), and evicts them from hardware by
    unsetting their GMAC_VLAN_TAG_DATA_VEN (MAC_VLAN_Tag_Data bit VEN - VLAN
    Tag Enable) bit. Then it unsets the GMAC_VLAN_VTHM bit, which disables
    hash matching.
    
    This leaves the MAC, according to table "VLAN Match Status" from the
    documentation, to always enter these data paths:
    
    VID    |VLAN Perfect Filter |VTHM Bit |VLAN Hash Filter |Final VLAN Match
           |Match Result        |         |Match Result     |Status
    -------|--------------------|---------|-----------------|----------------
    VID!=0 |Fail                |0        |don't care       |Pass
    
    So, dwmac4_vlan_promisc_enable() does its job, but by unsetting
    GMAC_VLAN_VTHM, it conflicts with the other code path which controls
    this bit: dwmac4_update_vlan_hash(), called through stmmac_update_vlan_hash()
    from stmmac_vlan_rx_add_vid() and from stmmac_vlan_rx_kill_vid().
    This is, I guess, why dwmac4_add_hw_vlan_rx_fltr() is not allowed to run
    after dwmac4_vlan_promisc_enable() has unset GMAC_VLAN_VTHM: because if
    it did, then dwmac4_update_vlan_hash() would set GMAC_VLAN_VTHM again,
    breaking the "VLAN promiscuity".
    
    It turns out that dwmac4_vlan_promisc_enable() is way too complicated
    for what needs to be done. The MAC_Packet_Filter register also has the
    VTFE bit (VLAN Tag Filter Enable), which simply controls whether VLAN
    tagged packets which don't match the filtering tables (either perfect or
    hash) are dropped or not. At the moment, this driver unconditionally
    sets GMAC_PACKET_FILTER_VTFE if NETIF_F_HW_VLAN_CTAG_FILTER was detected
    through the priv->dma_cap.vlhash capability bits of the device, in
    stmmac_dvr_probe().
    
    I would suggest deleting the unnecessarily complex logic from
    dwmac4_vlan_promisc_enable(), and simply unsetting GMAC_PACKET_FILTER_VTFE
    when becoming IFF_PROMISC, which has the same effect of allowing packets
    with any VLAN tags, but has the additional benefit of being able to run
    concurrently with stmmac_vlan_rx_add_vid() and stmmac_vlan_rx_kill_vid().
    
    As much as I believe that the VTFE bit should have been exclusively
    controlled by NETIF_F_HW_VLAN_CTAG_FILTER through ethtool, and not by
    IFF_PROMISC, changing that is not a punctual fix to the problem, and it
    would probably break the VFFQ feature added by the later commit
    e0f9956a3862 ("net: stmmac: Add option for VLAN filter fail queue
    enable"). From the commit description, VFFQ needs IFF_PROMISC=on and
    VTFE=off in order to work (and this change respects that). But if VTFE
    was changed to be controlled through ethtool -k, then a user-visible
    change would have been introduced in Intel's scripts (a need to run
    "ethtool -k eth0 rx-vlan-filter off" which did not exist before).
    
    The patch was tested with this set of commands:
    
      ip link set eth0 up
      ip link add link eth0 name eth0.100 type vlan id 100
      ip addr add 192.168.100.2/24 dev eth0.100 && ip link set eth0.100 up
      ip link set eth0 promisc on
      ip link add link eth0 name eth0.101 type vlan id 101
      ip addr add 192.168.101.2/24 dev eth0.101 && ip link set eth0.101 up
      ip link set eth0 promisc off
      ping -c 5 192.168.100.1
      ping -c 5 192.168.101.1
      ip link set eth0 promisc on
      ping -c 5 192.168.100.1
      ping -c 5 192.168.101.1
      ip link del eth0.100
      ip link del eth0.101
      # Wait for VLAN-tagged pings from the other end...
      # Check with "tcpdump -i eth0 -e -n -p" and we should see them
      ip link set eth0 promisc off
      # Wait for VLAN-tagged pings from the other end...
      # Check with "tcpdump -i eth0 -e -n -p" and we shouldn't see them
      # anymore, but remove the "-p" argument from tcpdump and they're there.
    
    Fixes: c89f44ff10fd ("net: stmmac: Add support for VLAN promiscuous mode")
    Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
    Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
    Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index ec9c130276d89..54bb072aeb2d3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -532,7 +532,6 @@ struct mac_device_info {
 	unsigned int xlgmac;
 	unsigned int num_vlan;
 	u32 vlan_filter[32];
-	unsigned int promisc;
 	bool vlan_fail_q_en;
 	u8 vlan_fail_q;
 };
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index 8c7a0b7c99520..36251ec2589c9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -472,12 +472,6 @@ static int dwmac4_add_hw_vlan_rx_fltr(struct net_device *dev,
 	if (vid > 4095)
 		return -EINVAL;
 
-	if (hw->promisc) {
-		netdev_err(dev,
-			   "Adding VLAN in promisc mode not supported\n");
-		return -EPERM;
-	}
-
 	/* Single Rx VLAN Filter */
 	if (hw->num_vlan == 1) {
 		/* For single VLAN filter, VID 0 means VLAN promiscuous */
@@ -527,12 +521,6 @@ static int dwmac4_del_hw_vlan_rx_fltr(struct net_device *dev,
 {
 	int i, ret = 0;
 
-	if (hw->promisc) {
-		netdev_err(dev,
-			   "Deleting VLAN in promisc mode not supported\n");
-		return -EPERM;
-	}
-
 	/* Single Rx VLAN Filter */
 	if (hw->num_vlan == 1) {
 		if ((hw->vlan_filter[0] & GMAC_VLAN_TAG_VID) == vid) {
@@ -557,39 +545,6 @@ static int dwmac4_del_hw_vlan_rx_fltr(struct net_device *dev,
 	return ret;
 }
 
-static void dwmac4_vlan_promisc_enable(struct net_device *dev,
-				       struct mac_device_info *hw)
-{
-	void __iomem *ioaddr = hw->pcsr;
-	u32 value;
-	u32 hash;
-	u32 val;
-	int i;
-
-	/* Single Rx VLAN Filter */
-	if (hw->num_vlan == 1) {
-		dwmac4_write_single_vlan(dev, 0);
-		return;
-	}
-
-	/* Extended Rx VLAN Filter Enable */
-	for (i = 0; i < hw->num_vlan; i++) {
-		if (hw->vlan_filter[i] & GMAC_VLAN_TAG_DATA_VEN) {
-			val = hw->vlan_filter[i] & ~GMAC_VLAN_TAG_DATA_VEN;
-			dwmac4_write_vlan_filter(dev, hw, i, val);
-		}
-	}
-
-	hash = readl(ioaddr + GMAC_VLAN_HASH_TABLE);
-	if (hash & GMAC_VLAN_VLHT) {
-		value = readl(ioaddr + GMAC_VLAN_TAG);
-		if (value & GMAC_VLAN_VTHM) {
-			value &= ~GMAC_VLAN_VTHM;
-			writel(value, ioaddr + GMAC_VLAN_TAG);
-		}
-	}
-}
-
 static void dwmac4_restore_hw_vlan_rx_fltr(struct net_device *dev,
 					   struct mac_device_info *hw)
 {
@@ -709,22 +664,12 @@ static void dwmac4_set_filter(struct mac_device_info *hw,
 	}
 
 	/* VLAN filtering */
-	if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
+	if (dev->flags & IFF_PROMISC && !hw->vlan_fail_q_en)
+		value &= ~GMAC_PACKET_FILTER_VTFE;
+	else if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
 		value |= GMAC_PACKET_FILTER_VTFE;
 
 	writel(value, ioaddr + GMAC_PACKET_FILTER);
-
-	if (dev->flags & IFF_PROMISC && !hw->vlan_fail_q_en) {
-		if (!hw->promisc) {
-			hw->promisc = 1;
-			dwmac4_vlan_promisc_enable(dev, hw);
-		}
-	} else {
-		if (hw->promisc) {
-			hw->promisc = 0;
-			dwmac4_restore_hw_vlan_rx_fltr(dev, hw);
-		}
-	}
 }
 
 static void dwmac4_flow_ctrl(struct mac_device_info *hw, unsigned int duplex,



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux