[Pcsclite-muscle] Fix libudev hotplug

[Stefani Seibold]

Hello Mr. Ludovic Rousseau,

Am Donnerstag, den 31.07.2014, 15:51 +0200 schrieb Ludovic Rousseau:
> 2014-07-07 14:22 GMT+02:00 Stefani Seibold <stefani at seibold.net>:

> > the current libudev hotplug code have some issues.
> >
> > The HPRegisterForHotplugEvents() does first a HPRescanUsbBus() and than
> > start the HPEstablishUSBNotifications() thread. This could lead in a
> > race condition, where a add event occurred  between the scan and the
> > start of the thread. I moved the scan after the
> > udev_monitor_enable_receiving() call, so the race is fixed.
> 
> Do you get this problem in a real use case or is it theoretical?
> 
> If you do get the problem in real life what is your application use case?
> 

It is true that the time window  is very small, but i was faced with
this kind of problem. Not with pcsc-lite, but with other hotplug code
with did it in the same wrong way. But does this make a difference? A
bug is a bug and should be fixed. 

> > Next: The HPRescanUsbBus() is broken too. It will set all
> > readerTracker.status to READER_ABSENT. This make it very hard to track
> > reader where added twice. This can happen due the fix above and due the
> > fact that any udev event in the origin code could lead to a add, because
> > the "remove" event whill call HPRescanUsbBus() which calls
> > HPAddDevice(). This problem could be fixed using a temporary array of
> > status bytes.
> 
> It looks like the problem is that HPRescanUsbBus() is not reentrant by
> using a global state.
> But HPRescanUsbBus() is only called from HPEstablishUSBNotifications()
> when a device is removed. The call to HPRescanUsbBus() is synchronous
> and thus HPEstablishUSBNotifications() can't call HPRescanUsbBus()
> again before the previous call has returned.
> 

Sorry, but the whole hotplug is broken. You try to add devices when you
get an remove and vice versa.

> So unless you modify the code to try solve the 1st issue the use of
> HPRescanUsbBus() should be correct and safe.
> 

The first issue has noting to do with this problem.

> > And at last the HPAddDevice() is broken because a SCARD_E_UNKNOWN_READER
> > status could never overwriten. But the "remove" event handling which
> > also do device adds it could happen that a HPRescanUsbBus() will try to
> > add a device via HPAddDevice() which is still in use by other processes
> > or not ready. So a device should not permanently marked as
> > SCARD_E_UNKNOWN_READER because when the udev "add" event arrives than
> > the device should be normaly accessible.
> 
> I do not follow you here.
> What do you call "a device [..] which is still in use by other
> processes or not ready."
> 
> 

It is very simple, the hotplug_libudev code will receive events from and
USB device, no matter if is a card reader or any other device. And when
a event for a non CCID device you will call HPRescanUsbBus and try to
add a CCID device when one is found. But at this time udevd will still
working with the device.

Udevd has something to do before the hotplug_libudev can access the
device, f.e. create the device node, set the file mode bits, maybe
execute a helper program and a least it will send a udev netlink
messages "add".

I was faced with the problem that udev was not ready with the whole
process above and the device node was not ready during the call of
HPAddDevice().

A clean udev hotplug application handles events seperatly: add events
will only add of a device and remove events only remove of a device.

The "add" code will also check for duplicate devices adds. This can
happen when during the udev_monitor_enable_receiving() and the coldplug
scan a device will be plugged. Then the coldplug scanner probably will
find the device and a the hotplug add event will be received. 

> If the driver can't use the device then the device is marked
> SCARD_E_UNKNOWN_READER.
> If the device need more time so the driver can use it then the driver
> should wait for the device to be ready before returning
> IFD_NO_SUCH_DEVICE error code.
> 
> A device has no second chance of being detected. It works or it does not.
> 

You are right. But since the hotplug_libudev.c code is not race free...

> > The following small patch fix all this issues.
> 
> Thanks for your patch.
> First I need to understand what the problems are so I can work on
> unitary tests and verify the problems are really solved.
> 

The patch is more a workaround. For a clean solution the code must be
revamped. For a clean example of libudev usage have a look at
http://www.signal11.us/oss/udev/ If you like i can do this for you.

> It looks like you are working with virtual machines and moving USB
> devices from one virtual machine to another. Is that the case?
> 

No, i did not work with virtual machines. This happens on a real PowerPC
embedded device. The boot up of this device is very optimized and fast,
despite the is very slow CPU (400 MHz). 

I got all the problems and since i fixed it, i was never faced with the
problem that a USB CCID Reader was not found during the boot phase.

- Stefani