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

[Ding Tianhong <dingtianhong@huawei.com>]

On 2013/11/14 18:10, Veaceslav Falico wrote:
> 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!
> 

ok

>> +        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...
> 
> .
>