[RFC,v2,3/3] libcamera: device_enumerator_udev: Handle duplicate devices

25388 diff mbox series

It is possible that same device is processed multiple times, leading to
multiple `MediaDevice`s being instantiated, mostly likely leading to
a fatal error:

  Trying to register a camera with a duplicated ID xyzw...

There is a time window after the `udev_monitor` has been created in `init()`
and the first (and only) enumeration done in `enumerate()`. If e.g. a UVC
camera is connected in this time frame, then it is possible that it will be
processed both by the `udevNotify()` and the initial `enumerate()`, leading
to the fatal error. This can be reproduced as follows:

  1. $ gdb --args cam -m
  2. (gdb) break libcamera::DeviceEnumeratorUdev::enumerate
  3. (gdb) run
  4. when the breakpoint is hit, connect a usb camera
  5. (gdb) continue
  6. observe fatal error

To address this, keep track of the devnums of all devices reported by
udev, and reject devices with already known devnums. This ensures that
the same device won't be reported multiple times (assuming that udev
reports "add" / "remove" events in the correct order).

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
changes in v2:
  * address review comments

v1: https://patchwork.libcamera.org/patch/25336/
---
 .../internal/device_enumerator_udev.h         |  3 ++
 src/libcamera/device_enumerator_udev.cpp      | 38 ++++++++++++++-----
 2 files changed, 32 insertions(+), 9 deletions(-)

--
2.52.0

Hi Barnabás

On Mon, Dec 08, 2025 at 12:00:35PM +0100, Barnabás Pőcze wrote:
> It is possible that same device is processed multiple times, leading to
> multiple `MediaDevice`s being instantiated, mostly likely leading to
> a fatal error:
>
>   Trying to register a camera with a duplicated ID xyzw...
>
> There is a time window after the `udev_monitor` has been created in `init()`
> and the first (and only) enumeration done in `enumerate()`. If e.g. a UVC
> camera is connected in this time frame, then it is possible that it will be
> processed both by the `udevNotify()` and the initial `enumerate()`, leading
> to the fatal error. This can be reproduced as follows:
>
>   1. $ gdb --args cam -m
>   2. (gdb) break libcamera::DeviceEnumeratorUdev::enumerate
>   3. (gdb) run
>   4. when the breakpoint is hit, connect a usb camera
>   5. (gdb) continue
>   6. observe fatal error
>
> To address this, keep track of the devnums of all devices reported by
> udev, and reject devices with already known devnums. This ensures that
> the same device won't be reported multiple times (assuming that udev
> reports "add" / "remove" events in the correct order).
>
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293

If only we had a way to notify in an issue that a series targeting it
is in review...

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks!

> ---
> changes in v2:
>   * address review comments
>
> v1: https://patchwork.libcamera.org/patch/25336/
> ---
>  .../internal/device_enumerator_udev.h         |  3 ++
>  src/libcamera/device_enumerator_udev.cpp      | 38 ++++++++++++++-----
>  2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> index c2f7154b6b..958e27a2f1 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -13,6 +13,7 @@
>  #include <set>
>  #include <string>
>  #include <sys/types.h>
> +#include <unordered_set>
>
>  #include <libcamera/base/class.h>
>
> @@ -59,6 +60,7 @@ private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(DeviceEnumeratorUdev)
>
>  	int addUdevDevice(struct udev_device *dev);
> +	void removeUdevDevice(struct udev_device *dev);
>  	int populateMediaDevice(MediaDevice *media, DependencyMap *deps);
>  	std::string lookupDeviceNode(dev_t devnum);
>
> @@ -70,6 +72,7 @@ private:
>  	EventNotifier *notifier_;
>
>  	std::set<dev_t> orphans_;
> +	std::unordered_set<dev_t> devices_;
>  	std::list<MediaDeviceDeps> pending_;
>  	std::map<dev_t, MediaDeviceDeps *> devMap_;
>  };
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 4e20a3cc0c..3195dd06e2 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -76,6 +76,21 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
>  	if (!subsystem)
>  		return -ENODEV;
>
> +	/*
> +	 * Record that udev reported the given devnum. And reject if it has
> +	 * already been seen (e.g. device added between udev monitor creation
> +	 * in `init()` and `enumerate()`). This record is kept even if later
> +	 * in this function an error is encountered. Only a "remove" event
> +	 * from udev should erase it from `devices_`.
> +	 */
> +	const dev_t devnum = udev_device_get_devnum(dev);
> +	if (devnum == makedev(0, 0))
> +		return -ENODEV;
> +
> +	const auto [it, inserted] = devices_.insert(devnum);
> +	if (!inserted)
> +		return -EEXIST;
> +
>  	if (!strcmp(subsystem, "media")) {
>  		std::unique_ptr<MediaDevice> media =
>  			createDevice(udev_device_get_devnode(dev));
> @@ -111,13 +126,22 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
>  	}
>
>  	if (!strcmp(subsystem, "video4linux")) {
> -		addV4L2Device(udev_device_get_devnum(dev));
> +		addV4L2Device(devnum);
>  		return 0;
>  	}
>
>  	return -ENODEV;
>  }
>
> +void DeviceEnumeratorUdev::removeUdevDevice(struct udev_device *dev)
> +{
> +	const char *subsystem = udev_device_get_subsystem(dev);
> +	if (subsystem && !strcmp(subsystem, "media"))
> +		removeDevice(udev_device_get_devnode(dev));
> +
> +	devices_.erase(udev_device_get_devnum(dev));
> +}
> +
>  int DeviceEnumeratorUdev::enumerate()
>  {
>  	struct udev_enumerate *udev_enum = nullptr;
> @@ -341,18 +365,14 @@ void DeviceEnumeratorUdev::udevNotify()
>  	}
>
>  	std::string_view action(udev_device_get_action(dev));
> -	std::string_view deviceNode(udev_device_get_devnode(dev));
>
>  	LOG(DeviceEnumerator, Debug)
> -		<< action << " device " << deviceNode;
> +		<< action << " device " << udev_device_get_devnode(dev);
>
> -	if (action == "add") {
> +	if (action == "add")
>  		addUdevDevice(dev);
> -	} else if (action == "remove") {
> -		const char *subsystem = udev_device_get_subsystem(dev);
> -		if (subsystem && !strcmp(subsystem, "media"))
> -			removeDevice(std::string(deviceNode));
> -	}
> +	else if (action == "remove")
> +		removeUdevDevice(dev);
>
>  	udev_device_unref(dev);
>  }
> --
> 2.52.0