* [PATCH net v3] ipv6: fix multipath route replace error recovery
@ 2015-09-06 20:46 Roopa Prabhu
2015-09-07 12:37 ` Nicolas Dichtel
2015-09-08 0:42 ` roopa
0 siblings, 2 replies; 6+ messages in thread
From: Roopa Prabhu @ 2015-09-06 20:46 UTC (permalink / raw)
To: davem; +Cc: mkubecek, Mazziesaccount, hannes, kuznet, jmorris, yoshfuji,
netdev
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Problem:
The ecmp route replace support for ipv6 in the kernel, deletes the
existing ecmp route too early, ie when it installs the first nexthop.
If there is an error in installing the subsequent nexthops, its too late
to recover the already deleted existing route
This patch fixes the problem with the following:
a) Changes the existing multipath route add code to a two stage process:
build rt6_infos + insert them
ip6_route_add rt6_info creation code is moved into
ip6_route_info_create.
b) This ensures that all errors are caught during building rt6_infos
and we fail early
c) Separates multipath add and del code. Because add needs the special
two stage mode in a) and delete essentially does not care.
d) In any event if the code fails during inserting a route again, a
warning is printed (This should be unlikely)
Before the patch:
$ip -6 route show
3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
/* Try replacing the route with a duplicate nexthop */
$ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
RTNETLINK answers: File exists
$ip -6 route show
/* previously added ecmp route 3000:1000:1000:1000::2 dissappears from
* kernel */
After the patch:
$ip -6 route show
3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
/* Try replacing the route with a duplicate nexthop */
$ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
RTNETLINK answers: File exists
$ip -6 route show
3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
v1 - v2 : fix leak
v2 - v3: fix 'Fixes' tag and warn msg (feedback from nicolas)
resending against net
net/ipv6/route.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 175 insertions(+), 26 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f45cac6..8da1269 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1748,7 +1748,7 @@ static int ip6_convert_metrics(struct mx6_config *mxc,
return -EINVAL;
}
-int ip6_route_add(struct fib6_config *cfg)
+int ip6_route_info_create(struct fib6_config *cfg, struct rt6_info **rt_ret)
{
int err;
struct net *net = cfg->fc_nlinfo.nl_net;
@@ -1756,7 +1756,6 @@ int ip6_route_add(struct fib6_config *cfg)
struct net_device *dev = NULL;
struct inet6_dev *idev = NULL;
struct fib6_table *table;
- struct mx6_config mxc = { .mx = NULL, };
int addr_type;
if (cfg->fc_dst_len > 128 || cfg->fc_src_len > 128)
@@ -1981,6 +1980,32 @@ install_route:
cfg->fc_nlinfo.nl_net = dev_net(dev);
+ *rt_ret = rt;
+
+ return 0;
+out:
+ if (dev)
+ dev_put(dev);
+ if (idev)
+ in6_dev_put(idev);
+ if (rt)
+ dst_free(&rt->dst);
+
+ *rt_ret = NULL;
+
+ return err;
+}
+
+int ip6_route_add(struct fib6_config *cfg)
+{
+ struct mx6_config mxc = { .mx = NULL, };
+ struct rt6_info *rt = NULL;
+ int err;
+
+ err = ip6_route_info_create(cfg, &rt);
+ if (err)
+ goto out;
+
err = ip6_convert_metrics(&mxc, cfg);
if (err)
goto out;
@@ -1988,14 +2013,12 @@ install_route:
err = __ip6_ins_rt(rt, &cfg->fc_nlinfo, &mxc);
kfree(mxc.mx);
+
return err;
out:
- if (dev)
- dev_put(dev);
- if (idev)
- in6_dev_put(idev);
if (rt)
dst_free(&rt->dst);
+
return err;
}
@@ -2776,19 +2799,78 @@ errout:
return err;
}
-static int ip6_route_multipath(struct fib6_config *cfg, int add)
+struct rt6_nh {
+ struct rt6_info *rt6_info;
+ struct fib6_config r_cfg;
+ struct mx6_config mxc;
+ struct list_head next;
+};
+
+static void ip6_print_replace_route_err(struct list_head *rt6_nh_list)
+{
+ struct rt6_nh *nh;
+
+ list_for_each_entry(nh, rt6_nh_list, next) {
+ pr_warn("IPV6: replace premature del %pI6 nexthop %pI6 ifi %d\n",
+ &nh->r_cfg.fc_dst, &nh->r_cfg.fc_gateway,
+ nh->r_cfg.fc_ifindex);
+ }
+}
+
+static int ip6_route_info_append(struct list_head *rt6_nh_list,
+ struct rt6_info *rt, struct fib6_config *r_cfg)
+{
+ struct rt6_nh *nh;
+ struct rt6_info *rtnh;
+ int err = -EEXIST;
+
+ list_for_each_entry(nh, rt6_nh_list, next) {
+ /* check if rt6_info already exists */
+ rtnh = nh->rt6_info;
+
+ if (rtnh->dst.dev == rt->dst.dev &&
+ rtnh->rt6i_idev == rt->rt6i_idev &&
+ ipv6_addr_equal(&rtnh->rt6i_gateway,
+ &rt->rt6i_gateway))
+ return err;
+ }
+
+ nh = kzalloc(sizeof(*nh), GFP_KERNEL);
+ if (!nh)
+ return -ENOMEM;
+ nh->rt6_info = rt;
+ err = ip6_convert_metrics(&nh->mxc, r_cfg);
+ if (err) {
+ kfree(nh);
+ return err;
+ }
+ memcpy(&nh->r_cfg, r_cfg, sizeof(*r_cfg));
+ list_add_tail(&nh->next, rt6_nh_list);
+
+ return 0;
+}
+
+static int ip6_route_multipath_add(struct fib6_config *cfg)
{
struct fib6_config r_cfg;
struct rtnexthop *rtnh;
+ struct rt6_info *rt;
+ struct rt6_nh *err_nh;
+ struct rt6_nh *nh, *nh_safe;
int remaining;
int attrlen;
- int err = 0, last_err = 0;
+ int err = 1;
+ int nhn = 0;
+ int replace = (cfg->fc_nlinfo.nlh &&
+ (cfg->fc_nlinfo.nlh->nlmsg_flags & NLM_F_REPLACE));
+ LIST_HEAD(rt6_nh_list);
remaining = cfg->fc_mp_len;
-beginning:
rtnh = (struct rtnexthop *)cfg->fc_mp;
- /* Parse a Multipath Entry */
+ /* Parse a Multipath Entry and build a list (rt6_nh_list) of
+ * rt6_info structs per nexthop
+ */
while (rtnh_ok(rtnh, remaining)) {
memcpy(&r_cfg, cfg, sizeof(*cfg));
if (rtnh->rtnh_ifindex)
@@ -2808,22 +2890,32 @@ beginning:
if (nla)
r_cfg.fc_encap_type = nla_get_u16(nla);
}
- err = add ? ip6_route_add(&r_cfg) : ip6_route_del(&r_cfg);
+
+ err = ip6_route_info_create(&r_cfg, &rt);
+ if (err)
+ goto cleanup;
+
+ err = ip6_route_info_append(&rt6_nh_list, rt, &r_cfg);
if (err) {
- last_err = err;
- /* If we are trying to remove a route, do not stop the
- * loop when ip6_route_del() fails (because next hop is
- * already gone), we should try to remove all next hops.
- */
- if (add) {
- /* If add fails, we should try to delete all
- * next hops that have been already added.
- */
- add = 0;
- remaining = cfg->fc_mp_len - remaining;
- goto beginning;
- }
+ dst_free(&rt->dst);
+ goto cleanup;
+ }
+
+ rtnh = rtnh_next(rtnh, &remaining);
+ }
+
+ err_nh = NULL;
+ list_for_each_entry(nh, &rt6_nh_list, next) {
+ err = __ip6_ins_rt(nh->rt6_info, &cfg->fc_nlinfo, &nh->mxc);
+ /* nh->rt6_info is used or freed at this point, reset to NULL*/
+ nh->rt6_info = NULL;
+ if (err) {
+ if (replace && nhn)
+ ip6_print_replace_route_err(&rt6_nh_list);
+ err_nh = nh;
+ goto add_errout;
}
+
/* Because each route is added like a single route we remove
* these flags after the first nexthop: if there is a collision,
* we have already failed to add the first nexthop:
@@ -2833,6 +2925,63 @@ beginning:
*/
cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL |
NLM_F_REPLACE);
+ nhn++;
+ }
+
+ goto cleanup;
+
+add_errout:
+ /* Delete routes that were already added */
+ list_for_each_entry(nh, &rt6_nh_list, next) {
+ if (err_nh == nh)
+ break;
+ ip6_route_del(&nh->r_cfg);
+ }
+
+cleanup:
+ list_for_each_entry_safe(nh, nh_safe, &rt6_nh_list, next) {
+ if (nh->rt6_info)
+ dst_free(&nh->rt6_info->dst);
+ if (nh->mxc.mx)
+ kfree(nh->mxc.mx);
+ list_del(&nh->next);
+ kfree(nh);
+ }
+
+ return err;
+}
+
+static int ip6_route_multipath_del(struct fib6_config *cfg)
+{
+ struct fib6_config r_cfg;
+ struct rtnexthop *rtnh;
+ int remaining;
+ int attrlen;
+ int err = 1, last_err = 0;
+
+ remaining = cfg->fc_mp_len;
+ rtnh = (struct rtnexthop *)cfg->fc_mp;
+
+ /* Parse a Multipath Entry */
+ while (rtnh_ok(rtnh, remaining)) {
+ memcpy(&r_cfg, cfg, sizeof(*cfg));
+ if (rtnh->rtnh_ifindex)
+ r_cfg.fc_ifindex = rtnh->rtnh_ifindex;
+
+ attrlen = rtnh_attrlen(rtnh);
+ if (attrlen > 0) {
+ struct nlattr *nla, *attrs = rtnh_attrs(rtnh);
+
+ nla = nla_find(attrs, attrlen, RTA_GATEWAY);
+ if (nla) {
+ nla_memcpy(&r_cfg.fc_gateway, nla, 16);
+ r_cfg.fc_flags |= RTF_GATEWAY;
+ }
+ }
+ err = ip6_route_del(&r_cfg);
+ if (err)
+ last_err = err;
+
rtnh = rtnh_next(rtnh, &remaining);
}
@@ -2849,7 +2998,7 @@ static int inet6_rtm_delroute(struct sk_buff *skb, struct nlmsghdr *nlh)
return err;
if (cfg.fc_mp)
- return ip6_route_multipath(&cfg, 0);
+ return ip6_route_multipath_del(&cfg);
else
return ip6_route_del(&cfg);
}
@@ -2864,7 +3013,7 @@ static int inet6_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh)
return err;
if (cfg.fc_mp)
- return ip6_route_multipath(&cfg, 1);
+ return ip6_route_multipath_add(&cfg);
else
return ip6_route_add(&cfg);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v3] ipv6: fix multipath route replace error recovery
2015-09-06 20:46 [PATCH net v3] ipv6: fix multipath route replace error recovery Roopa Prabhu
@ 2015-09-07 12:37 ` Nicolas Dichtel
2015-09-08 0:14 ` roopa
2015-09-08 0:42 ` roopa
1 sibling, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2015-09-07 12:37 UTC (permalink / raw)
To: Roopa Prabhu, davem
Cc: mkubecek, Mazziesaccount, hannes, kuznet, jmorris, yoshfuji,
netdev
Le 06/09/2015 22:46, Roopa Prabhu a écrit :
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
I've sent you some comments about the v2, so please keep me in CC for the next
versions.
>
> Problem:
> The ecmp route replace support for ipv6 in the kernel, deletes the
> existing ecmp route too early, ie when it installs the first nexthop.
> If there is an error in installing the subsequent nexthops, its too late
> to recover the already deleted existing route
>
> This patch fixes the problem with the following:
It does not really 'fix' the problem, it only reduces the probability to have
an error. This is really different. The status is much better after this patch,
but it could be good to reword a bit the commitlog to reflect that.
> a) Changes the existing multipath route add code to a two stage process:
> build rt6_infos + insert them
> ip6_route_add rt6_info creation code is moved into
> ip6_route_info_create.
> b) This ensures that all errors are caught during building rt6_infos
> and we fail early
> c) Separates multipath add and del code. Because add needs the special
> two stage mode in a) and delete essentially does not care.
> d) In any event if the code fails during inserting a route again, a
> warning is printed (This should be unlikely)
>
> Before the patch:
> $ip -6 route show
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>
> /* Try replacing the route with a duplicate nexthop */
> $ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
> fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
> swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
> RTNETLINK answers: File exists
>
> $ip -6 route show
> /* previously added ecmp route 3000:1000:1000:1000::2 dissappears from
> * kernel */
>
> After the patch:
> $ip -6 route show
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>
> /* Try replacing the route with a duplicate nexthop */
> $ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
> fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
> swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
> RTNETLINK answers: File exists
>
> $ip -6 route show
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
As said in the v2 thread, I still don't agree with this tag.
[snip]
> +static void ip6_print_replace_route_err(struct list_head *rt6_nh_list)
> +{
> + struct rt6_nh *nh;
> +
> + list_for_each_entry(nh, rt6_nh_list, next) {
> + pr_warn("IPV6: replace premature del %pI6 nexthop %pI6 ifi %d\n",
I don't think that a user (who didn't read the code) can understand this
sentence. Another suggestion:
"ECMPv6: route replacement failed (check the consistency of the installed
route)". Not sure that the nexthops should be listed after.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3] ipv6: fix multipath route replace error recovery
2015-09-07 12:37 ` Nicolas Dichtel
@ 2015-09-08 0:14 ` roopa
0 siblings, 0 replies; 6+ messages in thread
From: roopa @ 2015-09-08 0:14 UTC (permalink / raw)
To: nicolas.dichtel
Cc: davem, mkubecek, Mazziesaccount, hannes, kuznet, jmorris,
yoshfuji, netdev
On 9/7/15, 5:37 AM, Nicolas Dichtel wrote:
> Le 06/09/2015 22:46, Roopa Prabhu a écrit :
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> I've sent you some comments about the v2, so please keep me in CC for
> the next
> versions.
>
>>
>> Problem:
>> The ecmp route replace support for ipv6 in the kernel, deletes the
>> existing ecmp route too early, ie when it installs the first nexthop.
>> If there is an error in installing the subsequent nexthops, its too late
>> to recover the already deleted existing route
>>
>> This patch fixes the problem with the following:
> It does not really 'fix' the problem, it only reduces the probability
> to have
> an error. This is really different. The status is much better after
> this patch,
> but it could be good to reword a bit the commitlog to reflect that.
sure.
>
>> a) Changes the existing multipath route add code to a two stage process:
>> build rt6_infos + insert them
>> ip6_route_add rt6_info creation code is moved into
>> ip6_route_info_create.
>> b) This ensures that all errors are caught during building rt6_infos
>> and we fail early
>> c) Separates multipath add and del code. Because add needs the special
>> two stage mode in a) and delete essentially does not care.
>> d) In any event if the code fails during inserting a route again, a
>> warning is printed (This should be unlikely)
>>
>> Before the patch:
>> $ip -6 route show
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>>
>> /* Try replacing the route with a duplicate nexthop */
>> $ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
>> fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
>> swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
>> RTNETLINK answers: File exists
>>
>> $ip -6 route show
>> /* previously added ecmp route 3000:1000:1000:1000::2 dissappears from
>> * kernel */
>>
>> After the patch:
>> $ip -6 route show
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>>
>> /* Try replacing the route with a duplicate nexthop */
>> $ip -6 route change 3000:1000:1000:1000::2/128 nexthop via
>> fe80::202:ff:fe00:b dev swp49s0 nexthop via fe80::202:ff:fe00:d dev
>> swp49s1 nexthop via fe80::202:ff:fe00:d dev swp49s1
>> RTNETLINK answers: File exists
>>
>> $ip -6 route show
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:b dev swp49s0 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:d dev swp49s1 metric 1024
>> 3000:1000:1000:1000::2 via fe80::202:ff:fe00:f dev swp49s2 metric 1024
>>
>> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> As said in the v2 thread, I still don't agree with this tag.
>
> [snip]
>> +static void ip6_print_replace_route_err(struct list_head *rt6_nh_list)
>> +{
>> + struct rt6_nh *nh;
>> +
>> + list_for_each_entry(nh, rt6_nh_list, next) {
>> + pr_warn("IPV6: replace premature del %pI6 nexthop %pI6 ifi
>> %d\n",
> I don't think that a user (who didn't read the code) can understand this
> sentence. Another suggestion:
> "ECMPv6: route replacement failed (check the consistency of the
> installed route)". Not sure that the nexthops should be listed after.
sure, i don't have a preference. I will resubmit if we converge on the
commit message.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3] ipv6: fix multipath route replace error recovery
2015-09-06 20:46 [PATCH net v3] ipv6: fix multipath route replace error recovery Roopa Prabhu
2015-09-07 12:37 ` Nicolas Dichtel
@ 2015-09-08 0:42 ` roopa
2015-09-08 9:55 ` Nicolas Dichtel
1 sibling, 1 reply; 6+ messages in thread
From: roopa @ 2015-09-08 0:42 UTC (permalink / raw)
To: davem
Cc: mkubecek, Mazziesaccount, hannes, kuznet, jmorris, yoshfuji,
netdev, Michal Kubecek, Nicolas Dichtel
On 9/6/15, 1:46 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> Problem:
> The ecmp route replace support for ipv6 in the kernel, deletes the
> existing ecmp route too early, ie when it installs the first nexthop.
> If there is an error in installing the subsequent nexthops, its too late
> to recover the already deleted existing route
>
> This patch fixes the problem with the following:
> a) Changes the existing multipath route add code to a two stage process:
> build rt6_infos + insert them
> ip6_route_add rt6_info creation code is moved into
> ip6_route_info_create.
> b) This ensures that all errors are caught during building rt6_infos
> and we fail early
>
The other way I have been thinking of solving the problem is to mark the
sibling routes being replaced with some state
...so they can be restored on error. Still figuring out a way to do this
in a clean and non-intrusive way.
Or maybe just save the sibling routes (rt6_infos) being replaced in a
list and re-insert them on error.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3] ipv6: fix multipath route replace error recovery
2015-09-08 0:42 ` roopa
@ 2015-09-08 9:55 ` Nicolas Dichtel
2015-09-08 13:53 ` roopa
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Dichtel @ 2015-09-08 9:55 UTC (permalink / raw)
To: roopa, davem
Cc: mkubecek, Mazziesaccount, hannes, kuznet, jmorris, yoshfuji,
netdev
Le 08/09/2015 02:42, roopa a écrit :
> On 9/6/15, 1:46 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> Problem:
>> The ecmp route replace support for ipv6 in the kernel, deletes the
>> existing ecmp route too early, ie when it installs the first nexthop.
>> If there is an error in installing the subsequent nexthops, its too late
>> to recover the already deleted existing route
>>
>> This patch fixes the problem with the following:
>> a) Changes the existing multipath route add code to a two stage process:
>> build rt6_infos + insert them
>> ip6_route_add rt6_info creation code is moved into
>> ip6_route_info_create.
>> b) This ensures that all errors are caught during building rt6_infos
>> and we fail early
>>
> The other way I have been thinking of solving the problem is to mark the sibling
> routes being replaced with some state
> ...so they can be restored on error. Still figuring out a way to do this in a
> clean and non-intrusive way.
If I'm not wrong, the only error which may result to an inconsistent list of
nexthops is ENOMEM (after your patch). I'm not sure it's worth to add too much
complexity to the code to handle this error.
> Or maybe just save the sibling routes (rt6_infos) being replaced in a list and
> re-insert them on error.
Yes, but we can also fail to re-insert the route.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v3] ipv6: fix multipath route replace error recovery
2015-09-08 9:55 ` Nicolas Dichtel
@ 2015-09-08 13:53 ` roopa
0 siblings, 0 replies; 6+ messages in thread
From: roopa @ 2015-09-08 13:53 UTC (permalink / raw)
To: nicolas.dichtel
Cc: davem, mkubecek, Mazziesaccount, hannes, kuznet, jmorris,
yoshfuji, netdev
On 9/8/15, 2:55 AM, Nicolas Dichtel wrote:
> Le 08/09/2015 02:42, roopa a écrit :
>> On 9/6/15, 1:46 PM, Roopa Prabhu wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>
>>> Problem:
>>> The ecmp route replace support for ipv6 in the kernel, deletes the
>>> existing ecmp route too early, ie when it installs the first nexthop.
>>> If there is an error in installing the subsequent nexthops, its too
>>> late
>>> to recover the already deleted existing route
>>>
>>> This patch fixes the problem with the following:
>>> a) Changes the existing multipath route add code to a two stage
>>> process:
>>> build rt6_infos + insert them
>>> ip6_route_add rt6_info creation code is moved into
>>> ip6_route_info_create.
>>> b) This ensures that all errors are caught during building rt6_infos
>>> and we fail early
>>>
>> The other way I have been thinking of solving the problem is to mark
>> the sibling
>> routes being replaced with some state
>> ...so they can be restored on error. Still figuring out a way to do
>> this in a
>> clean and non-intrusive way.
> If I'm not wrong, the only error which may result to an inconsistent
> list of
> nexthops is ENOMEM (after your patch). I'm not sure it's worth to add
> too much
> complexity to the code to handle this error.
yes, agreed. And that's the reason i went down the path presented in
the patch in context.
I was just reflecting back on the other possible implementations. thanks
for the review.
>
>> Or maybe just save the sibling routes (rt6_infos) being replaced in
>> a list and
>> re-insert them on error.
> Yes, but we can also fail to re-insert the route.
ack.
posting v4 soon.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-08 13:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-06 20:46 [PATCH net v3] ipv6: fix multipath route replace error recovery Roopa Prabhu
2015-09-07 12:37 ` Nicolas Dichtel
2015-09-08 0:14 ` roopa
2015-09-08 0:42 ` roopa
2015-09-08 9:55 ` Nicolas Dichtel
2015-09-08 13:53 ` roopa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).