From: Vasu Dev <vasu.dev@linux.intel.com>
To: Hannes Reinecke <hare@suse.de>,
"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>,
Bart van Assche <bart.vanassche@sandisk.com>,
James Bottomley <james.bottomley@hansenpartnership.com>,
linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCHv2] libfc: Update rport reference counting
Date: Tue, 24 May 2016 09:39:04 -0700 [thread overview]
Message-ID: <1464107944.29525.1.camel@linux.intel.com> (raw)
In-Reply-To: <1464070318-69866-1-git-send-email-hare@suse.de>
On Tue, 2016-05-24 at 08:11 +0200, Hannes Reinecke wrote:
> Originally libfc would just be initializing the refcount to '1',
> and using the disc_mutex to synchronize if and when the final put
> should be happening.
> This has a race condition as the mutex might be delayed, causing
> other threads to access an invalid structure.
> This patch updates the rport reference counting to increase the
> reference every time 'rport_lookup' is called, and decreases
> the reference correspondingly.
> This removes the need to hold 'disc_mutex' when removing the
> structure, and avoids the above race condition.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
> drivers/scsi/fcoe/fcoe_ctlr.c | 7 +------
> drivers/scsi/libfc/fc_lport.c | 19 ++++++++++-------
> drivers/scsi/libfc/fc_rport.c | 49 ++++++++++++++++++++++-----------
> ----------
> 3 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c
> b/drivers/scsi/fcoe/fcoe_ctlr.c
> index 3e83d48..ada4bde 100644
> --- a/drivers/scsi/fcoe/fcoe_ctlr.c
> +++ b/drivers/scsi/fcoe/fcoe_ctlr.c
> @@ -2496,14 +2496,13 @@ static int fcoe_ctlr_vn_lookup(struct
> fcoe_ctlr *fip, u32 port_id, u8 *mac)
> struct fcoe_rport *frport;
> int ret = -1;
>
> - rcu_read_lock();
> rdata = lport->tt.rport_lookup(lport, port_id);
> if (rdata) {
> frport = fcoe_ctlr_rport(rdata);
> memcpy(mac, frport->enode_mac, ETH_ALEN);
> ret = 0;
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> }
> - rcu_read_unlock();
> return ret;
> }
>
> @@ -2585,11 +2584,7 @@ static void fcoe_ctlr_vn_beacon(struct
> fcoe_ctlr *fip,
> fcoe_ctlr_vn_send(fip, FIP_SC_VN_PROBE_REQ,
> fcoe_all_vn2vn, 0);
> return;
> }
> - mutex_lock(&lport->disc.disc_mutex);
> rdata = lport->tt.rport_lookup(lport, new->ids.port_id);
> - if (rdata)
> - kref_get(&rdata->kref);
> - mutex_unlock(&lport->disc.disc_mutex);
> if (rdata) {
> if (rdata->ids.node_name == new->ids.node_name &&
> rdata->ids.port_name == new->ids.port_name) {
> diff --git a/drivers/scsi/libfc/fc_lport.c
> b/drivers/scsi/libfc/fc_lport.c
> index e01a298..b9b44da 100644
> --- a/drivers/scsi/libfc/fc_lport.c
> +++ b/drivers/scsi/libfc/fc_lport.c
> @@ -2090,7 +2090,7 @@ int fc_lport_bsg_request(struct fc_bsg_job
> *job)
> struct fc_rport *rport;
> struct fc_rport_priv *rdata;
> int rc = -EINVAL;
> - u32 did;
> + u32 did, tov;
>
> job->reply->reply_payload_rcv_len = 0;
> if (rsp)
> @@ -2121,15 +2121,20 @@ int fc_lport_bsg_request(struct fc_bsg_job
> *job)
>
> case FC_BSG_HST_CT:
> did = ntoh24(job->request->rqst_data.h_ct.port_id);
> - if (did == FC_FID_DIR_SERV)
> + if (did == FC_FID_DIR_SERV) {
> rdata = lport->dns_rdata;
> - else
> + if (!rdata)
> + break;
> + tov = rdata->e_d_tov;
> + } else {
> rdata = lport->tt.rport_lookup(lport, did);
> + if (!rdata)
> + break;
> + tov = rdata->e_d_tov;
> + kref_put(&rdata->kref, lport-
> >tt.rport_destroy);
> + }
>
> - if (!rdata)
> - break;
> -
> - rc = fc_lport_ct_request(job, lport, did, rdata-
> >e_d_tov);
> + rc = fc_lport_ct_request(job, lport, did, tov);
> break;
>
> case FC_BSG_HST_ELS_NOLOGIN:
> diff --git a/drivers/scsi/libfc/fc_rport.c
> b/drivers/scsi/libfc/fc_rport.c
> index 589ff9a..93f5961 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -95,17 +95,23 @@ static const char *fc_rport_state_names[] = {
> * @lport: The local port to lookup the remote port on
> * @port_id: The remote port ID to look up
> *
> - * The caller must hold either disc_mutex or rcu_read_lock().
> + * The reference count of the fc_rport_priv structure is
> + * increased by one.
> */
> static struct fc_rport_priv *fc_rport_lookup(const struct fc_lport
> *lport,
> u32 port_id)
> {
> - struct fc_rport_priv *rdata;
> + struct fc_rport_priv *rdata = NULL, *tmp_rdata;
>
> - list_for_each_entry_rcu(rdata, &lport->disc.rports, peers)
> - if (rdata->ids.port_id == port_id)
> - return rdata;
> - return NULL;
> + rcu_read_lock();
> + list_for_each_entry_rcu(tmp_rdata, &lport->disc.rports,
> peers)
> + if (tmp_rdata->ids.port_id == port_id &&
> + kref_get_unless_zero(&tmp_rdata->kref)) {
> + rdata = tmp_rdata;
> + break;
> + }
> + rcu_read_unlock();
> + return rdata;
> }
>
> /**
> @@ -340,7 +346,6 @@ static void fc_rport_work(struct work_struct
> *work)
> fc_remote_port_delete(rport);
> }
>
> - mutex_lock(&lport->disc.disc_mutex);
> mutex_lock(&rdata->rp_mutex);
> if (rdata->rp_state == RPORT_ST_DELETE) {
> if (port_id == FC_FID_DIR_SERV) {
> @@ -370,7 +375,6 @@ static void fc_rport_work(struct work_struct
> *work)
> fc_rport_enter_ready(rdata);
> mutex_unlock(&rdata->rp_mutex);
> }
> - mutex_unlock(&lport->disc.disc_mutex);
> break;
>
> default:
> @@ -702,7 +706,7 @@ out:
> err:
> mutex_unlock(&rdata->rp_mutex);
> put:
> - kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> return;
> bad:
> FC_RPORT_DBG(rdata, "Bad FLOGI response\n");
> @@ -762,8 +766,6 @@ static void fc_rport_recv_flogi_req(struct
> fc_lport *lport,
> FC_RPORT_ID_DBG(lport, sid, "Received FLOGI request\n");
>
> disc = &lport->disc;
> - mutex_lock(&disc->disc_mutex);
> -
> if (!lport->point_to_multipoint) {
> rjt_data.reason = ELS_RJT_UNSUP;
> rjt_data.explan = ELS_EXPL_NONE;
> @@ -808,7 +810,7 @@ static void fc_rport_recv_flogi_req(struct
> fc_lport *lport,
> mutex_unlock(&rdata->rp_mutex);
> rjt_data.reason = ELS_RJT_FIP;
> rjt_data.explan = ELS_EXPL_NOT_NEIGHBOR;
> - goto reject;
> + goto reject_put;
> case RPORT_ST_FLOGI:
> case RPORT_ST_PLOGI_WAIT:
> case RPORT_ST_PLOGI:
> @@ -825,13 +827,13 @@ static void fc_rport_recv_flogi_req(struct
> fc_lport *lport,
> mutex_unlock(&rdata->rp_mutex);
> rjt_data.reason = ELS_RJT_BUSY;
> rjt_data.explan = ELS_EXPL_NONE;
> - goto reject;
> + goto reject_put;
> }
> if (fc_rport_login_complete(rdata, fp)) {
> mutex_unlock(&rdata->rp_mutex);
> rjt_data.reason = ELS_RJT_LOGIC;
> rjt_data.explan = ELS_EXPL_NONE;
> - goto reject;
> + goto reject_put;
> }
>
> fp = fc_frame_alloc(lport, sizeof(*flp));
> @@ -851,12 +853,13 @@ static void fc_rport_recv_flogi_req(struct
> fc_lport *lport,
> fc_rport_state_enter(rdata, RPORT_ST_PLOGI_WAIT);
> out:
> mutex_unlock(&rdata->rp_mutex);
> - mutex_unlock(&disc->disc_mutex);
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> fc_frame_free(rx_fp);
> return;
>
> +reject_put:
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> reject:
> - mutex_unlock(&disc->disc_mutex);
> lport->tt.seq_els_rsp_send(rx_fp, ELS_LS_RJT, &rjt_data);
> fc_frame_free(rx_fp);
> }
> @@ -923,7 +926,7 @@ out:
> fc_frame_free(fp);
> err:
> mutex_unlock(&rdata->rp_mutex);
> - kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> }
>
> static bool
> @@ -1477,14 +1480,11 @@ static void fc_rport_recv_els_req(struct
> fc_lport *lport, struct fc_frame *fp)
> struct fc_rport_priv *rdata;
> struct fc_seq_els_data els_data;
>
> - mutex_lock(&lport->disc.disc_mutex);
> rdata = lport->tt.rport_lookup(lport, fc_frame_sid(fp));
> - if (!rdata) {
> - mutex_unlock(&lport->disc.disc_mutex);
> + if (!rdata)
> goto reject;
> - }
> +
> mutex_lock(&rdata->rp_mutex);
> - mutex_unlock(&lport->disc.disc_mutex);
>
> switch (rdata->rp_state) {
> case RPORT_ST_PRLI:
> @@ -1494,6 +1494,7 @@ static void fc_rport_recv_els_req(struct
> fc_lport *lport, struct fc_frame *fp)
> break;
> default:
> mutex_unlock(&rdata->rp_mutex);
> + kref_put(&rdata->kref, lport->tt.rport_destroy);
> goto reject;
> }
>
> @@ -1524,6 +1525,7 @@ static void fc_rport_recv_els_req(struct
> fc_lport *lport, struct fc_frame *fp)
> }
>
> mutex_unlock(&rdata->rp_mutex);
> + kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
> return;
>
> reject:
> @@ -1907,7 +1909,6 @@ static void fc_rport_recv_logo_req(struct
> fc_lport *lport, struct fc_frame *fp)
>
> sid = fc_frame_sid(fp);
>
> - mutex_lock(&lport->disc.disc_mutex);
> rdata = lport->tt.rport_lookup(lport, sid);
> if (rdata) {
> mutex_lock(&rdata->rp_mutex);
> @@ -1916,10 +1917,10 @@ static void fc_rport_recv_logo_req(struct
> fc_lport *lport, struct fc_frame *fp)
>
> fc_rport_enter_delete(rdata, RPORT_EV_LOGO);
> mutex_unlock(&rdata->rp_mutex);
> + kref_put(&rdata->kref, rdata->local_port-
> >tt.rport_destroy);
> } else
> FC_RPORT_ID_DBG(lport, sid,
> "Received LOGO from non-logged-in
> port\n");
> - mutex_unlock(&lport->disc.disc_mutex);
> fc_frame_free(fp);
> }
Looks good.
Acked-by: Vasu Dev <vasu.dev@intel.com>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2016-05-24 16:39 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-24 6:11 [PATCHv2] libfc: Update rport reference counting Hannes Reinecke
2016-05-24 16:39 ` Vasu Dev [this message]
2016-06-14 10:20 ` Johannes Thumshirn
2016-06-15 1:10 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1464107944.29525.1.camel@linux.intel.com \
--to=vasu.dev@linux.intel.com \
--cc=bart.vanassche@sandisk.com \
--cc=hare@suse.com \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=james.bottomley@hansenpartnership.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.