[PATCH v2 net-next 0/12] bonding: add an option to rely on unvalidated arp packets

[PATCH v2 net-next 0/12] bonding: add an option to rely on unvalidated arp packets

[Veaceslav Falico]

Hi,

v1 -> v2:
Don't remove the 'all traffic' functionality - rather, add new arp_validate
options to specify that we want *only* unvalidated arps.

Currently, if arp_validate is off (0), slave_last_rx() returns the
slave->dev->last_rx, which is always updated on *any* packet received by
slave, and not only arps. This means that, if the validation of arps is
off, we're treating *any* incoming packet as a proof of slave being up, and
not only arps.

This might seem logical at the first glance, however it can cause a lot of
troubles and false-positives, one example would be:

The arp_ip_target is NOT accessible, however someone in the broadcast domain
spams with any broadcast traffic. This way bonding will be tricked that the
slave is still up (as in - can access arp_ip_target), while it's not.

The net_device->last_rx is already used in a lot of drivers (even though the
comment states to NOT do it :)), and it's also ugly to modify it from bonding.

However, some loadbalance setups might rely on the fact that even non-arp
traffic is a sign of slave being up - and we definitely can't break anyones
config - so an extension to arp_validate is needed.

So, to fix this, add an option for the user to specify if he wants to
filter out non-arp traffic on unvalidated slaves, remove the last_rx from
bonding, *always* call bond_arp_rcv() in slave's rx_handler (which is
bond_handle_frame), and if we spot an arp there with this option on - update
the slave->last_arp_rx - and use it instead of net_device->last_rx. Finally,
rename last_arp_rx to last_rx to reflect the changes.

Also rename slave->jiffies to ->last_link_up, to reflect better its
meaning, add the new option's documentation and update the arp_validate one
to be a bit more descriptive.

CC: Rob Landley <rob@landley.net>
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
CC: Ding Tianhong <dingtianhong@huawei.com>
CC: Neil Horman <nhorman@tuxdriver.com>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

---
 Documentation/networking/bonding.txt | 40 ++++++++++++++++++++++-----
 drivers/net/bonding/bond_main.c      | 53 ++++++++++++++++--------------------
 drivers/net/bonding/bond_options.c   | 18 ++----------
 drivers/net/bonding/bonding.h        | 26 ++++++++++++------
 include/linux/netdevice.h            |  8 +-----
 5 files changed, 77 insertions(+), 68 deletions(-)

[PATCH v2 net-next 01/12] bonding: remove bond->lock from bond_arp_rcv

[Veaceslav Falico]

We're always called with rcu_read_lock() held (bond_arp_rcv() is only
called from bond_handle_frame(), which is rx_handler and always called
under rcu from __netif_receive_skb_core() ).

The slave active/passive and/or bonding params can change in-flight, however
we don't really care about that - we only modify the last time packet was
received, which is harmless.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f00dd45..479ca56 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2292,8 +2292,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
 		return RX_HANDLER_ANOTHER;
 
-	read_lock(&bond->lock);
-
 	if (!slave_do_arp_validate(bond, slave))
 		goto out_unlock;
 
@@ -2350,7 +2348,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 		bond_validate_arp(bond, slave, tip, sip);
 
 out_unlock:
-	read_unlock(&bond->lock);
 	if (arp != (struct arphdr *)skb->data)
 		kfree(arp);
 	return RX_HANDLER_ANOTHER;
-- 
1.8.4

[PATCH v2 net-next 02/12] bonding: permit using arp_validate with non-ab modes

[Veaceslav Falico]

Currently it's diabled because it's sometimes hard, in typical configs, to
make it work - because of the nature how the loadbalance modes work - as
it's hard to deliver valid arp replies to correct slaves by the switch.

However we still can use arp_validation in loadbalance in several other
configs, per example with arp_validate == 2 for backup with one broadcast
domain, without the switch(es) doing any balancing - this way we'd be (a
bit more) sure that the slave is up.

So, enable it to let users decide which one works/suits them best.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 Documentation/networking/bonding.txt | 6 +++---
 drivers/net/bonding/bond_main.c      | 4 ----
 drivers/net/bonding/bond_options.c   | 6 ------
 3 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index a4d925e..3620690 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -270,9 +270,9 @@ arp_ip_target
 arp_validate
 
 	Specifies whether or not ARP probes and replies should be
-	validated in the active-backup mode.  This causes the ARP
-	monitor to examine the incoming ARP requests and replies, and
-	only consider a slave to be up if it is receiving the
+	validated in any mode that supports arp monitoring.  This causes
+	the ARP monitor to examine the incoming ARP requests and replies,
+	and only consider a slave to be up if it is receiving the
 	appropriate ARP traffic.
 
 	Possible values are:
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 479ca56..cc30618 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4198,10 +4198,6 @@ static int bond_check_params(struct bond_params *params)
 	}
 
 	if (arp_validate) {
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
-			pr_err("arp_validate only supported in active-backup mode\n");
-			return -EINVAL;
-		}
 		if (!arp_interval) {
 			pr_err("arp_validate requires arp_interval\n");
 			return -EINVAL;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 945a666..f1c4fba 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -436,12 +436,6 @@ int bond_option_arp_validate_set(struct bonding *bond, int arp_validate)
 		return -EINVAL;
 	}
 
-	if (bond->params.mode != BOND_MODE_ACTIVEBACKUP) {
-		pr_err("%s: arp_validate only supported in active-backup mode.\n",
-		       bond->dev->name);
-		return -EINVAL;
-	}
-
 	pr_info("%s: setting arp_validate to %s (%d).\n",
 		bond->dev->name, arp_validate_tbl[arp_validate].modename,
 		arp_validate);
-- 
1.8.4

[PATCH v2 net-next 03/12] bonding: always update last_arp_rx on packet recieve

[Veaceslav Falico]

Currently we're updating the last_arp_rx only when we've validate the
packet, however afterwards we use it as 'ANY last packet received', but not
only validated ARPs.

Fix this by updating it in case of any packet received. It won't break the
arp_validation=0 because we, anyway, return the correct slave->dev->last_rx in
slave_last_rx().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cc30618..909f164 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2289,6 +2289,8 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 	__be32 sip, tip;
 	int alen;
 
+	slave->last_arp_rx = jiffies;
+
 	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
 		return RX_HANDLER_ANOTHER;
 
-- 
1.8.4

[PATCH v2 net-next 04/12] bonding: always set recv_probe to bond_arp_rcv in arp monitor

[Veaceslav Falico]

Currently we only set bond_arp_rcv() if we're using arp_validate, however
this makes us skip updating last_arp_rx if we're not validating incoming
ARPs - thus, if arp_validate is off, last_arp_rx will never be updated.

Fix this by always setting up recv_probe = bond_arp_rcv, even if we're not
using arp_validate.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c    |  3 +--
 drivers/net/bonding/bond_options.c | 12 ++----------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 909f164..07ae82d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3077,8 +3077,7 @@ static int bond_open(struct net_device *bond_dev)
 
 	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
 		queue_delayed_work(bond->wq, &bond->arp_work, 0);
-		if (bond->params.arp_validate)
-			bond->recv_probe = bond_arp_rcv;
+		bond->recv_probe = bond_arp_rcv;
 	}
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index f1c4fba..9d6d231 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -283,13 +283,11 @@ int bond_option_arp_interval_set(struct bonding *bond, int arp_interval)
 		 * is called.
 		 */
 		if (!arp_interval) {
-			if (bond->params.arp_validate)
-				bond->recv_probe = NULL;
+			bond->recv_probe = NULL;
 			cancel_delayed_work_sync(&bond->arp_work);
 		} else {
 			/* arp_validate can be set only in active-backup mode */
-			if (bond->params.arp_validate)
-				bond->recv_probe = bond_arp_rcv;
+			bond->recv_probe = bond_arp_rcv;
 			cancel_delayed_work_sync(&bond->mii_work);
 			queue_delayed_work(bond->wq, &bond->arp_work, 0);
 		}
@@ -440,12 +438,6 @@ int bond_option_arp_validate_set(struct bonding *bond, int arp_validate)
 		bond->dev->name, arp_validate_tbl[arp_validate].modename,
 		arp_validate);
 
-	if (bond->dev->flags & IFF_UP) {
-		if (!arp_validate)
-			bond->recv_probe = NULL;
-		else if (bond->params.arp_interval)
-			bond->recv_probe = bond_arp_rcv;
-	}
 	bond->params.arp_validate = arp_validate;
 
 	return 0;
-- 
1.8.4

[PATCH v2 net-next 05/12] bonding: extend arp_validate to be able to receive unvalidated arp-only traffic

[Veaceslav Falico]

Currently we can either receive any traffic as a proff of slave being up,
or only *validated* arp traffic (i.e. with src/dst ip checked).

Add an option to be able to specify if we want to receive non-validated arp
traffic only.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |  3 +++
 drivers/net/bonding/bonding.h   | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 07ae82d..532a452 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -246,6 +246,9 @@ const struct bond_parm_tbl arp_validate_tbl[] = {
 {	"active",		BOND_ARP_VALIDATE_ACTIVE},
 {	"backup",		BOND_ARP_VALIDATE_BACKUP},
 {	"all",			BOND_ARP_VALIDATE_ALL},
+{	"arp",			BOND_ARP_VALIDATE_ARP},
+{	"active_arp",		BOND_ARP_VALIDATE_ACTIVE_ARP},
+{	"backup_arp",		BOND_ARP_VALIDATE_BACKUP_ARP},
 {	NULL,			-1},
 };
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 955dc48..1fbbf04 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -318,6 +318,11 @@ static inline bool bond_is_active_slave(struct slave *slave)
 #define BOND_ARP_VALIDATE_BACKUP	(1 << BOND_STATE_BACKUP)
 #define BOND_ARP_VALIDATE_ALL		(BOND_ARP_VALIDATE_ACTIVE | \
 					 BOND_ARP_VALIDATE_BACKUP)
+#define BOND_ARP_VALIDATE_ARP		(BOND_ARP_VALIDATE_ALL + 1)
+#define BOND_ARP_VALIDATE_ACTIVE_ARP	(BOND_ARP_VALIDATE_ACTIVE | \
+					 BOND_ARP_VALIDATE_ARP)
+#define BOND_ARP_VALIDATE_BACKUP_ARP	(BOND_ARP_VALIDATE_BACKUP | \
+					 BOND_ARP_VALIDATE_ARP)
 
 static inline int slave_do_arp_validate(struct bonding *bond,
 					struct slave *slave)
@@ -325,6 +330,12 @@ static inline int slave_do_arp_validate(struct bonding *bond,
 	return bond->params.arp_validate & (1 << bond_slave_state(slave));
 }
 
+static inline int slave_do_arp_validate_only(struct bonding *bond,
+					     struct slave *slave)
+{
+	return bond->params.arp_validate & BOND_ARP_VALIDATE_ARP;
+}
+
 /* Get the oldest arp which we've received on this slave for bond's
  * arp_targets.
  */
-- 
1.8.4

Re: [PATCH v2 net-next 05/12] bonding: extend arp_validate to be able to receive unvalidated arp-only traffic

[Jay Vosburgh]

Veaceslav Falico <vfalico@redhat.com> wrote:

>Currently we can either receive any traffic as a proff of slave being up,
>or only *validated* arp traffic (i.e. with src/dst ip checked).
>
>Add an option to be able to specify if we want to receive non-validated arp
>traffic only.
>
>CC: Jay Vosburgh <fubar@us.ibm.com>
>CC: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>---
> drivers/net/bonding/bond_main.c |  3 +++
> drivers/net/bonding/bonding.h   | 11 +++++++++++
> 2 files changed, 14 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 07ae82d..532a452 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -246,6 +246,9 @@ const struct bond_parm_tbl arp_validate_tbl[] = {
> {	"active",		BOND_ARP_VALIDATE_ACTIVE},
> {	"backup",		BOND_ARP_VALIDATE_BACKUP},
> {	"all",			BOND_ARP_VALIDATE_ALL},
>+{	"arp",			BOND_ARP_VALIDATE_ARP},
>+{	"active_arp",		BOND_ARP_VALIDATE_ACTIVE_ARP},
>+{	"backup_arp",		BOND_ARP_VALIDATE_BACKUP_ARP},
> {	NULL,			-1},
> };
>
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 955dc48..1fbbf04 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -318,6 +318,11 @@ static inline bool bond_is_active_slave(struct slave *slave)
> #define BOND_ARP_VALIDATE_BACKUP	(1 << BOND_STATE_BACKUP)
> #define BOND_ARP_VALIDATE_ALL		(BOND_ARP_VALIDATE_ACTIVE | \
> 					 BOND_ARP_VALIDATE_BACKUP)
>+#define BOND_ARP_VALIDATE_ARP		(BOND_ARP_VALIDATE_ALL + 1)
>+#define BOND_ARP_VALIDATE_ACTIVE_ARP	(BOND_ARP_VALIDATE_ACTIVE | \
>+					 BOND_ARP_VALIDATE_ARP)
>+#define BOND_ARP_VALIDATE_BACKUP_ARP	(BOND_ARP_VALIDATE_BACKUP | \
>+					 BOND_ARP_VALIDATE_ARP)

	If you go with my suggestion to call the new thing "filtering,"
I'd change these option names, labels, and the function
"slave_do_arp_validate_only" names.  The function name seems kind of
confusing in particular. I think it'd be clearer to replace the
"validate" stuff with "filter."

	-J

> static inline int slave_do_arp_validate(struct bonding *bond,
> 					struct slave *slave)
>@@ -325,6 +330,12 @@ static inline int slave_do_arp_validate(struct bonding *bond,
> 	return bond->params.arp_validate & (1 << bond_slave_state(slave));
> }
>
>+static inline int slave_do_arp_validate_only(struct bonding *bond,
>+					     struct slave *slave)
>+{
>+	return bond->params.arp_validate & BOND_ARP_VALIDATE_ARP;
>+}
>+
> /* Get the oldest arp which we've received on this slave for bond's
>  * arp_targets.
>  */
>-- 
>1.8.4
>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH v2 net-next 05/12] bonding: extend arp_validate to be able to receive unvalidated arp-only traffic

[Veaceslav Falico]

On Fri, Jan 17, 2014 at 12:41:29PM -0800, Jay Vosburgh wrote:
>Veaceslav Falico <vfalico@redhat.com> wrote:
...snip...
>>--- a/drivers/net/bonding/bonding.h
>>+++ b/drivers/net/bonding/bonding.h
>>@@ -318,6 +318,11 @@ static inline bool bond_is_active_slave(struct slave *slave)
>> #define BOND_ARP_VALIDATE_BACKUP	(1 << BOND_STATE_BACKUP)
>> #define BOND_ARP_VALIDATE_ALL		(BOND_ARP_VALIDATE_ACTIVE | \
>> 					 BOND_ARP_VALIDATE_BACKUP)
>>+#define BOND_ARP_VALIDATE_ARP		(BOND_ARP_VALIDATE_ALL + 1)
>>+#define BOND_ARP_VALIDATE_ACTIVE_ARP	(BOND_ARP_VALIDATE_ACTIVE | \
>>+					 BOND_ARP_VALIDATE_ARP)
>>+#define BOND_ARP_VALIDATE_BACKUP_ARP	(BOND_ARP_VALIDATE_BACKUP | \
>>+					 BOND_ARP_VALIDATE_ARP)
>
>	If you go with my suggestion to call the new thing "filtering,"
>I'd change these option names, labels, and the function
>"slave_do_arp_validate_only" names.  The function name seems kind of
>confusing in particular. I think it'd be clearer to replace the
>"validate" stuff with "filter."

Hi Jay,

Sorry for the delay. Yep, completely agree, I'll change the doc/rename the
functions/defines to use "filtering" instead of double-arp.

I'll re-send it once I'll fix the current bonding state...

Thank you!

>
>	-J
>
>> static inline int slave_do_arp_validate(struct bonding *bond,
>> 					struct slave *slave)
>>@@ -325,6 +330,12 @@ static inline int slave_do_arp_validate(struct bonding *bond,
>> 	return bond->params.arp_validate & (1 << bond_slave_state(slave));
>> }
>>
>>+static inline int slave_do_arp_validate_only(struct bonding *bond,
>>+					     struct slave *slave)
>>+{
>>+	return bond->params.arp_validate & BOND_ARP_VALIDATE_ARP;
>>+}
>>+
>> /* Get the oldest arp which we've received on this slave for bond's
>>  * arp_targets.
>>  */
>>--
>>1.8.4
>>
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

[PATCH v2 net-next 06/12] bonding: document the new _arp options for arp_validate

[Veaceslav Falico]

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 Documentation/networking/bonding.txt | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 3620690..a0c1bca2 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -279,19 +279,45 @@ arp_validate
 
 	none or 0
 
-		No validation is performed.  This is the default.
+		No validation is performed.  This is the default. Any arriving
+		traffic (arp or non-arp) is considered a proof that the slave
+		is up.
 
 	active or 1
 
-		Validation is performed only for the active slave.
+		Validation is performed only for the active slave. Only ARPs
+		that arrive from any arp_ip_target are considered legit. The
+		backup slave still does no validation (as if arp_validate=0).
 
 	backup or 2
 
-		Validation is performed only for backup slaves.
+		Validation is performed only for backup slaves. Only ARPs
+		that arrive from any arp_ip_target are considered legit. The
+		active slave still has no validation (as if arp_validate=0).
 
 	all or 3
 
-		Validation is performed for all slaves.
+		Validation is performed for all slaves. Only ARPs
+		that arrive from any arp_ip_target are considered legit.
+
+	arp or 4
+
+		Any arp packet is accepted as a proof that any slave is up,
+		but no IP-based validation is made.
+
+	active_arp or 5
+
+		Validation is performed only for the active slave. Only ARPs
+		that arrive from any arp_ip_target are considered legit. The
+		backup slave validates only arp packets, but doesn't check the
+		source (as if arp_validate=4).
+
+	backup_any or 6
+
+		Validation is performed only for backup slaves. Only ARPs
+		that arrive from any arp_ip_target are considered legit. The
+		active slave validates only arp packets, but doesn't check the
+		source (as if arp_validate=4).
 
 	For the active slave, the validation checks ARP replies to
 	confirm that they were generated by an arp_ip_target.  Since
-- 
1.8.4

Re: [PATCH v2 net-next 06/12] bonding: document the new _arp options for arp_validate

[Jay Vosburgh]

Veaceslav Falico <vfalico@redhat.com> wrote:

>CC: Jay Vosburgh <fubar@us.ibm.com>
>CC: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>---
> Documentation/networking/bonding.txt | 34 ++++++++++++++++++++++++++++++----
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>index 3620690..a0c1bca2 100644
>--- a/Documentation/networking/bonding.txt
>+++ b/Documentation/networking/bonding.txt
>@@ -279,19 +279,45 @@ arp_validate
>
> 	none or 0
>
>-		No validation is performed.  This is the default.
>+		No validation is performed.  This is the default. Any arriving
>+		traffic (arp or non-arp) is considered a proof that the slave
>+		is up.
>
> 	active or 1
>
>-		Validation is performed only for the active slave.
>+		Validation is performed only for the active slave. Only ARPs
>+		that arrive from any arp_ip_target are considered legit. The
>+		backup slave still does no validation (as if arp_validate=0).
>
> 	backup or 2
>
>-		Validation is performed only for backup slaves.
>+		Validation is performed only for backup slaves. Only ARPs
>+		that arrive from any arp_ip_target are considered legit. The
>+		active slave still has no validation (as if arp_validate=0).
>
> 	all or 3
>
>-		Validation is performed for all slaves.
>+		Validation is performed for all slaves. Only ARPs
>+		that arrive from any arp_ip_target are considered legit.
>+
>+	arp or 4
>+
>+		Any arp packet is accepted as a proof that any slave is up,
>+		but no IP-based validation is made.
>+
>+	active_arp or 5
>+
>+		Validation is performed only for the active slave. Only ARPs
>+		that arrive from any arp_ip_target are considered legit. The
>+		backup slave validates only arp packets, but doesn't check the
>+		source (as if arp_validate=4).
>+
>+	backup_any or 6
>+
>+		Validation is performed only for backup slaves. Only ARPs
>+		that arrive from any arp_ip_target are considered legit. The
>+		active slave validates only arp packets, but doesn't check the
>+		source (as if arp_validate=4).

	I think that, for the last three options, saying that
"validation is performed" is not quite right, since the next paragraph
goes on to explain what "validation" is (that the incoming ARP came from
us or was a response to ours), and these options don't really validate
in that sense, but merely filter anything that's not an ARP.

	There'a a sentence with a similar problem further down: "Use of
the arp_validate option can resolve this, as the ARP monitor will only
consider ARP requests and replies associated with its own instance of
bonding."  For the three new options, this sentence is not accurate.

	I think I'd rework this whole block something like the following
(this is a diff against your patched version).  I'm calling the two
separate things "validation" and "filtering," since the wording you used
kind of combined things into two styles of validation; I think it's
clearer to make them discrete things.

	This would also necessitate change the option tag names; I also
put the "filter" ones into the same order as the "validate" ones
(active, backup, then all).

	Comments?

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index a0c1bca2..5fd6a6a 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -270,80 +270,87 @@ arp_ip_target
 arp_validate
 
 	Specifies whether or not ARP probes and replies should be
-	validated in any mode that supports arp monitoring.  This causes
-	the ARP monitor to examine the incoming ARP requests and replies,
-	and only consider a slave to be up if it is receiving the
-	appropriate ARP traffic.
-
+	validated in any mode that supports arp monitoring, or whether
+	non-ARP traffic should be filtered (disregarded) for link
+	monitoring purposes.
+	
 	Possible values are:
 
 	none or 0
 
-		No validation is performed.  This is the default. Any arriving
-		traffic (arp or non-arp) is considered a proof that the slave
-		is up.
+		No validation or filtering is performed.
 
 	active or 1
 
-		Validation is performed only for the active slave. Only ARPs
-		that arrive from any arp_ip_target are considered legit. The
-		backup slave still does no validation (as if arp_validate=0).
+		Validation is performed only for the active slave.
 
 	backup or 2
 
-		Validation is performed only for backup slaves. Only ARPs
-		that arrive from any arp_ip_target are considered legit. The
-		active slave still has no validation (as if arp_validate=0).
+		Validation is performed only for backup slaves.
 
 	all or 3
 
-		Validation is performed for all slaves. Only ARPs
-		that arrive from any arp_ip_target are considered legit.
-
-	arp or 4
-
-		Any arp packet is accepted as a proof that any slave is up,
-		but no IP-based validation is made.
-
-	active_arp or 5
-
-		Validation is performed only for the active slave. Only ARPs
-		that arrive from any arp_ip_target are considered legit. The
-		backup slave validates only arp packets, but doesn't check the
-		source (as if arp_validate=4).
-
-	backup_any or 6
-
-		Validation is performed only for backup slaves. Only ARPs
-		that arrive from any arp_ip_target are considered legit. The
-		active slave validates only arp packets, but doesn't check the
-		source (as if arp_validate=4).
-
-	For the active slave, the validation checks ARP replies to
-	confirm that they were generated by an arp_ip_target.  Since
-	backup slaves do not typically receive these replies, the
-	validation performed for backup slaves is on the ARP request
-	sent out via the active slave.  It is possible that some
-	switch or network configurations may result in situations
-	wherein the backup slaves do not receive the ARP requests; in
-	such a situation, validation of backup slaves must be
-	disabled.
-
-	The validation of ARP requests on backup slaves is mainly
-	helping bonding to decide which slaves are more likely to
-	work in case of the active slave failure, it doesn't really
-	guarantee that the backup slave will work if it's selected
-	as the next active slave.
-
-	This option is useful in network configurations in which
-	multiple bonding hosts are concurrently issuing ARPs to one or
-	more targets beyond a common switch.  Should the link between
-	the switch and target fail (but not the switch itself), the
-	probe traffic generated by the multiple bonding instances will
-	fool the standard ARP monitor into considering the links as
-	still up.  Use of the arp_validate option can resolve this, as
-	the ARP monitor will only consider ARP requests and replies
-	associated with its own instance of bonding.
+		Validation is performed for all slaves.
+
+	filter_active or 4
+
+		Filtering is applied to the active slave only.
+
+	filter_backup or 5
+
+		Filtering is applied to the backup slave(s) only.
+
+	filter_all or 6
+
+		Filtering is applied to all slaves.
+
+	Validation:
+
+	Enabling validation causes the ARP monitor to examine the incoming
+	ARP requests and replies, and only consider a slave to be up if it
+	is receiving the appropriate ARP traffic.
+
+	For an active slave, the validation checks ARP replies to confirm
+	that they were generated by an arp_ip_target.  Since backup slaves
+	do not typically receive these replies, the validation performed
+	for backup slaves is on the broadcast ARP request sent out via the
+	active slave.  It is possible that some switch or network
+	configurations may result in situations wherein the backup slaves
+	do not receive the ARP requests; in such a situation, validation
+	of backup slaves must be disabled.
+
+	The validation of ARP requests on backup slaves is mainly helping
+	bonding to decide which slaves are more likely to work in case of
+	the active slave failure, it doesn't really guarantee that the
+	backup slave will work if it's selected as the next active slave.
+
+	Validation is useful in network configurations in which multiple
+	bonding hosts are concurrently issuing ARPs to one or more targets
+	beyond a common switch.  Should the link between the switch and
+	target fail (but not the switch itself), the probe traffic
+	generated by the multiple bonding instances will fool the standard
+	ARP monitor into considering the links as still up.  Use of
+	validation can resolve this, as the ARP monitor will only consider
+	ARP requests and replies associated with its own instance of
+	bonding.
+
+	Filtering:
+
+	Enabling filtering causes the ARP monitor to only use incoming ARP
+	packets for link availability purposes.  Arriving packets that are
+	not ARPs are delivered normally, but do not count when determining
+	if a slave is available.
+
+	Filtering operates by only considering the reception of ARP
+	packets (any ARP packet, regardless of source or destination) when
+	determining if a slave has received traffic for link availability
+	purposes.
+
+	Filtering is useful in network configurations in which significant
+	levels of third party broadcast traffic would fool the standard
+	ARP monitor into considering the links as still up.  Use of
+	filtering can resolve this, as only ARP traffic is considered for
+	link availability purposes.
 
 	This option was added in bonding version 3.1.0.
 


---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH v2 net-next 06/12] bonding: document the new _arp options for arp_validate

[Veaceslav Falico]

On Fri, Jan 17, 2014 at 12:38:28PM -0800, Jay Vosburgh wrote:
>Veaceslav Falico <vfalico@redhat.com> wrote:
>
>>CC: Jay Vosburgh <fubar@us.ibm.com>
>>CC: Andy Gospodarek <andy@greyhouse.net>
>>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>>---
>> Documentation/networking/bonding.txt | 34 ++++++++++++++++++++++++++++++----
>> 1 file changed, 30 insertions(+), 4 deletions(-)
>>
>>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>>index 3620690..a0c1bca2 100644
>>--- a/Documentation/networking/bonding.txt
>>+++ b/Documentation/networking/bonding.txt
>>@@ -279,19 +279,45 @@ arp_validate
>>
>> 	none or 0
>>
>>-		No validation is performed.  This is the default.
>>+		No validation is performed.  This is the default. Any arriving
>>+		traffic (arp or non-arp) is considered a proof that the slave
>>+		is up.
>>
>> 	active or 1
>>
>>-		Validation is performed only for the active slave.
>>+		Validation is performed only for the active slave. Only ARPs
>>+		that arrive from any arp_ip_target are considered legit. The
>>+		backup slave still does no validation (as if arp_validate=0).
>>
>> 	backup or 2
>>
>>-		Validation is performed only for backup slaves.
>>+		Validation is performed only for backup slaves. Only ARPs
>>+		that arrive from any arp_ip_target are considered legit. The
>>+		active slave still has no validation (as if arp_validate=0).
>>
>> 	all or 3
>>
>>-		Validation is performed for all slaves.
>>+		Validation is performed for all slaves. Only ARPs
>>+		that arrive from any arp_ip_target are considered legit.
>>+
>>+	arp or 4
>>+
>>+		Any arp packet is accepted as a proof that any slave is up,
>>+		but no IP-based validation is made.
>>+
>>+	active_arp or 5
>>+
>>+		Validation is performed only for the active slave. Only ARPs
>>+		that arrive from any arp_ip_target are considered legit. The
>>+		backup slave validates only arp packets, but doesn't check the
>>+		source (as if arp_validate=4).
>>+
>>+	backup_any or 6
>>+
>>+		Validation is performed only for backup slaves. Only ARPs
>>+		that arrive from any arp_ip_target are considered legit. The
>>+		active slave validates only arp packets, but doesn't check the
>>+		source (as if arp_validate=4).

Hi Jay,

Sorry for taking it so long - however I didn't manage to update/re-send the
patchset before the net-next closure, so sending it now. Few comments
below, though.

>
>	I think that, for the last three options, saying that
>"validation is performed" is not quite right, since the next paragraph
>goes on to explain what "validation" is (that the incoming ARP came from
>us or was a response to ours), and these options don't really validate
>in that sense, but merely filter anything that's not an ARP.

Yes and no. Indeed, validation is not performed if we use the "filter_all"
option (filter both slaves, don't do any validation). However it's
performed for both filter_active/backup - as in "Validate arp packets on
active/backup, and filter only on the other one".

However your text is really awesome in explaining what is
validating/filtering, so I've included it in the patch with minor
corrections, see below.

>
>	There'a a sentence with a similar problem further down: "Use of
>the arp_validate option can resolve this, as the ARP monitor will only
>consider ARP requests and replies associated with its own instance of
>bonding."  For the three new options, this sentence is not accurate.
>
>	I think I'd rework this whole block something like the following
>(this is a diff against your patched version).  I'm calling the two
>separate things "validation" and "filtering," since the wording you used
>kind of combined things into two styles of validation; I think it's
>clearer to make them discrete things.
>
>	This would also necessitate change the option tag names; I also
>put the "filter" ones into the same order as the "validate" ones
>(active, backup, then all).

I've left the original order, because the "filter" is, actually, bit 4, so
if we want to filter AND validate, we set FITLER | VALIDATE_{ACTIVE,BACKUP},
and this way we get 5 or 6. I've also changed the filter_all to just
"filter", to make it more clear what's happening. Here are the only lines
changed on top of your patch:

         filter or 4

                 Filtering is applied to all slaves. No validation is
                 performed.

         filter_active or 5

                 Filtering is applied to all slaves, validation is performed
                 only for the active slave.

         filter_backup or 6

                 Filtering is applied to all slaves, validation is performed
                 only for backup slaves.

This way we can set any pair of filtering/validation:

No validation + no filtering? none
Validation + no filtering? acitve/backup
No validation + filtering? filter
Validation + filtering? filter_active/backup

I'll send v3 in a few minutes, so that it'll be easier to review (it's also
rebased on top of latest net-next).

Thanks a lot, and sorry for the delay.

>
>	Comments?
>
>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>index a0c1bca2..5fd6a6a 100644
>--- a/Documentation/networking/bonding.txt
>+++ b/Documentation/networking/bonding.txt
>@@ -270,80 +270,87 @@ arp_ip_target
> arp_validate
>
> 	Specifies whether or not ARP probes and replies should be
>-	validated in any mode that supports arp monitoring.  This causes
>-	the ARP monitor to examine the incoming ARP requests and replies,
>-	and only consider a slave to be up if it is receiving the
>-	appropriate ARP traffic.
>-
>+	validated in any mode that supports arp monitoring, or whether
>+	non-ARP traffic should be filtered (disregarded) for link
>+	monitoring purposes.
>+	
> 	Possible values are:
>
> 	none or 0
>
>-		No validation is performed.  This is the default. Any arriving
>-		traffic (arp or non-arp) is considered a proof that the slave
>-		is up.
>+		No validation or filtering is performed.
>
> 	active or 1
>
>-		Validation is performed only for the active slave. Only ARPs
>-		that arrive from any arp_ip_target are considered legit. The
>-		backup slave still does no validation (as if arp_validate=0).
>+		Validation is performed only for the active slave.
>
> 	backup or 2
>
>-		Validation is performed only for backup slaves. Only ARPs
>-		that arrive from any arp_ip_target are considered legit. The
>-		active slave still has no validation (as if arp_validate=0).
>+		Validation is performed only for backup slaves.
>
> 	all or 3
>
>-		Validation is performed for all slaves. Only ARPs
>-		that arrive from any arp_ip_target are considered legit.
>-
>-	arp or 4
>-
>-		Any arp packet is accepted as a proof that any slave is up,
>-		but no IP-based validation is made.
>-
>-	active_arp or 5
>-
>-		Validation is performed only for the active slave. Only ARPs
>-		that arrive from any arp_ip_target are considered legit. The
>-		backup slave validates only arp packets, but doesn't check the
>-		source (as if arp_validate=4).
>-
>-	backup_any or 6
>-
>-		Validation is performed only for backup slaves. Only ARPs
>-		that arrive from any arp_ip_target are considered legit. The
>-		active slave validates only arp packets, but doesn't check the
>-		source (as if arp_validate=4).
>-
>-	For the active slave, the validation checks ARP replies to
>-	confirm that they were generated by an arp_ip_target.  Since
>-	backup slaves do not typically receive these replies, the
>-	validation performed for backup slaves is on the ARP request
>-	sent out via the active slave.  It is possible that some
>-	switch or network configurations may result in situations
>-	wherein the backup slaves do not receive the ARP requests; in
>-	such a situation, validation of backup slaves must be
>-	disabled.
>-
>-	The validation of ARP requests on backup slaves is mainly
>-	helping bonding to decide which slaves are more likely to
>-	work in case of the active slave failure, it doesn't really
>-	guarantee that the backup slave will work if it's selected
>-	as the next active slave.
>-
>-	This option is useful in network configurations in which
>-	multiple bonding hosts are concurrently issuing ARPs to one or
>-	more targets beyond a common switch.  Should the link between
>-	the switch and target fail (but not the switch itself), the
>-	probe traffic generated by the multiple bonding instances will
>-	fool the standard ARP monitor into considering the links as
>-	still up.  Use of the arp_validate option can resolve this, as
>-	the ARP monitor will only consider ARP requests and replies
>-	associated with its own instance of bonding.
>+		Validation is performed for all slaves.
>+
>+	filter_active or 4
>+
>+		Filtering is applied to the active slave only.
>+
>+	filter_backup or 5
>+
>+		Filtering is applied to the backup slave(s) only.
>+
>+	filter_all or 6
>+
>+		Filtering is applied to all slaves.
>+
>+	Validation:
>+
>+	Enabling validation causes the ARP monitor to examine the incoming
>+	ARP requests and replies, and only consider a slave to be up if it
>+	is receiving the appropriate ARP traffic.
>+
>+	For an active slave, the validation checks ARP replies to confirm
>+	that they were generated by an arp_ip_target.  Since backup slaves
>+	do not typically receive these replies, the validation performed
>+	for backup slaves is on the broadcast ARP request sent out via the
>+	active slave.  It is possible that some switch or network
>+	configurations may result in situations wherein the backup slaves
>+	do not receive the ARP requests; in such a situation, validation
>+	of backup slaves must be disabled.
>+
>+	The validation of ARP requests on backup slaves is mainly helping
>+	bonding to decide which slaves are more likely to work in case of
>+	the active slave failure, it doesn't really guarantee that the
>+	backup slave will work if it's selected as the next active slave.
>+
>+	Validation is useful in network configurations in which multiple
>+	bonding hosts are concurrently issuing ARPs to one or more targets
>+	beyond a common switch.  Should the link between the switch and
>+	target fail (but not the switch itself), the probe traffic
>+	generated by the multiple bonding instances will fool the standard
>+	ARP monitor into considering the links as still up.  Use of
>+	validation can resolve this, as the ARP monitor will only consider
>+	ARP requests and replies associated with its own instance of
>+	bonding.
>+
>+	Filtering:
>+
>+	Enabling filtering causes the ARP monitor to only use incoming ARP
>+	packets for link availability purposes.  Arriving packets that are
>+	not ARPs are delivered normally, but do not count when determining
>+	if a slave is available.
>+
>+	Filtering operates by only considering the reception of ARP
>+	packets (any ARP packet, regardless of source or destination) when
>+	determining if a slave has received traffic for link availability
>+	purposes.
>+
>+	Filtering is useful in network configurations in which significant
>+	levels of third party broadcast traffic would fool the standard
>+	ARP monitor into considering the links as still up.  Use of
>+	filtering can resolve this, as only ARP traffic is considered for
>+	link availability purposes.
>
> 	This option was added in bonding version 3.1.0.
>
>
>
>---
>	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

[PATCH v2 net-next 07/12] bonding: use the new options to correctly set last_arp_rx

[Veaceslav Falico]

Now that the options are in place - arp_validate can be set to receive all
the traffic or only arp packets to verify if the slave is up, when the
slave isn't validated.

CC: Rob Landley <rob@landley.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
CC: Ding Tianhong <dingtianhong@huawei.com>
CC: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 532a452..48491d9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2290,15 +2290,16 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 	struct arphdr *arp = (struct arphdr *)skb->data;
 	unsigned char *arp_ptr;
 	__be32 sip, tip;
-	int alen;
+	int alen, is_arp = skb->protocol == __cpu_to_be16(ETH_P_ARP);
 
-	slave->last_arp_rx = jiffies;
-
-	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
+	if (!slave_do_arp_validate(bond, slave)) {
+		if ((slave_do_arp_validate_only(bond, slave) && is_arp) ||
+		    !slave_do_arp_validate_only(bond, slave))
+			slave->last_arp_rx = jiffies;
 		return RX_HANDLER_ANOTHER;
-
-	if (!slave_do_arp_validate(bond, slave))
-		goto out_unlock;
+	} else if (!is_arp) {
+		return RX_HANDLER_ANOTHER;
+	}
 
 	alen = arp_hdr_len(bond->dev);
 
-- 
1.8.4

Re: [PATCH v2 net-next 07/12] bonding: use the new options to correctly set last_arp_rx

[Neil Horman]

On Fri, Jan 17, 2014 at 05:58:55PM +0100, Veaceslav Falico wrote:
> Now that the options are in place - arp_validate can be set to receive all
> the traffic or only arp packets to verify if the slave is up, when the
> slave isn't validated.
> 
> CC: Rob Landley <rob@landley.net>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Nikolay Aleksandrov <nikolay@redhat.com>
> CC: Ding Tianhong <dingtianhong@huawei.com>
> CC: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 532a452..48491d9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2290,15 +2290,16 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
>  	struct arphdr *arp = (struct arphdr *)skb->data;
>  	unsigned char *arp_ptr;
>  	__be32 sip, tip;
> -	int alen;
> +	int alen, is_arp = skb->protocol == __cpu_to_be16(ETH_P_ARP);
>  
> -	slave->last_arp_rx = jiffies;
> -
> -	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
> +	if (!slave_do_arp_validate(bond, slave)) {
> +		if ((slave_do_arp_validate_only(bond, slave) && is_arp) ||
> +		    !slave_do_arp_validate_only(bond, slave))
> +			slave->last_arp_rx = jiffies;
>  		return RX_HANDLER_ANOTHER;
> -
> -	if (!slave_do_arp_validate(bond, slave))
> -		goto out_unlock;
> +	} else if (!is_arp) {
> +		return RX_HANDLER_ANOTHER;
> +	}
>  
>  	alen = arp_hdr_len(bond->dev);
>  
> -- 
> 1.8.4
> 
> 

Seems ok
Acked-by: Neil Horman <nhorman@tuxdriver.com>

[PATCH v2 net-next 08/12] bonding: use last_arp_rx in slave_last_rx()

[Veaceslav Falico]

Now that last_arp_rx really has the last time we've received any (validated or
not) ARP, we can use it in slave_last_rx() instead of slave->dev->last_rx.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bonding.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 1fbbf04..d0964c0 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -355,14 +355,10 @@ static inline unsigned long slave_oldest_target_arp_rx(struct bonding *bond,
 static inline unsigned long slave_last_rx(struct bonding *bond,
 					struct slave *slave)
 {
-	if (slave_do_arp_validate(bond, slave)) {
-		if (bond->params.arp_all_targets == BOND_ARP_TARGETS_ALL)
-			return slave_oldest_target_arp_rx(bond, slave);
-		else
-			return slave->last_arp_rx;
-	}
-
-	return slave->dev->last_rx;
+	if (bond->params.arp_all_targets == BOND_ARP_TARGETS_ALL)
+		return slave_oldest_target_arp_rx(bond, slave);
+
+	return slave->last_arp_rx;
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-- 
1.8.4

[PATCH v2 net-next 09/12] bonding: use last_arp_rx in bond_loadbalance_arp_mon()

[Veaceslav Falico]

Now that last_arp_rx correctly show the last time we've received an ARP, we
can use it safely instead of slave->dev->last_rx.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 48491d9..eccd407 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2407,7 +2407,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, trans_start, 1) &&
-			    bond_time_in_interval(bond, slave->dev->last_rx, 1)) {
+			    bond_time_in_interval(bond, slave->last_arp_rx, 1)) {
 
 				slave->link  = BOND_LINK_UP;
 				bond_set_active_slave(slave);
@@ -2436,7 +2436,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 			 * if we don't know our ip yet
 			 */
 			if (!bond_time_in_interval(bond, trans_start, 2) ||
-			    !bond_time_in_interval(bond, slave->dev->last_rx, 2)) {
+			    !bond_time_in_interval(bond, slave->last_arp_rx, 2)) {
 
 				slave->link  = BOND_LINK_DOWN;
 				bond_set_backup_slave(slave);
-- 
1.8.4

[PATCH v2 net-next 10/12] bonding: remove useless updating of slave->dev->last_rx

[Veaceslav Falico]

Now that all the logic is handled via last_arp_rx, we don't need to use
last_rx.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 3 ---
 include/linux/netdevice.h       | 8 +-------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index eccd407..3f95042 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1166,9 +1166,6 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 	slave = bond_slave_get_rcu(skb->dev);
 	bond = slave->bond;
 
-	if (bond->params.arp_interval)
-		slave->dev->last_rx = jiffies;
-
 	recv_probe = ACCESS_ONCE(bond->recv_probe);
 	if (recv_probe) {
 		ret = recv_probe(skb, bond, slave);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e985231..0fe8af0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1312,13 +1312,7 @@ struct net_device {
 /*
  * Cache lines mostly used on receive path (including eth_type_trans())
  */
-	unsigned long		last_rx;	/* Time of last Rx
-						 * This should not be set in
-						 * drivers, unless really needed,
-						 * because network stack (bonding)
-						 * use it if/when necessary, to
-						 * avoid dirtying this cache line.
-						 */
+	unsigned long		last_rx;	/* Time of last Rx */
 
 	/* Interface address info used in eth_type_trans() */
 	unsigned char		*dev_addr;	/* hw address, (before bcast
-- 
1.8.4

[PATCH v2 net-next 11/12] bonding: trivial: rename slave->jiffies to ->last_link_up

[Veaceslav Falico]

slave->jiffies is updated every time the slave becomes active, which, for
bonding, means that its link is 'up'.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 20 ++++++++++----------
 drivers/net/bonding/bonding.h   |  3 ++-
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3f95042..13a3ff8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -849,7 +849,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 		return;
 
 	if (new_active) {
-		new_active->jiffies = jiffies;
+		new_active->last_link_up = jiffies;
 
 		if (new_active->link == BOND_LINK_BACK) {
 			if (USES_PRIMARY(bond->params.mode)) {
@@ -1491,7 +1491,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	}
 
 	if (new_slave->link != BOND_LINK_DOWN)
-		new_slave->jiffies = jiffies;
+		new_slave->last_link_up = jiffies;
 	pr_debug("Initial state of slave_dev is BOND_LINK_%s\n",
 		new_slave->link == BOND_LINK_DOWN ? "DOWN" :
 			(new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));
@@ -1926,7 +1926,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 				 * recovered before downdelay expired
 				 */
 				slave->link = BOND_LINK_UP;
-				slave->jiffies = jiffies;
+				slave->last_link_up = jiffies;
 				pr_info("%s: link status up again after %d ms for interface %s.\n",
 					bond->dev->name,
 					(bond->params.downdelay - slave->delay) *
@@ -2001,7 +2001,7 @@ static void bond_miimon_commit(struct bonding *bond)
 
 		case BOND_LINK_UP:
 			slave->link = BOND_LINK_UP;
-			slave->jiffies = jiffies;
+			slave->last_link_up = jiffies;
 
 			if (bond->params.mode == BOND_MODE_8023AD) {
 				/* prevent it from being the active one */
@@ -2347,7 +2347,7 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 		bond_validate_arp(bond, slave, sip, tip);
 	else if (bond->curr_active_slave &&
 		 time_after(slave_last_rx(bond, bond->curr_active_slave),
-			    bond->curr_active_slave->jiffies))
+			    bond->curr_active_slave->last_link_up))
 		bond_validate_arp(bond, slave, tip, sip);
 
 out_unlock:
@@ -2393,9 +2393,9 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 	oldcurrent = ACCESS_ONCE(bond->curr_active_slave);
 	/* see if any of the previous devices are up now (i.e. they have
 	 * xmt and rcv traffic). the curr_active_slave does not come into
-	 * the picture unless it is null. also, slave->jiffies is not needed
-	 * here because we send an arp on each slave and give a slave as
-	 * long as it needs to get the tx/rx within the delta.
+	 * the picture unless it is null. also, slave->last_link_up is not
+	 * needed here because we send an arp on each slave and give a slave
+	 * as long as it needs to get the tx/rx within the delta.
 	 * TODO: what about up/down delay in arp mode? it wasn't here before
 	 *       so it can wait
 	 */
@@ -2517,7 +2517,7 @@ static int bond_ab_arp_inspect(struct bonding *bond)
 		 * active.  This avoids bouncing, as the last receive
 		 * times need a full ARP monitor cycle to be updated.
 		 */
-		if (bond_time_in_interval(bond, slave->jiffies, 2))
+		if (bond_time_in_interval(bond, slave->last_link_up, 2))
 			continue;
 
 		/*
@@ -2710,7 +2710,7 @@ static void bond_ab_arp_probe(struct bonding *bond)
 	new_slave->link = BOND_LINK_BACK;
 	bond_set_slave_active_flags(new_slave);
 	bond_arp_send_all(bond, new_slave);
-	new_slave->jiffies = jiffies;
+	new_slave->last_link_up = jiffies;
 	rcu_assign_pointer(bond->current_arp_slave, new_slave);
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index d0964c0..15cf6b7 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -184,7 +184,8 @@ struct slave {
 	struct net_device *dev; /* first - useful for panic debug */
 	struct bonding *bond; /* our master */
 	int    delay;
-	unsigned long jiffies;
+	/* all three in jiffies */
+	unsigned long last_link_up;
 	unsigned long last_arp_rx;
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
 	s8     link;    /* one of BOND_LINK_XXXX */
-- 
1.8.4

[PATCH v2 net-next 12/12] bonding: rename last_arp_rx to last_rx

[Veaceslav Falico]

To reflect the new meaning.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 12 ++++++------
 drivers/net/bonding/bonding.h   |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 13a3ff8..41f3109 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1444,10 +1444,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	bond_update_speed_duplex(new_slave);
 
-	new_slave->last_arp_rx = jiffies -
+	new_slave->last_rx = jiffies -
 		(msecs_to_jiffies(bond->params.arp_interval) + 1);
 	for (i = 0; i < BOND_MAX_ARP_TARGETS; i++)
-		new_slave->target_last_arp_rx[i] = new_slave->last_arp_rx;
+		new_slave->target_last_arp_rx[i] = new_slave->last_rx;
 
 	if (bond->params.miimon && !bond->params.use_carrier) {
 		link_reporting = bond_check_dev_link(bond, slave_dev, 1);
@@ -2277,7 +2277,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
 		pr_debug("bva: sip %pI4 not found in targets\n", &sip);
 		return;
 	}
-	slave->last_arp_rx = jiffies;
+	slave->last_rx = jiffies;
 	slave->target_last_arp_rx[i] = jiffies;
 }
 
@@ -2292,7 +2292,7 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond,
 	if (!slave_do_arp_validate(bond, slave)) {
 		if ((slave_do_arp_validate_only(bond, slave) && is_arp) ||
 		    !slave_do_arp_validate_only(bond, slave))
-			slave->last_arp_rx = jiffies;
+			slave->last_rx = jiffies;
 		return RX_HANDLER_ANOTHER;
 	} else if (!is_arp) {
 		return RX_HANDLER_ANOTHER;
@@ -2404,7 +2404,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 
 		if (slave->link != BOND_LINK_UP) {
 			if (bond_time_in_interval(bond, trans_start, 1) &&
-			    bond_time_in_interval(bond, slave->last_arp_rx, 1)) {
+			    bond_time_in_interval(bond, slave->last_rx, 1)) {
 
 				slave->link  = BOND_LINK_UP;
 				bond_set_active_slave(slave);
@@ -2433,7 +2433,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 			 * if we don't know our ip yet
 			 */
 			if (!bond_time_in_interval(bond, trans_start, 2) ||
-			    !bond_time_in_interval(bond, slave->last_arp_rx, 2)) {
+			    !bond_time_in_interval(bond, slave->last_rx, 2)) {
 
 				slave->link  = BOND_LINK_DOWN;
 				bond_set_backup_slave(slave);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 15cf6b7..3087f78 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -186,7 +186,7 @@ struct slave {
 	int    delay;
 	/* all three in jiffies */
 	unsigned long last_link_up;
-	unsigned long last_arp_rx;
+	unsigned long last_rx;
 	unsigned long target_last_arp_rx[BOND_MAX_ARP_TARGETS];
 	s8     link;    /* one of BOND_LINK_XXXX */
 	s8     new_link;
@@ -359,7 +359,7 @@ static inline unsigned long slave_last_rx(struct bonding *bond,
 	if (bond->params.arp_all_targets == BOND_ARP_TARGETS_ALL)
 		return slave_oldest_target_arp_rx(bond, slave);
 
-	return slave->last_arp_rx;
+	return slave->last_rx;
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-- 
1.8.4