On Sat, May 26, 2001 at 01:49:45AM +0200, Oliver Neukum wrote:
>
> OK, here we go again.
Thanks for doing this, I really appreciate it. Now if all of the usb
drivers were only audited this well :)
> 1) in skel_open:
>
> You still increase the module use count after a down(), this is a race.
> Either trust the character device code to handle the counts or don't sleep
> before you increase the count.
Thanks, fixed.
> 2) in skel_release:
>
> The sanity check for open_count will leave a semaphore permanently down
> in the "goto exit_non_opened" code path
Ah, good catch. Now fixed.
> 3) in skel_write:
>
> You fail to set the flag for synchronous unlink. Thus all places where you use
> 'usb_unlink_urb' are race conditions. (a callback pointing into an unloaded
> module could be called)
Hm, in looking over the other USB drivers, almost none of them set this
bit. So I've added it to the skel_release function, right before the
usb_unlink_urb function is called. Thanks for letting me know about
this.
> 4) in skel_write_bulk_callback:
>
> You don't check for 'usb_unlink_urb' having been called on the urb. That
> error code does not really refer to an error and should not be reported as
> such.
Now fixed.
> 5) in skel_probe:
>
> The return value of devfs_register is not checked (not very serious)
With all of the people reporting the printer driver as having failed by
printing out when this function returns an error, I didn't want to make
that same mistake for when people do not compile in devfs. If devfs is
compiled in, and there is an error, the devfs_register function itself
will report an error.
> 6) in skel_disconnect:
>
> if (skel->open_count) {
>
> Shouldn't that be !skel->open_count ?
Yes it should, fixed.
> In addition that code path leaves a semaphore permanently down.
But the whole skel object is now freed. The fact that the semaphore was
set to down, shouldn't matter anymore, right? Or is it a better practice
to clear the semaphore (with a call to up()) before freeing the memory?
Thanks again so much for the comments. I'll be posting a new version in
a bit.
thanks,
greg k-h
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
