[PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

The commit ecd9883724b [ipv6: fix race condition regarding dst->expires and
dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the TAHI
test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly generated
rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and dst.from
is NULL}.

Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem with
expired dst cache]

Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/ipv6/route.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a1a5752..e3d7f21 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1908,7 +1908,7 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
 			rt->rt6i_gateway = ort->rt6i_gateway;
 		else
 			rt->rt6i_gateway = *dest;
-		rt->rt6i_flags = ort->rt6i_flags;
+		rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
 		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
 		    (RTF_DEFAULT | RTF_ADDRCONF))
 			rt6_set_from(rt, ort);
-- 
1.7.10.4

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Eric Dumazet]

On Mon, 2013-12-16 at 16:31 +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> The commit ecd9883724b [ipv6: fix race condition regarding dst->expires and
> dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the TAHI
> test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly generated
> rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and dst.from
> is NULL}.
> 
> Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem with
> expired dst cache]

Any particular reason you didnt CC the author of the commit ?

Thanks

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/16/2013 04:31 PM, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> The commit ecd9883724b [ipv6: fix race condition regarding dst->expires and
> dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the TAHI
> test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly generated
> rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and dst.from
> is NULL}.
> 
> Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem with
> expired dst cache]
> 
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  net/ipv6/route.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index a1a5752..e3d7f21 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1908,7 +1908,7 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
>  			rt->rt6i_gateway = ort->rt6i_gateway;
>  		else
>  			rt->rt6i_gateway = *dest;
> -		rt->rt6i_flags = ort->rt6i_flags;
> +		rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
>  		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
>  		    (RTF_DEFAULT | RTF_ADDRCONF))
>  			rt6_set_from(rt, ort);
> 

I don't understand, rt6_set_from already clears the RTF_EXPIRES from rt->rt6i_flags.

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[RongQing Li]

If the ort->rt6i_flags is RTF_EXPIRES|RTF_ADDRCONF, then rt6_set_from will not
be called, and new created rt will have RTF_EXPIRES, but dst.expires is 0, and
dst.from is NULL

-Roy

2013/12/17 Gao feng <gaofeng@cn.fujitsu.com>:
> On 12/16/2013 04:31 PM, roy.qing.li@gmail.com wrote:
>> From: Li RongQing <roy.qing.li@gmail.com>
>>
>> The commit ecd9883724b [ipv6: fix race condition regarding dst->expires and
>> dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the TAHI
>> test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly generated
>> rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and dst.from
>> is NULL}.
>>
>> Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem with
>> expired dst cache]
>>
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>> ---
>>  net/ipv6/route.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index a1a5752..e3d7f21 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -1908,7 +1908,7 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
>>                       rt->rt6i_gateway = ort->rt6i_gateway;
>>               else
>>                       rt->rt6i_gateway = *dest;
>> -             rt->rt6i_flags = ort->rt6i_flags;
>> +             rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
>>               if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
>>                   (RTF_DEFAULT | RTF_ADDRCONF))
>>                       rt6_set_from(rt, ort);
>>
>
> I don't understand, rt6_set_from already clears the RTF_EXPIRES from rt->rt6i_flags.
>

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/17/2013 11:32 AM, RongQing Li wrote:
> If the ort->rt6i_flags is RTF_EXPIRES|RTF_ADDRCONF, then rt6_set_from will not
> be called, and new created rt will have RTF_EXPIRES, but dst.expires is 0, and
> dst.from is NULL

Ok, but I think you need to add more detail/test purpose of the test case v6LC.4.1.4
{ Reduce PMTU On-link }. just the number of test case is not good for people to know
what's the real problem.

> 
> -Roy
> 
> 2013/12/17 Gao feng <gaofeng@cn.fujitsu.com>:
>> On 12/16/2013 04:31 PM, roy.qing.li@gmail.com wrote:
>>> From: Li RongQing <roy.qing.li@gmail.com>
>>>
>>> The commit ecd9883724b [ipv6: fix race condition regarding dst->expires and
>>> dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the TAHI
>>> test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly generated
>>> rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and dst.from
>>> is NULL}.
>>>
>>> Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem with
>>> expired dst cache]
>>>
>>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>>> ---
>>>  net/ipv6/route.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index a1a5752..e3d7f21 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -1908,7 +1908,7 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
>>>                       rt->rt6i_gateway = ort->rt6i_gateway;
>>>               else
>>>                       rt->rt6i_gateway = *dest;
>>> -             rt->rt6i_flags = ort->rt6i_flags;
>>> +             rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
>>>               if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
>>>                   (RTF_DEFAULT | RTF_ADDRCONF))
>>>                       rt6_set_from(rt, ort);
>>>
>>
>> I don't understand, rt6_set_from already clears the RTF_EXPIRES from rt->rt6i_flags.
>>
> 

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[RongQing Li]

On 12/17/13, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> On 12/17/2013 11:32 AM, RongQing Li wrote:
>> If the ort->rt6i_flags is RTF_EXPIRES|RTF_ADDRCONF, then rt6_set_from will
>> not
>> be called, and new created rt will have RTF_EXPIRES, but dst.expires is 0,
>> and
>> dst.from is NULL
>
> Ok, but I think you need to add more detail/test purpose of the test case
> v6LC.4.1.4
> { Reduce PMTU On-link }. just the number of test case is not good for people
> to know
> what's the real problem.
>

I have a question, why does we set dst.from only when the ort has flag
RTF_ADDRCONF
and RTF_DEFAULT?

-Roy


>>
>> -Roy
>>
>> 2013/12/17 Gao feng <gaofeng@cn.fujitsu.com>:
>>> On 12/16/2013 04:31 PM, roy.qing.li@gmail.com wrote:
>>>> From: Li RongQing <roy.qing.li@gmail.com>
>>>>
>>>> The commit ecd9883724b [ipv6: fix race condition regarding dst->expires
>>>> and
>>>> dst->from.] removed rt6_clean_expires in ip6_rt_copy, which causes the
>>>> TAHI
>>>> test case v6LC.4.1.4 { Reduce PMTU On-link } failed, since the newly
>>>> generated
>>>> rt maybe always expired {with RTF_EXPIRES flag, dst.expires is 0, and
>>>> dst.from
>>>> is NULL}.
>>>>
>>>> Fix it by clearing RTF_EXPIRES as before 1716a96101[ipv6: fix problem
>>>> with
>>>> expired dst cache]
>>>>
>>>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>>>> ---
>>>>  net/ipv6/route.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>> index a1a5752..e3d7f21 100644
>>>> --- a/net/ipv6/route.c
>>>> +++ b/net/ipv6/route.c
>>>> @@ -1908,7 +1908,7 @@ static struct rt6_info *ip6_rt_copy(struct
>>>> rt6_info *ort,
>>>>                       rt->rt6i_gateway = ort->rt6i_gateway;
>>>>               else
>>>>                       rt->rt6i_gateway = *dest;
>>>> -             rt->rt6i_flags = ort->rt6i_flags;
>>>> +             rt->rt6i_flags = ort->rt6i_flags & ~RTF_EXPIRES;
>>>>               if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
>>>>                   (RTF_DEFAULT | RTF_ADDRCONF))
>>>>                       rt6_set_from(rt, ort);
>>>>
>>>
>>> I don't understand, rt6_set_from already clears the RTF_EXPIRES from
>>> rt->rt6i_flags.
>>>
>>
>
>

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Tue, Dec 17, 2013 at 02:42:01PM +0800, RongQing Li wrote:
> On 12/17/13, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> > On 12/17/2013 11:32 AM, RongQing Li wrote:
> >> If the ort->rt6i_flags is RTF_EXPIRES|RTF_ADDRCONF, then rt6_set_from will
> >> not
> >> be called, and new created rt will have RTF_EXPIRES, but dst.expires is 0,
> >> and
> >> dst.from is NULL
> >
> > Ok, but I think you need to add more detail/test purpose of the test case
> > v6LC.4.1.4
> > { Reduce PMTU On-link }. just the number of test case is not good for people
> > to know
> > what's the real problem.
> >
> 
> I have a question, why does we set dst.from only when the ort has flag
> RTF_ADDRCONF
> and RTF_DEFAULT?

Good question. ;)

I wonder, too. In the past from and expires were a union and either from or
expires was allowed to be used. In the commit you referred to this union was
split into seperate fields.

It somehow worked in the past because routes normally have longer timeouts
and the routes will get evicted from the route cache eventually. My guess
is that we can set from unconditionally or copy over the expires value.

This patch already needed quite a long time to review for me and I am still
unsure. :/

Gao, do you still remember why you used RTF_ADDRCONF|RTF_DEFAULT?

Greetings,

  Hannes

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/17/2013 03:02 PM, Hannes Frederic Sowa wrote:
> On Tue, Dec 17, 2013 at 02:42:01PM +0800, RongQing Li wrote:
>> On 12/17/13, Gao feng <gaofeng@cn.fujitsu.com> wrote:
>>> On 12/17/2013 11:32 AM, RongQing Li wrote:
>>>> If the ort->rt6i_flags is RTF_EXPIRES|RTF_ADDRCONF, then rt6_set_from will
>>>> not
>>>> be called, and new created rt will have RTF_EXPIRES, but dst.expires is 0,
>>>> and
>>>> dst.from is NULL
>>>
>>> Ok, but I think you need to add more detail/test purpose of the test case
>>> v6LC.4.1.4
>>> { Reduce PMTU On-link }. just the number of test case is not good for people
>>> to know
>>> what's the real problem.
>>>
>>
>> I have a question, why does we set dst.from only when the ort has flag
>> RTF_ADDRCONF
>> and RTF_DEFAULT?
> 
> Good question. ;)
> 
> I wonder, too. In the past from and expires were a union and either from or
> expires was allowed to be used. In the commit you referred to this union was
> split into seperate fields.
> 
> It somehow worked in the past because routes normally have longer timeouts
> and the routes will get evicted from the route cache eventually. My guess
> is that we can set from unconditionally or copy over the expires value.
> 
> This patch already needed quite a long time to review for me and I am still
> unsure. :/
> 
> Gao, do you still remember why you used RTF_ADDRCONF|RTF_DEFAULT?
> 

It's a mystery， I noticed this problem when I wrote the codes.
http://lists.openwall.net/netdev/2012/03/19/7

I used the flags RTF_ADDRCONF|RTF_DEFAULT because they are exist in
rt6_{get,add,purge}_dflt_router.

The from of new cloned rt should not be set if it's impossible for the ort
to be expired.

but seems we should set from if flags have RTF_ADDRCONF bit. RA package
not only generate the default route.

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
> > Gao, do you still remember why you used RTF_ADDRCONF|RTF_DEFAULT?
> > 
> 
> It's a mystery， I noticed this problem when I wrote the codes.
> http://lists.openwall.net/netdev/2012/03/19/7

I already found this thread, thanks. ;)

> I used the flags RTF_ADDRCONF|RTF_DEFAULT because they are exist in
> rt6_{get,add,purge}_dflt_router.

I thought so, but we have to deal with !DEFAULT ADDRCONF routes, too.

> The from of new cloned rt should not be set if it's impossible for the ort
> to be expired.

Ok.

> but seems we should set from if flags have RTF_ADDRCONF bit. RA package
> not only generate the default route.

Exactly, but it is worse:

Prefix routes can be added with expiration time if user space installs a
prefix with valid_lft != infinity. So I fear we already have permament route
entries which expire.

Userspace router advertisment listener already use that.

I fear the flags don't have a well defined semantic any more. :(

As for the original patch in this thread, I would suggest to hold it
back until this mess is understood. Ok?

Thanks,

  Hannes

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/17/2013 04:30 PM, Hannes Frederic Sowa wrote:
> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>>> Gao, do you still remember why you used RTF_ADDRCONF|RTF_DEFAULT?
>>>
>>
>> It's a mystery， I noticed this problem when I wrote the codes.
>> http://lists.openwall.net/netdev/2012/03/19/7
> 
> I already found this thread, thanks. ;)
> 
>> I used the flags RTF_ADDRCONF|RTF_DEFAULT because they are exist in
>> rt6_{get,add,purge}_dflt_router.
> 
> I thought so, but we have to deal with !DEFAULT ADDRCONF routes, too.

Right.

> 
>> The from of new cloned rt should not be set if it's impossible for the ort
>> to be expired.
> 
> Ok.
> 
>> but seems we should set from if flags have RTF_ADDRCONF bit. RA package
>> not only generate the default route.
> 
> Exactly, but it is worse:
> 
> Prefix routes can be added with expiration time if user space installs a
> prefix with valid_lft != infinity. So I fear we already have permament route
> entries which expire.

Yes, this should happen..
> 
> Userspace router advertisment listener already use that.
> 
> I fear the flags don't have a well defined semantic any more. :(
> 
> As for the original patch in this thread, I would suggest to hold it
> back until this mess is understood. Ok?
> 

I'm ok if we want the behave back to the commit 1716a96101[ipv6: fix problem with
expired dst cache].

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
> The from of new cloned rt should not be set if it's impossible for the ort
> to be expired.

Actually, why do you think so? What could go wrong?

Greetings,

  Hannes

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>> The from of new cloned rt should not be set if it's impossible for the ort
>> to be expired.
> 
> Actually, why do you think so? What could go wrong?
> 

I just don't want rt6_check_expired to check some impossible expired routes.

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[RongQing Li]

On Wed, Dec 18, 2013 at 8:48 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
>> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>>> The from of new cloned rt should not be set if it's impossible for the ort
>>> to be expired.
>>
>> Actually, why do you think so? What could go wrong?
>>
>
> I just don't want rt6_check_expired to check some impossible expired routes.
>

What is wrong if we set from for new cloned rt by checking if ort has
RTF_EXPIRES flag?

-Roy

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/18/2013 09:58 AM, RongQing Li wrote:
> On Wed, Dec 18, 2013 at 8:48 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
>> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
>>> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>>>> The from of new cloned rt should not be set if it's impossible for the ort
>>>> to be expired.
>>>
>>> Actually, why do you think so? What could go wrong?
>>>
>>
>> I just don't want rt6_check_expired to check some impossible expired routes.
>>
> 
> What is wrong if we set from for new cloned rt by checking if ort has
> RTF_EXPIRES flag?


The RTF_EXPIRES flag may be changed by router advertisment package,
the ort may become expired after you hadn't set from for new cloned rt.

we should set from even this kind of ort doesn't have RTF_EXPIRES flag.

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[RongQing Li]

On Wed, Dec 18, 2013 at 10:09 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> On 12/18/2013 09:58 AM, RongQing Li wrote:
>> On Wed, Dec 18, 2013 at 8:48 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
>>> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
>>>> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>>>>> The from of new cloned rt should not be set if it's impossible for the ort
>>>>> to be expired.
>>>>
>>>> Actually, why do you think so? What could go wrong?
>>>>
>>>
>>> I just don't want rt6_check_expired to check some impossible expired routes.
>>>
>>
>> What is wrong if we set from for new cloned rt by checking if ort has
>> RTF_EXPIRES flag?
>
>
> The RTF_EXPIRES flag may be changed by router advertisment package,
> the ort may become expired after you hadn't set from for new cloned rt.
>
> we should set from even this kind of ort doesn't have RTF_EXPIRES flag.


Thanks;

Do we want to set from only from RA route?  if so,  we should check
ort with RTF_ADDRCONF|RTF_DEFAULT, or RTF_ADDRCONF | RTF_ROUTEINFO,

like in rt6_fill_node


        else if (rt->rt6i_flags & RTF_ADDRCONF) {
                if (rt->rt6i_flags & (RTF_DEFAULT | RTF_ROUTEINFO))
                        rtm->rtm_protocol = RTPROT_RA;
                else
                        rtm->rtm_protocol = RTPROT_KERNEL;
        }


-R

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Wed, Dec 18, 2013 at 10:21:12AM +0800, RongQing Li wrote:
> On Wed, Dec 18, 2013 at 10:09 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> > On 12/18/2013 09:58 AM, RongQing Li wrote:
> >> On Wed, Dec 18, 2013 at 8:48 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> >>> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
> >>>> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
> >>>>> The from of new cloned rt should not be set if it's impossible for the ort
> >>>>> to be expired.
> >>>>
> >>>> Actually, why do you think so? What could go wrong?
> >>>>
> >>>
> >>> I just don't want rt6_check_expired to check some impossible expired routes.
> >>>
> >>
> >> What is wrong if we set from for new cloned rt by checking if ort has
> >> RTF_EXPIRES flag?
> >
> >
> > The RTF_EXPIRES flag may be changed by router advertisment package,
> > the ort may become expired after you hadn't set from for new cloned rt.
> >
> > we should set from even this kind of ort doesn't have RTF_EXPIRES flag.
> 
> 
> Thanks;
> 
> Do we want to set from only from RA route?  if so,  we should check
> ort with RTF_ADDRCONF|RTF_DEFAULT, or RTF_ADDRCONF | RTF_ROUTEINFO,

Nope, as I said, also prefix routes which did get installed by hand locally
can have an expiration. I don't see any flag combination which ensure a
potential from does never expire. IMHO we should always from.

Also, in case we overwrite a route and the route is already in there, we reset
the expiry values: search for rt6_set_expires in fib6_add_rt2node.

We had the same problem with ECMP routes. We only wanted to add non-expiration
routes but we could not find a combination which did ensure that. In the end
we now also add non-expiration routes to an ecmp set (see commit
307f2fb95e9b96).

Greetings,

  Hannes

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Gao feng]

On 12/18/2013 02:20 PM, Hannes Frederic Sowa wrote:
> On Wed, Dec 18, 2013 at 10:21:12AM +0800, RongQing Li wrote:
>> On Wed, Dec 18, 2013 at 10:09 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
>>> On 12/18/2013 09:58 AM, RongQing Li wrote:
>>>> On Wed, Dec 18, 2013 at 8:48 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
>>>>> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
>>>>>> On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
>>>>>>> The from of new cloned rt should not be set if it's impossible for the ort
>>>>>>> to be expired.
>>>>>>
>>>>>> Actually, why do you think so? What could go wrong?
>>>>>>
>>>>>
>>>>> I just don't want rt6_check_expired to check some impossible expired routes.
>>>>>
>>>>
>>>> What is wrong if we set from for new cloned rt by checking if ort has
>>>> RTF_EXPIRES flag?
>>>
>>>
>>> The RTF_EXPIRES flag may be changed by router advertisment package,
>>> the ort may become expired after you hadn't set from for new cloned rt.
>>>
>>> we should set from even this kind of ort doesn't have RTF_EXPIRES flag.
>>
>>
>> Thanks;
>>
>> Do we want to set from only from RA route?  if so,  we should check
>> ort with RTF_ADDRCONF|RTF_DEFAULT, or RTF_ADDRCONF | RTF_ROUTEINFO,
> 
> Nope, as I said, also prefix routes which did get installed by hand locally
> can have an expiration. I don't see any flag combination which ensure a
> potential from does never expire. IMHO we should always from.
> 
> Also, in case we overwrite a route and the route is already in there, we reset
> the expiry values: search for rt6_set_expires in fib6_add_rt2node.

Seem like user can add expired route by manually. rtmsg_to_fib6_config set the
cfg->fc_expires. I'm ok if you want to set from always now.

Thanks

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Wed, Dec 18, 2013 at 04:40:04PM +0800, Gao feng wrote:
> Seem like user can add expired route by manually. rtmsg_to_fib6_config set the
> cfg->fc_expires. I'm ok if you want to set from always now.

Roy, can you send an updated patch now?

Thanks,

  Hannes

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[RongQing Li]

On Thu, Dec 19, 2013 at 8:37 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Wed, Dec 18, 2013 at 04:40:04PM +0800, Gao feng wrote:
>> Seem like user can add expired route by manually. rtmsg_to_fib6_config set the
>> cfg->fc_expires. I'm ok if you want to set from always now.
>
> Roy, can you send an updated patch now?
>
> Thanks,
>
>   Hannes
>

Ok, Thanks

-Roy

[PATCH v2] ipv6: always set the new created dst's from in ip6_rt_copy

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com> 

ip6_rt_copy only sets dst.from if ort has flag RTF_ADDRCONF and RTF_DEFAULT.
but the prefix routes which did get installed by hand locally can have an
expiration, and no any flag combination which can ensure a potential from
does never expire, so we should always set the new created dst's from.

This also fixes the new created dst is always expired since the ort, which
is created by RA, maybe has RTF_EXPIRES and RTF_ADDRCONF, but no RTF_DEFAULT.

Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
CC: Gao feng <gaofeng@cn.fujitsu.com>
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/ipv6/route.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a1a5752..5da9fb1 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1909,9 +1909,7 @@ static struct rt6_info *ip6_rt_copy(struct rt6_info *ort,
 		else
 			rt->rt6i_gateway = *dest;
 		rt->rt6i_flags = ort->rt6i_flags;
-		if ((ort->rt6i_flags & (RTF_DEFAULT | RTF_ADDRCONF)) ==
-		    (RTF_DEFAULT | RTF_ADDRCONF))
-			rt6_set_from(rt, ort);
+		rt6_set_from(rt, ort);
 		rt->rt6i_metric = 0;
 
 #ifdef CONFIG_IPV6_SUBTREES
-- 
1.7.10.4

Re: [PATCH v2] ipv6: always set the new created dst's from in ip6_rt_copy

[Hannes Frederic Sowa]

On Thu, Dec 19, 2013 at 12:40:26PM +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com> 
> 
> ip6_rt_copy only sets dst.from if ort has flag RTF_ADDRCONF and RTF_DEFAULT.
> but the prefix routes which did get installed by hand locally can have an
> expiration, and no any flag combination which can ensure a potential from
> does never expire, so we should always set the new created dst's from.
> 
> This also fixes the new created dst is always expired since the ort, which
> is created by RA, maybe has RTF_EXPIRES and RTF_ADDRCONF, but no RTF_DEFAULT.
> 
> Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> CC: Gao feng <gaofeng@cn.fujitsu.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks!

Re: [PATCH v2] ipv6: always set the new created dst's from in ip6_rt_copy

[David Miller]

From: roy.qing.li@gmail.com
Date: Thu, 19 Dec 2013 12:40:26 +0800

> From: Li RongQing <roy.qing.li@gmail.com> 
> 
> ip6_rt_copy only sets dst.from if ort has flag RTF_ADDRCONF and RTF_DEFAULT.
> but the prefix routes which did get installed by hand locally can have an
> expiration, and no any flag combination which can ensure a potential from
> does never expire, so we should always set the new created dst's from.
> 
> This also fixes the new created dst is always expired since the ort, which
> is created by RA, maybe has RTF_EXPIRES and RTF_ADDRCONF, but no RTF_DEFAULT.
> 
> Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> CC: Gao feng <gaofeng@cn.fujitsu.com>
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

Applied and queued up for -stable, thanks.

Re: [PATCH] ipv6: clear RTF_EXPIRES when call ip6_rt_copy

[Hannes Frederic Sowa]

On Wed, Dec 18, 2013 at 08:48:04AM +0800, Gao feng wrote:
> On 12/17/2013 09:48 PM, Hannes Frederic Sowa wrote:
> > On Tue, Dec 17, 2013 at 03:46:24PM +0800, Gao feng wrote:
> >> The from of new cloned rt should not be set if it's impossible for the ort
> >> to be expired.
> > 
> > Actually, why do you think so? What could go wrong?
> > 
> 
> I just don't want rt6_check_expired to check some impossible expired routes.

Only for performance? It does not seem to do any harm?