linux-kernel - Re: [PATCH v2 2/6] media: uvcvideo: Import standard controls from uvcdynctrl

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: <CANiDSCsrAvwygBrQLsF9RVOUGpdOEaOhzE90=c2CrW8GGe6-=g@mail.gmail.com>
Date: Tue, 9 Dec 2025 07:28:35 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans de Goede <hansg@...nel.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-usb@...r.kernel.org, Manav Gautama <bandwidthcrunch@...il.com>, 
	Martin Rubli <martin_rubli@...itech.com>
Subject: Re: [PATCH v2 2/6] media: uvcvideo: Import standard controls from uvcdynctrl

Hi Hans


On Mon, 8 Dec 2025 at 20:12, Hans de Goede <hansg@...nel.org> wrote:
>
> Hi,
>
> On 8-Dec-25 12:02 PM, Hans de Goede wrote:
> > Hi Ricardo,
> >
> > Thank you very much for doing this, this has been on my own TODO list for
> > a long time, so it is great to finally see this happen.
> >
> > On 19-Nov-25 8:37 PM, Ricardo Ribalda wrote:
> >> The uvcdynctrl tool from libwebcam:
> >> https://sourceforge.net/projects/libwebcam/
> >> maps proprietary controls into v4l2 controls using the UVCIOC_CTRL_MAP
> >> ioctl.
> >>
> >> The tool has not been updated for 10+ years now, and there is no reason
> >> for the UVC driver to not do the mapping by itself.
> >>
> >> This patch adds the mappings from the uvcdynctrl into the driver. Hopefully
> >> this effort can help in deprecating the UVCIOC_CTRL_MAP ioctl.
> >
> > ...
> >
> > Question what happens if uvcdynctrl is run after applying this patch ?
>
> Answering my own question here, we will hit:
>
> drivers/media/usb/uvc/uvc_ctrl.c: 3166:
>
>         list_for_each_entry(map, &ctrl->info.mappings, list) {
>                 if (mapping->id == map->id) {
>                         uvc_dbg(dev, CONTROL,
>                                 "Can't add mapping '%s', control id 0x%08x already exists\n",
>                                 uvc_map_get_name(mapping), mapping->id);
>                         ret = -EEXIST;
>                         goto done;
>                 }
>         }
>
> So uvcdynctrl will see an EEXIST error. I think we need to add an -EEXIST check
> to uvc_ctrl_add_mapping() )or uvc_ioctl_xu_ctrl_map() which is the only caller of
> uvc_ctrl_add_mapping()) and if -EEXIST is returned do a uvc_warn_once() that duplicate
> mappings are being ignored and return 0 instead of -EEXIST to avoid breaking existing

uvcdynctrl seems to handle this kind of error ok:

https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/dynctrl.c#l1215

while(node_mapping) {
  CResult ret = process_mapping(node_mapping, ctx);
  if(ctx->info) {
    if(ret)
      ctx->info->stats.mappings.successful++;
    else
      ctx->info->stats.mappings.failed++;
  }
  node_mapping = xml_get_next_sibling_by_name(node_mapping, "mapping");
}

https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/dynctrl.c#l1199
if(v4l2_ret != 0
#ifdef DYNCTRL_IGNORE_EEXIST_AFTER_PASS1
&& (ctx->pass == 1 || errno != EEXIST)
#endif
)

https://sourceforge.net/p/libwebcam/code/ci/master/tree/libwebcam/libwebcam.h#l69
/// Ignore EEXIST errors for the UVCIOC_CTRL_ADD and UVCIOC_CTRL_MAP ioctls for
/// all but the first device. This is required if the driver uses
global controls
/// instead of per-device controls.
#define DYNCTRL_IGNORE_EEXIST_AFTER_PASS1


(BTW, I think the last comment ^^^ is wrong, it should be. Ignore
EEXIST errors, or errors for second passes.
But I might need a coffee :P)


Looking at the debian codesearch:
https://codesearch.debian.net/search?q=UVCIOC_CTRL_MAP+-path%3Aioctl.rs+-path%3Auvc_v4l2.c+-file%3Auvcvideo.rst+-file%3Auvc_ctrl.c+-file%3Auvcvideo.h&literal=1

the only occurrence that I am no sure if it will properly handle -EEXIST is:
https://sources.debian.org/src/chromium/143.0.7499.40-1/chrome/browser/ash/chromebox_for_meetings/xu_camera/xu_camera_service.cc?hl=400#L400
But that is ash-> ChromeOS browser. I can ask the code owner to fix it
if needed.


I'd rather not add the quirk that you are proposing if possible. I
would expect that any/all the userspace handles -EEXIST because the
uvc control state outlives the userspace.


Let me know what you think.


> userspace.
>
> Regards,
>
> Hans
>
>



--
Ricardo Ribalda