* [PATCH master 1/6] usb: gadget: implement and use system_partitions_get_null
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 2/6] usb: gadget: don't register UMS with empty function Ahmad Fatoum
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
system_partitions_get() clones the system partitions file list and
returns the copy. usb multi gadget code expects disabled gadgets to
have a NULL file list, not an empty one, so fastboot and DFU handle
this case. Add a new system_partitions_get_null helper that can be
used instead. This will be used for USB mass storage gadget as well
in a follow-up commit.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/fastboot.c | 4 +---
common/usbgadget.c | 4 +---
include/system-partitions.h | 8 ++++++++
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/common/fastboot.c b/common/fastboot.c
index 04a8573b4adb..f8ed40c86e00 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -920,9 +920,7 @@ struct file_list *get_fastboot_partitions(void)
{
if (fastboot_partitions && *fastboot_partitions)
return file_list_parse_null(fastboot_partitions);
- if (!system_partitions_empty())
- return system_partitions_get();
- return NULL;
+ return system_partitions_get_null();
}
static int fastboot_globalvars_init(void)
diff --git a/common/usbgadget.c b/common/usbgadget.c
index e8c9f7d236d3..92e486199556 100644
--- a/common/usbgadget.c
+++ b/common/usbgadget.c
@@ -27,9 +27,7 @@ static inline struct file_list *get_dfu_function(void)
{
if (dfu_function && *dfu_function)
return file_list_parse_null(dfu_function);
- if (!system_partitions_empty())
- return system_partitions_get();
- return NULL;
+ return system_partitions_get_null();
}
int usbgadget_register(const struct usbgadget_funcs *funcs)
diff --git a/include/system-partitions.h b/include/system-partitions.h
index 86de3612ccd8..e6d1a0f88bf3 100644
--- a/include/system-partitions.h
+++ b/include/system-partitions.h
@@ -2,6 +2,7 @@
#ifndef SYSTEM_PARTITIONS_H_
#define SYSTEM_PARTITIONS_H_
+#include <linux/types.h>
#include <file-list.h>
#ifdef CONFIG_SYSTEM_PARTITIONS
@@ -37,4 +38,11 @@ static inline bool system_partitions_empty(void)
#endif
+static inline struct file_list *system_partitions_get_null(void)
+{
+ if (system_partitions_empty())
+ return NULL;
+ return system_partitions_get();
+}
+
#endif
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH master 2/6] usb: gadget: don't register UMS with empty function
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 1/6] usb: gadget: implement and use system_partitions_get_null Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 3/6] usb: gadget: mass-storage: fix clean up of file descriptors Ahmad Fatoum
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
system_partitions_get clones the system partitions and passes it along.
DFU and Fastboot use system partitions as a fallback and pass along a
NULL file list if they are empty. This enables e.g. usbgadget -A '' to
work: No files are expored, but fastboot OEM commands are possible.
USB mass storage though does pass along an empty system partitions file
list instead of NULL, which leads to bind failure, because UMS gadget
refuses to bind with no LUNs. Detect this case.
Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/usbgadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usbgadget.c b/common/usbgadget.c
index 92e486199556..2ec6d9226cca 100644
--- a/common/usbgadget.c
+++ b/common/usbgadget.c
@@ -52,7 +52,7 @@ int usbgadget_register(const struct usbgadget_funcs *funcs)
opts->ums_opts.files = file_list_parse_null(funcs->ums_opts);
if (IS_ENABLED(CONFIG_USB_GADGET_MASS_STORAGE) && file_list_empty(opts->ums_opts.files)) {
file_list_free(opts->ums_opts.files);
- opts->ums_opts.files = system_partitions_get();
+ opts->ums_opts.files = system_partitions_get_null();
}
}
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH master 3/6] usb: gadget: mass-storage: fix clean up of file descriptors
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 1/6] usb: gadget: implement and use system_partitions_get_null Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 2/6] usb: gadget: don't register UMS with empty function Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 4/6] usb: gadget: mass-storage: reference count allocations used in bthread Ahmad Fatoum
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
When encountering an issue during bind time, we currently close
only the last opened file descriptor, leaking others when more
than one LUN was registered. Fix this.
Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/usb/gadget/f_mass_storage.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 753042125d14..a49ac7803337 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2419,7 +2419,7 @@ static int fsg_common_init(struct fsg_common *common,
struct usb_gadget *gadget = cdev->gadget;
struct file_list_entry *fentry;
struct fsg_buffhd *bh;
- int nluns, i, fd = -1, rc;
+ int nluns, i, rc;
ums_count = 0;
@@ -2436,17 +2436,20 @@ static int fsg_common_init(struct fsg_common *common,
file_list_for_each_entry(ums_files, fentry) {
unsigned flags = O_RDWR;
struct stat st;
+ int fd;
if (fentry->flags) {
pr_err("flags not supported\n");
- return -ENOSYS;
+ rc = -ENOSYS;
+ goto close;
}
fd = open(fentry->filename, flags);
if (fd < 0) {
pr_err("open('%s') failed: %pe\n",
fentry->filename, ERR_PTR(fd));
- return fd;
+ rc = fd;
+ goto close;
}
rc = fstat(fd, &st);
@@ -2543,7 +2546,8 @@ error_release:
common->state = FSG_STATE_TERMINATED; /* The thread is dead */
fsg_common_release(common);
close:
- close(fd);
+ for (i = 0; i < ums_count; i++)
+ close(ums[i].fd);
return rc;
}
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH master 4/6] usb: gadget: mass-storage: reference count allocations used in bthread
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
` (2 preceding siblings ...)
2022-03-19 11:02 ` [PATCH master 3/6] usb: gadget: mass-storage: fix clean up of file descriptors Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 5/6] usb: gadget: multi: fix broken handling of USB function bind error Ahmad Fatoum
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Since 997cca0f15dc ("bthread: replace blocking bthread_stop with
nonblocking bthread_cancel"), the bthread may survive longer than the
multigadget unbind. This didn't cause issues so far, because the multi
gadget unbind didn't call usb_put_function[_instance] for mass-storage
(but did so for other functions), so we just leaked the memory.
In preparation for fixing the memory leak, we will need to straighten
out the mass storage cleanup. We do so by reference counting the
two shared structures: If bthread runs before usb_put_function[_instance],
it will not free them yet (avoiding a double free) and if bthread runs
after usb_put_function[_instance], it will still be able to access them
(avoiding a use-after-free).
A cleaner way would've been to wait for bthread completion, but we can't
do that here, because gadget could be unbound in a poller and bthreads
are only scheduled in command context.
Fixes: 997cca0f15dc ("bthread: replace blocking bthread_stop with nonblocking bthread_cancel")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/usb/gadget/f_mass_storage.c | 52 ++++++++++++++++++++++++-----
include/usb/mass_storage.h | 1 +
2 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index a49ac7803337..1c26c4d99681 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -263,8 +263,6 @@ static struct usb_gadget_strings fsg_stringtab = {
struct bthread *thread_task;
-struct kref {int x; };
-
struct fsg_dev;
static struct file_list *ums_files;
@@ -282,6 +280,8 @@ struct fsg_common {
struct fsg_buffhd *next_buffhd_to_drain;
struct fsg_buffhd buffhds[FSG_NUM_BUFFERS];
+ struct f_ums_opts *opts;
+
int cmnd_size;
u8 cmnd[MAX_COMMAND_SIZE];
@@ -322,6 +322,20 @@ struct fsg_common {
char inquiry_string[8 + 16 + 4 + 1];
};
+static struct f_ums_opts *f_ums_opts_get(struct f_ums_opts *opts)
+{
+ opts->refcnt++;
+ return opts;
+}
+
+static void f_ums_opts_put(struct f_ums_opts *opts)
+{
+ if (--opts->refcnt == 0) {
+ kfree(opts->common);
+ kfree(opts);
+ }
+}
+
struct fsg_config {
unsigned nluns;
struct fsg_lun_config {
@@ -348,6 +362,8 @@ struct fsg_dev {
struct usb_gadget *gadget; /* Copy of cdev->gadget */
struct fsg_common *common;
+ int refcnt;
+
u16 interface_number;
unsigned int bulk_in_enabled:1;
@@ -360,6 +376,17 @@ struct fsg_dev {
struct usb_ep *bulk_out;
};
+static struct fsg_dev *fsg_dev_get(struct fsg_dev *fsg)
+{
+ fsg->refcnt++;
+ return fsg;
+}
+
+static void fsg_dev_put(struct fsg_dev *fsg)
+{
+ if (--fsg->refcnt == 0)
+ kfree(fsg);
+}
static inline int __fsg_is_set(struct fsg_common *common,
const char *func, unsigned line)
@@ -2337,12 +2364,14 @@ static void handle_exception(struct fsg_common *common)
static void fsg_main_thread(void *fsg_)
{
- struct fsg_dev *fsg = fsg_;
+ struct fsg_dev *fsg = fsg_dev_get(fsg_);
struct fsg_common *common = fsg->common;
+ struct f_ums_opts *opts = f_ums_opts_get(common->opts);
struct fsg_buffhd *bh;
unsigned i;
int ret = 0;
+
/* The main loop */
while (common->state != FSG_STATE_TERMINATED) {
if (exception_in_progress(common)) {
@@ -2394,11 +2423,14 @@ static void fsg_main_thread(void *fsg_)
ums_count = 0;
ums_files = NULL;
+
+ f_ums_opts_put(opts);
+ fsg_dev_put(fsg);
}
static void fsg_common_release(struct fsg_common *common);
-static struct fsg_common *fsg_common_setup(void)
+static struct fsg_common *fsg_common_setup(struct f_ums_opts *opts)
{
struct fsg_common *common;
@@ -2409,6 +2441,7 @@ static struct fsg_common *fsg_common_setup(void)
common->ops = NULL;
common->private_data = NULL;
+ common->opts = opts;
return common;
}
@@ -2659,7 +2692,7 @@ static void fsg_free(struct usb_function *f)
fsg = container_of(f, struct fsg_dev, function);
- kfree(fsg);
+ fsg_dev_put(fsg);
}
static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
@@ -2683,7 +2716,7 @@ static struct usb_function *fsg_alloc(struct usb_function_instance *fi)
fsg->function.free_func = fsg_free;
fsg->common = common;
- common->fsg = fsg;
+ common->fsg = fsg_dev_get(fsg);
return &fsg->function;
}
@@ -2692,8 +2725,7 @@ static void fsg_free_instance(struct usb_function_instance *fi)
{
struct f_ums_opts *opts = fsg_opts_from_func_inst(fi);
- kfree(opts->common);
- kfree(opts);
+ f_ums_opts_put(opts);
}
static struct usb_function_instance *fsg_alloc_inst(void)
@@ -2706,12 +2738,14 @@ static struct usb_function_instance *fsg_alloc_inst(void)
opts->func_inst.free_func_inst = fsg_free_instance;
- opts->common = fsg_common_setup();
+ opts->common = fsg_common_setup(opts);
if (!opts->common) {
free(opts);
return ERR_PTR(-ENOMEM);
}
+ f_ums_opts_get(opts);
+
return &opts->func_inst;
}
diff --git a/include/usb/mass_storage.h b/include/usb/mass_storage.h
index 084b3c8e8f31..7be665ee4729 100644
--- a/include/usb/mass_storage.h
+++ b/include/usb/mass_storage.h
@@ -20,6 +20,7 @@ struct f_ums_opts {
struct file_list *files;
unsigned int num_sectors;
int fd;
+ int refcnt;
char name[16];
};
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH master 5/6] usb: gadget: multi: fix broken handling of USB function bind error
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
` (3 preceding siblings ...)
2022-03-19 11:02 ` [PATCH master 4/6] usb: gadget: mass-storage: reference count allocations used in bthread Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-19 11:02 ` [PATCH master 6/6] usb: gadget: multi: free UMS instance at multi_unbind time Ahmad Fatoum
2022-03-28 8:53 ` [PATCH master 0/6] usb: gadget: multi: fix bind error path Sascha Hauer
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
If a function of a multi gadget fails, we run into multiple bugs:
- All gadget are unbound, even those which weren't bound yet
- We deallocate functions and function instances, but don't
remove them from USB configuration, which leads to
use-after-free when doing the composite unbind later on
The correct course of action here is to undo the function instance
allocation only, like Linux does. The rest will be cleaned up later
at composite gadget unbind time.
Fixes: bfb7aa1e1916 ("USB: gadget: Add a multi function gadget")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/usb/gadget/multi.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 0eb6d049d152..cd5b529d3eba 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -218,28 +218,28 @@ static int multi_bind(struct usb_composite_dev *cdev)
printf("%s: creating Fastboot function\n", __func__);
ret = multi_bind_fastboot(cdev);
if (ret)
- goto out;
+ return ret;
}
if (gadget_multi_opts->dfu_opts.files) {
printf("%s: creating DFU function\n", __func__);
ret = multi_bind_dfu(cdev);
if (ret)
- goto out;
+ goto unbind_fastboot;
}
if (gadget_multi_opts->ums_opts.files) {
printf("%s: creating USB Mass Storage function\n", __func__);
ret = multi_bind_ums(cdev);
if (ret)
- goto out;
+ goto unbind_dfu;
}
if (gadget_multi_opts->create_acm) {
printf("%s: creating ACM function\n", __func__);
ret = multi_bind_acm(cdev);
if (ret)
- goto out;
+ goto unbind_ums;
}
usb_ep_autoconfig_reset(cdev->gadget);
@@ -247,8 +247,15 @@ static int multi_bind(struct usb_composite_dev *cdev)
dev_info(&gadget->dev, DRIVER_DESC "\n");
return 0;
-out:
- multi_unbind(cdev);
+unbind_ums:
+ if (gadget_multi_opts->ums_opts.files)
+ usb_put_function_instance(fi_ums);
+unbind_dfu:
+ if (gadget_multi_opts->dfu_opts.files)
+ usb_put_function_instance(fi_dfu);
+unbind_fastboot:
+ if (gadget_multi_opts->fastboot_opts.files)
+ usb_put_function_instance(fi_fastboot);
return ret;
}
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH master 6/6] usb: gadget: multi: free UMS instance at multi_unbind time
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
` (4 preceding siblings ...)
2022-03-19 11:02 ` [PATCH master 5/6] usb: gadget: multi: fix broken handling of USB function bind error Ahmad Fatoum
@ 2022-03-19 11:02 ` Ahmad Fatoum
2022-03-28 8:53 ` [PATCH master 0/6] usb: gadget: multi: fix bind error path Sascha Hauer
6 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2022-03-19 11:02 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We do this for all other gadgets, but not for UMS. Fix this.
Fixes: 57313f83e83e ("usbgadget: add support for USB mass storage gadget")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/usb/gadget/multi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index cd5b529d3eba..102d8714f86d 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -169,6 +169,11 @@ static int multi_unbind(struct usb_composite_dev *cdev)
usb_put_function_instance(fi_acm);
}
+ if (gadget_multi_opts->ums_opts.files) {
+ usb_put_function(f_ums);
+ usb_put_function_instance(fi_ums);
+ }
+
if (gadget_multi_opts->dfu_opts.files) {
usb_put_function(f_dfu);
usb_put_function_instance(fi_dfu);
--
2.30.2
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master 0/6] usb: gadget: multi: fix bind error path
2022-03-19 11:02 [PATCH master 0/6] usb: gadget: multi: fix bind error path Ahmad Fatoum
` (5 preceding siblings ...)
2022-03-19 11:02 ` [PATCH master 6/6] usb: gadget: multi: free UMS instance at multi_unbind time Ahmad Fatoum
@ 2022-03-28 8:53 ` Sascha Hauer
6 siblings, 0 replies; 8+ messages in thread
From: Sascha Hauer @ 2022-03-28 8:53 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Sat, Mar 19, 2022 at 12:02:40PM +0100, Ahmad Fatoum wrote:
> USB mass storage gadget may fail during bind. USB multi gadget error
> path for failed bind is broken. USB mass storage gadget unbind leaks
> resources. Fix these three issues.
>
> Ahmad Fatoum (6):
> usb: gadget: implement and use system_partitions_get_null
> usb: gadget: don't register UMS with empty function
> usb: gadget: mass-storage: fix clean up of file descriptors
> usb: gadget: mass-storage: reference count allocations used in bthread
> usb: gadget: multi: fix broken handling of USB function bind error
> usb: gadget: multi: free UMS instance at multi_unbind time
Applied, thanks
Sascha
>
> common/fastboot.c | 4 +-
> common/usbgadget.c | 6 +--
> drivers/usb/gadget/f_mass_storage.c | 64 +++++++++++++++++++++++------
> drivers/usb/gadget/multi.c | 24 ++++++++---
> include/system-partitions.h | 8 ++++
> include/usb/mass_storage.h | 1 +
> 6 files changed, 81 insertions(+), 26 deletions(-)
>
> --
> 2.30.2
>
>
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 8+ messages in thread