Άλκης Γεωργόπουλος
2008-Jun-14 19:25 UTC
[klibc] PATCH: ipconfig may discard useful packets
Thank you Maximilian, this was much easier! I still have problems with my email setup, I haven't found the time to migrate my mailbox from Windows yet. And gmail (webmail) doesn't allow many customizations, so I'm also uploading the patch to http://users.sch.gr/alkisg/temp/0001-Signed-off-by.patch I'm sending a patch regarding the discard_packet() function, please consider it for inclusion. Signed-off-by: ????? ???????????? (alkisg at gmail.com) In main.c, function loop(), there's a call to packet_discard(). Unfortunately, there was a logic error in the return values of some functions, which caused it either to 1. Be called when a useful packet was in the queue, such as DHCPACK, and so this packet was discarded, a timeout occured and the correct address was acknowledged only by a retransmittion after a few seconds, or 2. Be called when no packets where in the queue, so packet_discard() just delayed until some new packet arrived, and this new packet was again discarded. I've seen many times ipconfig needing many seconds to configure a card (in my school we have 2 dhcp servers, maybe this makes the problem more common). I think I corrected the function return values, and in my tests (with either one or two dhcp servers) now ipconfig gets an address in only a few msec. Sorry, but this means that many functions were modified, but in the good side, the return values were commented. I did my best to avoid any regressions, but a second look at the code or extra testing may be needed. A scenario where the bug occurs: In main.c: nr = do_pkt_recv(pkt_fd, now.tv_sec); if (nr == 1) break; else if (nr == 0) packet_discard(); <== This one The following functions may be called: do_pkt_recv > process_receive_event > e.g. dhcp_recv_offer > dhcp_recv> packet_recvpacket_recv may receive a not-appropriate packet, and jump to discard_pkt, in which case it *discards* the packet and returns 0. This return value (0) will propagate to main, so (nr == 0) and packet_discard() will be called *again*, causing the NIC to wait until it happens to receive some packet. Kind regards, Alkis Georgopoulos --- usr/kinit/ipconfig/bootp_proto.c | 6 +++++- usr/kinit/ipconfig/dhcp_proto.c | 10 +++++----- usr/kinit/ipconfig/main.c | 32 +++++++++++++++++++++----------- usr/kinit/ipconfig/packet.c | 15 ++++++++++++--- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/usr/kinit/ipconfig/bootp_proto.c b/usr/kinit/ipconfig/bootp_proto.c index 236bde9..7df3137 100644 --- a/usr/kinit/ipconfig/bootp_proto.c +++ b/usr/kinit/ipconfig/bootp_proto.c @@ -153,6 +153,10 @@ int bootp_parse(struct netdev *dev, struct bootp_hdr *hdr, /* * Receive a bootp reply and parse packet + * Returns: + *-1 = Error in packet_recv, try again later + * 0 = Unexpected packet, discarded + * 1 = Correctly received and parsed packet */ int bootp_recv_reply(struct netdev *dev) { @@ -167,7 +171,7 @@ int bootp_recv_reply(struct netdev *dev) ret = packet_recv(iov, 3); if (ret <= 0) - return ret; + return -1; if (ret < sizeof(struct bootp_hdr) || bootp.op != BOOTP_REPLY || /* RFC951 7.5 */ diff --git a/usr/kinit/ipconfig/dhcp_proto.c b/usr/kinit/ipconfig/dhcp_proto.c index d4f2c09..82cd1ed 100644 --- a/usr/kinit/ipconfig/dhcp_proto.c +++ b/usr/kinit/ipconfig/dhcp_proto.c @@ -72,7 +72,7 @@ static struct iovec dhcp_request_iov[] = { /* * Parse a DHCP response packet * Returns: - * 0 = Not handled + * 0 = Unexpected packet, not parsed * 2 = DHCPOFFER (from dhcp_proto.h) * 5 = DHCPACK * 6 = DHCPNACK @@ -128,8 +128,8 @@ static int dhcp_parse(struct netdev *dev, struct bootp_hdr *hdr, /* * Receive and parse a DHCP packet * Returns: - *-1 = Error in packet_recv - * 0 = Not handled + *-1 = Error in packet_recv, try again later + * 0 = Unexpected packet, discarded * 2 = DHCPOFFER (from dhcp_proto.h) * 5 = DHCPACK * 6 = DHCPNACK @@ -146,8 +146,8 @@ static int dhcp_recv(struct netdev *dev) int ret; ret = packet_recv(iov, 3); - if (ret <= 0) - return ret; + if (ret == 0) + return -1; DEBUG(("\n dhcp xid %08x ", dev->bootp.xid)); diff --git a/usr/kinit/ipconfig/main.c b/usr/kinit/ipconfig/main.c index 2ded0f3..4e728e0 100644 --- a/usr/kinit/ipconfig/main.c +++ b/usr/kinit/ipconfig/main.c @@ -169,6 +169,11 @@ static void complete_device(struct netdev *dev) ifaces = dev; } +/* + * Returns: + * 0 = Not handled, the packet is still in the queue + * 1 = Handled + */ static int process_receive_event(struct state *s, time_t now) { int handled = 1; @@ -214,6 +219,10 @@ static int process_receive_event(struct state *s, time_t now) break; } break; + + default: + handled = 0; + break; } switch (s->state) { @@ -224,9 +233,6 @@ static int process_receive_event(struct state *s, time_t now) case DEVST_ERROR: /* error occurred, try again in 10 seconds */ s->expire = now + 10; - default: - DEBUG(("\n")); - handled = 0; break; } @@ -288,23 +294,30 @@ static void process_timeout_event(struct state *s, time_t now) static struct state *slist; struct netdev *ifaces; +/* + * Returns: + * 0 = Error, packet not received or discarded + * 1 = A packet was received and handled + */ static int do_pkt_recv(int pkt_fd, time_t now) { int ifindex, ret; struct state *s; ret = packet_peek(&ifindex); - if (ret < 0) - goto bail; + if (ret == 0) + return ret; for (s = slist; s; s = s->next) { if (s->dev->ifindex == ifindex) { - ret |= process_receive_event(s, now); + ret = process_receive_event(s, now); break; } } - bail: + if (ret == 0) + packet_discard(); + return ret; } @@ -371,11 +384,8 @@ static int loop(void) gettimeofday(&now, NULL); if ((fds[0].revents & POLLRDNORM)) { - nr = do_pkt_recv(pkt_fd, now.tv_sec); - if (nr == 1) + if (do_pkt_recv(pkt_fd, now.tv_sec) == 1) break; - else if (nr == 0) - packet_discard(); } if (loop_timeout >= 0 && diff --git a/usr/kinit/ipconfig/packet.c b/usr/kinit/ipconfig/packet.c index 283cf02..4bc01b5 100644 --- a/usr/kinit/ipconfig/packet.c +++ b/usr/kinit/ipconfig/packet.c @@ -165,6 +165,12 @@ int packet_send(struct netdev *dev, struct iovec *iov, int iov_len) return sendmsg(pkt_fd, &msg, 0); } +/* + * Fetches a bootp packet, but doesn't remove it. + * Returns: + * 0 = Error + * >0 = A packet of size "ret" is available for interface ifindex + */ int packet_peek(int *ifindex) { struct sockaddr_ll sll; @@ -177,7 +183,7 @@ int packet_peek(int *ifindex) ret = recvfrom(pkt_fd, &iph, sizeof(struct iphdr), MSG_PEEK, (struct sockaddr *)&sll, &sllen); if (ret == -1) - return -1; + return 0; if (sll.sll_family != AF_PACKET) goto discard_pkt; @@ -187,7 +193,7 @@ int packet_peek(int *ifindex) *ifindex = sll.sll_ifindex; - return 0; + return ret; discard_pkt: packet_discard(); @@ -207,6 +213,9 @@ void packet_discard(void) /* * Receive a bootp packet. The options are listed in iov[1...iov_len]. * iov[0] must point to the bootp packet header. + * Returns: + * 0 = Error, try again later + * >0 = Size of packet */ int packet_recv(struct iovec *iov, int iov_len) { @@ -226,7 +235,7 @@ int packet_recv(struct iovec *iov, int iov_len) ret = recvfrom(pkt_fd, &iph, sizeof(struct iphdr), MSG_PEEK, NULL, NULL); if (ret == -1) - return -1; + return 0; if (iph.ihl < 5 || iph.version != IPVERSION) goto discard_pkt; -- 1.5.4.3
On Sat, Jun 14, 2008 at 12:25 PM, ????? ???????????? <alkisg at gmail.com> wrote:> Thank you Maximilian, this was much easier! > I still have problems with my email setup, I haven't found the time to > migrate my mailbox from Windows yet. And gmail (webmail) doesn't allow > many customizations, so I'm also uploading the patch to > http://users.sch.gr/alkisg/temp/0001-Signed-off-by.patchgmail allows imap4 and smtp outgoing, so you should be able to wire mutt up to go through your gmail for sending patches. Contact me off-list if you need help. -- Jeff Bailey - http://www.raspberryginger.com/jbailey/ "Gay marriage will encourage people to be gay, in the same way that hanging around tall people will make you tall." - Peter Gallagher
Άλκης Γεωργόπουλος
2008-Jun-16 09:14 UTC
[klibc] PATCH: ipconfig may discard useful packets
I'm sending same debug output samples to demonstrate the problem. In the places where I say "the older version lost an ACK", I had wireshark running on the ltsp server to see it. I think there's ***ANOTHER*** problem as well, ipconfig considers receiving IGMP or ARP packets an error, and this causes rare but large delays (I got about 5 of them in 30 minutes of testing). I'll look into that afterwards, if you'll allow me. The debug output files are from 3 different versions: ipconfig-1.5.10 = the original unmodified version, ipconfig-git = the one in git right now, ipconfig-1.5.10-patched = with the patch proposed in this thread. Test environment: an adsl router/dhcp server = 192.168.192.1, a laptop/LTSP server/dhcp server = 192.168.0.1. Because of many attachments, I uploaded elsewhere the output, and I only enter the links and the description here. Output of ipconfig-1.5.10 losing an ACK, receiving an ARP packet, considering it an error and delaying for 1 sec: http://users.sch.gr/alkisg/temp/output1.txt Output of ipconfig-git losing an ACK, receiving 2 ARP packets, considering them as errors and delaying for *** 96 + 75 *** seconds. I think it shouldn't consider them as errors, it should just discard the packets. http://users.sch.gr/alkisg/temp/output2.txt Output of ipconfig-1.5.10-patched receiving an ARP packet, considering it an error and delaying for 10 secs. It didn't drop any packets before the error (as the other versions did), the error happend before the offer (rare - took me many minutes to reproduce). So this ARP error is on all versions. The delay depends on the errors received, I've seen all versions needing from 1 sec to some minutes. http://users.sch.gr/alkisg/temp/output3.txt Output of ipconfig-1.5.10 accepting a DHCPOFFER as DHCPNAK: http://users.sch.gr/alkisg/temp/output4.txt Output of ipconfig-1.5.10-patched ignoring the above DHCPOFFER: http://users.sch.gr/alkisg/temp/output5.txt (Usual) output of correct address aquisition with ipconfig-1.5.10-patched: http://users.sch.gr/alkisg/temp/output6.txt Sample output from dhclient: http://users.sch.gr/alkisg/temp/output7.txt Kind regards, Alkis Georgopoulos