From 0e7e4fb0a6b8f1043182dcccc91a7b984587d1ae Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Fri, 8 Sep 2017 16:53:59 +0100 Subject: [PATCH 1/3] slirp: Add explanation for hostfwd parsing failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e.g. ./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22' qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address) Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Samuel Thibault --- net/slirp.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/net/slirp.c b/net/slirp.c index 01ed21c006..318a26e892 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, char buf[256]; int is_udp; char *end; + const char *fail_reason = "Unknown reason"; p = redir_str; if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) { + fail_reason = "No : separators"; goto fail_syntax; } if (!strcmp(buf, "tcp") || buf[0] == '\0') { @@ -506,35 +508,43 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, } else if (!strcmp(buf, "udp")) { is_udp = 1; } else { + fail_reason = "Bad protocol name"; goto fail_syntax; } if (!legacy_format) { if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) { + fail_reason = "Missing : separator"; goto fail_syntax; } if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) { + fail_reason = "Bad host address"; goto fail_syntax; } } if (get_str_sep(buf, sizeof(buf), &p, legacy_format ? ':' : '-') < 0) { + fail_reason = "Bad host port separator"; goto fail_syntax; } host_port = strtol(buf, &end, 0); if (*end != '\0' || host_port < 0 || host_port > 65535) { + fail_reason = "Bad host port"; goto fail_syntax; } if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) { + fail_reason = "Missing guest address"; goto fail_syntax; } if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) { + fail_reason = "Bad guest address"; goto fail_syntax; } guest_port = strtol(p, &end, 0); if (*end != '\0' || guest_port < 1 || guest_port > 65535) { + fail_reason = "Bad guest port"; goto fail_syntax; } @@ -547,7 +557,8 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, return 0; fail_syntax: - error_setg(errp, "Invalid host forwarding rule '%s'", redir_str); + error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str, + fail_reason); return -1; } From e2aad34d73a9bd2b95275598daf05f190a02b899 Mon Sep 17 00:00:00 2001 From: Kevin Cernekee Date: Wed, 20 Sep 2017 13:42:04 -0700 Subject: [PATCH 2/3] slirp: Fix intermittent send queue hangs on a socket if_output() originally sent one mbuf per call and used the slirp->next_m variable to keep track of where it left off. But nowadays it tries to send all of the mbufs from the fastq, and one mbuf from each session on the batchq. The next_m variable is both redundant and harmful: there is a case[0] involving delayed packets in which next_m ends up pointing to &slirp->if_batchq when an active session still exists, and this blocks all traffic for that session until qemu is restarted. The test case was created to reproduce a problem that was seen on long-running Chromium OS VM tests[1] which rapidly create and destroy ssh connections through hostfwd. [0] https://pastebin.com/NNy6LreF [1] https://bugs.chromium.org/p/chromium/issues/detail?id=766323 Signed-off-by: Kevin Cernekee Signed-off-by: Samuel Thibault --- slirp/if.c | 51 +++++++++++++++++---------------------------------- slirp/slirp.h | 1 - 2 files changed, 17 insertions(+), 35 deletions(-) diff --git a/slirp/if.c b/slirp/if.c index 51ae0d0e9a..6262d77495 100644 --- a/slirp/if.c +++ b/slirp/if.c @@ -30,7 +30,6 @@ if_init(Slirp *slirp) { slirp->if_fastq.qh_link = slirp->if_fastq.qh_rlink = &slirp->if_fastq; slirp->if_batchq.qh_link = slirp->if_batchq.qh_rlink = &slirp->if_batchq; - slirp->next_m = (struct mbuf *) &slirp->if_batchq; } /* @@ -100,10 +99,6 @@ if_output(struct socket *so, struct mbuf *ifm) } } else { ifq = (struct mbuf *) slirp->if_batchq.qh_rlink; - /* Set next_m if the queue was empty so far */ - if ((struct quehead *) slirp->next_m == &slirp->if_batchq) { - slirp->next_m = ifm; - } } /* Create a new doubly linked list for this session */ @@ -143,21 +138,18 @@ diddit: } /* - * Send a packet - * We choose a packet based on its position in the output queues; + * Send one packet from each session. * If there are packets on the fastq, they are sent FIFO, before - * everything else. Otherwise we choose the first packet from the - * batchq and send it. the next packet chosen will be from the session - * after this one, then the session after that one, and so on.. So, - * for example, if there are 3 ftp session's fighting for bandwidth, + * everything else. Then we choose the first packet from each + * batchq session (socket) and send it. + * For example, if there are 3 ftp sessions fighting for bandwidth, * one packet will be sent from the first session, then one packet - * from the second session, then one packet from the third, then back - * to the first, etc. etc. + * from the second session, then one packet from the third. */ void if_start(Slirp *slirp) { uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME); - bool from_batchq, next_from_batchq; + bool from_batchq = false; struct mbuf *ifm, *ifm_next, *ifqt; DEBUG_CALL("if_start"); @@ -167,26 +159,29 @@ void if_start(Slirp *slirp) } slirp->if_start_busy = true; + struct mbuf *batch_head = NULL; + if (slirp->if_batchq.qh_link != &slirp->if_batchq) { + batch_head = (struct mbuf *) slirp->if_batchq.qh_link; + } + if (slirp->if_fastq.qh_link != &slirp->if_fastq) { ifm_next = (struct mbuf *) slirp->if_fastq.qh_link; - next_from_batchq = false; - } else if ((struct quehead *) slirp->next_m != &slirp->if_batchq) { - /* Nothing on fastq, pick up from batchq via next_m */ - ifm_next = slirp->next_m; - next_from_batchq = true; + } else if (batch_head) { + /* Nothing on fastq, pick up from batchq */ + ifm_next = batch_head; + from_batchq = true; } else { ifm_next = NULL; } while (ifm_next) { ifm = ifm_next; - from_batchq = next_from_batchq; ifm_next = ifm->ifq_next; if ((struct quehead *) ifm_next == &slirp->if_fastq) { /* No more packets in fastq, switch to batchq */ - ifm_next = slirp->next_m; - next_from_batchq = true; + ifm_next = batch_head; + from_batchq = true; } if ((struct quehead *) ifm_next == &slirp->if_batchq) { /* end of batchq */ @@ -199,11 +194,6 @@ void if_start(Slirp *slirp) continue; } - if (ifm == slirp->next_m) { - /* Set which packet to send on next iteration */ - slirp->next_m = ifm->ifq_next; - } - /* Remove it from the queue */ ifqt = ifm->ifq_prev; remque(ifm); @@ -214,15 +204,8 @@ void if_start(Slirp *slirp) insque(next, ifqt); ifs_remque(ifm); - if (!from_batchq) { - /* Next packet in fastq is from the same session */ ifm_next = next; - next_from_batchq = false; - } else if ((struct quehead *) slirp->next_m == &slirp->if_batchq) { - /* Set next_m and ifm_next if the session packet is now the - * only one on batchq */ - slirp->next_m = ifm_next = next; } } diff --git a/slirp/slirp.h b/slirp/slirp.h index 5af4f482b5..898ec9516d 100644 --- a/slirp/slirp.h +++ b/slirp/slirp.h @@ -183,7 +183,6 @@ struct Slirp { /* if states */ struct quehead if_fastq; /* fast queue (for interactive data) */ struct quehead if_batchq; /* queue for non-interactive data */ - struct mbuf *next_m; /* pointer to next mbuf to output */ bool if_start_busy; /* avoid if_start recursion */ /* ip states */ From 13146a83951e045c810c37c5c11c2a016ebc0663 Mon Sep 17 00:00:00 2001 From: Kevin Cernekee Date: Wed, 20 Sep 2017 13:42:05 -0700 Subject: [PATCH 3/3] slirp: Add a special case for the NULL socket NULL sockets are used for NDP, BOOTP, and other critical operations. If the topmost mbuf in a NULL session is blocked pending resolution, it may cause problems if it blocks other packets with a NULL socket. So do not add mbufs with a NULL socket field to the same session. Signed-off-by: Kevin Cernekee Signed-off-by: Samuel Thibault --- slirp/if.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/slirp/if.c b/slirp/if.c index 6262d77495..590753c658 100644 --- a/slirp/if.c +++ b/slirp/if.c @@ -73,14 +73,16 @@ if_output(struct socket *so, struct mbuf *ifm) * We mustn't put this packet back on the fastq (or we'll send it out of order) * XXX add cache here? */ - for (ifq = (struct mbuf *) slirp->if_batchq.qh_rlink; - (struct quehead *) ifq != &slirp->if_batchq; - ifq = ifq->ifq_prev) { - if (so == ifq->ifq_so) { - /* A match! */ - ifm->ifq_so = so; - ifs_insque(ifm, ifq->ifs_prev); - goto diddit; + if (so) { + for (ifq = (struct mbuf *) slirp->if_batchq.qh_rlink; + (struct quehead *) ifq != &slirp->if_batchq; + ifq = ifq->ifq_prev) { + if (so == ifq->ifq_so) { + /* A match! */ + ifm->ifq_so = so; + ifs_insque(ifm, ifq->ifs_prev); + goto diddit; + } } }