[PATCH] sg: atomize check and set sdp->exclude in sg_open

[PATCH] sg: atomize check and set sdp->exclude in sg_open

[vaughan]

Check and set sdp->exclude should be atomic when set in sg_open().

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
  drivers/scsi/sg.c | 17 ++++++++++++++++-
  1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..0ede08f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -245,6 +245,21 @@ static int set_exclude(Sg_device *sdp, char val)
      return val;
  }

+/* Check if we can set exclude and then set, return 1 if success */
+static int try_set_exclude(Sg_device *sdp)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+    if (sdp->exclude) {
+        spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+        return 0;
+    }
+    sdp->exclude = 1;
+    spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+    return 1;
+}
+
  static int sfds_list_empty(Sg_device *sdp)
  {
      unsigned long flags;
@@ -303,7 +318,7 @@ sg_open(struct inode *inode, struct file *filp)
              goto error_out;
          }
          res = wait_event_interruptible(sdp->o_excl_wait,
-                       ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 
: set_exclude(sdp, 1)));
+            ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : 
try_set_exclude(sdp)));
          if (res) {
              retval = res;    /* -ERESTARTSYS because signal hit process */
              goto error_out;
-- 
1.7.11.7

Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

[Jörn Engel]

On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
> 
> Check and set sdp->exclude should be atomic when set in sg_open().

The patch is line-wrapped.  More importantly, it doesn't seem to do
what your description indicates it should do.  And lastly, does this
fix a bug, possibly even one you have a testcase for, or was it found
by code inspection?

> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
> ---
>  drivers/scsi/sg.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 25b5455..0ede08f 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -245,6 +245,21 @@ static int set_exclude(Sg_device *sdp, char val)
>      return val;
>  }
> 
> +/* Check if we can set exclude and then set, return 1 if success */
> +static int try_set_exclude(Sg_device *sdp)
> +{
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&sg_open_exclusive_lock, flags);
> +    if (sdp->exclude) {
> +        spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
> +        return 0;
> +    }
> +    sdp->exclude = 1;
> +    spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
> +    return 1;
> +}
> +
>  static int sfds_list_empty(Sg_device *sdp)
>  {
>      unsigned long flags;
> @@ -303,7 +318,7 @@ sg_open(struct inode *inode, struct file *filp)
>              goto error_out;
>          }
>          res = wait_event_interruptible(sdp->o_excl_wait,
> -                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
> ? 0 : set_exclude(sdp, 1)));
> +            ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 :
> try_set_exclude(sdp)));
>          if (res) {
>              retval = res;    /* -ERESTARTSYS because signal hit process */
>              goto error_out;
> -- 
> 1.7.11.7
> 

Jörn

--
Fantasy is more important than knowledge. Knowledge is limited,
while fantasy embraces the whole world.
-- Albert Einstein

Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

[vaughan]

于 2013年06月05日 21:27, Jörn Engel 写道:
> On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
>>
>> Check and set sdp->exclude should be atomic when set in sg_open().
>
> The patch is line-wrapped.  More importantly, it doesn't seem to do
It's shorter than the original line, so I just leave it like this...

> what your description indicates it should do.  And lastly, does this
> fix a bug, possibly even one you have a testcase for, or was it found
> by code inspection?
I found it by code inspection. A race condition may happen with the old 
code 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. So it's necessary to 
check again with sg_open_exclusive_lock held to ensure only one can set 
sdp->exclude and return >0 to break out from wait_event loop.

>
>> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
>> ---
>>   drivers/scsi/sg.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 25b5455..0ede08f 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -245,6 +245,21 @@ static int set_exclude(Sg_device *sdp, char val)
>>       return val;
>>   }
>>
>> +/* Check if we can set exclude and then set, return 1 if success */
>> +static int try_set_exclude(Sg_device *sdp)
>> +{
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&sg_open_exclusive_lock, flags);
>> +    if (sdp->exclude) {
>> +        spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
>> +        return 0;
>> +    }
>> +    sdp->exclude = 1;
>> +    spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
>> +    return 1;
>> +}
>> +
>>   static int sfds_list_empty(Sg_device *sdp)
>>   {
>>       unsigned long flags;
>> @@ -303,7 +318,7 @@ sg_open(struct inode *inode, struct file *filp)
>>               goto error_out;
>>           }
>>           res = wait_event_interruptible(sdp->o_excl_wait,
>> -                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
>> ? 0 : set_exclude(sdp, 1)));
>> +            ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 :
>> try_set_exclude(sdp)));
>>           if (res) {
>>               retval = res;    /* -ERESTARTSYS because signal hit process */
>>               goto error_out;
>> --
>> 1.7.11.7
>>
>
> Jörn
>
> --
> Fantasy is more important than knowledge. Knowledge is limited,
> while fantasy embraces the whole world.
> -- Albert Einstein
>

Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

[Jörn Engel]

On Thu, 6 June 2013 00:16:45 +0800, vaughan wrote:
> 于 2013年06月05日 21:27, Jörn Engel 写道:
> >On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
> >>
> >>Check and set sdp->exclude should be atomic when set in sg_open().
> >
> >The patch is line-wrapped.  More importantly, it doesn't seem to do
> It's shorter than the original line, so I just leave it like this...

Sure.  What I meant by line-wrapped is that your mailer mangled the
patch.  Those two lines should have been one:
> >>-                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
> >>? 0 : set_exclude(sdp, 1)));

> >what your description indicates it should do.  And lastly, does this
> >fix a bug, possibly even one you have a testcase for, or was it found
> >by code inspection?
> I found it by code inspection. A race condition may happen with the
> old code 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. So it's necessary to check again with sg_open_exclusive_lock
> held to ensure only one can set sdp->exclude and return >0 to break
> out from wait_event loop.

Makes sense.  And reading the code again, I have to wonder what monkey
came up with the get_exclude/set_exclude functions.

Can I sucker you into a slightly larger cleanup?  I think the entire
"get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)" should be simplified.
And once you add the try_set_exclude(), set_exclude will only ever do
clear_exclude, so you might as well rename and simplify that as well.

Let no good deed go unpunished.

Jörn

--
It's just what we asked for, but not what we want!
-- anonymous

Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

[vaughan]

于 2013年06月05日 23:41, Jörn Engel 写道:
> On Thu, 6 June 2013 00:16:45 +0800, vaughan wrote:
>> 于 2013年06月05日 21:27, Jörn Engel 写道:
>>> On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
>>>>
>>>> Check and set sdp->exclude should be atomic when set in sg_open().
>>>
>>> The patch is line-wrapped.  More importantly, it doesn't seem to do
>> It's shorter than the original line, so I just leave it like this...
>
> Sure.  What I meant by line-wrapped is that your mailer mangled the
> patch.  Those two lines should have been one:
>>>> -                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
>>>> ? 0 : set_exclude(sdp, 1)));
>
>>> what your description indicates it should do.  And lastly, does this
>>> fix a bug, possibly even one you have a testcase for, or was it found
>>> by code inspection?
>> I found it by code inspection. A race condition may happen with the
>> old code 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. So it's necessary to check again with sg_open_exclusive_lock
>> held to ensure only one can set sdp->exclude and return >0 to break
>> out from wait_event loop.
>
> Makes sense.  And reading the code again, I have to wonder what monkey
> came up with the get_exclude/set_exclude functions.
>
> Can I sucker you into a slightly larger cleanup?  I think the entire
> "get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)" should be simplified.
> And once you add the try_set_exclude(), set_exclude will only ever do
> clear_exclude, so you might as well rename and simplify that as well.
I find my patch is not enough to avoid this race condition said above. 
Since sg_add_sfp() just do an add_to_list without check and wait_event 
check don't set a sign to announce a future add_to_list is on going, the 
time window between wait_event and sg_add_sfp gives others to open sg 
before the prechecked sg_add_sfp() called.

The same case also happens when one shared and one exclude open occur 
simultaneously. If the shared open pass the precheck stage and ready to 
sg_add_sfp(). At this time another exclude open will also pass the check:
   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : 
try_set_exclude(sdp)));
Then, both open can succeed.

I think the point is we separate the check&add routine and haven't set 
an sign to let others wait until the whole actions complete. I suppose 
we may change the steps a bit to avoid trouble like this. If we can 
malloc&initialize sfp at first, and then check&add sfp under the 
protection of sg_index_lock, everything seems to be quite simple.


Regards,
Vaughan

>
> Let no good deed go unpunished.
>
> Jörn
>
> --
> It's just what we asked for, but not what we want!
> -- anonymous
>

Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open

[vaughan]

于 2013年06月06日 15:19, vaughan 写道:
> 于 2013年06月05日 23:41, Jörn Engel 写道:
>> On Thu, 6 June 2013 00:16:45 +0800, vaughan wrote:
>>> 于 2013年06月05日 21:27, Jörn Engel 写道:
>>>> On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote:
>>>>>
>>>>> Check and set sdp->exclude should be atomic when set in sg_open().
>>>>
>>>> The patch is line-wrapped.  More importantly, it doesn't seem to do
>>> It's shorter than the original line, so I just leave it like this...
>>
>> Sure.  What I meant by line-wrapped is that your mailer mangled the
>> patch.  Those two lines should have been one:
>>>>> -                       ((!sfds_list_empty(sdp) || get_exclude(sdp))
>>>>> ? 0 : set_exclude(sdp, 1)));
>>
>>>> what your description indicates it should do.  And lastly, does this
>>>> fix a bug, possibly even one you have a testcase for, or was it found
>>>> by code inspection?
>>> I found it by code inspection. A race condition may happen with the
>>> old code 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. So it's necessary to check again with sg_open_exclusive_lock
>>> held to ensure only one can set sdp->exclude and return >0 to break
>>> out from wait_event loop.
>>
>> Makes sense.  And reading the code again, I have to wonder what monkey
>> came up with the get_exclude/set_exclude functions.
>>
>> Can I sucker you into a slightly larger cleanup?  I think the entire
>> "get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)" should be simplified.
>> And once you add the try_set_exclude(), set_exclude will only ever do
>> clear_exclude, so you might as well rename and simplify that as well.
> I find my patch is not enough to avoid this race condition said above.
> Since sg_add_sfp() just do an add_to_list without check and wait_event
> check don't set a sign to announce a future add_to_list is on going, the
> time window between wait_event and sg_add_sfp gives others to open sg
> before the prechecked sg_add_sfp() called.
>
> The same case also happens when one shared and one exclude open occur
> simultaneously. If the shared open pass the precheck stage and ready to
> sg_add_sfp(). At this time another exclude open will also pass the check:
>    ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 :
> try_set_exclude(sdp)));
> Then, both open can succeed.
>
> I think the point is we separate the check&add routine and haven't set
> an sign to let others wait until the whole actions complete. I suppose
> we may change the steps a bit to avoid trouble like this. If we can
> malloc&initialize sfp at first, and then check&add sfp under the
> protection of sg_index_lock, everything seems to be quite simple.
We also should prevent sg_device change those parameters which are 
needed to copy to sfp during sfp initialization.

Regards,
Vaughan

>
>
> Regards,
> Vaughan
>
>>
>> Let no good deed go unpunished.
>>
>> Jörn
>>
>> --
>> It's just what we asked for, but not what we want!
>> -- anonymous
>>

[PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[vaughan]

Rewrite the last patch.
Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 152 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 90 insertions(+), 62 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..b0ea73f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -103,11 +103,8 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);	/* protect sg index */
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -144,7 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
+	/* sfd_siblings is protected by sfd_lock of sg_device */
 	struct list_head sfd_siblings;
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
@@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
 	struct list_head sfds;
+	int toopen;		/* number of who are ready to open sg */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
@@ -224,25 +221,44 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
 }
 
-static int get_exclude(Sg_device *sdp)
+static void clear_exclude(Sg_device *sdp)
 {
 	unsigned long flags;
-	int ret;
 
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
+	sdp->exclude = 0;
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
+	return;
+}
+
+/* we can add exclusively only when no other addition is going on */
+static int try_add_exclude(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
+	if (list_empty(&sdp->sfds) && !sdp->toopen) {
+		sdp->exclude = 1;
+		sdp->toopen++;
+		ret = 1;
+	}
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
-static int set_exclude(Sg_device *sdp, char val)
+static int try_add_shareable(Sg_device *sdp)
 {
 	unsigned long flags;
+	int ret = 0;
 
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
+	if (!sdp->exclude && sdp->toopen != INT_MAX) {
+		sdp->toopen++;
+		ret = 1;
+	}
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
+	return ret;
 }
 
 static int sfds_list_empty(Sg_device *sdp)
@@ -250,9 +266,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -266,6 +282,7 @@ sg_open(struct inode *inode, struct file *filp)
 	Sg_fd *sfp;
 	int res;
 	int retval;
+	unsigned long iflags;
 
 	nonseekable_open(inode, filp);
 	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
@@ -293,27 +310,26 @@ sg_open(struct inode *inode, struct file *filp)
 		goto error_out;
 	}
 
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
+		goto error_out;
+	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL)
+			res = try_add_exclude(sdp);
+		else
+			res = try_add_shareable(sdp);
+		if (!res) {
 			retval = -EBUSY;
 			goto error_out;
 		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
+	} else {
+		if (flags & O_EXCL)
+			res = wait_event_interruptible(sdp->o_excl_wait,
+				try_add_exclude(sdp));
+		else
+			res = wait_event_interruptible(sdp->o_excl_wait,
+				try_add_shareable(sdp));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;
@@ -331,10 +347,12 @@ 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);
-		}
+		spin_lock_irqsave(&sdp->sfd_lock, iflags);
+		sdp->toopen--;
+		if (flags & O_EXCL)
+			sdp->exclude = 0;	/* undo if error */
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
+		wake_up_interruptible(&sdp->o_excl_wait);
 		retval = -ENOMEM;
 		goto error_out;
 	}
@@ -362,7 +380,7 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	set_exclude(sdp, 0);
+	clear_exclude(sdp);
 	wake_up_interruptible(&sdp->o_excl_wait);
 
 	scsi_autopm_put_device(sdp->device);
@@ -1414,6 +1432,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_waitqueue_head(&sdp->o_excl_wait);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1557,10 +1576,12 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
 	sdp->detached = 1;
+	spin_lock(&sdp->sfd_lock);
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2085,9 +2106,12 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	sdp->toopen--; /* ongoing open is complete */
+	if (sdp->toopen == INT_MAX-1)
+		wake_up_interruptible(&sdp->o_excl_wait);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2137,9 +2161,9 @@ static void sg_remove_sfp(struct kref *kref)
 	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	wake_up_interruptible(&sdp->o_excl_wait);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
@@ -2531,7 +2555,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2616,22 +2640,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+					(s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+					 scsidp->host->host_no,
+					 scsidp->channel, scsidp->id,
+					 scsidp->lun,
+					 scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+					sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.7.11.7

Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[vaughan]

Hi Jörn Engel,

Ping.
How about this one? I found my lat patch hasn't fix the issue, so I
modified it a little more. Last thread is:
Subject: Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open
Message-ID: <20130605154106.GA2737@logfs.org>

Regards,
Vaughan

于 2013年06月17日 21:10, vaughan 写道:
> Rewrite the last patch.
> Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
> Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
> ---
>  drivers/scsi/sg.c | 152 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 90 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 25b5455..b0ea73f 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -103,11 +103,8 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
>  static int sg_add(struct device *, struct class_interface *);
>  static void sg_remove(struct device *, struct class_interface *);
>  
> -static DEFINE_SPINLOCK(sg_open_exclusive_lock);
> -
>  static DEFINE_IDR(sg_index_idr);
> -static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
> -							   file descriptor list for device */
> +static DEFINE_RWLOCK(sg_index_lock);	/* protect sg index */
>  
>  static struct class_interface sg_interface = {
>  	.add_dev	= sg_add,
> @@ -144,7 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
>  } Sg_request;
>  
>  typedef struct sg_fd {		/* holds the state of a file descriptor */
> -	/* sfd_siblings is protected by sg_index_lock */
> +	/* sfd_siblings is protected by sfd_lock of sg_device */
>  	struct list_head sfd_siblings;
>  	struct sg_device *parentdp;	/* owning device */
>  	wait_queue_head_t read_wait;	/* queue read until command done */
> @@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
>  	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
>  	int sg_tablesize;	/* adapter's max scatter-gather table size */
>  	u32 index;		/* device index number */
> -	/* sfds is protected by sg_index_lock */
> +	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
>  	struct list_head sfds;
> +	int toopen;		/* number of who are ready to open sg */
>  	volatile char detached;	/* 0->attached, 1->detached pending removal */
> -	/* exclude protected by sg_open_exclusive_lock */
>  	char exclude;		/* opened for exclusive access */
>  	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
>  	struct gendisk *disk;
> @@ -224,25 +221,44 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
>  	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
>  }
>  
> -static int get_exclude(Sg_device *sdp)
> +static void clear_exclude(Sg_device *sdp)
>  {
>  	unsigned long flags;
> -	int ret;
>  
> -	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
> -	ret = sdp->exclude;
> -	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
> +	sdp->exclude = 0;
> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
> +	return;
> +}
> +
> +/* we can add exclusively only when no other addition is going on */
> +static int try_add_exclude(Sg_device *sdp)
> +{
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
> +	if (list_empty(&sdp->sfds) && !sdp->toopen) {
> +		sdp->exclude = 1;
> +		sdp->toopen++;
> +		ret = 1;
> +	}
> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>  	return ret;
>  }
>  
> -static int set_exclude(Sg_device *sdp, char val)
> +static int try_add_shareable(Sg_device *sdp)
>  {
>  	unsigned long flags;
> +	int ret = 0;
>  
> -	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
> -	sdp->exclude = val;
> -	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
> -	return val;
> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
> +	if (!sdp->exclude && sdp->toopen != INT_MAX) {
> +		sdp->toopen++;
> +		ret = 1;
> +	}
> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
> +	return ret;
>  }
>  
>  static int sfds_list_empty(Sg_device *sdp)
> @@ -250,9 +266,9 @@ static int sfds_list_empty(Sg_device *sdp)
>  	unsigned long flags;
>  	int ret;
>  
> -	read_lock_irqsave(&sg_index_lock, flags);
> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
>  	ret = list_empty(&sdp->sfds);
> -	read_unlock_irqrestore(&sg_index_lock, flags);
> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>  	return ret;
>  }
>  
> @@ -266,6 +282,7 @@ sg_open(struct inode *inode, struct file *filp)
>  	Sg_fd *sfp;
>  	int res;
>  	int retval;
> +	unsigned long iflags;
>  
>  	nonseekable_open(inode, filp);
>  	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
> @@ -293,27 +310,26 @@ sg_open(struct inode *inode, struct file *filp)
>  		goto error_out;
>  	}
>  
> -	if (flags & O_EXCL) {
> -		if (O_RDONLY == (flags & O_ACCMODE)) {
> -			retval = -EPERM; /* Can't lock it with read only access */
> -			goto error_out;
> -		}
> -		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
> -			retval = -EBUSY;
> -			goto error_out;
> -		}
> -		res = wait_event_interruptible(sdp->o_excl_wait,
> -					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
> -		if (res) {
> -			retval = res;	/* -ERESTARTSYS because signal hit process */
> -			goto error_out;
> -		}
> -	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
> -		if (flags & O_NONBLOCK) {
> +	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
> +		retval = -EPERM; /* Can't lock it with read only access */
> +		goto error_out;
> +	}
> +	if (flags & O_NONBLOCK) {
> +		if (flags & O_EXCL)
> +			res = try_add_exclude(sdp);
> +		else
> +			res = try_add_shareable(sdp);
> +		if (!res) {
>  			retval = -EBUSY;
>  			goto error_out;
>  		}
> -		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
> +	} else {
> +		if (flags & O_EXCL)
> +			res = wait_event_interruptible(sdp->o_excl_wait,
> +				try_add_exclude(sdp));
> +		else
> +			res = wait_event_interruptible(sdp->o_excl_wait,
> +				try_add_shareable(sdp));
>  		if (res) {
>  			retval = res;	/* -ERESTARTSYS because signal hit process */
>  			goto error_out;
> @@ -331,10 +347,12 @@ 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);
> -		}
> +		spin_lock_irqsave(&sdp->sfd_lock, iflags);
> +		sdp->toopen--;
> +		if (flags & O_EXCL)
> +			sdp->exclude = 0;	/* undo if error */
> +		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
> +		wake_up_interruptible(&sdp->o_excl_wait);
>  		retval = -ENOMEM;
>  		goto error_out;
>  	}
> @@ -362,7 +380,7 @@ sg_release(struct inode *inode, struct file *filp)
>  		return -ENXIO;
>  	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
>  
> -	set_exclude(sdp, 0);
> +	clear_exclude(sdp);
>  	wake_up_interruptible(&sdp->o_excl_wait);
>  
>  	scsi_autopm_put_device(sdp->device);
> @@ -1414,6 +1432,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
>  	disk->first_minor = k;
>  	sdp->disk = disk;
>  	sdp->device = scsidp;
> +	spin_lock_init(&sdp->sfd_lock);
>  	INIT_LIST_HEAD(&sdp->sfds);
>  	init_waitqueue_head(&sdp->o_excl_wait);
>  	sdp->sg_tablesize = queue_max_segments(q);
> @@ -1557,10 +1576,12 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
>  	/* Need a write lock to set sdp->detached. */
>  	write_lock_irqsave(&sg_index_lock, iflags);
>  	sdp->detached = 1;
> +	spin_lock(&sdp->sfd_lock);
>  	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
>  		wake_up_interruptible(&sfp->read_wait);
>  		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
>  	}
> +	spin_unlock(&sdp->sfd_lock);
>  	write_unlock_irqrestore(&sg_index_lock, iflags);
>  
>  	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
> @@ -2085,9 +2106,12 @@ sg_add_sfp(Sg_device * sdp, int dev)
>  	sfp->cmd_q = SG_DEF_COMMAND_Q;
>  	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
>  	sfp->parentdp = sdp;
> -	write_lock_irqsave(&sg_index_lock, iflags);
> +	spin_lock_irqsave(&sdp->sfd_lock, iflags);
>  	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
> -	write_unlock_irqrestore(&sg_index_lock, iflags);
> +	sdp->toopen--; /* ongoing open is complete */
> +	if (sdp->toopen == INT_MAX-1)
> +		wake_up_interruptible(&sdp->o_excl_wait);
> +	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
>  	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
>  	if (unlikely(sg_big_buff != def_reserved_size))
>  		sg_big_buff = def_reserved_size;
> @@ -2137,9 +2161,9 @@ static void sg_remove_sfp(struct kref *kref)
>  	struct sg_device *sdp = sfp->parentdp;
>  	unsigned long iflags;
>  
> -	write_lock_irqsave(&sg_index_lock, iflags);
> +	spin_lock_irqsave(&sdp->sfd_lock, iflags);
>  	list_del(&sfp->sfd_siblings);
> -	write_unlock_irqrestore(&sg_index_lock, iflags);
> +	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
>  	wake_up_interruptible(&sdp->o_excl_wait);
>  
>  	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
> @@ -2531,7 +2555,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
>  	return 0;
>  }
>  
> -/* must be called while holding sg_index_lock */
> +/* must be called while holding sg_index_lock and sfd_lock */
>  static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
>  {
>  	int k, m, new_interface, blen, usg;
> @@ -2616,22 +2640,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
>  
>  	read_lock_irqsave(&sg_index_lock, iflags);
>  	sdp = it ? sg_lookup_dev(it->index) : NULL;
> -	if (sdp && !list_empty(&sdp->sfds)) {
> -		struct scsi_device *scsidp = sdp->device;
> +	if (sdp) {
> +		spin_lock(&sdp->sfd_lock);
> +		if (!list_empty(&sdp->sfds)) {
> +			struct scsi_device *scsidp = sdp->device;
>  
> -		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
> -		if (sdp->detached)
> -			seq_printf(s, "detached pending close ");
> -		else
> -			seq_printf
> -			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
> -			     scsidp->host->host_no,
> -			     scsidp->channel, scsidp->id,
> -			     scsidp->lun,
> -			     scsidp->host->hostt->emulated);
> -		seq_printf(s, " sg_tablesize=%d excl=%d\n",
> -			   sdp->sg_tablesize, get_exclude(sdp));
> -		sg_proc_debug_helper(s, sdp);
> +			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
> +			if (sdp->detached)
> +				seq_printf(s, "detached pending close ");
> +			else
> +				seq_printf
> +					(s, "scsi%d chan=%d id=%d lun=%d   em=%d",
> +					 scsidp->host->host_no,
> +					 scsidp->channel, scsidp->id,
> +					 scsidp->lun,
> +					 scsidp->host->hostt->emulated);
> +			seq_printf(s, " sg_tablesize=%d excl=%d\n",
> +					sdp->sg_tablesize, sdp->exclude);
> +			sg_proc_debug_helper(s, sdp);
> +		}
> +		spin_unlock(&sdp->sfd_lock);
>  	}
>  	read_unlock_irqrestore(&sg_index_lock, iflags);
>  	return 0;
> 

Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[vaughan]

On 06/26/2013 09:37 AM, vaughan wrote:
> Hi Jörn Engel,
>
> Ping.
> How about this one? I found my lat patch hasn't fix the issue, so I
> modified it a little more. Last thread is:
> Subject: Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open
> Message-ID: <20130605154106.GA2737@logfs.org>
>
> Regards,
> Vaughan
>
> 于 2013年06月17日 21:10, vaughan 写道:
>> Rewrite the last patch.
>> Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
>> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
>> Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().
>>
>> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
>> ---
>>   drivers/scsi/sg.c | 152 ++++++++++++++++++++++++++++++++----------------------
>>   1 file changed, 90 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 25b5455..b0ea73f 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -103,11 +103,8 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
>>   static int sg_add(struct device *, struct class_interface *);
>>   static void sg_remove(struct device *, struct class_interface *);
>>
>> -static DEFINE_SPINLOCK(sg_open_exclusive_lock);
>> -
>>   static DEFINE_IDR(sg_index_idr);
>> -static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
>> -							   file descriptor list for device */
>> +static DEFINE_RWLOCK(sg_index_lock);	/* protect sg index */
>>
>>   static struct class_interface sg_interface = {
>>   	.add_dev	= sg_add,
>> @@ -144,7 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
>>   } Sg_request;
>>
>>   typedef struct sg_fd {		/* holds the state of a file descriptor */
>> -	/* sfd_siblings is protected by sg_index_lock */
>> +	/* sfd_siblings is protected by sfd_lock of sg_device */
>>   	struct list_head sfd_siblings;
>>   	struct sg_device *parentdp;	/* owning device */
>>   	wait_queue_head_t read_wait;	/* queue read until command done */
>> @@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
>>   	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
>>   	int sg_tablesize;	/* adapter's max scatter-gather table size */
>>   	u32 index;		/* device index number */
>> -	/* sfds is protected by sg_index_lock */
>> +	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
>>   	struct list_head sfds;
>> +	int toopen;		/* number of who are ready to open sg */
>>   	volatile char detached;	/* 0->attached, 1->detached pending removal */
>> -	/* exclude protected by sg_open_exclusive_lock */
>>   	char exclude;		/* opened for exclusive access */
>>   	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
>>   	struct gendisk *disk;
>> @@ -224,25 +221,44 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
>>   	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
>>   }
>>
>> -static int get_exclude(Sg_device *sdp)
>> +static void clear_exclude(Sg_device *sdp)
>>   {
>>   	unsigned long flags;
>> -	int ret;
>>
>> -	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
>> -	ret = sdp->exclude;
>> -	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
>> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
>> +	sdp->exclude = 0;
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>> +	return;
>> +}
>> +
>> +/* we can add exclusively only when no other addition is going on */
>> +static int try_add_exclude(Sg_device *sdp)
>> +{
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
>> +	if (list_empty(&sdp->sfds) && !sdp->toopen) {
>> +		sdp->exclude = 1;
>> +		sdp->toopen++;
>> +		ret = 1;
>> +	}
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>>   	return ret;
>>   }
>>
>> -static int set_exclude(Sg_device *sdp, char val)
>> +static int try_add_shareable(Sg_device *sdp)
>>   {
>>   	unsigned long flags;
>> +	int ret = 0;
>>
>> -	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
>> -	sdp->exclude = val;
>> -	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
>> -	return val;
>> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
>> +	if (!sdp->exclude && sdp->toopen != INT_MAX) {
>> +		sdp->toopen++;
>> +		ret = 1;
>> +	}
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>> +	return ret;
>>   }
>>
>>   static int sfds_list_empty(Sg_device *sdp)
>> @@ -250,9 +266,9 @@ static int sfds_list_empty(Sg_device *sdp)
>>   	unsigned long flags;
>>   	int ret;
>>
>> -	read_lock_irqsave(&sg_index_lock, flags);
>> +	spin_lock_irqsave(&sdp->sfd_lock, flags);
>>   	ret = list_empty(&sdp->sfds);
>> -	read_unlock_irqrestore(&sg_index_lock, flags);
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
>>   	return ret;
>>   }
>>
>> @@ -266,6 +282,7 @@ sg_open(struct inode *inode, struct file *filp)
>>   	Sg_fd *sfp;
>>   	int res;
>>   	int retval;
>> +	unsigned long iflags;
>>
>>   	nonseekable_open(inode, filp);
>>   	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
>> @@ -293,27 +310,26 @@ sg_open(struct inode *inode, struct file *filp)
>>   		goto error_out;
>>   	}
>>
>> -	if (flags & O_EXCL) {
>> -		if (O_RDONLY == (flags & O_ACCMODE)) {
>> -			retval = -EPERM; /* Can't lock it with read only access */
>> -			goto error_out;
>> -		}
>> -		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
>> -			retval = -EBUSY;
>> -			goto error_out;
>> -		}
>> -		res = wait_event_interruptible(sdp->o_excl_wait,
>> -					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
>> -		if (res) {
>> -			retval = res;	/* -ERESTARTSYS because signal hit process */
>> -			goto error_out;
>> -		}
>> -	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
>> -		if (flags & O_NONBLOCK) {
>> +	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
>> +		retval = -EPERM; /* Can't lock it with read only access */
>> +		goto error_out;
>> +	}
>> +	if (flags & O_NONBLOCK) {
>> +		if (flags & O_EXCL)
>> +			res = try_add_exclude(sdp);
>> +		else
>> +			res = try_add_shareable(sdp);
>> +		if (!res) {
>>   			retval = -EBUSY;
>>   			goto error_out;
>>   		}
>> -		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
>> +	} else {
>> +		if (flags & O_EXCL)
>> +			res = wait_event_interruptible(sdp->o_excl_wait,
>> +				try_add_exclude(sdp));
>> +		else
>> +			res = wait_event_interruptible(sdp->o_excl_wait,
>> +				try_add_shareable(sdp));
>>   		if (res) {
>>   			retval = res;	/* -ERESTARTSYS because signal hit process */
>>   			goto error_out;
>> @@ -331,10 +347,12 @@ 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);
>> -		}
>> +		spin_lock_irqsave(&sdp->sfd_lock, iflags);
>> +		sdp->toopen--;
>> +		if (flags & O_EXCL)
>> +			sdp->exclude = 0;	/* undo if error */
>> +		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
>> +		wake_up_interruptible(&sdp->o_excl_wait);
>>   		retval = -ENOMEM;
>>   		goto error_out;
>>   	}
>> @@ -362,7 +380,7 @@ sg_release(struct inode *inode, struct file *filp)
>>   		return -ENXIO;
>>   	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
>>
>> -	set_exclude(sdp, 0);
>> +	clear_exclude(sdp);
>>   	wake_up_interruptible(&sdp->o_excl_wait);
>>
>>   	scsi_autopm_put_device(sdp->device);
>> @@ -1414,6 +1432,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
>>   	disk->first_minor = k;
>>   	sdp->disk = disk;
>>   	sdp->device = scsidp;
>> +	spin_lock_init(&sdp->sfd_lock);
>>   	INIT_LIST_HEAD(&sdp->sfds);
>>   	init_waitqueue_head(&sdp->o_excl_wait);
>>   	sdp->sg_tablesize = queue_max_segments(q);
>> @@ -1557,10 +1576,12 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
>>   	/* Need a write lock to set sdp->detached. */
>>   	write_lock_irqsave(&sg_index_lock, iflags);
>>   	sdp->detached = 1;
>> +	spin_lock(&sdp->sfd_lock);
>>   	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
>>   		wake_up_interruptible(&sfp->read_wait);
>>   		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
>>   	}
>> +	spin_unlock(&sdp->sfd_lock);
>>   	write_unlock_irqrestore(&sg_index_lock, iflags);
>>
>>   	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
>> @@ -2085,9 +2106,12 @@ sg_add_sfp(Sg_device * sdp, int dev)
>>   	sfp->cmd_q = SG_DEF_COMMAND_Q;
>>   	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
>>   	sfp->parentdp = sdp;
>> -	write_lock_irqsave(&sg_index_lock, iflags);
>> +	spin_lock_irqsave(&sdp->sfd_lock, iflags);
>>   	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
>> -	write_unlock_irqrestore(&sg_index_lock, iflags);
>> +	sdp->toopen--; /* ongoing open is complete */
>> +	if (sdp->toopen == INT_MAX-1)
>> +		wake_up_interruptible(&sdp->o_excl_wait);
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
>>   	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
>>   	if (unlikely(sg_big_buff != def_reserved_size))
>>   		sg_big_buff = def_reserved_size;
>> @@ -2137,9 +2161,9 @@ static void sg_remove_sfp(struct kref *kref)
>>   	struct sg_device *sdp = sfp->parentdp;
>>   	unsigned long iflags;
>>
>> -	write_lock_irqsave(&sg_index_lock, iflags);
>> +	spin_lock_irqsave(&sdp->sfd_lock, iflags);
>>   	list_del(&sfp->sfd_siblings);
>> -	write_unlock_irqrestore(&sg_index_lock, iflags);
>> +	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
>>   	wake_up_interruptible(&sdp->o_excl_wait);
>>
>>   	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
>> @@ -2531,7 +2555,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
>>   	return 0;
>>   }
>>
>> -/* must be called while holding sg_index_lock */
>> +/* must be called while holding sg_index_lock and sfd_lock */
>>   static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
>>   {
>>   	int k, m, new_interface, blen, usg;
>> @@ -2616,22 +2640,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
>>
>>   	read_lock_irqsave(&sg_index_lock, iflags);
>>   	sdp = it ? sg_lookup_dev(it->index) : NULL;
>> -	if (sdp && !list_empty(&sdp->sfds)) {
>> -		struct scsi_device *scsidp = sdp->device;
>> +	if (sdp) {
>> +		spin_lock(&sdp->sfd_lock);
>> +		if (!list_empty(&sdp->sfds)) {
>> +			struct scsi_device *scsidp = sdp->device;
>>
>> -		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
>> -		if (sdp->detached)
>> -			seq_printf(s, "detached pending close ");
>> -		else
>> -			seq_printf
>> -			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
>> -			     scsidp->host->host_no,
>> -			     scsidp->channel, scsidp->id,
>> -			     scsidp->lun,
>> -			     scsidp->host->hostt->emulated);
>> -		seq_printf(s, " sg_tablesize=%d excl=%d\n",
>> -			   sdp->sg_tablesize, get_exclude(sdp));
>> -		sg_proc_debug_helper(s, sdp);
>> +			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
>> +			if (sdp->detached)
>> +				seq_printf(s, "detached pending close ");
>> +			else
>> +				seq_printf
>> +					(s, "scsi%d chan=%d id=%d lun=%d   em=%d",
>> +					 scsidp->host->host_no,
>> +					 scsidp->channel, scsidp->id,
>> +					 scsidp->lun,
>> +					 scsidp->host->hostt->emulated);
>> +			seq_printf(s, " sg_tablesize=%d excl=%d\n",
>> +					sdp->sg_tablesize, sdp->exclude);
>> +			sg_proc_debug_helper(s, sdp);
>> +		}
>> +		spin_unlock(&sdp->sfd_lock);
>>   	}
>>   	read_unlock_irqrestore(&sg_index_lock, iflags);
>>   	return 0;
>>

Ping. Hope you're back.
I've tested it with many combinations of exclusive and shared opens.

Regards,
Vaughan

Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[Jörn Engel]

Sorry about replying so late.

On Mon, 17 June 2013 21:10:53 +0800, vaughan wrote:
> 
> Rewrite the last patch.
> Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
> Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().
> 
...
> @@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
>  	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
>  	int sg_tablesize;	/* adapter's max scatter-gather table size */
>  	u32 index;		/* device index number */
> -	/* sfds is protected by sg_index_lock */
> +	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
>  	struct list_head sfds;
> +	int toopen;		/* number of who are ready to open sg */
                                            ^
I think the 'toopen' is a bad choice.  I'm having trouble wrapping my
head around the semantics of this variable, your description feels a
bit handwavy, the main noun is missing in the command above, I think I
found one more overflow bug,...

What you ended up doing is reimplement a rw_semaphone.  Why not use
one instead?  down_write() for exclusive access, down_read() for
non-exclusive, _trylock variants for nonblocking opens, etc.

Would this work?

Jörn

--
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello

Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[vaughan]

On 07/06/2013 01:39 AM, Jörn Engel wrote:
> Sorry about replying so late.
>
> On Mon, 17 June 2013 21:10:53 +0800, vaughan wrote:
>> Rewrite the last patch.
>> Add a new field 'toopen' in sg_device to count ongoing sg_open's. By checking both 'toopen' and 'exclude' marks when do exclusive open, old race conditions can be avoided.
>> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
>> Also did some cleanup, such as remove get_exclude() and rename set_exclude() to clear_exclude().
>>
> ...
>> @@ -171,10 +168,10 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
>>   	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
>>   	int sg_tablesize;	/* adapter's max scatter-gather table size */
>>   	u32 index;		/* device index number */
>> -	/* sfds is protected by sg_index_lock */
>> +	spinlock_t sfd_lock;	/* protect sfds, exclude, toopen */
>>   	struct list_head sfds;
>> +	int toopen;		/* number of who are ready to open sg */
>                                              ^
> I think the 'toopen' is a bad choice.  I'm having trouble wrapping my
> head around the semantics of this variable, your description feels a
> bit handwavy, the main noun is missing in the command above, I think I
> found one more overflow bug,...
>
> What you ended up doing is reimplement a rw_semaphone.  Why not use
> one instead?  down_write() for exclusive access, down_read() for
> non-exclusive, _trylock variants for nonblocking opens, etc.
The critical part of open is to add a new sfd to the list and its 
protected by the
spin_lock(sg_index_lock previously) well. So I added an counter as a 
sign rather than
introducing another spinlock or mutex which means I should deal with 
potential deadlock.
The code may be simpler with a rwsem implementation as you suggest, I'll 
modify it in
this way.

There is no overflow bug, I eliminated it with the following line :)
      if (!sdp->exclude && sdp->toopen != INT_MAX) { ...

Do you agree that I use a per device spin_lock 'sfd_lock' to protect 
sfds list and leave sg_index_lock
only protect the global sg device lookup? I think it's reasonable for 
concurrency.


Thanks,
Vaughan

>
> Would this work?

>
> Jörn
>
> --
> I've never met a human being who would want to read 17,000 pages of
> documentation, and if there was, I'd kill him to get him out of the
> gene pool.
> -- Joseph Costello

[PATCH v3 1/1] [SCSI] sg: fix race condition when do exclusive open

[vaughan]

Use rwsem to aid opens. Exclusive open has to get write lock and non-exclusive open should get read lock.
Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.

Please pay attention to sdp->detached. Previously sg_open may also race with sg_remove. Now there are two points for sg_open to detect detached and finish itself. One is at sg_device lookup and the other is when trying to link new sfp into the sfds list in sg_add_sfp.
I don't think it's necessary to do extra set_exclude and wake_up o_excl_wait in sg_release before, so I remove them and only do the cleanup in sg_remove_sfp.
Although simple testcases are passed, I'm not certain if I've fixed it well, please give me any comments as you wish.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
  drivers/scsi/sg.c | 187 ++++++++++++++++++++++++++----------------------------
  1 file changed, 89 insertions(+), 98 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..3fc8c19 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -103,11 +103,8 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
  static int sg_add(struct device *, struct class_interface *);
  static void sg_remove(struct device *, struct class_interface *);
  
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
  static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);	/* protect sg index */
  
  static struct class_interface sg_interface = {
  	.add_dev	= sg_add,
@@ -144,7 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
  } Sg_request;
  
  typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
+	/* sfd_siblings is protected by sfd_lock of sg_device */
  	struct list_head sfd_siblings;
  	struct sg_device *parentdp;	/* owning device */
  	wait_queue_head_t read_wait;	/* queue read until command done */
@@ -168,13 +165,12 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
  
  typedef struct sg_device { /* holds the state of each scsi generic device */
  	struct scsi_device *device;
-	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
  	int sg_tablesize;	/* adapter's max scatter-gather table size */
  	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect sfds, exclude */
  	struct list_head sfds;
+	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
  	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
  	char exclude;		/* opened for exclusive access */
  	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
  	struct gendisk *disk;
@@ -199,7 +195,7 @@ static void sg_remove_scat(Sg_scatter_hold * schp);
  static void sg_build_reserve(Sg_fd * sfp, int req_size);
  static void sg_link_reserve(Sg_fd * sfp, Sg_request * srp, int size);
  static void sg_unlink_reserve(Sg_fd * sfp, Sg_request * srp);
-static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev);
+static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev, int excl, int * reason);
  static void sg_remove_sfp(struct kref *);
  static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
  static Sg_request *sg_add_request(Sg_fd * sfp);
@@ -224,35 +220,14 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
  	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
  }
  
-static int get_exclude(Sg_device *sdp)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
-}
-
  static int sfds_list_empty(Sg_device *sdp)
  {
  	unsigned long flags;
  	int ret;
  
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
  	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
  	return ret;
  }
  
@@ -264,8 +239,8 @@ sg_open(struct inode *inode, struct file *filp)
  	struct request_queue *q;
  	Sg_device *sdp;
  	Sg_fd *sfp;
-	int res;
  	int retval;
+	unsigned long iflags;
  
  	nonseekable_open(inode, filp);
  	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
@@ -293,52 +268,45 @@ sg_open(struct inode *inode, struct file *filp)
  		goto error_out;
  	}
  
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	}
-	if (sdp->detached) {
-		retval = -ENODEV;
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
  		goto error_out;
  	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL) {
+			if (!down_write_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+			/* if write lock is held, there is only one user. */
+		} else {
+			if (!down_read_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+		}
+	} else {
+		if (flags & O_EXCL)
+			down_write(&sdp->o_sem);
+		else
+			down_read(&sdp->o_sem);
+	}
+
  	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
  		sdp->sgdebug = 0;
  		q = sdp->device->request_queue;
  		sdp->sg_tablesize = queue_max_segments(q);
  	}
-	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;
-	}
+	if (!(sfp = sg_add_sfp(sdp, dev, (flags & O_EXCL ? 1 : 0), &retval)))
+		goto sem_out;
+	filp->private_data = sfp;
  	retval = 0;
+sem_out:
+	if (retval)
+		if (flags & O_EXCL)
+			up_write(&sdp->o_sem);
+		else
+			up_read(&sdp->o_sem);
  error_out:
  	if (retval) {
  		scsi_autopm_put_device(sdp->device);
@@ -362,9 +330,6 @@ sg_release(struct inode *inode, struct file *filp)
  		return -ENXIO;
  	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
  
-	set_exclude(sdp, 0);
-	wake_up_interruptible(&sdp->o_excl_wait);
-
  	scsi_autopm_put_device(sdp->device);
  	kref_put(&sfp->f_ref, sg_remove_sfp);
  	return 0;
@@ -1414,8 +1379,9 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
  	disk->first_minor = k;
  	sdp->disk = disk;
  	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
  	INIT_LIST_HEAD(&sdp->sfds);
-	init_waitqueue_head(&sdp->o_excl_wait);
+	init_rwsem(&sdp->o_sem);
  	sdp->sg_tablesize = queue_max_segments(q);
  	sdp->index = k;
  	kref_init(&sdp->d_ref);
@@ -1556,11 +1522,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
  
  	/* Need a write lock to set sdp->detached. */
  	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
  	sdp->detached = 1;
  	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
  		wake_up_interruptible(&sfp->read_wait);
  		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
  	}
+	spin_unlock(&sdp->sfd_lock);
  	write_unlock_irqrestore(&sg_index_lock, iflags);
  
  	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2063,15 +2031,18 @@ sg_remove_request(Sg_fd * sfp, Sg_request * srp)
  }
  
  static Sg_fd *
-sg_add_sfp(Sg_device * sdp, int dev)
+sg_add_sfp(Sg_device * sdp, int dev, int excl, int * reason)
  {
  	Sg_fd *sfp;
  	unsigned long iflags;
  	int bufflen;
  
  	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
-	if (!sfp)
+	if (!sfp) {
+		if (reason)
+			*reason = -ENOMEM;
  		return NULL;
+	}
  
  	init_waitqueue_head(&sfp->read_wait);
  	rwlock_init(&sfp->rq_list_lock);
@@ -2085,9 +2056,19 @@ sg_add_sfp(Sg_device * sdp, int dev)
  	sfp->cmd_q = SG_DEF_COMMAND_Q;
  	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
  	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
+	if (sdp->detached) {
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
+		if (reason)
+			*reason = -ENODEV;
+		return NULL;
+	}
  	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	if (excl)
+		sdp->exclude = 1;
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
+
  	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
  	if (unlikely(sg_big_buff != def_reserved_size))
  		sg_big_buff = def_reserved_size;
@@ -2136,11 +2117,17 @@ static void sg_remove_sfp(struct kref *kref)
  	struct sg_fd *sfp = container_of(kref, struct sg_fd, f_ref);
  	struct sg_device *sdp = sfp->parentdp;
  	unsigned long iflags;
+	int excl;
  
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
  	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
-	wake_up_interruptible(&sdp->o_excl_wait);
+	if ((excl = sdp->exclude))
+		sdp->exclude = 0;
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
+	if (excl)
+		up_write(&sdp->o_sem);
+	else
+		up_read(&sdp->o_sem);
  
  	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
  	schedule_work(&sfp->ew.work);
@@ -2531,7 +2518,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
  	return 0;
  }
  
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
  static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
  {
  	int k, m, new_interface, blen, usg;
@@ -2616,22 +2603,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
  
  	read_lock_irqsave(&sg_index_lock, iflags);
  	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
  
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+					(s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+					 scsidp->host->host_no,
+					 scsidp->channel, scsidp->id,
+					 scsidp->lun,
+					 scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+					sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
  	}
  	read_unlock_irqrestore(&sg_index_lock, iflags);
  	return 0;
-- 
1.7.11.7

Re: [PATCH v3 1/1] [SCSI] sg: fix race condition when do exclusive open

[Jörn Engel]

On Mon, 8 July 2013 03:53:49 +0800, vaughan wrote:
> 
> Use rwsem to aid opens. Exclusive open has to get write lock and non-exclusive open should get read lock.
> Replace global sg_open_exclusive_lock with a per device lock - sfd_lock. Since sfds list is now protected by the lock owned by the same sg_device, sg_index_lock becomes a real global lock to only protect sg devices lookup.
> 
> Please pay attention to sdp->detached. Previously sg_open may also race with sg_remove. Now there are two points for sg_open to detect detached and finish itself. One is at sg_device lookup and the other is when trying to link new sfp into the sfds list in sg_add_sfp.
> I don't think it's necessary to do extra set_exclude and wake_up o_excl_wait in sg_release before, so I remove them and only do the cleanup in sg_remove_sfp.
> Although simple testcases are passed, I'm not certain if I've fixed it well, please give me any comments as you wish.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
> ---
>  drivers/scsi/sg.c | 187 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 89 insertions(+), 98 deletions(-)

Had a quick read and I think I like the code.  What I still dislike is
that it merges several independent functional changes into one patch.

Can you create three patches, one for the rwsem part, one for pushing
the locking down to per-device locking and one for the rest?  That
would make it easier for me to understand and trust the individual
patches and for James/Linus to revert one, if it turns out to be bad.

Jörn

--
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare

[PATCH v4 0/4] [SCSI] sg: fix race condition in sg_open

[Vaughan Cao]

There is a race when open sg with O_EXCL flag. Also a race may happen between
sg_open and sg_remove.

Changes from v3:
 * release o_sem in sg_release(), not in sg_remove_sfp().
 * not set exclude with sfd_lock held.

Vaughan Cao (4):
  [SCSI] sg: use rwsem to solve race during exclusive open
  [SCSI] sg: no need sg_open_exclusive_lock
  [SCSI] sg: checking sdp->detached isn't protected when open
  [SCSI] sg: push file descriptor list locking down to per-device
    locking

 drivers/scsi/sg.c | 186 ++++++++++++++++++++++++++----------------------------
 1 file changed, 88 insertions(+), 98 deletions(-)

-- 
1.7.11.7

[PATCH v4 1/4] [SCSI] sg: use rwsem to solve race during exclusive open

[Vaughan Cao]

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@oracle.com>
---
 drivers/scsi/sg.c | 77 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..edc395a 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -168,11 +168,11 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 
 typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
-	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
 	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
+	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
@@ -293,35 +293,35 @@ sg_open(struct inode *inode, struct file *filp)
 		goto error_out;
 	}
 
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
+		goto error_out;
+	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL) {
+			if (!down_write_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+		} else {
+			if (!down_read_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
 		}
+	} else {
+		if (flags & O_EXCL)
+			down_write(&sdp->o_sem);
+		else
+			down_read(&sdp->o_sem);
 	}
+	/* Since write lock is held, no need to check sfd_list */
+	if (flags & O_EXCL)
+		set_exclude(sdp, 1);
+
 	if (sdp->detached) {
 		retval = -ENODEV;
-		goto error_out;
+		goto sem_out;
 	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
@@ -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) {
+			set_exclude(sdp, 0);	/* undo if error */
+			up_write(&sdp->o_sem);
+		} else
+			up_read(&sdp->o_sem);
+error_out:
 		scsi_autopm_put_device(sdp->device);
 sdp_put:
 		scsi_device_put(sdp->device);
@@ -357,13 +360,18 @@ sg_release(struct inode *inode, struct file *filp)
 {
 	Sg_device *sdp;
 	Sg_fd *sfp;
+	int excl;
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
+	excl = get_exclude(sdp);
 	set_exclude(sdp, 0);
-	wake_up_interruptible(&sdp->o_excl_wait);
+	if (excl)
+		up_write(&sdp->o_sem);
+	else
+		up_read(&sdp->o_sem);
 
 	scsi_autopm_put_device(sdp->device);
 	kref_put(&sfp->f_ref, sg_remove_sfp);
@@ -1415,7 +1423,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	sdp->disk = disk;
 	sdp->device = scsidp;
 	INIT_LIST_HEAD(&sdp->sfds);
-	init_waitqueue_head(&sdp->o_excl_wait);
+	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
 	sdp->index = k;
 	kref_init(&sdp->d_ref);
@@ -2140,7 +2148,6 @@ static void sg_remove_sfp(struct kref *kref)
 	write_lock_irqsave(&sg_index_lock, iflags);
 	list_del(&sfp->sfd_siblings);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
-	wake_up_interruptible(&sdp->o_excl_wait);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
-- 
1.7.11.7

Re: [PATCH v4 1/4] [SCSI] sg: use rwsem to solve race during exclusive open

[Jörn Engel]

On Wed, 17 July 2013 23:34:03 +0800, Vaughan Cao wrote:
> Date: Wed, 17 Jul 2013 23:34:03 +0800
> From: Vaughan Cao <vaughan.cao@oracle.com>
> To: joern@logfs.org
> Cc: dgilbert@interlog.com, JBottomley@parallels.com,
> 	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
> 	vaughan.cao@oracle.com
> Subject: [PATCH v4 1/4] [SCSI] sg: use rwsem to solve race during
>  exclusive open
> X-Mailer: git-send-email 1.7.11.7
> 
> 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@oracle.com>
Reviewed-by: Joern Engel <joern@logfs.org>

Jörn

--
When you close your hand, you own nothing. When you open it up, you
own the whole world.
-- Li Mu Bai in Tiger & Dragon

[PATCH v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock

[Vaughan Cao]

Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
@exclude is used to record which type of rwsem we are holding.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index edc395a..671b760 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -103,8 +103,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
 							   file descriptor list for device */
@@ -174,7 +172,6 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
@@ -224,27 +221,6 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
 }
 
-static int get_exclude(Sg_device *sdp)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
-}
-
 static int sfds_list_empty(Sg_device *sdp)
 {
 	unsigned long flags;
@@ -317,7 +293,7 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	/* Since write lock is held, no need to check sfd_list */
 	if (flags & O_EXCL)
-		set_exclude(sdp, 1);
+		sdp->exclude = 1;	/* used by release lock */
 
 	if (sdp->detached) {
 		retval = -ENODEV;
@@ -339,7 +315,7 @@ sg_open(struct inode *inode, struct file *filp)
 	if (retval) {
 sem_out:
 		if (flags & O_EXCL) {
-			set_exclude(sdp, 0);	/* undo if error */
+			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
 		} else
 			up_read(&sdp->o_sem);
@@ -366,8 +342,8 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	excl = get_exclude(sdp);
-	set_exclude(sdp, 0);
+	excl = sdp->exclude;
+	sdp->exclude = 0;
 	if (excl)
 		up_write(&sdp->o_sem);
 	else
@@ -2637,7 +2613,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
+			   sdp->sg_tablesize, sdp->exclude);
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
-- 
1.7.11.7

Re: [PATCH v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock

[Jörn Engel]

On Wed, 17 July 2013 23:34:04 +0800, Vaughan Cao wrote:
> Date: Wed, 17 Jul 2013 23:34:04 +0800
> From: Vaughan Cao <vaughan.cao@oracle.com>
> To: joern@logfs.org
> Cc: dgilbert@interlog.com, JBottomley@parallels.com,
> 	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
> 	vaughan.cao@oracle.com
> Subject: [PATCH v4 2/4] [SCSI] sg: no need sg_open_exclusive_lock
> X-Mailer: git-send-email 1.7.11.7
> 
> Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
> @exclude is used to record which type of rwsem we are holding.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
Reviewed-by: Joern Engel <joern@logfs.org>

Jörn

--
"Error protection by error detection and correction."
-- from a university class

[PATCH v4 3/4] [SCSI] sg: checking sdp->detached isn't protected when open

[Vaughan Cao]

@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 671b760..4d2a19f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -196,7 +196,7 @@ static void sg_remove_scat(Sg_scatter_hold * schp);
 static void sg_build_reserve(Sg_fd * sfp, int req_size);
 static void sg_link_reserve(Sg_fd * sfp, Sg_request * srp, int size);
 static void sg_unlink_reserve(Sg_fd * sfp, Sg_request * srp);
-static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev);
+static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev, int * reason);
 static void sg_remove_sfp(struct kref *);
 static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
 static Sg_request *sg_add_request(Sg_fd * sfp);
@@ -295,21 +295,14 @@ sg_open(struct inode *inode, struct file *filp)
 	if (flags & O_EXCL)
 		sdp->exclude = 1;	/* used by release lock */
 
-	if (sdp->detached) {
-		retval = -ENODEV;
-		goto sem_out;
-	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);
 	}
-	if ((sfp = sg_add_sfp(sdp, dev)))
-		filp->private_data = sfp;
-	else {
-		retval = -ENOMEM;
+	if (!(sfp = sg_add_sfp(sdp, dev, &retval)))
 		goto sem_out;
-	}
+	filp->private_data = sfp;
 	retval = 0;
 
 	if (retval) {
@@ -2047,15 +2040,18 @@ sg_remove_request(Sg_fd * sfp, Sg_request * srp)
 }
 
 static Sg_fd *
-sg_add_sfp(Sg_device * sdp, int dev)
+sg_add_sfp(Sg_device * sdp, int dev, int * reason)
 {
 	Sg_fd *sfp;
 	unsigned long iflags;
 	int bufflen;
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
-	if (!sfp)
+	if (!sfp) {
+		if (reason)
+			*reason = -ENOMEM;
 		return NULL;
+	}
 
 	init_waitqueue_head(&sfp->read_wait);
 	rwlock_init(&sfp->rq_list_lock);
@@ -2070,6 +2066,12 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
 	write_lock_irqsave(&sg_index_lock, iflags);
+	if (sdp->detached) {
+		write_unlock_irqrestore(&sg_index_lock, iflags);
+		if (reason)
+			*reason = -ENODEV;
+		return NULL;
+	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
-- 
1.7.11.7

Re: [PATCH v4 3/4] [SCSI] sg: checking sdp->detached isn't protected when open

[Jörn Engel]

On Wed, 17 July 2013 23:34:05 +0800, Vaughan Cao wrote:
>
> -static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev);
> +static Sg_fd *sg_add_sfp(Sg_device * sdp, int dev, int * reason);

You can use ERR_PTR and friends instead of adding another parameter.

>  static void sg_remove_sfp(struct kref *);
>  static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
>  static Sg_request *sg_add_request(Sg_fd * sfp);
> @@ -295,21 +295,14 @@ sg_open(struct inode *inode, struct file *filp)
>  	if (flags & O_EXCL)
>  		sdp->exclude = 1;	/* used by release lock */
>  
> -	if (sdp->detached) {
> -		retval = -ENODEV;
> -		goto sem_out;
> -	}
>  	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
>  		sdp->sgdebug = 0;
>  		q = sdp->device->request_queue;
>  		sdp->sg_tablesize = queue_max_segments(q);
>  	}
> -	if ((sfp = sg_add_sfp(sdp, dev)))
> -		filp->private_data = sfp;
> -	else {
> -		retval = -ENOMEM;
> +	if (!(sfp = sg_add_sfp(sdp, dev, &retval)))
>  		goto sem_out;
> -	}

	sfp = sg_add_sfp(sdp, dev);
	if (IS_ERR(sfp)) {
		retval = PTR_ERR(sfp);
		goto sem_out;
	}

> +	filp->private_data = sfp;
>  	retval = 0;
>  
>  	if (retval) {
> @@ -2047,15 +2040,18 @@ sg_remove_request(Sg_fd * sfp, Sg_request * srp)
>  }
>  
>  static Sg_fd *
> -sg_add_sfp(Sg_device * sdp, int dev)
> +sg_add_sfp(Sg_device * sdp, int dev, int * reason)
>  {
>  	Sg_fd *sfp;
>  	unsigned long iflags;
>  	int bufflen;
>  
>  	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
> -	if (!sfp)
> +	if (!sfp) {
> +		if (reason)
> +			*reason = -ENOMEM;
>  		return NULL;
> +	}

	if (!sfp)
		return ERR_PTR(-ENOMEM);

Jörn

--
Luck is when opportunity meets good preparation.
-- Chinese proverb

[PATCH v5 3/4] [SCSI] sg: checking sdp->detached isn't protected when open

[Vaughan Cao]

@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().

Changes from v4:
 * use ERR_PTR series instead of adding another parameter in sg_add_sfp

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 671b760..f0e4785 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -295,21 +295,17 @@ sg_open(struct inode *inode, struct file *filp)
 	if (flags & O_EXCL)
 		sdp->exclude = 1;	/* used by release lock */
 
-	if (sdp->detached) {
-		retval = -ENODEV;
-		goto sem_out;
-	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);
 	}
-	if ((sfp = sg_add_sfp(sdp, dev)))
-		filp->private_data = sfp;
-	else {
-		retval = -ENOMEM;
+	sfp = sg_add_sfp(sdp, dev);
+	if (IS_ERR(sfp)) {
+		retval = PTR_ERR(sfp);
 		goto sem_out;
 	}
+	filp->private_data = sfp;
 	retval = 0;
 
 	if (retval) {
@@ -2055,7 +2051,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
 	if (!sfp)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	init_waitqueue_head(&sfp->read_wait);
 	rwlock_init(&sfp->rq_list_lock);
@@ -2070,6 +2066,10 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
 	write_lock_irqsave(&sg_index_lock, iflags);
+	if (sdp->detached) {
+		write_unlock_irqrestore(&sg_index_lock, iflags);
+		return ERR_PTR(-ENODEV);
+	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
-- 
1.7.11.7

Re: [PATCH v4 3/4] [SCSI] sg: checking sdp->detached isn't protected when open

[Jörn Engel]

On Sat, 20 July 2013 15:53:16 +0800, Xitao Cao wrote:
> 
> Thanks for your comment. Do I need to update it and resend?

Yes, please.

Jörn

--
When people work hard for you for a pat on the back, you've got
to give them that pat.
-- Robert Heinlein

[PATCH v4 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking

[Vaughan Cao]

Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 61 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4d2a19f..3ea7c09 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -104,8 +104,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -142,8 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
-	struct list_head sfd_siblings;
+	struct list_head sfd_siblings; /* protected by sfd_lock of device */
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
 	rwlock_t rq_list_lock;	/* protect access to list in req_arr */
@@ -168,7 +166,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect file descriptor list for device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
@@ -226,9 +224,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -1391,6 +1389,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1533,11 +1532,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
 	sdp->detached = 1;
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2065,15 +2066,15 @@ sg_add_sfp(Sg_device * sdp, int dev, int * reason)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
-		write_unlock_irqrestore(&sg_index_lock, iflags);
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 		if (reason)
 			*reason = -ENODEV;
 		return NULL;
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2123,9 +2124,9 @@ static void sg_remove_sfp(struct kref *kref)
 	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
@@ -2516,7 +2517,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2601,22 +2602,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+				    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+				     scsidp->host->host_no,
+				     scsidp->channel, scsidp->id,
+				     scsidp->lun,
+				     scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+				   sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.7.11.7

Re: [PATCH v4 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking

[Jörn Engel]

On Wed, 17 July 2013 23:34:06 +0800, Vaughan Cao wrote:
> Date: Wed, 17 Jul 2013 23:34:06 +0800
> From: Vaughan Cao <vaughan.cao@oracle.com>
> To: joern@logfs.org
> Cc: dgilbert@interlog.com, JBottomley@parallels.com,
> 	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
> 	vaughan.cao@oracle.com
> Subject: [PATCH v4 4/4] [SCSI] sg: push file descriptor list locking down
>  to per-device locking
> X-Mailer: git-send-email 1.7.11.7
> 
> Push file descriptor list locking down to per-device locking. Let sg_index_lock
> only protect device lookup.
> sdp->detached is also set and checked with this lock held.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>

Reviewed-by: Joern Engel <joern@logfs.org>

Jörn

--
What one programmer can do in one month, two programmers can do in two months.
-- Fred Brooks

[PATCH v5 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking

[Vaughan Cao]

Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.

Changes from v4:
 * Since I use ERR_PTR and friends in sg_add_sfp, this patch should also be
updated to resolve conflict in cherrry-pick.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 61 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f0e4785..3431d12 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -104,8 +104,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -142,8 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
-	struct list_head sfd_siblings;
+	struct list_head sfd_siblings; /* protected by sfd_lock of device */
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
 	rwlock_t rq_list_lock;	/* protect access to list in req_arr */
@@ -168,7 +166,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect file descriptor list for device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
@@ -226,9 +224,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -1394,6 +1392,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1536,11 +1535,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
 	sdp->detached = 1;
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2065,13 +2066,13 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
-		write_unlock_irqrestore(&sg_index_lock, iflags);
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 		return ERR_PTR(-ENODEV);
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2121,9 +2122,9 @@ static void sg_remove_sfp(struct kref *kref)
 	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
@@ -2514,7 +2515,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2599,22 +2600,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+				    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+				     scsidp->host->host_no,
+				     scsidp->channel, scsidp->id,
+				     scsidp->lun,
+				     scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+				   sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.7.11.7

[PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Vaughan Cao]

There is a race when open sg with O_EXCL flag. Also a race may happen between
sg_open and sg_remove.

Changes from v4:
 * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
 * [4/4] fix conflict for cherry-pick from v3.

Changes from v3:
 * release o_sem in sg_release(), not in sg_remove_sfp().
 * not set exclude with sfd_lock held.

Vaughan Cao (4):
  [SCSI] sg: use rwsem to solve race during exclusive open
  [SCSI] sg: no need sg_open_exclusive_lock
  [SCSI] sg: checking sdp->detached isn't protected when open
  [SCSI] sg: push file descriptor list locking down to per-device
    locking

 drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
 1 file changed, 83 insertions(+), 95 deletions(-)

-- 
1.7.11.7

[PATCH v5 1/4] [SCSI] sg: use rwsem to solve race during exclusive open

[Vaughan Cao]

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@oracle.com>
---
 drivers/scsi/sg.c | 77 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 25b5455..edc395a 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -168,11 +168,11 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 
 typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
-	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
 	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
+	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
@@ -293,35 +293,35 @@ sg_open(struct inode *inode, struct file *filp)
 		goto error_out;
 	}
 
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
+		goto error_out;
+	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL) {
+			if (!down_write_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+		} else {
+			if (!down_read_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
 		}
+	} else {
+		if (flags & O_EXCL)
+			down_write(&sdp->o_sem);
+		else
+			down_read(&sdp->o_sem);
 	}
+	/* Since write lock is held, no need to check sfd_list */
+	if (flags & O_EXCL)
+		set_exclude(sdp, 1);
+
 	if (sdp->detached) {
 		retval = -ENODEV;
-		goto error_out;
+		goto sem_out;
 	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
@@ -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) {
+			set_exclude(sdp, 0);	/* undo if error */
+			up_write(&sdp->o_sem);
+		} else
+			up_read(&sdp->o_sem);
+error_out:
 		scsi_autopm_put_device(sdp->device);
 sdp_put:
 		scsi_device_put(sdp->device);
@@ -357,13 +360,18 @@ sg_release(struct inode *inode, struct file *filp)
 {
 	Sg_device *sdp;
 	Sg_fd *sfp;
+	int excl;
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
+	excl = get_exclude(sdp);
 	set_exclude(sdp, 0);
-	wake_up_interruptible(&sdp->o_excl_wait);
+	if (excl)
+		up_write(&sdp->o_sem);
+	else
+		up_read(&sdp->o_sem);
 
 	scsi_autopm_put_device(sdp->device);
 	kref_put(&sfp->f_ref, sg_remove_sfp);
@@ -1415,7 +1423,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	sdp->disk = disk;
 	sdp->device = scsidp;
 	INIT_LIST_HEAD(&sdp->sfds);
-	init_waitqueue_head(&sdp->o_excl_wait);
+	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
 	sdp->index = k;
 	kref_init(&sdp->d_ref);
@@ -2140,7 +2148,6 @@ static void sg_remove_sfp(struct kref *kref)
 	write_lock_irqsave(&sg_index_lock, iflags);
 	list_del(&sfp->sfd_siblings);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
-	wake_up_interruptible(&sdp->o_excl_wait);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
-- 
1.7.11.7

Re: [PATCH v5 1/4] [SCSI] sg: use rwsem to solve race during exclusive open

[James Bottomley]

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@oracle.com>

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

[PATCH v6 0/4][SCSI] sg: fix race condition in sg_open

[Vaughan Cao]

There is a race when open sg with O_EXCL flag. Also a race may happen between
sg_open and sg_remove.

Changes from v5:
 Patches based on v3.11-rc7 and passed sg_tst_excl test.
 * [1/4] * remove unused variables - res,sdp.
	 * fix insane code dealing with sg_add_sfp.
 * [2/4] resolve conflict with v3.11-rc7. 
 * [3/4] remove sem_out label.
 * [4/4] add sdp definition.

Changes from v4:
 * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
 * [4/4] fix conflict for cherry-pick from v3.

Changes from v3:
 * release o_sem in sg_release(), not in sg_remove_sfp().
 * not set exclude with sfd_lock held.

Vaughan Cao (4):
  sg: use rwsem to solve race during exclusive open
  sg: no need sg_open_exclusive_lock
  sg: checking sdp->detached isn't protected when open
  sg: push file descriptor list locking down to per-device locking

 drivers/scsi/sg.c | 173 +++++++++++++++++++++++++-----------------------------
 1 file changed, 80 insertions(+), 93 deletions(-)

-- 
1.8.3.1

[PATCH v6 1/4] sg: use rwsem to solve race during exclusive open

[Vaughan Cao]

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.

Changes from v5:
 * remove unused variables
 * fix insane code dealing with sg_add_sfp.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 83 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..7a54c92 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -170,11 +170,11 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 
 typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
-	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
 	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
+	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
@@ -265,7 +265,6 @@ sg_open(struct inode *inode, struct file *filp)
 	struct request_queue *q;
 	Sg_device *sdp;
 	Sg_fd *sfp;
-	int res;
 	int retval;
 
 	nonseekable_open(inode, filp);
@@ -294,35 +293,35 @@ sg_open(struct inode *inode, struct file *filp)
 		goto error_out;
 	}
 
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
+		goto error_out;
+	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL) {
+			if (!down_write_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+		} else {
+			if (!down_read_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
 		}
+	} else {
+		if (flags & O_EXCL)
+			down_write(&sdp->o_sem);
+		else
+			down_read(&sdp->o_sem);
 	}
+	/* Since write lock is held, no need to check sfd_list */
+	if (flags & O_EXCL)
+		set_exclude(sdp, 1);
+
 	if (sdp->detached) {
 		retval = -ENODEV;
-		goto error_out;
+		goto sem_out;
 	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
@@ -331,17 +330,20 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	if ((sfp = sg_add_sfp(sdp, dev)))
 		filp->private_data = sfp;
-	else {
+		/* retval is already provably zero at this point because of the
+		 * check after retval = scsi_autopm_get_device(sdp->device))
+		 */
+	else
+		retval = -ENOMEM;
+
+	if (retval) {
+sem_out:
 		if (flags & O_EXCL) {
 			set_exclude(sdp, 0);	/* undo if error */
-			wake_up_interruptible(&sdp->o_excl_wait);
-		}
-		retval = -ENOMEM;
-		goto error_out;
-	}
-	retval = 0;
+			up_write(&sdp->o_sem);
+		} else
+			up_read(&sdp->o_sem);
 error_out:
-	if (retval) {
 		scsi_autopm_put_device(sdp->device);
 sdp_put:
 		scsi_device_put(sdp->device);
@@ -358,13 +360,18 @@ sg_release(struct inode *inode, struct file *filp)
 {
 	Sg_device *sdp;
 	Sg_fd *sfp;
+	int excl;
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
+	excl = get_exclude(sdp);
 	set_exclude(sdp, 0);
-	wake_up_interruptible(&sdp->o_excl_wait);
+	if (excl)
+		up_write(&sdp->o_sem);
+	else
+		up_read(&sdp->o_sem);
 
 	scsi_autopm_put_device(sdp->device);
 	kref_put(&sfp->f_ref, sg_remove_sfp);
@@ -1416,7 +1423,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	sdp->disk = disk;
 	sdp->device = scsidp;
 	INIT_LIST_HEAD(&sdp->sfds);
-	init_waitqueue_head(&sdp->o_excl_wait);
+	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
 	sdp->index = k;
 	kref_init(&sdp->d_ref);
@@ -2127,13 +2134,11 @@ static void sg_remove_sfp_usercontext(struct work_struct *work)
 static void sg_remove_sfp(struct kref *kref)
 {
 	struct sg_fd *sfp = container_of(kref, struct sg_fd, f_ref);
-	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
 	write_lock_irqsave(&sg_index_lock, iflags);
 	list_del(&sfp->sfd_siblings);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
-	wake_up_interruptible(&sdp->o_excl_wait);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
-- 
1.8.3.1

Re: [PATCH v6 1/4] sg: use rwsem to solve race during exclusive open

[James Bottomley]

On Wed, 2013-08-28 at 18:07 +0800, Vaughan Cao wrote:
> @@ -331,17 +330,20 @@ sg_open(struct inode *inode, struct file *filp)
>         }
>         if ((sfp = sg_add_sfp(sdp, dev)))
>                 filp->private_data = sfp;
> -       else {
> +               /* retval is already provably zero at this point
> because of the
> +                * check after retval =
> scsi_autopm_get_device(sdp->device))
> +                */
> +       else
> +               retval = -ENOMEM;
> +
> +       if (retval) {
> +sem_out:

There's still no need for the double if.  You know the value of retval
in each of the legs of the first if; its only non zero for the else leg,
so you can combine them thus:

        if ((sfp = sg_add_sfp(sdp, dev)))
                filp->private_data = sfp;
               /* retval is already provably zero at this point because of the
                * check after retval scsi_autopm_get_device(sdp->device))
                */
       else {
               retval = -ENOMEM;
 sem_out:
...

James

[PATCH v7 0/4][SCSI] sg: fix race condition in sg_open

[Vaughan Cao]

There is a race when open sg with O_EXCL flag. Also a race may happen between
sg_open and sg_remove.

Changes from v6:
 * [1/4] remove double if.
 * [3/4] fix via IS_ERR
Changes from v5:
 Patches based on v3.11-rc7 and passed sg_tst_excl test.
 * [1/4] * remove unused variables - res,sdp.
	 * fix insane code dealing with sg_add_sfp.
 * [2/4] resolve conflict with v3.11-rc7. 
 * [3/4] remove sem_out label.
 * [4/4] add sdp definition.

Changes from v4:
 * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
 * [4/4] fix conflict for cherry-pick from v3.

Changes from v3:
 * release o_sem in sg_release(), not in sg_remove_sfp().
 * not set exclude with sfd_lock held.

Vaughan Cao (4):
  sg: use rwsem to solve race during exclusive open
  sg: no need sg_open_exclusive_lock
  sg: checking sdp->detached isn't protected when open
  sg: push file descriptor list locking down to per-device locking

 drivers/scsi/sg.c | 176 +++++++++++++++++++++++++-----------------------------
 1 file changed, 81 insertions(+), 95 deletions(-)

-- 
1.8.3.1

[PATCH v7 1/4] sg: use rwsem to solve race during exclusive open

[Vaughan Cao]

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.

Changes from v6:
 * remove double if.
Changes from v5:
 * remove unused variables
 * fix insane code dealing with sg_add_sfp.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 79 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 38 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..4efa9b5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -170,11 +170,11 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 
 typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
-	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
 	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
+	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
@@ -265,7 +265,6 @@ sg_open(struct inode *inode, struct file *filp)
 	struct request_queue *q;
 	Sg_device *sdp;
 	Sg_fd *sfp;
-	int res;
 	int retval;
 
 	nonseekable_open(inode, filp);
@@ -294,35 +293,35 @@ sg_open(struct inode *inode, struct file *filp)
 		goto error_out;
 	}
 
-	if (flags & O_EXCL) {
-		if (O_RDONLY == (flags & O_ACCMODE)) {
-			retval = -EPERM; /* Can't lock it with read only access */
-			goto error_out;
-		}
-		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
-		}
-	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
-		if (flags & O_NONBLOCK) {
-			retval = -EBUSY;
-			goto error_out;
-		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
-		if (res) {
-			retval = res;	/* -ERESTARTSYS because signal hit process */
-			goto error_out;
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Can't lock it with read only access */
+		goto error_out;
+	}
+	if (flags & O_NONBLOCK) {
+		if (flags & O_EXCL) {
+			if (!down_write_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
+		} else {
+			if (!down_read_trylock(&sdp->o_sem)) {
+				retval = -EBUSY;
+				goto error_out;
+			}
 		}
+	} else {
+		if (flags & O_EXCL)
+			down_write(&sdp->o_sem);
+		else
+			down_read(&sdp->o_sem);
 	}
+	/* Since write lock is held, no need to check sfd_list */
+	if (flags & O_EXCL)
+		set_exclude(sdp, 1);
+
 	if (sdp->detached) {
 		retval = -ENODEV;
-		goto error_out;
+		goto sem_out;
 	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
@@ -331,17 +330,18 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	if ((sfp = sg_add_sfp(sdp, dev)))
 		filp->private_data = sfp;
+		/* retval is already provably zero at this point because of the
+		 * check after retval = scsi_autopm_get_device(sdp->device))
+		 */
 	else {
+		retval = -ENOMEM;
+sem_out:
 		if (flags & O_EXCL) {
 			set_exclude(sdp, 0);	/* undo if error */
-			wake_up_interruptible(&sdp->o_excl_wait);
-		}
-		retval = -ENOMEM;
-		goto error_out;
-	}
-	retval = 0;
+			up_write(&sdp->o_sem);
+		} else
+			up_read(&sdp->o_sem);
 error_out:
-	if (retval) {
 		scsi_autopm_put_device(sdp->device);
 sdp_put:
 		scsi_device_put(sdp->device);
@@ -358,13 +358,18 @@ sg_release(struct inode *inode, struct file *filp)
 {
 	Sg_device *sdp;
 	Sg_fd *sfp;
+	int excl;
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
+	excl = get_exclude(sdp);
 	set_exclude(sdp, 0);
-	wake_up_interruptible(&sdp->o_excl_wait);
+	if (excl)
+		up_write(&sdp->o_sem);
+	else
+		up_read(&sdp->o_sem);
 
 	scsi_autopm_put_device(sdp->device);
 	kref_put(&sfp->f_ref, sg_remove_sfp);
@@ -1416,7 +1421,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	sdp->disk = disk;
 	sdp->device = scsidp;
 	INIT_LIST_HEAD(&sdp->sfds);
-	init_waitqueue_head(&sdp->o_excl_wait);
+	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
 	sdp->index = k;
 	kref_init(&sdp->d_ref);
@@ -2127,13 +2132,11 @@ static void sg_remove_sfp_usercontext(struct work_struct *work)
 static void sg_remove_sfp(struct kref *kref)
 {
 	struct sg_fd *sfp = container_of(kref, struct sg_fd, f_ref);
-	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
 	write_lock_irqsave(&sg_index_lock, iflags);
 	list_del(&sfp->sfd_siblings);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
-	wake_up_interruptible(&sdp->o_excl_wait);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
-- 
1.8.3.1

[PATCH v7 2/4] sg: no need sg_open_exclusive_lock

[Vaughan Cao]

Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
@exclude is used to record which type of rwsem we are holding.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4efa9b5..d4af132 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -105,8 +105,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
 							   file descriptor list for device */
@@ -176,7 +174,6 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
@@ -225,27 +222,6 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
 }
 
-static int get_exclude(Sg_device *sdp)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
-}
-
 static int sfds_list_empty(Sg_device *sdp)
 {
 	unsigned long flags;
@@ -317,7 +293,7 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	/* Since write lock is held, no need to check sfd_list */
 	if (flags & O_EXCL)
-		set_exclude(sdp, 1);
+		sdp->exclude = 1;	/* used by release lock */
 
 	if (sdp->detached) {
 		retval = -ENODEV;
@@ -337,7 +313,7 @@ sg_open(struct inode *inode, struct file *filp)
 		retval = -ENOMEM;
 sem_out:
 		if (flags & O_EXCL) {
-			set_exclude(sdp, 0);	/* undo if error */
+			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
 		} else
 			up_read(&sdp->o_sem);
@@ -364,8 +340,8 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	excl = get_exclude(sdp);
-	set_exclude(sdp, 0);
+	excl = sdp->exclude;
+	sdp->exclude = 0;
 	if (excl)
 		up_write(&sdp->o_sem);
 	else
@@ -2622,7 +2598,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
+			   sdp->sg_tablesize, sdp->exclude);
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
-- 
1.8.3.1

[PATCH v7 3/4] sg: checking sdp->detached isn't protected when open

[Vaughan Cao]

@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().

Changes from v6:
 * fix via IS_ERR
Changes from v5:
 * remove sem_out label.
Changes from v4:
 * use ERR_PTR series instead of adding another parameter in sg_add_sfp

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d4af132..64df1ab 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -295,23 +295,20 @@ sg_open(struct inode *inode, struct file *filp)
 	if (flags & O_EXCL)
 		sdp->exclude = 1;	/* used by release lock */
 
-	if (sdp->detached) {
-		retval = -ENODEV;
-		goto sem_out;
-	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);
 	}
-	if ((sfp = sg_add_sfp(sdp, dev)))
+	sfp = sg_add_sfp(sdp, dev);
+	if (!IS_ERR(sfp))
 		filp->private_data = sfp;
 		/* retval is already provably zero at this point because of the
 		 * check after retval = scsi_autopm_get_device(sdp->device))
 		 */
 	else {
-		retval = -ENOMEM;
-sem_out:
+		retval = PTR_ERR(sfp);
+
 		if (flags & O_EXCL) {
 			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
@@ -2045,7 +2042,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
 	if (!sfp)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	init_waitqueue_head(&sfp->read_wait);
 	rwlock_init(&sfp->rq_list_lock);
@@ -2060,6 +2057,10 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
 	write_lock_irqsave(&sg_index_lock, iflags);
+	if (sdp->detached) {
+		write_unlock_irqrestore(&sg_index_lock, iflags);
+		return ERR_PTR(-ENODEV);
+	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
-- 
1.8.3.1

[PATCH v7 4/4] sg: push file descriptor list locking down to per-device locking

[Vaughan Cao]

Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.

Changes from v4:
 * Since I use ERR_PTR and friends in sg_add_sfp, this patch should also be
updated to resolve conflict in cherrry-pick.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 62 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 64df1ab..5cbc4bb 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -106,8 +106,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -144,8 +143,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
-	struct list_head sfd_siblings;
+	struct list_head sfd_siblings; /* protected by sfd_lock of device */
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
 	rwlock_t rq_list_lock;	/* protect access to list in req_arr */
@@ -170,7 +168,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect file descriptor list for device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
@@ -227,9 +225,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -1393,6 +1391,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1527,11 +1526,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
 	sdp->detached = 1;
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2056,13 +2057,13 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
-		write_unlock_irqrestore(&sg_index_lock, iflags);
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 		return ERR_PTR(-ENODEV);
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2109,11 +2110,12 @@ static void sg_remove_sfp_usercontext(struct work_struct *work)
 static void sg_remove_sfp(struct kref *kref)
 {
 	struct sg_fd *sfp = container_of(kref, struct sg_fd, f_ref);
+	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
@@ -2500,7 +2502,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2585,22 +2587,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+				    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+				     scsidp->host->host_no,
+				     scsidp->channel, scsidp->id,
+				     scsidp->lun,
+				     scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+				   sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.8.3.1

[PATCH v6 2/4] sg: no need sg_open_exclusive_lock

[Vaughan Cao]

Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
@exclude is used to record which type of rwsem we are holding.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 7a54c92..dcbd95f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -105,8 +105,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
 							   file descriptor list for device */
@@ -176,7 +174,6 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
@@ -225,27 +222,6 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
 }
 
-static int get_exclude(Sg_device *sdp)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
-}
-
 static int sfds_list_empty(Sg_device *sdp)
 {
 	unsigned long flags;
@@ -317,7 +293,7 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	/* Since write lock is held, no need to check sfd_list */
 	if (flags & O_EXCL)
-		set_exclude(sdp, 1);
+		sdp->exclude = 1;	/* used by release lock */
 
 	if (sdp->detached) {
 		retval = -ENODEV;
@@ -339,7 +315,7 @@ sg_open(struct inode *inode, struct file *filp)
 	if (retval) {
 sem_out:
 		if (flags & O_EXCL) {
-			set_exclude(sdp, 0);	/* undo if error */
+			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
 		} else
 			up_read(&sdp->o_sem);
@@ -366,8 +342,8 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	excl = get_exclude(sdp);
-	set_exclude(sdp, 0);
+	excl = sdp->exclude;
+	sdp->exclude = 0;
 	if (excl)
 		up_write(&sdp->o_sem);
 	else
@@ -2624,7 +2600,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
+			   sdp->sg_tablesize, sdp->exclude);
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
-- 
1.8.3.1

[PATCH v6 3/4] sg: checking sdp->detached isn't protected when open

[Vaughan Cao]

@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().

Changes from v5:
 * remove sem_out label.
Changes from v4:
 * use ERR_PTR series instead of adding another parameter in sg_add_sfp

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index dcbd95f..6bffe52 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -295,10 +295,6 @@ sg_open(struct inode *inode, struct file *filp)
 	if (flags & O_EXCL)
 		sdp->exclude = 1;	/* used by release lock */
 
-	if (sdp->detached) {
-		retval = -ENODEV;
-		goto sem_out;
-	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
@@ -309,16 +305,16 @@ sg_open(struct inode *inode, struct file *filp)
 		/* retval is already provably zero at this point because of the
 		 * check after retval = scsi_autopm_get_device(sdp->device))
 		 */
-	else
-		retval = -ENOMEM;
-
-	if (retval) {
-sem_out:
+	else {
+		retval = PTR_ERR(sfp);
 		if (flags & O_EXCL) {
 			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
 		} else
 			up_read(&sdp->o_sem);
+	}
+
+	if (retval) {
 error_out:
 		scsi_autopm_put_device(sdp->device);
 sdp_put:
@@ -2047,7 +2043,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
 	if (!sfp)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	init_waitqueue_head(&sfp->read_wait);
 	rwlock_init(&sfp->rq_list_lock);
@@ -2062,6 +2058,10 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
 	write_lock_irqsave(&sg_index_lock, iflags);
+	if (sdp->detached) {
+		write_unlock_irqrestore(&sg_index_lock, iflags);
+		return ERR_PTR(-ENODEV);
+	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
-- 
1.8.3.1

[PATCH v6 4/4] sg: push file descriptor list locking down to per-device locking

[Vaughan Cao]

Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.

Changes from v4:
 * Since I use ERR_PTR and friends in sg_add_sfp, this patch should also be
updated to resolve conflict in cherrry-pick.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 62 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 6bffe52..10d6943 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -106,8 +106,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -144,8 +143,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
-	struct list_head sfd_siblings;
+	struct list_head sfd_siblings; /* protected by sfd_lock of device */
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
 	rwlock_t rq_list_lock;	/* protect access to list in req_arr */
@@ -170,7 +168,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect file descriptor list for device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
@@ -227,9 +225,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -1394,6 +1392,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1528,11 +1527,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
 	sdp->detached = 1;
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2057,13 +2058,13 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
-		write_unlock_irqrestore(&sg_index_lock, iflags);
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 		return ERR_PTR(-ENODEV);
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2110,11 +2111,12 @@ static void sg_remove_sfp_usercontext(struct work_struct *work)
 static void sg_remove_sfp(struct kref *kref)
 {
 	struct sg_fd *sfp = container_of(kref, struct sg_fd, f_ref);
+	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
@@ -2501,7 +2503,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2586,22 +2588,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+				    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+				     scsidp->host->host_no,
+				     scsidp->channel, scsidp->id,
+				     scsidp->lun,
+				     scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+				   sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.8.3.1

[PATCH v5 2/4] [SCSI] sg: no need sg_open_exclusive_lock

[Vaughan Cao]

Open exclusive check is protected by o_sem, no need sg_open_exclusive_lock.
@exclude is used to record which type of rwsem we are holding.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index edc395a..671b760 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -103,8 +103,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_SPINLOCK(sg_open_exclusive_lock);
-
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
 							   file descriptor list for device */
@@ -174,7 +172,6 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	/* exclude protected by sg_open_exclusive_lock */
 	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
@@ -224,27 +221,6 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(q, cmd, filp->f_mode & FMODE_WRITE);
 }
 
-static int get_exclude(Sg_device *sdp)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	ret = sdp->exclude;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return ret;
-}
-
-static int set_exclude(Sg_device *sdp, char val)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
-	sdp->exclude = val;
-	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
-	return val;
-}
-
 static int sfds_list_empty(Sg_device *sdp)
 {
 	unsigned long flags;
@@ -317,7 +293,7 @@ sg_open(struct inode *inode, struct file *filp)
 	}
 	/* Since write lock is held, no need to check sfd_list */
 	if (flags & O_EXCL)
-		set_exclude(sdp, 1);
+		sdp->exclude = 1;	/* used by release lock */
 
 	if (sdp->detached) {
 		retval = -ENODEV;
@@ -339,7 +315,7 @@ sg_open(struct inode *inode, struct file *filp)
 	if (retval) {
 sem_out:
 		if (flags & O_EXCL) {
-			set_exclude(sdp, 0);	/* undo if error */
+			sdp->exclude = 0;	/* undo if error */
 			up_write(&sdp->o_sem);
 		} else
 			up_read(&sdp->o_sem);
@@ -366,8 +342,8 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	excl = get_exclude(sdp);
-	set_exclude(sdp, 0);
+	excl = sdp->exclude;
+	sdp->exclude = 0;
 	if (excl)
 		up_write(&sdp->o_sem);
 	else
@@ -2637,7 +2613,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, get_exclude(sdp));
+			   sdp->sg_tablesize, sdp->exclude);
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
-- 
1.7.11.7

[PATCH v5 3/4] [SCSI] sg: checking sdp->detached isn't protected when open

[Vaughan Cao]

@detached is set under the protection of sg_index_lock. Without getting the
lock, new sfp will be added during sg removal and there is no chance for it
to be picked out. So check with sg_index_lock held in sg_add_sfp().

Changes from v4:
 * use ERR_PTR series instead of adding another parameter in sg_add_sfp

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 671b760..f0e4785 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -295,21 +295,17 @@ sg_open(struct inode *inode, struct file *filp)
 	if (flags & O_EXCL)
 		sdp->exclude = 1;	/* used by release lock */
 
-	if (sdp->detached) {
-		retval = -ENODEV;
-		goto sem_out;
-	}
 	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);
 	}
-	if ((sfp = sg_add_sfp(sdp, dev)))
-		filp->private_data = sfp;
-	else {
-		retval = -ENOMEM;
+	sfp = sg_add_sfp(sdp, dev);
+	if (IS_ERR(sfp)) {
+		retval = PTR_ERR(sfp);
 		goto sem_out;
 	}
+	filp->private_data = sfp;
 	retval = 0;
 
 	if (retval) {
@@ -2055,7 +2051,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
 	if (!sfp)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	init_waitqueue_head(&sfp->read_wait);
 	rwlock_init(&sfp->rq_list_lock);
@@ -2070,6 +2066,10 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
 	write_lock_irqsave(&sg_index_lock, iflags);
+	if (sdp->detached) {
+		write_unlock_irqrestore(&sg_index_lock, iflags);
+		return ERR_PTR(-ENODEV);
+	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
-- 
1.7.11.7

[PATCH v5 4/4] [SCSI] sg: push file descriptor list locking down to per-device locking

[Vaughan Cao]

Push file descriptor list locking down to per-device locking. Let sg_index_lock
only protect device lookup.
sdp->detached is also set and checked with this lock held.

Changes from v4:
 * Since I use ERR_PTR and friends in sg_add_sfp, this patch should also be
updated to resolve conflict in cherrry-pick.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sg.c | 61 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f0e4785..3431d12 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -104,8 +104,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_IDR(sg_index_idr);
-static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock
-							   file descriptor list for device */
+static DEFINE_RWLOCK(sg_index_lock);
 
 static struct class_interface sg_interface = {
 	.add_dev	= sg_add,
@@ -142,8 +141,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
-	/* sfd_siblings is protected by sg_index_lock */
-	struct list_head sfd_siblings;
+	struct list_head sfd_siblings; /* protected by sfd_lock of device */
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */
 	rwlock_t rq_list_lock;	/* protect access to list in req_arr */
@@ -168,7 +166,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	struct scsi_device *device;
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
-	/* sfds is protected by sg_index_lock */
+	spinlock_t sfd_lock;	/* protect file descriptor list for device */
 	struct list_head sfds;
 	struct rw_semaphore o_sem;	/* exclude open should hold this rwsem */
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
@@ -226,9 +224,9 @@ static int sfds_list_empty(Sg_device *sdp)
 	unsigned long flags;
 	int ret;
 
-	read_lock_irqsave(&sg_index_lock, flags);
+	spin_lock_irqsave(&sdp->sfd_lock, flags);
 	ret = list_empty(&sdp->sfds);
-	read_unlock_irqrestore(&sg_index_lock, flags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
 	return ret;
 }
 
@@ -1394,6 +1392,7 @@ static Sg_device *sg_alloc(struct gendisk *disk, struct scsi_device *scsidp)
 	disk->first_minor = k;
 	sdp->disk = disk;
 	sdp->device = scsidp;
+	spin_lock_init(&sdp->sfd_lock);
 	INIT_LIST_HEAD(&sdp->sfds);
 	init_rwsem(&sdp->o_sem);
 	sdp->sg_tablesize = queue_max_segments(q);
@@ -1536,11 +1535,13 @@ static void sg_remove(struct device *cl_dev, struct class_interface *cl_intf)
 
 	/* Need a write lock to set sdp->detached. */
 	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock(&sdp->sfd_lock);
 	sdp->detached = 1;
 	list_for_each_entry(sfp, &sdp->sfds, sfd_siblings) {
 		wake_up_interruptible(&sfp->read_wait);
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_HUP);
 	}
+	spin_unlock(&sdp->sfd_lock);
 	write_unlock_irqrestore(&sg_index_lock, iflags);
 
 	sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
@@ -2065,13 +2066,13 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
-		write_unlock_irqrestore(&sg_index_lock, iflags);
+		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 		return ERR_PTR(-ENODEV);
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp: sfp=0x%p\n", sfp));
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
@@ -2121,9 +2122,9 @@ static void sg_remove_sfp(struct kref *kref)
 	struct sg_device *sdp = sfp->parentdp;
 	unsigned long iflags;
 
-	write_lock_irqsave(&sg_index_lock, iflags);
+	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	list_del(&sfp->sfd_siblings);
-	write_unlock_irqrestore(&sg_index_lock, iflags);
+	spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
 
 	INIT_WORK(&sfp->ew.work, sg_remove_sfp_usercontext);
 	schedule_work(&sfp->ew.work);
@@ -2514,7 +2515,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 	return 0;
 }
 
-/* must be called while holding sg_index_lock */
+/* must be called while holding sg_index_lock and sfd_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
 	int k, m, new_interface, blen, usg;
@@ -2599,22 +2600,26 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 
 	read_lock_irqsave(&sg_index_lock, iflags);
 	sdp = it ? sg_lookup_dev(it->index) : NULL;
-	if (sdp && !list_empty(&sdp->sfds)) {
-		struct scsi_device *scsidp = sdp->device;
+	if (sdp) {
+		spin_lock(&sdp->sfd_lock);
+		if (!list_empty(&sdp->sfds)) {
+			struct scsi_device *scsidp = sdp->device;
 
-		seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
-		if (sdp->detached)
-			seq_printf(s, "detached pending close ");
-		else
-			seq_printf
-			    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
-			     scsidp->host->host_no,
-			     scsidp->channel, scsidp->id,
-			     scsidp->lun,
-			     scsidp->host->hostt->emulated);
-		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
-		sg_proc_debug_helper(s, sdp);
+			seq_printf(s, " >>> device=%s ", sdp->disk->disk_name);
+			if (sdp->detached)
+				seq_printf(s, "detached pending close ");
+			else
+				seq_printf
+				    (s, "scsi%d chan=%d id=%d lun=%d   em=%d",
+				     scsidp->host->host_no,
+				     scsidp->channel, scsidp->id,
+				     scsidp->lun,
+				     scsidp->host->hostt->emulated);
+			seq_printf(s, " sg_tablesize=%d excl=%d\n",
+				   sdp->sg_tablesize, sdp->exclude);
+			sg_proc_debug_helper(s, sdp);
+		}
+		spin_unlock(&sdp->sfd_lock);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);
 	return 0;
-- 
1.7.11.7

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Jörn Engel]

On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
> 
> There is a race when open sg with O_EXCL flag. Also a race may happen between
> sg_open and sg_remove.
> 
> Changes from v4:
>  * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
>  * [4/4] fix conflict for cherry-pick from v3.
> 
> Changes from v3:
>  * release o_sem in sg_release(), not in sg_remove_sfp().
>  * not set exclude with sfd_lock held.
> 
> Vaughan Cao (4):
>   [SCSI] sg: use rwsem to solve race during exclusive open
>   [SCSI] sg: no need sg_open_exclusive_lock
>   [SCSI] sg: checking sdp->detached isn't protected when open
>   [SCSI] sg: push file descriptor list locking down to per-device
>     locking
> 
>  drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 83 insertions(+), 95 deletions(-)

Patchset looks good to me, although I didn't test it on hardware yet.
Signed-off-by: Joern Engel <joern@logfs.org>

James, care to pick this up?

Jörn

--
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 07/23/2013 01:03 AM, Jörn Engel wrote:
> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>> There is a race when open sg with O_EXCL flag. Also a race may happen between
>> sg_open and sg_remove.
>>
>> Changes from v4:
>>  * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
>>  * [4/4] fix conflict for cherry-pick from v3.
>>
>> Changes from v3:
>>  * release o_sem in sg_release(), not in sg_remove_sfp().
>>  * not set exclude with sfd_lock held.
>>
>> Vaughan Cao (4):
>>   [SCSI] sg: use rwsem to solve race during exclusive open
>>   [SCSI] sg: no need sg_open_exclusive_lock
>>   [SCSI] sg: checking sdp->detached isn't protected when open
>>   [SCSI] sg: push file descriptor list locking down to per-device
>>     locking
>>
>>  drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
>>  1 file changed, 83 insertions(+), 95 deletions(-)
> Patchset looks good to me, although I didn't test it on hardware yet.
> Signed-off-by: Joern Engel <joern@logfs.org>
>
> James, care to pick this up?
>
> Jörn
Hi James,

sg driver has two races happen in
 a) exclusive open and non-exclusive open
 b) sg removal and sg open
I explained the scenario detail in the separate patches. I did test
those patches and
Jörn has reviewed them.  I got no response from Doug Gilbert for a long
time.
Would you care to pick these up?

Thanks,
Vaughan

>
> --
> Good warriors cause others to come to them and do not go to others.
> -- Sun Tzu

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-07-25 11:32 AM, vaughan wrote:
> On 07/23/2013 01:03 AM, Jörn Engel wrote:
>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>> There is a race when open sg with O_EXCL flag. Also a race may happen between
>>> sg_open and sg_remove.
>>>
>>> Changes from v4:
>>>   * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
>>>   * [4/4] fix conflict for cherry-pick from v3.
>>>
>>> Changes from v3:
>>>   * release o_sem in sg_release(), not in sg_remove_sfp().
>>>   * not set exclude with sfd_lock held.
>>>
>>> Vaughan Cao (4):
>>>    [SCSI] sg: use rwsem to solve race during exclusive open
>>>    [SCSI] sg: no need sg_open_exclusive_lock
>>>    [SCSI] sg: checking sdp->detached isn't protected when open
>>>    [SCSI] sg: push file descriptor list locking down to per-device
>>>      locking
>>>
>>>   drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
>>>   1 file changed, 83 insertions(+), 95 deletions(-)
>> Patchset looks good to me, although I didn't test it on hardware yet.
>> Signed-off-by: Joern Engel <joern@logfs.org>
>>
>> James, care to pick this up?
>>
>> Jörn
> Hi James,
>
> sg driver has two races happen in
>   a) exclusive open and non-exclusive open
>   b) sg removal and sg open
> I explained the scenario detail in the separate patches. I did test
> those patches and
> Jörn has reviewed them.  I got no response from Doug Gilbert for a long
> time.
> Would you care to pick these up?

Hi,
Your patches applied with a little noise to lk 3.10.2 and
gave this warning from the build.

   CC [M]  drivers/scsi/sg.o
drivers/scsi/sg.c: In function ‘sg_open’:
drivers/scsi/sg.c:242:6: warning: unused variable ‘res’ [-Wunused-variable]

I'll keep testing ...

Doug Gilbert

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 07/26/2013 04:33 AM, Douglas Gilbert wrote:
> On 13-07-25 11:32 AM, vaughan wrote:
>> On 07/23/2013 01:03 AM, Jörn Engel wrote:
>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>> happen between
>>>> sg_open and sg_remove.
>>>>
>>>> Changes from v4:
>>>>   * [3/4] use ERR_PTR series instead of adding another parameter in
>>>> sg_add_sfp
>>>>   * [4/4] fix conflict for cherry-pick from v3.
>>>>
>>>> Changes from v3:
>>>>   * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>   * not set exclude with sfd_lock held.
>>>>
>>>> Vaughan Cao (4):
>>>>    [SCSI] sg: use rwsem to solve race during exclusive open
>>>>    [SCSI] sg: no need sg_open_exclusive_lock
>>>>    [SCSI] sg: checking sdp->detached isn't protected when open
>>>>    [SCSI] sg: push file descriptor list locking down to per-device
>>>>      locking
>>>>
>>>>   drivers/scsi/sg.c | 178
>>>> +++++++++++++++++++++++++-----------------------------
>>>>   1 file changed, 83 insertions(+), 95 deletions(-)
>>> Patchset looks good to me, although I didn't test it on hardware yet.
>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>
>>> James, care to pick this up?
>>>
>>> Jörn
>> Hi James,
>>
>> sg driver has two races happen in
>>   a) exclusive open and non-exclusive open
>>   b) sg removal and sg open
>> I explained the scenario detail in the separate patches. I did test
>> those patches and
>> Jörn has reviewed them.  I got no response from Doug Gilbert for a long
>> time.
>> Would you care to pick these up?
>
> Hi,
> Your patches applied with a little noise to lk 3.10.2 and
> gave this warning from the build.
>
>   CC [M]  drivers/scsi/sg.o
> drivers/scsi/sg.c: In function ‘sg_open’:
> drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
> [-Wunused-variable]
>
> I'll keep testing ...
Hi Doug,

Can I ask how about the test result?

Thanks,
Vaughan
>
> Doug Gilbert
>

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-07-22 01:03 PM, Jörn Engel wrote:
> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>
>> There is a race when open sg with O_EXCL flag. Also a race may happen between
>> sg_open and sg_remove.
>>
>> Changes from v4:
>>   * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
>>   * [4/4] fix conflict for cherry-pick from v3.
>>
>> Changes from v3:
>>   * release o_sem in sg_release(), not in sg_remove_sfp().
>>   * not set exclude with sfd_lock held.
>>
>> Vaughan Cao (4):
>>    [SCSI] sg: use rwsem to solve race during exclusive open
>>    [SCSI] sg: no need sg_open_exclusive_lock
>>    [SCSI] sg: checking sdp->detached isn't protected when open
>>    [SCSI] sg: push file descriptor list locking down to per-device
>>      locking
>>
>>   drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
>>   1 file changed, 83 insertions(+), 95 deletions(-)
>
> Patchset looks good to me, although I didn't test it on hardware yet.
> Signed-off-by: Joern Engel <joern@logfs.org>
>
> James, care to pick this up?

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

Tested O_EXCL with multiple processes and threads; passed.
sg driver prior to this patch had "leaky" O_EXCL logic
according to the same test. Block device passed.

James, could you clean this up:
   drivers/scsi/sg.c:242:6: warning: unused variable ‘res’ [-Wunused-variable]

Doug Gilbert

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-08-01 01:01 AM, Douglas Gilbert wrote:
> On 13-07-22 01:03 PM, Jörn Engel wrote:
>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>
>>> There is a race when open sg with O_EXCL flag. Also a race may happen between
>>> sg_open and sg_remove.
>>>
>>> Changes from v4:
>>>   * [3/4] use ERR_PTR series instead of adding another parameter in sg_add_sfp
>>>   * [4/4] fix conflict for cherry-pick from v3.
>>>
>>> Changes from v3:
>>>   * release o_sem in sg_release(), not in sg_remove_sfp().
>>>   * not set exclude with sfd_lock held.
>>>
>>> Vaughan Cao (4):
>>>    [SCSI] sg: use rwsem to solve race during exclusive open
>>>    [SCSI] sg: no need sg_open_exclusive_lock
>>>    [SCSI] sg: checking sdp->detached isn't protected when open
>>>    [SCSI] sg: push file descriptor list locking down to per-device
>>>      locking
>>>
>>>   drivers/scsi/sg.c | 178 +++++++++++++++++++++++++-----------------------------
>>>   1 file changed, 83 insertions(+), 95 deletions(-)
>>
>> Patchset looks good to me, although I didn't test it on hardware yet.
>> Signed-off-by: Joern Engel <joern@logfs.org>
>>
>> James, care to pick this up?
>
> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>
> Tested O_EXCL with multiple processes and threads; passed.
> sg driver prior to this patch had "leaky" O_EXCL logic
> according to the same test. Block device passed.
>
> James, could you clean this up:
>    drivers/scsi/sg.c:242:6: warning: unused variable ‘res’ [-Wunused-variable]

Further testing suggests this patch on the sg driver is
broken, so I'll rescind my ack.

The case it is broken for is when a device is opened
without O_EXCL. Now if, while it is open, a second
thread/process tries to open the same device O_EXCL
then IMO the second open should fail with EBUSY.

My testing shows that O_EXCL opens properly deflect
other O_EXCL opens.

BTW the standard block driver (e.g. /dev/sdc) is broken
in exactly the same way, according to my tests.

Doug Gilbert

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>
>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>> happen between
>>>> sg_open and sg_remove.
>>>>
>>>> Changes from v4:
>>>>   * [3/4] use ERR_PTR series instead of adding another parameter in
>>>> sg_add_sfp
>>>>   * [4/4] fix conflict for cherry-pick from v3.
>>>>
>>>> Changes from v3:
>>>>   * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>   * not set exclude with sfd_lock held.
>>>>
>>>> Vaughan Cao (4):
>>>>    [SCSI] sg: use rwsem to solve race during exclusive open
>>>>    [SCSI] sg: no need sg_open_exclusive_lock
>>>>    [SCSI] sg: checking sdp->detached isn't protected when open
>>>>    [SCSI] sg: push file descriptor list locking down to per-device
>>>>      locking
>>>>
>>>>   drivers/scsi/sg.c | 178
>>>> +++++++++++++++++++++++++-----------------------------
>>>>   1 file changed, 83 insertions(+), 95 deletions(-)
>>>
>>> Patchset looks good to me, although I didn't test it on hardware yet.
>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>
>>> James, care to pick this up?
>>
>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>
>> Tested O_EXCL with multiple processes and threads; passed.
>> sg driver prior to this patch had "leaky" O_EXCL logic
>> according to the same test. Block device passed.
>>
>> James, could you clean this up:
>>    drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>> [-Wunused-variable]
>
> Further testing suggests this patch on the sg driver is
> broken, so I'll rescind my ack.
>
> The case it is broken for is when a device is opened
> without O_EXCL. Now if, while it is open, a second
> thread/process tries to open the same device O_EXCL
> then IMO the second open should fail with EBUSY.
>
> My testing shows that O_EXCL opens properly deflect
> other O_EXCL opens.
Hi  Doug,

My test don't have this issue. The routine is something as below:

I start three opens without O_EXCL, wait 30s each, and open with
O_EXCL|O_NONBLOCK, it failed with EBUSY.
And I also call myopen with/without O_EXCL many times in background at
the same time, and the test is passed. I don't know why it failed in
your test.

Usage: myopen [-e][-n][-d delay] -f file
      -e: exclude
      -n: nonblock
      -d: delay N seconds and then close.

[root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
[1] 3417
[root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
[2] 3418
[root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
[3] 3419
[root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
max_active_device=6(origin 1)
 def_reserved_size=32768
 >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
   FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active
   FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active
   FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active

[root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
[4] 3422
[3422:3351] /dev/sg5:exclude: Device or resource busy

[4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30

[root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
max_active_device=6(origin 1)
 def_reserved_size=32768
 >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
   FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active
   FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active
   FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
   cmd_q=0 f_packid=0 k_orphan=0 closed=0
     No requests active
[root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
[1]   Done                    ./myopen -f /dev/sg5 -d 30
[2]-  Done                    ./myopen -f /dev/sg5 -d 30
[3]+  Done                    ./myopen -f /dev/sg5 -d 30


>
> BTW the standard block driver (e.g. /dev/sdc) is broken
> in exactly the same way, according to my tests.
>
> Doug Gilbert
>
>

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-08-04 10:19 PM, vaughan wrote:
> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>
>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>> happen between
>>>>> sg_open and sg_remove.
>>>>>
>>>>> Changes from v4:
>>>>>    * [3/4] use ERR_PTR series instead of adding another parameter in
>>>>> sg_add_sfp
>>>>>    * [4/4] fix conflict for cherry-pick from v3.
>>>>>
>>>>> Changes from v3:
>>>>>    * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>    * not set exclude with sfd_lock held.
>>>>>
>>>>> Vaughan Cao (4):
>>>>>     [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>     [SCSI] sg: no need sg_open_exclusive_lock
>>>>>     [SCSI] sg: checking sdp->detached isn't protected when open
>>>>>     [SCSI] sg: push file descriptor list locking down to per-device
>>>>>       locking
>>>>>
>>>>>    drivers/scsi/sg.c | 178
>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>    1 file changed, 83 insertions(+), 95 deletions(-)
>>>>
>>>> Patchset looks good to me, although I didn't test it on hardware yet.
>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>
>>>> James, care to pick this up?
>>>
>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>
>>> Tested O_EXCL with multiple processes and threads; passed.
>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>> according to the same test. Block device passed.
>>>
>>> James, could you clean this up:
>>>     drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>> [-Wunused-variable]
>>
>> Further testing suggests this patch on the sg driver is
>> broken, so I'll rescind my ack.
>>
>> The case it is broken for is when a device is opened
>> without O_EXCL. Now if, while it is open, a second
>> thread/process tries to open the same device O_EXCL
>> then IMO the second open should fail with EBUSY.
>>
>> My testing shows that O_EXCL opens properly deflect
>> other O_EXCL opens.
> Hi  Doug,
>
> My test don't have this issue. The routine is something as below:
>
> I start three opens without O_EXCL, wait 30s each, and open with
> O_EXCL|O_NONBLOCK, it failed with EBUSY.
> And I also call myopen with/without O_EXCL many times in background at
> the same time, and the test is passed. I don't know why it failed in
> your test.
>
> Usage: myopen [-e][-n][-d delay] -f file
>        -e: exclude
>        -n: nonblock
>        -d: delay N seconds and then close.
>
> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
> [1] 3417
> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
> [2] 3418
> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
> [3] 3419
> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
> max_active_device=6(origin 1)
>   def_reserved_size=32768
>   >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>     FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
>     FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
>     FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
>
> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
> [4] 3422
> [3422:3351] /dev/sg5:exclude: Device or resource busy
>
> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>
> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
> max_active_device=6(origin 1)
>   def_reserved_size=32768
>   >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>     FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
>     FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
>     FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>       No requests active
> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
> [1]   Done                    ./myopen -f /dev/sg5 -d 30
> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>

Hi,
After the initial failures about 36 hours ago, retesting
yesterday and today has not produced any unexpected
failures. And I have been trying hard on lk 3.10.4 and
lk 3.10.5 .

My test program is a bit more intense than yours and can
be found in the sg3_utils beta in the News section of this
page:
   http://sg.danny.cz/sg/

It is in the examples directory, two variants called
sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
compiler, IOW something that can compile c++11 . gcc 4.7.3
in Ubuntu 13.04 only just manages, fedora 19 should do
better with gcc 4.8.1 . The threading is implemented using
pthreads so it should be reliable.

Typically I run multiple instances (processes) and each has
multiple threads. One instance can run '-x' which will cause
its first thread not to use O_EXCL **. All my tests currently
use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
in the billions).

Doug Gilbert


** Using '-x' on two instances will cause an expected failure
    so can be used as a control.

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 08/06/2013 04:52 AM, Douglas Gilbert wrote:
> On 13-08-04 10:19 PM, vaughan wrote:
>> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>>
>>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>>> happen between
>>>>>> sg_open and sg_remove.
>>>>>>
>>>>>> Changes from v4:
>>>>>>    * [3/4] use ERR_PTR series instead of adding another parameter in
>>>>>> sg_add_sfp
>>>>>>    * [4/4] fix conflict for cherry-pick from v3.
>>>>>>
>>>>>> Changes from v3:
>>>>>>    * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>>    * not set exclude with sfd_lock held.
>>>>>>
>>>>>> Vaughan Cao (4):
>>>>>>     [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>>     [SCSI] sg: no need sg_open_exclusive_lock
>>>>>>     [SCSI] sg: checking sdp->detached isn't protected when open
>>>>>>     [SCSI] sg: push file descriptor list locking down to per-device
>>>>>>       locking
>>>>>>
>>>>>>    drivers/scsi/sg.c | 178
>>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>>    1 file changed, 83 insertions(+), 95 deletions(-)
>>>>>
>>>>> Patchset looks good to me, although I didn't test it on hardware yet.
>>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>>
>>>>> James, care to pick this up?
>>>>
>>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>>
>>>> Tested O_EXCL with multiple processes and threads; passed.
>>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>>> according to the same test. Block device passed.
>>>>
>>>> James, could you clean this up:
>>>>     drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>>> [-Wunused-variable]
>>>
>>> Further testing suggests this patch on the sg driver is
>>> broken, so I'll rescind my ack.
>>>
>>> The case it is broken for is when a device is opened
>>> without O_EXCL. Now if, while it is open, a second
>>> thread/process tries to open the same device O_EXCL
>>> then IMO the second open should fail with EBUSY.
>>>
>>> My testing shows that O_EXCL opens properly deflect
>>> other O_EXCL opens.
>> Hi  Doug,
>>
>> My test don't have this issue. The routine is something as below:
>>
>> I start three opens without O_EXCL, wait 30s each, and open with
>> O_EXCL|O_NONBLOCK, it failed with EBUSY.
>> And I also call myopen with/without O_EXCL many times in background at
>> the same time, and the test is passed. I don't know why it failed in
>> your test.
>>
>> Usage: myopen [-e][-n][-d delay] -f file
>>        -e: exclude
>>        -n: nonblock
>>        -d: delay N seconds and then close.
>>
>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>> [1] 3417
>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>> [2] 3418
>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>> [3] 3419
>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>> max_active_device=6(origin 1)
>>   def_reserved_size=32768
>>   >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>>     FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>>     FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>>     FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>>
>> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
>> [4] 3422
>> [3422:3351] /dev/sg5:exclude: Device or resource busy
>>
>> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>>
>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>> max_active_device=6(origin 1)
>>   def_reserved_size=32768
>>   >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>>     FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>>     FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>>     FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>     cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>       No requests active
>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>> [1]   Done                    ./myopen -f /dev/sg5 -d 30
>> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
>> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>>
>
> Hi,
> After the initial failures about 36 hours ago, retesting
> yesterday and today has not produced any unexpected
> failures. And I have been trying hard on lk 3.10.4 and
> lk 3.10.5 .
>
> My test program is a bit more intense than yours and can
> be found in the sg3_utils beta in the News section of this
> page:
>   http://sg.danny.cz/sg/
>
> It is in the examples directory, two variants called
> sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
> compiler, IOW something that can compile c++11 . gcc 4.7.3
> in Ubuntu 13.04 only just manages, fedora 19 should do
> better with gcc 4.8.1 . The threading is implemented using
> pthreads so it should be reliable.
>
> Typically I run multiple instances (processes) and each has
> multiple threads. One instance can run '-x' which will cause
> its first thread not to use O_EXCL **. All my tests currently
> use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
> in the billions).
>
> Doug Gilbert
>
>
> ** Using '-x' on two instances will cause an expected failure
>    so can be used as a control.
>
Hi Doug,

Can I regard this as you ACK it again?

Vaughan

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-08-12 10:46 PM, vaughan wrote:
> On 08/06/2013 04:52 AM, Douglas Gilbert wrote:
>> On 13-08-04 10:19 PM, vaughan wrote:
>>> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>>>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>>>
>>>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>>>> happen between
>>>>>>> sg_open and sg_remove.
>>>>>>>
>>>>>>> Changes from v4:
>>>>>>>     * [3/4] use ERR_PTR series instead of adding another parameter in
>>>>>>> sg_add_sfp
>>>>>>>     * [4/4] fix conflict for cherry-pick from v3.
>>>>>>>
>>>>>>> Changes from v3:
>>>>>>>     * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>>>     * not set exclude with sfd_lock held.
>>>>>>>
>>>>>>> Vaughan Cao (4):
>>>>>>>      [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>>>      [SCSI] sg: no need sg_open_exclusive_lock
>>>>>>>      [SCSI] sg: checking sdp->detached isn't protected when open
>>>>>>>      [SCSI] sg: push file descriptor list locking down to per-device
>>>>>>>        locking
>>>>>>>
>>>>>>>     drivers/scsi/sg.c | 178
>>>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>>>     1 file changed, 83 insertions(+), 95 deletions(-)
>>>>>>
>>>>>> Patchset looks good to me, although I didn't test it on hardware yet.
>>>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>>>
>>>>>> James, care to pick this up?
>>>>>
>>>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>>>
>>>>> Tested O_EXCL with multiple processes and threads; passed.
>>>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>>>> according to the same test. Block device passed.
>>>>>
>>>>> James, could you clean this up:
>>>>>      drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>>>> [-Wunused-variable]
>>>>
>>>> Further testing suggests this patch on the sg driver is
>>>> broken, so I'll rescind my ack.
>>>>
>>>> The case it is broken for is when a device is opened
>>>> without O_EXCL. Now if, while it is open, a second
>>>> thread/process tries to open the same device O_EXCL
>>>> then IMO the second open should fail with EBUSY.
>>>>
>>>> My testing shows that O_EXCL opens properly deflect
>>>> other O_EXCL opens.
>>> Hi  Doug,
>>>
>>> My test don't have this issue. The routine is something as below:
>>>
>>> I start three opens without O_EXCL, wait 30s each, and open with
>>> O_EXCL|O_NONBLOCK, it failed with EBUSY.
>>> And I also call myopen with/without O_EXCL many times in background at
>>> the same time, and the test is passed. I don't know why it failed in
>>> your test.
>>>
>>> Usage: myopen [-e][-n][-d delay] -f file
>>>         -e: exclude
>>>         -n: nonblock
>>>         -d: delay N seconds and then close.
>>>
>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>> [1] 3417
>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>> [2] 3418
>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>> [3] 3419
>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>> max_active_device=6(origin 1)
>>>    def_reserved_size=32768
>>>    >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>>>      FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>>      FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>>      FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>>
>>> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
>>> [4] 3422
>>> [3422:3351] /dev/sg5:exclude: Device or resource busy
>>>
>>> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>>>
>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>> max_active_device=6(origin 1)
>>>    def_reserved_size=32768
>>>    >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55 excl=0
>>>      FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>>      FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>>      FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>        No requests active
>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>> [1]   Done                    ./myopen -f /dev/sg5 -d 30
>>> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
>>> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>>>
>>
>> Hi,
>> After the initial failures about 36 hours ago, retesting
>> yesterday and today has not produced any unexpected
>> failures. And I have been trying hard on lk 3.10.4 and
>> lk 3.10.5 .
>>
>> My test program is a bit more intense than yours and can
>> be found in the sg3_utils beta in the News section of this
>> page:
>>    http://sg.danny.cz/sg/
>>
>> It is in the examples directory, two variants called
>> sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
>> compiler, IOW something that can compile c++11 . gcc 4.7.3
>> in Ubuntu 13.04 only just manages, fedora 19 should do
>> better with gcc 4.8.1 . The threading is implemented using
>> pthreads so it should be reliable.
>>
>> Typically I run multiple instances (processes) and each has
>> multiple threads. One instance can run '-x' which will cause
>> its first thread not to use O_EXCL **. All my tests currently
>> use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
>> in the billions).
>>
>> Doug Gilbert
>>
>>
>> ** Using '-x' on two instances will cause an expected failure
>>     so can be used as a control.
>>
> Hi Doug,
>
> Can I regard this as you ACK it again?

Hi,
I'd like you to test your setup with sg_tst_excl or sg_tst_excl2 .
Since my last email, I have not seen any more failures with those
tests on the patched sg driver but I did see a couple on
/dev/sd* . With sg_tst_excl2, bsg devices can be used and since bsg
accepts and ignores O_EXCL, it fails reliably.

BTW I use scsi_debug with 'delay=0' for a pseudo device.

Doug Gilbert

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 08/13/2013 11:16 AM, Douglas Gilbert wrote:
> On 13-08-12 10:46 PM, vaughan wrote:
>> On 08/06/2013 04:52 AM, Douglas Gilbert wrote:
>>> On 13-08-04 10:19 PM, vaughan wrote:
>>>> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>>>>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>>>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>>>>
>>>>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>>>>> happen between
>>>>>>>> sg_open and sg_remove.
>>>>>>>>
>>>>>>>> Changes from v4:
>>>>>>>>     * [3/4] use ERR_PTR series instead of adding another
>>>>>>>> parameter in
>>>>>>>> sg_add_sfp
>>>>>>>>     * [4/4] fix conflict for cherry-pick from v3.
>>>>>>>>
>>>>>>>> Changes from v3:
>>>>>>>>     * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>>>>     * not set exclude with sfd_lock held.
>>>>>>>>
>>>>>>>> Vaughan Cao (4):
>>>>>>>>      [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>>>>      [SCSI] sg: no need sg_open_exclusive_lock
>>>>>>>>      [SCSI] sg: checking sdp->detached isn't protected when open
>>>>>>>>      [SCSI] sg: push file descriptor list locking down to
>>>>>>>> per-device
>>>>>>>>        locking
>>>>>>>>
>>>>>>>>     drivers/scsi/sg.c | 178
>>>>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>>>>     1 file changed, 83 insertions(+), 95 deletions(-)
>>>>>>>
>>>>>>> Patchset looks good to me, although I didn't test it on hardware
>>>>>>> yet.
>>>>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>>>>
>>>>>>> James, care to pick this up?
>>>>>>
>>>>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>>>>
>>>>>> Tested O_EXCL with multiple processes and threads; passed.
>>>>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>>>>> according to the same test. Block device passed.
>>>>>>
>>>>>> James, could you clean this up:
>>>>>>      drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>>>>> [-Wunused-variable]
>>>>>
>>>>> Further testing suggests this patch on the sg driver is
>>>>> broken, so I'll rescind my ack.
>>>>>
>>>>> The case it is broken for is when a device is opened
>>>>> without O_EXCL. Now if, while it is open, a second
>>>>> thread/process tries to open the same device O_EXCL
>>>>> then IMO the second open should fail with EBUSY.
>>>>>
>>>>> My testing shows that O_EXCL opens properly deflect
>>>>> other O_EXCL opens.
>>>> Hi  Doug,
>>>>
>>>> My test don't have this issue. The routine is something as below:
>>>>
>>>> I start three opens without O_EXCL, wait 30s each, and open with
>>>> O_EXCL|O_NONBLOCK, it failed with EBUSY.
>>>> And I also call myopen with/without O_EXCL many times in background at
>>>> the same time, and the test is passed. I don't know why it failed in
>>>> your test.
>>>>
>>>> Usage: myopen [-e][-n][-d delay] -f file
>>>>         -e: exclude
>>>>         -n: nonblock
>>>>         -d: delay N seconds and then close.
>>>>
>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>> [1] 3417
>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>> [2] 3418
>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>> [3] 3419
>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>> max_active_device=6(origin 1)
>>>>    def_reserved_size=32768
>>>>    >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>> excl=0
>>>>      FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>>      FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>>      FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>>
>>>> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
>>>> [4] 3422
>>>> [3422:3351] /dev/sg5:exclude: Device or resource busy
>>>>
>>>> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>>>>
>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>> max_active_device=6(origin 1)
>>>>    def_reserved_size=32768
>>>>    >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>> excl=0
>>>>      FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>>      FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>>      FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>      cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>        No requests active
>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>> [1]   Done                    ./myopen -f /dev/sg5 -d 30
>>>> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
>>>> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>>>>
>>>
>>> Hi,
>>> After the initial failures about 36 hours ago, retesting
>>> yesterday and today has not produced any unexpected
>>> failures. And I have been trying hard on lk 3.10.4 and
>>> lk 3.10.5 .
>>>
>>> My test program is a bit more intense than yours and can
>>> be found in the sg3_utils beta in the News section of this
>>> page:
>>>    http://sg.danny.cz/sg/
>>>
>>> It is in the examples directory, two variants called
>>> sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
>>> compiler, IOW something that can compile c++11 . gcc 4.7.3
>>> in Ubuntu 13.04 only just manages, fedora 19 should do
>>> better with gcc 4.8.1 . The threading is implemented using
>>> pthreads so it should be reliable.
>>>
>>> Typically I run multiple instances (processes) and each has
>>> multiple threads. One instance can run '-x' which will cause
>>> its first thread not to use O_EXCL **. All my tests currently
>>> use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
>>> in the billions).
>>>
>>> Doug Gilbert
>>>
>>>
>>> ** Using '-x' on two instances will cause an expected failure
>>>     so can be used as a control.
>>>
>> Hi Doug,
>>
>> Can I regard this as you ACK it again?
>
> Hi,
> I'd like you to test your setup with sg_tst_excl or sg_tst_excl2 .
> Since my last email, I have not seen any more failures with those
> tests on the patched sg driver but I did see a couple on
> /dev/sd* . With sg_tst_excl2, bsg devices can be used and since bsg
> accepts and ignores O_EXCL, it fails reliably.
>
> BTW I use scsi_debug with 'delay=0' for a pseudo device.
>
> Doug Gilbert
Hi Doug,

I run test for sg and sd drivers with both sg_tst_excl and sg_tst_excl2
on kernel vanilla 3.10.9 with mypatches included on Fedora19 x86_64
baremachine.
* I've tried several times with different -w and -n setting, no failure
for sg driver found.
* It's easy to find failure on both patched and non-patched kernel for
sd driver with the following test command:
    ./sg_tst_excl -w 3 -n 2000 -t 16 -x $1 &
    ./sg_tst_excl -w 3 -n 2000 -t 16 $1 &
    ./sg_tst_excl -w 0 -n 2000 -t 16 $1 &
    ./sg_tst_excl -w -1 -n 2000 -t 16 $1 &
    ./sg_tst_excl -w -2 -n 2000 -t 16 $1 &
I think option '-w 0/-1/-2' is significant to trigger the failure, since
when I only use '-w >0', test usually passed.


Thanks,
Vaughan

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[Douglas Gilbert]

On 13-08-27 10:16 AM, vaughan wrote:
> On 08/13/2013 11:16 AM, Douglas Gilbert wrote:
>> On 13-08-12 10:46 PM, vaughan wrote:
>>> On 08/06/2013 04:52 AM, Douglas Gilbert wrote:
>>>> On 13-08-04 10:19 PM, vaughan wrote:
>>>>> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>>>>>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>>>>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>>>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>>>>>
>>>>>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>>>>>> happen between
>>>>>>>>> sg_open and sg_remove.
>>>>>>>>>
>>>>>>>>> Changes from v4:
>>>>>>>>>      * [3/4] use ERR_PTR series instead of adding another
>>>>>>>>> parameter in
>>>>>>>>> sg_add_sfp
>>>>>>>>>      * [4/4] fix conflict for cherry-pick from v3.
>>>>>>>>>
>>>>>>>>> Changes from v3:
>>>>>>>>>      * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>>>>>      * not set exclude with sfd_lock held.
>>>>>>>>>
>>>>>>>>> Vaughan Cao (4):
>>>>>>>>>       [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>>>>>       [SCSI] sg: no need sg_open_exclusive_lock
>>>>>>>>>       [SCSI] sg: checking sdp->detached isn't protected when open
>>>>>>>>>       [SCSI] sg: push file descriptor list locking down to
>>>>>>>>> per-device
>>>>>>>>>         locking
>>>>>>>>>
>>>>>>>>>      drivers/scsi/sg.c | 178
>>>>>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>>>>>      1 file changed, 83 insertions(+), 95 deletions(-)
>>>>>>>>
>>>>>>>> Patchset looks good to me, although I didn't test it on hardware
>>>>>>>> yet.
>>>>>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>>>>>
>>>>>>>> James, care to pick this up?
>>>>>>>
>>>>>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>>>>>
>>>>>>> Tested O_EXCL with multiple processes and threads; passed.
>>>>>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>>>>>> according to the same test. Block device passed.
>>>>>>>
>>>>>>> James, could you clean this up:
>>>>>>>       drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>>>>>> [-Wunused-variable]
>>>>>>
>>>>>> Further testing suggests this patch on the sg driver is
>>>>>> broken, so I'll rescind my ack.
>>>>>>
>>>>>> The case it is broken for is when a device is opened
>>>>>> without O_EXCL. Now if, while it is open, a second
>>>>>> thread/process tries to open the same device O_EXCL
>>>>>> then IMO the second open should fail with EBUSY.
>>>>>>
>>>>>> My testing shows that O_EXCL opens properly deflect
>>>>>> other O_EXCL opens.
>>>>> Hi  Doug,
>>>>>
>>>>> My test don't have this issue. The routine is something as below:
>>>>>
>>>>> I start three opens without O_EXCL, wait 30s each, and open with
>>>>> O_EXCL|O_NONBLOCK, it failed with EBUSY.
>>>>> And I also call myopen with/without O_EXCL many times in background at
>>>>> the same time, and the test is passed. I don't know why it failed in
>>>>> your test.
>>>>>
>>>>> Usage: myopen [-e][-n][-d delay] -f file
>>>>>          -e: exclude
>>>>>          -n: nonblock
>>>>>          -d: delay N seconds and then close.
>>>>>
>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>> [1] 3417
>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>> [2] 3418
>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>> [3] 3419
>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>> max_active_device=6(origin 1)
>>>>>     def_reserved_size=32768
>>>>>     >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>>> excl=0
>>>>>       FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>>       FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>>       FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>>
>>>>> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
>>>>> [4] 3422
>>>>> [3422:3351] /dev/sg5:exclude: Device or resource busy
>>>>>
>>>>> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>>>>>
>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>> max_active_device=6(origin 1)
>>>>>     def_reserved_size=32768
>>>>>     >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>>> excl=0
>>>>>       FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>>       FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>>       FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>         No requests active
>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>> [1]   Done                    ./myopen -f /dev/sg5 -d 30
>>>>> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
>>>>> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>>>>>
>>>>
>>>> Hi,
>>>> After the initial failures about 36 hours ago, retesting
>>>> yesterday and today has not produced any unexpected
>>>> failures. And I have been trying hard on lk 3.10.4 and
>>>> lk 3.10.5 .
>>>>
>>>> My test program is a bit more intense than yours and can
>>>> be found in the sg3_utils beta in the News section of this
>>>> page:
>>>>     http://sg.danny.cz/sg/
>>>>
>>>> It is in the examples directory, two variants called
>>>> sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
>>>> compiler, IOW something that can compile c++11 . gcc 4.7.3
>>>> in Ubuntu 13.04 only just manages, fedora 19 should do
>>>> better with gcc 4.8.1 . The threading is implemented using
>>>> pthreads so it should be reliable.
>>>>
>>>> Typically I run multiple instances (processes) and each has
>>>> multiple threads. One instance can run '-x' which will cause
>>>> its first thread not to use O_EXCL **. All my tests currently
>>>> use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
>>>> in the billions).
>>>>
>>>> Doug Gilbert
>>>>
>>>>
>>>> ** Using '-x' on two instances will cause an expected failure
>>>>      so can be used as a control.
>>>>
>>> Hi Doug,
>>>
>>> Can I regard this as you ACK it again?
>>
>> Hi,
>> I'd like you to test your setup with sg_tst_excl or sg_tst_excl2 .
>> Since my last email, I have not seen any more failures with those
>> tests on the patched sg driver but I did see a couple on
>> /dev/sd* . With sg_tst_excl2, bsg devices can be used and since bsg
>> accepts and ignores O_EXCL, it fails reliably.
>>
>> BTW I use scsi_debug with 'delay=0' for a pseudo device.
>>
>> Doug Gilbert
> Hi Doug,
>
> I run test for sg and sd drivers with both sg_tst_excl and sg_tst_excl2
> on kernel vanilla 3.10.9 with mypatches included on Fedora19 x86_64
> baremachine.
> * I've tried several times with different -w and -n setting, no failure
> for sg driver found.
> * It's easy to find failure on both patched and non-patched kernel for
> sd driver with the following test command:
>      ./sg_tst_excl -w 3 -n 2000 -t 16 -x $1 &
>      ./sg_tst_excl -w 3 -n 2000 -t 16 $1 &
>      ./sg_tst_excl -w 0 -n 2000 -t 16 $1 &
>      ./sg_tst_excl -w -1 -n 2000 -t 16 $1 &
>      ./sg_tst_excl -w -2 -n 2000 -t 16 $1 &
> I think option '-w 0/-1/-2' is significant to trigger the failure, since
> when I only use '-w >0', test usually passed.

Hi,
Thanks for testing that. For others trying to follow this,
the usage message for that test utility is:

Usage: sg_tst_excl [-b] [-l <lba>] [-n <n_per_thr>] [-t <num_thrs>]
                    [-V] [-w <wait_ms>] [-x] <disk_device>
   where
     -b                block on open (def: O_NONBLOCK)
     -l <lba>          logical block to increment (def: 1000)
     -n <n_per_thr>    number of loops per thread (def: 200)
     -t <num_thrs>     number of threads (def: 4)
     -V                print version number then exit
     -w <wait_ms>      >0: sleep_for(<wait_ms>); =0: yield(); -1: no
                       wait; -2: sleep(0)  (def: 0)
     -x                don't use O_EXCL on first thread (def: use
                       O_EXCL on all threads)

Test O_EXCL open flag with sg driver. Each open/close cycle with the
O_EXCL flag does a double increment on lba (using its first 4 bytes).

----------------------

The test is using O_EXCL as a lock so that if a 4 byte integer in
a block starts out as even (and for a scsi_debug pseudo, blocks are
zero filled) then a open,double_increment,close sequence should
always see an even number after the open. Notice that it should be
safe for one process to run with '-x' so that its first thread
opens the device without the O_EXCL flag.


Acked-by: Douglas Gilbert <dgilbert@interlog.com>

And I let you run with the sd driver quirk :-)

Re: [PATCH v5 0/4] [SCSI] sg: fix race condition in sg_open

[vaughan]

On 08/27/2013 09:13 PM, Douglas Gilbert wrote:
> On 13-08-27 10:16 AM, vaughan wrote:
>> On 08/13/2013 11:16 AM, Douglas Gilbert wrote:
>>> On 13-08-12 10:46 PM, vaughan wrote:
>>>> On 08/06/2013 04:52 AM, Douglas Gilbert wrote:
>>>>> On 13-08-04 10:19 PM, vaughan wrote:
>>>>>> On 08/03/2013 01:25 PM, Douglas Gilbert wrote:
>>>>>>> On 13-08-01 01:01 AM, Douglas Gilbert wrote:
>>>>>>>> On 13-07-22 01:03 PM, Jörn Engel wrote:
>>>>>>>>> On Mon, 22 July 2013 12:40:29 +0800, Vaughan Cao wrote:
>>>>>>>>>>
>>>>>>>>>> There is a race when open sg with O_EXCL flag. Also a race may
>>>>>>>>>> happen between
>>>>>>>>>> sg_open and sg_remove.
>>>>>>>>>>
>>>>>>>>>> Changes from v4:
>>>>>>>>>>      * [3/4] use ERR_PTR series instead of adding another
>>>>>>>>>> parameter in
>>>>>>>>>> sg_add_sfp
>>>>>>>>>>      * [4/4] fix conflict for cherry-pick from v3.
>>>>>>>>>>
>>>>>>>>>> Changes from v3:
>>>>>>>>>>      * release o_sem in sg_release(), not in sg_remove_sfp().
>>>>>>>>>>      * not set exclude with sfd_lock held.
>>>>>>>>>>
>>>>>>>>>> Vaughan Cao (4):
>>>>>>>>>>       [SCSI] sg: use rwsem to solve race during exclusive open
>>>>>>>>>>       [SCSI] sg: no need sg_open_exclusive_lock
>>>>>>>>>>       [SCSI] sg: checking sdp->detached isn't protected when
>>>>>>>>>> open
>>>>>>>>>>       [SCSI] sg: push file descriptor list locking down to
>>>>>>>>>> per-device
>>>>>>>>>>         locking
>>>>>>>>>>
>>>>>>>>>>      drivers/scsi/sg.c | 178
>>>>>>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>>>>>>      1 file changed, 83 insertions(+), 95 deletions(-)
>>>>>>>>>
>>>>>>>>> Patchset looks good to me, although I didn't test it on hardware
>>>>>>>>> yet.
>>>>>>>>> Signed-off-by: Joern Engel <joern@logfs.org>
>>>>>>>>>
>>>>>>>>> James, care to pick this up?
>>>>>>>>
>>>>>>>> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>>>>>>>>
>>>>>>>> Tested O_EXCL with multiple processes and threads; passed.
>>>>>>>> sg driver prior to this patch had "leaky" O_EXCL logic
>>>>>>>> according to the same test. Block device passed.
>>>>>>>>
>>>>>>>> James, could you clean this up:
>>>>>>>>       drivers/scsi/sg.c:242:6: warning: unused variable ‘res’
>>>>>>>> [-Wunused-variable]
>>>>>>>
>>>>>>> Further testing suggests this patch on the sg driver is
>>>>>>> broken, so I'll rescind my ack.
>>>>>>>
>>>>>>> The case it is broken for is when a device is opened
>>>>>>> without O_EXCL. Now if, while it is open, a second
>>>>>>> thread/process tries to open the same device O_EXCL
>>>>>>> then IMO the second open should fail with EBUSY.
>>>>>>>
>>>>>>> My testing shows that O_EXCL opens properly deflect
>>>>>>> other O_EXCL opens.
>>>>>> Hi  Doug,
>>>>>>
>>>>>> My test don't have this issue. The routine is something as below:
>>>>>>
>>>>>> I start three opens without O_EXCL, wait 30s each, and open with
>>>>>> O_EXCL|O_NONBLOCK, it failed with EBUSY.
>>>>>> And I also call myopen with/without O_EXCL many times in
>>>>>> background at
>>>>>> the same time, and the test is passed. I don't know why it failed in
>>>>>> your test.
>>>>>>
>>>>>> Usage: myopen [-e][-n][-d delay] -f file
>>>>>>          -e: exclude
>>>>>>          -n: nonblock
>>>>>>          -d: delay N seconds and then close.
>>>>>>
>>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>>> [1] 3417
>>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>>> [2] 3418
>>>>>> [root@vacaowol5 16835013]# ./myopen  -f /dev/sg5 -d 30 &
>>>>>> [3] 3419
>>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>>> max_active_device=6(origin 1)
>>>>>>     def_reserved_size=32768
>>>>>>     >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>>>> excl=0
>>>>>>       FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>>       FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>>       FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>>
>>>>>> [root@vacaowol5 16835013]# ./myopen -e -n  -f /dev/sg5 -d 30 &
>>>>>> [4] 3422
>>>>>> [3422:3351] /dev/sg5:exclude: Device or resource busy
>>>>>>
>>>>>> [4]+  Exit 1                  ./myopen -e -n -f /dev/sg5 -d 30
>>>>>>
>>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>>> max_active_device=6(origin 1)
>>>>>>     def_reserved_size=32768
>>>>>>     >>> device=sg5 scsi5 chan=0 id=1 lun=0   em=0 sg_tablesize=55
>>>>>> excl=0
>>>>>>       FD(1): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>>       FD(2): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>>       FD(3): timeout=60000ms bufflen=32768 (res)sgat=1 low_dma=0
>>>>>>       cmd_q=0 f_packid=0 k_orphan=0 closed=0
>>>>>>         No requests active
>>>>>> [root@vacaowol5 16835013]# cat /proc/scsi/sg/debug
>>>>>> [1]   Done                    ./myopen -f /dev/sg5 -d 30
>>>>>> [2]-  Done                    ./myopen -f /dev/sg5 -d 30
>>>>>> [3]+  Done                    ./myopen -f /dev/sg5 -d 30
>>>>>>
>>>>>
>>>>> Hi,
>>>>> After the initial failures about 36 hours ago, retesting
>>>>> yesterday and today has not produced any unexpected
>>>>> failures. And I have been trying hard on lk 3.10.4 and
>>>>> lk 3.10.5 .
>>>>>
>>>>> My test program is a bit more intense than yours and can
>>>>> be found in the sg3_utils beta in the News section of this
>>>>> page:
>>>>>     http://sg.danny.cz/sg/
>>>>>
>>>>> It is in the examples directory, two variants called
>>>>> sg_tst_excl and sg_tst_excl2 . You will need a recent gcc
>>>>> compiler, IOW something that can compile c++11 . gcc 4.7.3
>>>>> in Ubuntu 13.04 only just manages, fedora 19 should do
>>>>> better with gcc 4.8.1 . The threading is implemented using
>>>>> pthreads so it should be reliable.
>>>>>
>>>>> Typically I run multiple instances (processes) and each has
>>>>> multiple threads. One instance can run '-x' which will cause
>>>>> its first thread not to use O_EXCL **. All my tests currently
>>>>> use O_NONBLOCK and that leads to lots of EBUSYs (sometimes
>>>>> in the billions).
>>>>>
>>>>> Doug Gilbert
>>>>>
>>>>>
>>>>> ** Using '-x' on two instances will cause an expected failure
>>>>>      so can be used as a control.
>>>>>
>>>> Hi Doug,
>>>>
>>>> Can I regard this as you ACK it again?
>>>
>>> Hi,
>>> I'd like you to test your setup with sg_tst_excl or sg_tst_excl2 .
>>> Since my last email, I have not seen any more failures with those
>>> tests on the patched sg driver but I did see a couple on
>>> /dev/sd* . With sg_tst_excl2, bsg devices can be used and since bsg
>>> accepts and ignores O_EXCL, it fails reliably.
>>>
>>> BTW I use scsi_debug with 'delay=0' for a pseudo device.
>>>
>>> Doug Gilbert
>> Hi Doug,
>>
>> I run test for sg and sd drivers with both sg_tst_excl and sg_tst_excl2
>> on kernel vanilla 3.10.9 with mypatches included on Fedora19 x86_64
>> baremachine.
>> * I've tried several times with different -w and -n setting, no failure
>> for sg driver found.
>> * It's easy to find failure on both patched and non-patched kernel for
>> sd driver with the following test command:
>>      ./sg_tst_excl -w 3 -n 2000 -t 16 -x $1 &
>>      ./sg_tst_excl -w 3 -n 2000 -t 16 $1 &
>>      ./sg_tst_excl -w 0 -n 2000 -t 16 $1 &
>>      ./sg_tst_excl -w -1 -n 2000 -t 16 $1 &
>>      ./sg_tst_excl -w -2 -n 2000 -t 16 $1 &
>> I think option '-w 0/-1/-2' is significant to trigger the failure, since
>> when I only use '-w >0', test usually passed.
>
> Hi,
> Thanks for testing that. For others trying to follow this,
> the usage message for that test utility is:
>
> Usage: sg_tst_excl [-b] [-l <lba>] [-n <n_per_thr>] [-t <num_thrs>]
>                    [-V] [-w <wait_ms>] [-x] <disk_device>
>   where
>     -b                block on open (def: O_NONBLOCK)
>     -l <lba>          logical block to increment (def: 1000)
>     -n <n_per_thr>    number of loops per thread (def: 200)
>     -t <num_thrs>     number of threads (def: 4)
>     -V                print version number then exit
>     -w <wait_ms>      >0: sleep_for(<wait_ms>); =0: yield(); -1: no
>                       wait; -2: sleep(0)  (def: 0)
>     -x                don't use O_EXCL on first thread (def: use
>                       O_EXCL on all threads)
>
> Test O_EXCL open flag with sg driver. Each open/close cycle with the
> O_EXCL flag does a double increment on lba (using its first 4 bytes).
>
> ----------------------
>
> The test is using O_EXCL as a lock so that if a 4 byte integer in
> a block starts out as even (and for a scsi_debug pseudo, blocks are
> zero filled) then a open,double_increment,close sequence should
> always see an even number after the open. Notice that it should be
> safe for one process to run with '-x' so that its first thread
> opens the device without the O_EXCL flag.
>
>
> Acked-by: Douglas Gilbert <dgilbert@interlog.com>
>
> And I let you run with the sd driver quirk :-)
Thanks, I appreciate testing the quirk for sd.

Vaughan

Re: [PATCH v2 1/1] [SCSI] sg: fix race condition when do exclusive open

[Jörn Engel]

On Sun, 7 July 2013 01:24:44 +0800, vaughan wrote:
> 
> Do you agree that I use a per device spin_lock 'sfd_lock' to protect
> sfds list and leave sg_index_lock
> only protect the global sg device lookup? I think it's reasonable
> for concurrency.

That bit looks fine to me.  I don't think it matters much, but sure,
why not!

Jörn

--
One of the painful things about our time is that those who feel certainty
are stupid, and those with any imagination and understanding are filled
with doubt and indecision.
-- Bertrand Russell