Message-ID: <20190321104911-mutt-send-email-mst@kernel.org>
Date:   Thu, 21 Mar 2019 10:49:26 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Liran Alon <liran.alon@...cle.com>
Cc:     Si-Wei Liu <si-wei.liu@...cle.com>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Jakub Kicinski <kubakici@...pl>, Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        virtualization@...ts.linux-foundation.org,
        boris.ostrovsky@...cle.com, vijay.balakrishna@...cle.com
Subject: Re: [PATCH net v2] failover: allow name change on IFF_UP slave
 interfaces

On Thu, Mar 21, 2019 at 04:20:40PM +0200, Liran Alon wrote:
> 
> 
> > On 21 Mar 2019, at 16:04, Michael S. Tsirkin <mst@...hat.com> wrote:
> > 
> > On Wed, Mar 06, 2019 at 10:08:32PM -0500, Si-Wei Liu wrote:
> >> When a netdev appears through hot plug then gets enslaved by a failover
> >> master that is already up and running, the slave will be opened
> >> right away after getting enslaved. Today there's a race that userspace
> >> (udev) may fail to rename the slave if the kernel (net_failover)
> >> opens the slave earlier than when the userspace rename happens.
> >> Unlike bond or team, the primary slave of failover can't be renamed by
> >> userspace ahead of time, since the kernel initiated auto-enslavement is
> >> unable to, or rather, is never meant to be synchronized with the rename
> >> request from userspace.
> >> 
> >> As the failover slave interfaces are not designed to be operated
> >> directly by userspace apps: IP configuration, filter rules with
> >> regard to network traffic passing and etc., should all be done on master
> >> interface. In general, userspace apps only care about the
> >> name of master interface, while slave names are less important as long
> >> as admin users can see reliable names that may carry
> >> other information describing the netdev. For e.g., they can infer that
> >> "ens3nsby" is a standby slave of "ens3", while for a
> >> name like "eth0" they can't tell which master it belongs to.
> >> 
> >> Historically the name of IFF_UP interface can't be changed because
> >> there might be admin script or management software that is already
> >> relying on such behavior and assumes that the slave name can't be
> >> changed once UP. But failover is special: with the in-kernel
> >> auto-enslavement mechanism, the userspace expectation for device
> >> enumeration and bring-up order is already broken. Previously initramfs
> >> and various userspace config tools were modified to bypass failover
> >> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> >> in case that users care about seeing reliable slave name, the new type
> >> of failover slaves needs to be taken care of specifically in userspace
> >> anyway.
> >> 
> >> It's less risky to lift up the rename restriction on failover slave
> >> which is already UP. Although it's possible this change may potentially
> >> break userspace component (most likely configuration scripts or
> >> management software) that assumes slave name can't be changed while
> >> UP, it's relatively a limited and controllable set among all userspace
> >> components, which can be fixed specifically to work with the new naming
> >> behavior of failover slaves. Userspace component interacting with
> >> slaves should be changed to operate on failover master instead, as the
> >> failover slave is dynamic in nature which may come and go at any point.
> >> The goal is to make the role of failover slaves less relevant, and
> >> all userspace should only deal with master in the long run.
> >> 
> >> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
> >> Reviewed-by: Liran Alon <liran.alon@...cle.com>
> >> Acked-by: Michael S. Tsirkin <mst@...hat.com>
> > 
> > I worry that userspace might have made a bunch of assumptions
> > that names never change as long as interface is up.
> > So listening for up events ensures that interface
> > is not renamed.
> 
> That’s true. This is exactly what is described in 3rd paragraph of commit message.
> However, as commit message claims, net-failover slaves can be treated specially
> because userspace is already broken on their handling and they need to be modified
> to behave specially in regards to those slaves. Therefore, it’s less risky to lift up the
> rename restriction on failover slave which is already UP.
> 
> > 
> > How about sending down and up events around such renames?
> 
> You mean that dev_change_name() will behave as proposed in this patch but also in addition
> send fake DOWN and UP uevents to userspace?
> 
> -Liran

That was what I was trying to say.

> > 
> > 
> > 
> >> ---
> >> v1 -> v2:
> >> - Drop configurable module parameter (Sridhar)
> >> 
> >> 
> >> include/linux/netdevice.h | 3 +++
> >> net/core/dev.c            | 3 ++-
> >> net/core/failover.c       | 6 +++---
> >> 3 files changed, 8 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 857f8ab..6d9e4e0 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -1487,6 +1487,7 @@ struct net_device_ops {
> >>  * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> >>  * @IFF_FAILOVER: device is a failover master device
> >>  * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> >> + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
> >>  */
> >> enum netdev_priv_flags {
> >> 	IFF_802_1Q_VLAN			= 1<<0,
> >> @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
> >> 	IFF_NO_RX_HANDLER		= 1<<26,
> >> 	IFF_FAILOVER			= 1<<27,
> >> 	IFF_FAILOVER_SLAVE		= 1<<28,
> >> +	IFF_SLAVE_RENAME_OK		= 1<<29,
> >> };
> >> 
> >> #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
> >> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> >> #define IFF_NO_RX_HANDLER		IFF_NO_RX_HANDLER
> >> #define IFF_FAILOVER			IFF_FAILOVER
> >> #define IFF_FAILOVER_SLAVE		IFF_FAILOVER_SLAVE
> >> +#define IFF_SLAVE_RENAME_OK		IFF_SLAVE_RENAME_OK
> >> 
> >> /**
> >>  *	struct net_device - The DEVICE structure.
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 722d50d..ae070de 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -1180,7 +1180,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
> >> 	BUG_ON(!dev_net(dev));
> >> 
> >> 	net = dev_net(dev);
> >> -	if (dev->flags & IFF_UP)
> >> +	if (dev->flags & IFF_UP &&
> >> +	    !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
> >> 		return -EBUSY;
> >> 
> >> 	write_seqcount_begin(&devnet_rename_seq);
> >> diff --git a/net/core/failover.c b/net/core/failover.c
> >> index 4a92a98..34c5c87 100644
> >> --- a/net/core/failover.c
> >> +++ b/net/core/failover.c
> >> @@ -80,14 +80,14 @@ static int failover_slave_register(struct net_device *slave_dev)
> >> 		goto err_upper_link;
> >> 	}
> >> 
> >> -	slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> >> +	slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >> 
> >> 	if (fops && fops->slave_register &&
> >> 	    !fops->slave_register(slave_dev, failover_dev))
> >> 		return NOTIFY_OK;
> >> 
> >> 	netdev_upper_dev_unlink(slave_dev, failover_dev);
> >> -	slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> >> +	slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >> err_upper_link:
> >> 	netdev_rx_handler_unregister(slave_dev);
> >> done:
> >> @@ -121,7 +121,7 @@ int failover_slave_unregister(struct net_device *slave_dev)
> >> 
> >> 	netdev_rx_handler_unregister(slave_dev);
> >> 	netdev_upper_dev_unlink(slave_dev, failover_dev);
> >> -	slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> >> +	slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_SLAVE_RENAME_OK);
> >> 
> >> 	if (fops && fops->slave_unregister &&
> >> 	    !fops->slave_unregister(slave_dev, failover_dev))
> >> -- 
> >> 1.8.3.1

Powered by blists - more mailing lists