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>
In-Reply-To: <20200420135112.6749-2-scerveau@collabora.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 20 Apr 2020 10:21:44 -0700
Message-ID: <CABBYNZJGFLEQoNqu=Z6pOxTzQ=jUpX1qxYu+rMZo4PyDW51QHQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] add hog ref before adding to instances
To:     =?UTF-8?Q?St=C3=A9phane_Cerveau?= <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 St=C3=A9phane,

On Mon, Apr 20, 2020 at 9:39 AM St=C3=A9phane Cerveau <scerveau@collabora.c=
om> 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 *n=
ame, 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(inst=
ance));

That sounds like like a double ref since bt_hog_new already does add a
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 *us=
er_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 product=
,
> @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, stru=
ct gatt_primary *primary)
>                         primary->range.end, find_included_cb, instance);
>
>         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(inst=
ance));
>  }
>
>  static void primary_cb(uint8_t status, GSList *services, void *user_data=
)
> --
> 2.17.1
>


--=20
Luiz Augusto von Dentz