netdev - Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant portion of __skb_vlan

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: <1dab843b-268f-7c7c-6049-8f0f6d4c920a@gmail.com>
Date:   Thu, 23 Mar 2023 09:34:00 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 9/9] net: dsa: tag_ocelot: call only the relevant
 portion of __skb_vlan_pop() on TX

On 3/22/23 16:38, Vladimir Oltean wrote:
> ocelot_xmit_get_vlan_info() calls __skb_vlan_pop() as the most
> appropriate helper I could find which strips away a VLAN header.
> That's all I need it to do, but __skb_vlan_pop() has more logic, which
> will become incompatible with the future revert of commit 6d1ccff62780
> ("net: reset mac header in dev_start_xmit()").
> 
> Namely, it performs a sanity check on skb_mac_header(), which will stop
> being set after the above revert, so it will return an error instead of
> removing the VLAN tag.
> 
> ocelot_xmit_get_vlan_info() gets called in 2 circumstances:
> 
> (1) the port is under a VLAN-aware bridge and the bridge sends
>      VLAN-tagged packets
> 
> (2) the port is under a VLAN-aware bridge and somebody else (an 8021q
>      upper) sends VLAN-tagged packets (using a VID that isn't in the
>      bridge vlan tables)
> 
> In case (1), there is actually no bug to defend against, because
> br_dev_xmit() calls skb_reset_mac_header() and things continue to work.
> 
> However, in case (2), illustrated using the commands below, it can be
> seen that our intervention is needed, since __skb_vlan_pop() complains:
> 
> $ ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up
> $ ip link set $eth master br0 && ip link set $eth up
> $ ip link add link $eth name $eth.100 type vlan id 100 && ip link set $eth.100 up
> $ ip addr add 192.168.100.1/24 dev $eth.100
> $ # needed to work around an apparent DSA RX filtering bug
> $ ip link set $eth promisc on
> 
> I could fend off the checks in __skb_vlan_pop() with some
> skb_mac_header_was_set() calls, but seeing how few callers of
> __skb_vlan_pop() there are from TX paths, that seems rather
> unproductive.
> 
> As an alternative solution, extract the bare minimum logic to strip a
> VLAN header, and move it to a new helper named vlan_remove_tag(), close
> to the definition of vlan_insert_tag(). Document it appropriately and
> make ocelot_xmit_get_vlan_info() call this smaller helper instead.
> 
> Seeing that it doesn't appear illegal to test skb->protocol in the TX
> path, I guess it would be a good for vlan_remove_tag() to also absorb
> the vlan_set_encap_proto() function call.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian