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=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,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 103A8C43381 for ; Thu, 28 Mar 2019 17:14:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C0DB6206B6 for ; Thu, 28 Mar 2019 17:14:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=networkplumber-org.20150623.gappssmtp.com header.i=@networkplumber-org.20150623.gappssmtp.com header.b="eElWAdd4" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726287AbfC1ROc (ORCPT ); Thu, 28 Mar 2019 13:14:32 -0400 Received: from mail-pg1-f195.google.com ([209.85.215.195]:33211 "EHLO mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726034AbfC1ROc (ORCPT ); Thu, 28 Mar 2019 13:14:32 -0400 Received: by mail-pg1-f195.google.com with SMTP id b12so11945021pgk.0 for ; Thu, 28 Mar 2019 10:14:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=BqwEFKbysea6iDhj0/x2xjzwcr9Ku4XDDIeS819W9qE=; b=eElWAdd4Zz3mDZKvCCUsShQkvX3NlAmonTsl+Yavorj0OYM9xD+SHnDTsvzfMW3NBU /6EVBSdWxj08FXmkiKCGZUmAiuLdQF3rr9Sl3CQVxTQkLjq95YmJI0dZPy8FpkeZW6gC 3nAs+6QNGBPZaFerEuJiYArGnSMriIJQWexKOS71ZugQSKCr0WQFQgf1Rgq1bHW2hmKV UIjOR07APB8Al/eC/nuMa4Rlm/RX8NSdnGUbJnfvvH+Ipv4GcwQZ4iDvCfoh5z6GSp5j He3pUol0WOWbJzNdJzFDHOq7VAS2cX1fLlrDuJ7oEDmmaVnVSIFpNT3pVbhnTjaEgf4p Y+Nw== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=BqwEFKbysea6iDhj0/x2xjzwcr9Ku4XDDIeS819W9qE=; b=bbzoB5yFmEoMs8AKd556ZC+HEzZaZG8LBuBd4qBsGZodmaRISl1Zz4Yzkba3HV28e1 k6kybqeFoD54l3VHhUxlCfLNhmKmLIg5VxGwE3jqslrDXux5QEkdiCy1hK2eZX7Sq4Cu jTqsfhLuy7pGLp+si/urcQ6tkk+skAh44mry9KGEvTupvMgU3FkxREStEsnqKkZoqWOT jy7TEl7hZ981llss8UaJUVxWeXVPci01+wkq8fnG+9Sugr2imWWJMdxdhlUCtHclZnYC aJuypDSKAQ775TwXS3NDCNgi408ZDma6Y3cLikIQ5UFDMDFjLtIhjXGmln9glW6U199q DmgQ== X-Gm-Message-State: APjAAAWVESNIYuuXXZEIIcSlICig9QWxiQtv4Z7139uP0f7IlSCAqZb4 fSpqPLL+C761to0zK6S7Pn/X/A== X-Google-Smtp-Source: APXvYqxqjp4XoJ7FGcnY/6aqOWbOMzXgKYPLmUcBJlTjOoQwZtct61MlzpI/2mhm05KQXtsG6lnNtw== X-Received: by 2002:a63:6142:: with SMTP id v63mr40835842pgb.342.1553793271261; Thu, 28 Mar 2019 10:14:31 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id 26sm26154332pfj.93.2019.03.28.10.14.30 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 28 Mar 2019 10:14:31 -0700 (PDT) Date: Thu, 28 Mar 2019 10:14:26 -0700 From: Stephen Hemminger To: si-wei liu Cc: Jiri Pirko , mst@redhat.com, sridhar.samudrala@intel.com, davem@davemloft.net, kubakici@wp.pl, alexander.duyck@gmail.com, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, liran.alon@oracle.com, boris.ostrovsky@oracle.com, vijay.balakrishna@oracle.com Subject: Re: [PATCH net v3] failover: allow name change on IFF_UP slave interfaces Message-ID: <20190328101426.25bf5316@shemminger-XPS-13-9360> In-Reply-To: <06fa1aec-d9a6-3ca7-8849-a1656349ab83@oracle.com> References: <1553644093-10917-1-git-send-email-si-wei.liu@oracle.com> <20190327111132.GI6979@nanopsycho> <06fa1aec-d9a6-3ca7-8849-a1656349ab83@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 27 Mar 2019 16:44:19 -0700 si-wei liu wrote: > On 3/27/2019 4:11 AM, Jiri Pirko wrote: > > Wed, Mar 27, 2019 at 12:48:13AM 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. > >> > >> 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 > >> Reviewed-by: Liran Alon > >> > >> -- > >> v1 -> v2: > >> - Drop configurable module parameter (Sridhar) > >> > >> v2 -> v3: > >> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar) > >> - Send down and up events around rename (Michael S. Tsirkin) > >> --- > >> net/core/dev.c | 37 ++++++++++++++++++++++++++++++++++--- > >> 1 file changed, 34 insertions(+), 3 deletions(-) > >> > >> diff --git a/net/core/dev.c b/net/core/dev.c > >> index 722d50d..3e0cd80 100644 > >> --- a/net/core/dev.c > >> +++ b/net/core/dev.c > >> @@ -1171,6 +1171,7 @@ int dev_get_valid_name(struct net *net, struct net_device *dev, > >> int dev_change_name(struct net_device *dev, const char *newname) > >> { > >> unsigned char old_assign_type; > >> + bool reopen_needed = false; > >> char oldname[IFNAMSIZ]; > >> int err = 0; > >> int ret; > >> @@ -1180,8 +1181,24 @@ 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) > >> - return -EBUSY; > >> + > >> + /* Allow failover slave to rename even when > >> + * it is up and running. > >> + * > >> + * Failover slaves are special, since userspace > >> + * might rename the slave after the interface > >> + * has been brought up and running due to > >> + * auto-enslavement. > >> + * > >> + * Failover users don't actually care about slave > >> + * name change, as they are only expected to operate > >> + * on master interface directly. > >> + */ > >> + if (dev->flags & IFF_UP) { > >> + if (likely(!(dev->priv_flags & IFF_FAILOVER_SLAVE))) > >> + return -EBUSY; > >> + reopen_needed = true; > >> + } > >> > >> write_seqcount_begin(&devnet_rename_seq); > >> > >> @@ -1198,6 +1215,9 @@ int dev_change_name(struct net_device *dev, const char *newname) > >> return err; > >> } > >> > >> + if (reopen_needed) > >> + dev_close(dev); > > Ugh. Don't dev_close/dev_open on name change. > See my response to Michael and Stephen. What's your suggestion then? To a DEV_CHANGE notification instead? My opinion is that allowing name change is not worth the doing. Also, the kernel should never do the name change, it is up to userspace.