Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200420135112.6749-1-scerveau@collabora.com> <20200420135112.6749-2-scerveau@collabora.com>
 <CABBYNZJGFLEQoNqu=Z6pOxTzQ=jUpX1qxYu+rMZo4PyDW51QHQ@mail.gmail.com> <ff94c0ef-5778-8f96-3e91-b4e6ddb66d29@collabora.com>
In-Reply-To: <ff94c0ef-5778-8f96-3e91-b4e6ddb66d29@collabora.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 20 Apr 2020 14:05:15 -0700
Message-ID: <CABBYNZKVTJf8tyn0JbEeR9s2K7TGyaUZRVYof5H0mJOMxQF_fw@mail.gmail.com>
Subject: Re: [PATCH 1/1] add hog ref before adding to instances
To:     scerveau <scerveau@collabora.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Scerveau,

On Mon, Apr 20, 2020 at 11:37 AM scerveau <scerveau@collabora.com> wrote:
>
> Dear Luiz,
>
> Thanks but see my comment below.
>
> On 20/4/20 19:21, Luiz Augusto von Dentz wrote:
> > Hi St=C3=A9phane,
> >
> > On Mon, Apr 20, 2020 at 9:39 AM St=C3=A9phane Cerveau <scerveau@collabo=
ra.com> wrote:
> >>
> >> To avoid a double hog free, need to add a ref
> >> when adding the hog to the slist.
> >>
> >> This bug has been reproduced with gamepad-8718
> >> which was connecting/disconnecting frantically.
> >>
> >> Fix also a typo in the method hog_attach_instance
> >> ---
> >>   profiles/input/hog-lib.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> >> index 9c5c814a7..b9b5d565c 100644
> >> --- a/profiles/input/hog-lib.c
> >> +++ b/profiles/input/hog-lib.c
> >> @@ -1357,7 +1357,7 @@ static struct bt_hog *hog_new(int fd, const char=
 *name, uint16_t vendor,
> >>          return hog;
> >>   }
> >>
> >> -static void hog_attach_instace(struct bt_hog *hog,
> >> +static void hog_attach_instance(struct bt_hog *hog,
> >>                                  struct gatt_db_attribute *attr)
> >>   {
> >>          struct bt_hog *instance;
> >> @@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *=
hog,
> >>          if (!instance)
> >>                  return;
> >>
> >> -       hog->instances =3D g_slist_append(hog->instances, instance);
> >> +       hog->instances =3D g_slist_append(hog->instances, bt_hog_ref(i=
nstance));
> >
> > That sounds like like a double ref since bt_hog_new already does add a
>
> Yes but in `hog_attach_instance`, `hog_new` is used and not `bt_hog_new`
> which is indeed reffing. So I dont think there is double ref in this meth=
od.

Yep, you are rigth it is not adding a reference, but that seems on
purpose since on it doesn't use bt_hog_unref:

g_slist_free_full(hog->instances, hog_free);

> Unfortunately when I was preparing the patch, I noticed that another
> instance was added to slist and here you are totally right there is a
> double reference. I will remove this from the initial patch.

Right, I see now that is really missing the ref because
destroy_gatt_req does actually unref its own reference but since we
use hog_new that would destroy the instance so adding the ref should
be the right way to fix this.

>
>
> > reference, so chances are that you may be fixing the the symptom not
> > the real problem which seems that we cannot unref the instances
> > because they are not removed from the parent as it they should, in
> > fact we might need to redesign it a little bit since bt_hog might
> > actually be inefficient when there are multiple instances as it does
> > allocate a new uhid instance of each of then so we might do something
> > like:
> >
> > struct bt_hog_instance {
> >      struct bt_hog *parent;
> >      struct gatt_db_attribute *attr;
> >      struct gatt_primary *primary;
> >      GAttrib *attrib;
> >      GSList *reports;
> >      gboolean has_report_id;
> >      uint16_t bcdhid;
> >      uint8_t bcountrycode;
> >      uint16_t proto_mode_handle;
> >      uint16_t ctrlpt_handle;
> >      uint8_t flags;
> >      unsigned int getrep_att;
> >      uint16_t getrep_id;
> >      unsigned int setrep_att;
> >      uint16_t setrep_id;
> > }
> >
> > That way the instances are indenpendent of the bt_hog which should be
> > 1:1 to a device, while we can have multple instances of
> > bt_hog_instance, if you don't have time to do the rework then lets
> > just add a parent pointer added to bt_hog so we can can remove child
> > instances and resolve the double free.
> >
> >>   }
> >>
> >>   static void foreach_hog_service(struct gatt_db_attribute *attr, void=
 *user_data)
> >>   {
> >>          struct bt_hog *hog =3D user_data;
> >>
> >> -       hog_attach_instace(hog, attr);
> >> +       hog_attach_instance(hog, attr);
> >>   }
> >>
> >>   static void dis_notify(uint8_t source, uint16_t vendor, uint16_t pro=
duct,
> >> @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, s=
truct gatt_primary *primary)
> >>                          primary->range.end, find_included_cb, instanc=
e);
> >>
> >>          bt_hog_attach(instance, hog->attrib);
> >> -       hog->instances =3D g_slist_append(hog->instances, instance);
> >> +       hog->instances =3D g_slist_append(hog->instances, bt_hog_ref(i=
nstance));
> >>   }
> >>
> >>   static void primary_cb(uint8_t status, GSList *services, void *user_=
data)
> >> --
> >> 2.17.1
> >>
> >
> >


--=20
Luiz Augusto von Dentz