<p>laforge <strong>merged</strong> this change.</p><p><a href="https://gerrit.osmocom.org/c/osmo-ggsn/+/15228">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, approved
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">libgtp: Remove packets in tx queue belonging pdp being freed<br><br>Doing so should avoid the crash seen in OS#3956, where a message is<br>received in osmo-sgsn gtp iface after having received a DeleteCtxAccept<br>message where pdp and associated cbp is freed. As a result, when new<br>confirmation arrives, it can still be matched against an old request and<br>be sent to upper layers providing an already freed cbp.<br><br>With this patch, since all queued messages belonging to that pdp are<br>dropped, confirmation won't find a match and be discarded in libgtp.<br><br>In order to be able to drop all req messages belonging to a pdp, a new list<br>is added to pdp_t and qmsg_t are added to that list when inserted into the per-gsn<br>req transmit queue. This way upon pdp free time it's simply a<br>matter of iterating over that list to remove all messages.<br><br>There's no need to do same for resp queue, and it'd be actually<br>counter-productive, because it wouldn't be possible to detect and<br>discard duplicates anymore after pdp ctx has been freed.<br><br>Related: OS#3956<br>Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a<br>---<br>M TODO-RELEASE<br>M gtp/gtp.c<br>M gtp/pdp.c<br>M gtp/pdp.h<br>M gtp/queue.c<br>M gtp/queue.h<br>6 files changed, 28 insertions(+), 1 deletion(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/TODO-RELEASE b/TODO-RELEASE</span><br><span>index d0852fc..73e3189 100644</span><br><span>--- a/TODO-RELEASE</span><br><span>+++ b/TODO-RELEASE</span><br><span>@@ -7,3 +7,5 @@</span><br><span> # If any interfaces have been added since the last public release: c:r:a + 1.</span><br><span> # If any interfaces have been removed or changed since the last public release: c:r:0.</span><br><span> #library what description / commit summary line</span><br><span style="color: hsl(120, 100%, 40%);">+libgtp queue.h struct qmsg_t got a new field: entry</span><br><span style="color: hsl(120, 100%, 40%);">+libgtp pdp.h struct pdp_t got a new field: qmsg_list_req</span><br><span>diff --git a/gtp/gtp.c b/gtp/gtp.c</span><br><span>index 94c3245..799f8a7 100644</span><br><span>--- a/gtp/gtp.c</span><br><span>+++ b/gtp/gtp.c</span><br><span>@@ -513,6 +513,8 @@</span><br><span> qmsg->cbp = cbp;</span><br><span> qmsg->type = ntoh8(packet->gtp0.h.type);</span><br><span> qmsg->fd = fd;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (pdp) /* echo requests are not pdp-bound */</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_add(&qmsg->entry, &pdp->qmsg_list_req);</span><br><span> }</span><br><span> gsn->seq_next++; /* Count up this time */</span><br><span> return 0;</span><br><span>@@ -697,6 +699,9 @@</span><br><span> qmsg->cbp = NULL;</span><br><span> qmsg->type = 0;</span><br><span> qmsg->fd = fd;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* No need to add to pdp list here, because even on pdp ctx free</span><br><span style="color: hsl(120, 100%, 40%);">+ we want to leave messages in queue_resp until timeout to</span><br><span style="color: hsl(120, 100%, 40%);">+ detect duplicates */</span><br><span> }</span><br><span> return 0;</span><br><span> }</span><br><span>diff --git a/gtp/pdp.c b/gtp/pdp.c</span><br><span>index d745916..eaef545 100644</span><br><span>--- a/gtp/pdp.c</span><br><span>+++ b/gtp/pdp.c</span><br><span>@@ -31,6 +31,7 @@</span><br><span> #include "pdp.h"</span><br><span> #include "gtp.h"</span><br><span> #include "lookupa.h"</span><br><span style="color: hsl(120, 100%, 40%);">+#include "queue.h"</span><br><span> </span><br><span> /* ***********************************************************</span><br><span> * Functions related to PDP storage</span><br><span>@@ -156,7 +157,7 @@</span><br><span> }</span><br><span> /* Default: Generate G-PDU sequence numbers on Tx */</span><br><span> (*pdp)->tx_gpdu_seq = true;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(120, 100%, 40%);">+ INIT_LLIST_HEAD(&(*pdp)->qmsg_list_req);</span><br><span> return 0;</span><br><span> }</span><br><span> }</span><br><span>@@ -165,7 +166,17 @@</span><br><span> </span><br><span> int pdp_freepdp(struct pdp_t *pdp)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct qmsg_t *qmsg, *qmsg2;</span><br><span> struct pdp_t *pdpa = pdp->gsn->pdpa;</span><br><span style="color: hsl(120, 100%, 40%);">+ int rc;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Remove all enqueued messages belonging to this pdp from req tx transmit</span><br><span style="color: hsl(120, 100%, 40%);">+ queue. queue_freemsg will call llist_del(). */</span><br><span style="color: hsl(120, 100%, 40%);">+ llist_for_each_entry_safe(qmsg, qmsg2, &pdp->qmsg_list_req, entry) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if ((rc = queue_freemsg(pdp->gsn->queue_req, qmsg)))</span><br><span style="color: hsl(120, 100%, 40%);">+ LOGP(DLGTP, LOGL_ERROR,</span><br><span style="color: hsl(120, 100%, 40%);">+ "Failed freeing qmsg from qmsg_list_req during pdp_freepdp()! %d\n", rc);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span> </span><br><span> pdp_tiddel(pdp);</span><br><span> </span><br><span>diff --git a/gtp/pdp.h b/gtp/pdp.h</span><br><span>index d64d394..fdfa824 100644</span><br><span>--- a/gtp/pdp.h</span><br><span>+++ b/gtp/pdp.h</span><br><span>@@ -17,6 +17,7 @@</span><br><span> #include <netinet/in.h></span><br><span> </span><br><span> #include <osmocom/core/defs.h></span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/linuxlist.h></span><br><span> </span><br><span> struct gsn_t;</span><br><span> </span><br><span>@@ -241,6 +242,8 @@</span><br><span> struct gsn_t *gsn; /* Back pointer to GSN where this pdp ctx belongs to */</span><br><span> </span><br><span> bool tx_gpdu_seq; /* Transmit (true) or suppress G-PDU sequence numbers */</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ struct llist_head qmsg_list_req; /* list of req qmsg_t in retrans queue belonging this pdp ctx */</span><br><span> };</span><br><span> </span><br><span> /* functions related to pdp_t management */</span><br><span>diff --git a/gtp/queue.c b/gtp/queue.c</span><br><span>index ce4713e..4c25913 100644</span><br><span>--- a/gtp/queue.c</span><br><span>+++ b/gtp/queue.c</span><br><span>@@ -172,6 +172,7 @@</span><br><span> } else {</span><br><span> *qmsg = &queue->qmsga[queue->next];</span><br><span> queue_seqset(queue, *qmsg, peer, seq);</span><br><span style="color: hsl(120, 100%, 40%);">+ INIT_LLIST_HEAD(&(*qmsg)->entry);</span><br><span> (*qmsg)->state = 1; /* Space taken */</span><br><span> (*qmsg)->this = queue->next;</span><br><span> (*qmsg)->next = -1; /* End of the queue */</span><br><span>@@ -206,6 +207,8 @@</span><br><span> return EOF; /* Not in queue */</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ llist_del(&qmsg->entry);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> queue_seqdel(queue, qmsg);</span><br><span> </span><br><span> if (qmsg->next == -1) /* Are we the last in queue? */</span><br><span>diff --git a/gtp/queue.h b/gtp/queue.h</span><br><span>index 76cb7be..9b0367b 100644</span><br><span>--- a/gtp/queue.h</span><br><span>+++ b/gtp/queue.h</span><br><span>@@ -17,6 +17,8 @@</span><br><span> #ifndef _QUEUE_H</span><br><span> #define _QUEUE_H</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+#include <osmocom/core/linuxlist.h></span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> #include "gtp.h"</span><br><span> </span><br><span> #define QUEUE_DEBUG 0 /* Print debug information */</span><br><span>@@ -39,6 +41,7 @@</span><br><span> int this; /* Pointer to myself */</span><br><span> time_t timeout; /* When do we retransmit this packet? */</span><br><span> int retrans; /* How many times did we retransmit this? */</span><br><span style="color: hsl(120, 100%, 40%);">+ struct llist_head entry; /* Listed with other qmsg_t belonging to a pdp_t->qmsg_list_req */</span><br><span> };</span><br><span> </span><br><span> struct queue_t {</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ggsn/+/15228">change 15228</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-ggsn/+/15228"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-ggsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id86d0b241454d3ad49c64c28087fd2710fa2d17a </div>
<div style="display:none"> Gerrit-Change-Number: 15228 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>