Commit 83e8405e authored by Kalyan Kadiyala's avatar Kalyan Kadiyala Committed by Konrad Sztyber
Browse files

nvmf/fc: Qpair disconnect callback: Serialize FC delete connection & close qpair process 



    An invalid pointer access can happen with KATO and FC delete
    connection running parallelly. This is a regression that got
    introduced when the qpair disconnect callback was removed.

Change-Id: I8cf4ca3c3d91797e44c501e09222f503e00db4be
Signed-off-by: default avatarKalyan Kadiyala <kalyan.kadiyala@broadcom.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25421


Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
parent 0eab4c6f

lib/nvmf/fc.c

+27 −6
Original line number Diff line number Diff line
@@ -1305,7 +1305,8 @@ nvmf_fc_request_abort(struct spdk_nvmf_fc_request *fc_req, bool send_abts, 
	switch (fc_req->state) {

	case SPDK_NVMF_FC_REQ_BDEV_ABORTED:

		/* Aborted by backend */

		goto complete;

		_nvmf_fc_request_free(fc_req);

		break;



	case SPDK_NVMF_FC_REQ_READ_BDEV:

	case SPDK_NVMF_FC_REQ_WRITE_BDEV:
@@ -2225,6 +2226,9 @@ _nvmf_fc_close_qpair(void *arg) 
	int rc;



	fc_conn = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_fc_conn, qpair);



	SPDK_NOTICELOG("Close qpair %p, fc_conn %p conn_state %d conn_id 0x%lx\n",

		       qpair, fc_conn, fc_conn->conn_state, fc_conn->conn_id);

	if (fc_conn->conn_id == NVMF_FC_INVALID_CONN_ID) {

		struct spdk_nvmf_fc_ls_add_conn_api_data *api_data = NULL;
@@ -2242,11 +2246,7 @@ _nvmf_fc_close_qpair(void *arg) 
			return;

		}



		SPDK_ERRLOG("%s: Delete FC connection failed.\n", __func__);

	} else if (fc_conn->conn_state == SPDK_NVMF_FC_OBJECT_TO_BE_DELETED) {

		/* This is the case where deletion started from FC layer. */

		spdk_thread_send_msg(fc_ctx->qpair_thread, fc_conn->qpair_disconnect_cb_fn,

				     fc_conn->qpair_disconnect_ctx);

		SPDK_ERRLOG("Delete fc_conn %p failed.\n", fc_conn);

	}



	nvmf_fc_connection_delete_done_cb(fc_ctx);
@@ -2257,6 +2257,25 @@ nvmf_fc_close_qpair(struct spdk_nvmf_qpair *qpair, 
		    spdk_nvmf_transport_qpair_fini_cb cb_fn, void *cb_arg)

{

	struct spdk_nvmf_fc_qpair_remove_ctx *fc_ctx;

	struct spdk_nvmf_fc_conn *fc_conn;



	fc_conn = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_fc_conn, qpair);

	fc_conn->qpair_fini_done = true;



	if (fc_conn->conn_state == SPDK_NVMF_FC_OBJECT_TO_BE_DELETED) {

		if (fc_conn->qpair_fini_done_cb) {

			SPDK_NOTICELOG("Invoke qpair_fini_done_cb, fc_conn %p conn_id 0x%lx qpair %p conn_state %d\n",

				       fc_conn, fc_conn->conn_id, qpair, fc_conn->conn_state);



			fc_conn->qpair_fini_done_cb(fc_conn->hwqp, 0, fc_conn->qpair_fini_done_cb_args);

		}



		if (cb_fn) {

			cb_fn(cb_arg);

		}



		return;

	}



	fc_ctx = calloc(1, sizeof(struct spdk_nvmf_fc_qpair_remove_ctx));

	if (!fc_ctx) {
@@ -2264,8 +2283,10 @@ nvmf_fc_close_qpair(struct spdk_nvmf_qpair *qpair, 
		if (cb_fn) {

			cb_fn(cb_arg);

		}



		return;

	}



	fc_ctx->qpair = qpair;

	fc_ctx->cb_fn = cb_fn;

	fc_ctx->cb_ctx = cb_arg;

lib/nvmf/fc_ls.c

+41 −37
Original line number Diff line number Diff line
@@ -330,8 +330,6 @@ nvmf_fc_ls_new_connection(struct spdk_nvmf_fc_association *assoc, uint16_t qid, 
	fc_conn->conn_state = SPDK_NVMF_FC_OBJECT_CREATED;

	TAILQ_INIT(&fc_conn->in_use_reqs);

	TAILQ_INIT(&fc_conn->fused_waiting_queue);

	fc_conn->qpair_disconnect_cb_fn = NULL;

	fc_conn->qpair_disconnect_ctx = NULL;



	/* save target port trid in connection (for subsystem

	 * listener validation in fabric connect command)
@@ -1455,14 +1453,6 @@ nvmf_fc_poller_api_activate_queue(void *arg) 
	nvmf_fc_poller_api_perform_cb(&q_args->cb_info, 0);

}



static void

nvmf_fc_disconnect_qpair_cb(void *ctx)

{

	struct spdk_nvmf_fc_poller_api_cb_info *cb_info = ctx;

	/* perform callback */

	nvmf_fc_poller_api_perform_cb(cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);

}



static void

nvmf_fc_poller_conn_abort_done(void *hwqp, int32_t status, void *cb_args)

{
@@ -1475,31 +1465,33 @@ nvmf_fc_poller_conn_abort_done(void *hwqp, int32_t status, void *cb_args) 
	if (!conn_args->fc_request_cnt) {

		struct spdk_nvmf_fc_conn *fc_conn = conn_args->fc_conn, *tmp;



		SPDK_DEBUGLOG(nvmf_fc_poller_api, "Poller connection abort done, fc_conn %p conn_id 0x%lx "

			      "s_id 0x%x rpi 0x%x qpair_fini_done %d\n", fc_conn, fc_conn->conn_id,

			      fc_conn->s_id, fc_conn->rpi, fc_conn->qpair_fini_done);



		if (!fc_conn->qpair_fini_done) { /* nvmf_fc_close_qpair not called yet */

			fc_conn->qpair_fini_done_cb = nvmf_fc_poller_conn_abort_done;

			fc_conn->qpair_fini_done_cb_args = cb_args;



			spdk_nvmf_qpair_disconnect(&fc_conn->qpair);

			/* Wait for upper layer to call qpair_fini */

			return;

		}



		if (rte_hash_lookup_data(conn_args->hwqp->connection_list_hash,

					 (void *)&fc_conn->conn_id, (void *)&tmp) >= 0) {

			/* All the requests for this connection are aborted. */

			nvmf_fc_poller_del_conn_lookup_data(conn_args->hwqp, fc_conn);

			fc_conn->hwqp->num_conns--;



			SPDK_DEBUGLOG(nvmf_fc_poller_api, "Connection deleted, conn_id 0x%lx\n", fc_conn->conn_id);

			SPDK_DEBUGLOG(nvmf_fc_poller_api, "Connection lookup data deleted, fc_conn %p conn_id 0x%lx "

				      "rport %p rpi 0x%x assoc %p\n", fc_conn, fc_conn->conn_id,

				      fc_conn->fc_assoc->rport, fc_conn->rpi, fc_conn->fc_assoc);



			if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) {

				/* disconnect qpair from nvmf controller */

				fc_conn->qpair_disconnect_cb_fn = nvmf_fc_disconnect_qpair_cb;

				fc_conn->qpair_disconnect_ctx = &conn_args->cb_info;

				spdk_nvmf_qpair_disconnect(&fc_conn->qpair);

			} else {

				nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);

			}

		} else {

			/*

			 * Duplicate connection delete can happen if one is

			 * coming in via an association disconnect and the other

			 * is initiated by a port reset.

			 */

			SPDK_DEBUGLOG(nvmf_fc_poller_api, "Duplicate conn delete.");

			/* perform callback */

			nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);

		} else {

			SPDK_ERRLOG("fc_conn %p conn_id 0x%lx hash on delete failed.\n", fc_conn, fc_conn->conn_id);

		}

	}

}
@@ -1513,8 +1505,10 @@ nvmf_fc_poller_api_del_connection(void *arg) 
	struct spdk_nvmf_fc_request *fc_req = NULL, *tmp;

	struct spdk_nvmf_fc_hwqp *hwqp = conn_args->hwqp;



	SPDK_DEBUGLOG(nvmf_fc_poller_api, "Poller delete connection, conn_id 0x%lx\n",

		      fc_conn->conn_id);

	SPDK_DEBUGLOG(nvmf_fc_poller_api,

		      "Poller delete connection, fc_conn %p conn_id 0x%lx s_id 0x%x rpi 0x%x\n",

		      conn_args->fc_conn, conn_args->fc_conn->conn_id,

		      conn_args->fc_conn->s_id, conn_args->fc_conn->rpi);



	/* Make sure connection is valid */

	if (rte_hash_lookup_data(hwqp->connection_list_hash,
@@ -1524,6 +1518,8 @@ nvmf_fc_poller_api_del_connection(void *arg) 
		return;

	}



	assert(conn_args->fc_conn == fc_conn);



	conn_args->fc_request_cnt = 0;



	TAILQ_FOREACH_SAFE(fc_req, &fc_conn->in_use_reqs, conn_link, tmp) {
@@ -1539,21 +1535,29 @@ nvmf_fc_poller_api_del_connection(void *arg) 
				      conn_args);

	}



	SPDK_DEBUGLOG(nvmf_fc_poller_api, "Poller Disconnect API, fc_conn %p conn_id 0x%lx "

		      "s_id %x rpi %x requested abort count %d qpair_fini_done %d\n", fc_conn,

		      fc_conn->conn_id, fc_conn->s_id, fc_conn->rpi,

		      conn_args->fc_request_cnt, fc_conn->qpair_fini_done);



	if (!conn_args->fc_request_cnt) {

		SPDK_DEBUGLOG(nvmf_fc_poller_api, "Connection deleted.\n");

		if (!fc_conn->qpair_fini_done) { /* nvmf_fc_close_qpair not called yet */

			conn_args->fc_conn->qpair_fini_done_cb = nvmf_fc_poller_conn_abort_done;

			conn_args->fc_conn->qpair_fini_done_cb_args = conn_args;

			spdk_nvmf_qpair_disconnect(&fc_conn->qpair);

			return;

		}



		SPDK_DEBUGLOG(nvmf_fc_poller_api, "Connection lookup data deleted, fc_conn %p conn_id 0x%lx "

			      "rport %p rpi 0x%x assoc %p\n", fc_conn, fc_conn->conn_id,

			      fc_conn->fc_assoc->rport, fc_conn->rpi, fc_conn->fc_assoc);



		nvmf_fc_poller_del_conn_lookup_data(conn_args->hwqp, conn_args->fc_conn);

		hwqp->num_conns--;



		if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) {

			/* disconnect qpair from nvmf controller */

			fc_conn->qpair_disconnect_cb_fn = nvmf_fc_disconnect_qpair_cb;

			fc_conn->qpair_disconnect_ctx = &conn_args->cb_info;

			spdk_nvmf_qpair_disconnect(&fc_conn->qpair);

		} else {

		nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);

	}

}

}



static void

nvmf_fc_poller_abts_done(void *hwqp, int32_t status, void *cb_args)

lib/nvmf/nvmf_fc.h

+5 −4
Original line number Diff line number Diff line
@@ -187,14 +187,12 @@ struct spdk_nvmf_fc_nport { 
	TAILQ_ENTRY(spdk_nvmf_fc_nport) link; /* list of nports on a hw port. */

};



typedef void (*spdk_nvmf_fc_caller_cb)(void *hwqp, int32_t status, void *args);

/*

 * NVMF FC Connection

 */

struct spdk_nvmf_fc_conn {

	struct spdk_nvmf_qpair qpair;

	nvmf_qpair_disconnect_cb qpair_disconnect_cb_fn;

	void *qpair_disconnect_ctx;



	struct spdk_nvme_transport_id trid;



	uint32_t s_id;
@@ -253,6 +251,10 @@ struct spdk_nvmf_fc_conn { 


	/* Delete conn callback list */

	void *ls_del_op_ctx;



	bool qpair_fini_done;

	spdk_nvmf_fc_caller_cb qpair_fini_done_cb;

	void *qpair_fini_done_cb_args;

};



/*
@@ -819,7 +821,6 @@ nvmf_fc_dump_buf_print(struct spdk_nvmf_fc_queue_dump_info *dump_info, char *fmt 
/*

 * NVMF FC caller callback definitions

 */

typedef void (*spdk_nvmf_fc_caller_cb)(void *hwqp, int32_t status, void *args);



struct spdk_nvmf_fc_caller_ctx {

	void *ctx;