[PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[Xin Long]

In inet6_rtm_getroute, since Commit 93531c674315 ("net/ipv6: separate
handling of FIB entries from dst based routes"), it has used rt->from
to dump route info instead of rt.

However for some route like cache, its information is not the same as
that of the 'from' one. It caused 'ip -6 route get' to dump the wrong
route information.

In Jianlin's testing, the output information even lost the expiration
time for a pmtu route cache due to the wrong fib6_flags.

So change to use rt6_info members when it tries to dump a route entry
without fibmatch set.

Note that we will fix the gw/nh dump in another patch.

Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv6/route.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 18e00ce..e554922 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			 int iif, int type, u32 portid, u32 seq,
 			 unsigned int flags)
 {
-	struct rtmsg *rtm;
+	struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
+	struct rt6_info *rt6 = (struct rt6_info *)dst;
+	u32 *pmetrics, table, fib6_flags;
 	struct nlmsghdr *nlh;
+	struct rtmsg *rtm;
 	long expires = 0;
-	u32 *pmetrics;
-	u32 table;
 
 	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
 	if (!nlh)
 		return -EMSGSIZE;
 
+	if (rt6) {
+		fib6_dst = &rt6->rt6i_dst;
+		fib6_src = &rt6->rt6i_src;
+		fib6_flags = rt6->rt6i_flags;
+		fib6_prefsrc = &rt6->rt6i_prefsrc;
+	} else {
+		fib6_dst = &rt->fib6_dst;
+		fib6_src = &rt->fib6_src;
+		fib6_flags = rt->fib6_flags;
+		fib6_prefsrc = &rt->fib6_prefsrc;
+	}
+
 	rtm = nlmsg_data(nlh);
 	rtm->rtm_family = AF_INET6;
-	rtm->rtm_dst_len = rt->fib6_dst.plen;
-	rtm->rtm_src_len = rt->fib6_src.plen;
+	rtm->rtm_dst_len = fib6_dst->plen;
+	rtm->rtm_src_len = fib6_src->plen;
 	rtm->rtm_tos = 0;
 	if (rt->fib6_table)
 		table = rt->fib6_table->tb6_id;
@@ -4698,7 +4711,7 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 	rtm->rtm_scope = RT_SCOPE_UNIVERSE;
 	rtm->rtm_protocol = rt->fib6_protocol;
 
-	if (rt->fib6_flags & RTF_CACHE)
+	if (fib6_flags & RTF_CACHE)
 		rtm->rtm_flags |= RTM_F_CLONED;
 
 	if (dest) {
@@ -4706,7 +4719,7 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			goto nla_put_failure;
 		rtm->rtm_dst_len = 128;
 	} else if (rtm->rtm_dst_len)
-		if (nla_put_in6_addr(skb, RTA_DST, &rt->fib6_dst.addr))
+		if (nla_put_in6_addr(skb, RTA_DST, &fib6_dst->addr))
 			goto nla_put_failure;
 #ifdef CONFIG_IPV6_SUBTREES
 	if (src) {
@@ -4714,12 +4727,12 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			goto nla_put_failure;
 		rtm->rtm_src_len = 128;
 	} else if (rtm->rtm_src_len &&
-		   nla_put_in6_addr(skb, RTA_SRC, &rt->fib6_src.addr))
+		   nla_put_in6_addr(skb, RTA_SRC, &fib6_src->addr))
 		goto nla_put_failure;
 #endif
 	if (iif) {
 #ifdef CONFIG_IPV6_MROUTE
-		if (ipv6_addr_is_multicast(&rt->fib6_dst.addr)) {
+		if (ipv6_addr_is_multicast(&fib6_dst->addr)) {
 			int err = ip6mr_get_route(net, skb, rtm, portid);
 
 			if (err == 0)
@@ -4737,9 +4750,9 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			goto nla_put_failure;
 	}
 
-	if (rt->fib6_prefsrc.plen) {
+	if (fib6_prefsrc->plen) {
 		struct in6_addr saddr_buf;
-		saddr_buf = rt->fib6_prefsrc.addr;
+		saddr_buf = fib6_prefsrc->addr;
 		if (nla_put_in6_addr(skb, RTA_PREFSRC, &saddr_buf))
 			goto nla_put_failure;
 	}
@@ -4777,7 +4790,7 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			goto nla_put_failure;
 	}
 
-	if (rt->fib6_flags & RTF_EXPIRES) {
+	if (fib6_flags & RTF_EXPIRES) {
 		expires = dst ? dst->expires : rt->expires;
 		expires -= jiffies;
 	}
@@ -4785,7 +4798,7 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 	if (rtnl_put_cacheinfo(skb, dst, 0, expires, dst ? dst->error : 0) < 0)
 		goto nla_put_failure;
 
-	if (nla_put_u8(skb, RTA_PREF, IPV6_EXTRACT_PREF(rt->fib6_flags)))
+	if (nla_put_u8(skb, RTA_PREF, IPV6_EXTRACT_PREF(fib6_flags)))
 		goto nla_put_failure;
 
 
-- 
2.1.0

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[David Ahern]

On 9/8/18 3:24 AM, Xin Long wrote:
> In inet6_rtm_getroute, since Commit 93531c674315 ("net/ipv6: separate
> handling of FIB entries from dst based routes"), it has used rt->from
> to dump route info instead of rt.
> 
> However for some route like cache, its information is not the same as
> that of the 'from' one. It caused 'ip -6 route get' to dump the wrong
> route information.
> 
> In Jianlin's testing, the output information even lost the expiration
> time for a pmtu route cache due to the wrong fib6_flags.

you are right about the flags ...

> 
> So change to use rt6_info members when it tries to dump a route entry
> without fibmatch set.

but not the src, dst and prefsrc.

> 
> Note that we will fix the gw/nh dump in another patch.

And only the gateway can change do to a redirect and redirects do not
change the device - only the gateway.

Let's do both changes in a single patch.

> 
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Reported-by: Jianlin Shi <jishi@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/ipv6/route.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 18e00ce..e554922 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
>  			 int iif, int type, u32 portid, u32 seq,
>  			 unsigned int flags)
>  {
> -	struct rtmsg *rtm;
> +	struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
> +	struct rt6_info *rt6 = (struct rt6_info *)dst;
> +	u32 *pmetrics, table, fib6_flags;
>  	struct nlmsghdr *nlh;
> +	struct rtmsg *rtm;
>  	long expires = 0;
> -	u32 *pmetrics;
> -	u32 table;
>  
>  	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> +	if (rt6) {
> +		fib6_dst = &rt6->rt6i_dst;
> +		fib6_src = &rt6->rt6i_src;
> +		fib6_flags = rt6->rt6i_flags;
> +		fib6_prefsrc = &rt6->rt6i_prefsrc;
> +	} else {
> +		fib6_dst = &rt->fib6_dst;
> +		fib6_src = &rt->fib6_src;
> +		fib6_flags = rt->fib6_flags;
> +		fib6_prefsrc = &rt->fib6_prefsrc;
> +	}

Unless I am missing something at the moment, an rt6_info can only have
the same dst, src and prefsrc as the fib6_info on which it is based.
Thus, only the flags is needed above. That simplifies this patch a lot.

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[Xin Long]

On Sun, Sep 9, 2018 at 9:45 AM David Ahern <dsa@cumulusnetworks.com> wrote:
>
> On 9/8/18 3:24 AM, Xin Long wrote:
> > In inet6_rtm_getroute, since Commit 93531c674315 ("net/ipv6: separate
> > handling of FIB entries from dst based routes"), it has used rt->from
> > to dump route info instead of rt.
> >
> > However for some route like cache, its information is not the same as
> > that of the 'from' one. It caused 'ip -6 route get' to dump the wrong
> > route information.
> >
> > In Jianlin's testing, the output information even lost the expiration
> > time for a pmtu route cache due to the wrong fib6_flags.
>
> you are right about the flags ...
>
> >
> > So change to use rt6_info members when it tries to dump a route entry
> > without fibmatch set.
>
> but not the src, dst and prefsrc.
>
> >
> > Note that we will fix the gw/nh dump in another patch.
>
> And only the gateway can change do to a redirect and redirects do not
> change the device - only the gateway.
>
> Let's do both changes in a single patch.
>
> >
> > Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> > Reported-by: Jianlin Shi <jishi@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/ipv6/route.c | 39 ++++++++++++++++++++++++++-------------
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index 18e00ce..e554922 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
> >                        int iif, int type, u32 portid, u32 seq,
> >                        unsigned int flags)
> >  {
> > -     struct rtmsg *rtm;
> > +     struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
> > +     struct rt6_info *rt6 = (struct rt6_info *)dst;
> > +     u32 *pmetrics, table, fib6_flags;
> >       struct nlmsghdr *nlh;
> > +     struct rtmsg *rtm;
> >       long expires = 0;
> > -     u32 *pmetrics;
> > -     u32 table;
> >
> >       nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
> >       if (!nlh)
> >               return -EMSGSIZE;
> >
> > +     if (rt6) {
> > +             fib6_dst = &rt6->rt6i_dst;
> > +             fib6_src = &rt6->rt6i_src;
> > +             fib6_flags = rt6->rt6i_flags;
> > +             fib6_prefsrc = &rt6->rt6i_prefsrc;
> > +     } else {
> > +             fib6_dst = &rt->fib6_dst;
> > +             fib6_src = &rt->fib6_src;
> > +             fib6_flags = rt->fib6_flags;
> > +             fib6_prefsrc = &rt->fib6_prefsrc;
> > +     }
>
> Unless I am missing something at the moment, an rt6_info can only have
> the same dst, src and prefsrc as the fib6_info on which it is based.
> Thus, only the flags is needed above. That simplifies this patch a lot.
If dst, src and prefsrc in rt6_info are always the same as these in fib6_info,
why do we need them in rt6_info? we could just get it by 'from'.

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[David Ahern]

On 9/9/18 12:29 AM, Xin Long wrote:
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 18e00ce..e554922 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
>>>                        int iif, int type, u32 portid, u32 seq,
>>>                        unsigned int flags)
>>>  {
>>> -     struct rtmsg *rtm;
>>> +     struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
>>> +     struct rt6_info *rt6 = (struct rt6_info *)dst;
>>> +     u32 *pmetrics, table, fib6_flags;
>>>       struct nlmsghdr *nlh;
>>> +     struct rtmsg *rtm;
>>>       long expires = 0;
>>> -     u32 *pmetrics;
>>> -     u32 table;
>>>
>>>       nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
>>>       if (!nlh)
>>>               return -EMSGSIZE;
>>>
>>> +     if (rt6) {
>>> +             fib6_dst = &rt6->rt6i_dst;
>>> +             fib6_src = &rt6->rt6i_src;
>>> +             fib6_flags = rt6->rt6i_flags;
>>> +             fib6_prefsrc = &rt6->rt6i_prefsrc;
>>> +     } else {
>>> +             fib6_dst = &rt->fib6_dst;
>>> +             fib6_src = &rt->fib6_src;
>>> +             fib6_flags = rt->fib6_flags;
>>> +             fib6_prefsrc = &rt->fib6_prefsrc;
>>> +     }
>>
>> Unless I am missing something at the moment, an rt6_info can only have
>> the same dst, src and prefsrc as the fib6_info on which it is based.
>> Thus, only the flags is needed above. That simplifies this patch a lot.
> If dst, src and prefsrc in rt6_info are always the same as these in fib6_info,
> why do we need them in rt6_info? we could just get it by 'from'.
> 

I just sent a patch removing rt6i_prefsrc. It is set with only 1 reader
that can be converted.

rt6i_src is checked against the fib6_info to invalidate a dst if the src
has changed, so a valid rt will always have the same rt6i_src as the
rt->from.

rt6i_dst is set to the dest address / 128 in cases, so it should be used
for rt6_info cases above.

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[Xin Long]

On Tue, Sep 11, 2018 at 12:13 AM David Ahern <dsa@cumulusnetworks.com> wrote:
>
> On 9/9/18 12:29 AM, Xin Long wrote:
> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> >>> index 18e00ce..e554922 100644
> >>> --- a/net/ipv6/route.c
> >>> +++ b/net/ipv6/route.c
> >>> @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
> >>>                        int iif, int type, u32 portid, u32 seq,
> >>>                        unsigned int flags)
> >>>  {
> >>> -     struct rtmsg *rtm;
> >>> +     struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
> >>> +     struct rt6_info *rt6 = (struct rt6_info *)dst;
> >>> +     u32 *pmetrics, table, fib6_flags;
> >>>       struct nlmsghdr *nlh;
> >>> +     struct rtmsg *rtm;
> >>>       long expires = 0;
> >>> -     u32 *pmetrics;
> >>> -     u32 table;
> >>>
> >>>       nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
> >>>       if (!nlh)
> >>>               return -EMSGSIZE;
> >>>
> >>> +     if (rt6) {
> >>> +             fib6_dst = &rt6->rt6i_dst;
> >>> +             fib6_src = &rt6->rt6i_src;
> >>> +             fib6_flags = rt6->rt6i_flags;
> >>> +             fib6_prefsrc = &rt6->rt6i_prefsrc;
> >>> +     } else {
> >>> +             fib6_dst = &rt->fib6_dst;
> >>> +             fib6_src = &rt->fib6_src;
> >>> +             fib6_flags = rt->fib6_flags;
> >>> +             fib6_prefsrc = &rt->fib6_prefsrc;
> >>> +     }
> >>
> >> Unless I am missing something at the moment, an rt6_info can only have
> >> the same dst, src and prefsrc as the fib6_info on which it is based.
> >> Thus, only the flags is needed above. That simplifies this patch a lot.
> > If dst, src and prefsrc in rt6_info are always the same as these in fib6_info,
> > why do we need them in rt6_info? we could just get it by 'from'.
> >
>
> I just sent a patch removing rt6i_prefsrc. It is set with only 1 reader
> that can be converted.
>
> rt6i_src is checked against the fib6_info to invalidate a dst if the src
> has changed, so a valid rt will always have the same rt6i_src as the
> rt->from.
>
> rt6i_dst is set to the dest address / 128 in cases, so it should be used
> for rt6_info cases above.
So that means, I will use rt6i_dst and rt6i_flags when dst is set?
how about I use rt6i_src there as well? just to make it look clear.
and plus the gw/nh dump fix in rt6_fill_node():
-        if (rt->fib6_nsiblings) {
+        if (rt6) {
+                if (fib6_flags & RTF_GATEWAY)
+                        if (nla_put_in6_addr(skb, RTA_GATEWAY,
+                                             &rt6->rt6i_gateway) < 0)
+                                goto nla_put_failure;
+
+                if (dst->dev && nla_put_u32(skb, RTA_OIF, dst->dev->ifindex))
+                        goto nla_put_failure;
+        } else if (rt->fib6_nsiblings) {
                 struct fib6_info *sibling, *next_sibling;
                 struct nlattr *mp;

looks good to you?

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[David Ahern]

On 9/10/18 11:55 AM, Xin Long wrote:
> On Tue, Sep 11, 2018 at 12:13 AM David Ahern <dsa@cumulusnetworks.com> wrote:
>>
>> On 9/9/18 12:29 AM, Xin Long wrote:
>>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>>> index 18e00ce..e554922 100644
>>>>> --- a/net/ipv6/route.c
>>>>> +++ b/net/ipv6/route.c
>>>>> @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
>>>>>                        int iif, int type, u32 portid, u32 seq,
>>>>>                        unsigned int flags)
>>>>>  {
>>>>> -     struct rtmsg *rtm;
>>>>> +     struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
>>>>> +     struct rt6_info *rt6 = (struct rt6_info *)dst;
>>>>> +     u32 *pmetrics, table, fib6_flags;
>>>>>       struct nlmsghdr *nlh;
>>>>> +     struct rtmsg *rtm;
>>>>>       long expires = 0;
>>>>> -     u32 *pmetrics;
>>>>> -     u32 table;
>>>>>
>>>>>       nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
>>>>>       if (!nlh)
>>>>>               return -EMSGSIZE;
>>>>>
>>>>> +     if (rt6) {
>>>>> +             fib6_dst = &rt6->rt6i_dst;
>>>>> +             fib6_src = &rt6->rt6i_src;
>>>>> +             fib6_flags = rt6->rt6i_flags;
>>>>> +             fib6_prefsrc = &rt6->rt6i_prefsrc;
>>>>> +     } else {
>>>>> +             fib6_dst = &rt->fib6_dst;
>>>>> +             fib6_src = &rt->fib6_src;
>>>>> +             fib6_flags = rt->fib6_flags;
>>>>> +             fib6_prefsrc = &rt->fib6_prefsrc;
>>>>> +     }
>>>>
>>>> Unless I am missing something at the moment, an rt6_info can only have
>>>> the same dst, src and prefsrc as the fib6_info on which it is based.
>>>> Thus, only the flags is needed above. That simplifies this patch a lot.
>>> If dst, src and prefsrc in rt6_info are always the same as these in fib6_info,
>>> why do we need them in rt6_info? we could just get it by 'from'.
>>>
>>
>> I just sent a patch removing rt6i_prefsrc. It is set with only 1 reader
>> that can be converted.
>>
>> rt6i_src is checked against the fib6_info to invalidate a dst if the src
>> has changed, so a valid rt will always have the same rt6i_src as the
>> rt->from.
>>
>> rt6i_dst is set to the dest address / 128 in cases, so it should be used
>> for rt6_info cases above.
> So that means, I will use rt6i_dst and rt6i_flags when dst is set?
> how about I use rt6i_src there as well? just to make it look clear.
> and plus the gw/nh dump fix in rt6_fill_node():
> -        if (rt->fib6_nsiblings) {
> +        if (rt6) {
> +                if (fib6_flags & RTF_GATEWAY)
> +                        if (nla_put_in6_addr(skb, RTA_GATEWAY,
> +                                             &rt6->rt6i_gateway) < 0)
> +                                goto nla_put_failure;
> +
> +                if (dst->dev && nla_put_u32(skb, RTA_OIF, dst->dev->ifindex))
> +                        goto nla_put_failure;
> +        } else if (rt->fib6_nsiblings) {
>                  struct fib6_info *sibling, *next_sibling;
>                  struct nlattr *mp;
> 
> looks good to you?
> 

sure

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[Hangbin Liu]

On Mon, Sep 10, 2018 at 01:07:11PM -0600, David Ahern wrote:
> On 9/10/18 11:55 AM, Xin Long wrote:
> > On Tue, Sep 11, 2018 at 12:13 AM David Ahern <dsa@cumulusnetworks.com> wrote:
> >>
> >> On 9/9/18 12:29 AM, Xin Long wrote:
> >>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> >>>>> index 18e00ce..e554922 100644
> >>>>> --- a/net/ipv6/route.c
> >>>>> +++ b/net/ipv6/route.c
> >>>>> @@ -4670,20 +4670,33 @@ static int rt6_fill_node(struct net *net, struct sk_buff *skb,
> >>>>>                        int iif, int type, u32 portid, u32 seq,
> >>>>>                        unsigned int flags)
> >>>>>  {
> >>>>> -     struct rtmsg *rtm;
> >>>>> +     struct rt6key *fib6_prefsrc, *fib6_dst, *fib6_src;
> >>>>> +     struct rt6_info *rt6 = (struct rt6_info *)dst;
> >>>>> +     u32 *pmetrics, table, fib6_flags;
> >>>>>       struct nlmsghdr *nlh;
> >>>>> +     struct rtmsg *rtm;
> >>>>>       long expires = 0;
> >>>>> -     u32 *pmetrics;
> >>>>> -     u32 table;
> >>>>>
> >>>>>       nlh = nlmsg_put(skb, portid, seq, type, sizeof(*rtm), flags);
> >>>>>       if (!nlh)
> >>>>>               return -EMSGSIZE;
> >>>>>
> >>>>> +     if (rt6) {
> >>>>> +             fib6_dst = &rt6->rt6i_dst;
> >>>>> +             fib6_src = &rt6->rt6i_src;
> >>>>> +             fib6_flags = rt6->rt6i_flags;
> >>>>> +             fib6_prefsrc = &rt6->rt6i_prefsrc;
> >>>>> +     } else {
> >>>>> +             fib6_dst = &rt->fib6_dst;
> >>>>> +             fib6_src = &rt->fib6_src;
> >>>>> +             fib6_flags = rt->fib6_flags;
> >>>>> +             fib6_prefsrc = &rt->fib6_prefsrc;
> >>>>> +     }
> >>>>
> >>>> Unless I am missing something at the moment, an rt6_info can only have
> >>>> the same dst, src and prefsrc as the fib6_info on which it is based.
> >>>> Thus, only the flags is needed above. That simplifies this patch a lot.
> >>> If dst, src and prefsrc in rt6_info are always the same as these in fib6_info,
> >>> why do we need them in rt6_info? we could just get it by 'from'.
> >>>
> >>
> >> I just sent a patch removing rt6i_prefsrc. It is set with only 1 reader
> >> that can be converted.
> >>
> >> rt6i_src is checked against the fib6_info to invalidate a dst if the src
> >> has changed, so a valid rt will always have the same rt6i_src as the
> >> rt->from.
> >>
> >> rt6i_dst is set to the dest address / 128 in cases, so it should be used
> >> for rt6_info cases above.
> > So that means, I will use rt6i_dst and rt6i_flags when dst is set?
> > how about I use rt6i_src there as well? just to make it look clear.
> > and plus the gw/nh dump fix in rt6_fill_node():
> > -        if (rt->fib6_nsiblings) {
> > +        if (rt6) {
> > +                if (fib6_flags & RTF_GATEWAY)
> > +                        if (nla_put_in6_addr(skb, RTA_GATEWAY,
> > +                                             &rt6->rt6i_gateway) < 0)
> > +                                goto nla_put_failure;
> > +
> > +                if (dst->dev && nla_put_u32(skb, RTA_OIF, dst->dev->ifindex))
> > +                        goto nla_put_failure;
> > +        } else if (rt->fib6_nsiblings) {
> >                  struct fib6_info *sibling, *next_sibling;
> >                  struct nlattr *mp;
> > 
> > looks good to you?
> > 
> 
> sure

Hmm... Then how to deal the other nh info filled by rt6_nexthop_info()?
I was planed to fix the gw issue[1] by syncing the gw and flags info. Like

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 18e00ce..62621b4 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -998,6 +998,21 @@ static void ip6_rt_copy_init(struct rt6_info *rt, struct fib6_info *ort)
 	rt->rt6i_prefsrc = ort->fib6_prefsrc;
 }
 
+static void rt6_update_info(struct rt6_info *rt)
+{
+	struct fib6_info *from;
+
+	rcu_read_lock();
+	from = rcu_dereference(rt->from);
+	fib6_info_hold(from);
+	rcu_read_unlock();
+
+	from->fib6_flags = rt->rt6i_flags;
+	from->fib6_nh.nh_gw = rt->rt6i_gateway;
+
+	fib6_info_release(from);
+}
+
 static struct fib6_node* fib6_backtrack(struct fib6_node *fn,
 					struct in6_addr *saddr)
 {
@@ -3446,6 +3463,8 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu
 		goto out;
 	}
 
+	rt6_update_info(nrt);
+
 	netevent.old = &rt->dst;
 	netevent.new = &nrt->dst;
 	netevent.daddr = &msg->dest;
-- 
2.5.5


[1] https://patchwork.ozlabs.org/patch/966972/

Thanks
Hangbin

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[David Ahern]

On 9/10/18 7:04 PM, Hangbin Liu wrote:

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 18e00ce..62621b4 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -998,6 +998,21 @@ static void ip6_rt_copy_init(struct rt6_info *rt, struct fib6_info *ort)
>  	rt->rt6i_prefsrc = ort->fib6_prefsrc;
>  }
>  
> +static void rt6_update_info(struct rt6_info *rt)
> +{
> +	struct fib6_info *from;
> +
> +	rcu_read_lock();
> +	from = rcu_dereference(rt->from);
> +	fib6_info_hold(from);
> +	rcu_read_unlock();
> +
> +	from->fib6_flags = rt->rt6i_flags;
> +	from->fib6_nh.nh_gw = rt->rt6i_gateway;

As I mentioned on your last patch, redirects do *not* update fib
entries. Exceptions, yes, but not core data of a fib entry.

Re: [PATCH net] ipv6: use rt6_info members when dst is set in rt6_fill_node

[Hangbin Liu]

Hi David,
On Mon, Sep 10, 2018 at 08:39:34PM -0600, David Ahern wrote:
> On 9/10/18 7:04 PM, Hangbin Liu wrote:
> 
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index 18e00ce..62621b4 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -998,6 +998,21 @@ static void ip6_rt_copy_init(struct rt6_info *rt, struct fib6_info *ort)
> >  	rt->rt6i_prefsrc = ort->fib6_prefsrc;
> >  }
> >  
> > +static void rt6_update_info(struct rt6_info *rt)
> > +{
> > +	struct fib6_info *from;
> > +
> > +	rcu_read_lock();
> > +	from = rcu_dereference(rt->from);
> > +	fib6_info_hold(from);
> > +	rcu_read_unlock();
> > +
> > +	from->fib6_flags = rt->rt6i_flags;
> > +	from->fib6_nh.nh_gw = rt->rt6i_gateway;
> 
> As I mentioned on your last patch, redirects do *not* update fib
> entries. Exceptions, yes, but not core data of a fib entry.

Thanks for the comments, I understand that we should not update original route.
And here I know the redirect (should?) do not update fib entries.

So Xin Long's patch looks more reasonable.

Thanks
Hangbin