Re: [PATCH net v2] bonding: add ip checks when store ip target

[Veaceslav Falico <vfalico@redhat.com>]

On Thu, Nov 14, 2013 at 10:35:44AM +0800, Ding Tianhong wrote:
...snip...
>-	newtarget = in_aton(buf + 1);
>+	if (!in4_pton(buf + 1, -1, (u8 *)&newtarget, -1, NULL) ||
>+			newtarget == 0) {

Good catch on that newtarget verification, forgot to add it. However, the
previous checked not only for all-zeroes, but also for the broadcast, so
it'd be great if you could maintain the previous functionality/checks.

Or, even better, you could do something like SCTP does in
IS_IPV4_UNUSABLE_ADDRESS():

353 #define IS_IPV4_UNUSABLE_ADDRESS(a)         \
354         ((htonl(INADDR_BROADCAST) == a) ||  \
355          ipv4_is_multicast(a) ||            \
356          ipv4_is_zeronet(a) ||              \
357          ipv4_is_test_198(a) ||             \
358          ipv4_is_anycast_6to4(a))

It's up to you, though, I think that either way works good - it's
sysadmin's job to know which addresses can/have to be used.

Also, fix the identation.

Thanks!

>+		pr_err("%s: invalid ARP target %pI4 specified for addition\n",
>+		       bond->dev->name, &newtarget);
>+		goto out;
>+	}
> 	/* look for adds */
> 	if (buf[0] == '+') {
>-		if ((newtarget == 0) || (newtarget == htonl(INADDR_BROADCAST))) {
>-			pr_err("%s: invalid ARP target %pI4 specified for addition\n",
>-			       bond->dev->name, &newtarget);
>-			goto out;
>-		}
>-
> 		if (bond_get_targets_ip(targets, newtarget) != -1) { /* dup */
> 			pr_err("%s: ARP target %pI4 is already present\n",
> 			       bond->dev->name, &newtarget);
...snip...