Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage.

[Jiri Pirko]

Thu, Sep 08, 2016 at 05:05:05PM CEST, jasminder.kaur@xxxxxxx 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@xxxxxxxxxxxxx]
>Sent: Tuesday, September 06, 2016 8:39 PM
>To: Kaur, Jasminder (STSD) <jasminder.kaur@xxxxxxx>
>Cc: vfalico@xxxxxxxxx; gospo@xxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Gurunath, Vasundhara (STSD) <vasundhara.gurunath@xxxxxxx>; Arackal, Paulose Kuriakose (STSD) <paulose.kuriakose.arackal@xxxxxxx>
>Subject: Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage.
>
>
>
>Kaur, Jasminder <jasminder.kaur@xxxxxxx<mailto:jasminder.kaur@xxxxxxx>> wrote:
>
>
>
>>From: "Kaur, Jasminder" <jasminder.kaur@xxxxxxx<mailto:jasminder.kaur@xxxxxxx>>
>
>>
>
>>If a bond is in use such as with IP address configured, removing it can
>
>>result in application disruptions. If bond is used for cluster
>
>>communication or network file system interfaces, removing it can cause
>
>>system down time.
>
>>
>
>>An additional write option â?-â is added to sysfs bond interfaces as
>
>>below, in order to prevent accidental deletions while bond is in use.
>
>>In the absence of any usage, the below option proceeds with bond deletion.
>
>>â echo "?-bondX" > /sys/class/net/bonding_masters â .
>
>>If usage is detected such as an IP address configured, deletion is
>
>>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.
>
>
>
>                -J
>
>
>
>>In the absence of any usage, the below option proceeds with deletion of
>
>>slaves from a bond.
>
>>â echo "?-enoX" > /sys/class/net/bondX/bonding/slaves â .
>
>>If usage is detected such as an IP address configured on bond, deletion
>
>>is prevented if the last slave is being removed from bond.
>
>>An appropriate message is logged to syslog.
>
>>
>
>>Signed-off-by: Jasminder Kaur <jasminder.kaur@xxxxxxx<mailto:jasminder.kaur@xxxxxxx>>
>
>>---
>
>> drivers/net/bonding/bond_options.c | 24 ++++++++++++++++++++++--
>
>> drivers/net/bonding/bond_sysfs.c   | 35 +++++++++++++++++++++++++++++++++--
>
>> 2 files changed, 55 insertions(+), 4 deletions(-)
>
>>
>
>>diff --git a/drivers/net/bonding/bond_options.c
>
>>b/drivers/net/bonding/bond_options.c
>
>>index 577e57c..e7640ea 100644
>
>>--- a/drivers/net/bonding/bond_options.c
>
>>+++ b/drivers/net/bonding/bond_options.c
>
>>@@ -1335,9 +1335,15 @@ static int bond_option_slaves_set(struct bonding *bond,
>
>>             struct net_device *dev;
>
>>             char *ifname;
>
>>             int ret;
>
>>+           struct in_device *in_dev;
>
>>
>
>>             sscanf(newval->string, "%16s", command); /* IFNAMSIZ*/
>
>>-            ifname = command + 1;
>
>>+
>
>>+           if ((command[0] == '?') && (command[1] == '-'))
>
>>+                           ifname = command + 2;
>
>>+           else
>
>>+                           ifname = command + 1;
>
>>+
>
>>             if ((strlen(command) <= 1) ||
>
>>                 !dev_valid_name(ifname))
>
>>                             goto err_no_cmd;
>
>>@@ -1356,6 +1362,20 @@ static int bond_option_slaves_set(struct bonding *bond,
>
>>                             ret = bond_enslave(bond->dev, dev);
>
>>                             break;
>
>>
>
>>+           case '?':
>
>>+                           if (command[1] == '-') {
>
>>+                                           in_dev = __in_dev_get_rtnl(bond->dev);
>
>>+                                           if ((bond->slave_cnt == 1) &&
>
>>+                                               ((in_dev->ifa_list) != NULL)) {
>
>>+                                                           netdev_info(bond->dev, "attempt to remove last slave %s from bond.\n",
>
>>+                                                                               dev->name);
>
>>+                                                           ret = -EBUSY;
>
>>+                                                           break;
>
>>+                                           }
>
>>+                           } else {
>
>>+                                           goto err_no_cmd;
>
>>+                           }
>
>>+
>
>>             case '-':
>
>>                             netdev_info(bond->dev, "Removing slave %s\n", dev->name);
>
>>                             ret = bond_release(bond->dev, dev);
>
>>@@ -1369,7 +1389,7 @@ out:
>
>>             return ret;
>
>>
>
>> err_no_cmd:
>
>>-            netdev_err(bond->dev, "no command found in slaves file - use +ifname or -ifname\n");
>
>>+           netdev_err(bond->dev, "no command found in slaves file - use +ifname
>
>>+or -ifname or ?-ifname\n");
>
>>             ret = -EPERM;
>
>>             goto out;
>
>> }
>
>>diff --git a/drivers/net/bonding/bond_sysfs.c
>
>>b/drivers/net/bonding/bond_sysfs.c
>
>>index e23c3ed..7c2ef64 100644
>
>>--- a/drivers/net/bonding/bond_sysfs.c
>
>>+++ b/drivers/net/bonding/bond_sysfs.c
>
>>@@ -102,7 +102,12 @@ static ssize_t bonding_store_bonds(struct class *cls,
>
>>             int rv, res = count;
>
>>
>
>>             sscanf(buffer, "%16s", command); /* IFNAMSIZ*/
>
>>-            ifname = command + 1;
>
>>+
>
>>+           if ((command[0] == '?') && (command[1] == '-'))
>
>>+                           ifname = command + 2;
>
>>+           else
>
>>+                           ifname = command + 1;
>
>>+
>
>>             if ((strlen(command) <= 1) ||
>
>>                 !dev_valid_name(ifname))
>
>>                             goto err_no_cmd;
>
>>@@ -130,6 +135,32 @@ static ssize_t bonding_store_bonds(struct class *cls,
>
>>                                             res = -ENODEV;
>
>>                             }
>
>>                             rtnl_unlock();
>
>>+           } else if ((command[0] == '?') && (command[1] == '-')) {
>
>>+                           struct net_device *bond_dev;
>
>>+
>
>>+                           rtnl_lock();
>
>>+                           bond_dev = bond_get_by_name(bn, ifname);
>
>>+
>
>>+                           if (bond_dev) {
>
>>+                                           struct in_device *in_dev;
>
>>+                                           struct bonding *bond = netdev_priv(bond_dev);
>
>>+
>
>>+                                           in_dev = __in_dev_get_rtnl(bond_dev);
>
>>+
>
>>+                                           if (((in_dev->ifa_list) != NULL) &&
>
>>+                                               (bond->slave_cnt > 0)) {
>
>>+                                                           pr_err("%s is in use. Unconfigure IP %pI4 before deletion.\n",
>
>>+                                                                  ifname, &in_dev->ifa_list->ifa_local);
>
>>+                                                           rtnl_unlock();
>
>>+                                                           return -EBUSY;
>
>>+                                           }
>
>>+                                           pr_info("%s is being deleted...\n", ifname);
>
>>+                                           unregister_netdevice(bond_dev);
>
>>+                           } else {
>
>>+                                           pr_err("unable to delete non-existent %s\n", ifname);
>
>>+                                           res = -ENODEV;
>
>>+                           }
>
>>+                           rtnl_unlock();
>
>>             } else
>
>>                             goto err_no_cmd;
>
>>
>
>>@@ -139,7 +170,7 @@ static ssize_t bonding_store_bonds(struct class *cls,
>
>>             return res;
>
>>
>
>> err_no_cmd:
>
>>-            pr_err("no command found in bonding_masters - use +ifname or -ifname\n");
>
>>+           pr_err("no command found in bonding_masters - use +ifname or -ifname
>
>>+or ?-ifname\n");
>
>>             return -EPERM;
>
>> }
>
>>
>
>>--
>
>>1.8.3.1
>
>>
>
>
>
>---
>
>                -Jay Vosburgh, jay.vosburgh@xxxxxxxxxxxxx<mailto:jay.vosburgh@xxxxxxxxxxxxx>