| Message ID | 20230502145737.140856-7-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 | success | Gitlint PASS |
| 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: > > Previously, hci_conn_unlink was implemented as a recursion function. To > unlink physical connections (e.g. ACL/LE), it calls itself to unlink all > its logical channels (e.g. SCO/eSCO/ISO). > > Recursion is not required. This patch refactors hci_conn_unlink into two > functions, where hci_conn_unlink_parent takes a physical connection, > checks out all its logical channels, and calls hci_conn_unlink_child for > each logical channel to unlink it. > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > --- > net/bluetooth/hci_conn.c | 55 +++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 23 deletions(-) > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index de553e062..243d68a64 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -1074,34 +1074,13 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, > return conn; > } > > -static void hci_conn_unlink(struct hci_conn *conn) > +static void hci_conn_unlink_parent(struct hci_conn *conn) > { > struct hci_dev *hdev = conn->hdev; > > bt_dev_dbg(hdev, "hcon %p", conn); > > - if (!conn->parent) { > - struct hci_link *link, *t; > - > - list_for_each_entry_safe(link, t, &conn->link_list, list) { > - struct hci_conn *child = link->conn; > - > - hci_conn_unlink(child); > - > - /* Due to race, SCO connection might be not established > - * yet at this point. Delete it now, otherwise it is > - * possible for it to be stuck and can't be deleted. > - */ > - if ((child->type == SCO_LINK || > - child->type == ESCO_LINK) && > - child->handle == HCI_CONN_HANDLE_UNSET) > - hci_conn_del(child); > - } > - > - return; > - } > - > - if (!conn->link) > + if (WARN_ON(!conn->link)) > return; > > list_del_rcu(&conn->link->list); > @@ -1115,6 +1094,36 @@ static void hci_conn_unlink(struct hci_conn *conn) > conn->link = NULL; > } > > +static void hci_conn_unlink_children(struct hci_conn *conn) > +{ > + struct hci_dev *hdev = conn->hdev; > + struct hci_link *link, *t; > + > + bt_dev_dbg(hdev, "hcon %p", conn); > + > + list_for_each_entry_safe(link, t, &conn->link_list, list) { > + struct hci_conn *child = link->conn; > + > + hci_conn_unlink_parent(child); > + > + /* Due to race, SCO connection might be not established > + * yet at this point. Delete it now, otherwise it is > + * possible for it to be stuck and can't be deleted. > + */ > + if (child->type == SCO_LINK || child->type == ESCO_LINK) > + if (child->handle == HCI_CONN_HANDLE_UNSET) > + hci_conn_del(child); > + } This is not quite right, when we are unlinking the children's hci_conn it shall only unlink itself from the parent not everything. > +} > + > +static void hci_conn_unlink(struct hci_conn *conn) > +{ > + if (conn->parent) > + hci_conn_unlink_parent(conn); > + else > + hci_conn_unlink_children(conn); > +} > + > void hci_conn_del(struct hci_conn *conn) > { > struct hci_dev *hdev = conn->hdev; > -- > 2.40.0 >
Hi Luiz, On Tue, May 02, 2023 at 09:18:02AM -0700, Luiz Augusto von Dentz wrote: > Hi Ruihan, > > On Tue, May 2, 2023 at 7:57 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > > > Previously, hci_conn_unlink was implemented as a recursion function. To > > unlink physical connections (e.g. ACL/LE), it calls itself to unlink all > > its logical channels (e.g. SCO/eSCO/ISO). > > > > Recursion is not required. This patch refactors hci_conn_unlink into two > > functions, where hci_conn_unlink_parent takes a physical connection, > > checks out all its logical channels, and calls hci_conn_unlink_child for > > each logical channel to unlink it. > > > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > > --- > > net/bluetooth/hci_conn.c | 55 +++++++++++++++++++++++----------------- > > 1 file changed, 32 insertions(+), 23 deletions(-) > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index de553e062..243d68a64 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -1074,34 +1074,13 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, > > return conn; > > } > > > > -static void hci_conn_unlink(struct hci_conn *conn) > > +static void hci_conn_unlink_parent(struct hci_conn *conn) > > { > > struct hci_dev *hdev = conn->hdev; > > > > bt_dev_dbg(hdev, "hcon %p", conn); > > > > - if (!conn->parent) { > > - struct hci_link *link, *t; > > - > > - list_for_each_entry_safe(link, t, &conn->link_list, list) { > > - struct hci_conn *child = link->conn; > > - > > - hci_conn_unlink(child); > > - > > - /* Due to race, SCO connection might be not established > > - * yet at this point. Delete it now, otherwise it is > > - * possible for it to be stuck and can't be deleted. > > - */ > > - if ((child->type == SCO_LINK || > > - child->type == ESCO_LINK) && > > - child->handle == HCI_CONN_HANDLE_UNSET) > > - hci_conn_del(child); > > - } > > - > > - return; > > - } > > - > > - if (!conn->link) > > + if (WARN_ON(!conn->link)) > > return; > > > > list_del_rcu(&conn->link->list); > > @@ -1115,6 +1094,36 @@ static void hci_conn_unlink(struct hci_conn *conn) > > conn->link = NULL; > > } > > > > +static void hci_conn_unlink_children(struct hci_conn *conn) > > +{ > > + struct hci_dev *hdev = conn->hdev; > > + struct hci_link *link, *t; > > + > > + bt_dev_dbg(hdev, "hcon %p", conn); > > + > > + list_for_each_entry_safe(link, t, &conn->link_list, list) { > > + struct hci_conn *child = link->conn; > > + > > + hci_conn_unlink_parent(child); > > + > > + /* Due to race, SCO connection might be not established > > + * yet at this point. Delete it now, otherwise it is > > + * possible for it to be stuck and can't be deleted. > > + */ > > + if (child->type == SCO_LINK || child->type == ESCO_LINK) > > + if (child->handle == HCI_CONN_HANDLE_UNSET) > > + hci_conn_del(child); > > + } > > This is not quite right, when we are unlinking the children's hci_conn > it shall only unlink itself from the parent not everything. My assumption is that each hci_conn is either * a logical channel, which implies that it has a parent and no children, or * a physical link, which means that it has no parent and possibly some children. So here, as children of physical links, they must be logical channels and thus cannot have more children, so just unlinking them from the parent should be ok. We can add some assertions like WARN_ON(!conn->parent || !conn->link) // conn has a parent WARN_ON(!list_empty(&conn->link_list)) // conn has no children in hci_conn_unlink_parent. Do you think this assumption is wrong, or could become wrong in the future? Otherwise, I don't think there are correctness problems. In my opinion, separating the functions for logical channels and physical links makes the code a bit cleaner. But it's my opinion, if you think it actually makes the code harder to understand, I won't insist. > > > +} > > + > > +static void hci_conn_unlink(struct hci_conn *conn) > > +{ > > + if (conn->parent) > > + hci_conn_unlink_parent(conn); > > + else > > + hci_conn_unlink_children(conn); > > +} > > + > > void hci_conn_del(struct hci_conn *conn) > > { > > struct hci_dev *hdev = conn->hdev; > > -- > > 2.40.0 > > > > > -- > Luiz Augusto von Dentz Thanks, Ruihan Li
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index de553e062..243d68a64 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -1074,34 +1074,13 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, return conn; } -static void hci_conn_unlink(struct hci_conn *conn) +static void hci_conn_unlink_parent(struct hci_conn *conn) { struct hci_dev *hdev = conn->hdev; bt_dev_dbg(hdev, "hcon %p", conn); - if (!conn->parent) { - struct hci_link *link, *t; - - list_for_each_entry_safe(link, t, &conn->link_list, list) { - struct hci_conn *child = link->conn; - - hci_conn_unlink(child); - - /* Due to race, SCO connection might be not established - * yet at this point. Delete it now, otherwise it is - * possible for it to be stuck and can't be deleted. - */ - if ((child->type == SCO_LINK || - child->type == ESCO_LINK) && - child->handle == HCI_CONN_HANDLE_UNSET) - hci_conn_del(child); - } - - return; - } - - if (!conn->link) + if (WARN_ON(!conn->link)) return; list_del_rcu(&conn->link->list); @@ -1115,6 +1094,36 @@ static void hci_conn_unlink(struct hci_conn *conn) conn->link = NULL; } +static void hci_conn_unlink_children(struct hci_conn *conn) +{ + struct hci_dev *hdev = conn->hdev; + struct hci_link *link, *t; + + bt_dev_dbg(hdev, "hcon %p", conn); + + list_for_each_entry_safe(link, t, &conn->link_list, list) { + struct hci_conn *child = link->conn; + + hci_conn_unlink_parent(child); + + /* Due to race, SCO connection might be not established + * yet at this point. Delete it now, otherwise it is + * possible for it to be stuck and can't be deleted. + */ + if (child->type == SCO_LINK || child->type == ESCO_LINK) + if (child->handle == HCI_CONN_HANDLE_UNSET) + hci_conn_del(child); + } +} + +static void hci_conn_unlink(struct hci_conn *conn) +{ + if (conn->parent) + hci_conn_unlink_parent(conn); + else + hci_conn_unlink_children(conn); +} + void hci_conn_del(struct hci_conn *conn) { struct hci_dev *hdev = conn->hdev;
Previously, hci_conn_unlink was implemented as a recursion function. To unlink physical connections (e.g. ACL/LE), it calls itself to unlink all its logical channels (e.g. SCO/eSCO/ISO). Recursion is not required. This patch refactors hci_conn_unlink into two functions, where hci_conn_unlink_parent takes a physical connection, checks out all its logical channels, and calls hci_conn_unlink_child for each logical channel to unlink it. Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> --- net/bluetooth/hci_conn.c | 55 +++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 23 deletions(-)