drivers/usb/gadget/udc/core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
This commit adds a null pointer check for udc in gadget_match_driver to
prevent the below potential dangling pointer access. The issue arises
due to continuous USB role switch and simultaneous UDC write operations
performed by init.rc from user space through configfs. In these
scenarios, there was a possibility of usb_udc_release being done before
gadget_match_driver.
[27635.233849] BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
[27635.233871] Read of size 8 at addr d7ffff8837ead080 by task init/1
[27635.233881] Pointer tag: [d7], memory tag: [fe]
[27635.233888]
[27635.233917] Call trace:
[27635.233923] dump_backtrace+0xec/0x10c
[27635.233935] show_stack+0x18/0x24
[27635.233944] dump_stack_lvl+0x50/0x6c
[27635.233958] print_report+0x150/0x6b4
[27635.233977] kasan_report+0xe8/0x148
[27635.233985] __hwasan_load8_noabort+0x88/0x98
[27635.233995] gadget_match_driver+0x40/0x94
[27635.234005] __driver_attach+0x60/0x304
[27635.234018] bus_for_each_dev+0x154/0x1b4
[27635.234027] driver_attach+0x34/0x48
[27635.234036] bus_add_driver+0x1ec/0x310
[27635.234045] driver_register+0xc8/0x1b4
[27635.234055] usb_gadget_register_driver_owner+0x7c/0x140
[27635.234066] gadget_dev_desc_UDC_store+0x148/0x19c
[27635.234075] configfs_write_iter+0x180/0x1e0
[27635.234087] vfs_write+0x298/0x3e4
[27635.234105] ksys_write+0x88/0x100
[27635.234115] __arm64_sys_write+0x44/0x5c
[27635.234126] invoke_syscall+0x6c/0x17c
[27635.234143] el0_svc_common+0xf8/0x138
[27635.234154] do_el0_svc+0x30/0x40
[27635.234164] el0_svc+0x38/0x68
[27635.234174] el0t_64_sync_handler+0x68/0xbc
[27635.234184] el0t_64_sync+0x19c/0x1a0
Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
Cc: stable <stable@kernel.org>
Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
---
drivers/usb/gadget/udc/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index cf6478f97f4a..77dc0f28ff01 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
udc = container_of(dev, struct usb_udc, dev);
dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
kfree(udc);
+ udc = NULL;
}
static const struct attribute_group *usb_udc_attr_groups[];
@@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
struct usb_gadget_driver, driver);
/* If the driver specifies a udc_name, it must match the UDC's name */
- if (driver->udc_name &&
+ if (driver->udc_name && udc &&
strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
return 0;
--
2.17.1On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
> This commit adds a null pointer check for udc in gadget_match_driver to
> prevent the below potential dangling pointer access. The issue arises
> due to continuous USB role switch and simultaneous UDC write operations
> performed by init.rc from user space through configfs. In these
> scenarios, there was a possibility of usb_udc_release being done before
> gadget_match_driver.
>
> [27635.233849] BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
> [27635.233871] Read of size 8 at addr d7ffff8837ead080 by task init/1
> [27635.233881] Pointer tag: [d7], memory tag: [fe]
> [27635.233888]
> [27635.233917] Call trace:
> [27635.233923] dump_backtrace+0xec/0x10c
> [27635.233935] show_stack+0x18/0x24
> [27635.233944] dump_stack_lvl+0x50/0x6c
> [27635.233958] print_report+0x150/0x6b4
> [27635.233977] kasan_report+0xe8/0x148
> [27635.233985] __hwasan_load8_noabort+0x88/0x98
> [27635.233995] gadget_match_driver+0x40/0x94
> [27635.234005] __driver_attach+0x60/0x304
> [27635.234018] bus_for_each_dev+0x154/0x1b4
> [27635.234027] driver_attach+0x34/0x48
> [27635.234036] bus_add_driver+0x1ec/0x310
> [27635.234045] driver_register+0xc8/0x1b4
> [27635.234055] usb_gadget_register_driver_owner+0x7c/0x140
> [27635.234066] gadget_dev_desc_UDC_store+0x148/0x19c
> [27635.234075] configfs_write_iter+0x180/0x1e0
> [27635.234087] vfs_write+0x298/0x3e4
> [27635.234105] ksys_write+0x88/0x100
> [27635.234115] __arm64_sys_write+0x44/0x5c
> [27635.234126] invoke_syscall+0x6c/0x17c
> [27635.234143] el0_svc_common+0xf8/0x138
> [27635.234154] do_el0_svc+0x30/0x40
> [27635.234164] el0_svc+0x38/0x68
> [27635.234174] el0t_64_sync_handler+0x68/0xbc
> [27635.234184] el0t_64_sync+0x19c/0x1a0
>
> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> ---
> drivers/usb/gadget/udc/core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index cf6478f97f4a..77dc0f28ff01 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
> udc = container_of(dev, struct usb_udc, dev);
> dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
> kfree(udc);
> + udc = NULL;
That's not ok, as what happens if you race right between freeing it and
accessing it elsewhere?
> }
>
> static const struct attribute_group *usb_udc_attr_groups[];
> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
> struct usb_gadget_driver, driver);
>
> /* If the driver specifies a udc_name, it must match the UDC's name */
> - if (driver->udc_name &&
> + if (driver->udc_name && udc &&
I agree this isn't good, but you just made the window smaller, please
fix this properly.
thanks,
greg k-h
On Wed, Aug 28, 2024 at 11:39:58AM +0200, Greg KH wrote:
> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
> > This commit adds a null pointer check for udc in gadget_match_driver to
> > prevent the below potential dangling pointer access. The issue arises
> > due to continuous USB role switch and simultaneous UDC write operations
> > performed by init.rc from user space through configfs. In these
> > scenarios, there was a possibility of usb_udc_release being done before
> > gadget_match_driver.
> >
> > [27635.233849] BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
> > [27635.233871] Read of size 8 at addr d7ffff8837ead080 by task init/1
> > [27635.233881] Pointer tag: [d7], memory tag: [fe]
> > [27635.233888]
> > [27635.233917] Call trace:
> > [27635.233923] dump_backtrace+0xec/0x10c
> > [27635.233935] show_stack+0x18/0x24
> > [27635.233944] dump_stack_lvl+0x50/0x6c
> > [27635.233958] print_report+0x150/0x6b4
> > [27635.233977] kasan_report+0xe8/0x148
> > [27635.233985] __hwasan_load8_noabort+0x88/0x98
> > [27635.233995] gadget_match_driver+0x40/0x94
> > [27635.234005] __driver_attach+0x60/0x304
> > [27635.234018] bus_for_each_dev+0x154/0x1b4
> > [27635.234027] driver_attach+0x34/0x48
> > [27635.234036] bus_add_driver+0x1ec/0x310
> > [27635.234045] driver_register+0xc8/0x1b4
> > [27635.234055] usb_gadget_register_driver_owner+0x7c/0x140
> > [27635.234066] gadget_dev_desc_UDC_store+0x148/0x19c
> > [27635.234075] configfs_write_iter+0x180/0x1e0
> > [27635.234087] vfs_write+0x298/0x3e4
> > [27635.234105] ksys_write+0x88/0x100
> > [27635.234115] __arm64_sys_write+0x44/0x5c
> > [27635.234126] invoke_syscall+0x6c/0x17c
> > [27635.234143] el0_svc_common+0xf8/0x138
> > [27635.234154] do_el0_svc+0x30/0x40
> > [27635.234164] el0_svc+0x38/0x68
> > [27635.234174] el0t_64_sync_handler+0x68/0xbc
> > [27635.234184] el0t_64_sync+0x19c/0x1a0
> >
> > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> > ---
> > drivers/usb/gadget/udc/core.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index cf6478f97f4a..77dc0f28ff01 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
> > udc = container_of(dev, struct usb_udc, dev);
> > dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
> > kfree(udc);
> > + udc = NULL;
>
> That's not ok, as what happens if you race right between freeing it and
> accessing it elsewhere?
In fact, this assignment does nothing at all. This is at the end of the
function and udc is a local variable, so it's not going to be used
again. The compiler won't even generate any code for this.
> > }
> >
> > static const struct attribute_group *usb_udc_attr_groups[];
> > @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
> > struct usb_gadget_driver, driver);
> >
> > /* If the driver specifies a udc_name, it must match the UDC's name */
> > - if (driver->udc_name &&
> > + if (driver->udc_name && udc &&
>
> I agree this isn't good, but you just made the window smaller, please
> fix this properly.
I don't see how udc can possibly be NULL here. It gets initialized to a
non-NULL value when usb_add_gadget() does:
gadget->udc = udc;
and nothing changes its value thereafter. It seems much more likely
that the error shown above is an invalid pointer access because
gadget->udc points to a location that has been deallocated. Adding this
NULL check won't fix the bug.
Apparently the problem is caused by the fact that bus_for_each_dev(),
iterating over the things on the gadget bus, is still using gadget after
device_del(&gadget->dev);
in usb_del_gadget() returns and while
device_unregister(&udc->dev);
runs and the udc structure is deallocated. The only solution I can
think of is for the gadget to take a reference to the udc and drop the
reference when the gadget is released. Unfortunately, several UDC
drivers define their own gadget-release routines; they will all need to
be modified. And the core will need its own gadget-release routine for
use when the UDC driver does not specify its own.
Alan Stern
On 8/28/2024 8:24 PM, Alan Stern wrote:
> On Wed, Aug 28, 2024 at 11:39:58AM +0200, Greg KH wrote:
>> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
>>> This commit adds a null pointer check for udc in gadget_match_driver to
>>> prevent the below potential dangling pointer access. The issue arises
>>> due to continuous USB role switch and simultaneous UDC write operations
>>> performed by init.rc from user space through configfs. In these
>>> scenarios, there was a possibility of usb_udc_release being done before
>>> gadget_match_driver.
>>>
>>> [27635.233849] BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
>>> [27635.233871] Read of size 8 at addr d7ffff8837ead080 by task init/1
>>> [27635.233881] Pointer tag: [d7], memory tag: [fe]
>>> [27635.233888]
>>> [27635.233917] Call trace:
>>> [27635.233923] dump_backtrace+0xec/0x10c
>>> [27635.233935] show_stack+0x18/0x24
>>> [27635.233944] dump_stack_lvl+0x50/0x6c
>>> [27635.233958] print_report+0x150/0x6b4
>>> [27635.233977] kasan_report+0xe8/0x148
>>> [27635.233985] __hwasan_load8_noabort+0x88/0x98
>>> [27635.233995] gadget_match_driver+0x40/0x94
>>> [27635.234005] __driver_attach+0x60/0x304
>>> [27635.234018] bus_for_each_dev+0x154/0x1b4
>>> [27635.234027] driver_attach+0x34/0x48
>>> [27635.234036] bus_add_driver+0x1ec/0x310
>>> [27635.234045] driver_register+0xc8/0x1b4
>>> [27635.234055] usb_gadget_register_driver_owner+0x7c/0x140
>>> [27635.234066] gadget_dev_desc_UDC_store+0x148/0x19c
>>> [27635.234075] configfs_write_iter+0x180/0x1e0
>>> [27635.234087] vfs_write+0x298/0x3e4
>>> [27635.234105] ksys_write+0x88/0x100
>>> [27635.234115] __arm64_sys_write+0x44/0x5c
>>> [27635.234126] invoke_syscall+0x6c/0x17c
>>> [27635.234143] el0_svc_common+0xf8/0x138
>>> [27635.234154] do_el0_svc+0x30/0x40
>>> [27635.234164] el0_svc+0x38/0x68
>>> [27635.234174] el0t_64_sync_handler+0x68/0xbc
>>> [27635.234184] el0t_64_sync+0x19c/0x1a0
>>>
>>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>>> Cc: stable <stable@kernel.org>
>>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
>>> ---
>>> drivers/usb/gadget/udc/core.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>>> index cf6478f97f4a..77dc0f28ff01 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
>>> udc = container_of(dev, struct usb_udc, dev);
>>> dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>>> kfree(udc);
>>> + udc = NULL;
>> That's not ok, as what happens if you race right between freeing it and
>> accessing it elsewhere?
> In fact, this assignment does nothing at all. This is at the end of the
> function and udc is a local variable, so it's not going to be used
> again. The compiler won't even generate any code for this.
>
>>> }
>>>
>>> static const struct attribute_group *usb_udc_attr_groups[];
>>> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
>>> struct usb_gadget_driver, driver);
>>>
>>> /* If the driver specifies a udc_name, it must match the UDC's name */
>>> - if (driver->udc_name &&
>>> + if (driver->udc_name && udc &&
>> I agree this isn't good, but you just made the window smaller, please
>> fix this properly.
> I don't see how udc can possibly be NULL here. It gets initialized to a
> non-NULL value when usb_add_gadget() does:
>
> gadget->udc = udc;
>
> and nothing changes its value thereafter. It seems much more likely
> that the error shown above is an invalid pointer access because
> gadget->udc points to a location that has been deallocated. Adding this
> NULL check won't fix the bug.
>
> Apparently the problem is caused by the fact that bus_for_each_dev(),
> iterating over the things on the gadget bus, is still using gadget after
>
> device_del(&gadget->dev);
>
> in usb_del_gadget() returns and while
>
> device_unregister(&udc->dev);
>
> runs and the udc structure is deallocated. The only solution I can
> think of is for the gadget to take a reference to the udc and drop the
> reference when the gadget is released. Unfortunately, several UDC
> drivers define their own gadget-release routines; they will all need to
> be modified. And the core will need its own gadget-release routine for
> use when the UDC driver does not specify its own.
>
> Alan Stern
Hi Alan,
Thanks for your comments. I understand your suggestions. We already have
a similar reference check with the udc name before calling
usb_gadget_register_driver.
In the drivers/usb/gadget/configfs.c file, I am wondering if there might
be an issue with the check of udc_name before
usb_gadget_register_driver. This is the only way to allow
gadget_register to be called before releasing or unregistering an
existing udc. Do you think we need to add an additional check here,
referencing the UDC, to prevent gadget_register from being called before
the existing UDC is released?
drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
===========================================================
if (gi->composite.gadget_driver.udc_name) {
ret = -EBUSY;
goto err;
}
gi->composite.gadget_driver.udc_name = name;
ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
Thanks,
Selva
>
On Fri, Aug 30, 2024 at 06:16:12PM +0530, Selvarasu Ganesan wrote:
> Hi Alan,
>
> Thanks for your comments. I understand your suggestions. We already have
> a similar reference check with the udc name before calling
> usb_gadget_register_driver.
> In the drivers/usb/gadget/configfs.c file, I am wondering if there might
> be an issue with the check of udc_name before
> usb_gadget_register_driver. This is the only way to allow
> gadget_register to be called before releasing or unregistering an
> existing udc. Do you think we need to add an additional check here,
> referencing the UDC, to prevent gadget_register from being called before
> the existing UDC is released?
I don't understand what you're saying. There is no routine named
"gadget_register". (And there is no variable named "udc_name" in the
code below, although there is gi->composite.gadget_driver.udc_name --
but that's not a variable, it is a field in a structure.)
> drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
> ===========================================================
> if (gi->composite.gadget_driver.udc_name) {
> ret = -EBUSY;
> goto err;
> }
> gi->composite.gadget_driver.udc_name = name;
Are you talking about this check and assignment? Why do you think there
might be a problem here?
Are you worried that some UDC might be released while this code is
running? If that happens, why would it be a problem?
> ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
Alan Stern
On 8/31/2024 9:59 AM, Alan Stern wrote:
> On Fri, Aug 30, 2024 at 06:16:12PM +0530, Selvarasu Ganesan wrote:
>> Hi Alan,
>>
>> Thanks for your comments. I understand your suggestions. We already have
>> a similar reference check with the udc name before calling
>> usb_gadget_register_driver.
>> In the drivers/usb/gadget/configfs.c file, I am wondering if there might
>> be an issue with the check of udc_name before
>> usb_gadget_register_driver. This is the only way to allow
>> gadget_register to be called before releasing or unregistering an
>> existing udc. Do you think we need to add an additional check here,
>> referencing the UDC, to prevent gadget_register from being called before
>> the existing UDC is released?
> I don't understand what you're saying. There is no routine named
> "gadget_register". (And there is no variable named "udc_name" in the
> code below, although there is gi->composite.gadget_driver.udc_name --
> but that's not a variable, it is a field in a structure.)
>
>> drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
>> ===========================================================
>> if (gi->composite.gadget_driver.udc_name) {
>> ret = -EBUSY;
>> goto err;
>> }
>> gi->composite.gadget_driver.udc_name = name;
> Are you talking about this check and assignment? Why do you think there
> might be a problem here?
>
> Are you worried that some UDC might be released while this code is
> running? If that happens, why would it be a problem?
I am talking here based on the call traces, we are observing the
following call traces at the time of failures. One specific point of
interest is the gadget_match_driver() function, which is called as part
of the usb_gadget_register_driver() function. I am wondering how the
usb_gadget_register_driver() function allows the registration of a new
driver even when an existing same UDC is not releasing. One possibility
is that gi->composite.gadget_driver.udc_name becomes NULL before the UDC
is released. However, as of now, we do not have any evidence to support
this theory. We are still trying to reproduce the same issue with added
more debugging logs.
CPU0: (ROLE SWITCH DEVICE <-> HOST)
==================================
->usb_role_switch_set_role()
->dwc3_usb_role_switch_set()
->dwc3_set_mode()
->__dwc3_set_mode()
->dwc3_gadget_exit()
->usb_del_gadget()
->device_unregister()
->put_device(dev)
->usb_udc_release()
CPU1 (echo "<dwc3 device name>" > <path of udc
config>/config/usb_gadget/g1/UDC)
=================================================================================
->configfs_write_iter()
->gadget_dev_desc_UDC_store()
->usb_gadget_register_driver()
->driver_register()
->bus_add_driver()
->driver_attach()
->bus_for_each_dev()
->__driver_attach()
->gadget_match_driver()
>
>> ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
> Alan Stern
>
On 8/28/2024 3:09 PM, Greg KH wrote:
> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
>> This commit adds a null pointer check for udc in gadget_match_driver to
>> prevent the below potential dangling pointer access. The issue arises
>> due to continuous USB role switch and simultaneous UDC write operations
>> performed by init.rc from user space through configfs. In these
>> scenarios, there was a possibility of usb_udc_release being done before
>> gadget_match_driver.
>>
>> [27635.233849] BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
>> [27635.233871] Read of size 8 at addr d7ffff8837ead080 by task init/1
>> [27635.233881] Pointer tag: [d7], memory tag: [fe]
>> [27635.233888]
>> [27635.233917] Call trace:
>> [27635.233923] dump_backtrace+0xec/0x10c
>> [27635.233935] show_stack+0x18/0x24
>> [27635.233944] dump_stack_lvl+0x50/0x6c
>> [27635.233958] print_report+0x150/0x6b4
>> [27635.233977] kasan_report+0xe8/0x148
>> [27635.233985] __hwasan_load8_noabort+0x88/0x98
>> [27635.233995] gadget_match_driver+0x40/0x94
>> [27635.234005] __driver_attach+0x60/0x304
>> [27635.234018] bus_for_each_dev+0x154/0x1b4
>> [27635.234027] driver_attach+0x34/0x48
>> [27635.234036] bus_add_driver+0x1ec/0x310
>> [27635.234045] driver_register+0xc8/0x1b4
>> [27635.234055] usb_gadget_register_driver_owner+0x7c/0x140
>> [27635.234066] gadget_dev_desc_UDC_store+0x148/0x19c
>> [27635.234075] configfs_write_iter+0x180/0x1e0
>> [27635.234087] vfs_write+0x298/0x3e4
>> [27635.234105] ksys_write+0x88/0x100
>> [27635.234115] __arm64_sys_write+0x44/0x5c
>> [27635.234126] invoke_syscall+0x6c/0x17c
>> [27635.234143] el0_svc_common+0xf8/0x138
>> [27635.234154] do_el0_svc+0x30/0x40
>> [27635.234164] el0_svc+0x38/0x68
>> [27635.234174] el0t_64_sync_handler+0x68/0xbc
>> [27635.234184] el0t_64_sync+0x19c/0x1a0
>>
>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
>> ---
>> drivers/usb/gadget/udc/core.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index cf6478f97f4a..77dc0f28ff01 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
>> udc = container_of(dev, struct usb_udc, dev);
>> dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>> kfree(udc);
>> + udc = NULL;
> That's not ok, as what happens if you race right between freeing it and
> accessing it elsewhere?
Hi Greg,
Thanks for your comments.
Agree This race can occur at any time, and we are investigating the
possibility of this issue through the following call trace. In our
entire test sequence, the only place where we encounter UDC null is in
the gadget_match_driver. It seems difficult to use locking to prevent
this race, so we believe it would be acceptable to implement a NULL
pointer check. We would appreciate any alternative solutions you may
suggest.
CPU0: (ROLE SWITCH DEVICE -> HOST) CPU1 (echo "<dwc3 device name>" > UDC)
==============================================================================
VENDOR usb notify driver()
VENDOR USB glue driver() configfs_write_iter()
usb_role_switch_set_role() gadget_dev_desc_UDC_store()
dwc3_usb_role_switch_set() driver_register()
dwc3_set_mode() bus_add_driver()
__dwc3_set_mode() driver_attach()
dwc3_gadget_exit() bus_for_each_dev()
usb_put_gadget(dwc->gadget); __driver_attach()
usb_udc_release()
gadget_match_driver()
>
>> }
>>
>> static const struct attribute_group *usb_udc_attr_groups[];
>> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
>> struct usb_gadget_driver, driver);
>>
>> /* If the driver specifies a udc_name, it must match the UDC's name */
>> - if (driver->udc_name &&
>> + if (driver->udc_name && udc &&
> I agree this isn't good, but you just made the window smaller, please
> fix this properly.
>
> thanks,
>
> greg k-h
Sorry i did not understand on the above statement. Could you please
provide more details on this?. Please correct me if i am wrong, Based on
your statement, it seems that the time between the role switch and the
write UDC is shorter than what is required, and you believe that we need
to fix our glue driver itself where trigger the
usb_role_switch_set_role(). Is that correct understanding?
Thanks,
Selva
>
© 2016 - 2025 Red Hat, Inc.