Re: [Open-FCoE] [PATCH 1/2] fcoe: provide exclusion to concurrent ema_list access

[Bhanu Prakash Gollapudi]

On 2/2/2012 7:50 AM, Neil Horman wrote:
fc_port->ema_list is being accessed in multiple parallel contexts with no
locking around it leading to potential corruption.  Whats more the fc_find_ema
function was manually accessing ema_list.prev without checking for a list_empty
condition.  The ema_list could easily be emtpy if an transation came in to the
foce stack with an UNKNOWN xid prior to our adding a ema anchor to the list
(dev_add_pack gets called before said add occurs).
Fix it by adding an ema_lock to list accesses.  Make it an rcu lock so we don't
get Warnings when we try to tx a frame with a spinlock held

Signed-off-by: Neil Horman<nhor...@tuxdriver.com>
CC: kiran.pa...@intel.com
CC: bprak...@broadcom.com
CC: robert.w.l...@intel.com
---
  drivers/scsi/libfc/fc_exch.c |  102 +++++++++++++++++++++++++++++++-----------
  include/scsi/libfc.h         |    2 +
  2 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 630291f..9c63464 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -130,6 +130,7 @@ struct fc_exch_mgr_anchor {
        struct list_head ema_list;
        struct fc_exch_mgr *mp;
        bool (*match)(struct fc_frame *);
+       struct rcu_head rcu;
  };

  static void fc_exch_rrq(struct fc_exch *);
@@ -402,9 +403,10 @@ static void fc_exch_delete(struct fc_exch *ep)
  {
        struct fc_exch_pool *pool;
        u16 index;
+       unsigned long flags;

        pool = ep->pool;
-       spin_lock_bh(&pool->lock);
+       spin_lock_irqsave(&pool->lock, flags);

There is no contention between hard IRQs, so why do we need 
spin_lock_irqsave()? spin_lock_bh() should be good enough as the 
contention is only between softirq and process context.

If we can use spin_lock_bh(), second patch can be avoided.

        WARN_ON(pool->total_exches<= 0);
        pool->total_exches--;

@@ -419,7 +421,7 @@ static void fc_exch_delete(struct fc_exch *ep)

        fc_exch_ptr_set(pool, index, NULL);
        list_del(&ep->ex_list);
-       spin_unlock_bh(&pool->lock);
+       spin_unlock_irqrestore(&pool->lock, flags);
        fc_exch_release(ep);    /* drop hold for exch in mp */
  }

@@ -801,9 +803,13 @@ static inline struct fc_exch *fc_exch_alloc(struct 
fc_lport *lport,
  {
        struct fc_exch_mgr_anchor *ema;

-       list_for_each_entry(ema,&lport->ema_list, ema_list)
-               if (!ema->match || ema->match(fp))
+       rcu_read_lock();
+       list_for_each_entry_rcu(ema,&lport->ema_list, ema_list)
+               if (!ema->match || ema->match(fp)) {
+                       rcu_read_unlock();
                        return fc_exch_em_alloc(lport, ema->mp);
+               }
+       rcu_read_unlock();
        return NULL;
  }

@@ -1324,15 +1330,17 @@ free:
  static struct fc_seq *fc_seq_assign(struct fc_lport *lport, struct fc_frame 
*fp)
  {
        struct fc_exch_mgr_anchor *ema;
-
        WARN_ON(lport != fr_dev(fp));
        WARN_ON(fr_seq(fp));
        fr_seq(fp) = NULL;

-       list_for_each_entry(ema,&lport->ema_list, ema_list)
+       rcu_read_lock();
+       list_for_each_entry_rcu(ema,&lport->ema_list, ema_list)
                if ((!ema->match || ema->match(fp))&&
                fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)
                        break;
+
+       rcu_read_unlock();
        return fr_seq(fp);
  }

@@ -1733,8 +1741,9 @@ static void fc_exch_reset(struct fc_exch *ep)
        void (*resp)(struct fc_seq *, struct fc_frame *, void *);
        void *arg;
        int rc = 1;
+       unsigned long flags;

-       spin_lock_bh(&ep->ex_lock);
+       spin_lock_irqsave(&ep->ex_lock, flags);
        fc_exch_abort_locked(ep, 0);
        ep->state |= FC_EX_RST_CLEANUP;
        if (cancel_delayed_work(&ep->timeout_work))
@@ -1747,7 +1756,7 @@ static void fc_exch_reset(struct fc_exch *ep)
        arg = ep->arg;
        sp =&ep->seq;
        rc = fc_exch_done_locked(ep);
-       spin_unlock_bh(&ep->ex_lock);
+       spin_unlock_irqrestore(&ep->ex_lock, flags);
        if (!rc)
                fc_exch_delete(ep);

@@ -1773,20 +1782,21 @@ static void fc_exch_pool_reset(struct fc_lport *lport,
  {
        struct fc_exch *ep;
        struct fc_exch *next;
+       unsigned long flags;

-       spin_lock_bh(&pool->lock);
+       spin_lock_irqsave(&pool->lock, flags);
  restart:
        list_for_each_entry_safe(ep, next,&pool->ex_list, ex_list) {
                if ((lport == ep->lp)&&
                (sid == 0 || sid == ep->sid)&&
                (did == 0 || did == ep->did)) {
                        fc_exch_hold(ep);
-                       spin_unlock_bh(&pool->lock);
+                       spin_unlock_irqrestore(&pool->lock, flags);

                        fc_exch_reset(ep);

                        fc_exch_release(ep);
-                       spin_lock_bh(&pool->lock);
+                       spin_lock_irqsave(&pool->lock, flags);

                        /*
                         * must restart loop incase while lock
@@ -1798,7 +1808,7 @@ restart:
        pool->next_index = 0;
        pool->left = FC_XID_UNKNOWN;
        pool->right = FC_XID_UNKNOWN;
-       spin_unlock_bh(&pool->lock);
+       spin_unlock_irqrestore(&pool->lock,flags);
  }

  /**
@@ -1817,12 +1827,14 @@ void fc_exch_mgr_reset(struct fc_lport *lport, u32 sid, 
u32 did)
        struct fc_exch_mgr_anchor *ema;
        unsigned int cpu;

-       list_for_each_entry(ema,&lport->ema_list, ema_list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(ema,&lport->ema_list, ema_list) {
                for_each_possible_cpu(cpu)
                        fc_exch_pool_reset(lport,
                                           per_cpu_ptr(ema->mp->pool, cpu),
                                           sid, did);
        }
+       rcu_read_unlock();
  }
  EXPORT_SYMBOL(fc_exch_mgr_reset);

@@ -1836,11 +1848,17 @@ EXPORT_SYMBOL(fc_exch_mgr_reset);
  static struct fc_exch *fc_exch_lookup(struct fc_lport *lport, u32 xid)
  {
        struct fc_exch_mgr_anchor *ema;
+       struct fc_exch *exch = NULL;

-       list_for_each_entry(ema,&lport->ema_list, ema_list)
-               if (ema->mp->min_xid<= xid&&  xid<= ema->mp->max_xid)
-                       return fc_exch_find(ema->mp, xid);
-       return NULL;
+       rcu_read_lock();
+       list_for_each_entry_rcu(ema,&lport->ema_list, ema_list) {
+               if (ema->mp->min_xid<= xid&&  xid<= ema->mp->max_xid) {
+                       exch = fc_exch_find(ema->mp, xid);
+                       break;
+               }
+       }
+       rcu_read_unlock();
+       return exch;
  }

  /**
@@ -2166,6 +2184,7 @@ struct fc_exch_mgr_anchor *fc_exch_mgr_add(struct 
fc_lport *lport,
                                           bool (*match)(struct fc_frame *))
  {
        struct fc_exch_mgr_anchor *ema;
+       unsigned long flags;

        ema = kmalloc(sizeof(*ema), GFP_ATOMIC);
        if (!ema)
@@ -2174,8 +2193,10 @@ struct fc_exch_mgr_anchor *fc_exch_mgr_add(struct 
fc_lport *lport,
        ema->mp = mp;
        ema->match = match;
        /* add EM anchor to EM anchors list */
-       list_add_tail(&ema->ema_list,&lport->ema_list);
+       spin_lock_irqsave(&lport->ema_lock, flags);
+       list_add_tail_rcu(&ema->ema_list,&lport->ema_list);
        kref_get(&mp->kref);
+       spin_unlock_irqrestore(&lport->ema_lock, flags);
        return ema;
  }
  EXPORT_SYMBOL(fc_exch_mgr_add);
@@ -2193,16 +2214,26 @@ static void fc_exch_mgr_destroy(struct kref *kref)
        kfree(mp);
  }

+static void del_ema_rcu_cb(struct rcu_head *head)
+{
+       struct fc_exch_mgr_anchor *ema;
+
+       ema = container_of(head, struct fc_exch_mgr_anchor, rcu);
+
+       kref_put(&ema->mp->kref, fc_exch_mgr_destroy);
+       kfree(ema);
+}
+
  /**
   * fc_exch_mgr_del() - Delete an EM from a local port's list
   * @ema: The exchange manager anchor identifying the EM to be deleted
+ * NOTE: Assumes that the ema_lock for the list this anchor is on is held
   */
  void fc_exch_mgr_del(struct fc_exch_mgr_anchor *ema)
  {
        /* remove EM anchor from EM anchors list */
-       list_del(&ema->ema_list);
-       kref_put(&ema->mp->kref, fc_exch_mgr_destroy);
-       kfree(ema);
+       list_del_rcu(&ema->ema_list);
+       call_rcu(&ema->rcu, del_ema_rcu_cb);
  }
  EXPORT_SYMBOL(fc_exch_mgr_del);

@@ -2214,15 +2245,21 @@ EXPORT_SYMBOL(fc_exch_mgr_del);
  int fc_exch_mgr_list_clone(struct fc_lport *src, struct fc_lport *dst)
  {
        struct fc_exch_mgr_anchor *ema, *tmp;
+       unsigned long flags;

+       spin_lock_irqsave(&src->ema_lock, flags);
        list_for_each_entry(ema,&src->ema_list, ema_list) {
                if (!fc_exch_mgr_add(dst, ema->mp, ema->match))
                        goto err;
        }
+       spin_unlock_irqrestore(&src->ema_lock, flags);
        return 0;
  err:
+       spin_unlock_irqrestore(&src->ema_lock, flags);
+       spin_lock_irqsave(&dst->ema_lock, flags);
        list_for_each_entry_safe(ema, tmp,&dst->ema_list, ema_list)
                fc_exch_mgr_del(ema);
+       spin_unlock_irqrestore(&dst->ema_lock, flags);
        return -ENOMEM;
  }
  EXPORT_SYMBOL(fc_exch_mgr_list_clone);
@@ -2322,10 +2359,13 @@ EXPORT_SYMBOL(fc_exch_mgr_alloc);
  void fc_exch_mgr_free(struct fc_lport *lport)
  {
        struct fc_exch_mgr_anchor *ema, *next;
+       unsigned long flags;

        flush_workqueue(fc_exch_workqueue);
+       spin_lock_irqsave(&lport->ema_lock, flags);
        list_for_each_entry_safe(ema, next,&lport->ema_list, ema_list)
                fc_exch_mgr_del(ema);
+       spin_unlock_irqrestore(&lport->ema_lock, flags);
  }
  EXPORT_SYMBOL(fc_exch_mgr_free);

@@ -2340,24 +2380,32 @@ static struct fc_exch_mgr_anchor *fc_find_ema(u32 f_ctl,
                                              struct fc_lport *lport,
                                              struct fc_frame_header *fh)
  {
-       struct fc_exch_mgr_anchor *ema;
        u16 xid;
+       struct fc_exch_mgr_anchor *ema = NULL;

+       rcu_read_lock();
        if (f_ctl&  FC_FC_EX_CTX)
                xid = ntohs(fh->fh_ox_id);
        else {
                xid = ntohs(fh->fh_rx_id);
-               if (xid == FC_XID_UNKNOWN)
-                       return list_entry(lport->ema_list.prev,
+               if (xid == FC_XID_UNKNOWN) {
+
+                       ema = list_empty(&lport->ema_list) ?
+                               NULL :
+                               list_entry(lport->ema_list.prev,
                                          typeof(*ema), ema_list);
+                       goto out;
+               }
        }

-       list_for_each_entry(ema,&lport->ema_list, ema_list) {
+       list_for_each_entry_rcu(ema,&lport->ema_list, ema_list) {
                if ((xid>= ema->mp->min_xid)&&
                (xid<= ema->mp->max_xid))
-                       return ema;
+                       goto out;
        }
-       return NULL;
+out:
+       rcu_read_unlock();
+       return ema;
  }
  /**
   * fc_exch_recv() - Handler for received frames
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index 8f9dfba..5627d99 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -850,6 +850,7 @@ struct fc_lport {
        /* Associations */
        struct Scsi_Host               *host;
        struct list_head               ema_list;
+       spinlock_t                     ema_lock;
        struct fc_rport_priv           *dns_rdata;
        struct fc_rport_priv           *ms_rdata;
        struct fc_rport_priv           *ptp_rdata;
@@ -1024,6 +1025,7 @@ libfc_host_alloc(struct scsi_host_template *sht, int 
priv_size)
        lport = shost_priv(shost);
        lport->host = shost;
        INIT_LIST_HEAD(&lport->ema_list);
+       spin_lock_init(&lport->ema_lock);
        INIT_LIST_HEAD(&lport->vports);
        return lport;
  }


_______________________________________________
devel mailing list
devel@open-fcoe.org
https://lists.open-fcoe.org/mailman/listinfo/devel