So I've been scanning the code for roughly a week now and I feel
like I should report something, for the sake of collecting my
thoughts here if nothing else.
We've been trying to find #232. It's one hell of a bug. On one
hand, it's easy to tell from the stack trace that the crash
happened during the translation of the *inner* payload of an ICMP
error from IPv4 to IPv6 caused by a TCP packet. On the other hand,
there is no way to reproduce it yet, the review is yielding little
more than optimizations and the failure rate (once in the two years
the relevant code has existed) suggests that the problem is
something otherworldly (ie. undefined behavior, which could have
been triggered anywhere in the kernel).
There was no hairpinning involved. ICMP errors are never supposed
to be fragmented. Even if this particular packet were, the crash
happened during the first fragment's copy. The fact that SIIT is
the one that crashed is fortunate since it means there's less code
to worry about.
It crashed during one of the `memcpy()`s of the kernel's
`skb_copy_bits()`, sitting at Jool's `copy_payload()`. The crash
looks like a typical memory access fault, which would mean that at
least one of the following fields had an incorrect value during the
Jool rarely needs to edit the incoming packet and when it does it's
via the kernel API. The borked field is most likely one of the
Ok so I haven't necessarily fixed the bug but I did find room for
improvement. Fragment translation is one area where I feel Jool is
too kernel-aware and, though I don't see even potential problems in
this code now (given that fragmentation has little to do with the
crashed packet to begin with), future kernel refactors regarding
fragment representation can come back and shoot me in the foot. The
problem is that Jool is copying subsequent packet payload *and even
pages* when a simple reference grab can do the job. Subsequent
fragments lack headers so they can theoretically be quirklessly
shared between incoming and translated packets. Fixing this would
have the additional benefit of speeding up translation since only
head data (not paged nor fragmented) would need to be copied. I can
also see it trumping the offloading problem but I've been there
before and I'm not getting my hopes up.
IIRC, I implemented it as it is because the kernel's suggested
fragment-transparent solution does not necessarily account for the
potential header growth (from IPv4 to IPv6) and the kernel can find
itself in deep trouble if an skb cannot be `skb_push`ed enough. I
did some tests however, and it seems that precisely when
fragmentation is involved the kernel tends to reserve plenty of
excess headroom for some reason. So I might be on to something.
I also found other small errors but I don't see a kernel panic
coming out of any of them. In fact, since this is the first time
I've seen them I'm somewhat skeptical as to whether I actually
fixed something or introduced more problems. Currently testing.
If anything, this commit should stick because I added and updated
loads of documentation during the review.