laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30174 )

Change subject: osmux: Obey current batch_size restrictions when creating forged RTP packets to fill holes ......................................................................

osmux: Obey current batch_size restrictions when creating forged RTP packets to fill holes

osmux_link_add() is renamed to osmux_link_handle_rtp_req(), and the last part of it is split out and kept as osmux_link_add(). hence osmux_link_handle_rtp_req() does proper input checking (like duplicates, holes, etc.) while osmux_link_add() expects all that to be sorted out. Reuse osmux_link_add() in osmux_replay_lost_packets() to properly update the link state of the to-be-transmitted packet, circuit state, etc.

Change-Id: I4ea435bfb2490a375ad3e5068ee926e48b53cf5c --- M src/osmux_input.c M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 3 files changed, 125 insertions(+), 110 deletions(-)

Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved

diff --git a/src/osmux_input.c b/src/osmux_input.c index a50a179..0126da7 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -353,71 +353,6 @@

}

-/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ -static int osmux_replay_lost_packets(struct osmux_link *link, const struct osmux_in_req *req) -{ - int16_t diff; - struct msgb *last; - struct rtp_hdr *last_rtph, *rtph; - uint16_t last_seq, cur_seq; - int i, rc; - - /* Have we seen any RTP packet in this batch before? */ - if (llist_empty(&req->circuit->msg_list)) - return 0; - - /* Get last RTP packet seen in this batch */ - last = llist_entry(req->circuit->msg_list.prev, struct msgb, list); - last_rtph = osmo_rtp_get_hdr(last); - if (last_rtph == NULL) - return -1; - last_seq = ntohs(last_rtph->sequence); - cur_seq = ntohs(req->rtph->sequence); - diff = cur_seq - last_seq; - - /* If diff between last RTP packet seen and this one is > 1, - * then we lost several RTP packets, let's replay them. - */ - if (diff <= 1) - return 0; - - LOGP(DLMUX, LOGL_INFO, "RTP seq jump detected, recreating %" PRId16 - " lost packets (seq jump: %" PRIu16 " -> %" PRIu16 ")\n", - diff - 1, last_seq, cur_seq); - - /* Lifesaver: make sure bugs don't spawn lots of clones */ - if (diff > 16) - diff = 16; - - rc = 0; - rtph = last_rtph; - for (i = 1; i < diff; i++) { - struct msgb *clone; - - /* Clone last RTP packet seen */ - clone = msgb_copy(last, "RTP clone"); - if (!clone) - continue; - - /* The original RTP message has been already sanity checked. */ - rtph = osmo_rtp_get_hdr(clone); - /* Faking a follow up RTP pkt here, so no Marker bit: */ - rtph->marker = false; - /* Adjust sequence number and timestamp */ - rtph->sequence = htons(ntohs(rtph->sequence) + i); - rtph->timestamp = htonl(ntohl(rtph->timestamp) + - DELTA_RTP_TIMESTAMP); - - /* No more room in this batch, skip padding with more clones */ - rc = osmux_circuit_enqueue(link, req->circuit, clone); - if (rc != 0) { - msgb_free(clone); - return rc; - } - } - return rc; -} - static struct osmux_circuit * osmux_link_find_circuit(struct osmux_link *link, int ccid) { @@ -440,51 +375,10 @@ }

/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ -static int -osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req) +static int osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req) { - struct msgb *cur, *next; - int rc; unsigned int needed_bytes = 0; - - /* We've seen the first RTP message, disable dummy padding */ - if (req->circuit->dummy) { - req->circuit->dummy = 0; - link->ndummy--; - } - - /* Extra validation: check if this message already exists, should not - * happen but make sure we don't propagate duplicated messages. - */ - llist_for_each_entry_safe(cur, next, &req->circuit->msg_list, list) { - struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); - OSMO_ASSERT(rtph2); - - /* Already exists message with this sequence. Let's copy over - * the new RTP, since there's the chance that the existing one may - * be a forged copy we did when we detected a hole. */ - if (rtph2->sequence == req->rtph->sequence) { - if (msgb_length(cur) != msgb_length(req->msg)) { - /* Different packet size, AMR FT may have changed. Let's avoid changing it to - * break accounted size to be written (would need new osmux_hdr, etc.) */ - LOGP(DLMUX, LOGL_NOTICE, "RTP pkt with seq=%u and different len %u != %u already exists, skip it\n", - ntohs(req->rtph->sequence), msgb_length(cur), msgb_length(req->msg)); - return -1; - } - LOGP(DLMUX, LOGL_INFO, "RTP pkt with seq=%u already exists, replace it\n", - ntohs(req->rtph->sequence)); - __llist_add(&req->msg->list, &cur->list, cur->list.next); - llist_del(&cur->list); - msgb_free(cur); - return 0; - } - } - - /* Handle RTP packet loss scenario */ - rc = osmux_replay_lost_packets(link, req); - if (rc != 0) - return rc; - + int rc; /* Init of talkspurt (RTP M marker bit) needs to be in the first AMR slot * of the OSMUX packet, enforce sending previous batch if required: */ @@ -528,6 +422,119 @@ link->nmsgs++;

return 0; +}; + +/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ +static int osmux_replay_lost_packets(struct osmux_link *link, const struct osmux_in_req *req) +{ + int16_t diff; + struct msgb *last; + struct rtp_hdr *last_rtph; + uint16_t last_seq, cur_seq; + int i, rc; + struct osmux_in_req clone_req; + + /* Have we seen any RTP packet in this batch before? */ + if (llist_empty(&req->circuit->msg_list)) + return 0; + + /* Get last RTP packet seen in this batch */ + last = llist_entry(req->circuit->msg_list.prev, struct msgb, list); + last_rtph = osmo_rtp_get_hdr(last); + if (last_rtph == NULL) + return -1; + last_seq = ntohs(last_rtph->sequence); + cur_seq = ntohs(req->rtph->sequence); + diff = cur_seq - last_seq; + + /* If diff between last RTP packet seen and this one is > 1, + * then we lost several RTP packets, let's replay them. + */ + if (diff <= 1) + return 0; + + LOGP(DLMUX, LOGL_INFO, "RTP seq jump detected, recreating %" PRId16 + " lost packets (seq jump: %" PRIu16 " -> %" PRIu16 ")\n", + diff - 1, last_seq, cur_seq); + + /* Lifesaver: make sure bugs don't spawn lots of clones */ + if (diff > 16) + diff = 16; + + rc = 0; + clone_req = *req; + for (i = 1; i < diff; i++) { + /* Clone last RTP packet seen */ + clone_req.msg = msgb_copy(last, "RTP clone"); + if (!clone_req.msg) + continue; + + /* The original RTP message has been already sanity checked. */ + clone_req.rtph = osmo_rtp_get_hdr(clone_req.msg); + clone_req.amrh = osmo_rtp_get_payload(clone_req.rtph, clone_req.msg, &clone_req.rtp_payload_len); + clone_req.amr_payload_len = osmux_rtp_amr_payload_len(clone_req.amrh, clone_req.rtp_payload_len); + + /* Faking a follow up RTP pkt here, so no Marker bit: */ + clone_req.rtph->marker = false; + /* Adjust sequence number and timestamp */ + clone_req.rtph->sequence = htons(ntohs(clone_req.rtph->sequence) + i); + clone_req.rtph->timestamp = htonl(ntohl(clone_req.rtph->timestamp) + + DELTA_RTP_TIMESTAMP); + rc = osmux_link_add(link, &clone_req); + /* No more room in this batch, skip padding with more clones */ + if (rc != 0) { + msgb_free(clone_req.msg); + return rc; + } + } + return rc; +} + +/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ +static int osmux_link_handle_rtp_req(struct osmux_link *link, struct osmux_in_req *req) +{ + struct msgb *cur, *next; + int rc; + + /* We've seen the first RTP message, disable dummy padding */ + if (req->circuit->dummy) { + req->circuit->dummy = 0; + link->ndummy--; + } + + /* Extra validation: check if this message already exists, should not + * happen but make sure we don't propagate duplicated messages. + */ + llist_for_each_entry_safe(cur, next, &req->circuit->msg_list, list) { + struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); + OSMO_ASSERT(rtph2); + + /* Already exists message with this sequence. Let's copy over + * the new RTP, since there's the chance that the existing one may + * be a forged copy we did when we detected a hole. */ + if (rtph2->sequence == req->rtph->sequence) { + if (msgb_length(cur) != msgb_length(req->msg)) { + /* Different packet size, AMR FT may have changed. Let's avoid changing it to + * break accounted size to be written (would need new osmux_hdr, etc.) */ + LOGP(DLMUX, LOGL_NOTICE, "RTP pkt with seq=%u and different len %u != %u already exists, skip it\n", + ntohs(req->rtph->sequence), msgb_length(cur), msgb_length(req->msg)); + return -1; + } + LOGP(DLMUX, LOGL_INFO, "RTP pkt with seq=%u already exists, replace it\n", + ntohs(req->rtph->sequence)); + __llist_add(&req->msg->list, &cur->list, cur->list.next); + llist_del(&cur->list); + msgb_free(cur); + return 0; + } + } + + /* Handle RTP packet loss scenario */ + rc = osmux_replay_lost_packets(link, req); + if (rc != 0) + return rc; + + return osmux_link_add(link, req); }

/** @@ -592,7 +599,7 @@ }

/* Add this RTP to the OSMUX batch */ - ret = osmux_link_add(link, &req); + ret = osmux_link_handle_rtp_req(link, &req); if (ret < 0) { /* Cannot put this message into the batch. * Malformed, duplicated, OOM. Drop it and tell diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index a728f6b..eb2fb79 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -683,7 +683,7 @@ clock_debug("osmux batch transmitted"); clock_override_add(0, TIME_RTP_PKT_MS*1000); osmo_select_main(0); - OSMO_ASSERT(osmux_transmitted == 1); /* FIXME: this is a bug, should be 2! */ + OSMO_ASSERT(osmux_transmitted == 2);

clock_debug("Closing circuit"); osmux_xfrm_input_close_circuit(h_input, cid); diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok index 006d3f0..f8a5e6f 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -119,6 +119,8 @@ sys={0.280000}, mono={0.280000}: clock_override_add sys={0.280000}, mono={0.280000}: osmux batch transmitted sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + sys={0.300000}, mono={0.300000}: Closing circuit ===test_rtp_pkt_gap_bigger_than_batch_factor(65533)=== sys={0.000000}, mono={0.000000}: clock_override_set @@ -146,6 +148,8 @@ sys={0.280000}, mono={0.280000}: clock_override_add sys={0.280000}, mono={0.280000}: osmux batch transmitted sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + sys={0.300000}, mono={0.300000}: Closing circuit ===test_rtp_pkt_gap_bigger_than_batch_factor(65534)=== sys={0.000000}, mono={0.000000}: clock_override_set @@ -173,6 +177,8 @@ sys={0.280000}, mono={0.280000}: clock_override_add sys={0.280000}, mono={0.280000}: osmux batch transmitted sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + sys={0.300000}, mono={0.300000}: Closing circuit ===test_rtp_pkt_gap_bigger_than_batch_factor(65535)=== sys={0.000000}, mono={0.000000}: clock_override_set @@ -200,5 +206,7 @@ sys={0.280000}, mono={0.280000}: clock_override_add sys={0.280000}, mono={0.280000}: osmux batch transmitted sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: OSMUX message 2 (len=64): OSMUX seq=124 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + sys={0.300000}, mono={0.300000}: Closing circuit OK: Test passed