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.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,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 827B3C43381 for ; Tue, 5 Mar 2019 22:50:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 27DE020675 for ; Tue, 5 Mar 2019 22:50:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=oracle.com header.i=@oracle.com header.b="dlFrO954" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728616AbfCEWuR (ORCPT ); Tue, 5 Mar 2019 17:50:17 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:60712 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728604AbfCEWuR (ORCPT ); Tue, 5 Mar 2019 17:50:17 -0500 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x25Mmh92082030; Tue, 5 Mar 2019 22:50:05 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : references : cc : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=QBIq+RFeO759KwYkHwLB8EQdIX9KFaYIXC7zAF0ccX0=; b=dlFrO9543h7v5GIzYfWef01IufLIGOVdASmA13/YZ3O0tN4d0KDGcICKAv0226V9+8Bj Hi0qPVUNQD2cCzb9bq9iZ6V2bJT6AlkvBLNQOU7NEU++97+KtS8RyTo9SmSuFz5Yg2Dh 2l2u1H8Y+TV4BWUaqh0untGwpBo8fGeT613B5wHlK1GzgqWjeVgu+nJ7Rjxgx7YCZ26t 2L18RbS7P6QDSQzTCkTV77IScVHGnpOIjzeqapPpAhzUpW/ShINK3mZwMzGS5VZIBfp0 HBcL6IjaMKsdIekNd4NvAtOPdAamWZQ7zu2bBrfhtLM1bADMX57BLvQCSJnYzjBsmNCR zA== Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by userp2130.oracle.com with ESMTP id 2qyh8u8h2u-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 05 Mar 2019 22:50:05 +0000 Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id x25Mnx5I010150 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 5 Mar 2019 22:49:59 GMT Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x25Mnw3l023512; Tue, 5 Mar 2019 22:49:58 GMT Received: from [10.159.129.123] (/10.159.129.123) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 05 Mar 2019 14:49:58 -0800 Subject: Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces To: "Michael S. Tsirkin" References: <1551747059-11831-1-git-send-email-si-wei.liu@oracle.com> <20190304213032-mutt-send-email-mst@kernel.org> <20190305152740-mutt-send-email-mst@kernel.org> Cc: Sridhar Samudrala , Stephen Hemminger , Jakub Kicinski , Jiri Pirko , David Miller , Netdev , virtualization@lists.linux-foundation.org, liran.alon@oracle.com, boris.ostrovsky@oracle.com, vijay.balakrishna@oracle.com From: si-wei liu Organization: Oracle Corporation Message-ID: <99a65fbd-0dd2-e5bc-d555-2f559d8287a7@oracle.com> Date: Tue, 5 Mar 2019 14:49:53 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20190305152740-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9186 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1903050146 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 3/5/2019 12:28 PM, Michael S. Tsirkin wrote: > On Tue, Mar 05, 2019 at 11:19:32AM -0800, si-wei liu wrote: >> >> On 3/4/2019 6:33 PM, Michael S. Tsirkin wrote: >>> On Mon, Mar 04, 2019 at 07:50:59PM -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. >>>> >>>> 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 >>> Not sure which of the versions I should reply to. >> Sorry for multiple copies sent. It's fine to reply to this one. >> >>> I have a vague idea: would it work to *not* set >>> IFF_UP on slave devices at all? >> Hmm, I ever thought about this option, and it appears this solution is more >> invasive than required to convert existing scripts, despite the controversy >> of introducing internal netdev state to differentiate user visible state. >> Either we disallow slave to be brought up by user, or to not set IFF_UP flag >> but instead use the internal one, could end up with substantial behavioral >> change that breaks scripts. Consider any admin script that does `ip link set >> dev ... up' successfully just assumes the link is up and subsequent >> operation can be done as usual. While it *may* work for dracut (yet to be >> verified), I'm a bit concerned that there are more scripts to be converted >> than those that don't follow volatile failover slave names. It's technically >> doable, but may not worth the effort (in terms of porting existing >> scripts/apps). >> >> Thanks >> -Siwei > > Right. Advantage could be that we prevent all kind of > misconfigurations e.g. when one has a route on a slave. The fix for the slave route problem is already there in dracut. The ship has sailed, no matter how seamless upstream thought failover could work with the existing userspace. I would rather avoid introducing more breakage to userspace if there's simple yet less intrusive way to fix the rename issue itself. -Siwei > >>> Would this reduce the chances of existing scripts such as dracut being >>> confused? >>> >>> And this leaves open the option for scripts to address >>> slaves by checking some custom attribute. >>> >>>> --- >>>> 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"); >>>> 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