[PATCH net-next] Bonding: Convert multiple netdev_info messages to netdev_dbg

[Michael J Dilmore]

The bond_options.c file contains multiple netdev_info statements that clutter 
kernel output.
This patch replaces all netdev_info with netdev_dbg and adds a netdev_dbg 
statement for the
packets per slave parameter. Also fixes misalignment at line 467.

Suggested-by: Joe Perches <j...@perches.com>
Signed-off-by: Michael J Dilmore <michael.j.dilm...@gmail.com>
---
 drivers/net/bonding/bond_options.c | 132 +++--
 1 file changed, 67 insertions(+), 65 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c 
b/drivers/net/bonding/bond_options.c
index 1bcbb89..b1e5b8a 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -464,7 +464,7 @@ const struct bond_opt_value *bond_opt_get_val(unsigned int 
option, u64 val)
 
 /* Searches for a value in opt's values[] table which matches the flagmask */
 static const struct bond_opt_value *bond_opt_get_flags(const struct 
bond_option *opt,
-u32 flagmask)
+  u32 flagmask)
 {
int i;
 
@@ -721,14 +721,14 @@ static int bond_option_mode_set(struct bonding *bond,
const struct bond_opt_value *newval)
 {
if (!bond_mode_uses_arp(newval->value) && bond->params.arp_interval) {
-   netdev_info(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
-   newval->string);
+   netdev_dbg(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
+  newval->string);
/* disable arp monitoring */
bond->params.arp_interval = 0;
/* set miimon to default value */
bond->params.miimon = BOND_DEFAULT_MIIMON;
-   netdev_info(bond->dev, "Setting MII monitoring interval to 
%d\n",
-   bond->params.miimon);
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %d\n",
+  bond->params.miimon);
}
 
/* don't cache arp_validate between modes */
@@ -771,7 +771,7 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
block_netpoll_tx();
/* check to see if we are clearing active */
if (!slave_dev) {
-   netdev_info(bond->dev, "Clearing current active slave\n");
+   netdev_dbg(bond->dev, "Clearing current active slave\n");
RCU_INIT_POINTER(bond->curr_active_slave, NULL);
bond_select_active_slave(bond);
} else {
@@ -782,13 +782,13 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 
if (new_active == old_active) {
/* do nothing */
-   netdev_info(bond->dev, "%s is already the current 
active slave\n",
-   new_active->dev->name);
+   netdev_dbg(bond->dev, "%s is already the current active 
slave\n",
+  new_active->dev->name);
} else {
if (old_active && (new_active->link == BOND_LINK_UP) &&
bond_slave_is_up(new_active)) {
-   netdev_info(bond->dev, "Setting %s as active 
slave\n",
-   new_active->dev->name);
+   netdev_dbg(bond->dev, "Setting %s as active 
slave\n",
+  new_active->dev->name);
bond_change_active_slave(bond, new_active);
} else {
netdev_err(bond->dev, "Could not set %s as 
active slave; either %s is down or the link is down\n",
@@ -810,17 +810,17 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 static int bond_option_miimon_set(struct bonding *bond,
  const struct bond_opt_value *newval)
 {
-   netdev_info(bond->dev, "Setting MII monitoring interval to %llu\n",
-   newval->value);
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %llu\n",
+  newval->value);
bond->params.miimon = newval->value;
if (bond->params.updelay)
-   netdev_info(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
-   bond->params.updelay * bond->params.miimon);
+   netdev_dbg(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
+   

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

On 21/06/17 23:39, Jay Vosburgh wrote:

Michael J Dilmore <michael.j.dilm...@gmail.com> wrote:


On 21/06/17 22:56, David Miller wrote:


From: Michael D <michael.j.dilm...@gmail.com>
Date: Wed, 21 Jun 2017 22:41:07 +0100


I don't think you can stop it being dereferenced... you just need to
prevent an attacker from exploiting the null pointer dereference
vulnerability right? And this is done by returning the function right
away?

What's all of this about an "attacker"?

If there is a bug, we dererence a NULL pointer, and we should
fix that bug.

The BUG_ON() helps us see where the problem is while at the
same time stopping the kernel before the NULL deref happens.

Ok this is starting to make sense now - went a bit off track but think my
general thinking is ok - i.e. if we return the function with an error code
before the dereference then this basically does the same thing as BUG_ON
but without crashing the kernel.

Something like:

if (WARN_ON(!new_active_slave) {
netdev_dbg("Can't add new active slave - pointer null");
return ERROR_CODE
}

In general, yes, but in this case, the condition should be
impossible to hit, so BUG_ON seems appropriate.

If bond_slave_get_rtnl/rcu() returns NULL for an actual bonding
slave, other code paths (bond_fill_slave_info, bond_handle_frame) will
likely crash before getting to this one.

-J

---
-Jay Vosburgh, jay.vosbu...@canonical.com
That did cross my mind but I read that Linus that was quite averse to 
BUG_ON anywhere in the kernel so thought it might be have been worth doing.


Is it worth at least wrapping BUG_ON in an unlikely macro then?

Re: [PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

On 21/06/17 22:56, David Miller wrote:


From: Michael D 
Date: Wed, 21 Jun 2017 22:41:07 +0100


I don't think you can stop it being dereferenced... you just need to
prevent an attacker from exploiting the null pointer dereference
vulnerability right? And this is done by returning the function right
away?

What's all of this about an "attacker"?

If there is a bug, we dererence a NULL pointer, and we should
fix that bug.

The BUG_ON() helps us see where the problem is while at the
same time stopping the kernel before the NULL deref happens.
Ok this is starting to make sense now - went a bit off track but think 
my general thinking is ok - i.e. if we return the function with an error 
code before the dereference then this basically does the same thing as 
BUG_ON but without crashing the kernel.


Something like:

if (WARN_ON(!new_active_slave) {
netdev_dbg("Can't add new active slave - pointer null");
return ERROR_CODE
}

[PATCH] Convert BUG_ON to WARN_ON in bond_options.c

[Michael J Dilmore]

The function below contains a BUG_ON where no active slave is detected. The 
patch
converts this to a WARN_ON to avoid crashing the kernel.

Signed-off-by: Michael J Dilmore <michael.j.dilm...@gmail.com>
---
 drivers/net/bonding/bond_options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_options.c 
b/drivers/net/bonding/bond_options.c
index 1bcbb89..c4b4791 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -778,7 +778,7 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
struct slave *old_active = 
rtnl_dereference(bond->curr_active_slave);
struct slave *new_active = bond_slave_get_rtnl(slave_dev);
 
-   BUG_ON(!new_active);
+   WARN_ON(!new_active);
 
if (new_active == old_active) {
/* do nothing */
-- 
2.7.4

[PATCH] PATCH v3 Convert multiple netdev_info messages to netdev_dbg

[Michael J Dilmore]

The bond_options.c file contains multiple netdev_info messages that
clutter kernel output. This patches replaces these with netdev_dbg messages
and adds a netdev_dbg for packets for slave.

Signed-off-by: Michael J Dilmore <michael.j.dilm...@gmail.com>
Suggested-by: Joe Perches <j...@perches.com>
---
 drivers/net/bonding/bond_options.c | 54 +++---
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c 
b/drivers/net/bonding/bond_options.c
index e3a9af6..9110e5b 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -722,13 +722,13 @@ static int bond_option_mode_set(struct bonding *bond,
 {
if (!bond_mode_uses_arp(newval->value) && bond->params.arp_interval) {
netdev_dbg(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
-   newval->string);
+  newval->string);
/* disable arp monitoring */
bond->params.arp_interval = 0;
/* set miimon to default value */
bond->params.miimon = BOND_DEFAULT_MIIMON;
netdev_dbg(bond->dev, "Setting MII monitoring interval to %d\n",
-   bond->params.miimon);
+  bond->params.miimon);
}
 
/* don't cache arp_validate between modes */
@@ -783,12 +783,12 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
if (new_active == old_active) {
/* do nothing */
netdev_dbg(bond->dev, "%s is already the current active 
slave\n",
-   new_active->dev->name);
+  new_active->dev->name);
} else {
if (old_active && (new_active->link == BOND_LINK_UP) &&
bond_slave_is_up(new_active)) {
netdev_dbg(bond->dev, "Setting %s as active 
slave\n",
-   new_active->dev->name);
+  new_active->dev->name);
bond_change_active_slave(bond, new_active);
} else {
netdev_err(bond->dev, "Could not set %s as 
active slave; either %s is down or the link is down\n",
@@ -811,14 +811,14 @@ static int bond_option_miimon_set(struct bonding *bond,
  const struct bond_opt_value *newval)
 {
netdev_dbg(bond->dev, "Setting MII monitoring interval to %llu\n",
-   newval->value);
+  newval->value);
bond->params.miimon = newval->value;
if (bond->params.updelay)
netdev_dbg(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
-   bond->params.updelay * bond->params.miimon);
+  bond->params.updelay * bond->params.miimon);
if (bond->params.downdelay)
netdev_dbg(bond->dev, "Note: Updating downdelay (to %d) since 
it is a multiple of the miimon value\n",
-   bond->params.downdelay * bond->params.miimon);
+  bond->params.downdelay * bond->params.miimon);
if (newval->value && bond->params.arp_interval) {
netdev_dbg(bond->dev, "MII monitoring cannot be used with ARP 
monitoring - disabling ARP monitoring...\n");
bond->params.arp_interval = 0;
@@ -863,7 +863,7 @@ static int bond_option_updelay_set(struct bonding *bond,
}
bond->params.updelay = value / bond->params.miimon;
netdev_dbg(bond->dev, "Setting up delay to %d\n",
-   bond->params.updelay * bond->params.miimon);
+  bond->params.updelay * bond->params.miimon);
 
return 0;
 }
@@ -885,7 +885,7 @@ static int bond_option_downdelay_set(struct bonding *bond,
}
bond->params.downdelay = value / bond->params.miimon;
netdev_dbg(bond->dev, "Setting down delay to %d\n",
-   bond->params.downdelay * bond->params.miimon);
+  bond->params.downdelay * bond->params.miimon);
 
return 0;
 }
@@ -894,7 +894,7 @@ static int bond_option_use_carrier_set(struct bonding *bond,
   const struct bond_opt_value *newval)
 {
netdev_dbg(bond->dev, "Setting use_carrier to %llu\n",
-   newval->value);
+  newval->value

[PATCH] [PATCH v2 net-next] bonding: Convert multiple netdev_info messages to netdev_dbg

[Michael J Dilmore]

The bond_options.c file contains several netdev_info messages that clutter 
kernel output. This patch changes all netdev_info messages
to netdev_dbg and adds a netdev debug for the packets per slave parameter.

Suggested-by: Joe Perches <j...@perches.com>
Signed-off-by: Michael J Dilmore <michael.j.dilm...@gmail.com>
---
 drivers/net/bonding/bond_options.c | 79 +++---
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c 
b/drivers/net/bonding/bond_options.c
index 1bcbb89..e3a9af6 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -721,13 +721,13 @@ static int bond_option_mode_set(struct bonding *bond,
const struct bond_opt_value *newval)
 {
if (!bond_mode_uses_arp(newval->value) && bond->params.arp_interval) {
-   netdev_info(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
+   netdev_dbg(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
newval->string);
/* disable arp monitoring */
bond->params.arp_interval = 0;
/* set miimon to default value */
bond->params.miimon = BOND_DEFAULT_MIIMON;
-   netdev_info(bond->dev, "Setting MII monitoring interval to 
%d\n",
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %d\n",
bond->params.miimon);
}
 
@@ -771,7 +771,7 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
block_netpoll_tx();
/* check to see if we are clearing active */
if (!slave_dev) {
-   netdev_info(bond->dev, "Clearing current active slave\n");
+   netdev_dbg(bond->dev, "Clearing current active slave\n");
RCU_INIT_POINTER(bond->curr_active_slave, NULL);
bond_select_active_slave(bond);
} else {
@@ -782,12 +782,12 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 
if (new_active == old_active) {
/* do nothing */
-   netdev_info(bond->dev, "%s is already the current 
active slave\n",
+   netdev_dbg(bond->dev, "%s is already the current active 
slave\n",
new_active->dev->name);
} else {
if (old_active && (new_active->link == BOND_LINK_UP) &&
bond_slave_is_up(new_active)) {
-   netdev_info(bond->dev, "Setting %s as active 
slave\n",
+   netdev_dbg(bond->dev, "Setting %s as active 
slave\n",
new_active->dev->name);
bond_change_active_slave(bond, new_active);
} else {
@@ -810,17 +810,17 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 static int bond_option_miimon_set(struct bonding *bond,
  const struct bond_opt_value *newval)
 {
-   netdev_info(bond->dev, "Setting MII monitoring interval to %llu\n",
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %llu\n",
newval->value);
bond->params.miimon = newval->value;
if (bond->params.updelay)
-   netdev_info(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
+   netdev_dbg(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
bond->params.updelay * bond->params.miimon);
if (bond->params.downdelay)
-   netdev_info(bond->dev, "Note: Updating downdelay (to %d) since 
it is a multiple of the miimon value\n",
+   netdev_dbg(bond->dev, "Note: Updating downdelay (to %d) since 
it is a multiple of the miimon value\n",
bond->params.downdelay * bond->params.miimon);
if (newval->value && bond->params.arp_interval) {
-   netdev_info(bond->dev, "MII monitoring cannot be used with ARP 
monitoring - disabling ARP monitoring...\n");
+   netdev_dbg(bond->dev, "MII monitoring cannot be used with ARP 
monitoring - disabling ARP monitoring...\n");
bond->params.arp_interval = 0;
if (bond->params.arp_validate)
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
@@ -862,7 +862,7 @@ static int bond_optio

[PATCH] Convert multiple netdev_info messages to netdev_dbg

[Michael J Dilmore]

Multiple netdev_info messages clutter kernel output. Also add netdev_dbg for 
packets per slave.

Suggested-by: Joe Perches <j...@perches.com>
Signed-off-by: Michael J Dilmore <michael.j.dilm...@gmail.com>

cc: Veaceslav Falico <vfal...@gmail.com>, Andy Gospodarek <a...@greyhouse.net>, 
netdev@vger.kernel.org, linux-ker...@vger.kernel.org

---
 drivers/net/bonding/bond_options.c | 82 --
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c 
b/drivers/net/bonding/bond_options.c
index 577e57c..ee31ec1 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -9,6 +9,8 @@
  * (at your option) any later version.
  */
 
+#define DEBUG 1
+
 #include 
 #include 
 #include 
@@ -719,13 +721,13 @@ static int bond_option_mode_set(struct bonding *bond,
const struct bond_opt_value *newval)
 {
if (!bond_mode_uses_arp(newval->value) && bond->params.arp_interval) {
-   netdev_info(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
+   netdev_dbg(bond->dev, "%s mode is incompatible with arp 
monitoring, start mii monitoring\n",
newval->string);
/* disable arp monitoring */
bond->params.arp_interval = 0;
/* set miimon to default value */
bond->params.miimon = BOND_DEFAULT_MIIMON;
-   netdev_info(bond->dev, "Setting MII monitoring interval to 
%d\n",
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %d\n",
bond->params.miimon);
}
 
@@ -769,7 +771,7 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
block_netpoll_tx();
/* check to see if we are clearing active */
if (!slave_dev) {
-   netdev_info(bond->dev, "Clearing current active slave\n");
+   netdev_dbg(bond->dev, "Clearing current active slave\n");
RCU_INIT_POINTER(bond->curr_active_slave, NULL);
bond_select_active_slave(bond);
} else {
@@ -780,12 +782,12 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 
if (new_active == old_active) {
/* do nothing */
-   netdev_info(bond->dev, "%s is already the current 
active slave\n",
+   netdev_dbg(bond->dev, "%s is already the current active 
slave\n",
new_active->dev->name);
} else {
if (old_active && (new_active->link == BOND_LINK_UP) &&
bond_slave_is_up(new_active)) {
-   netdev_info(bond->dev, "Setting %s as active 
slave\n",
+   netdev_dbg(bond->dev, "Setting %s as active 
slave\n",
new_active->dev->name);
bond_change_active_slave(bond, new_active);
} else {
@@ -808,17 +810,17 @@ static int bond_option_active_slave_set(struct bonding 
*bond,
 static int bond_option_miimon_set(struct bonding *bond,
  const struct bond_opt_value *newval)
 {
-   netdev_info(bond->dev, "Setting MII monitoring interval to %llu\n",
+   netdev_dbg(bond->dev, "Setting MII monitoring interval to %llu\n",
newval->value);
bond->params.miimon = newval->value;
if (bond->params.updelay)
-   netdev_info(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
+   netdev_dbg(bond->dev, "Note: Updating updelay (to %d) since it 
is a multiple of the miimon value\n",
bond->params.updelay * bond->params.miimon);
if (bond->params.downdelay)
-   netdev_info(bond->dev, "Note: Updating downdelay (to %d) since 
it is a multiple of the miimon value\n",
+   netdev_dbg(bond->dev, "Note: Updating downdelay (to %d) since 
it is a multiple of the miimon value\n",
bond->params.downdelay * bond->params.miimon);
if (newval->value && bond->params.arp_interval) {
-   netdev_info(bond->dev, "MII monitoring cannot be used with ARP 
monitoring - disabling ARP monitoring...\n");
+   netdev_dbg(bond->dev, "MII monitoring cannot be used with ARP 
monitoring - disabling ARP monitoring...\n");
bond->params.arp_interval = 0;