From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80634C43381 for ; Thu, 21 Mar 2019 14:49:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4E014218D4 for ; Thu, 21 Mar 2019 14:49:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728166AbfCUOtg (ORCPT ); Thu, 21 Mar 2019 10:49:36 -0400 Received: from mail-qt1-f193.google.com ([209.85.160.193]:44709 "EHLO mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727823AbfCUOtf (ORCPT ); Thu, 21 Mar 2019 10:49:35 -0400 Received: by mail-qt1-f193.google.com with SMTP id w5so1653608qtb.11 for ; Thu, 21 Mar 2019 07:49:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=fEAU/jV7+vGYxTqPwPLYM26iJgPJ0oDjU4RnbxKEnbw=; b=L6ZfqfdjWAnQrmFObJt+umGF0ULbuz+OKbTGZ8BMLYl8+hjEZ/Gwanc3NhuhPeoY+R X5+0mMfhJOadycmvUXw/2ImkDVu+KeBR9oDhQ7yUB28U7/4l63W0lqrgeSAxBdixRiUw luQaCWTu9EMoNhLbY5TZj45x0QKOvI0qgEO0VVyTHSrQgIdIvb+klmCDwW/P8RajIkw4 U+iTGApN0POk5uVKWh1jTDD3rMAGzERphYn/ptqXqV0ylfCk6x1dRqB++mukx1oFzCFO qdnv+iHsD/PXTp0XC6FVGVyn/zWrJQ4ZVGTr27eF88K8kA8xxK0/V7NHBfp3jC9TMST1 DY+w== X-Gm-Message-State: APjAAAUrRqhZmoZnh4dNHxHdvtwWO1ZISQVIdgvK3S0/mWUg43BS0YLu qlWDC5ALVEsoFHHIFj3YgAmIow== X-Google-Smtp-Source: APXvYqzwkFA7v2FM6lEhsnNjDhCwb1t1xonqaGWfFPN2eJFU7+TTaRnSsp/DQSSoegeg38CLsecAVw== X-Received: by 2002:ac8:1911:: with SMTP id t17mr3398794qtj.247.1553179774395; Thu, 21 Mar 2019 07:49:34 -0700 (PDT) Received: from redhat.com ([195.39.71.253]) by smtp.gmail.com with ESMTPSA id l28sm3131572qtl.53.2019.03.21.07.49.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 21 Mar 2019 07:49:33 -0700 (PDT) Date: Thu, 21 Mar 2019 10:49:26 -0400 From: "Michael S. Tsirkin" To: Liran Alon Cc: Si-Wei Liu , Sridhar Samudrala , Alexander Duyck , Stephen Hemminger , Jakub Kicinski , Jiri Pirko , David Miller , Netdev , virtualization@lists.linux-foundation.org, boris.ostrovsky@oracle.com, vijay.balakrishna@oracle.com Subject: Re: [PATCH net v2] failover: allow name change on IFF_UP slave interfaces Message-ID: <20190321104911-mutt-send-email-mst@kernel.org> References: <1551928112-32109-1-git-send-email-si-wei.liu@oracle.com> <20190321100110-mutt-send-email-mst@kernel.org> <00E221D7-FA96-4D12-8863-1D985F044968@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <00E221D7-FA96-4D12-8863-1D985F044968@oracle.com> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Mar 21, 2019 at 04:20:40PM +0200, Liran Alon wrote: > > > > On 21 Mar 2019, at 16:04, Michael S. Tsirkin 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 > >> Reviewed-by: Liran Alon > >> Acked-by: Michael S. Tsirkin > > > > 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