linux-kernel - Re: [PATCH v2 4/4] media: uvcvideo: Remove redundant NULL assignment

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241128214727.GE25839@pendragon.ideasonboard.com>
Date: Thu, 28 Nov 2024 23:47:27 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>,
	Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] media: uvcvideo: Remove redundant NULL assignment

On Thu, Nov 28, 2024 at 10:34:38PM +0100, Ricardo Ribalda wrote:
> On Thu, 28 Nov 2024 at 22:26, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2024 at 12:14:52PM +0000, Ricardo Ribalda wrote:
> > > ctrl->handle will only be different than NULL for controls that have
> > > mappings. This is because that assignment is only done inside
> > > uvc_ctrl_set() for mapped controls.
> >
> > That seems right. I checked the unplug paths, and the mappings are
> > destroyed from uvc_delete(), after all file handles get closed.
> >
> > I think this patch should go first in the series, as otherwise there's a
> > temporary state where you rely on handle never being touched without
> > holding the proper lock, and this code path violates that requirement.
> > Is there anything I may be missing that would cause issues with
> > bisection if I move this patch at the beginning of the series ?
> 
> You probably want to add the Cc: stable and Fixes: tags if you make
> this patch the first one.

Good point. Given that ctrl->handle for controls without mappings can't
be touched anywhere else, I think it's safe to keep this patch last, and
not backport it. There may be a theoretical issue with mappings
dynamically added at runtime, as the list could then become non-empty
and the condition change, but the condition should never turn from false
to true, only from true to false, so I think it's safe.

I'll keep the patch last.

> > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> >
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 99ddc5e9dff8..b9c9ff629ab6 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1645,10 +1645,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,
> > >       struct uvc_device *dev = chain->dev;
> > >       struct uvc_ctrl_work *w = &dev->async_ctrl;
> > >
> > > -     if (list_empty(&ctrl->info.mappings)) {
> > > -             ctrl->handle = NULL;
> > > +     if (list_empty(&ctrl->info.mappings))
> > >               return false;
> > > -     }
> > >
> > >       w->data = data;
> > >       w->urb = urb;

-- 
Regards,

Laurent Pinchart