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@xxxxxxxxx>
> ---
> 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)
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
