netdev - Re: [PATCH v2 net-next 1/8] 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:   Tue, 15 Feb 2022 10:10:18 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        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 Tue, Feb 15, 2022 at 11:54:05AM +0200, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> > > +/* 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.
> 
> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> into a single function? But "would_change" returns bool, and "add"
> returns void.

Plus, we have call paths that would bypass the "prepare" stage and jump
to commit, and for good reason. Do we want to change those as well, or
what would be the benefit?