netdev - Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not null entry.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170728110442.GF5465@leo.usersys.redhat.com>
Date:   Fri, 28 Jul 2017 19:04:42 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     David Ahern <dsahern@...il.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>,
        network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] ipv6: no need to return rt->dst.error if it is not
 null entry.

On Thu, Jul 27, 2017 at 09:56:08PM -0700, Cong Wang wrote:
> On Wed, Jul 26, 2017 at 11:49 AM, David Ahern <dsahern@...il.com> wrote:
> > On 7/26/17 12:27 PM, Roopa Prabhu wrote:
> >> agreed...so looks like the check in v3 should be
> >>
> >>
> >> +       if ( rt == net->ipv6.ip6_null_entry ||
> >> +            (rt->dst.error &&
> >> + #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> >> +              rt != net->ipv6.ip6_prohibit_entry &&
> >> +              rt != net->ipv6.ip6_blk_hole_entry &&
> >> +#endif
> >> +             )) {
> >>                 err = rt->dst.error;
> >>                 ip6_rt_put(rt);
> >>                 goto errout;
> >>
> >
> > I don't think so. If I add a prohibit route and use the fibmatch
> > attribute, I want to see the route from the FIB that was matched.
> 
> But net->ipv6.ip6_prohibit_entry is not the prohibit route you can
> add in user-space, it is only used by rule actions. So do you really
> want to dump it?? My gut feeling is no, but I am definitely not sure.
> 
> When you add a prohibit route, a new rt is allocated dynamically,
> net->ipv6.ip6_prohibit_entry is relatively static, internal and is the
> only one per netns. (Same for net->ipv6.ip6_blk_hole_entry)
> 
> I think Hangbin's example doesn't have ip rules, so this case
> is not shown up.

I mixed the rule entry and route entry these days. And with your help I can
separate them now.

When first time I find the rt->dst.error return directly issue, I was testing
ip rule actually.

e.g.
+ ip netns exec client ip -6 rule add to 2003::1/64 table 100 unreachable
+ ip netns exec server ip -6 rule add to 2001::1/64 table 100 prohibit
+ ip netns exec client ip -6 route get 2003::1
RTNETLINK answers: Network is unreachable
+ ip netns exec client ip -6 route get 2001::1
RTNETLINK answers: Permission denied

After check I thought we returned net->ipv6.ip6_null_entry /
net->ipv6.ip6_blk_hole_entry in function fib6_rule_action().

That's the reason I want to delete both rt->dst.error and
net->ipv6.ip6_null_entry check in patch v2 and v3.

Then with David's comments I realise we also need to take care about ip route
entrys.

my last mail's comment:
> Thanks for your explains. Now I know where I made the mistake. I mis-looked
> FR_ACT_UNREACHABLE to RTN_UNREACHABLE and thought we return rt =
> net->ipv6.ip6_null_entry in fib6_rule_action().

But then I fall in to the code logic and get lost... And thought
FR_ACT_UNREACHABLE and RTN_UNREACHABLE are not same. Today I re-check the
code and realise RTN_UNREACHABLE is defined in user space and FR_ACT_UNREACHABLE
is in kernel. Actually they are the same.

So after PATCH v4, we fixed the route side. And part of ip rule(prohibit and
blk hole). I will think over of this.

Thanks
Hangbin