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=-8.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT 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 C8823C43381 for ; Wed, 6 Mar 2019 12:14:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7285B20675 for ; Wed, 6 Mar 2019 12:14:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="dIsp7bBL" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729471AbfCFMOm (ORCPT ); Wed, 6 Mar 2019 07:14:42 -0500 Received: from mail-wr1-f66.google.com ([209.85.221.66]:41339 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728392AbfCFMOl (ORCPT ); Wed, 6 Mar 2019 07:14:41 -0500 Received: by mail-wr1-f66.google.com with SMTP id n2so13141777wrw.8 for ; Wed, 06 Mar 2019 04:14:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=m2ges+qPkp+7c+1naXxXYUTxDFqz56MFknyNgVLmXa0=; b=dIsp7bBLh2E0S49AJ29pj4cpfEsiirDYtaIStLD8pn1QyEsNluj5DG3HGt/MBhjafR sDbfDDRJIh36gHcOfqzerZIrxVn3wNX7utrsUltXx1CbQHwFOL4Ljp+oD9NX8o2z+iOG cLRPE1Jl0zUZ0Cm3WJfB0R4KIyTavAZNLzFOlIhIjfMw+10yAsyrDQysfCUqM/jbpAjy AzcDBxNNYF1Kzrx0WfRtM7uZyKVGx7NTIg8HZdMl+rvAmmeqNt2GKgETDf55K83pt69u wyiT0uIibf01LbRlYJNwgJb2xAORLp2wdTJaQyyMr7THObLPyz5AZwDUe5YXvwLkSbsU TokQ== 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:in-reply-to:user-agent; bh=m2ges+qPkp+7c+1naXxXYUTxDFqz56MFknyNgVLmXa0=; b=NXjE+MenRbYrExbEILMgme2XHpeFYVq4gPWV1p0RnlOer0AFiOvT4bX0aUcv2U57Dm iy/xA6pgAT+no1ttKc/9SJh46NJEJ0HRu7BGn5z5l7jTN8fK9iHBqxapXtaE+K9zIqWr XEDFLJ/rhtrzqH/qdIbb1TxeagXLORW2k49vsjQ0Bw5X44+BF1Emr5Qh2caPrXrOnkHD zOS5r40wv+eoGh4egyGYsfyHkEl1Eij48EmjbenJONk7KRy/C0aEdv27xiIHNPu1YUtV Uvd2Ry0OJrHs9FRovX1/CKMoZNR0BUJl26DtQL+UdscmeoZcFAJUb2Z/KkbaF0ufHtsR 5CTw== X-Gm-Message-State: APjAAAXg61A1foqhPoGBiVr8TJ0Tljcub4nttqMlD5OgNVHRUYefz8nv VjVnufhaucrOlU28JPi5ZqfWQg== X-Google-Smtp-Source: APXvYqx4kYJuO97TbWY9OCS7d5btN6IAd00pXnlPR7teoqqLF6nSAU6cUfPypiv4A2+PzY2/vs+PcA== X-Received: by 2002:a05:6000:128f:: with SMTP id f15mr2662742wrx.74.1551874478998; Wed, 06 Mar 2019 04:14:38 -0800 (PST) Received: from localhost (mail.chocen-mesto.cz. [85.163.43.2]) by smtp.gmail.com with ESMTPSA id i62sm2252197wmg.17.2019.03.06.04.14.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 06 Mar 2019 04:14:38 -0800 (PST) Date: Wed, 6 Mar 2019 13:04:30 +0100 From: Jiri Pirko To: Si-Wei Liu Cc: "Michael S. Tsirkin" , Sridhar Samudrala , Stephen Hemminger , Jakub Kicinski , David Miller , Netdev , virtualization@lists.linux-foundation.org, liran.alon@oracle.com, boris.ostrovsky@oracle.com, vijay.balakrishna@oracle.com Subject: Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces Message-ID: <20190306120430.GA2819@nanopsycho> References: <1551747059-11831-1-git-send-email-si-wei.liu@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1551747059-11831-1-git-send-email-si-wei.liu@oracle.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Tue, Mar 05, 2019 at 01:50:59AM CET, si-wei.liu@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. > >For that to work, now introduce a module-level tunable, >"slave_rename_ok" that allows users to lift up the rename restriction on >failover slave which is already UP. Although it's possible this change >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 the failover slave. 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. The default >for the "slave_rename_ok" is set to true(1). If userspace doesn't have >the right support in place meanwhile users don't care about reliable >userspace naming, the value can be set to false(0). > >Signed-off-by: Si-Wei.Liu@oracle.com >Reviewed-by: Liran Alon >--- > include/linux/netdevice.h | 3 +++ > net/core/dev.c | 3 ++- > net/core/failover.c | 11 +++++++++-- > 3 files changed, 14 insertions(+), 3 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..1fd8bbb 100644 >--- a/net/core/failover.c >+++ b/net/core/failover.c >@@ -16,6 +16,11 @@ > > static LIST_HEAD(failover_list); > static DEFINE_SPINLOCK(failover_lock); >+static bool slave_rename_ok = true; >+ >+module_param(slave_rename_ok, bool, (S_IRUGO | S_IWUSR)); >+MODULE_PARM_DESC(slave_rename_ok, >+ "If set allow renaming the slave when failover master is up"); No module parameters please. If you need to set something do it using rtnl_link_ops. Thanks. > > static struct net_device *failover_get_bymac(u8 *mac, struct failover_ops **ops) > { >@@ -81,13 +86,15 @@ static int failover_slave_register(struct net_device *slave_dev) > } > > slave_dev->priv_flags |= IFF_FAILOVER_SLAVE; >+ if (slave_rename_ok) >+ slave_dev->priv_flags |= 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 +128,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 >