On Mon, 2013-07-22 at 12:40 +0800, Vaughan Cao wrote:
> A race condition may happen if two threads are both trying to open the same sg
> with O_EXCL simultaneously. It's possible that they both find fsds list is
> empty and get_exclude(sdp) returns 0, then they both call set_exclude() and
> break out from wait_event_interruptible and resume open.
>
> Now use rwsem to protect this process. Exclusive open gets write lock and
> others get read lock. The lock will be held until file descriptor is closed.
> This also leads 'exclude' only a status rather than a check mark.
>
> Signed-off-by: Vaughan Cao <vaughan.cao@xxxxxxxxxx>
This produces a couple of unused variable warnings which will excite the
static checkers, so I've removed them:
drivers/scsi/sg.c: In function ‘sg_open’:
drivers/scsi/sg.c:268:6: warning: unused variable ‘res’ [-Wunused-variable]
drivers/scsi/sg.c: In function ‘sg_remove_sfp’:
drivers/scsi/sg.c:2138:20: warning: unused variable ‘sdp’ [-Wunused-variable]
Plus this:
> @@ -331,16 +331,19 @@ sg_open(struct inode *inode, struct file *filp)
> if ((sfp = sg_add_sfp(sdp, dev)))
> filp->private_data = sfp;
> else {
> - if (flags & O_EXCL) {
> - set_exclude(sdp, 0); /* undo if error */
> - wake_up_interruptible(&sdp->o_excl_wait);
> - }
> retval = -ENOMEM;
> - goto error_out;
> + goto sem_out;
> }
> retval = 0;
> -error_out:
> +
> if (retval) {
> +sem_out:
> + if (flags & O_EXCL) {
Is insane code. You're adding a label to jump around setting retval=0
(which is completely superfluous: retval is already provably zero at
this point because of the check after retval =
scsi_autopm_get_device(sdp->device))
The sane way to write this is
if ((sfp = sg_add_sfp(sdp, dev)))
filp->private_data = sfp;
else {
retval = -ENOMEM;
if (flags & O_EXCL) {
...
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
