netdev - Re: [PATCH v3 net-next 04/11] net: bridge: vlan: notify switchdev only when something changed

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]

Date:   Wed, 16 Feb 2022 13:25:11 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Vladimir Oltean <vladimir.oltean@....com>, <netdev@...r.kernel.org>
CC:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "Vivien Didelot" <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Ido Schimmel <idosch@...dia.com>,
        Rafael Richter <rafael.richter@....de>,
        Daniel Klauer <daniel.klauer@....de>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH v3 net-next 04/11] net: bridge: vlan: notify switchdev
 only when something changed

On 15/02/2022 19:02, Vladimir Oltean wrote:
> Currently, when a VLAN entry is added multiple times in a row to a
> bridge port, nbp_vlan_add() calls br_switchdev_port_vlan_add() each
> time, even if the VLAN already exists and nothing about it has changed:
> 
> bridge vlan add dev lan12 vid 100 master static
> 
> Similarly, when a VLAN is added multiple times in a row to a bridge,
> br_vlan_add_existing() doesn't filter at all the calls to
> br_switchdev_port_vlan_add():
> 
> bridge vlan add dev br0 vid 100 self
> 
> This behavior makes driver-level accounting of VLANs impossible, since
> it is enough for a single deletion event to remove a VLAN, but the
> addition event can be emitted an unlimited number of times.
> 
> The cause for this can be identified as follows: we rely on
> __vlan_add_flags() to retroactively tell us whether it has changed
> anything about the VLAN flags or VLAN group pvid. So we'd first have to
> call __vlan_add_flags() before calling br_switchdev_port_vlan_add(), in
> order to have access to the "bool *changed" information. But we don't
> want to change the event ordering, because we'd have to revert the
> struct net_bridge_vlan changes we've made if switchdev returns an error.
> 
> So to solve this, we need another function that tells us whether any
> change is going to occur in the VLAN or VLAN group, _prior_ to calling
> __vlan_add_flags().
> 
> Split __vlan_add_flags() into a precommit and a commit stage, and rename
> it to __vlan_flags_update(). The precommit stage,
> __vlan_flags_would_change(), will determine whether there is any reason
> to notify switchdev due to a change of flags (note: the BRENTRY flag
> transition from false to true is treated separately: as a new switchdev
> entry, because we skipped notifying the master VLAN when it wasn't a
> brentry yet, and therefore not as a change of flags).
> 
> With this lookahead/precommit function in place, we can avoid notifying
> switchdev if nothing changed for the VLAN and VLAN group.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> v2->v3:
> - create precommit and commit wrappers around __vlan_add_flags().
> - special-case the BRENTRY transition from false to true, instead of
>   treating it as a change of flags and letting drivers figure out that
>   it really isn't.
> - avoid setting *changed unless we know that functions will not error
>   out later.
> 
> v1->v2:
> - drop the br_vlan_restore_existing() approach, just figure out ahead of
>   time what will change.
> 

Thanks for seeing this through.

Signed-off-by: Nikolay Aleksandrov <nikolay@...dia.com>