Hi John,
On 11/18/22 10:04 PM, John Keeping wrote:
On Thu, Nov 17, 2022 at 11:05:48AM +0530, Udipto Goswami wrote:
Currently the function purge_configs_funcs isn't protected by any lock
So there is a potential race possible there id udc called composite_unbind
and say at the same time the configfs_composite_bind also got executed.
If bind fails and follows the error path, driver will end up calling the
purge_configfs_funcs again, resulting in double free from func_list.
Fix this by introducing mutex gi->lock to make sure serial execution
can be maintained. Also, usage of list_for_each_entry_safe is risky
here since the tmp variable is still valid for the next iteration, in
order to ensure list traversal is clean, use list_last_entry.
I don't understand how this can happen.
purge_configs_funcs() is called from the .bind and .unbind
usb_gadget_driver hooks, and those should not be able to run at the same
time - if bind has not succeeded, how does unbinding make sense?
Consider this,
we are going out of the composition i.e unbinding it,
lets say we have 7 functions and we were only at removing the 2 one
where in the meantime the bind process got called, the udc driver
doesn't make sure of this, depends on the controller when they call the
usb_add_gadget probably.
For dwc3 controller specifically, the mode will still be device so from
core.c will end up calling gadget_init() which in turn will call
usb_add_gadget which further does to check_pending_gadget_drivers,
eventually calling bind.
So, i don't see any serializing mechanism here which would stop the
execution of make the controller wait until the unbind is completed
right. Please correct me if i'm wrong.
Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
Signed-off-by: Udipto Goswami <quic_ugoswami@xxxxxxxxxxx>
---
drivers/usb/gadget/configfs.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 3a6b4926193e..f1ac87906897 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
{
struct usb_configuration *c;
+ mutex_lock(&gi->lock);
list_for_each_entry(c, &gi->cdev.configs, list) {
- struct usb_function *f, *tmp;
+ struct usb_function *f;
struct config_usb_cfg *cfg;
cfg = container_of(c, struct config_usb_cfg, c);
-
- list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
-
+ while (!list_empty(&c->functions)) {
+ f = list_last_entry(&c->functions, struct usb_function, list);
list_move(&f->list, &cfg->func_list);
if (f->unbind) {
dev_dbg(&gi->cdev.gadget->dev,
@@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
c->highspeed = 0;
c->fullspeed = 0;
}
+ mutex_unlock(&gi->lock);
}
static int configfs_composite_bind(struct usb_gadget *gadget,
--
2.17.1
Thanks,
-Udipto
