Currently passt wrongly ignores small IP packets if their ethernet frame was padded to ETH_ZLEN. This fixes the problem: diff --git a/tap.c b/tap.c index ee79be0..051d370 100644 --- a/tap.c +++ b/tap.c @@ -615,7 +615,7 @@ resume: continue; hlen = iph->ihl * 4UL; - if (hlen < sizeof(*iph) || htons(iph->tot_len) != l3_len || + if (hlen < sizeof(*iph) || htons(iph->tot_len) > l3_len || hlen > l3_len) continue; Note that ipv4 header is (AFAIK) not updated to the ethernet frame padding, so the strict compare was wrong.
I think the patch is correct, but with that change we should also update l4_len to reflect the padding bytes (l4_len = iph->tot_len - hlen -- not tested), right? We pass l4_len to protocol handlers.
Yep! And with that change, things actually work much better. I mean: I never got the things working, dhcp still doesn't work for me, and so on. So I just submitted what I had, and with your suggestion more things do work than do not: diff --git a/tap.c b/tap.c index ee79be0..8d7859c 100644 --- a/tap.c +++ b/tap.c @@ -615,7 +615,7 @@ resume: continue; hlen = iph->ihl * 4UL; - if (hlen < sizeof(*iph) || htons(iph->tot_len) != l3_len || + if (hlen < sizeof(*iph) || htons(iph->tot_len) > l3_len || hlen > l3_len) continue; @@ -623,7 +623,7 @@ resume: if (tap4_is_fragment(iph, now)) continue; - l4_len = l3_len - hlen; + l4_len = htons(iph->tot_len) - hlen; if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr) c->ip4.addr_seen.s_addr = iph->saddr;
Okay, thanks for the update! Should we wait to fix this until you have a couple more things running? I don't have a convenient way to test with small, padded packets.
No, small packets are now working fine (for the limited test-cases I tried). If I'd be debugging further, that would be DHCP, as I only got BOOTP working so far. If you want to debug my DHCP problems too, then I can explain your my setup, but that will require some work to replicate.
By the way, an off-topic question: have you considered the library approach, a-la libslirp? Yes, I realize you want a separate process for security considerations. But even in that case the lib can just fork() and communicate with the original process via pipes.
(In reply to stsp from comment #5) > By the way, an off-topic question: > have you considered the library approach, > a-la libslirp? We had questions in that sense, yes. But: > Yes, I realize you want a separate process > for security considerations. But even in > that case the lib can just fork() and > communicate with the original process > via pipes. ...that would still mean that we have a process, communicating via pipes with the "less trusted" process, which has access to the program segments and memory of, say, QEMU. And at the same time I don't really see the advantage -- what do you have in mind? Plus, in terms of LSMs (AppArmor, SELinux), seccomp profiles, sandboxing via namespacing (see isolation.c) this would add quite some complexity. (In reply to stsp from comment #4) > No, small packets are now working fine > (for the limited test-cases I tried). > If I'd be debugging further, that would > be DHCP, as I only got BOOTP working so far. > If you want to debug my DHCP problems > too, then I can explain your my setup, > but that will require some work to replicate. It would be interesting (maybe as part of bug 72) to have more details of your DHCP issues, yes. As to this one, if that part is now working for you, I would suggest you review a bit the rest of the function to see if more adjustments are needed (I didn't check) and post it too. Thanks.
> I would suggest you review a bit the rest of the function to see if > more adjustments are needed (I didn't check) and post it too. Thanks. Done. I made sure the rest of the function doesn't use l3_len and iph->tot_len - it just uses a (properly now) calculated l4_len.
> And at the same time I don't really see the advantage -- > what do you have in mind? More convenient. I, as a user, only launch the prog, and I don't care if that prog uses slirp or passt - everything "just works". With a standalone daemon I am confused. Should I start it every time before running my program? Or should root start it? Or should systemd start it? Why would I care, if I am just a user who wants to run the prog?
We could consider a "helper" library. It wouldn't share any code with passt proper, it would just contain things to start the passt daemon and communicate with it. I'm afraid it would be rather a long way down the priority list though.
stsp has posted a patch, moving to IN_PROGRESS.
> We could consider a "helper" library. It wouldn't share any code with > passt proper, it would just contain things to start the passt daemon and > communicate with it. That will solve the "I, as a user, don't care about running your daemons" part. There is also the second part. I as a programmer may want to port the network handling code of passt to non-canonical targets, like mingw or djgpp. libslirp provides a really excellent design for that: it uses callbacks for any platform-specific things, even timers. So you port only the very core, and make your own platform-specific bindings. Without seccomp, apparmor and other optioal things that are not available on some targets. Unless passt provides the similar layered design, porting to non-canonical targets is impossible.