From: Duan Jiong <duanj.fnst@cn•fujitsu.com>
To: hannes@stressinduktion•org
Cc: davem@davemloft•net, netdev@vger•kernel.org
Subject: Re: [PATCH v4] ipv6:introduce function to find route for redirect
Date: Tue, 03 Sep 2013 13:37:19 +0800 [thread overview]
Message-ID: <5225758F.8080604@cn.fujitsu.com> (raw)
In-Reply-To: <20130902195056.GA5451@order.stressinduktion.org>
于 2013年09月03日 03:50, Hannes Frederic Sowa 写道:
> On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote:
>> +static struct rt6_info *__ip6_route_redirect(struct net *net,
>> + struct fib6_table *table,
>> + struct flowi6 *fl6,
>> + int flags)
>> +{
>> + struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>> + struct rt6_info *rt;
>> + struct fib6_node *fn;
>> +
>> + /* Get the "current" route for this destination and
>> + * check if the redirect has come from approriate router.
>> + *
>> + * RFC 4861 specifies that redirects should only be
>> + * accepted if they come from the nexthop to the target.
>> + * Due to the way the routes are chosen, this notion
>> + * is a bit fuzzy and one might need to check all possible
>> + * routes.
>> + */
>> +
>> + read_lock_bh(&table->tb6_lock);
>> + fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>> +restart:
>> + for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
>> + if (rt6_check_expired(rt))
>> + continue;
>> + if (rt->dst.error)
>> + continue;
>
> Sorry, I should have been more clear what I meant with failing early:
>
> I considered a setup like this:
>
> ip -6 r a default nexthop via fe80::1 dev eth0
> ip -6 r a prohibit 2002:1::/64
>
> If the kernel receives a redirect for a destination e.g. 2002:1::1 we
> would backtrack above the prohibit rule and return the dst of the default
> route and would insert a new cached route which could circumvent the
> prohibit rule. We have to try to lock down the tree below 2002:1::/64
> in that case. A possible solution for that would be to do something
> like this:
>
> /* We don't accept a redirect in case a more specific route is
> * installed with dst.error and stop backtracking.
> */
> if (rt->dst.error)
> break;
>
> Either we have to replace the rt with net->ipv6.ip6_null_entry in that
> case or check dst->error before calling rt6_do_redirect below.
>
Thanks for you comment, i understand what you mean.
>> + if (!(rt->rt6i_flags & RTF_GATEWAY))
>> + continue;
>> + if (fl6->flowi6_oif != rt->dst.dev->ifindex)
>> + continue;
>> + if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
>> + continue;
>> + break;
>> + }
>> +
>> + if (!rt)
>> + rt = net->ipv6.ip6_null_entry;
>> + BACKTRACK(net, &fl6->saddr);
>> +out:
>> + dst_hold(&rt->dst);
>> +
>> + read_unlock_bh(&table->tb6_lock);
>> +
>> + return rt;
>> +};
>>
>> [...]
>>
>> @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
>> fl6.saddr = iph->saddr;
>> fl6.flowlabel = ip6_flowinfo(iph);
>>
>> - dst = ip6_route_output(net, NULL, &fl6);
>> - if (!dst->error)
>> - rt6_do_redirect(dst, NULL, skb);
>> + dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
>> + rt6_do_redirect(dst, NULL, skb);
>> dst_release(dst);
>> }
>> EXPORT_SYMBOL_GPL(ip6_redirect);
>> @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
>> fl6.daddr = msg->dest;
>> fl6.saddr = iph->daddr;
>>
>> - dst = ip6_route_output(net, NULL, &fl6);
>> - if (!dst->error)
>> - rt6_do_redirect(dst, NULL, skb);
>> + dst = ip6_route_redirect(net, &fl6, &iph->saddr);
>> + rt6_do_redirect(dst, NULL, skb);
>> dst_release(dst);
>> }
>
> Btw. I still think it should be possible to eliminate
> ip6_redirect_no_header:
>
> We could always use ip6_redirect_no_header and use the data of the redirected
> header option just for finding the socket to be notified. We can do the whole
> verification and route updating in ndisc layer and then just call into icmpv6
> layer if upper protocols need a notification of the redirect. But that should
> go into another patch. ;)
>
I think this is good, but i have a question below:
if the socket type is connection-based, the dst information is stored in related
sock struct, so there is no need to look up the route for redirect in ip6_redirect
or ip6_redirect_no_header, in this case, we do the verification and route
updating in the upper protocols' err_handler is better.
How do you think of this?
Thanks,
Duan
next prev parent reply other threads:[~2013-09-03 5:37 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-02 6:14 [PATCH v4] ipv6:introduce function to find route for redirect Duan Jiong
2013-09-02 19:50 ` Hannes Frederic Sowa
2013-09-03 5:37 ` Duan Jiong [this message]
2013-09-03 19:17 ` Hannes Frederic Sowa
2013-09-04 12:06 ` Duan Jiong
2013-09-08 18:00 ` Hannes Frederic Sowa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5225758F.8080604@cn.fujitsu.com \
--to=duanj.fnst@cn$(echo .)fujitsu.com \
--cc=davem@davemloft$(echo .)net \
--cc=hannes@stressinduktion$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox