| Message ID | CAPwn2JTF+j84+Bv4Zn=pqju2tnMuuJrhTr9Un786-PZ0D6af3w@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 7/20/17 9:23 AM, Hangbin Liu wrote: > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index 4d30c96..c290aa4 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -3637,13 +3637,8 @@ static int inet6_rtm_getroute(struct sk_buff > *in_skb, struct nlmsghdr *nlh, > dst = ip6_route_lookup(net, &fl6, 0); > > rt = container_of(dst, struct rt6_info, dst); > - if (rt->dst.error) { > - err = rt->dst.error; > - ip6_rt_put(rt); > - goto errout; > - } The above 5 lines introduced the change in behavior, so removing them should be sufficient.
On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote: > 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>: >>> +++ b/net/ipv6/route.c >>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, >>> dst = ip6_route_lookup(net, &fl6, 0); >>> >>> rt = container_of(dst, struct rt6_info, dst); >>> - if (rt->dst.error) { >>> - err = rt->dst.error; >>> - ip6_rt_put(rt); >>> - goto errout; >>> - } >> >> hmm... or instead of remove this check, should we check all the entry? Like >> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt != > ^^ mistake here >> net->ipv6.ip6_blk_hole_entry) || >> rt == net->ipv6.ip6_null_entry ) > > Sorry, there should be no need to check ip6_null_entry since the > error is already > -ENETUNREACH. So how about Hmm? All of these 3 entries have error set, right?? So we should only check dst.error...
On Fri, Jul 21, 2017 at 11:42 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote: >> 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>: >>>> +++ b/net/ipv6/route.c >>>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, >>>> dst = ip6_route_lookup(net, &fl6, 0); >>>> >>>> rt = container_of(dst, struct rt6_info, dst); >>>> - if (rt->dst.error) { >>>> - err = rt->dst.error; >>>> - ip6_rt_put(rt); >>>> - goto errout; >>>> - } >>> >>> hmm... or instead of remove this check, should we check all the entry? Like >>> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt != >> ^^ mistake here >>> net->ipv6.ip6_blk_hole_entry) || >>> rt == net->ipv6.ip6_null_entry ) >> >> Sorry, there should be no need to check ip6_null_entry since the >> error is already >> -ENETUNREACH. So how about > > Hmm? All of these 3 entries have error set, right?? > So we should only check dst.error... removing the check seems ok to me. We can also make the check conditional to fibmatch code only to eliminate any change in behavior introduced by fibmatch. ie if (fibmatch && rt->dst.error). Hangbin, can you also pls check that fibmatch works ok for such routes with your patch applied ?. ip netns exec client ip -6 route get fibmatch 2003::1 (with latest iproute2) thank you.
On Fri, Jul 21, 2017 at 2:53 PM, Roopa Prabhu <roopa@cumulusnetworks.com> wrote: > On Fri, Jul 21, 2017 at 11:42 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote: >> On Thu, Jul 20, 2017 at 8:23 AM, Hangbin Liu <liuhangbin@gmail.com> wrote: >>> 2017-07-20 23:06 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>: >>>>> +++ b/net/ipv6/route.c >>>>> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, >>>>> dst = ip6_route_lookup(net, &fl6, 0); >>>>> >>>>> rt = container_of(dst, struct rt6_info, dst); >>>>> - if (rt->dst.error) { >>>>> - err = rt->dst.error; >>>>> - ip6_rt_put(rt); >>>>> - goto errout; >>>>> - } >>>> >>>> hmm... or instead of remove this check, should we check all the entry? Like >>>> if ((rt->dst.error && rt != net->ipv6.ip6_null_entry && rt != >>> ^^ mistake here >>>> net->ipv6.ip6_blk_hole_entry) || >>>> rt == net->ipv6.ip6_null_entry ) >>> >>> Sorry, there should be no need to check ip6_null_entry since the >>> error is already >>> -ENETUNREACH. So how about >> >> Hmm? All of these 3 entries have error set, right?? >> So we should only check dst.error... > > removing the check seems ok to me. We can also make the check > conditional to fibmatch code only > to eliminate any change in behavior introduced by fibmatch. > > ie if (fibmatch && rt->dst.error). > > Hangbin, can you also pls check that fibmatch works ok for such routes > with your patch applied ?. > > ip netns exec client ip -6 route get fibmatch 2003::1 (with latest iproute2) > > thank you. I tried this with your patch and this works for the fibmatch case as well.
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 4d30c96..c290aa4 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -3637,13 +3637,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, dst = ip6_route_lookup(net, &fl6, 0); rt = container_of(dst, struct rt6_info, dst); - if (rt->dst.error) { - err = rt->dst.error; - ip6_rt_put(rt); - goto errout; - } - - if (rt == net->ipv6.ip6_null_entry) { + if (rt->dst.error && rt != net->ipv6.ip6_prohibit_entry && + rt != net->ipv6.ip6_blk_hole_entry) { err = rt->dst.error; ip6_rt_put(rt);