Re: [PATCH net-next 0/6] bonding: only rely on arp packets if arp monitor is used

[Veaceslav Falico <vfalico@redhat.com>]

On Thu, Jan 16, 2014 at 02:38:59PM -0800, Jay Vosburgh wrote:
...snip...
>	I think the bottom line here is pretty simple:
>
>	Using the ARP monitor with the loadbalance modes is not a common
>configuration in my experience, and making it work is tricky.  However,
>anyone using it today will be relying on the current behavior, which we
>therefore must not change.

Yep, agreed. It might be against the documentation, these use cases might
be weird/illogical - but they (kind of) work, and we both agree that this
change might break them, so it's definitely a no go.

OTOH, I'd still like to help people who have some kind of broadcast traffic
(STP, CDP, some routing etc.) running over network and keeping their slaves
up (and those that cannot/don't want to use arp_validate=3).

What do you think about this*? It's on top of this series, extends
arp_validate to (not) filter out ARPs on not-validated slaves and permits
it to be used in non-AB mode (also, we don't need that bond->lock, we're
always under RCU).

*:

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index a4d925e..7223ef4 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
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0f613ae..2ef1d5a 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},
  };
  
@@ -2284,16 +2287,15 @@ 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;
-
-	if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
-		return RX_HANDLER_ANOTHER;
-
-	read_lock(&bond->lock);
+	int alen, is_arp = skb->protocol == __cpu_to_be16(ETH_P_ARP);
  
  	if (!slave_do_arp_validate(bond, slave)) {
-		slave->last_arp_rx = jiffies;
-		goto out_unlock;
+		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;
+	} else if (!is_arp) {
+		return RX_HANDLER_ANOTHER;
  	}
  
  	alen = arp_hdr_len(bond->dev);
@@ -2349,7 +2351,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;
@@ -4181,10 +4182,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 1bab20e..9d6d231 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -434,12 +434,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);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 9f07af1..19eb023 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -319,6 +319,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)
@@ -326,6 +331,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.
   */

>
>	-J