Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage.
From: Jiri Pirko <jiri@resnulli.us>
Date: 2016-09-08 15:17:12
Also in:
lkml
Thu, Sep 08, 2016 at 05:05:05PM CEST, jasminder.kaur@hpe.com wrote:
Hi Jay, Hi Jiri, Thank you for your inputs. Some of the requests we got for such preventive checks are from Admins working on large scale up systems with multiple NICs, FlexNICs and IP addresses. § One use case for these checks is to give an alert, in case of any accidental removals owing to operator errors on large configurations. § Another use case is during online maintenance activities such as dynamic patching or a driver load/unload operation. Admin's would shut down applications and delete affected interfaces before unload of a driver. They would prefer to get an alert during delete operation in case some usages linger around. Such alerts are more useful in Cluster configurations, Network Attached Storage( NAS) configurations, VM configurations with Guests, etc. So these were mainly the situations that prompted us to add such checks in delete paths. True these checks are not comprehensive for all use cases, we would like to extend this if it can cover more scenarios. sysfs based use cases were the ones we noticed for bond/slave configurations. Do you suggest other CLI's such as “ip link” is more commonly used ? Possibly if these checks are rearranged a bit in code, multiple such CLI interfaces can be covered ? Please let us know.
Please avoid top-posting. It is annoying :(
Thanks & Regards, Jasminder -----Original Message----- From: Jay Vosburgh [mailto:jay.vosburgh@canonical.com] Sent: Tuesday, September 06, 2016 8:39 PM To: Kaur, Jasminder (STSD) <redacted> Cc: vfalico@gmail.com; gospo@cumulusnetworks.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gurunath, Vasundhara (STSD) <redacted>; Arackal, Paulose Kuriakose (STSD) <redacted> Subject: Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. Kaur, Jasminder <jasminder.kaur@hpe.com<mailto:jasminder.kaur@hpe.com>> wrote:quoted
From: "Kaur, Jasminder" <jasminder.kaur@hpe.com<mailto:jasminder.kaur@hpe.com>>quoted
quoted
If a bond is in use such as with IP address configured, removing it canquoted
result in application disruptions. If bond is used for clusterquoted
communication or network file system interfaces, removing it can causequoted
system down time.quoted
quoted
An additional write option “?-” is added to sysfs bond interfaces asquoted
below, in order to prevent accidental deletions while bond is in use.quoted
In the absence of any usage, the below option proceeds with bond deletion.quoted
“ echo "?-bondX" > /sys/class/net/bonding_masters “ .quoted
If usage is detected such as an IP address configured, deletion isquoted
prevented with appropriate message logged to syslog.The issue of interfaces being arbitrarily changed or deleted is not specific to bonding, and could affect any networking device (physical or virtual). Thus, if a facility such as this is to be provided, it should be generic, not specific to bonding. Separately, I'm not sure I see the value of such an option. Other than administrator error, I'm not sure when bonds (or other interfaces) would be randomly deleted. Are you seeing that happening? Also, this patch does not prevent other errors or malicious change, e.g., "ip link set bondX down" or "ip addr del 1.2.3.4/24" would still cause the service disruption you're trying to avoid. And, lastly, what Jiri said: use netlink for new bonding functionality, not sysfs. -Jquoted
In the absence of any usage, the below option proceeds with deletion ofquoted
slaves from a bond.quoted
“ echo "?-enoX" > /sys/class/net/bondX/bonding/slaves “ .quoted
If usage is detected such as an IP address configured on bond, deletionquoted
is prevented if the last slave is being removed from bond.quoted
An appropriate message is logged to syslog.quoted
quoted
Signed-off-by: Jasminder Kaur <jasminder.kaur@hpe.com<mailto:jasminder.kaur@hpe.com>>quoted
---quoted
drivers/net/bonding/bond_options.c | 24 ++++++++++++++++++++++--quoted
drivers/net/bonding/bond_sysfs.c | 35 +++++++++++++++++++++++++++++++++--quoted
2 files changed, 55 insertions(+), 4 deletions(-)quoted
quoted
diff --git a/drivers/net/bonding/bond_options.cquoted
b/drivers/net/bonding/bond_options.cquoted
index 577e57c..e7640ea 100644quoted
--- a/drivers/net/bonding/bond_options.cquoted
+++ b/drivers/net/bonding/bond_options.cquoted
@@ -1335,9 +1335,15 @@ static int bond_option_slaves_set(struct bonding *bond,quoted
struct net_device *dev;quoted
char *ifname;quoted
int ret;quoted
+ struct in_device *in_dev;quoted
quoted
sscanf(newval->string, "%16s", command); /* IFNAMSIZ*/quoted
- ifname = command + 1;quoted
+quoted
+ if ((command[0] == '?') && (command[1] == '-'))quoted
+ ifname = command + 2;quoted
+ elsequoted
+ ifname = command + 1;quoted
+quoted
if ((strlen(command) <= 1) ||quoted
!dev_valid_name(ifname))quoted
goto err_no_cmd;quoted
@@ -1356,6 +1362,20 @@ static int bond_option_slaves_set(struct bonding *bond,quoted
ret = bond_enslave(bond->dev, dev);quoted
break;quoted
quoted
+ case '?':quoted
+ if (command[1] == '-') {quoted
+ in_dev = __in_dev_get_rtnl(bond->dev);quoted
+ if ((bond->slave_cnt == 1) &"ed
+ ((in_dev->ifa_list) != NULL)) {quoted
+ netdev_info(bond->dev, "attempt to remove last slave %s from bond.\n",quoted
+ dev->name);quoted
+ ret = -EBUSY;quoted
+ break;quoted
+ }quoted
+ } else {quoted
+ goto err_no_cmd;quoted
+ }quoted
+quoted
case '-':quoted
netdev_info(bond->dev, "Removing slave %s\n", dev->name);quoted
ret = bond_release(bond->dev, dev);quoted
@@ -1369,7 +1389,7 @@ out:quoted
return ret;quoted
quoted
err_no_cmd:quoted
- netdev_err(bond->dev, "no command found in slaves file - use +ifname or -ifname\n");quoted
+ netdev_err(bond->dev, "no command found in slaves file - use +ifnamequoted
+or -ifname or ?-ifname\n");quoted
ret = -EPERM;quoted
goto out;quoted
}quoted
diff --git a/drivers/net/bonding/bond_sysfs.cquoted
b/drivers/net/bonding/bond_sysfs.cquoted
index e23c3ed..7c2ef64 100644quoted
--- a/drivers/net/bonding/bond_sysfs.cquoted
+++ b/drivers/net/bonding/bond_sysfs.cquoted
@@ -102,7 +102,12 @@ static ssize_t bonding_store_bonds(struct class *cls,quoted
int rv, res = count;quoted
quoted
sscanf(buffer, "%16s", command); /* IFNAMSIZ*/quoted
- ifname = command + 1;quoted
+quoted
+ if ((command[0] == '?') && (command[1] == '-'))quoted
+ ifname = command + 2;quoted
+ elsequoted
+ ifname = command + 1;quoted
+quoted
if ((strlen(command) <= 1) ||quoted
!dev_valid_name(ifname))quoted
goto err_no_cmd;quoted
@@ -130,6 +135,32 @@ static ssize_t bonding_store_bonds(struct class *cls,quoted
res = -ENODEV;quoted
}quoted
rtnl_unlock();quoted
+ } else if ((command[0] == '?') && (command[1] == '-')) {quoted
+ struct net_device *bond_dev;quoted
+quoted
+ rtnl_lock();quoted
+ bond_dev = bond_get_by_name(bn, ifname);quoted
+quoted
+ if (bond_dev) {quoted
+ struct in_device *in_dev;quoted
+ struct bonding *bond = netdev_priv(bond_dev);quoted
+quoted
+ in_dev = __in_dev_get_rtnl(bond_dev);quoted
+quoted
+ if (((in_dev->ifa_list) != NULL) &"ed
+ (bond->slave_cnt > 0)) {quoted
+ pr_err("%s is in use. Unconfigure IP %pI4 before deletion.\n",quoted
+ ifname, &in_dev->ifa_list->ifa_local);quoted
+ rtnl_unlock();quoted
+ return -EBUSY;quoted
+ }quoted
+ pr_info("%s is being deleted...\n", ifname);quoted
+ unregister_netdevice(bond_dev);quoted
+ } else {quoted
+ pr_err("unable to delete non-existent %s\n", ifname);quoted
+ res = -ENODEV;quoted
+ }quoted
+ rtnl_unlock();quoted
} elsequoted
goto err_no_cmd;quoted
quoted
@@ -139,7 +170,7 @@ static ssize_t bonding_store_bonds(struct class *cls,quoted
return res;quoted
quoted
err_no_cmd:quoted
- pr_err("no command found in bonding_masters - use +ifname or -ifname\n");quoted
+ pr_err("no command found in bonding_masters - use +ifname or -ifnamequoted
+or ?-ifname\n");quoted
return -EPERM;quoted
}quoted
quoted
--quoted
1.8.3.1quoted
--- -Jay Vosburgh, jay.vosburgh@canonical.com<mailto:jay.vosburgh@canonical.com>