| 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: <0548f1e3-1cc6-4194-99ff-20fb00205574@default>
Date: Mon, 22 Oct 2018 00:29:13 -0700 (PDT)
From: Manish Kumar Singh <mk.singh@...cle.com>
To: Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Cc: Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
"David S. Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org
Subject: RE: [PATCH] bonding: avoid repeated display of same link status
change
> -----Original Message-----
> From: Manish Kumar Singh
> Sent: 24 सितम्बर 2018 12:36
> To: Eric Dumazet; netdev@...r.kernel.org
> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> kernel@...r.kernel.org
> Subject: RE: [PATCH] bonding: avoid repeated display of same link status
> change
>
>
>
> > -----Original Message-----
> > From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> > Sent: 18 सितम्बर 2018 19:30
> > To: Manish Kumar Singh; Eric Dumazet; netdev@...r.kernel.org
> > Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> > kernel@...r.kernel.org
> > Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> > change
> >
> >
> >
> > On 09/17/2018 10:05 PM, Manish Kumar Singh wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> > >> Sent: 17 सितम्बर 2018 20:08
> > >> To: Manish Kumar Singh; netdev@...r.kernel.org
> > >> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller;
> > linux-
> > >> kernel@...r.kernel.org
> > >> Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> > >> change
> > >>
> > >>
> > >>
> > >> On 09/17/2018 12:20 AM, mk.singh@...cle.com wrote:
> > >>> From: Manish Kumar Singh <mk.singh@...cle.com>
> > >>>
> > >>> When link status change needs to be committed and rtnl lock couldn't
> be
> > >>> taken, avoid redisplay of same link status change message.
> > >>>
> > >>> Signed-off-by: Manish Kumar Singh <mk.singh@...cle.com>
> > >>> ---
> > >>> drivers/net/bonding/bond_main.c | 6 ++++--
> > >>> include/net/bonding.h | 1 +
> > >>> 2 files changed, 5 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/net/bonding/bond_main.c
> > >> b/drivers/net/bonding/bond_main.c
> > >>> index 217b790d22ed..fb4e3aff1677 100644
> > >>> --- a/drivers/net/bonding/bond_main.c
> > >>> +++ b/drivers/net/bonding/bond_main.c
> > >>> @@ -2087,7 +2087,7 @@ static int bond_miimon_inspect(struct
> bonding
> > >> *bond)
> > >>> bond_propose_link_state(slave, BOND_LINK_FAIL);
> > >>> commit++;
> > >>> slave->delay = bond->params.downdelay;
> > >>> - if (slave->delay) {
> > >>> + if (slave->delay && !bond->rtnl_needed) {
> > >>> netdev_info(bond->dev, "link status down
> > for
> > >> %sinterface %s, disabling it in %d ms\n",
> > >>> (BOND_MODE(bond) ==
> > >>> BOND_MODE_ACTIVEBACKUP) ?
> > >>> @@ -2127,7 +2127,7 @@ static int bond_miimon_inspect(struct
> bonding
> > >> *bond)
> > >>> commit++;
> > >>> slave->delay = bond->params.updelay;
> > >>>
> > >>> - if (slave->delay) {
> > >>> + if (slave->delay && !bond->rtnl_needed) {
> > >>> netdev_info(bond->dev, "link status up for
> > >> interface %s, enabling it in %d ms\n",
> > >>> slave->dev->name,
> > >>> ignore_updelay ? 0 :
> > >>> @@ -2301,9 +2301,11 @@ static void bond_mii_monitor(struct
> > >> work_struct *work)
> > >>> if (!rtnl_trylock()) {
> > >>> delay = 1;
> > >>> should_notify_peers = false;
> > >>> + bond->rtnl_needed = true;
> > >>
> > >> How can you set a shared variable with no synchronization ?
> > > Thanks Eric for reviewing the patch. rtnl_needed is not a shared variable,
> it
> > is part of bonding structure, that is one per bonding driver instance. There
> > can't be two parallel instances of bond_miimon_inspect for a
> single bonding
> > driver instance at any given point of time. and only bond_miimon_inspect
> > updates it. That’s why I think there is no need of any synchronization here.
> > >
> > >
> >
> > If rtnl_trylock() can not grab RTNL,
> > there is no way the current thread can set the variable without a race, if
> the
> > word including rtnl_needed is shared by other fields in the structure.
> >
> > Your patch adds a subtle possibility of future bugs, even if it runs fine
> today.
> >
> > Do not pave the way for future bugs, make your code robust, please.
>
> Thankyou Eric, we are making the changes and will repost the patch after
> testing it.
> -Manish
Please review the updated patch, I have changed the rtnl_needed to an atomic variable, to avoid any race condition:
From: Manish Kumar Singh <mk.singh@...cle.com>
When link status change needs to be committed and rtnl lock couldn't be taken, avoid redisplay of same link status change message.
Signed-off-by: Manish Kumar Singh <mk.singh@...cle.com>
---
drivers/net/bonding/bond_main.c | 6 ++++--
include/net/bonding.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 217b790d22ed..fac5350bf19c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2087,7 +2087,7 @@ static int bond_miimon_inspect(struct bonding *bond)
bond_propose_link_state(slave, BOND_LINK_FAIL);
commit++;
slave->delay = bond->params.downdelay;
- if (slave->delay) {
+ if (slave->delay && !atomic_read(&bond->rtnl_needed)) {
netdev_info(bond->dev, "link status down for %sinterface %s, disabling it in %d ms\n",
(BOND_MODE(bond) ==
BOND_MODE_ACTIVEBACKUP) ?
@@ -2127,7 +2127,7 @@ static int bond_miimon_inspect(struct bonding *bond)
commit++;
slave->delay = bond->params.updelay;
- if (slave->delay) {
+ if (slave->delay && !atomic_read(&bond->rtnl_needed)) {
netdev_info(bond->dev, "link status up for interface %s, enabling it in %d ms\n",
slave->dev->name,
ignore_updelay ? 0 :
@@ -2301,9 +2301,11 @@ static void bond_mii_monitor(struct work_struct *work)
if (!rtnl_trylock()) {
delay = 1;
should_notify_peers = false;
+ atomic_set(&bond->rtnl_needed, 1);
goto re_arm;
}
+ atomic_set(&bond->rtnl_needed, 0);
bond_for_each_slave(bond, slave, iter) {
bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER);
}
diff --git a/include/net/bonding.h b/include/net/bonding.h index 808f1d167349..ffc1219f7a07 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -234,6 +234,7 @@ struct bonding {
struct dentry *debug_dir;
#endif /* CONFIG_DEBUG_FS */
struct rtnl_link_stats64 bond_stats;
+ atomic_t rtnl_needed;
};
#define bond_slave_get_rcu(dev) \
--
2.14.1
Thanks,
Manish
> >
> >
> >
> >
> >
> >
> >
Powered by blists - more mailing lists