Dmitry Torokhov wrote:
On Sat, Dec 04, 2010 at 09:22:23PM +0100, Oliver Neukum wrote:
Am Samstag, 4. Dezember 2010, 00:16:12 schrieb Dmitry Torokhov:
Since disconnect can happen at any time, we can't initialize
struct hid_device *hid = hiddev->hid at the beginning of ioctl
and then use it.
This change checks hiddev->exist flag while holding
the existancelock and uses hid_device only if it exists.
Why didn't you take the lock and check hiddev->exist at the beginning of
ioctl handler instead of pushing it down into individual command
handlers? I guess it would slow down HIDIOCGVERSION but I think we could
pay this price for code that is more clear ;)
Strictly speaking you'd change the semantics. Right now you can execute
the ioctl even if you know you are holding an fd to a disconnected device
open.
No, I do not think I would. I do not believe that the availability for
HIDIOCGVERSION on disconnected device is spelled out in API/ABI spec. We
only know that ioctl will either succeed or appropriate error code is
returned. The fact that right now HIDIOCGVERSION is available on
disconnected devices is just an implementation detail subject to change.
It's not just HIDIOCGVERSION. A couple of other commands
(HIDIOCGFLAG/HIDIOCSFLAG) didn't check device existence in the first
place either.
Current implementation depends on when the device is actually removed.
If it has been removed before the hiddev_ioctl(), hiddev_ioctl() returns
-EIO.
If the device is removed while hiddev_ioctl() is in progress, we either
do not notice that and handle HIDIOCGVERSION and HIDIOCGFLAG/HIDIOCSFLAG
just fine, or return -ENODEV.
I'll submit a patch in a bit that applies on top of the "[PATCH] USB:
USBHID: Fix race between disconnect and hiddev_ioctl" and makes the
hiddev_ioctl() check
device existence before processing the command and always return -ENODEV
in case the device has been removed.
Thanks,
Val.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
