Tue 13 Mar 2012 04:31:45 PM UTC, comment #12:
Lets assume it is fixed and reopen if Sebastian updates otherwise.
|
Fri 20 Jan 2012 04:22:28 PM UTC, comment #11:
It's been working well for me, but we should run the original test to be sure.
|
Fri 20 Jan 2012 06:41:24 AM UTC, comment #10:
What's the status on this? It's fixed, isn't it? Sebastian, any progress testing this again?
|
Tue 29 Nov 2011 07:53:56 PM UTC, comment #9:
I modified this again. We were checking for current_netif == NULL, but sometimes this was not true. For example, when someone sends a fragment and we free the oldest fragment in buffer, we were sending time-exceeded to the latter (as ip6_current_src() was a valid address, but the wrong one).
|
Tue 22 Nov 2011 08:44:48 PM UTC, comment #8:
Thanks for fixing that oversight.
|
Tue 22 Nov 2011 08:43:31 PM UTC, comment #7:
Ivan, there was an alignment problem in your fix (cannot assign (packed) ip6_addr_p_t* to ip6_addr_t*) which I just fixed (need to copy the packed address to an aligned address first).
|
Thu 17 Nov 2011 05:57:59 PM UTC, comment #6:
I've pushed a fix for this, Sebastian can you run your test again, if successfull I will close this bug.
Cheers, Ivan.
|
Thu 17 Nov 2011 05:26:56 PM UTC, comment #5:
Hey Sebastian, thanks for posting the bug.
I think we'll have to go with something like solution 1. I checked the patch and I see a small issue: the ICMP message might be triggered by ip6_reass_remove_oldest_datagram(), which is sometimes called from the receive callback if we need to make room for a new fragmented packet.
In this case, your patch overrides the valid ip6_current_src_addr() and that could create a problem.
I will fix this by matching the destination address in the fragmented packet to a netif, it should match the netif on which the packet was accepted originally. I think this approach is used in ND6 as well.
|
Thu 17 Nov 2011 07:01:45 AM UTC, comment #4:
If the packets originate from a link-local address and you don't know the netif, you can't tell where to send the responses, or can you? If so, there's no problem with your suggestion.
> I'm assuming that the current IP data (such as the current
> source address) isn't used when not in a receive callback.
That assumption is true. That's why I suggested to set-and-reset it when we are not in the input call stack (if we have to use it for ICMP, that is).
|
Thu 17 Nov 2011 06:53:53 AM UTC, comment #3:
What's the reasoning behind sending the ICMP out on the exact same interface the packet came in on? My patch changes that and uses the normal routing to determine the interface the packet should be sent out on. I believe that's a better solution because it probably works better with mobile IP v6.
Please note that my change affects all ICMP responses not just the time exceeded response from the reassembly timer.
I'm assuming that the current IP data (such as the current source address) isn't used when not in a receive callback. If that assumption doesn't actually hold, then my patch is going to do strange and bad things. One of the LWIP architecture gurus should have a good look at it.
|
Thu 17 Nov 2011 06:28:21 AM UTC, comment #2:
> the problem is the use of ip_current_netif() in icmp6_send_response
Plus it uses ip6_current_src_addr(), which also is only valid during input. However, while the IP6 source address can be found in the pbuf passed to icmp6_send_response (see the IPv4 version of that function), the netif is not noted anywhere.
Wouldn't solution 1 fail on link-local addresses when we have multiple netifs? I think a mix of 1 and 2 would be the best:
- remember the netif on input (add a member in struct ip6_reassdata)
- in ip6_reass_free_complete_datagram, check if the netif is still in netif_list (and if it's up) and set ip_current_netif to it (resetting it to NULL after icmp6_time_exceeded() returned)
- then, use that netif in icmp6_send_response or use the default routing if it is NULL
> BTW the version says CVS Head, but it really is the GIT HEAD.
Yeah, I think only project leaders can change that. However, it's clear to us what you mean. :-)
|
Thu 17 Nov 2011 04:10:50 AM UTC, comment #1:
BTW the version says CVS Head, but it really is the GIT HEAD.
|
Thu 17 Nov 2011 12:40:51 AM UTC, original submission:
Hi there,
When I run the IPv6Ready logo test suite phase 2 against LWIP, it segfaults in one of the tests. The following is a backtrace from gdb:
Program terminated with signal 11, Segmentation fault.
#0 0x080b6fde in ip6_select_source_address (netif=0x0, dest=0x8523650) at lwip/core/ipv6/ip6.c:221
221 if (ip6_addr_isvalid(netif_ip6_addr_state(netif, i)) &&
(gdb) bt
#0 0x080b6fde in ip6_select_source_address (netif=0x0, dest=0x8523650) at lwip/core/ipv6/ip6.c:221
#1 0x080b8111 in icmp6_send_response (p=0x8513728, code=1 '�01', data=0, type=3 '�03') at lwip/core/ipv6/icmp6.c:281
#2 0x080b8008 in icmp6_time_exceeded (p=0x8513728, c=ICMP6_TE_FRAG) at lwip/core/ipv6/icmp6.c:227
#3 0x080b8296 in ip6_reass_free_complete_datagram (ipr=0x850796c) at lwip/core/ipv6/ip6_frag.c:153
#4 0x080b821a in ip6_reass_tmr () at lwip/core/ipv6/ip6_frag.c:123
#5 0x080b38d5 in ip6_reass_timer (arg=0x0) at lwip/core/timers.c:249
#6 0x080b3b89 in sys_check_timeouts () at lwip/core/timers.c:404
(gdb) up
#1 0x080b8111 in icmp6_send_response (p=0x8513728, code=1 '�01', data=0, type=3 '�03') at lwip/core/ipv6/icmp6.c:281
281 reply_src = ip6_select_source_address(ip_current_netif(), ip6_current_src_addr());
As far as I can see, the problem is the use of ip_current_netif() in icmp6_send_response. It's documentation clearly states that it may only be called from a receive callback, but we are not in a receive callback.
The question is, how best to fix it. I can conceive of two basic solutions:
1. In icmp6_send_response, if ip_current_netif() returns 0, use the normal logic for finding the outgoing interface based on the destination address.
2. When we queue up a fragment for reassembly, we remember the interface it came in on and use that when sending the response.
However, I can think of a host of problems with 2, such as interfaces disappearing in the meantime etc. So I would lean towards solution 1.
|