| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160908151705.GD1842@nanopsycho.orion>
Date: Thu, 8 Sep 2016 17:17:05 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kaur, Jasminder (STSD)" <jasminder.kaur@....com>
Cc: Jay Vosburgh <jay.vosburgh@...onical.com>,
"vfalico@...il.com" <vfalico@...il.com>,
"gospo@...ulusnetworks.com" <gospo@...ulusnetworks.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Gurunath, Vasundhara (STSD)" <vasundhara.gurunath@....com>,
"Arackal, Paulose Kuriakose (STSD)"
<paulose.kuriakose.arackal@....com>
Subject: Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave
from a bond, with active usage.
Thu, Sep 08, 2016 at 05:05:05PM CEST, jasminder.kaur@....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@...onical.com]
>Sent: Tuesday, September 06, 2016 8:39 PM
>To: Kaur, Jasminder (STSD) <jasminder.kaur@....com>
>Cc: vfalico@...il.com; gospo@...ulusnetworks.com; netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Gurunath, Vasundhara (STSD) <vasundhara.gurunath@....com>; Arackal, Paulose Kuriakose (STSD) <paulose.kuriakose.arackal@....com>
>Subject: Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage.
>
>
>
>Kaur, Jasminder <jasminder.kaur@....com<mailto:jasminder.kaur@....com>> wrote:
>
>
>
>>From: "Kaur, Jasminder" <jasminder.kaur@....com<mailto:jasminder.kaur@....com>>
>
>>
>
>>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@....com<mailto:jasminder.kaur@....com>>
>
>>---
>
>> 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@...onical.com<mailto:jay.vosburgh@...onical.com>
Powered by blists - more mailing lists