ICMP errors born at Jool are not being sent. #79

Closed
ydahhrk opened this Issue Feb 25, 2014 · 4 comments

Comments

Projects
None yet
2 participants
@ydahhrk
Member

ydahhrk commented Feb 25, 2014

When Jool is translating a ICMP error from 4 to 6 or 6 to 4, it crosses the border troublelessly.

But when Jool is the one who emits the error, it never reaches the cord.

The problem seems to stem from incorrect use of the kernel's icmp_send() and icmpv6_send() functions: The offending packet is apparently supposed to be already routed; if it's not, then those functions fail early and silently.

This is expected to be fixed by 3.1.3, but I'll try to work a bit extra to get it done by 3.1.2.

@ydahhrk ydahhrk added this to the 3.1.3 milestone Feb 25, 2014

@ydahhrk ydahhrk self-assigned this Feb 25, 2014

@ydahhrk

This comment has been minimized.

Show comment
Hide comment
@ydahhrk

ydahhrk Mar 6, 2014

Member

Sorry; it didn't make it to 3.1.2...

Member

ydahhrk commented Mar 6, 2014

Sorry; it didn't make it to 3.1.2...

@tadokoro

This comment has been minimized.

Show comment
Hide comment
@tadokoro

tadokoro Mar 14, 2014

Contributor

I hacked to solve this issue. Because rtable is not made before routing, I forced it made by ip_route_input_noref as the following patch.
It seems to work for me. For example, ping via IPv4 has now become being returned with "Destination Host Unreachable" generated by icmp64_send in get_or_create_bib_ipv4.
However, I don't have the confidence that it is correct.
If you have already tackled with or solved this issue, I'm sorry for my nonsense...

diff --git a/mod/icmp_wrapper.c b/mod/icmp_wrapper.c
index 3e0afab..1a33f5d 100644
--- a/mod/icmp_wrapper.c
+++ b/mod/icmp_wrapper.c
@@ -1,6 +1,7 @@
 #include "nat64/mod/icmp_wrapper.h"

 #include <net/icmp.h>
+#include <net/route.h>
 #include <linux/icmpv6.h>

 static void icmp4_send(struct fragment *frag, icmp_error_code error, __be32 info)
@@ -76,6 +77,17 @@ void icmp64_send(struct fragment *frag, icmp_error_code error, __be32 info)
        if (!frag->original_skb || !frag->original_skb->dev)
                return;

+       /* make rtable if NULL */
+       if (skb_rtable(frag->original_skb) == NULL) {
+               struct sk_buff *skb = frag->original_skb;
+               struct iphdr *iph = ip_hdr(skb);
+               int err = ip_route_input_noref(skb, iph->daddr, iph->saddr, iph->tos, skb->dev);
+               if (err) { 
+                       log_err(ERR_UNKNOWN_ERROR, "ip_route_input_noref failed: %d", err);
+               }
+               log_debug("making rtable %p", skb_rtable(frag->original_skb));
+       }
+
        switch (be16_to_cpu(frag->original_skb->protocol)) {
        case ETH_P_IP:
                icmp4_send(frag, error, info);

Contributor

tadokoro commented Mar 14, 2014

I hacked to solve this issue. Because rtable is not made before routing, I forced it made by ip_route_input_noref as the following patch.
It seems to work for me. For example, ping via IPv4 has now become being returned with "Destination Host Unreachable" generated by icmp64_send in get_or_create_bib_ipv4.
However, I don't have the confidence that it is correct.
If you have already tackled with or solved this issue, I'm sorry for my nonsense...

diff --git a/mod/icmp_wrapper.c b/mod/icmp_wrapper.c
index 3e0afab..1a33f5d 100644
--- a/mod/icmp_wrapper.c
+++ b/mod/icmp_wrapper.c
@@ -1,6 +1,7 @@
 #include "nat64/mod/icmp_wrapper.h"

 #include <net/icmp.h>
+#include <net/route.h>
 #include <linux/icmpv6.h>

 static void icmp4_send(struct fragment *frag, icmp_error_code error, __be32 info)
@@ -76,6 +77,17 @@ void icmp64_send(struct fragment *frag, icmp_error_code error, __be32 info)
        if (!frag->original_skb || !frag->original_skb->dev)
                return;

+       /* make rtable if NULL */
+       if (skb_rtable(frag->original_skb) == NULL) {
+               struct sk_buff *skb = frag->original_skb;
+               struct iphdr *iph = ip_hdr(skb);
+               int err = ip_route_input_noref(skb, iph->daddr, iph->saddr, iph->tos, skb->dev);
+               if (err) { 
+                       log_err(ERR_UNKNOWN_ERROR, "ip_route_input_noref failed: %d", err);
+               }
+               log_debug("making rtable %p", skb_rtable(frag->original_skb));
+       }
+
        switch (be16_to_cpu(frag->original_skb->protocol)) {
        case ETH_P_IP:
                icmp4_send(frag, error, info);

@ydahhrk

This comment has been minimized.

Show comment
Hide comment
@ydahhrk

ydahhrk Mar 21, 2014

Member

Your solution looks good to me. It only needs to be mirrored to IPv6, since ip_route_input_noref() is IPv4 only.

I've been thinking: For some reason, we seemed to either assume that we didn't need to route the incoming packet, or that it was already routed, but this bug contradicts the former, and issue #84 contradicts the latter.
Routing the packet, then, seems to be the proper fix for both issues.

Because your solution is also a fix for #84, I will move it to the fragment's initialization code (probably next to frag->original_skb in packet.c), so both icmp_wrapper and translate_packet can see it.

Thank you. I better commit the fix tomorrow, since otherwise it won't make it to the milestone.

Member

ydahhrk commented Mar 21, 2014

Your solution looks good to me. It only needs to be mirrored to IPv6, since ip_route_input_noref() is IPv4 only.

I've been thinking: For some reason, we seemed to either assume that we didn't need to route the incoming packet, or that it was already routed, but this bug contradicts the former, and issue #84 contradicts the latter.
Routing the packet, then, seems to be the proper fix for both issues.

Because your solution is also a fix for #84, I will move it to the fragment's initialization code (probably next to frag->original_skb in packet.c), so both icmp_wrapper and translate_packet can see it.

Thank you. I better commit the fix tomorrow, since otherwise it won't make it to the milestone.

ydahhrk added a commit that referenced this issue Mar 22, 2014

Issue #79 solved, issue #84 apparently also solved.
I still want to run a few more tests. Guess the milestone will be held back for a day.

ydahhrk added a commit that referenced this issue Mar 25, 2014

Fixed three errors found while testing the #79 fix:
- The routing of the skb needed to happen before the initialization of most of the fragment's fields, because the latter can generate ICMP errors.
- I just realized that if the BIB or session databases are too big, Jool will flood dmesg with lots of identical error messages. Fixed, but did introduce issue #88.
- When Jool didn't recognize the layer-4 protocol of a IPv6 packet, the wrong ICMP error was being generated.
@ydahhrk

This comment has been minimized.

Show comment
Hide comment
@ydahhrk

ydahhrk Mar 25, 2014

Member

It didn't need to be mirrored after all, since IPv6 packets are always already routed.

The code was merged into the master branch; closing.

Member

ydahhrk commented Mar 25, 2014

It didn't need to be mirrored after all, since IPv6 packets are always already routed.

The code was merged into the master branch; closing.

@ydahhrk ydahhrk closed this Mar 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment