Several types of kernel panics can occur due to timing during the uvc
gadget removal. This appears to be a problem with gadget resources being
managed by both the client application's v4l2 open/close and the UDC
gadget bind/unbind. Since the concept of USB_GADGET_DELAYED_STATUS
doesn't exist for unbind, add a wait to allow for the application to
close out.
Some examples of the panics that can occur are:
<1>[ 1147.652313] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000028
<4>[ 1147.652510] Call trace:
<4>[ 1147.652514] usb_gadget_disconnect+0x74/0x1f0
<4>[ 1147.652516] usb_gadget_deactivate+0x38/0x168
<4>[ 1147.652520] usb_function_deactivate+0x54/0x90
<4>[ 1147.652524] uvc_function_disconnect+0x14/0x38
<4>[ 1147.652527] uvc_v4l2_release+0x34/0xa0
<4>[ 1147.652537] __fput+0xdc/0x2c0
<4>[ 1147.652540] ____fput+0x10/0x1c
<4>[ 1147.652545] task_work_run+0xe4/0x12c
<4>[ 1147.652549] do_notify_resume+0x108/0x168
<1>[ 282.950561][ T1472] Unable to handle kernel NULL pointer
dereference at virtual address 00000000000005b8
<6>[ 282.953111][ T1472] Call trace:
<6>[ 282.953121][ T1472] usb_function_deactivate+0x54/0xd4
<6>[ 282.953134][ T1472] uvc_v4l2_release+0xac/0x1e4
<6>[ 282.953145][ T1472] v4l2_release+0x134/0x1f0
<6>[ 282.953167][ T1472] __fput+0xf4/0x428
<6>[ 282.953178][ T1472] ____fput+0x14/0x24
<6>[ 282.953193][ T1472] task_work_run+0xac/0x130
<3>[ 213.410077][ T29] configfs-gadget gadget: uvc: Failed to queue
request (-108).
<1>[ 213.410116][ T29] Unable to handle kernel NULL pointer
dereference at virtual address 0000000000000003
<6>[ 213.413460][ T29] Call trace:
<6>[ 213.413474][ T29] uvcg_video_pump+0x1f0/0x384
<6>[ 213.413489][ T29] process_one_work+0x2a4/0x544
<6>[ 213.413502][ T29] worker_thread+0x350/0x784
<6>[ 213.413515][ T29] kthread+0x2ac/0x320
<6>[ 213.413528][ T29] ret_from_fork+0x10/0x30
Signed-off-by: Dan Vacura <[email protected]>
---
drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++++++++++++++
drivers/usb/gadget/function/uvc.h | 2 ++
drivers/usb/gadget/function/uvc_v4l2.c | 3 ++-
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 50e6e7a58b41..3cc8cf24a7c7 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
{
struct usb_composite_dev *cdev = c->cdev;
struct uvc_device *uvc = to_uvc(f);
+ int wait_ret = 1;
uvcg_info(f, "%s()\n", __func__);
+ /* If we know we're connected via v4l2, then there should be a cleanup
+ * of the device from userspace either via UVC_EVENT_DISCONNECT or
+ * though the video device removal uevent. Allow some time for the
+ * application to close out before things get deleted.
+ */
+ if (uvc->func_connected) {
+ uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
+ wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
+ uvc->func_connected == false, msecs_to_jiffies(500));
+ uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
+ }
+
device_remove_file(&uvc->vdev.dev, &dev_attr_function_name);
video_unregister_device(&uvc->vdev);
v4l2_device_unregister(&uvc->v4l2_dev);
+ if (uvc->func_connected) {
+ /* Wait for the release to occur to ensure there are no longer any
+ * pending operations that may cause panics when resources are cleaned
+ * up.
+ */
+ uvcg_warn(f, "%s no clean disconnect, wait for release\n", __func__);
+ wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
+ uvc->func_connected == false, msecs_to_jiffies(1000));
+ }
+
usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
kfree(uvc->control_buf);
@@ -917,6 +940,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
mutex_init(&uvc->video.mutex);
uvc->state = UVC_STATE_DISCONNECTED;
+ init_waitqueue_head(&uvc->func_connected_queue);
opts = fi_to_f_uvc_opts(fi);
mutex_lock(&opts->lock);
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 9eec104b3666..58e383afdd44 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -14,6 +14,7 @@
#include <linux/spinlock.h>
#include <linux/usb/composite.h>
#include <linux/videodev2.h>
+#include <linux/wait.h>
#include <media/v4l2-device.h>
#include <media/v4l2-dev.h>
@@ -130,6 +131,7 @@ struct uvc_device {
struct usb_function func;
struct uvc_video video;
bool func_connected;
+ wait_queue_head_t func_connected_queue;
/* Descriptors */
struct {
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index a2c78690c5c2..fd8f73bb726d 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -253,10 +253,11 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
static void uvc_v4l2_disable(struct uvc_device *uvc)
{
- uvc->func_connected = false;
uvc_function_disconnect(uvc);
uvcg_video_enable(&uvc->video, 0);
uvcg_free_buffers(&uvc->video.queue);
+ uvc->func_connected = false;
+ wake_up_interruptible(&uvc->func_connected_queue);
}
static int
--
2.32.0
On Fri, Apr 29, 2022 at 02:20:01PM -0500, Dan Vacura wrote:
> Several types of kernel panics can occur due to timing during the uvc
> gadget removal. This appears to be a problem with gadget resources being
> managed by both the client application's v4l2 open/close and the UDC
> gadget bind/unbind. Since the concept of USB_GADGET_DELAYED_STATUS
> doesn't exist for unbind, add a wait to allow for the application to
> close out.
>
> Some examples of the panics that can occur are:
>
> <1>[ 1147.652313] Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000028
> <4>[ 1147.652510] Call trace:
> <4>[ 1147.652514] usb_gadget_disconnect+0x74/0x1f0
> <4>[ 1147.652516] usb_gadget_deactivate+0x38/0x168
> <4>[ 1147.652520] usb_function_deactivate+0x54/0x90
> <4>[ 1147.652524] uvc_function_disconnect+0x14/0x38
> <4>[ 1147.652527] uvc_v4l2_release+0x34/0xa0
> <4>[ 1147.652537] __fput+0xdc/0x2c0
> <4>[ 1147.652540] ____fput+0x10/0x1c
> <4>[ 1147.652545] task_work_run+0xe4/0x12c
> <4>[ 1147.652549] do_notify_resume+0x108/0x168
>
> <1>[ 282.950561][ T1472] Unable to handle kernel NULL pointer
> dereference at virtual address 00000000000005b8
> <6>[ 282.953111][ T1472] Call trace:
> <6>[ 282.953121][ T1472] usb_function_deactivate+0x54/0xd4
> <6>[ 282.953134][ T1472] uvc_v4l2_release+0xac/0x1e4
> <6>[ 282.953145][ T1472] v4l2_release+0x134/0x1f0
> <6>[ 282.953167][ T1472] __fput+0xf4/0x428
> <6>[ 282.953178][ T1472] ____fput+0x14/0x24
> <6>[ 282.953193][ T1472] task_work_run+0xac/0x130
>
> <3>[ 213.410077][ T29] configfs-gadget gadget: uvc: Failed to queue
> request (-108).
> <1>[ 213.410116][ T29] Unable to handle kernel NULL pointer
> dereference at virtual address 0000000000000003
> <6>[ 213.413460][ T29] Call trace:
> <6>[ 213.413474][ T29] uvcg_video_pump+0x1f0/0x384
> <6>[ 213.413489][ T29] process_one_work+0x2a4/0x544
> <6>[ 213.413502][ T29] worker_thread+0x350/0x784
> <6>[ 213.413515][ T29] kthread+0x2ac/0x320
> <6>[ 213.413528][ T29] ret_from_fork+0x10/0x30
>
> Signed-off-by: Dan Vacura <[email protected]>
> ---
> drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++++++++++++++
> drivers/usb/gadget/function/uvc.h | 2 ++
> drivers/usb/gadget/function/uvc_v4l2.c | 3 ++-
> 3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 50e6e7a58b41..3cc8cf24a7c7 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
> {
> struct usb_composite_dev *cdev = c->cdev;
> struct uvc_device *uvc = to_uvc(f);
> + int wait_ret = 1;
>
> uvcg_info(f, "%s()\n", __func__);
Ick, wait, is that in the kernel? That needs to be removed, ftrace can
do that for you.
> + /* If we know we're connected via v4l2, then there should be a cleanup
> + * of the device from userspace either via UVC_EVENT_DISCONNECT or
> + * though the video device removal uevent. Allow some time for the
> + * application to close out before things get deleted.
> + */
> + if (uvc->func_connected) {
> + uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
> + wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
> + uvc->func_connected == false, msecs_to_jiffies(500));
> + uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
Please remove debugging code before submitting patches.
thanks,
greg k-h
On Sun, May 01, 2022 at 11:11:36PM -0500, Dan Vacura wrote:
> On Sat, Apr 30, 2022 at 09:56:50AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 29, 2022 at 02:20:01PM -0500, Dan Vacura wrote:
> > > Several types of kernel panics can occur due to timing during the uvc
> > > gadget removal. This appears to be a problem with gadget resources being
> > > managed by both the client application's v4l2 open/close and the UDC
> > > gadget bind/unbind. Since the concept of USB_GADGET_DELAYED_STATUS
> > > doesn't exist for unbind, add a wait to allow for the application to
> > > close out.
> > >
> > > Some examples of the panics that can occur are:
> > >
> > > <1>[ 1147.652313] Unable to handle kernel NULL pointer dereference at
> > > virtual address 0000000000000028
> > > <4>[ 1147.652510] Call trace:
> > > <4>[ 1147.652514] usb_gadget_disconnect+0x74/0x1f0
> > > <4>[ 1147.652516] usb_gadget_deactivate+0x38/0x168
> > > <4>[ 1147.652520] usb_function_deactivate+0x54/0x90
> > > <4>[ 1147.652524] uvc_function_disconnect+0x14/0x38
> > > <4>[ 1147.652527] uvc_v4l2_release+0x34/0xa0
> > > <4>[ 1147.652537] __fput+0xdc/0x2c0
> > > <4>[ 1147.652540] ____fput+0x10/0x1c
> > > <4>[ 1147.652545] task_work_run+0xe4/0x12c
> > > <4>[ 1147.652549] do_notify_resume+0x108/0x168
> > >
> > > <1>[ 282.950561][ T1472] Unable to handle kernel NULL pointer
> > > dereference at virtual address 00000000000005b8
> > > <6>[ 282.953111][ T1472] Call trace:
> > > <6>[ 282.953121][ T1472] usb_function_deactivate+0x54/0xd4
> > > <6>[ 282.953134][ T1472] uvc_v4l2_release+0xac/0x1e4
> > > <6>[ 282.953145][ T1472] v4l2_release+0x134/0x1f0
> > > <6>[ 282.953167][ T1472] __fput+0xf4/0x428
> > > <6>[ 282.953178][ T1472] ____fput+0x14/0x24
> > > <6>[ 282.953193][ T1472] task_work_run+0xac/0x130
> > >
> > > <3>[ 213.410077][ T29] configfs-gadget gadget: uvc: Failed to queue
> > > request (-108).
> > > <1>[ 213.410116][ T29] Unable to handle kernel NULL pointer
> > > dereference at virtual address 0000000000000003
> > > <6>[ 213.413460][ T29] Call trace:
> > > <6>[ 213.413474][ T29] uvcg_video_pump+0x1f0/0x384
> > > <6>[ 213.413489][ T29] process_one_work+0x2a4/0x544
> > > <6>[ 213.413502][ T29] worker_thread+0x350/0x784
> > > <6>[ 213.413515][ T29] kthread+0x2ac/0x320
> > > <6>[ 213.413528][ T29] ret_from_fork+0x10/0x30
> > >
> > > Signed-off-by: Dan Vacura <[email protected]>
> > > ---
> > > drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++++++++++++++
> > > drivers/usb/gadget/function/uvc.h | 2 ++
> > > drivers/usb/gadget/function/uvc_v4l2.c | 3 ++-
> > > 3 files changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > > index 50e6e7a58b41..3cc8cf24a7c7 100644
> > > --- a/drivers/usb/gadget/function/f_uvc.c
> > > +++ b/drivers/usb/gadget/function/f_uvc.c
> > > @@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
> > > {
> > > struct usb_composite_dev *cdev = c->cdev;
> > > struct uvc_device *uvc = to_uvc(f);
> > > + int wait_ret = 1;
> > >
> > > uvcg_info(f, "%s()\n", __func__);
> >
> > Ick, wait, is that in the kernel? That needs to be removed, ftrace can
> > do that for you.
>
> Yes, part of the kernel, and tbh, I find it to be quite helpful in
> debugging field issues from customers, where enabling ftrace isn't
> practical.
Why isn't ftrace ok to enable in a running kernel?
Worst case, this should be dev_dbg(), right?
> If you still want to remove, there are other locations in
> this gadget driver that log function entry. Perhaps it'd be better to
> do a separate change that cleans up logging a bit or do you prefer to
> just refactor this one now?
This commit is fine, it's a separate issue, I just noticed it as it was
in the context of this change.
> > > + /* If we know we're connected via v4l2, then there should be a cleanup
> > > + * of the device from userspace either via UVC_EVENT_DISCONNECT or
> > > + * though the video device removal uevent. Allow some time for the
> > > + * application to close out before things get deleted.
> > > + */
> > > + if (uvc->func_connected) {
> > > + uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
> > > + wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
> > > + uvc->func_connected == false, msecs_to_jiffies(500));
> > > + uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
> >
> > Please remove debugging code before submitting patches.
>
> Will do.
But this should be removed :)
Feel free to change it to dev_dbg(), which gives you the __func__
automatically without anything extra needed.
thanks,
greg k-h
On Sat, Apr 30, 2022 at 09:56:50AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 29, 2022 at 02:20:01PM -0500, Dan Vacura wrote:
> > Several types of kernel panics can occur due to timing during the uvc
> > gadget removal. This appears to be a problem with gadget resources being
> > managed by both the client application's v4l2 open/close and the UDC
> > gadget bind/unbind. Since the concept of USB_GADGET_DELAYED_STATUS
> > doesn't exist for unbind, add a wait to allow for the application to
> > close out.
> >
> > Some examples of the panics that can occur are:
> >
> > <1>[ 1147.652313] Unable to handle kernel NULL pointer dereference at
> > virtual address 0000000000000028
> > <4>[ 1147.652510] Call trace:
> > <4>[ 1147.652514] usb_gadget_disconnect+0x74/0x1f0
> > <4>[ 1147.652516] usb_gadget_deactivate+0x38/0x168
> > <4>[ 1147.652520] usb_function_deactivate+0x54/0x90
> > <4>[ 1147.652524] uvc_function_disconnect+0x14/0x38
> > <4>[ 1147.652527] uvc_v4l2_release+0x34/0xa0
> > <4>[ 1147.652537] __fput+0xdc/0x2c0
> > <4>[ 1147.652540] ____fput+0x10/0x1c
> > <4>[ 1147.652545] task_work_run+0xe4/0x12c
> > <4>[ 1147.652549] do_notify_resume+0x108/0x168
> >
> > <1>[ 282.950561][ T1472] Unable to handle kernel NULL pointer
> > dereference at virtual address 00000000000005b8
> > <6>[ 282.953111][ T1472] Call trace:
> > <6>[ 282.953121][ T1472] usb_function_deactivate+0x54/0xd4
> > <6>[ 282.953134][ T1472] uvc_v4l2_release+0xac/0x1e4
> > <6>[ 282.953145][ T1472] v4l2_release+0x134/0x1f0
> > <6>[ 282.953167][ T1472] __fput+0xf4/0x428
> > <6>[ 282.953178][ T1472] ____fput+0x14/0x24
> > <6>[ 282.953193][ T1472] task_work_run+0xac/0x130
> >
> > <3>[ 213.410077][ T29] configfs-gadget gadget: uvc: Failed to queue
> > request (-108).
> > <1>[ 213.410116][ T29] Unable to handle kernel NULL pointer
> > dereference at virtual address 0000000000000003
> > <6>[ 213.413460][ T29] Call trace:
> > <6>[ 213.413474][ T29] uvcg_video_pump+0x1f0/0x384
> > <6>[ 213.413489][ T29] process_one_work+0x2a4/0x544
> > <6>[ 213.413502][ T29] worker_thread+0x350/0x784
> > <6>[ 213.413515][ T29] kthread+0x2ac/0x320
> > <6>[ 213.413528][ T29] ret_from_fork+0x10/0x30
> >
> > Signed-off-by: Dan Vacura <[email protected]>
> > ---
> > drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++++++++++++++
> > drivers/usb/gadget/function/uvc.h | 2 ++
> > drivers/usb/gadget/function/uvc_v4l2.c | 3 ++-
> > 3 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > index 50e6e7a58b41..3cc8cf24a7c7 100644
> > --- a/drivers/usb/gadget/function/f_uvc.c
> > +++ b/drivers/usb/gadget/function/f_uvc.c
> > @@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
> > {
> > struct usb_composite_dev *cdev = c->cdev;
> > struct uvc_device *uvc = to_uvc(f);
> > + int wait_ret = 1;
> >
> > uvcg_info(f, "%s()\n", __func__);
>
> Ick, wait, is that in the kernel? That needs to be removed, ftrace can
> do that for you.
Yes, part of the kernel, and tbh, I find it to be quite helpful in
debugging field issues from customers, where enabling ftrace isn't
practical. If you still want to remove, there are other locations in
this gadget driver that log function entry. Perhaps it'd be better to
do a separate change that cleans up logging a bit or do you prefer to
just refactor this one now?
>
> > + /* If we know we're connected via v4l2, then there should be a cleanup
> > + * of the device from userspace either via UVC_EVENT_DISCONNECT or
> > + * though the video device removal uevent. Allow some time for the
> > + * application to close out before things get deleted.
> > + */
> > + if (uvc->func_connected) {
> > + uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
> > + wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
> > + uvc->func_connected == false, msecs_to_jiffies(500));
> > + uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
>
> Please remove debugging code before submitting patches.
Will do.
>
> thanks,
>
> greg k-h
Hi Greg,
Thanks for the feedback.
On Mon, May 02, 2022 at 01:46:39PM +0200, Greg Kroah-Hartman wrote:
> On Sun, May 01, 2022 at 11:11:36PM -0500, Dan Vacura wrote:
> > On Sat, Apr 30, 2022 at 09:56:50AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 29, 2022 at 02:20:01PM -0500, Dan Vacura wrote:
> > > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > > > index 50e6e7a58b41..3cc8cf24a7c7 100644
> > > > --- a/drivers/usb/gadget/function/f_uvc.c
> > > > +++ b/drivers/usb/gadget/function/f_uvc.c
> > > > @@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
> > > > {
> > > > struct usb_composite_dev *cdev = c->cdev;
> > > > struct uvc_device *uvc = to_uvc(f);
> > > > + int wait_ret = 1;
> > > >
> > > > uvcg_info(f, "%s()\n", __func__);
> > >
> > > Ick, wait, is that in the kernel? That needs to be removed, ftrace can
> > > do that for you.
> >
> > Yes, part of the kernel, and tbh, I find it to be quite helpful in
> > debugging field issues from customers, where enabling ftrace isn't
> > practical.
>
> Why isn't ftrace ok to enable in a running kernel?
ftrace is totally fine to enable. I should've said convenient instead of
practical, from my experience there's a bit more offline/developer
overhead for setup and integrating the trace logs with a panic or kmsg,
then bundling that with the bug reports we get.
>
> Worst case, this should be dev_dbg(), right?
>
> > If you still want to remove, there are other locations in
> > this gadget driver that log function entry. Perhaps it'd be better to
> > do a separate change that cleans up logging a bit or do you prefer to
> > just refactor this one now?
>
> This commit is fine, it's a separate issue, I just noticed it as it was
> in the context of this change.
>
> > > > + /* If we know we're connected via v4l2, then there should be a cleanup
> > > > + * of the device from userspace either via UVC_EVENT_DISCONNECT or
> > > > + * though the video device removal uevent. Allow some time for the
> > > > + * application to close out before things get deleted.
> > > > + */
> > > > + if (uvc->func_connected) {
> > > > + uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
> > > > + wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
> > > > + uvc->func_connected == false, msecs_to_jiffies(500));
> > > > + uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
> > >
> > > Please remove debugging code before submitting patches.
> >
> > Will do.
>
> But this should be removed :)
>
> Feel free to change it to dev_dbg(), which gives you the __func__
> automatically without anything extra needed.
Yes, I'll go with this approach, as it doesn't make sense to always
print, but is good to have when developing or debugging. The timeouts
seem reasonable to me and work for our setup, but I was expecting some
comments about them or a suggestion for a different approach.
Thanks,
Dan