On Thu, 2012-02-02 at 16:03 -0500, Neil Horman wrote:
> fc_port->ema_list is being accessed in multiple parallel contexts with no
> locking around it leading to potential corruption.
I don't see any possibility of ema_list corruption here and if there is
any such possible case/s then we should fix such case/s at source
instead ensuring list sanity by any un-necessary additional locking as
that may hide real issue/s at source.This patch is based on code
observations and I've explanation why those cases are not issues here,
see more details below.
> 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).
Yes dev_add_pack is called before done with ema_list setup with lport
but added fcoe interface is not hooked up with lport until we are done
with complete lport config and that prevents getting to even lport which
holds ema_list head. So added packet handler fcoe_rcv() will drops all
frames untill lport/ema_list set at:-
fcoe = container_of(ptype, struct fcoe_interface,
fcoe_packet_type);
lport = fcoe->ctlr.lp;
if (unlikely(!lport)) {
FCOE_NETDEV_DBG(netdev, "Cannot find hba structure");
goto err2;
}
BTW FCoE switch is not expected to send FCoE frame until lport logged
in but just in case safety check is already in place to drop fcoe frames
in that case.
Also no possibility of list corruption during sysfs create/destroy as
all fcoe creation/destroys are serialized by global fcoe_config_mutex.
May be I'm missing something among lengthy side references after some
good review comments discussion points from Bhanu on initial patch, in
that case help me understand in what specific scenario this patch will
prevent a possible bug.
Thanks
Vasu
>
> 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
>
> Change notes:
> v2) Removed pool->lock and ex_lock changes that had incorectly crept into
> this commit
>
> 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 | 82 ++++++++++++++++++++++++++++++++---------
> include/scsi/libfc.h | 2 +
> 2 files changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> index 630291f..3c03c87 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 *);
> @@ -801,9 +802,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;
> }
>
> @@ -1329,10 +1334,13 @@ static struct fc_seq *fc_seq_assign(struct fc_lport
> *lport, struct fc_frame *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);
> }
>
> @@ -1817,12 +1825,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 +1846,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 +2182,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 +2191,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 +2212,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 +2243,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 +2357,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 +2378,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
