Linux Virtualization - Mar 2019 - [PATCH net v3] failover: allow name change on IFF_UP slave interfaces

On Tue, 26 Mar 2019 19:48:13 -0400
Si-Wei Liu <si-wei.liu at oracle.com> 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 listen for the rename
> and/or link down/up events on failover slaves. Userspace component
> interacting with slaves is expected to be changed to operate on failover
> master interface 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 userspace components should only
> deal with failover master in the long run.
> 
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
> Reviewed-by: Liran Alon <liran.alon at oracle.com>

Why do you need to do dev_close/dev_open which will bounce
the link?

On Tue, Mar 26, 2019 at 07:13:42PM -0700, Stephen Hemminger
wrote:
> On Tue, 26 Mar 2019 19:48:13 -0400
> Si-Wei Liu <si-wei.liu at oracle.com> 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 listen for the rename
> > and/or link down/up events on failover slaves. Userspace component
> > interacting with slaves is expected to be changed to operate on
failover
> > master interface 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 userspace components should only
> > deal with failover master in the long run.
> > 
> > Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover
module")
> > Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
> > Reviewed-by: Liran Alon <liran.alon at oracle.com>
> 
> 
> Why do you need to do dev_close/dev_open which will bounce
> the link?
What we need is notify userspace that link went up/down.
close/open will do that but just sending notifications
would do that as well without playing with link states.

-- 
MST

On Wed, Mar 27, 2019 at 01:10:10PM -0700, si-wei liu
wrote:
> Another less safer option is that we just notify userspace anyway without
> sending down/up event around, as I don't see *any real application*
cares
> about the link state or whatsoever when it attempts to detect rename.
How do you write a race ree handler then? ATM just detecting link up is
sufficient and covers 100% of cases. Seems like a good idea to keep it
that way.

-- 
MST