| 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
| ||
|
Date: Tue, 15 Feb 2022 10:54:26 +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 v2 net-next 1/8] net: bridge: vlan: notify switchdev only
when something changed
On 15/02/2022 01:31, 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(). In fact, we even make __vlan_add_flags() return void.
>
> With this lookahead 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>
> ---
> v1->v2: drop the br_vlan_restore_existing() approach, just figure out
> ahead of time what will change.
>
> net/bridge/br_vlan.c | 71 ++++++++++++++++++++++++++++----------------
> 1 file changed, 46 insertions(+), 25 deletions(-)
>
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1402d5ca242d..c5355695c976 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -34,36 +34,29 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
> return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
> }
>
> -static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
> +static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
> const struct net_bridge_vlan *v)
> {
> if (vg->pvid == v->vid)
> - return false;
> + return;
>
> smp_wmb();
> br_vlan_set_pvid_state(vg, v->state);
> vg->pvid = v->vid;
> -
> - return true;
> }
>
> -static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
> +static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
> {
> if (vg->pvid != vid)
> - return false;
> + return;
>
> smp_wmb();
> vg->pvid = 0;
> -
> - return true;
> }
>
> -/* return true if anything changed, false otherwise */
> -static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> +static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> {
> struct net_bridge_vlan_group *vg;
> - u16 old_flags = v->flags;
> - bool ret;
>
> if (br_vlan_is_master(v))
> vg = br_vlan_group(v->br);
> @@ -71,16 +64,36 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> vg = nbp_vlan_group(v->port);
>
> if (flags & BRIDGE_VLAN_INFO_PVID)
> - ret = __vlan_add_pvid(vg, v);
> + __vlan_add_pvid(vg, v);
> else
> - ret = __vlan_delete_pvid(vg, v->vid);
> + __vlan_delete_pvid(vg, v->vid);
>
> if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
> v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
> else
> v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
> +}
> +
> +/* return true if anything will change as a result of __vlan_add_flags,
> + * false otherwise
> + */
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> + struct net_bridge_vlan_group *vg;
> + u16 old_flags = v->flags;
> + bool pvid_changed;
>
> - return ret || !!(old_flags ^ v->flags);
> + if (br_vlan_is_master(v))
> + vg = br_vlan_group(v->br);
> + else
> + vg = nbp_vlan_group(v->port);
> +
> + if (flags & BRIDGE_VLAN_INFO_PVID)
> + pvid_changed = (vg->pvid == v->vid);
> + else
> + pvid_changed = (vg->pvid != v->vid);
> +
> + return pvid_changed || !!(old_flags ^ v->flags);
> }
These two have to depend on each other, otherwise it's error-prone and
surely in the future someone will forget to update both.
How about add a "commit" argument to __vlan_add_flags and possibly rename
it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
with commit == false and __vlan_update_flags_commit with commit == true.
Or some other naming, the point is to always use the same flow and checks
when updating the flags to make sure people don't forget.
>
> static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> @@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
> {
> int err;
>
> - err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
> - if (err && err != -EOPNOTSUPP)
> - return err;
> + *changed = __vlan_flags_would_change(vlan, flags);
> + if (*changed) {
> + err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
> + extack);
> + if (err && err != -EOPNOTSUPP)
> + return err;
> + }
You should revert *changed to false in the error path below,
otherwise we will emit a notification without anything changed
if the br_vlan_is_brentry(vlan)) { } block errors out.
>
> if (!br_vlan_is_brentry(vlan)) {
> /* Trying to change flags of non-existent bridge vlan */
> @@ -696,8 +713,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
> br_multicast_toggle_one_vlan(vlan, true);
> }
>
> - if (__vlan_add_flags(vlan, flags))
> - *changed = true;
> + __vlan_add_flags(vlan, flags);
>
> return 0;
>
> @@ -1247,11 +1263,16 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
> *changed = false;
> vlan = br_vlan_find(nbp_vlan_group(port), vid);
> if (vlan) {
> - /* Pass the flags to the hardware bridge */
> - ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
> - if (ret && ret != -EOPNOTSUPP)
> - return ret;
> - *changed = __vlan_add_flags(vlan, flags);
> + *changed = __vlan_flags_would_change(vlan, flags);
> + if (*changed) {
> + /* Pass the flags to the hardware bridge */
> + ret = br_switchdev_port_vlan_add(port->dev, vid,
> + flags, extack);
> + if (ret && ret != -EOPNOTSUPP)
> + return ret;
> + }
> +
> + __vlan_add_flags(vlan, flags);
>
> return 0;
> }
Powered by blists - more mailing lists