netdev - Re: [RFC PATCH net-next 6/9] net: dsa: remove the transactional logic from VLAN objects

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: <20201217114336.4vohivu43hbp7ucw@skbuf>
Date:   Thu, 17 Dec 2020 11:43:37 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vadym Kochan <vkochan@...vell.com>,
        Taras Chornyi <tchornyi@...vell.com>,
        Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Ivan Vecera <ivecera@...hat.com>
Subject: Re: [RFC PATCH net-next 6/9] net: dsa: remove the transactional logic
 from VLAN objects

On Thu, Dec 17, 2020 at 01:04:26PM +0200, Vladimir Oltean wrote:
> On Thu, Dec 17, 2020 at 03:58:19AM +0200, Vladimir Oltean wrote:
> > It should be the driver's business to logically separate its VLAN
> > offloading into a preparation and a commit phase, and some drivers don't
> > need / can't do this.
> > 
> > So remove the transactional shim from DSA and let drivers to propagate
> > errors directly from the .port_vlan_add callback.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> [...]
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 65124bc3ddfb..bd00ef6296f9 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -217,35 +217,13 @@ static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
> >  	return false;
> >  }
> >  
> > -static int dsa_switch_vlan_prepare(struct dsa_switch *ds,
> > -				   struct dsa_notifier_vlan_info *info)
> > -{
> > -	int port, err;
> > -
> > -	if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
> > -		return -EOPNOTSUPP;
> > -
> > -	for (port = 0; port < ds->num_ports; port++) {
> > -		if (dsa_switch_vlan_match(ds, port, info)) {
> > -			err = ds->ops->port_vlan_prepare(ds, port, info->vlan);
> > -			if (err)
> > -				return err;
> > -		}
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  static int dsa_switch_vlan_add(struct dsa_switch *ds,
> >  			       struct dsa_notifier_vlan_info *info)
> >  {
> >  	int port;
> >  
> > -	if (switchdev_trans_ph_prepare(info->trans))
> > -		return dsa_switch_vlan_prepare(ds, info);
> > -
> >  	if (!ds->ops->port_vlan_add)
> > -		return 0;
> > +		return -EOPNOTSUPP;
> >  
> >  	for (port = 0; port < ds->num_ports; port++)
> >  		if (dsa_switch_vlan_match(ds, port, info))
> > -- 
> > 2.25.1
> > 
> 
> For anybody who wants to test, please paste this instead of the existing
> dsa_switch_vlan_add, to propagate the errors:
> 
> static int dsa_switch_vlan_add(struct dsa_switch *ds,
> 			       struct dsa_notifier_vlan_info *info)
> {
> 	int err = 0;
> 	int port;
> 
> 	if (!ds->ops->port_vlan_add)
> 		return -EOPNOTSUPP;
> 
> 	for (port = 0; port < ds->num_ports; port++) {
> 		if (dsa_switch_vlan_match(ds, port, info)) {
> 			err = ds->ops->port_vlan_add(ds, port, info->vlan);
> 			if (err)
> 				break;
> 		}
> 	}
> 
> 	return err;
> }

Having said that, now I finally understand one of the ways in which the
prepare phase was actually useful for something. Sometimes it takes
removing all the code before things like this are visible...

DSA uses notifiers to propagate a switchdev object to multiple ports.
For example, every VLAN is installed not only on the front-panel user
port, but also on the CPU port. Iterating through the port list, we find
that the addition on the user port may succeed, but the addition on the
CPU port might fail.

So, at the very least, we should attempt a rollback on error (iterating
backwards through the port list).

But there is a second problem. Nobody prevents you from adding a VLAN
twice on the same port.

bridge vlan add dev swp0 vid 100
bridge vlan add dev swp0 vid 90-110

If the second command fails at VLAN 110 and we attempt a rollback, we
would end up deleting VLAN 100 too, which was set up by the first command.

This is where the prepare/commit phase could come in handy. First all
the ports confirm that the VLAN range 90-110 is ok, and only then we
proceed to commit it. That would avoid the first problem.

However there's the other problem, which is that artificially splitting
into a prepare and a commit phase will still inevitably leave some
"catastrophic" failures unhandled in the commit phase. We typically
throw our hands up in the air when it comes to these, but normally if we
bother to do something about consistency in the first place, we should
have rollback logic too. It's just that the rollback will not be very
simple. We would need to know, for each VLAN, if it already existed or
not. And if it existed, what flags it had. Either that, or we start
disallowing the user, from the bridge code, to add VLAN objects that
already exist.