sg: O_EXCL and other lock handling

drivers/scsi/sg.c typedef struct sg_fd { /* holds the state of a file descriptor */ · line 171

Quoted 1 line

drivers/scsi/sg.c +1 -0

+	struct semaphore or_sem;  /* protect co-incident opens and releases */

Seems like this should be a mutex.

Quoted 5 lines

sfds_list_empty(Sg_device *sdp)
{
unsigned long flags;
int ret;

drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 230

Quoted 4 lines

drivers/scsi/sg.c +3 -0

+	spin_lock_irqsave(&sdp->sfd_lock, flags);
+	ret = list_empty(&sdp->sfds);
+	spin_unlock_irqrestore(&sdp->sfd_lock, flags);
  	return ret;

Protecting just a list_empty check with a local will give you racy results. Seems like you should take the look over the check and the resulting action that modifies the list. That'd also mean replacing the wait_event* calls with open coded prepare_wait / finish_wait loops.

drivers/scsi/sg.c sg_open(struct inode *inode, struct file *filp) · line 288

Quoted 6 lines

drivers/scsi/sg.c +6 -0

+	down(&sdp->or_sem);
+	alone = sfds_list_empty(sdp);
+	if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+		retval = -EPERM; /* Don't allow O_EXCL with read only access */
+		goto error_out;
+	}

Seems like the pure flags check should move to the beginning of the function before taking any locks.

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines

+ struct semaphore or_sem; /* protect co-incident opens and
releases */

Seems like this should be a mutex.

Yes, it is being used as a mutex. However looking at their semantics (mutex.h versus semaphore.h), a mutex takes into account the task owner. If the user space wants to pass around a sg file descriptor in a Unix domain socket (see TLPI, Kerrisk) I don't see why the sg driver should object (and pay the small performance hit for each check).

A nasty side effect of a mutex taking into account the task owner is that it can't be used in interrupt context. My patch does not try to do that yet (see next section) but why bother. Give me a simple mutex and I'll use it.

Quoted 10 lines

drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 230

Quoted 9 lines

sfds_list_empty(Sg_device *sdp)

  {
      unsigned long flags;
      int ret;

drivers/scsi/sg.c +3 -0

+    spin_lock_irqsave(&sdp->sfd_lock, flags);
+    ret = list_empty(&sdp->sfds);
+    spin_unlock_irqrestore(&sdp->sfd_lock, flags);
      return ret;

Quoted 4 lines

Protecting just a list_empty check with a local will give you racy results. Seems like you should take the look over the check and the resulting action that modifies the list. That'd also mean replacing the wait_event* calls with open coded prepare_wait / finish_wait loops.

Not (usually) in this case. The sdp->sfds list can only be expanded by another sg_open(same_dev) but this has been excluded by taking down(&sdp->or_sem) prior to that call. The sdp->sfds list is only normally decreased by sg_release() which is also excluded by down(&sdp->or_sem).

The abnormal case is device removal (detaching). Now an open(same_dev, O_EXCL) may start waiting just after a detach but miss the wake up on open_wait. That suggests the wake_up(open_wait) in sg_remove() should also take the sdp->or_sem semaphore.
Ah, and if sg_remove() can be called from an interrupt context then that takes out using mutexes :-)

The two level of locks in sg_remove() is already making me uncomfortable, adding the sdp->or_sem semaphore to the mix calls for more analysis.

Also note that sdp->detached is only protected by a volatile type modifier and checks on it are sprinkled around the code in a rather unscientific way.

As you are no doubt aware, handling the "surprise" device removal case safely is hard, very hard. And I have tried to test this, loading up 4 processes with 100 tasks each, some with asynchronous queueing requests but most hanging on an open_wait then remove the device. So far I have not seen a hang. Again, somebody with a big machine and patience might like to try a scaled up device removal test to see what breaks.

Quoted 8 lines

Quoted 7 lines

drivers/scsi/sg.c +4 -0

+    down(&sdp->or_sem);
+    alone = sfds_list_empty(sdp);
+    if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+        retval = -EPERM; /* Don't allow O_EXCL with read only

access */

(quoted diff) +2 -0

+        goto error_out;
+    }

Quoted 2 lines

Seems like the pure flags check should move to the beginning of the function before taking any locks.

As Vaughan pointed out, just prior to that down() is a call scsi_block_when_processing_errors() for blocking open()s.

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines

+ struct semaphore or_sem; /* protect co-incident opens and
releases */

Seems like this should be a mutex.

Yes, it is being used as a mutex. However looking at their semantics (mutex.h versus semaphore.h), a mutex takes into account the task owner. If the user space wants to pass around a sg file descriptor in a Unix domain socket (see TLPI, Kerrisk) I don't see why the sg driver should object (and pay the small performance hit for each check).

A nasty side effect of a mutex taking into account the task owner is that it can't be used in interrupt context. My patch does not try to do that yet (see next section) but why bother. Give me a simple mutex and I'll use it.

Quoted 10 lines

drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 230

Quoted 9 lines

sfds_list_empty(Sg_device *sdp)

  {
      unsigned long flags;
      int ret;

drivers/scsi/sg.c +3 -0

+    spin_lock_irqsave(&sdp->sfd_lock, flags);
+    ret = list_empty(&sdp->sfds);
+    spin_unlock_irqrestore(&sdp->sfd_lock, flags);
      return ret;

Quoted 4 lines

Protecting just a list_empty check with a local will give you racy results. Seems like you should take the look over the check and the resulting action that modifies the list. That'd also mean replacing the wait_event* calls with open coded prepare_wait / finish_wait loops.

Not (usually) in this case. The sdp->sfds list can only be expanded by another sg_open(same_dev) but this has been excluded by taking down(&sdp->or_sem) prior to that call. The sdp->sfds list is only normally decreased by sg_release() which is also excluded by down(&sdp->or_sem).

The abnormal case is device removal (detaching). Now an open(same_dev, O_EXCL) may start waiting just after a detach but miss the wake up on open_wait. That suggests the wake_up(open_wait) in sg_remove() should also take the sdp->or_sem semaphore.
Ah, and if sg_remove() can be called from an interrupt context then that takes out using mutexes :-)

The two level of locks in sg_remove() is already making me uncomfortable, adding the sdp->or_sem semaphore to the mix calls for more analysis.

Also note that sdp->detached is only protected by a volatile type modifier and checks on it are sprinkled around the code in a rather unscientific way.

As you are no doubt aware, handling the "surprise" device removal case safely is hard, very hard. And I have tried to test this, loading up 4 processes with 100 tasks each, some with asynchronous queueing requests but most hanging on an open_wait then remove the device. So far I have not seen a hang. Again, somebody with a big machine and patience might like to try a scaled up device removal test to see what breaks.

Quoted 8 lines

Quoted 7 lines

drivers/scsi/sg.c +4 -0

+    down(&sdp->or_sem);
+    alone = sfds_list_empty(sdp);
+    if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+        retval = -EPERM; /* Don't allow O_EXCL with read only

access */

(quoted diff) +2 -0

+        goto error_out;
+    }

Quoted 2 lines

Seems like the pure flags check should move to the beginning of the function before taking any locks.

As Vaughan pointed out, just prior to that down() is a call scsi_block_when_processing_errors() for blocking open()s.

Quoted 49 lines · Douglas Gilbert

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines

+ struct semaphore or_sem; /* protect co-incident opens and
releases */

Seems like this should be a mutex.

Yes, it is being used as a mutex. However looking at their semantics (mutex.h versus semaphore.h), a mutex takes into account the task owner. If the user space wants to pass around a sg file descriptor in a Unix domain socket (see TLPI, Kerrisk) I don't see why the sg driver should object (and pay the small performance hit for each check).

A nasty side effect of a mutex taking into account the task owner is that it can't be used in interrupt context. My patch does not try to do that yet (see next section) but why bother. Give me a simple mutex and I'll use it.

Quoted 10 lines

drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 230

Quoted 9 lines

sfds_list_empty(Sg_device *sdp)

   {
       unsigned long flags;
       int ret;

drivers/scsi/sg.c +3 -0

+    spin_lock_irqsave(&sdp->sfd_lock, flags);
+    ret = list_empty(&sdp->sfds);
+    spin_unlock_irqrestore(&sdp->sfd_lock, flags);
       return ret;

Quoted 4 lines

Protecting just a list_empty check with a local will give you racy results. Seems like you should take the look over the check and the resulting action that modifies the list. That'd also mean replacing the wait_event* calls with open coded prepare_wait / finish_wait loops.

Not (usually) in this case. The sdp->sfds list can only be expanded by another sg_open(same_dev) but this has been excluded by taking down(&sdp->or_sem) prior to that call. The sdp->sfds list is only normally decreased by sg_release() which is also excluded by down(&sdp->or_sem).

The abnormal case is device removal (detaching). Now an open(same_dev, O_EXCL) may start waiting just after a detach but miss the wake up on open_wait. That suggests the wake_up(open_wait) in sg_remove() should also take the sdp->or_sem semaphore.
Ah, and if sg_remove() can be called from an interrupt context then that takes out using mutexes :-)

Quoted 3 lines

The two level of locks in sg_remove() is already making me uncomfortable, adding the sdp->or_sem semaphore to the mix calls for more analysis.

CCed to Jörn Engel.

I feel the same regarding the many locks. After reviewing patches I wrote these days, I suppose the version 2
(https://lkml.org/lkml/2013/6/17/319) may worth a re-consideration somehow. The purpose of or_sem is to mutex co-incident open and allow only one thread to be processing since it completes checking condition to really linking a new sfd into sfd_siblings list. My v2 re-implement a sem using sfd_lock and toopen. But as Jörn pointed out in the following mail, what I implemented is a rw_sem and that is weird and made him "having trouble wrapping my head around the semantics of this variable"...
With wait_event involved, it's indeed no sense to use a rw_sem here. However, if we restrict toopen in [0, 1], not [0, INT_MAX] as I did in v2, it will act in the same way as or_sem does. Beside the same affect as or_sem, this implement avoids the problem using in interrupt context and more readable for me, rather than twisting my head among many spinlocks and semaphores.
Although v2 seems attractive to me, things would be more complicated when detached comes in...

The following is a thought I presented previously:
Is it possible to split the sg_add_sfp() into two
functions, sg_init_sfp() and sg_add_sfp2(). We can do all initialize work in sg_init_sfp()
without any lock and let sg_add_sfp2() only serve lock-check-add in one place. It is less flaky.

Quoted 24 lines

Also note that sdp->detached is only protected by a volatile type modifier and checks on it are sprinkled around the code in a rather unscientific way.

As you are no doubt aware, handling the "surprise" device removal case safely is hard, very hard. And I have tried to test this, loading up 4 processes with 100 tasks each, some with asynchronous queueing requests but most hanging on an open_wait then remove the device. So far I have not seen a hang. Again, somebody with a big machine and patience might like to try a scaled up device removal test to see what breaks.

Quoted 10 lines

Review

Quoted 7 lines

drivers/scsi/sg.c +4 -0

+    down(&sdp->or_sem);
+    alone = sfds_list_empty(sdp);
+    if ((flags & O_EXCL) && (O_RDONLY == (flags & O_ACCMODE))) {
+        retval = -EPERM; /* Don't allow O_EXCL with read only

access */

(quoted diff) +2 -0

+        goto error_out;
+    }

Seems like the pure flags check should move to the beginning of the function before taking any locks.

Quoted 2 lines

As Vaughan pointed out, just prior to that down() is a call scsi_block_when_processing_errors() for blocking open()s.

Douglas,
What Christoph points out is a sanity check for the input parameter flags, not the scsi_block_when_processing_errors issue. I guess you misread that:)

On 13-11-01 01:16 AM, vaughan wrote:

I do not follow the last point but that is not important.

For reasons that I listed in a private post I think that my patch presented in this thread is closer to our goals than your patch (2013/6/17/319). Timing is important as well since we are approaching the lk 3.13 merge window. Regressions are what will set us back.

Yes, it is being used as a mutex. However looking at their semantics (mutex.h versus semaphore.h), a mutex takes into account the task owner. If the user space wants to pass around a sg file descriptor in a Unix domain socket (see TLPI, Kerrisk) I don't see why the sg driver should object (and pay the small performance hit for each check).

section) but why bother. Give me a simple mutex and I'll use it.

Not (usually) in this case. The sdp->sfds list can only be expanded by another sg_open(same_dev) but this has been excluded by taking down(&sdp->or_sem) prior to that call. The sdp->sfds list is only normally decreased by sg_release() which is also excluded by down(&sdp->or_sem).

The abnormal case is device removal (detaching). Now an open(same_dev, O_EXCL) may start waiting just after a detach but miss the wake up on open_wait. That suggests the wake_up(open_wait) in sg_remove() should also take the sdp->or_sem semaphore.
Ah, and if sg_remove() can be called from an interrupt context then that takes out using mutexes :-)

The two level of locks in sg_remove() is already making me uncomfortable, adding the sdp->or_sem semaphore to the mix calls for more analysis.

IMO that is a bug in scsi_block_when_processing_errors() and the down() is placed lower than it should be in sg_open() to account for that bug.

Quoted 7 lines · Douglas Gilbert

Yes, it is being used as a mutex. However looking at their semantics (mutex.h versus semaphore.h), a mutex takes into account the task owner. If the user space wants to pass around a sg file descriptor in a Unix domain socket (see TLPI, Kerrisk) I don't see why the sg driver should object (and pay the small performance hit for each check).

The sg driver won't object. The lock is taken again and released during sg_open and sg_release, which are guranteed not to migrate to a different process during their run time.

Quoted 2 lines

section) but why bother. Give me a simple mutex and I'll use it.

mutex_init/mutex_lock/mutex_unlock from <linux/mutex.h>

Quoted 6 lines

Not (usually) in this case. The sdp->sfds list can only be expanded by another sg_open(same_dev) but this has been excluded by taking down(&sdp->or_sem) prior to that call. The sdp->sfds list is only normally decreased by sg_release() which is also excluded by down(&sdp->or_sem).

Quoted 7 lines

The abnormal case is device removal (detaching). Now an open(same_dev, O_EXCL) may start waiting just after a detach but miss the wake up on open_wait. That suggests the wake_up(open_wait) in sg_remove() should also take the sdp->or_sem semaphore.
Ah, and if sg_remove() can be called from an interrupt context then that takes out using mutexes :-)

I don't think that sg_remove can be called from irq context. It always is called through the class interface remove_dev method, which always is called under a lock.

Quoted 3 lines

The two level of locks in sg_remove() is already making me uncomfortable, adding the sdp->or_sem semaphore to the mix calls for more analysis.

I would suggest to remove the list lock and only use the or_sem replacement.

Quoted 3 lines

IMO that is a bug in scsi_block_when_processing_errors() and the down() is placed lower than it should be in sg_open() to account for that bug.

How about we get that fixed first?

Quoted 2 lines

during sg_open and sg_release, which are guranteed not to migrate to a different process during their run time.

True. What I stated would be a problem if a mutex tried to do something similar to Vaughan's patch that was reverted just before lk 3.12.0. That held a read or write semaphore between a sg_open() and sg_release().

But your point begs the question why should any driver pay the extra cost and usability restrictions of a kernel mutex over a semaphore without good reason:

So what contexts can mutexes be used in?

Also, which one in the above list tells me that sg_open() cannot return to the caller with a mutex held? Given the special relationship guaranteed by the OS (POSIX) between sg_open() and sg_release() *** for the same file handle, if this is not allowed, why?

Quoted 2 lines

It always is called through the class interface remove_dev method, which always is called under a lock.

This is an important piece of extra information. Does that mean sg_add() and sg_remove() can't wait on anything? I note that sg_add() calls sg_alloc() which immediately does:
sdp = kzalloc(sizeof(Sg_device), GFP_KERNEL);

Where are the preconditions from the scsi subsystem to a ULD stating:
- that sg_remove() and sg_add() are called from a non-interrupt, non tasklet and non timer context?

- invocations of sg_add() and sg_remove() are paired in a similar fashion as sg_open() and sg_release()

By the second point I mean, for example, that sg_remove() cannot be called twice for the same device without a sg_add() between them to add (back) that device.