| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6044a542-fbcd-0fe7-abd3-83f38b731ecc@amperemail.onmicrosoft.com>
Date: Wed, 14 Dec 2022 11:15:48 +0700
From: Chanh Nguyen <chanh@...eremail.onmicrosoft.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
Chanh Nguyen <chanh@...amperecomputing.com>
Cc: OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Dan Vacura <w36195@...orola.com>,
Jakob Koschel <jakobkoschel@...il.com>,
Alan Stern <stern@...land.harvard.edu>,
Vijayavardhan Vennapusa <vvreddy@...eaurora.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
Open Source Submission <patches@...erecomputing.com>,
Frank Li <Frank.Li@....com>, Rondreis <linhaoguo86@...il.com>
Subject: Re: [PATCH] USB: gadget: Add ID numbers to configfs-gadget driver
names
On 13/12/2022 14:22, Christophe JAILLET wrote:
> Le 13/12/2022 à 05:12, Chanh Nguyen a écrit :
>> It is unable to use configfs to attach more than one gadget. When
>> attaching the second gadget, it always fails and the kernel message
>> prints out:
>>
>> Error: Driver 'configfs-gadget' is already registered, aborting...
>> UDC core: g1: driver registration failed: -16
>>
>> This commit fixes the problem by a ".N" suffix added to each
>> configfs_gadget's driver name (where N is a unique ID number),
>> thus making the names distinct.
>>
>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>> Signed-off-by: Chanh Nguyen
>> <chanh-shex6MNQR2J/SfDzf78azzKzEDxYleXD@...lic.gmane.org>
>> ---
>> drivers/usb/gadget/configfs.c | 42 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/configfs.c
>> b/drivers/usb/gadget/configfs.c
>> index 96121d1c8df4..d8c5156ad777 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -3,6 +3,7 @@
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> +#include <linux/idr.h>
>> #include <linux/kstrtox.h>
>> #include <linux/nls.h>
>> #include <linux/usb/composite.h>
>> @@ -11,6 +12,16 @@
>> #include "u_f.h"
>> #include "u_os_desc.h"
>> +static DEFINE_IDA(driver_id_numbers);
>> +
>> +/*
>> + * Driver name has the form of "configfs-gadget.%d", where %d
>> + * is id allocated by ida_alloc(). The max value returns by
>> + * ida_alloc() is INT_MAX, in 64-bit system, it is about nine
>> + * quintillion: 19 digits in decimal. Set the max length to 35.
>> + */
>> +#define DRIVER_NAME_LENGTH_MAX 35
>
> Hi,
>
> if paranoiac, the final \0 seems to be missing in the max length
> computation, but see below.
Thanks CJ! Indeed, I have missed that.
>
>> +
>> int check_user_usb_string(const char *name,
>> struct usb_gadget_strings *stringtab_dev)
>> {
>> @@ -52,6 +63,9 @@ struct gadget_info {
>> char qw_sign[OS_STRING_QW_SIGN_LEN];
>> spinlock_t spinlock;
>> bool unbind;
>> +
>> + /* Make driver names unique */
>> + int driver_id_number;
>> };
>> static inline struct gadget_info *to_gadget_info(struct config_item
>> *item)
>> @@ -1582,6 +1596,8 @@ static struct config_group *gadgets_make(
>> const char *name)
>> {
>> struct gadget_info *gi;
>> + char *driver_name;
>> + int ret;
>> gi = kzalloc(sizeof(*gi), GFP_KERNEL);
>> if (!gi)
>> @@ -1623,6 +1639,21 @@ static struct config_group *gadgets_make(
>> gi->composite.gadget_driver = configfs_driver_template;
>> + ret = ida_alloc(&driver_id_numbers, GFP_KERNEL);
>> + if (ret < 0)
>> + goto err;
>> + gi->driver_id_number = ret;
>> +
>> + driver_name = kmalloc(DRIVER_NAME_LENGTH_MAX, GFP_KERNEL);
>> + if (!driver_name)
>> + goto out_free_driver_id_number;
>> +
>> + ret = scnprintf(driver_name, DRIVER_NAME_LENGTH_MAX,
>> + "configfs-gadget.%d", gi->driver_id_number);
>
>
> using kasprintf() looks simpler here.
> No need to kmalloc()+scnprintf(), and no need for DRIVER_NAME_LENGTH_MAX.
>
> Just my 2c,
>
> CJ
Thanks CJ for the review!
I've made some changes as below (in gadgets_make() to remove unnecessary
variables) and now trying to test it as much as possible. Will re-post
it as v2 if looks good soon.
static inline struct gadget_info *to_gadget_info(struct config_item *item)
@@ -1623,13 +1629,25 @@ static struct config_group *gadgets_make(
gi->composite.gadget_driver = configfs_driver_template;
+ gi->driver_id_number = ida_alloc(&driver_id_numbers, GFP_KERNEL);
+ if (gi->driver_id_number < 0)
+ goto err;
+
+ gi->composite.gadget_driver.driver.name =
+ kasprintf(GFP_KERNEL, "configfs-gadget.%d",
+ gi->driver_id_number);
+ if (!gi->composite.gadget_driver.driver.name)
+ goto out_free_driver_id_number;
+
gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
gi->composite.name = gi->composite.gadget_driver.function;
if (!gi->composite.gadget_driver.function)
- goto err;
+ goto out_free_driver_id_number;
return &gi->group;
+
+out_free_driver_id_number:
+ ida_free(&driver_id_numbers, gi->driver_id_number);
err:
kfree(gi);
return ERR_PTR(-ENOMEM);
>
>> + if (ret < 0)
>> + goto out_free_driver_name;
>> +
>> + gi->composite.gadget_driver.driver.name = driver_name;
>> gi->composite.gadget_driver.function = kstrdup(name, GFP_KERNEL);
>> gi->composite.name = gi->composite.gadget_driver.function;
>> @@ -1630,6 +1661,11 @@ static struct config_group *gadgets_make(
>> goto err;
>> return &gi->group;
>> +
>> +out_free_driver_name:
>> + kfree(driver_name);
>> +out_free_driver_id_number:
>> + ida_free(&driver_id_numbers, gi->driver_id_number);
>> err:
>> kfree(gi);
>> return ERR_PTR(-ENOMEM);
>> @@ -1637,6 +1673,12 @@ static struct config_group *gadgets_make(
>> static void gadgets_drop(struct config_group *group, struct
>> config_item *item)
>> {
>> + struct gadget_info *gi = to_gadget_info(item);
>> +
>> + mutex_lock(&gi->lock);
>> + kfree(gi->composite.gadget_driver.driver.name);
>> + ida_free(&driver_id_numbers, gi->driver_id_number);
>> + mutex_unlock(&gi->lock);
>> config_item_put(item);
>> }
>
Powered by blists - more mailing lists