Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
laforge has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. libgtp: Remove packets in tx queue belonging pdp being freed Doing so should avoid the crash seen in OS#3956, where a message is received in osmo-sgsn gtp iface after having received a DeleteCtxAccept message where pdp and associated cbp is freed. As a result, when new confirmation arrives, it can still be matched against an old request and be sent to upper layers providing an already freed cbp. With this patch, since all queued messages belonging to that pdp are dropped, confirmation won't find a match and be discarded in libgtp. In order to be able to drop all req messages belonging to a pdp, a new list is added to pdp_t and qmsg_t are added to that list when inserted into the per-gsn req transmit queue. This way upon pdp free time it's simply a matter of iterating over that list to remove all messages. There's no need to do same for resp queue, and it'd be actually counter-productive, because it wouldn't be possible to detect and discard duplicates anymore after pdp ctx has been freed. Related: OS#3956 Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a --- M TODO-RELEASE M gtp/gtp.c M gtp/pdp.c M gtp/pdp.h M gtp/queue.c M gtp/queue.h 6 files changed, 28 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/TODO-RELEASE b/TODO-RELEASE index d0852fc..73e3189 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -7,3 +7,5 @@ # If any interfaces have been added since the last public release: c:r:a + 1. # If any interfaces have been removed or changed since the last public release: c:r:0. #library whatdescription / commit summary line +libgtp queue.h struct qmsg_t got a new field: entry +libgtp pdp.h struct pdp_t got a new field: qmsg_list_req diff --git a/gtp/gtp.c b/gtp/gtp.c index 94c3245..799f8a7 100644 --- a/gtp/gtp.c +++ b/gtp/gtp.c @@ -513,6 +513,8 @@ qmsg->cbp = cbp; qmsg->type = ntoh8(packet->gtp0.h.type); qmsg->fd = fd; + if (pdp) /* echo requests are not pdp-bound */ + llist_add(>entry, >qmsg_list_req); } gsn->seq_next++;/* Count up this time */ return 0; @@ -697,6 +699,9 @@ qmsg->cbp = NULL; qmsg->type = 0; qmsg->fd = fd; + /* No need to add to pdp list here, because even on pdp ctx free + we want to leave messages in queue_resp until timeout to + detect duplicates */ } return 0; } diff --git a/gtp/pdp.c b/gtp/pdp.c index d745916..eaef545 100644 --- a/gtp/pdp.c +++ b/gtp/pdp.c @@ -31,6 +31,7 @@ #include "pdp.h" #include "gtp.h" #include "lookupa.h" +#include "queue.h" /* *** * Functions related to PDP storage @@ -156,7 +157,7 @@ } /* Default: Generate G-PDU sequence numbers on Tx */ (*pdp)->tx_gpdu_seq = true; - + INIT_LLIST_HEAD(&(*pdp)->qmsg_list_req); return 0; } } @@ -165,7 +166,17 @@ int pdp_freepdp(struct pdp_t *pdp) { + struct qmsg_t *qmsg, *qmsg2; struct pdp_t *pdpa = pdp->gsn->pdpa; + int rc; + + /* Remove all enqueued messages belonging to this pdp from req tx transmit + queue. queue_freemsg will call llist_del(). */ + llist_for_each_entry_safe(qmsg, qmsg2, >qmsg_list_req, entry) { + if ((rc = queue_freemsg(pdp->gsn->queue_req, qmsg))) + LOGP(DLGTP, LOGL_ERROR, +"Failed freeing qmsg from qmsg_list_req during pdp_freepdp()! %d\n", rc); + } pdp_tiddel(pdp); diff --git a/gtp/pdp.h b/gtp/pdp.h index d64d394..fdfa824 100644 --- a/gtp/pdp.h +++ b/gtp/pdp.h @@ -17,6 +17,7 @@ #include #include +#include struct gsn_t; @@ -241,6 +242,8 @@ struct gsn_t *gsn; /* Back pointer to GSN where this pdp ctx belongs to */ bool tx_gpdu_seq; /* Transmit (true) or suppress G-PDU sequence numbers */ + + struct llist_head qmsg_list_req; /* list of req qmsg_t in retrans queue belonging this pdp ctx */ }; /* functions related to pdp_t management */ diff --git a/gtp/queue.c b/gtp/queue.c index ce4713e..4c25913 100644 --- a/gtp/queue.c +++ b/gtp/queue.c @@ -172,6 +172,7 @@ } else { *qmsg = >qmsga[queue->next]; queue_seqset(queue, *qmsg, peer, seq); + INIT_LLIST_HEAD(&(*qmsg)->entry); (*qmsg)->state = 1; /* Space taken */ (*qmsg)->this = queue->next;
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. Patch Set 5: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 27 Aug 2019 12:11:19 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sun, 18 Aug 2019 17:07:44 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. Patch Set 4: Forgot an extra point: * libgtp already uses LOGP() -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Sat, 17 Aug 2019 16:15:26 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. Patch Set 4: libgtp already depends on libosmocore * It's already in osmo-ggsn.git's configure.ac as a PKG_CHECK_MODULES, so it depends on it at compile time. * only known users to us (osmo-ggsn, osmo-sgsn, sgsnemu?) already depend on libosmocore both compile time and runtime. * libgtp uses OSMO_DEPRECATED(), wich means again libosmocore is needed at compile time. * libgtp uses OSMO_ASSERT(), so again needed at compile time. * libgtp uses osmo_bcd2char(), so it requires libosmocore at runtime * in Makefile.am, libgtp already links against libosmocore: "libgtp_la_LIBADD = $(LIBOSMOCORE_LIBS)" So i'm in favour of actually moving libgtp to be more "osmocom" rather than trying to keep it away from it. -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Sat, 17 Aug 2019 14:20:15 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 ) Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. Patch Set 4: so far I don't think that libgtp depends on libosmocore. You are introducing linuxlist related bits to it. I'm not entirely sure if that's a good idea, or if we should rather keep it separate. However, we may need to check if a new library dependency needs to be handled in autoconf, etc. -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Sat, 17 Aug 2019 01:16:21 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-ggsn[master]: libgtp: Remove packets in tx queue belonging pdp being freed
Hello lynxis lazus, keith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 to look at the new patch set (#4). Change subject: libgtp: Remove packets in tx queue belonging pdp being freed .. libgtp: Remove packets in tx queue belonging pdp being freed Doing so should avoid the crash seen in OS#3956, where a message is received in osmo-sgsn gtp iface after having received a DeleteCtxAccept message where pdp and associated cbp is freed. As a result, when new confirmation arrives, it can still be matched against an old request and be sent to upper layers providing an already freed cbp. With this patch, since all queued messages belonging to that pdp are dropped, confirmation won't find a match and be discarded in libgtp. In order to be able to drop all req messages belonging to a pdp, a new list is added to pdp_t and qmsg_t are added to that list when inserted into the per-gsn req transmit queue. This way upon pdp free time it's simply a matter of iterating over that list to remove all messages. There's no need to do same for resp queue, and it'd be actually counter-productive, because it wouldn't be possible to detect and discard duplicates anymore after pdp ctx has been freed. Related: OS#3956 Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a --- M TODO-RELEASE M gtp/gtp.c M gtp/pdp.c M gtp/pdp.h M gtp/queue.c M gtp/queue.h 6 files changed, 28 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/osmo-ggsn refs/changes/28/15228/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-ggsn/+/15228 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a Gerrit-Change-Number: 15228 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: keith Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset