Created attachment 1599 [details]
source address and target address are RFC compliant now.

Hi Tommaso,

I know there will be mistake of my patch,sorry.But below is my patch's idea:
Part1:"Source Address should be link-local"
I modified it base your comment in NS3 developer-mail(I hope I didn't 
misunderstand your means)

Part 2:"Target Address should be link-local"
according to RFC 
http://tools.ietf.org/html/rfc4861#section-8.
Static routing implies that the next-hop router's address should be link-local
 address of the router.So I modify r1's route table in 
file "icmpv6-redirect.cc", and add some code for this purpose.

Part 3:"Target link-layer address is not include, and it should be (if know)"
I'm sorry if I don't get the point, I think it's OK.When the redirect message 
we really don't know the "Target link-layer address", because from wireshark 
I see a neighbor solicitation between the two nodes.Message is sent to find 
the target link-layer address. So I didn't try to solve this.

Part 4: you remind me on github that "dst should be link-local", but my method
 is not cool.

By the way, I find some other small bugs, and I have modified them.

Thank you for your review!

LOL, in fixing this bug you found a completeòy different and unrelated bug: DAD isn't handled properly.

Check it by assigning two hosts the very same address... one (or both) should put its address to Ipv6InterfaceAddress::INVALID. I'm quite sure that this won't happen... I'll double check, but the code is pretty clear.

I'll file another bug and I'll review your patch, probably splitting in two.


Cheers,

T.

Ok, I checked the patch carefully. As usual as soon as you fix a bug, another one shows up. Good, let's exterminate some more.

Bug lists:
1) ICMP redirects should be sent from link-local addresses.
2) Routers should advertise themselves from the link-local address (as is: the routing table should have link-local addresses as next hop)
3) NA isn't working as intended (it doesn't check if the receiver has the same IP as the sender)
4) more ?

Globally, I can say that the patch isn't bad at all. However you did violate one golden rule: bugs have to be squashed one by one. Two bugs == two bugzilla entries and two patches. Never combine patches fixing more than one bug.
The rationale is:
1) we can keep the changes to a minimum
2) we have an history of why some code was changed

Hence, I'd ask you to split the patch into multiple segments, each one belonging to a separate bug. Open a new bug if this is necessary. In this case it is.

Last but not least, about the patch itself. I don't like the following points.
- The changes to IpForward are really necessary ? So for the UnicastForwardCallback.
Avoid as much as possible to change the interfaces. Basically you should change the interfaces only if this can't be avoided by any other mean. It can screw up a lot of things in user's code.
I don't mind the solution, but if this is the case, then all the callbacks should be changed accordingly.
- Ipv6Address linkLocalSrc = Ipv6Address::MakeAutoconfiguredLinkLocalAddress (Mac48Address::ConvertFrom (hardwareSrc));
This shouldn't be necessary. Plus it's bugged :P
The bug: the host might have a non-autoconfigured address.
The necessity: The RFC states:
      Source Address
                     MUST be the link-local address assigned to the
                     interface from which this message is sent.
     Destination Address
                     The Source Address of the packet that triggered the
                     redirect.
Nobody said that the Destination Address should be link-local... A packet from a link local to a global address is legit. The router should discard it, but this isn't a packet going to be routed, it's going to the final destination.

Hence, please split the patch and fill more bugs with the relevant patches there.

By the way, I already included your modifications to the ICMPv6 Lookup function. I did a similar patch yesterday, but your is better.

Last but not least. The routers are advertising their global addresses as the next hop, while it should be the link-local one. This is a separate issue and the naive solution breaks a lot of stuff. I guess the issue is related to how the nodes handle the router prefix, but I have to look at it better. Anyway, it's a separate bug.

Hi Tommaso,

"Two bugs == two bugzilla entries", got it! And when I split the patch, I think I'd better talk with you first. I think our bug lists are:
1) ICMP redirects should be sent from link-local addresses.
2) Routers should advertise themselves from the link-local address (as is: the
routing table should have link-local addresses as next hop)
3) NA isn't working as intended (it doesn't check if the receiver has the same
IP as the sender)

4) when send message to other network, the source address should be global unicast address.
in function Ipv6StaticRouting::Lookup(), when the gateway is link-local, it try 
to find a link-local address to be source address which is wrong.
5) The icmpv6 redirect shouldn't be sent when the target is not link local.
I'm not sure whether this should be called as a bug, but I think if people set 
next-hop address of routing table to be global unicast address careless,  
redirect message shouldn't be sent.
6) icmpv6 redirect is not sent when the gateway(target) is link-local.
I modify the interface to solve this problem, I worry about this, but I think 
this deserve to discuss with you, so I add it in my patch.
Icmpv6Redirect use below method to decide whether to send redirect message:

 "if (m_sendIcmpv6Redirect &&((!rtentry->GetGateway ().IsAny () && rtentry->GetGateway ().CombinePrefix (Ipv6Prefix (64)) == header.GetSourceAddress ().CombinePrefix (Ipv6Prefix (64))) || (rtentry->GetDestination ().CombinePrefix (Ipv6Prefix (64)) == header.GetSourceAddress ().CombinePrefix (Ipv6Prefix (64)))))"

but I think this method is complex, when the gateway address is link-local, 
this method doesn't work,and use ipv6Prefix(64) which will be modified when we 
can handle non-64 networks(if keep this maybe I can find a method that doesn't 
modify the interface). And when I try to find a new method, I find the 
parameters of IpForward is not enough, I can modify class Ipv6Route to add 
something like m_inputDevice, but this will screw up the code. So I change the 
interface, and the new method is simple. I think you have already know all of these, but this is a big decision, I should say why I want to do this.
I find as if this interface(IpForward) doesn't affect other code besides IpForward() and Ipv6RoutingProtocol.h. I can't be totally confident with this 
but I check carefully. And you say: "but if this is the case, then all the 
callbacks should be changed accordingly" do you means I should change: 
MulticastForwardCallback, LocalDeliverCallback, ErrorCallback? but they don't need new parameter...
Anyway I'll be glad to find a method that doesn't modify the interface : )

if you agree I'll leave bug 1) here, and open new bugs for others.

Splitting this bug in multiple bugs, so to ease its tracking

pushed in changeset: 9833:a12922454a0c