Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 May 2023 21:15:33 +0800
From:   Ruihan Li <lrh2000@pku.edu.cn>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Ruihan Li <lrh2000@pku.edu.cn>
Subject: Re: [PATCH v3 6/6] Bluetooth: Avoid recursion in hci_conn_unlink
Message-ID: <55zqfg4flsequzz3ho2xqy775b5y7yj6r7qlwd2qizlhrc37r6@36pnpkoa5b4y>
References: <20230502145737.140856-1-lrh2000@pku.edu.cn>
 <20230502145737.140856-7-lrh2000@pku.edu.cn>
 <CABBYNZJS3QLbCPizwaTaKmy13+34vNLFo2YMy2cMS3zwvVeZsw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CABBYNZJS3QLbCPizwaTaKmy13+34vNLFo2YMy2cMS3zwvVeZsw@mail.gmail.com>
Precedence: bulk

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