| Message ID | 20230502145737.140856-5-lrh2000@pku.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: Fix potential double free caused by hci_conn_unlink | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 13: B3 Line contains hard tab characters (\t): " hci_conn_unlink(conn) [conn->parent == NULL]" 14: B3 Line contains hard tab characters (\t): " -> hci_conn_unlink(child) [child->parent == conn]" 15: B3 Line contains hard tab characters (\t): " -> hci_conn_drop(child->parent)" 16: B3 Line contains hard tab characters (\t): " -> queue_delayed_work(&conn->disc_work)" |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Ruihan, On Tue, May 2, 2023 at 7:57 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > Since hci_conn_link invokes both hci_conn_get and hci_conn_hold, > hci_conn_unlink should perform both hci_conn_put and hci_conn_drop as > well. However, currently it performs only hci_conn_put. > > This patch makes hci_conn_unlink call hci_conn_drop as well, which > simplifies the logic in hci_conn_del a bit and may benefit future users > of hci_conn_unlink. But it is noted that this change additionally > implies that hci_conn_unlink can queue disc_work on conn itself, with > the following call stack: > > hci_conn_unlink(conn) [conn->parent == NULL] > -> hci_conn_unlink(child) [child->parent == conn] > -> hci_conn_drop(child->parent) > -> queue_delayed_work(&conn->disc_work) > > Queued disc_work after hci_conn_del can be spurious, so during the > process of hci_conn_del, it is necessary to make the call to > cancel_delayed_work(&conn->disc_work) after invoking hci_conn_unlink. > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > --- > net/bluetooth/hci_conn.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index eef148291..e76ebb50d 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -1105,6 +1105,7 @@ static void hci_conn_unlink(struct hci_conn *conn) > list_del_rcu(&conn->link->list); > synchronize_rcu(); > > + hci_conn_drop(conn->parent); > hci_conn_put(conn->parent); > conn->parent = NULL; > > @@ -1118,7 +1119,6 @@ void hci_conn_del(struct hci_conn *conn) > > BT_DBG("%s hcon %p handle %d", hdev->name, conn, conn->handle); > > - cancel_delayed_work_sync(&conn->disc_work); > cancel_delayed_work_sync(&conn->auto_accept_work); > cancel_delayed_work_sync(&conn->idle_work); > > @@ -1134,12 +1134,7 @@ void hci_conn_del(struct hci_conn *conn) > else > hdev->acl_cnt += conn->sent; > } else { > - struct hci_conn *acl = conn->parent; > - > - if (acl) { > - hci_conn_unlink(conn); > - hci_conn_drop(acl); > - } > + hci_conn_unlink(conn); > > /* Unacked ISO frames */ > if (conn->type == ISO_LINK) { > @@ -1152,6 +1147,11 @@ void hci_conn_del(struct hci_conn *conn) > } > } > > + /* hci_conn_unlink may trigger additional disc_work, so > + * ensure to perform cancelling after that. > + */ > + cancel_delayed_work_sync(&conn->disc_work); Just merge the change where hci_conn_del calls hci_conn_unlink unconditionally so we don't have to do this change independently just to revert later. > if (conn->amp_mgr) > amp_mgr_put(conn->amp_mgr); > > -- > 2.40.0 >
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index eef148291..e76ebb50d 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -1105,6 +1105,7 @@ static void hci_conn_unlink(struct hci_conn *conn) list_del_rcu(&conn->link->list); synchronize_rcu(); + hci_conn_drop(conn->parent); hci_conn_put(conn->parent); conn->parent = NULL; @@ -1118,7 +1119,6 @@ void hci_conn_del(struct hci_conn *conn) BT_DBG("%s hcon %p handle %d", hdev->name, conn, conn->handle); - cancel_delayed_work_sync(&conn->disc_work); cancel_delayed_work_sync(&conn->auto_accept_work); cancel_delayed_work_sync(&conn->idle_work); @@ -1134,12 +1134,7 @@ void hci_conn_del(struct hci_conn *conn) else hdev->acl_cnt += conn->sent; } else { - struct hci_conn *acl = conn->parent; - - if (acl) { - hci_conn_unlink(conn); - hci_conn_drop(acl); - } + hci_conn_unlink(conn); /* Unacked ISO frames */ if (conn->type == ISO_LINK) { @@ -1152,6 +1147,11 @@ void hci_conn_del(struct hci_conn *conn) } } + /* hci_conn_unlink may trigger additional disc_work, so + * ensure to perform cancelling after that. + */ + cancel_delayed_work_sync(&conn->disc_work); + if (conn->amp_mgr) amp_mgr_put(conn->amp_mgr);
Since hci_conn_link invokes both hci_conn_get and hci_conn_hold, hci_conn_unlink should perform both hci_conn_put and hci_conn_drop as well. However, currently it performs only hci_conn_put. This patch makes hci_conn_unlink call hci_conn_drop as well, which simplifies the logic in hci_conn_del a bit and may benefit future users of hci_conn_unlink. But it is noted that this change additionally implies that hci_conn_unlink can queue disc_work on conn itself, with the following call stack: hci_conn_unlink(conn) [conn->parent == NULL] -> hci_conn_unlink(child) [child->parent == conn] -> hci_conn_drop(child->parent) -> queue_delayed_work(&conn->disc_work) Queued disc_work after hci_conn_del can be spurious, so during the process of hci_conn_del, it is necessary to make the call to cancel_delayed_work(&conn->disc_work) after invoking hci_conn_unlink. Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> --- net/bluetooth/hci_conn.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)