Re: [PATCHv3] net/ipv6: add sysctl option accept_ra_min_hop_limit

[YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com>]

Hi,

Hangbin Liu wrote:
>>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>>> index 0a05b35..acda056 100644
>>> --- a/net/ipv6/ndisc.c
>>> +++ b/net/ipv6/ndisc.c
>>> @@ -1226,13 +1226,11 @@ static void ndisc_router_discovery(struct sk_buff *skb)
>>>       if (rt)
>>>               rt6_set_expires(rt, jiffies + (HZ * lifetime));
>>>       if (ra_msg->icmph.icmp6_hop_limit) {
>>> -             /* Only set hop_limit on the interface if it is higher than
>>> -              * the current hop_limit.
>>> -              */
>>> -             if (in6_dev->cnf.hop_limit < ra_msg->icmph.icmp6_hop_limit) {
>>> +             if (in6_dev->cnf.accept_ra_min_hop_limit <= ra_msg->icmph.icmp6_hop_limit &&
>>> +                 ra_msg->icmph.icmp6_hop_limit != 0) {
>>>                       in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
>>>               } else {
>>> -                     ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than current\n");
>>> +                     ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than minimum\n");
>>>               }
>>>               if (rt)
>>>                       dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
>>>
>>
>> Please see my comments against your previous patch.
> 
> I pasted you comments here so we don't need to discuss in two mails :)
> 
>>
>> ra_msg->icmph.icmp6_hop_limit != 0 is checkd by outer "if".
> 
> Yes, thanks for your reminding :)
> 
>>
>> You do not need to update cnf.hop_limit if it is already equal to
>> hop limit received.
> 
> We need to update cnf.hop_limit if min_hop_limit <= icmp6_hop_limit. e.g.
> current hop limit is 64, min hop limit is 1 and ra hop limit is 32, then we need
> update current hop limit to 32.

OK

> 
>>
>> How about ignoring hop limit without message is configured value is
>> larger than 255, BTW?
> 
> Although set accept_ra_min_hop_limit great than 255 is meaningless,  there
> is also no need to check it since icmp6_hop_limit will not larger than 255. so
> 
> +               if (in6_dev->cnf.accept_ra_min_hop_limit <= 255 &&
> +                   in6_dev->cnf.accept_ra_min_hop_limit <=
> ra_msg->icmph.icmp6_hop_limit )
>                         in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
> 
> is  duplicated check. How do you think?

How about checking in6_dev->cnf.accept_ra_min_hop_limit by outer if, then?


if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
    ra_msg->icmph.icmp6_hop_limit) {
...
}

> 
>>
>>>               if (rt)
>>>                       dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
>>>
>>
>> This can be inside the inner "if".
> 
> OK, will move it.
> 
> 
> Best Regards
> Hangbin
> 

Regards,

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION