netdev - Re: [PATCH 3/4] bonding: arp_ip_count and arp

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30653.1368813652@death.nxdomain>
Date:	Fri, 17 May 2013 11:00:52 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
cc:	netdev@...r.kernel.org, andy@...yhouse.net, davem@...emloft.net
Subject: Re: [PATCH 3/4] bonding: arp_ip_count and arp_targets can be wrong

Nikolay Aleksandrov <nikolay@...hat.com> wrote:

>When getting arp_ip_targets if we encounter a bad IP, arp_ip_count still
>gets increased and all the targets after the wrong one will not be probed
>if arp_interval is enabled after that (unless a new IP target is added
>through sysfs) because of the zero entry, in this case reading
>arp_ip_target through sysfs will show valid targets even if there's a
>zero entry.
>Example: 1.2.3.4,4.5.6.7,blah,5.6.7.8
>When retrieving the list from arp_ip_target the output would be:
>1.2.3.4,4.5.6.7,5.6.7.8
>but there will be a 0 entry between 4.5.6.7 and 5.6.7.8. If arp_interval
>is enabled after that 5.6.7.8 will never be checked because of that.

	This patch looks good for the description above.

>Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.com>
>---
>Question about this one: Do we have to disable arp_interval if we have
>obtained at least 1 valid IP address ? 
>I think this can be addressed in a net-next patch.

	My personal thought is that, ideally, it should be all or none,
i.e., either all of the targets are valid and are accepted as a set, and
if any are wrong, the entire set is rejected.

	That change could break existing installations, although
probably only on embedded or other systems without sysfs; regular
distros generally configure bonding via sysfs.

	If we're not going to do it the right way for fear of
compatibility issues, then I'd just leave it alone.  If compatibility
isn't a concern, then I'd fix it as I described above.

	-J

> drivers/net/bonding/bond_main.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1a0cc13..d6a96cb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4470,7 +4470,7 @@ int bond_parse_parm(const char *buf, const struct bond_parm_tbl *tbl)
>
> static int bond_check_params(struct bond_params *params)
> {
>-	int arp_validate_value, fail_over_mac_value, primary_reselect_value;
>+	int arp_validate_value, fail_over_mac_value, primary_reselect_value, i;
>
> 	/*
> 	 * Convert string parameters.
>@@ -4650,19 +4650,18 @@ static int bond_check_params(struct bond_params *params)
> 		arp_interval = BOND_LINK_ARP_INTERV;
> 	}
>
>-	for (arp_ip_count = 0;
>-	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[arp_ip_count];
>-	     arp_ip_count++) {
>+	for (arp_ip_count = 0, i = 0;
>+	     (arp_ip_count < BOND_MAX_ARP_TARGETS) && arp_ip_target[i]; i++) {
> 		/* not complete check, but should be good enough to
> 		   catch mistakes */
>-		__be32 ip = in_aton(arp_ip_target[arp_ip_count]);
>-		if (!isdigit(arp_ip_target[arp_ip_count][0]) ||
>-		    ip == 0 || ip == htonl(INADDR_BROADCAST)) {
>+		__be32 ip = in_aton(arp_ip_target[i]);
>+		if (!isdigit(arp_ip_target[i][0]) || ip == 0 ||
>+		    ip == htonl(INADDR_BROADCAST)) {
> 			pr_warning("Warning: bad arp_ip_target module parameter (%s), ARP monitoring will not be performed\n",
>-				   arp_ip_target[arp_ip_count]);
>+				   arp_ip_target[i]);
> 			arp_interval = 0;
> 		} else {
>-			arp_target[arp_ip_count] = ip;
>+			arp_target[arp_ip_count++] = ip;
> 		}
> 	}
>
>@@ -4696,8 +4695,6 @@ static int bond_check_params(struct bond_params *params)
> 	if (miimon) {
> 		pr_info("MII link monitoring set to %d ms\n", miimon);
> 	} else if (arp_interval) {
>-		int i;
>-
> 		pr_info("ARP monitoring set to %d ms, validate %s, with %d target(s):",
> 			arp_interval,
> 			arp_validate_tbl[arp_validate_value].modename,
>-- 
>1.8.1.4

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html