linux-kernel - Re: [PATCH v4 6/6] media: uvcvideo: Move decode processing to process context

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 7 Nov 2018 13:38:19 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Kieran Bingham <kieran.bingham@...asonboard.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Guennadi Liakhovetski <g.liakhovetski@....de>,
        olivier.braun@...reolabs.com, troy.kisky@...ndarydevices.com,
        Randy Dunlap <rdunlap@...radead.org>,
        Philipp Zabel <philipp.zabel@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 6/6] media: uvcvideo: Move decode processing to process context

Hi Kieran,

On Wed, Nov 7, 2018 at 12:13 AM Kieran Bingham
<kieran.bingham@...asonboard.com> wrote:
>
> Hi Tomasz,
>
> On 07/08/2018 10:54, Tomasz Figa wrote:
> > Hi Kieran,
> >
> > On Wed, Mar 28, 2018 at 1:47 AM Kieran Bingham
> > <kieran.bingham@...asonboard.com> wrote:
> > [snip]
> >> @@ -1544,25 +1594,29 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
> >>   */
> >>  static void uvc_uninit_video(struct uvc_streaming *stream, int free_buffers)
> >>  {
> >> -       struct urb *urb;
> >> -       unsigned int i;
> >> +       struct uvc_urb *uvc_urb;
> >>
> >>         uvc_video_stats_stop(stream);
> >>
> >> -       for (i = 0; i < UVC_URBS; ++i) {
> >> -               struct uvc_urb *uvc_urb = &stream->uvc_urb[i];
> >> +       /*
> >> +        * We must poison the URBs rather than kill them to ensure that even
> >> +        * after the completion handler returns, any asynchronous workqueues
> >> +        * will be prevented from resubmitting the URBs
> >> +        */
> >> +       for_each_uvc_urb(uvc_urb, stream)
> >> +               usb_poison_urb(uvc_urb->urb);
> >>
> >> -               urb = uvc_urb->urb;
> >> -               if (urb == NULL)
> >> -                       continue;
> >> +       flush_workqueue(stream->async_wq);
> >>
> >> -               usb_kill_urb(urb);
> >> -               usb_free_urb(urb);
> >> +       for_each_uvc_urb(uvc_urb, stream) {
> >> +               usb_free_urb(uvc_urb->urb);
> >>                 uvc_urb->urb = NULL;
> >>         }
> >>
> >>         if (free_buffers)
> >>                 uvc_free_urb_buffers(stream);
> >> +
> >> +       destroy_workqueue(stream->async_wq);
> >
> > In our testing, this function ends up being called twice, if before
> > suspend the camera is streaming and if the camera disconnects between
> > suspend and resume. This is because uvc_video_suspend() calls this
> > function (with free_buffers = 0), but uvc_video_resume() wouldn't call
> > uvc_init_video() due to an earlier failure and uvc_v4l2_release()
> > would end up calling this function again, while the workqueue is
> > already destroyed.
> >
> > The following diff seems to take care of it:
>
> Thank you for this. After discussing with Laurent, I have gone with the
> approach of keeping the workqueue for the lifetime of the stream, rather
> than the lifetime of the streamon.
>

Sounds good to me. Thanks for heads up!

Best regards,
Tomasz