add hog ref in hog_attach_instance

PATCH In review Stéphane Cerveau 2 participants 5 messages Apr 20, 2020 Bluetooth drivers

lore

Share & tools

lore

Stéphane Cerveau

author

2020-04-20 13:51 UTC #0 lore raw

cover add hog ref in hog_attach_instance

When I tried to connect a bluetooth gamepad 8718, I was experiencing a crash which can be reproduced always.

https://www.amazon.com/Controller-PowerLead-Wireless-Compatible-Android/dp/B07XJ37HPK

After investigation, it appears that instance was not reffed initially when added to the slist and double free was requested on this hog during the free of the slist (unref during a destroy_gatt_req).

Stéphane Cerveau (1):
add hog ref before adding to instances

 profiles/input/hog-lib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Signature · 1 line

2.17.1

Stéphane Cerveau

author

2020-04-20 13:51 UTC #1 lore raw

PATCH 1/1 add hog ref before adding to instances

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(-)

profiles/input/hog-lib.c +4 -4

index 9c5c814a7..b9b5d565c 100644
@@ -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 = g_slist_append(hog->instances, instance);
+	hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
 }
 
 static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data)
 {
 	struct bt_hog *hog = 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, struct gatt_primary *primary)
 			primary->range.end, find_included_cb, instance);
 
 	bt_hog_attach(instance, hog->attrib);
-	hog->instances = g_slist_append(hog->instances, instance);
+	hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
 }
 
 static void primary_cb(uint8_t status, GSList *services, void *user_data)

Signature · 1 line

2.17.1

Luiz Augusto von Dentz

maintainer

2020-04-20 17:21 UTC #2 lore raw

Re: [PATCH 1/1] add hog ref before adding to instances

Hi Stéphane,

Quoted 26 lines · Stéphane Cerveau

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(-)

profiles/input/hog-lib.c +1 -1

index 9c5c814a7..b9b5d565c 100644
@@ -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;

profiles/input/hog-lib.c static void hog_attach_instace(struct bt_hog *hog, · line 1376

Quoted 5 lines

profiles/input/hog-lib.c +1 -1

@@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *hog,
        if (!instance)
                return;
 
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));

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.

Quoted 23 lines

profiles/input/hog-lib.c +2 -3

 }
 
 static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data)
 {
        struct bt_hog *hog = 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, struct gatt_primary *primary)
                        primary->range.end, find_included_cb, instance);
 
        bt_hog_attach(instance, hog->attrib);
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
 }
 
 static void primary_cb(uint8_t status, GSList *services, void *user_data)
--

2.17.1

Signature · 1 line

Luiz Augusto von Dentz

scerveau

author

2020-04-20 18:37 UTC #3 lore raw

Re: [PATCH 1/1] add hog ref before adding to instances

Dear Luiz,

Thanks but see my comment below.

Quoted 32 lines · Luiz Augusto von Dentz

Hi Stéphane,

Quoted 29 lines · Stéphane Cerveau

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(-)

profiles/input/hog-lib.c +1 -1

index 9c5c814a7..b9b5d565c 100644
@@ -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;

Quoted 4 lines

profiles/input/hog-lib.c static void hog_attach_instace(struct bt_hog *hog, · line 1376
Quoted 2 lines
profiles/input/hog-lib.c +1 -1
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
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 method.

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.

Quoted 57 lines

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.
Quoted 23 lines
profiles/input/hog-lib.c +2 -3
  }
 
  static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data)
  {
         struct bt_hog *hog = 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, struct gatt_primary *primary)
                         primary->range.end, find_included_cb, instance);
 
         bt_hog_attach(instance, hog->attrib);
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
  }
 
  static void primary_cb(uint8_t status, GSList *services, void *user_data)
--
2.17.1

Luiz Augusto von Dentz

maintainer

2020-04-20 21:05 UTC #4 lore raw

Re: [PATCH 1/1] add hog ref before adding to instances

Hi Scerveau,

Quoted 38 lines · scerveau

Dear Luiz,
Thanks but see my comment below.
Quoted 32 lines · Luiz Augusto von Dentz
Hi Stéphane,
Quoted 29 lines · Stéphane Cerveau
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(-)
profiles/input/hog-lib.c +1 -1
index 9c5c814a7..b9b5d565c 100644
@@ -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;
 

Quoted 7 lines

Quoted 4 lines
profiles/input/hog-lib.c static void hog_attach_instace(struct bt_hog *hog, · line 1376
Quoted 2 lines
profiles/input/hog-lib.c +1 -1
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
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 method.

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);

Quoted 3 lines

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.

Quoted 59 lines

Quoted 59 lines
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.
Quoted 23 lines
profiles/input/hog-lib.c +2 -3
  }
 
  static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data)
  {
         struct bt_hog *hog = 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, struct gatt_primary *primary)
                         primary->range.end, find_included_cb, instance);
 
         bt_hog_attach(instance, hog->attrib);
-       hog->instances = g_slist_append(hog->instances, instance);
+       hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance));
  }
 
  static void primary_cb(uint8_t status, GSList *services, void *user_data)
--
2.17.1

Signature · 1 line

Luiz Augusto von Dentz