On Mon, Feb 06, 2012 at 03:34:00PM -0800, Vasu Dev wrote:
> 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
I do, and I've tried to explain it here. It seems I've not been clear enough.
> 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
What do think I'm trying to do here?
> 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.
>
You're analysis below is incomplete.
> > 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
The fact that dev_add_pack is called early isn't overly relevent, it was just
meant to illustrate one way in which ema_list.perv could be accessed without any
elements on the list, which is a corrupting action. There are others.
> 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;
> }
>
Ok, I'll try make this as clear as I can. Here are the concurrent paths as I
see them:
CPU0
====
fcoe_rcv
fcoe_recv_frame
fc_exch_recv
fc_find_ema
list_for_each(lport->ema_list)
CPU1
====
fcoe_destroy_work
fcoe_if_destroy
fc_exch_mgr_free
fc_exch_mgr_del
list_del(lport->ema_list)
CPU2
====
fcoe_if_create
fcoe_em_config
fc_exch_mgr_add
list_add(lport->ema_list)
Notes:
CPU0 derives the lport from the structure surrounding the packet_type struct,
which isn't removed until after fcoe_if_destroy is called (via
fcoe_interface_cleanup).
CPU1 and CPU2 are serialized by the config_mutex. However, neither of them are
serialized against CPU0 which runs in softirq context. And since fcoe_rcv
determines the fcoe_ctlr and lport based on the struct surrounding the passed in
packet_type, which isn't removed until the end of fcoe_destroy_work (via
fcoe_interface_cleanup). Its entirely possible for the path in CPU0 to run
concurrently with a remove operation in CPU1, causing a list traversal at the
same time as a list_del (read, corruption).
> 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.
>
The above scenario is on delete, which presumes that the lport is already logged
in.
> Also no possibility of list corruption during sysfs create/destroy as
> all fcoe creation/destroys are serialized by global fcoe_config_mutex.
>
Correct, but I'm not talking about serialization of two list modifications, I'm
talking about the serialization of a list traversal with a modification.
> 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.
>
Please see above
Thanks
Neil
> 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
