[PATCH] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[PATCH] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

It is possible for one thread to to take se_sess->sess_cmd_lock in
core_tmr_abort_task() before taking a reference count on
se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.

This introduces kref_put_and_lock() and uses it in
target_put_sess_cmd() to close the race window.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/target/target_core_transport.c |   17 +++++++++++------
 include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 04ec9cb..2b8277c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
 {
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
 	struct se_session *se_sess = se_cmd->se_sess;
-	unsigned long flags;
 
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return;
 	}
 	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		complete(&se_cmd->cmd_wait_comp);
 		return;
 	}
 	list_del(&se_cmd->se_cmd_list);
-	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+	spin_unlock(&se_sess->sess_cmd_lock);
 
 	se_cmd->se_tfo->release_cmd(se_cmd);
 }
@@ -2232,7 +2230,14 @@ static void target_release_cmd_kref(struct kref *kref)
  */
 int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
 {
-	return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	ret = kref_put_and_lock(&se_cmd->cmd_kref, target_release_cmd_kref,
+			&se_sess->sess_cmd_lock);
+	local_irq_restore(flags);
+	return ret;
 }
 EXPORT_SYMBOL(target_put_sess_cmd);
 
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..bc4dbc1 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/spinlock_types.h>
 
 struct kref {
 	atomic_t refcount;
@@ -95,6 +96,31 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
 	return kref_sub(kref, 1, release);
 }
 
+/**
+ * kref_put_and_lock - decrement refcount for object.
+ * @kref: object.
+ * @release: pointer to the function that will clean up the object when the
+ *	     last reference to the object is released.
+ *	     This pointer is required, and it is not acceptable to pass kfree
+ *	     in as this function.
+ * @lock: lock to take in release case
+ *
+ * Behaves identical to kref_put with one exception.  If the reference count
+ * drops to zero, the lock will be taken atomically wrt dropping the reference
+ * count.
+ */
+static inline int kref_put_and_lock(struct kref *kref,
+		void (*release)(struct kref *kref),
+		spinlock_t *lock)
+{
+	WARN_ON(release == NULL);
+	if (atomic_dec_and_lock(&kref->refcount, lock)) {
+		release(kref);
+		return 1;
+	}
+	return 0;
+}
+
 static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
-- 
1.7.10.4

Re: [PATCH] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

On Mon, 18 March 2013 17:04:11 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 06:28:53PM -0400, Jörn Engel wrote:
> > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > core_tmr_abort_task() before taking a reference count on
> > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > 
> > This introduces kref_put_and_lock() and uses it in
> > target_put_sess_cmd() to close the race window.
> 
> We already have kref_put_mutex(), why not just call this
> kref_put_spinlock()?

Back when I originally wrote this patch, kref_put_mutex() didn't exist
yet.  So there is my evil predetermined plan to introduce random
inconsistencies by being consistent with atomic_dec_and_lock()
instead.

If you think this matters I can rename the function and resend.

Jörn

--
Computer system analysis is like child-rearing; you can do grievous damage,
but you cannot ensure success."
-- Tom DeMarco

[PATCH v2] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

On Mon, 18 March 2013 17:27:42 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 06:56:57PM -0400, Jörn Engel wrote:
> > 
> > Back when I originally wrote this patch, kref_put_mutex() didn't exist
> > yet.  So there is my evil predetermined plan to introduce random
> > inconsistencies by being consistent with atomic_dec_and_lock()
> > instead.
> 
> Don't be evil, leave that up to the professionals :)

I wouldn't dare question your authority in such matters. :-P

> > If you think this matters I can rename the function and resend.
> 
> Please do, it is good to name things correctly, when we have the chance.

Fair enough.

Jörn

--
ticks = jiffies;
while (ticks == jiffies);
ticks = jiffies;
-- /usr/src/linux/init/main.c


It is possible for one thread to to take se_sess->sess_cmd_lock in
core_tmr_abort_task() before taking a reference count on
se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.

This introduces kref_put_and_lock() and uses it in
target_put_sess_cmd() to close the race window.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/target/target_core_transport.c |   17 +++++++++++------
 include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 04ec9cb..b98c158 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
 {
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
 	struct se_session *se_sess = se_cmd->se_sess;
-	unsigned long flags;
 
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return;
 	}
 	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		complete(&se_cmd->cmd_wait_comp);
 		return;
 	}
 	list_del(&se_cmd->se_cmd_list);
-	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+	spin_unlock(&se_sess->sess_cmd_lock);
 
 	se_cmd->se_tfo->release_cmd(se_cmd);
 }
@@ -2232,7 +2230,14 @@ static void target_release_cmd_kref(struct kref *kref)
  */
 int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
 {
-	return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	ret = kref_put_spinlock(&se_cmd->cmd_kref, target_release_cmd_kref,
+			&se_sess->sess_cmd_lock);
+	local_irq_restore(flags);
+	return ret;
 }
 EXPORT_SYMBOL(target_put_sess_cmd);
 
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..c40eaf6 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/spinlock_types.h>
 
 struct kref {
 	atomic_t refcount;
@@ -95,6 +96,31 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
 	return kref_sub(kref, 1, release);
 }
 
+/**
+ * kref_put_and_lock - decrement refcount for object.
+ * @kref: object.
+ * @release: pointer to the function that will clean up the object when the
+ *	     last reference to the object is released.
+ *	     This pointer is required, and it is not acceptable to pass kfree
+ *	     in as this function.
+ * @lock: lock to take in release case
+ *
+ * Behaves identical to kref_put with one exception.  If the reference count
+ * drops to zero, the lock will be taken atomically wrt dropping the reference
+ * count.
+ */
+static inline int kref_put_spinlock(struct kref *kref,
+		void (*release)(struct kref *kref),
+		spinlock_t *lock)
+{
+	WARN_ON(release == NULL);
+	if (atomic_dec_and_lock(&kref->refcount, lock)) {
+		release(kref);
+		return 1;
+	}
+	return 0;
+}
+
 static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
-- 
1.7.10.4

[PATCH v3] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

And because I'm lame and stupid, here's v3.

It is possible for one thread to to take se_sess->sess_cmd_lock in
core_tmr_abort_task() before taking a reference count on
se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.

This introduces kref_put_and_lock() and uses it in
target_put_sess_cmd() to close the race window.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/target/target_core_transport.c |   17 +++++++++++------
 include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 04ec9cb..b98c158 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
 {
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
 	struct se_session *se_sess = se_cmd->se_sess;
-	unsigned long flags;
 
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		se_cmd->se_tfo->release_cmd(se_cmd);
 		return;
 	}
 	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
-		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+		spin_unlock(&se_sess->sess_cmd_lock);
 		complete(&se_cmd->cmd_wait_comp);
 		return;
 	}
 	list_del(&se_cmd->se_cmd_list);
-	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+	spin_unlock(&se_sess->sess_cmd_lock);
 
 	se_cmd->se_tfo->release_cmd(se_cmd);
 }
@@ -2232,7 +2230,14 @@ static void target_release_cmd_kref(struct kref *kref)
  */
 int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
 {
-	return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	ret = kref_put_spinlock(&se_cmd->cmd_kref, target_release_cmd_kref,
+			&se_sess->sess_cmd_lock);
+	local_irq_restore(flags);
+	return ret;
 }
 EXPORT_SYMBOL(target_put_sess_cmd);
 
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..1f26a86 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 
 struct kref {
 	atomic_t refcount;
@@ -95,6 +96,31 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
 	return kref_sub(kref, 1, release);
 }
 
+/**
+ * kref_put_and_lock - decrement refcount for object.
+ * @kref: object.
+ * @release: pointer to the function that will clean up the object when the
+ *	     last reference to the object is released.
+ *	     This pointer is required, and it is not acceptable to pass kfree
+ *	     in as this function.
+ * @lock: lock to take in release case
+ *
+ * Behaves identical to kref_put with one exception.  If the reference count
+ * drops to zero, the lock will be taken atomically wrt dropping the reference
+ * count.
+ */
+static inline int kref_put_spinlock(struct kref *kref,
+		void (*release)(struct kref *kref),
+		spinlock_t *lock)
+{
+	WARN_ON(release == NULL);
+	if (atomic_dec_and_lock(&kref->refcount, lock)) {
+		release(kref);
+		return 1;
+	}
+	return 0;
+}
+
 static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
-- 
1.7.10.4

Re: [PATCH] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 06:28:53PM -0400, Jörn Engel wrote:
> It is possible for one thread to to take se_sess->sess_cmd_lock in
> core_tmr_abort_task() before taking a reference count on
> se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> 
> This introduces kref_put_and_lock() and uses it in
> target_put_sess_cmd() to close the race window.

We already have kref_put_mutex(), why not just call this
kref_put_spinlock()?

thanks,

greg k-h

Re: [PATCH] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 06:56:57PM -0400, Jörn Engel wrote:
> On Mon, 18 March 2013 17:04:11 -0700, Greg Kroah-Hartman wrote:
> > On Mon, Mar 18, 2013 at 06:28:53PM -0400, Jörn Engel wrote:
> > > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > > core_tmr_abort_task() before taking a reference count on
> > > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > > 
> > > This introduces kref_put_and_lock() and uses it in
> > > target_put_sess_cmd() to close the race window.
> > 
> > We already have kref_put_mutex(), why not just call this
> > kref_put_spinlock()?
> 
> Back when I originally wrote this patch, kref_put_mutex() didn't exist
> yet.  So there is my evil predetermined plan to introduce random
> inconsistencies by being consistent with atomic_dec_and_lock()
> instead.

Don't be evil, leave that up to the professionals :)

> If you think this matters I can rename the function and resend.

Please do, it is good to name things correctly, when we have the chance.

greg k-h

Re: [PATCH v3] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 07:34:13PM -0400, Jörn Engel wrote:
> And because I'm lame and stupid, here's v3.
> 
> It is possible for one thread to to take se_sess->sess_cmd_lock in
> core_tmr_abort_task() before taking a reference count on
> se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> 
> This introduces kref_put_and_lock() and uses it in
> target_put_sess_cmd() to close the race window.
> 
> Signed-off-by: Joern Engel <joern@logfs.org>
> ---
>  drivers/target/target_core_transport.c |   17 +++++++++++------
>  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 04ec9cb..b98c158 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
>  {
>  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
>  	struct se_session *se_sess = se_cmd->se_sess;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  	if (list_empty(&se_cmd->se_cmd_list)) {
> -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +		spin_unlock(&se_sess->sess_cmd_lock);

Wait, who has this locked?  You took out the call to spin_lock_* above.

And why not _irqstore() anymore?

thanks,

greg k-h

Re: [PATCH v3] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Nicholas A. Bellinger]

On Mon, 2013-03-18 at 19:34 -0400, Jörn Engel wrote:
> And because I'm lame and stupid, here's v3.
> 
> It is possible for one thread to to take se_sess->sess_cmd_lock in
> core_tmr_abort_task() before taking a reference count on
> se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> 
> This introduces kref_put_and_lock() and uses it in
> target_put_sess_cmd() to close the race window.
> 
> Signed-off-by: Joern Engel <joern@logfs.org>
> ---
>  drivers/target/target_core_transport.c |   17 +++++++++++------
>  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 04ec9cb..b98c158 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
>  {
>  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
>  	struct se_session *se_sess = se_cmd->se_sess;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  	if (list_empty(&se_cmd->se_cmd_list)) {
> -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +		spin_unlock(&se_sess->sess_cmd_lock);
>  		se_cmd->se_tfo->release_cmd(se_cmd);
>  		return;
>  	}
>  	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +		spin_unlock(&se_sess->sess_cmd_lock);
>  		complete(&se_cmd->cmd_wait_comp);
>  		return;
>  	}
>  	list_del(&se_cmd->se_cmd_list);
> -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +	spin_unlock(&se_sess->sess_cmd_lock);
>  
>  	se_cmd->se_tfo->release_cmd(se_cmd);

The fact that this changes ->release_cmd() to be called with irqs still
disabled is going to cause a problem for any fabrics that take a _bh
lock from this code-path.

iscsi-target is the only fabric AFAICT that this could potentially be
problematic for, but as I'm currently in the process of converting it to
use ->cmd_kref for v3.10 code some extra refactoring to avoid these
locks from within ->release_cmd() should not be a big deal..

So that said, applied to target-pending/for-next.

Thanks,

--nab

>  }
> @@ -2232,7 +2230,14 @@ static void target_release_cmd_kref(struct kref *kref)
>   */
>  int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
>  {
> -	return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
> +	unsigned long flags;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	ret = kref_put_spinlock(&se_cmd->cmd_kref, target_release_cmd_kref,
> +			&se_sess->sess_cmd_lock);
> +	local_irq_restore(flags);
> +	return ret;
>  }
>  EXPORT_SYMBOL(target_put_sess_cmd);
>  
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index 4972e6e..1f26a86 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -19,6 +19,7 @@
>  #include <linux/atomic.h>
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
> +#include <linux/spinlock.h>
>  
>  struct kref {
>  	atomic_t refcount;
> @@ -95,6 +96,31 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
>  	return kref_sub(kref, 1, release);
>  }
>  
> +/**
> + * kref_put_and_lock - decrement refcount for object.
> + * @kref: object.
> + * @release: pointer to the function that will clean up the object when the
> + *	     last reference to the object is released.
> + *	     This pointer is required, and it is not acceptable to pass kfree
> + *	     in as this function.
> + * @lock: lock to take in release case
> + *
> + * Behaves identical to kref_put with one exception.  If the reference count
> + * drops to zero, the lock will be taken atomically wrt dropping the reference
> + * count.
> + */
> +static inline int kref_put_spinlock(struct kref *kref,
> +		void (*release)(struct kref *kref),
> +		spinlock_t *lock)
> +{
> +	WARN_ON(release == NULL);
> +	if (atomic_dec_and_lock(&kref->refcount, lock)) {
> +		release(kref);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  static inline int kref_put_mutex(struct kref *kref,
>  				 void (*release)(struct kref *kref),
>  				 struct mutex *lock)

Re: [PATCH v3] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 07:11:12PM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2013-03-18 at 19:34 -0400, Jörn Engel wrote:
> > And because I'm lame and stupid, here's v3.
> > 
> > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > core_tmr_abort_task() before taking a reference count on
> > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > 
> > This introduces kref_put_and_lock() and uses it in
> > target_put_sess_cmd() to close the race window.
> > 
> > Signed-off-by: Joern Engel <joern@logfs.org>
> > ---
> >  drivers/target/target_core_transport.c |   17 +++++++++++------
> >  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 04ec9cb..b98c158 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
> >  {
> >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> >  	struct se_session *se_sess = se_cmd->se_sess;
> > -	unsigned long flags;
> >  
> > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> >  	if (list_empty(&se_cmd->se_cmd_list)) {
> > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +		spin_unlock(&se_sess->sess_cmd_lock);
> >  		se_cmd->se_tfo->release_cmd(se_cmd);
> >  		return;
> >  	}
> >  	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +		spin_unlock(&se_sess->sess_cmd_lock);
> >  		complete(&se_cmd->cmd_wait_comp);
> >  		return;
> >  	}
> >  	list_del(&se_cmd->se_cmd_list);
> > -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +	spin_unlock(&se_sess->sess_cmd_lock);
> >  
> >  	se_cmd->se_tfo->release_cmd(se_cmd);
> 
> The fact that this changes ->release_cmd() to be called with irqs still
> disabled is going to cause a problem for any fabrics that take a _bh
> lock from this code-path.
> 
> iscsi-target is the only fabric AFAICT that this could potentially be
> problematic for, but as I'm currently in the process of converting it to
> use ->cmd_kref for v3.10 code some extra refactoring to avoid these
> locks from within ->release_cmd() should not be a big deal..
> 
> So that said, applied to target-pending/for-next.

Really?  Don't you need my ack as you are adding a new kref api call
here.

greg k-h

Re: [PATCH v3] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Nicholas A. Bellinger]

On Mon, 2013-03-18 at 19:39 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 07:11:12PM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2013-03-18 at 19:34 -0400, Jörn Engel wrote:
> > > And because I'm lame and stupid, here's v3.
> > > 
> > > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > > core_tmr_abort_task() before taking a reference count on
> > > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > > 
> > > This introduces kref_put_and_lock() and uses it in
> > > target_put_sess_cmd() to close the race window.
> > > 
> > > Signed-off-by: Joern Engel <joern@logfs.org>
> > > ---
> > >  drivers/target/target_core_transport.c |   17 +++++++++++------
> > >  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
> > >  2 files changed, 37 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > > index 04ec9cb..b98c158 100644
> > > --- a/drivers/target/target_core_transport.c
> > > +++ b/drivers/target/target_core_transport.c
> > > @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
> > >  {
> > >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> > >  	struct se_session *se_sess = se_cmd->se_sess;
> > > -	unsigned long flags;
> > >  
> > > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> > >  	if (list_empty(&se_cmd->se_cmd_list)) {
> > > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > > +		spin_unlock(&se_sess->sess_cmd_lock);
> > >  		se_cmd->se_tfo->release_cmd(se_cmd);
> > >  		return;
> > >  	}
> > >  	if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> > > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > > +		spin_unlock(&se_sess->sess_cmd_lock);
> > >  		complete(&se_cmd->cmd_wait_comp);
> > >  		return;
> > >  	}
> > >  	list_del(&se_cmd->se_cmd_list);
> > > -	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > > +	spin_unlock(&se_sess->sess_cmd_lock);
> > >  
> > >  	se_cmd->se_tfo->release_cmd(se_cmd);
> > 
> > The fact that this changes ->release_cmd() to be called with irqs still
> > disabled is going to cause a problem for any fabrics that take a _bh
> > lock from this code-path.
> > 
> > iscsi-target is the only fabric AFAICT that this could potentially be
> > problematic for, but as I'm currently in the process of converting it to
> > use ->cmd_kref for v3.10 code some extra refactoring to avoid these
> > locks from within ->release_cmd() should not be a big deal..
> > 
> > So that said, applied to target-pending/for-next.
> 
> Really?  Don't you need my ack as you are adding a new kref api call
> here.

Yes, sorry.  

Putting this into queue for the moment.

[PATCH v4] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

On Mon, 18 March 2013 18:53:54 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 07:34:13PM -0400, Jörn Engel wrote:
> > ---
> >  drivers/target/target_core_transport.c |   17 +++++++++++------
> >  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 04ec9cb..b98c158 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
> >  {
> >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> >  	struct se_session *se_sess = se_cmd->se_sess;
> > -	unsigned long flags;
> >  
> > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> >  	if (list_empty(&se_cmd->se_cmd_list)) {
> > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > +		spin_unlock(&se_sess->sess_cmd_lock);
> 
> Wait, who has this locked?  You took out the call to spin_lock_* above.

The spinlock needs to be taken atomically wrt. the final kref_put.  So
either the kref_put was not final and we don't end up calling
target_release_cmd_kref() or it was and the spinlock is already taken
when calling the release function.  You could call that the central
bit of the patch. ;)

> And why not _irqstore() anymore?

Because I thought the resulting code would be horrible.  But going
through the excercise, it does seem half as bad as I feared.  In fact,
I rather like it now.

Jörn

--
Happiness isn't having what you want, it's wanting what you have.
-- unknown


It is possible for one thread to to take se_sess->sess_cmd_lock in
core_tmr_abort_task() before taking a reference count on
se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.

This introduces kref_put_spinlock_irqsave() and uses it in
target_put_sess_cmd() to close the race window.

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/target/target_core_transport.c |    7 +++----
 include/linux/kref.h                   |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 04ec9cb..7e856b9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2203,13 +2203,11 @@ out:
 	return ret;
 }
 
-static void target_release_cmd_kref(struct kref *kref)
+static void target_release_cmd_kref(struct kref *kref, unsigned long flags)
 {
 	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
 	struct se_session *se_sess = se_cmd->se_sess;
-	unsigned long flags;
 
-	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 	if (list_empty(&se_cmd->se_cmd_list)) {
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 		se_cmd->se_tfo->release_cmd(se_cmd);
@@ -2232,7 +2230,8 @@ static void target_release_cmd_kref(struct kref *kref)
  */
 int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
 {
-	return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
+	return kref_put_spinlock_irqsave(&se_cmd->cmd_kref, target_release_cmd_kref,
+			&se_sess->sess_cmd_lock);
 }
 EXPORT_SYMBOL(target_put_sess_cmd);
 
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 4972e6e..796c4f0 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/spinlock.h>
 
 struct kref {
 	atomic_t refcount;
@@ -95,6 +96,37 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
 	return kref_sub(kref, 1, release);
 }
 
+/**
+ * kref_put_spinlock_irqsave - decrement refcount for object.
+ * @kref: object.
+ * @release: pointer to the function that will clean up the object when the
+ *	     last reference to the object is released.
+ *	     This pointer is required, and it is not acceptable to pass kfree
+ *	     in as this function.
+ * @lock: lock to take in release case
+ *
+ * Behaves identical to kref_put with one exception.  If the reference count
+ * drops to zero, the lock will be taken atomically wrt dropping the reference
+ * count.  The release function then has to call spin_unlock_irqrestore().
+ */
+static inline int kref_put_spinlock_irqsave(struct kref *kref,
+		void (*release)(struct kref *kref, unsigned long flags),
+		spinlock_t *lock)
+{
+	unsigned long flags;
+
+	WARN_ON(release == NULL);
+	if (atomic_add_unless(&kref->refcount, -1, 1))
+		return 0;
+	spin_lock_irqsave(lock, flags);
+	if (atomic_dec_and_test(&kref->refcount)) {
+		release(kref, flags);
+		return 1;
+	}
+	spin_unlock_irqrestore(lock, flags);
+	return 0;
+}
+
 static inline int kref_put_mutex(struct kref *kref,
 				 void (*release)(struct kref *kref),
 				 struct mutex *lock)
-- 
1.7.10.4

Re: [PATCH v4] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

On Mon, 18 March 2013 22:09:54 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 11:31:12PM -0400, Jörn Engel wrote:
> > On Mon, 18 March 2013 18:53:54 -0700, Greg Kroah-Hartman wrote:
> 
> > > And why not _irqstore() anymore?
> > 
> > Because I thought the resulting code would be horrible.  But going
> > through the excercise, it does seem half as bad as I feared.  In fact,
> > I rather like it now.
> 
> You changed the kref code too, does it work better now?

It compiles.  I don't have a good testcase, so the procedure is to
throw it into the test infrastructure and wait a week.

> > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > core_tmr_abort_task() before taking a reference count on
> > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > 
> > This introduces kref_put_spinlock_irqsave() and uses it in
> > target_put_sess_cmd() to close the race window.
> > 
> > Signed-off-by: Joern Engel <joern@logfs.org>
> > ---
> >  drivers/target/target_core_transport.c |    7 +++----
> >  include/linux/kref.h                   |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 04ec9cb..7e856b9 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2203,13 +2203,11 @@ out:
> >  	return ret;
> >  }
> >  
> > -static void target_release_cmd_kref(struct kref *kref)
> > +static void target_release_cmd_kref(struct kref *kref, unsigned long flags)
> >  {
> >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> >  	struct se_session *se_sess = se_cmd->se_sess;
> > -	unsigned long flags;
> >  
> > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> 
> Why pass flags to a release function?
> 
> I don't think you can do that, but it's been a while since I looked at
> the spinlock code.

The alternative would be to call local_irq_restore(flags); from
kref_put_spinlock_irqsave() and not pass the flags.  Getting rid of
the extra parameter would be nice.  But I'm not sure I want to prove
that
	spin_unlock(lock);
	local_irq_restore(flags);
is the same as 
	spin_unlock_irqrestore(lock, flags);
on all architectures and with all combinations of CONFIG options.  I
think it should be, but I wouldn't bet half a cookie on it.

Jörn

--
He who knows that enough is enough will always have enough.
-- Lao Tsu

Re: [PATCH v4] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 11:31:12PM -0400, Jörn Engel wrote:
> On Mon, 18 March 2013 18:53:54 -0700, Greg Kroah-Hartman wrote:
> > On Mon, Mar 18, 2013 at 07:34:13PM -0400, Jörn Engel wrote:
> > > ---
> > >  drivers/target/target_core_transport.c |   17 +++++++++++------
> > >  include/linux/kref.h                   |   26 ++++++++++++++++++++++++++
> > >  2 files changed, 37 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > > index 04ec9cb..b98c158 100644
> > > --- a/drivers/target/target_core_transport.c
> > > +++ b/drivers/target/target_core_transport.c
> > > @@ -2207,21 +2207,19 @@ static void target_release_cmd_kref(struct kref *kref)
> > >  {
> > >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> > >  	struct se_session *se_sess = se_cmd->se_sess;
> > > -	unsigned long flags;
> > >  
> > > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> > >  	if (list_empty(&se_cmd->se_cmd_list)) {
> > > -		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> > > +		spin_unlock(&se_sess->sess_cmd_lock);
> > 
> > Wait, who has this locked?  You took out the call to spin_lock_* above.
> 
> The spinlock needs to be taken atomically wrt. the final kref_put.  So
> either the kref_put was not final and we don't end up calling
> target_release_cmd_kref() or it was and the spinlock is already taken
> when calling the release function.  You could call that the central
> bit of the patch. ;)

Ah, ok.

> > And why not _irqstore() anymore?
> 
> Because I thought the resulting code would be horrible.  But going
> through the excercise, it does seem half as bad as I feared.  In fact,
> I rather like it now.

You changed the kref code too, does it work better now?



> 
> Jörn
> 
> --
> Happiness isn't having what you want, it's wanting what you have.
> -- unknown
> 
> 
> It is possible for one thread to to take se_sess->sess_cmd_lock in
> core_tmr_abort_task() before taking a reference count on
> se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> 
> This introduces kref_put_spinlock_irqsave() and uses it in
> target_put_sess_cmd() to close the race window.
> 
> Signed-off-by: Joern Engel <joern@logfs.org>
> ---
>  drivers/target/target_core_transport.c |    7 +++----
>  include/linux/kref.h                   |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 04ec9cb..7e856b9 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2203,13 +2203,11 @@ out:
>  	return ret;
>  }
>  
> -static void target_release_cmd_kref(struct kref *kref)
> +static void target_release_cmd_kref(struct kref *kref, unsigned long flags)
>  {
>  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
>  	struct se_session *se_sess = se_cmd->se_sess;
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);

Why pass flags to a release function?

I don't think you can do that, but it's been a while since I looked at
the spinlock code.

greg k-h

Re: [PATCH v4] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Greg Kroah-Hartman]

On Mon, Mar 18, 2013 at 11:58:03PM -0400, Jörn Engel wrote:
> On Mon, 18 March 2013 22:09:54 -0700, Greg Kroah-Hartman wrote:
> > On Mon, Mar 18, 2013 at 11:31:12PM -0400, Jörn Engel wrote:
> > > On Mon, 18 March 2013 18:53:54 -0700, Greg Kroah-Hartman wrote:
> > 
> > > > And why not _irqstore() anymore?
> > > 
> > > Because I thought the resulting code would be horrible.  But going
> > > through the excercise, it does seem half as bad as I feared.  In fact,
> > > I rather like it now.
> > 
> > You changed the kref code too, does it work better now?
> 
> It compiles.  I don't have a good testcase, so the procedure is to
> throw it into the test infrastructure and wait a week.

Really?  Please make a test cast to test it out properly.

> > > It is possible for one thread to to take se_sess->sess_cmd_lock in
> > > core_tmr_abort_task() before taking a reference count on
> > > se_cmd->cmd_kref, while another thread in target_put_sess_cmd() drops
> > > se_cmd->cmd_kref before taking se_sess->sess_cmd_lock.
> > > 
> > > This introduces kref_put_spinlock_irqsave() and uses it in
> > > target_put_sess_cmd() to close the race window.
> > > 
> > > Signed-off-by: Joern Engel <joern@logfs.org>
> > > ---
> > >  drivers/target/target_core_transport.c |    7 +++----
> > >  include/linux/kref.h                   |   32 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > > index 04ec9cb..7e856b9 100644
> > > --- a/drivers/target/target_core_transport.c
> > > +++ b/drivers/target/target_core_transport.c
> > > @@ -2203,13 +2203,11 @@ out:
> > >  	return ret;
> > >  }
> > >  
> > > -static void target_release_cmd_kref(struct kref *kref)
> > > +static void target_release_cmd_kref(struct kref *kref, unsigned long flags)
> > >  {
> > >  	struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> > >  	struct se_session *se_sess = se_cmd->se_sess;
> > > -	unsigned long flags;
> > >  
> > > -	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> > 
> > Why pass flags to a release function?
> > 
> > I don't think you can do that, but it's been a while since I looked at
> > the spinlock code.
> 
> The alternative would be to call local_irq_restore(flags); from
> kref_put_spinlock_irqsave() and not pass the flags.  Getting rid of
> the extra parameter would be nice.  But I'm not sure I want to prove
> that
> 	spin_unlock(lock);
> 	local_irq_restore(flags);
> is the same as 
> 	spin_unlock_irqrestore(lock, flags);
> on all architectures and with all combinations of CONFIG options.

It should be.

> I think it should be, but I wouldn't bet half a cookie on it.

That's why we have the code readable for everyone to see :)

I suggest doing some research and determining if this is true or not,
please, before I can accept this.

greg k-h

Re: [PATCH v4] target: close target_put_sess_cmd() vs. core_tmr_abort_task() race

[Jörn Engel]

On Tue, 19 March 2013 06:38:52 -0700, Greg Kroah-Hartman wrote:
> On Mon, Mar 18, 2013 at 11:58:03PM -0400, Jörn Engel wrote:
> > 
> > It compiles.  I don't have a good testcase, so the procedure is to
> > throw it into the test infrastructure and wait a week.
> 
> Really?  Please make a test cast to test it out properly.

So you are willing to reject a patch that clearly fixes a bug because
there is no testcase?  And given that the bug is a race, any testcase
will still boil down to "run this for as long as it took to reproduce
the problem, then triple the time".  I can reproduce the problem in
our test infrastructure maybe twice a week, so I could call that a
testcase just to provide you some formalistic argument to accept the
patch.

Of course the alternatives would be to stop using kref or to leave the
bug in.  I really wish I wouldn't have to contemplate either of those.

> > The alternative would be to call local_irq_restore(flags); from
> > kref_put_spinlock_irqsave() and not pass the flags.  Getting rid of
> > the extra parameter would be nice.  But I'm not sure I want to prove
> > that
> > 	spin_unlock(lock);
> > 	local_irq_restore(flags);
> > is the same as 
> > 	spin_unlock_irqrestore(lock, flags);
> > on all architectures and with all combinations of CONFIG options.
> 
> It should be.

We agree then.  I will change the patch and see how it behaves in our
setup.

Jörn

--
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan