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

25336 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...

This can most easily happen as a result of multiple `enumerate()` calls,
however, currently `enumerate()` is only ever called once, so that won't
trigger the issue.

Nonetheless, it is still possible to trigger this issue due to the fact
that 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
any number of `enumerate()` calls and hotplug events will work correctly
(assuming that udev reports "add" / "remove" events correctly).

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 .../internal/device_enumerator_udev.h         |  3 ++
 src/libcamera/device_enumerator_udev.cpp      | 38 ++++++++++++++-----
 2 files changed, 32 insertions(+), 9 deletions(-)

Quoting Barnabás Pőcze (2025-12-02 13:58:17)
> 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...
> 
> This can most easily happen as a result of multiple `enumerate()` calls,
> however, currently `enumerate()` is only ever called once, so that won't
> trigger the issue.
> 
> Nonetheless, it is still possible to trigger this issue due to the fact
> that 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
> 

This seems like such a small race window - it's surprising if that's
what caused the reported bug. But if it's a window it can happen right
...

But the patch looks reasonable to me:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> To address this, keep track of the devnums of all devices reported by
> udev, and reject devices with already known devnums. This ensures that
> any number of `enumerate()` calls and hotplug events will work correctly
> (assuming that udev reports "add" / "remove" events correctly).
> 
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../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..3de7c37104 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -11,6 +11,7 @@
>  #include <map>
>  #include <memory>
>  #include <set>
> +#include <unordered_set>
>  #include <string>
>  #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, 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
>

2025. 12. 02. 15:09 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-12-02 13:58:17)
>> 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...
>>
>> This can most easily happen as a result of multiple `enumerate()` calls,
>> however, currently `enumerate()` is only ever called once, so that won't
>> trigger the issue.
>>
>> Nonetheless, it is still possible to trigger this issue due to the fact
>> that 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
>>
> 
> This seems like such a small race window - it's surprising if that's
> what caused the reported bug. But if it's a window it can happen right

The reporter said

   libcamera in wireplumber crashed when I updated my system and Wireplumber restarted, here are logs:

So I imagine, due to the system update, there was some (a lot?) udev activity in
parallel with wireplumber restarting, which made the bug visible.


> ...
> 
> But the patch looks reasonable to me:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> To address this, keep track of the devnums of all devices reported by
>> udev, and reject devices with already known devnums. This ensures that
>> any number of `enumerate()` calls and hotplug events will work correctly
>> (assuming that udev reports "add" / "remove" events correctly).
>>
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../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..3de7c37104 100644
>> --- a/include/libcamera/internal/device_enumerator_udev.h
>> +++ b/include/libcamera/internal/device_enumerator_udev.h
>> @@ -11,6 +11,7 @@
>>   #include <map>
>>   #include <memory>
>>   #include <set>
>> +#include <unordered_set>
>>   #include <string>
>>   #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, 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
>>

Hi Barnabás,

Thank you for addressing this long-standing issue.

On Tue, Dec 02, 2025 at 02:58:17PM +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...
> 
> This can most easily happen as a result of multiple `enumerate()` calls,
> however, currently `enumerate()` is only ever called once, so that won't
> trigger the issue.
> 
> Nonetheless, it is still possible to trigger this issue due to the fact
> that 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
> any number of `enumerate()` calls and hotplug events will work correctly
> (assuming that udev reports "add" / "remove" events correctly).

A race between initial enumerate and hotplug events is unavoidable with
our code structure. As this is an issue that I would expect to affect
lots of applications using udev, I wonder if there's a recommended way
in the udev API to handle this. Are all applications expected to filter
out duplicates manually ?

> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../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..3de7c37104 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -11,6 +11,7 @@
>  #include <map>
>  #include <memory>
>  #include <set>
> +#include <unordered_set>
>  #include <string>
>  #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, 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);
>  }

2025. 12. 02. 16:43 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for addressing this long-standing issue.
> 
> On Tue, Dec 02, 2025 at 02:58:17PM +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...
>>
>> This can most easily happen as a result of multiple `enumerate()` calls,
>> however, currently `enumerate()` is only ever called once, so that won't
>> trigger the issue.
>>
>> Nonetheless, it is still possible to trigger this issue due to the fact
>> that 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
>> any number of `enumerate()` calls and hotplug events will work correctly
>> (assuming that udev reports "add" / "remove" events correctly).
> 
> A race between initial enumerate and hotplug events is unavoidable with
> our code structure. As this is an issue that I would expect to affect
> lots of applications using udev, I wonder if there's a recommended way
> in the udev API to handle this. Are all applications expected to filter
> out duplicates manually ?

My conclusion is "yes(?)". The udev enumerator and monitor are largely
separate things, they do refer to a common `struct udev` context, but
at least in the systemd implementation, that only stores a "user data"
`void *`. So I don't think we can expect udev to mediate this. I also
cannot see any way to "connect" the enumerator and the monitor.

I have looked at multiple users on debian code search, and while it wasn't
too conclusive, I have seen deduplication code for sure. E.g. libinput
explicitly mentions this very race condition:

	/* There is a race at startup: a device added between setting
	 * up the udev monitor and enumerating all current devices may show
	 * up in both lists. Filter those out.
	 */
	if (filter_duplicates(seat, udev_device))
		return 0;

   -- https://sources.debian.org/src/libinput/1.30.0-1/src/udev-seat.c?hl=57#L103


Regards,
Barnabás Pőcze

> 
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../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..3de7c37104 100644
>> --- a/include/libcamera/internal/device_enumerator_udev.h
>> +++ b/include/libcamera/internal/device_enumerator_udev.h
>> @@ -11,6 +11,7 @@
>>   #include <map>
>>   #include <memory>
>>   #include <set>
>> +#include <unordered_set>
>>   #include <string>
>>   #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, 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);
>>   }
>

On Tue, Dec 02, 2025 at 06:09:45PM +0100, Barnabás Pőcze wrote:
> 2025. 12. 02. 16:43 keltezéssel, Laurent Pinchart írta:
> > On Tue, Dec 02, 2025 at 02:58:17PM +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...
> >>
> >> This can most easily happen as a result of multiple `enumerate()` calls,
> >> however, currently `enumerate()` is only ever called once, so that won't
> >> trigger the issue.
> >>
> >> Nonetheless, it is still possible to trigger this issue due to the fact
> >> that 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
> >> any number of `enumerate()` calls and hotplug events will work correctly
> >> (assuming that udev reports "add" / "remove" events correctly).
> > 
> > A race between initial enumerate and hotplug events is unavoidable with
> > our code structure. As this is an issue that I would expect to affect
> > lots of applications using udev, I wonder if there's a recommended way
> > in the udev API to handle this. Are all applications expected to filter
> > out duplicates manually ?
> 
> My conclusion is "yes(?)". The udev enumerator and monitor are largely
> separate things, they do refer to a common `struct udev` context, but
> at least in the systemd implementation, that only stores a "user data"
> `void *`. So I don't think we can expect udev to mediate this. I also
> cannot see any way to "connect" the enumerator and the monitor.
> 
> I have looked at multiple users on debian code search, and while it wasn't
> too conclusive, I have seen deduplication code for sure. E.g. libinput
> explicitly mentions this very race condition:
> 
> 	/* There is a race at startup: a device added between setting
> 	 * up the udev monitor and enumerating all current devices may show
> 	 * up in both lists. Filter those out.
> 	 */
> 	if (filter_duplicates(seat, udev_device))
> 		return 0;
> 
>    -- https://sources.debian.org/src/libinput/1.30.0-1/src/udev-seat.c?hl=57#L103

Thank you for the information. It would have made sense for udev to
expose some sort of API to handle this, instead of causing dozens of
users to implement slightly different bugs :-)

> >> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   .../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..3de7c37104 100644
> >> --- a/include/libcamera/internal/device_enumerator_udev.h
> >> +++ b/include/libcamera/internal/device_enumerator_udev.h
> >> @@ -11,6 +11,7 @@
> >>   #include <map>
> >>   #include <memory>
> >>   #include <set>
> >> +#include <unordered_set>

Wrong order (pun intended).

> >>   #include <string>
> >>   #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, device added between udev

enumerate() is not meant to be called multiple times, so I wouldn't have
mentioned guarding against that as a goal here (or in the commit
message). If you want, you can add a sentence to the function's
documentation to indicate the function can be called once only.

> >> +	 * 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_`.

Please reflow comments to 80 columns.

> >> +	 */
> >> +	const dev_t devnum = udev_device_get_devnum(dev);

I wonder if there could be other types of race conditions that would
cause different devices to be seen with the same devnum (when a device
is removed and then re-added for instance). I can't think of any though.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >> +	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);
> >>   }

2025. 12. 03. 3:15 keltezéssel, Laurent Pinchart írta:
> On Tue, Dec 02, 2025 at 06:09:45PM +0100, Barnabás Pőcze wrote:
>> 2025. 12. 02. 16:43 keltezéssel, Laurent Pinchart írta:
>>> On Tue, Dec 02, 2025 at 02:58:17PM +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...
>>>>
>>>> This can most easily happen as a result of multiple `enumerate()` calls,
>>>> however, currently `enumerate()` is only ever called once, so that won't
>>>> trigger the issue.
>>>>
>>>> Nonetheless, it is still possible to trigger this issue due to the fact
>>>> that 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
>>>> any number of `enumerate()` calls and hotplug events will work correctly
>>>> (assuming that udev reports "add" / "remove" events correctly).
>>>
>>> A race between initial enumerate and hotplug events is unavoidable with
>>> our code structure. As this is an issue that I would expect to affect
>>> lots of applications using udev, I wonder if there's a recommended way
>>> in the udev API to handle this. Are all applications expected to filter
>>> out duplicates manually ?
>>
>> My conclusion is "yes(?)". The udev enumerator and monitor are largely
>> separate things, they do refer to a common `struct udev` context, but
>> at least in the systemd implementation, that only stores a "user data"
>> `void *`. So I don't think we can expect udev to mediate this. I also
>> cannot see any way to "connect" the enumerator and the monitor.
>>
>> I have looked at multiple users on debian code search, and while it wasn't
>> too conclusive, I have seen deduplication code for sure. E.g. libinput
>> explicitly mentions this very race condition:
>>
>> 	/* There is a race at startup: a device added between setting
>> 	 * up the udev monitor and enumerating all current devices may show
>> 	 * up in both lists. Filter those out.
>> 	 */
>> 	if (filter_duplicates(seat, udev_device))
>> 		return 0;
>>
>>     -- https://sources.debian.org/src/libinput/1.30.0-1/src/udev-seat.c?hl=57#L103
> 
> Thank you for the information. It would have made sense for udev to
> expose some sort of API to handle this, instead of causing dozens of
> users to implement slightly different bugs :-)
> 
>>>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>> ---
>>>>    .../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..3de7c37104 100644
>>>> --- a/include/libcamera/internal/device_enumerator_udev.h
>>>> +++ b/include/libcamera/internal/device_enumerator_udev.h
>>>> @@ -11,6 +11,7 @@
>>>>    #include <map>
>>>>    #include <memory>
>>>>    #include <set>
>>>> +#include <unordered_set>
> 
> Wrong order (pun intended).
> 
>>>>    #include <string>
>>>>    #include <sys/types.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..406e59b360 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. multiple `enumerate()` calls, device added between udev
> 
> enumerate() is not meant to be called multiple times, so I wouldn't have
> mentioned guarding against that as a goal here (or in the commit
> message). If you want, you can add a sentence to the function's
> documentation to indicate the function can be called once only.
> 
>>>> +	 * 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_`.
> 
> Please reflow comments to 80 columns.
> 
>>>> +	 */
>>>> +	const dev_t devnum = udev_device_get_devnum(dev);
> 
> I wonder if there could be other types of race conditions that would
> cause different devices to be seen with the same devnum (when a device
> is removed and then re-added for instance). I can't think of any though.

I think this part already depends on the unique-ness of the devnum,
see the `orphans_` set. So as long as that holds, and udev reports
add/remove events in the correct order, I think it should be fine.


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>>> +	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);
>>>>    }
> 
> --
> Regards,
> 
> Laurent Pinchart