* [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. @ 2016-09-06 9:34 Kaur, Jasminder 2016-09-06 14:59 ` Jiri Pirko 2016-09-06 15:08 ` Jay Vosburgh 0 siblings, 2 replies; 6+ messages in thread From: Kaur, Jasminder @ 2016-09-06 9:34 UTC (permalink / raw) To: j.vosburgh, vfalico, gospo Cc: netdev, linux-kernel, vasundhara.gurunath, paulose.kuriakose.arackal, Kaur, Jasminder From: "Kaur, Jasminder" <jasminder.kaur@hpe.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. 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@hpe.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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. 2016-09-06 9:34 [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage Kaur, Jasminder @ 2016-09-06 14:59 ` Jiri Pirko 2016-09-06 15:08 ` Jay Vosburgh 1 sibling, 0 replies; 6+ messages in thread From: Jiri Pirko @ 2016-09-06 14:59 UTC (permalink / raw) To: Kaur, Jasminder Cc: j.vosburgh, vfalico, gospo, netdev, linux-kernel, vasundhara.gurunath, paulose.kuriakose.arackal Tue, Sep 06, 2016 at 11:34:30AM CEST, jasminder.kaur@hpe.com wrote: >From: "Kaur, Jasminder" <jasminder.kaur@hpe.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. > >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. NACK sysfs bonding iface should die in peace, don't poke in it. Either fix you application or fix you configuration flow. Don't do this in kernel. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. 2016-09-06 9:34 [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage Kaur, Jasminder 2016-09-06 14:59 ` Jiri Pirko @ 2016-09-06 15:08 ` Jay Vosburgh [not found] ` <AT5PR84MB01151743CF3182FAFC91D7F98AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM> 2016-09-09 0:57 ` Stephen Hemminger 1 sibling, 2 replies; 6+ messages in thread From: Jay Vosburgh @ 2016-09-06 15:08 UTC (permalink / raw) To: Kaur, Jasminder Cc: vfalico, gospo, netdev, linux-kernel, vasundhara.gurunath, paulose.kuriakose.arackal Kaur, Jasminder <jasminder.kaur@hpe.com> wrote: >From: "Kaur, Jasminder" <jasminder.kaur@hpe.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@hpe.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@canonical.com ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <AT5PR84MB01151743CF3182FAFC91D7F98AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM>]
* Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. [not found] ` <AT5PR84MB01151743CF3182FAFC91D7F98AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM> @ 2016-09-08 15:17 ` Jiri Pirko [not found] ` <AT5PR84MB011502FB43B95AF91ADF6F0E8AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM> 0 siblings, 1 reply; 6+ messages in thread From: Jiri Pirko @ 2016-09-08 15:17 UTC (permalink / raw) To: Kaur, Jasminder (STSD) Cc: Jay Vosburgh, vfalico, gospo, netdev, linux-kernel, Gurunath, Vasundhara (STSD), Arackal, Paulose Kuriakose (STSD) 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) <jasminder.kaur@hpe.com> >Cc: vfalico@gmail.com; gospo@cumulusnetworks.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Gurunath, Vasundhara (STSD) <vasundhara.gurunath@hpe.com>; Arackal, Paulose Kuriakose (STSD) <paulose.kuriakose.arackal@hpe.com> >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: > > > >>From: "Kaur, Jasminder" <jasminder.kaur@hpe.com<mailto:jasminder.kaur@hpe.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@hpe.com<mailto:jasminder.kaur@hpe.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@canonical.com<mailto:jay.vosburgh@canonical.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <AT5PR84MB011502FB43B95AF91ADF6F0E8AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM>]
* Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. [not found] ` <AT5PR84MB011502FB43B95AF91ADF6F0E8AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM> @ 2016-09-09 6:38 ` Jiri Pirko 0 siblings, 0 replies; 6+ messages in thread From: Jiri Pirko @ 2016-09-09 6:38 UTC (permalink / raw) To: Kaur, Jasminder (STSD) Cc: Jay Vosburgh, vfalico, gospo, netdev, linux-kernel, Gurunath, Vasundhara (STSD), Arackal, Paulose Kuriakose (STSD) Thu, Sep 08, 2016 at 06:32:02PM CEST, jasminder.kaur@hpe.com wrote: >> 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. > >> > > > >Re-sending my response as per Jiri's input to avoid top-posting.. Hope this is fine. > > > >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. If admin is stupid and shoots himself in a foot, it's his problem. Kernel's work is not to babysit him. Stop wasting the time. > >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. > > > >Thanks & Regards, > >Jasminder > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage. 2016-09-06 15:08 ` Jay Vosburgh [not found] ` <AT5PR84MB01151743CF3182FAFC91D7F98AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM> @ 2016-09-09 0:57 ` Stephen Hemminger 1 sibling, 0 replies; 6+ messages in thread From: Stephen Hemminger @ 2016-09-09 0:57 UTC (permalink / raw) To: Jay Vosburgh Cc: Kaur, Jasminder, vfalico, gospo, netdev, linux-kernel, vasundhara.gurunath, paulose.kuriakose.arackal On Tue, 06 Sep 2016 08:08:59 -0700 Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > Kaur, Jasminder <jasminder.kaur@hpe.com> wrote: > > >From: "Kaur, Jasminder" <jasminder.kaur@hpe.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@hpe.com> I agree with Jay. Unless the kernel would crash there is no reason to prevent a user with sufficient permissions from deleting a device. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-09 6:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 9:34 [PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage Kaur, Jasminder
2016-09-06 14:59 ` Jiri Pirko
2016-09-06 15:08 ` Jay Vosburgh
[not found] ` <AT5PR84MB01151743CF3182FAFC91D7F98AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM>
2016-09-08 15:17 ` Jiri Pirko
[not found] ` <AT5PR84MB011502FB43B95AF91ADF6F0E8AFB0@AT5PR84MB0115.NAMPRD84.PROD.OUTLOOK.COM>
2016-09-09 6:38 ` Jiri Pirko
2016-09-09 0:57 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).