pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/34894?usp=email )
Change subject: Revert "trx_if: Allow calling trx_if_flush/close from within TRXC callback"
......................................................................
Revert "trx_if: Allow calling trx_if_flush/close from within TRXC callback"
This reverts commit 4444262a6ab1e1e231ea81c4ec990f1a1f571a1f.
This commit introduced several side effects:
- tcm is left out of the l1h->trx_ctrl_list and hence won't be ever
retransmitted.
- Since tcm is removed before rsp callback, the llist may become empty
and if somehwere in the rsp callback a new message is enqueud it will
be sent immediatelly, and will be retransmitted again when
trx_ctrl_read_cb() calls trx_ctrl_send() at the end.
Change-Id: Ideb2d08ac8a2902bceeabfb055c59c9a13dbe3c0
Related: OS#6020
---
M src/osmo-bts-trx/trx_if.c
1 file changed, 25 insertions(+), 9 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c
index fef8c22..3f9fc04 100644
--- a/src/osmo-bts-trx/trx_if.c
+++ b/src/osmo-bts-trx/trx_if.c
@@ -721,13 +721,6 @@
rsp.cb = tcm->cb;
- /* Remove command from list, save it to last_acked and remove previous
- * last_acked. Do it before calling callback to avoid user freeing tcm
- * pointer if flushing/closing the iface. */
- llist_del(&tcm->list);
- talloc_free(l1h->last_acked);
- l1h->last_acked = tcm;
-
/* check for response code */
rc = trx_ctrl_rx_rsp(l1h, &rsp, tcm);
if (rc == -EINVAL)
@@ -735,11 +728,15 @@
/* re-schedule last cmd in rc seconds time */
if (rc > 0) {
- if (!llist_empty(&l1h->trx_ctrl_list))
- osmo_timer_schedule(&l1h->trx_ctrl_timer, rc, 0);
+ osmo_timer_schedule(&l1h->trx_ctrl_timer, rc, 0);
return 0;
}
+ /* remove command from list, save it to last_acked and removed previous last_acked */
+ llist_del(&tcm->list);
+ talloc_free(l1h->last_acked);
+ l1h->last_acked = tcm;
+
trx_ctrl_send(l1h);
return 0;
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/34894?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ideb2d08ac8a2902bceeabfb055c59c9a13dbe3c0
Gerrit-Change-Number: 34894
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32552?usp=email )
Change subject: trx_if: Allow calling trx_if_flush/close from within TRXC callback (v2)
......................................................................
trx_if: Allow calling trx_if_flush/close from within TRXC callback (v2)
- If the llist is flushed during rx rsp callback, when the flow is
returned to trx_ctrl_read_cb() it would access tcm which was in the
llist and end up in use-after-free.
- We need to store state on whether code path is inside the read_cb in
order to:
-- Delay transmission of new message if callback calls trx_if_flush()
followed by trx_ctrl_send(), since the trx_ctrl_send() at the end of
trx_ctrl_read_cb would retransmit it again immediatelly.
-- Avoid accessing tcm pointer if the callback called trx_if_flush(),
since it has been freed.
Related: OS#6020
Change-Id: Ibdffa4644aa3a7d219452644d3e74b411734f1df
---
M src/osmo-bts-trx/l1_if.h
M src/osmo-bts-trx/trx_if.c
2 files changed, 52 insertions(+), 7 deletions(-)
Approvals:
daniel: Looks good to me, but someone else must approve
Jenkins Builder: Verified
pespin: Looks good to me, approved
diff --git a/src/osmo-bts-trx/l1_if.h b/src/osmo-bts-trx/l1_if.h
index 18d84c2..84fd4b5 100644
--- a/src/osmo-bts-trx/l1_if.h
+++ b/src/osmo-bts-trx/l1_if.h
@@ -120,6 +120,10 @@
struct llist_head trx_ctrl_list;
/* Latest RSPed cmd, used to catch duplicate RSPs from sent retransmissions */
struct trx_ctrl_msg *last_acked;
+ /* Whether the code path is in the middle of handling a received message. */
+ bool in_trx_ctrl_read_cb;
+ /* Whether the l1h->trx_ctrl_list was flushed by the callback handling a received message */
+ bool flushed_while_in_trx_ctrl_read_cb;
//struct gsm_bts_trx *trx;
struct phy_instance *phy_inst;
diff --git a/src/osmo-bts-trx/trx_if.c b/src/osmo-bts-trx/trx_if.c
index 3f9fc04..89078a3 100644
--- a/src/osmo-bts-trx/trx_if.c
+++ b/src/osmo-bts-trx/trx_if.c
@@ -269,8 +269,10 @@
tcm->cmd, tcm->params_len ? " " : "", tcm->params);
llist_add_tail(&tcm->list, &l1h->trx_ctrl_list);
- /* send message, if we didn't already have pending messages */
- if (prev == NULL)
+ /* send message, if we didn't already have pending messages.
+ * If we are in the rx_rsp callback code path, skip sending, the
+ * callback will do so when returning to it. */
+ if (prev == NULL && !l1h->in_trx_ctrl_read_cb)
trx_ctrl_send(l1h);
return 0;
@@ -673,6 +675,7 @@
struct trx_ctrl_rsp rsp;
int len, rc;
struct trx_ctrl_msg *tcm;
+ bool flushed;
len = recv(ofd->fd, buf, sizeof(buf) - 1, 0);
if (len <= 0)
@@ -722,21 +725,34 @@
rsp.cb = tcm->cb;
/* check for response code */
+ l1h->in_trx_ctrl_read_cb = true;
rc = trx_ctrl_rx_rsp(l1h, &rsp, tcm);
+ /* Reset state: */
+ flushed = l1h->flushed_while_in_trx_ctrl_read_cb;
+ l1h->flushed_while_in_trx_ctrl_read_cb = false;
+ l1h->in_trx_ctrl_read_cb = false;
+
if (rc == -EINVAL)
goto rsp_error;
/* re-schedule last cmd in rc seconds time */
if (rc > 0) {
- osmo_timer_schedule(&l1h->trx_ctrl_timer, rc, 0);
+ /* The queue may have been flushed in the trx_ctrl_rx_rsp(): */
+ if (!llist_empty(&l1h->trx_ctrl_list))
+ osmo_timer_schedule(&l1h->trx_ctrl_timer, rc, 0);
return 0;
}
- /* remove command from list, save it to last_acked and removed previous last_acked */
- llist_del(&tcm->list);
- talloc_free(l1h->last_acked);
- l1h->last_acked = tcm;
+ if (!flushed) {
+ /* Remove command from list, save it to last_acked and removed
+ * previous last_acked */
+ llist_del(&tcm->list);
+ talloc_free(l1h->last_acked);
+ l1h->last_acked = tcm;
+ } /* else: tcm was freed by trx_if_flush(), do not access it. */
+
+ /* Send next message waiting in the list: */
trx_ctrl_send(l1h);
return 0;
@@ -1224,6 +1240,10 @@
/* Tx queue is now empty, so there's no point in keeping the retrans timer armed: */
osmo_timer_del(&l1h->trx_ctrl_timer);
+
+ /* If we are in read_cb, signal to the returning code path that we freed the list. */
+ if (l1h->in_trx_ctrl_read_cb)
+ l1h->flushed_while_in_trx_ctrl_read_cb = true;
}
/*! close the TRX for given handle (data + control socket) */
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32552?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ibdffa4644aa3a7d219452644d3e74b411734f1df
Gerrit-Change-Number: 32552
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: fixeria <axilirator(a)gmail.com>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: fixeria, fixeria, laforge, pespin.
pespin has uploaded a new patch set (#6) to the change originally created by fixeria. ( https://gerrit.osmocom.org/c/osmo-bts/+/32552?usp=email )
The following approvals got outdated and were removed:
Code-Review+1 by fixeria, Verified+1 by Jenkins Builder
Change subject: trx_if: Allow calling trx_if_flush/close from within TRXC callback (v2)
......................................................................
trx_if: Allow calling trx_if_flush/close from within TRXC callback (v2)
- If the llist is flushed during rx rsp callback, when the flow is
returned to trx_ctrl_read_cb() it would access tcm which was in the
llist and end up in use-after-free.
- We need to store state on whether code path is inside the read_cb in
order to:
-- Delay transmission of new message if callback calls trx_if_flush()
followed by trx_ctrl_send(), since the trx_ctrl_send() at the end of
trx_ctrl_read_cb would retransmit it again immediatelly.
-- Avoid accessing tcm pointer if the callback called trx_if_flush(),
since it has been freed.
Related: OS#6020
Change-Id: Ibdffa4644aa3a7d219452644d3e74b411734f1df
---
M src/osmo-bts-trx/l1_if.h
M src/osmo-bts-trx/trx_if.c
2 files changed, 52 insertions(+), 7 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/52/32552/6
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32552?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ibdffa4644aa3a7d219452644d3e74b411734f1df
Gerrit-Change-Number: 32552
Gerrit-PatchSet: 6
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator(a)gmail.com>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <axilirator(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge, lynxis lazus.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/34884?usp=email )
Change subject: pySim-shell: don't get trapped in applications without file system
......................................................................
Patch Set 2:
(1 comment)
File pySim-shell.py:
https://gerrit.osmocom.org/c/pysim/+/34884/comment/e371d6fe_e4a0b750
PS2, Line 620: "a0000000871002", "a0000000871004"]:
> I like the approach in general, but I think we shouldn't open-code those AIDs explicitly here. […]
I have now changed the code so that it does no longer use the hardcoded identifiers. Also not "ADF.USIM" or "ADF.ISIM". (I still need to add ADF.HPSIM)
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/34884?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: Ia2fdd65f430c07acb1afdaf265d24c6928b654e0
Gerrit-Change-Number: 34884
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Wed, 25 Oct 2023 15:08:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: laforge, lynxis lazus.
Hello Jenkins Builder, laforge, lynxis lazus,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/pysim/+/34884?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by Jenkins Builder
Change subject: pySim-shell: don't get trapped in applications without file system
......................................................................
pySim-shell: don't get trapped in applications without file system
When we traverse the file system, we may also end up selecting
applications (ADF), which do not support an USIM/ISIM like file system.
This will leave us without the ability to select the MF (or any other
file) again. The only way out is to select the ISIM or USIM application
again to get the access to the file system again.
Related: OS#5418
Change-Id: Ia2fdd65f430c07acb1afdaf265d24c6928b654e0
---
M pySim-shell.py
1 file changed, 44 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/pysim refs/changes/84/34884/3
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/34884?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: Ia2fdd65f430c07acb1afdaf265d24c6928b654e0
Gerrit-Change-Number: 34884
Gerrit-PatchSet: 3
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-MessageType: newpatchset