[PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[Li Wei]

When opt->srr_is_hit is set skb_rtable(skb) has been updated for
'nexthop' and iph->daddr should always equals to skb_rtable->rt_dst
holds, We need update iph->daddr either.

Signed-off-by: Li Wei <lw@cn.fujitsu.com>
---
 net/ipv4/ip_options.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ec93335..05d20cc 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -640,6 +640,7 @@ int ip_options_rcv_srr(struct sk_buff *skb)
 	}
 	if (srrptr <= srrspace) {
 		opt->srr_is_hit = 1;
+		iph->daddr = nexthop;
 		opt->is_changed = 1;
 	}
 	return 0;
-- 
1.7.3.2

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[David Miller]

From: Li Wei <lw@cn.fujitsu.com>
Date: Wed, 09 Nov 2011 15:39:28 +0800

> When opt->srr_is_hit is set skb_rtable(skb) has been updated for
> 'nexthop' and iph->daddr should always equals to skb_rtable->rt_dst
> holds, We need update iph->daddr either.
> 
> Signed-off-by: Li Wei <lw@cn.fujitsu.com>

Applied, thank you.

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[Li Wei]

> From: Li Wei <lw@cn.fujitsu.com>
> Date: Wed, 09 Nov 2011 15:39:28 +0800
> 
>> When opt->srr_is_hit is set skb_rtable(skb) has been updated for
>> 'nexthop' and iph->daddr should always equals to skb_rtable->rt_dst
>> holds, We need update iph->daddr either.
>>
>> Signed-off-by: Li Wei <lw@cn.fujitsu.com>
> 
> Applied, thank you.
> 
> 

Hi, David

These days i am doing some ICMP tests and sadly found that wen can't update
iph->daddr in ip_options_rcv_srr(), It's too early. When some exception
ocurred later (eg. in ip_forward() when goto sr_failed) we need the ip header
be identical to the original one as ICMP need it.

It seems we need rt->rt_dst in ip_forward_options() and ip_forward() instead
of iph->daddr.

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[David Miller]

From: Li Wei <lw@cn.fujitsu.com>
Date: Tue, 15 Nov 2011 13:40:45 +0800

> These days i am doing some ICMP tests and sadly found that wen can't update
> iph->daddr in ip_options_rcv_srr(), It's too early. When some exception
> ocurred later (eg. in ip_forward() when goto sr_failed) we need the ip header
> be identical to the original one as ICMP need it.
> 
> It seems we need rt->rt_dst in ip_forward_options() and ip_forward() instead
> of iph->daddr.

An alternative is to store the calculated destination address somewhere
in the IPCB(skb).

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[Li Wei]

> From: Li Wei <lw@cn.fujitsu.com>
> Date: Tue, 15 Nov 2011 13:40:45 +0800
> 
>> These days i am doing some ICMP tests and sadly found that wen can't update
>> iph->daddr in ip_options_rcv_srr(), It's too early. When some exception
>> ocurred later (eg. in ip_forward() when goto sr_failed) we need the ip header
>> be identical to the original one as ICMP need it.
>>
>> It seems we need rt->rt_dst in ip_forward_options() and ip_forward() instead
>> of iph->daddr.
> 
> An alternative is to store the calculated destination address somewhere
> in the IPCB(skb).
> 
> 

You mean modify struct inet_skb_parm add some field to store the calculated 
destination address?

My question, in ip_forward() and ip_forward_options() rt is there, why not
we just use it?

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[David Miller]

From: Li Wei <lw@cn.fujitsu.com>
Date: Tue, 15 Nov 2011 15:26:31 +0800

> My question, in ip_forward() and ip_forward_options() rt is there, why not
> we just use it?

Because I'm trying to eliminate all accesses to rt->rt_dst because
when I remove the routing cache, routes will be shared.  One 'rt'
will refer to many specific destination addresses.

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[Li Wei]

> From: Li Wei <lw@cn.fujitsu.com>
> Date: Tue, 15 Nov 2011 15:26:31 +0800
> 
>> My question, in ip_forward() and ip_forward_options() rt is there, why not
>> we just use it?
> 
> Because I'm trying to eliminate all accesses to rt->rt_dst because
> when I remove the routing cache, routes will be shared.  One 'rt'
> will refer to many specific destination addresses.
> 
> 
Sorry, I can't understand what you said, can you make it more clear?

I think in ip_forward(), we need 'rt' to forward skb, until this skb has been 
forwarded we hold a reference to it in skb, We can use rt->rt_dst safely, It
is an ordinary __be32 not a reference.

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[David Miller]

From: Li Wei <lw@cn.fujitsu.com>
Date: Tue, 15 Nov 2011 15:51:29 +0800

>> From: Li Wei <lw@cn.fujitsu.com>
>> Date: Tue, 15 Nov 2011 15:26:31 +0800
>> 
>>> My question, in ip_forward() and ip_forward_options() rt is there, why not
>>> we just use it?
>> 
>> Because I'm trying to eliminate all accesses to rt->rt_dst because
>> when I remove the routing cache, routes will be shared.  One 'rt'
>> will refer to many specific destination addresses.
>> 
>> 
> Sorry, I can't understand what you said, can you make it more clear?
> 
> I think in ip_forward(), we need 'rt' to forward skb, until this skb has been 
> forwarded we hold a reference to it in skb, We can use rt->rt_dst safely, It
> is an ordinary __be32 not a reference.

My long term plan is to eliminate cached routes, we will directly use
routing table entries instead.

In such a scheme, the member rt->rt_dst will no longer even exist.

This is what I've been working towards for more than a year, it is a
slow and very difficult transformation.

If you starting adding rt->rt_dst references back, we are taking
steps backwards.

Re: [PATCH] ipv4: fix for ip_options_rcv_srr() daddr update.

[Li Wei]

> My long term plan is to eliminate cached routes, we will directly use
> routing table entries instead.
> 
> In such a scheme, the member rt->rt_dst will no longer even exist.
> 
> This is what I've been working towards for more than a year, it is a
> slow and very difficult transformation.
> 
> If you starting adding rt->rt_dst references back, we are taking
> steps backwards.
> 
> 

Thank you, I got it. 

I'll do my best to fix it but without any promise, because i am a kernel
newbie :D