[PATCHv2 for-next 00/19] Misc update for rtrs

[PATCHv2 for-next 00/19] Misc update for rtrs

[Gioh Kim]

Hi Jason, hi Doug,

Please consider to include following changes to the next merge window.
It contains:
- Patch 01 ~ 11: Typical code refactoring patches
- Patch 12: Requested by Jason
https://www.spinics.net/lists/linux-rdma/msg102009.html
- Patch 13 ~ 19: Bug fixes

V2->V1:
- drop one patch "RDMA/rtrs-clt: No need to check queue_depth when
receiving" as requested by Leon
- (void) casting will be removed by next patch set
as requested by Leon.
https://www.spinics.net/lists/linux-rdma/msg102200.html


Dima Stepanov (1):
  RDMA/rtrs: Use strscpy instead of strlcpy

Gioh Kim (7):
  RDMA/rtrs-clt: Remove MAX_SESS_QUEUE_DEPTH from rtrs_send_sess_info
  RDMA/rtrs: Change MAX_SESS_QUEUE_DEPTH
  RDMA/rtrs: Define MIN_CHUNK_SIZE
  RDMA/rtrs: Do not reset hb_missed_max after re-connection
  RDMA/rtrs-srv: Duplicated session name is not allowed
  RDMA/rtrs-srv: Fix memory leak of unfreed rtrs_srv_stats object
  RDMA/rtrs-clt: Fix memory leak of not-freed sess->stats and
    stats->pcpu_stats

Guoqing Jiang (6):
  RDMA/rtrs-srv: Kill reject_w_econnreset label
  RDMA/rtrs-srv: Clean up the code in __rtrs_srv_change_state
  RDMA/rtrs-clt: Kill rtrs_clt_{start,stop}_hb
  RDMA/rtrs-clt: Kill rtrs_clt_disconnect_from_sysfs
  RDMA/rtrs-srv: Kill __rtrs_srv_change_state
  RDMA/rtrs-clt: Remove redundant 'break'

Jack Wang (1):
  RDMA/rtrs-srv: Fix memory leak when having multiple sessions

Md Haris Iqbal (4):
  RDMA/rtrs-srv: Add error messages for cases when failing RDMA
    connection
  RDMA/rtrs-clt: Check state of the rtrs_clt_sess before reading its
    stats
  RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight
  RDMA/rtrs-clt: Check if the queue_depth has changed during a
    reconnection

 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c |   5 +-
 drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  59 +++----
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       |   2 +-
 drivers/infiniband/ulp/rtrs/rtrs-pri.h       |  14 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c |  12 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |   1 +
 drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 163 +++++++++++--------
 drivers/infiniband/ulp/rtrs/rtrs-srv.h       |   4 +-
 drivers/infiniband/ulp/rtrs/rtrs.c           |   1 -
 9 files changed, 147 insertions(+), 114 deletions(-)

-- 
2.25.1

[PATCHv2 for-next 01/19] RDMA/rtrs-srv: Kill reject_w_econnreset label

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

We can goto reject_w_err label after initialize err with -ECONNRESET.

Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 740cafffb7ef..3d09d01e34b4 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1780,33 +1780,33 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 
 	u16 version, con_num, cid;
 	u16 recon_cnt;
-	int err;
+	int err = -ECONNRESET;
 
 	if (len < sizeof(*msg)) {
 		pr_err("Invalid RTRS connection request\n");
-		goto reject_w_econnreset;
+		goto reject_w_err;
 	}
 	if (le16_to_cpu(msg->magic) != RTRS_MAGIC) {
 		pr_err("Invalid RTRS magic\n");
-		goto reject_w_econnreset;
+		goto reject_w_err;
 	}
 	version = le16_to_cpu(msg->version);
 	if (version >> 8 != RTRS_PROTO_VER_MAJOR) {
 		pr_err("Unsupported major RTRS version: %d, expected %d\n",
 		       version >> 8, RTRS_PROTO_VER_MAJOR);
-		goto reject_w_econnreset;
+		goto reject_w_err;
 	}
 	con_num = le16_to_cpu(msg->cid_num);
 	if (con_num > 4096) {
 		/* Sanity check */
 		pr_err("Too many connections requested: %d\n", con_num);
-		goto reject_w_econnreset;
+		goto reject_w_err;
 	}
 	cid = le16_to_cpu(msg->cid);
 	if (cid >= con_num) {
 		/* Sanity check */
 		pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
-		goto reject_w_econnreset;
+		goto reject_w_err;
 	}
 	recon_cnt = le16_to_cpu(msg->recon_cnt);
 	srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
@@ -1826,7 +1826,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 			rtrs_err(s, "Session in wrong state: %s\n",
 				  rtrs_srv_state_str(sess->state));
 			mutex_unlock(&srv->paths_mutex);
-			goto reject_w_econnreset;
+			goto reject_w_err;
 		}
 		/*
 		 * Sanity checks
@@ -1835,13 +1835,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 			rtrs_err(s, "Incorrect request: %d, %d\n",
 				  cid, con_num);
 			mutex_unlock(&srv->paths_mutex);
-			goto reject_w_econnreset;
+			goto reject_w_err;
 		}
 		if (s->con[cid]) {
 			rtrs_err(s, "Connection already exists: %d\n",
 				  cid);
 			mutex_unlock(&srv->paths_mutex);
-			goto reject_w_econnreset;
+			goto reject_w_err;
 		}
 	} else {
 		sess = __alloc_sess(srv, cm_id, con_num, recon_cnt,
@@ -1882,9 +1882,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 reject_w_err:
 	return rtrs_rdma_do_reject(cm_id, err);
 
-reject_w_econnreset:
-	return rtrs_rdma_do_reject(cm_id, -ECONNRESET);
-
 close_and_return_err:
 	mutex_unlock(&srv->paths_mutex);
 	close_sess(sess);
-- 
2.25.1

[PATCHv2 for-next 02/19] RDMA/rtrs-clt: Remove MAX_SESS_QUEUE_DEPTH from rtrs_send_sess_info

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

Client receives queue_depth value from server. There is no need
to use MAX_SESS_QUEUE_DEPTH value.

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 5e94686bb22d..930a1b496f84 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2455,7 +2455,7 @@ static int rtrs_send_sess_info(struct rtrs_clt_sess *sess)
 	int err;
 
 	rx_sz  = sizeof(struct rtrs_msg_info_rsp);
-	rx_sz += sizeof(u64) * MAX_SESS_QUEUE_DEPTH;
+	rx_sz += sizeof(struct rtrs_sg_desc) * sess->queue_depth;
 
 	tx_iu = rtrs_iu_alloc(1, sizeof(struct rtrs_msg_info_req), GFP_KERNEL,
 			       sess->s.dev->ib_dev, DMA_TO_DEVICE,
-- 
2.25.1

[PATCHv2 for-next 03/19] RDMA/rtrs-srv: Add error messages for cases when failing RDMA connection

[Gioh Kim]

From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>

It was difficult to find out why it failed to establish RDMA
connection. This patch adds some messages to show which function
has failed why.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 3d09d01e34b4..df17dd4c1e28 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
 	 * If this request is not the first connection request from the
 	 * client for this session then fail and return error.
 	 */
-	if (!first_conn)
+	if (!first_conn) {
+		pr_err("Error: Not the first connection request for this session\n");
 		return ERR_PTR(-ENXIO);
+	}
 
 	/* need to allocate a new srv */
 	srv = kzalloc(sizeof(*srv), GFP_KERNEL);
@@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 	srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
 	if (IS_ERR(srv)) {
 		err = PTR_ERR(srv);
+		pr_err("get_or_create_srv(), error %d\n", err);
 		goto reject_w_err;
 	}
 	mutex_lock(&srv->paths_mutex);
@@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 			mutex_unlock(&srv->paths_mutex);
 			put_srv(srv);
 			err = PTR_ERR(sess);
+			pr_err("RTRS server session allocation failed: %d\n", err);
 			goto reject_w_err;
 		}
 	}
 	err = create_con(sess, cm_id, cid);
 	if (err) {
+		rtrs_err((&sess->s), "create_con(), error %d\n", err);
 		(void)rtrs_rdma_do_reject(cm_id, err);
 		/*
 		 * Since session has other connections we follow normal way
@@ -1865,6 +1870,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
 	}
 	err = rtrs_rdma_do_accept(sess, cm_id);
 	if (err) {
+		rtrs_err((&sess->s), "rtrs_rdma_do_accept(), error %d\n", err);
 		(void)rtrs_rdma_do_reject(cm_id, err);
 		/*
 		 * Since current connection was successfully added to the
-- 
2.25.1

[PATCHv2 for-next 04/19] RDMA/rtrs-srv: Clean up the code in __rtrs_srv_change_state

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

No need to use double switch to check the change of state everywhere,
let's change them to "if" to reduce size.

Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index df17dd4c1e28..74a5dfe85813 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -77,32 +77,17 @@ static bool __rtrs_srv_change_state(struct rtrs_srv_sess *sess,
 	old_state = sess->state;
 	switch (new_state) {
 	case RTRS_SRV_CONNECTED:
-		switch (old_state) {
-		case RTRS_SRV_CONNECTING:
+		if (old_state == RTRS_SRV_CONNECTING)
 			changed = true;
-			fallthrough;
-		default:
-			break;
-		}
 		break;
 	case RTRS_SRV_CLOSING:
-		switch (old_state) {
-		case RTRS_SRV_CONNECTING:
-		case RTRS_SRV_CONNECTED:
+		if (old_state == RTRS_SRV_CONNECTING ||
+		    old_state == RTRS_SRV_CONNECTED)
 			changed = true;
-			fallthrough;
-		default:
-			break;
-		}
 		break;
 	case RTRS_SRV_CLOSED:
-		switch (old_state) {
-		case RTRS_SRV_CLOSING:
+		if (old_state == RTRS_SRV_CLOSING)
 			changed = true;
-			fallthrough;
-		default:
-			break;
-		}
 		break;
 	default:
 		break;
-- 
2.25.1

[PATCHv2 for-next 05/19] RDMA/rtrs: Change MAX_SESS_QUEUE_DEPTH

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

Max IB immediate data size is 2^28 (MAX_IMM_PAYL_BITS)
and the minimum chunk size is 4096 (2^12).
Therefore the maximum sess_queue_depth is 65536 (2^16).

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-pri.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
index bcad5e2168c5..62924ad5362d 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
@@ -47,12 +47,15 @@ enum {
 	MAX_PATHS_NUM = 128,
 
 	/*
-	 * With the size of struct rtrs_permit allocated on the client, 4K
-	 * is the maximum number of rtrs_permits we can allocate. This number is
-	 * also used on the client to allocate the IU for the user connection
-	 * to receive the RDMA addresses from the server.
+	 * Max IB immediate data size is 2^28 (MAX_IMM_PAYL_BITS)
+	 * and the minimum chunk size is 4096 (2^12).
+	 * So the maximum sess_queue_depth is 65536 (2^16) in theory.
+	 * But mempool_create, create_qp and ib_post_send fail with
+	 * "cannot allocate memory" error if sess_queue_depth is too big.
+	 * Therefore the pratical max value of sess_queue_depth is
+	 * somewhere between 1 and 65536 and it depends on the system.
 	 */
-	MAX_SESS_QUEUE_DEPTH = 4096,
+	MAX_SESS_QUEUE_DEPTH = 65536,
 
 	RTRS_HB_INTERVAL_MS = 5000,
 	RTRS_HB_MISSED_MAX = 5,
-- 
2.25.1

[PATCHv2 for-next 06/19] RDMA/rtrs: Define MIN_CHUNK_SIZE

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

Define MIN_CHUNK_SIZE to replace the hard-coding number.
We need 4k for metadata, so MIN_CHUNK_SIZE should be at least 8k.

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-pri.h | 1 +
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
index 62924ad5362d..a092f7d7c7b3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
@@ -56,6 +56,7 @@ enum {
 	 * somewhere between 1 and 65536 and it depends on the system.
 	 */
 	MAX_SESS_QUEUE_DEPTH = 65536,
+	MIN_CHUNK_SIZE = 8192,
 
 	RTRS_HB_INTERVAL_MS = 5000,
 	RTRS_HB_MISSED_MAX = 5,
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 74a5dfe85813..02bc704667a8 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -2165,9 +2165,9 @@ static int check_module_params(void)
 		       sess_queue_depth, 1, MAX_SESS_QUEUE_DEPTH);
 		return -EINVAL;
 	}
-	if (max_chunk_size < 4096 || !is_power_of_2(max_chunk_size)) {
+	if (max_chunk_size < MIN_CHUNK_SIZE || !is_power_of_2(max_chunk_size)) {
 		pr_err("Invalid max_chunk_size value %d, has to be >= %d and should be power of two.\n",
-		       max_chunk_size, 4096);
+		       max_chunk_size, MIN_CHUNK_SIZE);
 		return -EINVAL;
 	}
 
-- 
2.25.1

[PATCHv2 for-next 07/19] RDMA/rtrs: Use strscpy instead of strlcpy

[Gioh Kim]

From: Dima Stepanov <dmitrii.stepanov@cloud.ionos.com>

During checkpatch analyzing the following warning message was found:
  WARNING:STRLCPY: Prefer strscpy over strlcpy - see:
  https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
Fix it by using strscpy calls instead of strlcpy.

Signed-off-by: Dima Stepanov <dmitrii.stepanov@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 ++--
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 930a1b496f84..a2505ea4a7a2 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1499,7 +1499,7 @@ static struct rtrs_clt_sess *alloc_sess(struct rtrs_clt *clt,
 	if (path->src)
 		memcpy(&sess->s.src_addr, path->src,
 		       rdma_addr_size((struct sockaddr *)path->src));
-	strlcpy(sess->s.sessname, clt->sessname, sizeof(sess->s.sessname));
+	strscpy(sess->s.sessname, clt->sessname, sizeof(sess->s.sessname));
 	sess->s.con_num = con_num;
 	sess->clt = clt;
 	sess->max_pages_per_mr = max_segments * max_segment_size >> 12;
@@ -2644,7 +2644,7 @@ static struct rtrs_clt *alloc_clt(const char *sessname, size_t paths_num,
 	clt->priv = priv;
 	clt->link_ev = link_ev;
 	clt->mp_policy = MP_POLICY_MIN_INFLIGHT;
-	strlcpy(clt->sessname, sessname, sizeof(clt->sessname));
+	strscpy(clt->sessname, sessname, sizeof(clt->sessname));
 	init_waitqueue_head(&clt->permits_wait);
 	mutex_init(&clt->paths_ev_mutex);
 	mutex_init(&clt->paths_mutex);
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 02bc704667a8..afa63f06586b 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -780,7 +780,7 @@ static int process_info_req(struct rtrs_srv_con *con,
 	rwr = kcalloc(sess->mrs_num, sizeof(*rwr), GFP_KERNEL);
 	if (unlikely(!rwr))
 		return -ENOMEM;
-	strlcpy(sess->s.sessname, msg->sessname, sizeof(sess->s.sessname));
+	strscpy(sess->s.sessname, msg->sessname, sizeof(sess->s.sessname));
 
 	tx_sz  = sizeof(*rsp);
 	tx_sz += sizeof(rsp->desc[0]) * sess->mrs_num;
@@ -1261,7 +1261,7 @@ int rtrs_srv_get_sess_name(struct rtrs_srv *srv, char *sessname, size_t len)
 	list_for_each_entry(sess, &srv->paths_list, s.entry) {
 		if (sess->state != RTRS_SRV_CONNECTED)
 			continue;
-		strlcpy(sessname, sess->s.sessname,
+		strscpy(sessname, sess->s.sessname,
 		       min_t(size_t, sizeof(sess->s.sessname), len));
 		err = 0;
 		break;
@@ -1715,7 +1715,7 @@ static struct rtrs_srv_sess *__alloc_sess(struct rtrs_srv *srv,
 	path.src = &sess->s.src_addr;
 	path.dst = &sess->s.dst_addr;
 	rtrs_addr_to_str(&path, str, sizeof(str));
-	strlcpy(sess->s.sessname, str, sizeof(sess->s.sessname));
+	strscpy(sess->s.sessname, str, sizeof(sess->s.sessname));
 
 	sess->s.con_num = con_num;
 	sess->s.recon_cnt = recon_cnt;
-- 
2.25.1

[PATCHv2 for-next 08/19] RDMA/rtrs-clt: Kill rtrs_clt_{start,stop}_hb

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

The two wrappers are not needed since we can call rtrs_{start,stop}_hb
directly.

Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index a2505ea4a7a2..0f19c133b7cc 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1449,16 +1449,6 @@ static void rtrs_clt_init_hb(struct rtrs_clt_sess *sess)
 		      rtrs_wq);
 }
 
-static void rtrs_clt_start_hb(struct rtrs_clt_sess *sess)
-{
-	rtrs_start_hb(&sess->s);
-}
-
-static void rtrs_clt_stop_hb(struct rtrs_clt_sess *sess)
-{
-	rtrs_stop_hb(&sess->s);
-}
-
 static void rtrs_clt_reconnect_work(struct work_struct *work);
 static void rtrs_clt_close_work(struct work_struct *work);
 
@@ -2088,7 +2078,7 @@ static void rtrs_clt_stop_and_destroy_conns(struct rtrs_clt_sess *sess)
 	 */
 	synchronize_rcu();
 
-	rtrs_clt_stop_hb(sess);
+	rtrs_stop_hb(&sess->s);
 
 	/*
 	 * The order it utterly crucial: firstly disconnect and complete all
@@ -2281,7 +2271,7 @@ static int init_conns(struct rtrs_clt_sess *sess)
 	if (err)
 		goto destroy;
 
-	rtrs_clt_start_hb(sess);
+	rtrs_start_hb(&sess->s);
 
 	return 0;
 
-- 
2.25.1

[PATCHv2 for-next 09/19] RDMA/rtrs-clt: Kill rtrs_clt_disconnect_from_sysfs

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

The function is just a wrapper of rtrs_clt_close_conns, let's call
rtrs_clt_close_conns directly.

Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c | 5 +----
 drivers/infiniband/ulp/rtrs/rtrs-clt.c       | 9 +--------
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       | 2 +-
 3 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
index 7d53d18a5004..4ee592ccf979 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
@@ -250,7 +250,6 @@ static ssize_t rtrs_clt_disconnect_store(struct kobject *kobj,
 					  const char *buf, size_t count)
 {
 	struct rtrs_clt_sess *sess;
-	int ret;
 
 	sess = container_of(kobj, struct rtrs_clt_sess, kobj);
 	if (!sysfs_streq(buf, "1")) {
@@ -258,9 +257,7 @@ static ssize_t rtrs_clt_disconnect_store(struct kobject *kobj,
 			  attr->attr.name, buf);
 		return -EINVAL;
 	}
-	ret = rtrs_clt_disconnect_from_sysfs(sess);
-	if (ret)
-		return ret;
+	rtrs_clt_close_conns(sess, true);
 
 	return count;
 }
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 0f19c133b7cc..1de72784e1c0 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1849,7 +1849,7 @@ static int rtrs_rdma_conn_rejected(struct rtrs_clt_con *con,
 	return -ECONNRESET;
 }
 
-static void rtrs_clt_close_conns(struct rtrs_clt_sess *sess, bool wait)
+void rtrs_clt_close_conns(struct rtrs_clt_sess *sess, bool wait)
 {
 	if (rtrs_clt_change_state_get_old(sess, RTRS_CLT_CLOSING, NULL))
 		queue_work(rtrs_wq, &sess->close_work);
@@ -2824,13 +2824,6 @@ int rtrs_clt_reconnect_from_sysfs(struct rtrs_clt_sess *sess)
 	return err;
 }
 
-int rtrs_clt_disconnect_from_sysfs(struct rtrs_clt_sess *sess)
-{
-	rtrs_clt_close_conns(sess, true);
-
-	return 0;
-}
-
 int rtrs_clt_remove_path_from_sysfs(struct rtrs_clt_sess *sess,
 				     const struct attribute *sysfs_self)
 {
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
index ef15927dfeda..0a0ea43d2b46 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
@@ -203,7 +203,7 @@ static inline struct rtrs_permit *get_permit(struct rtrs_clt *clt, int idx)
 }
 
 int rtrs_clt_reconnect_from_sysfs(struct rtrs_clt_sess *sess);
-int rtrs_clt_disconnect_from_sysfs(struct rtrs_clt_sess *sess);
+void rtrs_clt_close_conns(struct rtrs_clt_sess *sess, bool wait);
 int rtrs_clt_create_path_from_sysfs(struct rtrs_clt *clt,
 				     struct rtrs_addr *addr);
 int rtrs_clt_remove_path_from_sysfs(struct rtrs_clt_sess *sess,
-- 
2.25.1

[PATCHv2 for-next 10/19] RDMA/rtrs-srv: Kill __rtrs_srv_change_state

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

No need since the only user is rtrs_srv_change_state.

Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index afa63f06586b..54a29285240d 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -67,13 +67,13 @@ static inline struct rtrs_srv_sess *to_srv_sess(struct rtrs_sess *s)
 	return container_of(s, struct rtrs_srv_sess, s);
 }
 
-static bool __rtrs_srv_change_state(struct rtrs_srv_sess *sess,
-				     enum rtrs_srv_state new_state)
+static bool rtrs_srv_change_state(struct rtrs_srv_sess *sess,
+				  enum rtrs_srv_state new_state)
 {
 	enum rtrs_srv_state old_state;
 	bool changed = false;
 
-	lockdep_assert_held(&sess->state_lock);
+	spin_lock_irq(&sess->state_lock);
 	old_state = sess->state;
 	switch (new_state) {
 	case RTRS_SRV_CONNECTED:
@@ -94,17 +94,6 @@ static bool __rtrs_srv_change_state(struct rtrs_srv_sess *sess,
 	}
 	if (changed)
 		sess->state = new_state;
-
-	return changed;
-}
-
-static bool rtrs_srv_change_state(struct rtrs_srv_sess *sess,
-				   enum rtrs_srv_state new_state)
-{
-	bool changed;
-
-	spin_lock_irq(&sess->state_lock);
-	changed = __rtrs_srv_change_state(sess, new_state);
 	spin_unlock_irq(&sess->state_lock);
 
 	return changed;
-- 
2.25.1

[PATCHv2 for-next 11/19] RDMA/rtrs-clt: Remove redundant 'break'

[Gioh Kim]

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>

It is duplicated with the very next line

Signed-off-by: Guoqing Jiang <guoqing.jiang@ionos.com>
Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 1de72784e1c0..757a4394fdb6 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -655,7 +655,6 @@ static void rtrs_clt_rdma_done(struct ib_cq *cq, struct ib_wc *wc)
 			rtrs_err(con->c.sess, "rtrs_post_recv_empty(): %d\n",
 				  err);
 			rtrs_rdma_error_recovery(con);
-			break;
 		}
 		break;
 	case IB_WC_RECV:
-- 
2.25.1

[PATCHv2 for-next 12/19] RDMA/rtrs-clt: Check state of the rtrs_clt_sess before reading its stats

[Gioh Kim]

From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>

When get_next_path_min_inflight is called to select the next path, it
iterates over the list of available rtrs_clt_sess (paths). It then reads
the number of inflight IOs for that path to select one which has the least
inflight IO.

But it may so happen that rtrs_clt_sess (path) is no longer in the
connected state because closing or error recovery paths can change
the status of the rtrs_clt_Sess.

For example, the client sent the heart-beat and did not get the
response, it would change the session status and stop IO processing.
The added checking of this patch can prevent accessing the broken path
and generating duplicated error messages.

It is ok if the status is changed after checking the status because
the error recovery path does not free memory and only tries to
reconnection. And also it is ok if the session is closed after checking
the status because closing the session changes the session status and
flush all IO beforing free memory. If the session is being accessed for
IO processing, the closing session will wait.

Fixes: 6a98d71daea18 ("RDMA/rtrs: client: main functionality")
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 757a4394fdb6..a3d20a5c5112 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -813,6 +813,9 @@ static struct rtrs_clt_sess *get_next_path_min_inflight(struct path_it *it)
 	int inflight;
 
 	list_for_each_entry_rcu(sess, &clt->paths_list, s.entry) {
+		if (unlikely(READ_ONCE(sess->state) != RTRS_CLT_CONNECTED))
+			continue;
+
 		if (unlikely(!list_empty(raw_cpu_ptr(sess->mp_skip_entry))))
 			continue;
 
-- 
2.25.1

[PATCHv2 for-next 13/19] RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight

[Gioh Kim]

From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>

ids_inflight is used to track the inflight IOs. But the use of atomic_t
variable can cause performance drops and can also become a performance
bottleneck.

This commit replaces the use of atomic_t with a percpu_ref structure. The
advantage it offers is, it doesn't check if the reference has fallen to 0,
until the user explicitly signals it to; and that is done by the
percpu_ref_kill() function call. After that, the percpu_ref structure
behaves like an atomic_t and for every put call, checks whether the
reference has fallen to 0 or not.

rtrs_srv_stats_rdma_to_str shows the count of ids_inflight as 0
for user-mode tools not to be confused.

Fixes: 9cb837480424e ("RDMA/rtrs: server: main functionality")
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c | 12 +++---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 43 +++++++++++++-------
 drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  4 +-
 3 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
index e102b1368d0c..df1d7d6b1884 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
@@ -27,12 +27,10 @@ ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
 				    char *page, size_t len)
 {
 	struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
-	struct rtrs_srv_sess *sess = stats->sess;
 
-	return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
-			 (s64)atomic64_read(&r->dir[READ].cnt),
-			 (s64)atomic64_read(&r->dir[READ].size_total),
-			 (s64)atomic64_read(&r->dir[WRITE].cnt),
-			 (s64)atomic64_read(&r->dir[WRITE].size_total),
-			 atomic_read(&sess->ids_inflight));
+	return sysfs_emit(page, "%lld %lld %lld %lldn\n",
+			  (s64)atomic64_read(&r->dir[READ].cnt),
+			  (s64)atomic64_read(&r->dir[READ].size_total),
+			  (s64)atomic64_read(&r->dir[WRITE].cnt),
+			  (s64)atomic64_read(&r->dir[WRITE].size_total));
 }
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 54a29285240d..f66f2be9f519 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -111,7 +111,6 @@ static void rtrs_srv_free_ops_ids(struct rtrs_srv_sess *sess)
 	struct rtrs_srv *srv = sess->srv;
 	int i;
 
-	WARN_ON(atomic_read(&sess->ids_inflight));
 	if (sess->ops_ids) {
 		for (i = 0; i < srv->queue_depth; i++)
 			free_id(sess->ops_ids[i]);
@@ -126,11 +125,19 @@ static struct ib_cqe io_comp_cqe = {
 	.done = rtrs_srv_rdma_done
 };
 
+static inline void rtrs_srv_inflight_ref_release(struct percpu_ref *ref)
+{
+	struct rtrs_srv_sess *sess = container_of(ref, struct rtrs_srv_sess, ids_inflight_ref);
+
+	percpu_ref_exit(&sess->ids_inflight_ref);
+	complete(&sess->complete_done);
+}
+
 static int rtrs_srv_alloc_ops_ids(struct rtrs_srv_sess *sess)
 {
 	struct rtrs_srv *srv = sess->srv;
 	struct rtrs_srv_op *id;
-	int i;
+	int i, ret;
 
 	sess->ops_ids = kcalloc(srv->queue_depth, sizeof(*sess->ops_ids),
 				GFP_KERNEL);
@@ -144,8 +151,14 @@ static int rtrs_srv_alloc_ops_ids(struct rtrs_srv_sess *sess)
 
 		sess->ops_ids[i] = id;
 	}
-	init_waitqueue_head(&sess->ids_waitq);
-	atomic_set(&sess->ids_inflight, 0);
+
+	ret = percpu_ref_init(&sess->ids_inflight_ref,
+			      rtrs_srv_inflight_ref_release, 0, GFP_KERNEL);
+	if (ret) {
+		pr_err("Percpu reference init failed\n");
+		goto err;
+	}
+	init_completion(&sess->complete_done);
 
 	return 0;
 
@@ -156,21 +169,14 @@ static int rtrs_srv_alloc_ops_ids(struct rtrs_srv_sess *sess)
 
 static inline void rtrs_srv_get_ops_ids(struct rtrs_srv_sess *sess)
 {
-	atomic_inc(&sess->ids_inflight);
+	percpu_ref_get(&sess->ids_inflight_ref);
 }
 
 static inline void rtrs_srv_put_ops_ids(struct rtrs_srv_sess *sess)
 {
-	if (atomic_dec_and_test(&sess->ids_inflight))
-		wake_up(&sess->ids_waitq);
+	percpu_ref_put(&sess->ids_inflight_ref);
 }
 
-static void rtrs_srv_wait_ops_ids(struct rtrs_srv_sess *sess)
-{
-	wait_event(sess->ids_waitq, !atomic_read(&sess->ids_inflight));
-}
-
-
 static void rtrs_srv_reg_mr_done(struct ib_cq *cq, struct ib_wc *wc)
 {
 	struct rtrs_srv_con *con = to_srv_con(wc->qp->qp_context);
@@ -1479,8 +1485,15 @@ static void rtrs_srv_close_work(struct work_struct *work)
 		rdma_disconnect(con->c.cm_id);
 		ib_drain_qp(con->c.qp);
 	}
-	/* Wait for all inflights */
-	rtrs_srv_wait_ops_ids(sess);
+
+	/*
+	 * Degrade ref count to the usual model with a single shared
+	 * atomic_t counter
+	 */
+	percpu_ref_kill(&sess->ids_inflight_ref);
+
+	/* Wait for all completion */
+	wait_for_completion(&sess->complete_done);
 
 	/* Notify upper layer if we are the last path */
 	rtrs_srv_sess_down(sess);
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
index 9543ae19996c..f8da2e3f0bda 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
@@ -81,8 +81,8 @@ struct rtrs_srv_sess {
 	spinlock_t		state_lock;
 	int			cur_cq_vector;
 	struct rtrs_srv_op	**ops_ids;
-	atomic_t		ids_inflight;
-	wait_queue_head_t	ids_waitq;
+	struct percpu_ref       ids_inflight_ref;
+	struct completion       complete_done;
 	struct rtrs_srv_mr	*mrs;
 	unsigned int		mrs_num;
 	dma_addr_t		*dma_addr;
-- 
2.25.1

[PATCHv2 for-next 14/19] RDMA/rtrs: Do not reset hb_missed_max after re-connection

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

When re-connecting, it resets hb_missed_max to 0.
Before the first re-connecting, client will trigger re-connection
when it gets hb-ack more than 5 times. But after the first
re-connecting, clients will do re-connection whenever it does
not get hb-ack because hb_missed_max is 0.

There is no need to reset hb_missed_max when re-connecting.
hb_missed_max should be kept until closing the session.

Fixes: c0894b3ea69d3 ("RDMA/rtrs: core: lib functions shared between client and server modules")
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index a7847282a2eb..4e602e40f623 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -376,7 +376,6 @@ void rtrs_stop_hb(struct rtrs_sess *sess)
 {
 	cancel_delayed_work_sync(&sess->hb_dwork);
 	sess->hb_missed_cnt = 0;
-	sess->hb_missed_max = 0;
 }
 EXPORT_SYMBOL_GPL(rtrs_stop_hb);
 
-- 
2.25.1

[PATCHv2 for-next 15/19] RDMA/rtrs-srv: Duplicated session name is not allowed

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

If two clients try to use the same session name, rtrs-server generates a
kernel error that it failed to create the sysfs because the filename
is duplicated.

This patch adds code to check if there already exists the same session
name with the different UUID. If a client tries to add more session,
it sends the UUID and the session name. Therefore it is ok if there is
already same session name with the same UUID. The rtrs-server must fail
only-if there is the same session name with the different UUID.

Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Aleksei Marov <aleksei.marov@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 42 +++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index f66f2be9f519..0e1e303dcbee 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -753,7 +753,40 @@ static void rtrs_srv_sess_down(struct rtrs_srv_sess *sess)
 	mutex_unlock(&srv->paths_ev_mutex);
 }
 
+static bool exist_sessname(struct rtrs_srv_ctx *ctx,
+			   const char *sessname, const uuid_t *path_uuid)
+{
+	struct rtrs_srv *srv;
+	struct rtrs_srv_sess *sess;
+	bool found = false;
+
+	mutex_lock(&ctx->srv_mutex);
+	list_for_each_entry(srv, &ctx->srv_list, ctx_list) {
+		mutex_lock(&srv->paths_mutex);
+
+		/* when a client with same uuid and same sessname tried to add a path */
+		if (uuid_equal(&srv->paths_uuid, path_uuid)) {
+			mutex_unlock(&srv->paths_mutex);
+			continue;
+		}
+
+		list_for_each_entry(sess, &srv->paths_list, s.entry) {
+			if (strlen(sess->s.sessname) == strlen(sessname) &&
+			    !strcmp(sess->s.sessname, sessname)) {
+				found = true;
+				break;
+			}
+		}
+		mutex_unlock(&srv->paths_mutex);
+		if (found)
+			break;
+	}
+	mutex_unlock(&ctx->srv_mutex);
+	return found;
+}
+
 static int post_recv_sess(struct rtrs_srv_sess *sess);
+static int rtrs_rdma_do_reject(struct rdma_cm_id *cm_id, int errno);
 
 static int process_info_req(struct rtrs_srv_con *con,
 			    struct rtrs_msg_info_req *msg)
@@ -772,10 +805,17 @@ static int process_info_req(struct rtrs_srv_con *con,
 		rtrs_err(s, "post_recv_sess(), err: %d\n", err);
 		return err;
 	}
+
+	if (exist_sessname(sess->srv->ctx,
+			   msg->sessname, &sess->srv->paths_uuid)) {
+		rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
+		return -EPERM;
+	}
+	strscpy(sess->s.sessname, msg->sessname, sizeof(sess->s.sessname));
+
 	rwr = kcalloc(sess->mrs_num, sizeof(*rwr), GFP_KERNEL);
 	if (unlikely(!rwr))
 		return -ENOMEM;
-	strscpy(sess->s.sessname, msg->sessname, sizeof(sess->s.sessname));
 
 	tx_sz  = sizeof(*rsp);
 	tx_sz += sizeof(rsp->desc[0]) * sess->mrs_num;
-- 
2.25.1

[PATCHv2 for-next 16/19] RDMA/rtrs-srv: Fix memory leak of unfreed rtrs_srv_stats object

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

When closing a session, currently the rtrs_srv_stats object in the
closing session is freed by kobject release. But if it failed
to create a session by various reasons, it must free the rtrs_srv_stats
object directly because kobject is not created yet.

This problem is found by kmemleak as below:

1. One client machine maps /dev/nullb0 with session name 'bla':
root@test1:~# echo "sessname=bla path=ip:192.168.122.190 \
device_path=/dev/nullb0" > /sys/devices/virtual/rnbd-client/ctl/map_device

2. Another machine failed to create a session with the same name 'bla':
root@test2:~# echo "sessname=bla path=ip:192.168.122.190 \
device_path=/dev/nullb1" > /sys/devices/virtual/rnbd-client/ctl/map_device
-bash: echo: write error: Connection reset by peer

3. The kmemleak on server machine reported an error:
unreferenced object 0xffff888033cdc800 (size 128):
  comm "kworker/2:1", pid 83, jiffies 4295086585 (age 2508.680s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000a72903b2>] __alloc_sess+0x1d4/0x1250 [rtrs_server]
    [<00000000d1e5321e>] rtrs_srv_rdma_cm_handler+0xc31/0xde0 [rtrs_server]
    [<00000000bb2f6e7e>] cma_ib_req_handler+0xdc5/0x2b50 [rdma_cm]
    [<00000000e896235d>] cm_process_work+0x2d/0x100 [ib_cm]
    [<00000000b6866c5f>] cm_req_handler+0x11bc/0x1c40 [ib_cm]
    [<000000005f5dd9aa>] cm_work_handler+0xe65/0x3cf2 [ib_cm]
    [<00000000610151e7>] process_one_work+0x4bc/0x980
    [<00000000541e0f77>] worker_thread+0x78/0x5c0
    [<00000000423898ca>] kthread+0x191/0x1e0
    [<000000005a24b239>] ret_from_fork+0x3a/0x50

Fixes: 39c2d639ca183 ("RDMA/rtrs-srv: Set .release function for rtrs srv device during device init")
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 0e1e303dcbee..879e6dbca86d 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1503,6 +1503,7 @@ static void free_sess(struct rtrs_srv_sess *sess)
 		kobject_del(&sess->kobj);
 		kobject_put(&sess->kobj);
 	} else {
+		kfree(sess->stats);
 		kfree(sess);
 	}
 }
-- 
2.25.1

[PATCHv2 for-next 17/19] RDMA/rtrs-srv: Fix memory leak when having multiple sessions

[Gioh Kim]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

Gioh notice memory leak below
unreferenced object 0xffff8880acda2000 (size 2048):
  comm "kworker/4:1", pid 77, jiffies 4295062871 (age 1270.730s)
  hex dump (first 32 bytes):
    00 20 da ac 80 88 ff ff 00 20 da ac 80 88 ff ff  . ....... ......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000e85d85b5>] rtrs_srv_rdma_cm_handler+0x8e5/0xa90 [rtrs_server]
    [<00000000e31a988a>] cma_ib_req_handler+0xdc5/0x2b50 [rdma_cm]
    [<000000000eb02c5b>] cm_process_work+0x2d/0x100 [ib_cm]
    [<00000000e1650ca9>] cm_req_handler+0x11bc/0x1c40 [ib_cm]
    [<000000009c28818b>] cm_work_handler+0xe65/0x3cf2 [ib_cm]
    [<000000002b53eaa1>] process_one_work+0x4bc/0x980
    [<00000000da3499fb>] worker_thread+0x78/0x5c0
    [<00000000167127a4>] kthread+0x191/0x1e0
    [<0000000060802104>] ret_from_fork+0x3a/0x50
unreferenced object 0xffff88806d595d90 (size 8):
  comm "kworker/4:1H", pid 131, jiffies 4295062972 (age 1269.720s)
  hex dump (first 8 bytes):
    62 6c 61 00 6b 6b 6b a5                          bla.kkk.
  backtrace:
    [<000000004447d253>] kstrdup+0x2e/0x60
    [<0000000047259793>] kobject_set_name_vargs+0x2f/0xb0
    [<00000000c2ee3bc8>] dev_set_name+0xab/0xe0
    [<000000002b6bdfb1>] rtrs_srv_create_sess_files+0x260/0x290 [rtrs_server]
    [<0000000075d87bd7>] rtrs_srv_info_req_done+0x71b/0x960 [rtrs_server]
    [<00000000ccdf1bb5>] __ib_process_cq+0x94/0x100 [ib_core]
    [<00000000cbcb60cb>] ib_cq_poll_work+0x32/0xc0 [ib_core]
    [<000000002b53eaa1>] process_one_work+0x4bc/0x980
    [<00000000da3499fb>] worker_thread+0x78/0x5c0
    [<00000000167127a4>] kthread+0x191/0x1e0
    [<0000000060802104>] ret_from_fork+0x3a/0x50
unreferenced object 0xffff88806d6bb100 (size 256):
  comm "kworker/4:1H", pid 131, jiffies 4295062972 (age 1269.720s)
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff 00 59 4d 86 ff ff ff ff  .........YM.....
  backtrace:
    [<00000000a18a11e4>] device_add+0x74d/0xa00
    [<00000000a915b95f>] rtrs_srv_create_sess_files.cold+0x49/0x1fe [rtrs_server]
    [<0000000075d87bd7>] rtrs_srv_info_req_done+0x71b/0x960 [rtrs_server]
    [<00000000ccdf1bb5>] __ib_process_cq+0x94/0x100 [ib_core]
    [<00000000cbcb60cb>] ib_cq_poll_work+0x32/0xc0 [ib_core]
    [<000000002b53eaa1>] process_one_work+0x4bc/0x980
    [<00000000da3499fb>] worker_thread+0x78/0x5c0
    [<00000000167127a4>] kthread+0x191/0x1e0
    [<0000000060802104>] ret_from_fork+0x3a/0x50

The problem is we increase device refcount by get_device in
process_info_req for each path, but only does put_deice for last path,
which lead to memory leak.

To fix it, it also calls put_device when dev_ref is not 0.

Fixes: e2853c49477d1 ("RDMA/rtrs-srv-sysfs: fix missing put_device")
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
index a9288175fbb5..20efd44297fb 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
@@ -208,6 +208,7 @@ rtrs_srv_destroy_once_sysfs_root_folders(struct rtrs_srv_sess *sess)
 		device_del(&srv->dev);
 		put_device(&srv->dev);
 	} else {
+		put_device(&srv->dev);
 		mutex_unlock(&srv->paths_mutex);
 	}
 }
-- 
2.25.1

[PATCHv2 for-next 18/19] RDMA/rtrs-clt: Check if the queue_depth has changed during a reconnection

[Gioh Kim]

From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>

The queue_depth is a module parameter for rtrs_server. It is used on the
client side to determing the queue_depth of the request queue for the RNBD
virtual block device.

During a reconnection event for an already mapped device, in case the
rtrs_server module queue_depth has changed, fail the reconnect attempt.

Also stop further auto reconnection attempts. A manual reconnect via
sysfs has to be triggerred.

Fixes: 6a98d71daea18 ("RDMA/rtrs: client: main functionality")
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index a3d20a5c5112..1bfa3f86c430 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -1770,7 +1770,20 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
 				  queue_depth);
 			return -ECONNRESET;
 		}
-		if (!sess->rbufs || sess->queue_depth < queue_depth) {
+
+		if (sess->queue_depth > 0 && queue_depth != sess->queue_depth) {
+			rtrs_err(clt, "Error: queue depth changed\n");
+
+			/*
+			 * Stop any more reconnection attempts
+			 */
+			sess->reconnect_attempts = -1;
+			rtrs_err(clt,
+				"Disabling auto-reconnect. Trigger a manual reconnect after issue is resolved\n");
+			return -ECONNRESET;
+		}
+
+		if (!sess->rbufs) {
 			kfree(sess->rbufs);
 			sess->rbufs = kcalloc(queue_depth, sizeof(*sess->rbufs),
 					      GFP_KERNEL);
@@ -1784,7 +1797,7 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
 		sess->chunk_size = sess->max_io_size + sess->max_hdr_size;
 
 		/*
-		 * Global queue depth and IO size is always a minimum.
+		 * Global IO size is always a minimum.
 		 * If while a reconnection server sends us a value a bit
 		 * higher - client does not care and uses cached minimum.
 		 *
@@ -1792,8 +1805,7 @@ static int rtrs_rdma_conn_established(struct rtrs_clt_con *con,
 		 * connections in parallel, use lock.
 		 */
 		mutex_lock(&clt->paths_mutex);
-		clt->queue_depth = min_not_zero(sess->queue_depth,
-						clt->queue_depth);
+		clt->queue_depth = sess->queue_depth;
 		clt->max_io_size = min_not_zero(sess->max_io_size,
 						clt->max_io_size);
 		mutex_unlock(&clt->paths_mutex);
-- 
2.25.1

[PATCHv2 for-next 19/19] RDMA/rtrs-clt: Fix memory leak of not-freed sess->stats and stats->pcpu_stats

[Gioh Kim]

From: Gioh Kim <gi-oh.kim@cloud.ionos.com>

sess->stats and sess->stats->pcpu_stats objects are freed
when sysfs entry is removed. If something wrong happens and
session is closed before sysfs entry is created,
sess->stats and sess->stats->pcpu_stats objects are not freed.

This patch adds freeing of them at three places:
1. When client uses wrong address and session creation fails.
2. When client fails to create a sysfs entry.
3. When client adds wrong address via sysfs add_path.

Fixes: 215378b838df0 ("RDMA/rtrs: client: sysfs interface functions")
Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index 1bfa3f86c430..2323c9ddae97 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2759,6 +2759,8 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
 		if (err) {
 			list_del_rcu(&sess->s.entry);
 			rtrs_clt_close_conns(sess, true);
+			free_percpu(sess->stats->pcpu_stats);
+			kfree(sess->stats);
 			free_sess(sess);
 			goto close_all_sess;
 		}
@@ -2767,6 +2769,8 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
 		if (err) {
 			list_del_rcu(&sess->s.entry);
 			rtrs_clt_close_conns(sess, true);
+			free_percpu(sess->stats->pcpu_stats);
+			kfree(sess->stats);
 			free_sess(sess);
 			goto close_all_sess;
 		}
@@ -3017,6 +3021,8 @@ int rtrs_clt_create_path_from_sysfs(struct rtrs_clt *clt,
 close_sess:
 	rtrs_clt_remove_path_from_arr(sess);
 	rtrs_clt_close_conns(sess, true);
+	free_percpu(sess->stats->pcpu_stats);
+	kfree(sess->stats);
 	free_sess(sess);
 
 	return err;
-- 
2.25.1

Re: [PATCHv2 for-next 13/19] RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight

[Jason Gunthorpe]

On Mon, May 17, 2021 at 11:18:37AM +0200, Gioh Kim wrote:
> From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> 
> ids_inflight is used to track the inflight IOs. But the use of atomic_t
> variable can cause performance drops and can also become a performance
> bottleneck.
> 
> This commit replaces the use of atomic_t with a percpu_ref structure. The
> advantage it offers is, it doesn't check if the reference has fallen to 0,
> until the user explicitly signals it to; and that is done by the
> percpu_ref_kill() function call. After that, the percpu_ref structure
> behaves like an atomic_t and for every put call, checks whether the
> reference has fallen to 0 or not.
> 
> rtrs_srv_stats_rdma_to_str shows the count of ids_inflight as 0
> for user-mode tools not to be confused.
> 
> Fixes: 9cb837480424e ("RDMA/rtrs: server: main functionality")
> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c | 12 +++---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 43 +++++++++++++-------
>  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  4 +-
>  3 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> index e102b1368d0c..df1d7d6b1884 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> @@ -27,12 +27,10 @@ ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
>  				    char *page, size_t len)
>  {
>  	struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> -	struct rtrs_srv_sess *sess = stats->sess;
>  
> -	return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
> -			 (s64)atomic64_read(&r->dir[READ].cnt),
> -			 (s64)atomic64_read(&r->dir[READ].size_total),
> -			 (s64)atomic64_read(&r->dir[WRITE].cnt),
> -			 (s64)atomic64_read(&r->dir[WRITE].size_total),
> -			 atomic_read(&sess->ids_inflight));
> +	return sysfs_emit(page, "%lld %lld %lld %lldn\n",
> +			  (s64)atomic64_read(&r->dir[READ].cnt),
> +			  (s64)atomic64_read(&r->dir[READ].size_total),
> +			  (s64)atomic64_read(&r->dir[WRITE].cnt),
> +			  (s64)atomic64_read(&r->dir[WRITE].size_total));
>  }

This seems like an unrelated hunk

Jason

Re: [PATCHv2 for-next 03/19] RDMA/rtrs-srv: Add error messages for cases when failing RDMA connection

[Jason Gunthorpe]

On Mon, May 17, 2021 at 11:18:27AM +0200, Gioh Kim wrote:
> From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> 
> It was difficult to find out why it failed to establish RDMA
> connection. This patch adds some messages to show which function
> has failed why.
> 
> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 3d09d01e34b4..df17dd4c1e28 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
>  	 * If this request is not the first connection request from the
>  	 * client for this session then fail and return error.
>  	 */
> -	if (!first_conn)
> +	if (!first_conn) {
> +		pr_err("Error: Not the first connection request for this session\n");
>  		return ERR_PTR(-ENXIO);

You really shouldn't be printing based on attacker controlled data..

Jason

Re: [PATCHv2 for-next 13/19] RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight

[Haris Iqbal]

On Tue, May 25, 2021 at 10:15 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, May 17, 2021 at 11:18:37AM +0200, Gioh Kim wrote:
> > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> >
> > ids_inflight is used to track the inflight IOs. But the use of atomic_t
> > variable can cause performance drops and can also become a performance
> > bottleneck.
> >
> > This commit replaces the use of atomic_t with a percpu_ref structure. The
> > advantage it offers is, it doesn't check if the reference has fallen to 0,
> > until the user explicitly signals it to; and that is done by the
> > percpu_ref_kill() function call. After that, the percpu_ref structure
> > behaves like an atomic_t and for every put call, checks whether the
> > reference has fallen to 0 or not.
> >
> > rtrs_srv_stats_rdma_to_str shows the count of ids_inflight as 0
> > for user-mode tools not to be confused.
> >
> > Fixes: 9cb837480424e ("RDMA/rtrs: server: main functionality")
> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c | 12 +++---
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 43 +++++++++++++-------
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  4 +-
> >  3 files changed, 35 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > index e102b1368d0c..df1d7d6b1884 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > @@ -27,12 +27,10 @@ ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
> >                                   char *page, size_t len)
> >  {
> >       struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> > -     struct rtrs_srv_sess *sess = stats->sess;
> >
> > -     return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
> > -                      (s64)atomic64_read(&r->dir[READ].cnt),
> > -                      (s64)atomic64_read(&r->dir[READ].size_total),
> > -                      (s64)atomic64_read(&r->dir[WRITE].cnt),
> > -                      (s64)atomic64_read(&r->dir[WRITE].size_total),
> > -                      atomic_read(&sess->ids_inflight));
> > +     return sysfs_emit(page, "%lld %lld %lld %lldn\n",
> > +                       (s64)atomic64_read(&r->dir[READ].cnt),
> > +                       (s64)atomic64_read(&r->dir[READ].size_total),
> > +                       (s64)atomic64_read(&r->dir[WRITE].cnt),
> > +                       (s64)atomic64_read(&r->dir[WRITE].size_total));
> >  }
>
> This seems like an unrelated hunk

Previous to the commit, ids_inflight was an atomic variable, and hence
was read and printed along with other rtrs_srv stats. With this
commit, since ids_inflight is changed to percpu_ref, we removed it
being printed in rtrs_srv stats. Its related to the commit.


>
> Jason

Re: [PATCHv2 for-next 03/19] RDMA/rtrs-srv: Add error messages for cases when failing RDMA connection

[Haris Iqbal]

On Tue, May 25, 2021 at 10:18 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, May 17, 2021 at 11:18:27AM +0200, Gioh Kim wrote:
> > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> >
> > It was difficult to find out why it failed to establish RDMA
> > connection. This patch adds some messages to show which function
> > has failed why.
> >
> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > index 3d09d01e34b4..df17dd4c1e28 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
> >        * If this request is not the first connection request from the
> >        * client for this session then fail and return error.
> >        */
> > -     if (!first_conn)
> > +     if (!first_conn) {
> > +             pr_err("Error: Not the first connection request for this session\n");
> >               return ERR_PTR(-ENXIO);
>
> You really shouldn't be printing based on attacker controlled data..

I want to make sure I understand correctly. Did you mean that an
attacker can bombard a server with such connection request, which can
lead to uncontrolled prints, and possibly DOS?

If so, would a ratelimited print be better?


>
> Jason

Re: [PATCHv2 for-next 13/19] RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight

[Jinpu Wang]

On Wed, May 26, 2021 at 11:17 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>
> On Tue, May 25, 2021 at 10:15 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Mon, May 17, 2021 at 11:18:37AM +0200, Gioh Kim wrote:
> > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > >
> > > ids_inflight is used to track the inflight IOs. But the use of atomic_t
> > > variable can cause performance drops and can also become a performance
> > > bottleneck.
> > >
> > > This commit replaces the use of atomic_t with a percpu_ref structure. The
> > > advantage it offers is, it doesn't check if the reference has fallen to 0,
> > > until the user explicitly signals it to; and that is done by the
> > > percpu_ref_kill() function call. After that, the percpu_ref structure
> > > behaves like an atomic_t and for every put call, checks whether the
> > > reference has fallen to 0 or not.
> > >
> > > rtrs_srv_stats_rdma_to_str shows the count of ids_inflight as 0
> > > for user-mode tools not to be confused.
> > >
> > > Fixes: 9cb837480424e ("RDMA/rtrs: server: main functionality")
> > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > ---
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c | 12 +++---
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 43 +++++++++++++-------
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  4 +-
> > >  3 files changed, 35 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > index e102b1368d0c..df1d7d6b1884 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > @@ -27,12 +27,10 @@ ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
> > >                                   char *page, size_t len)
> > >  {
> > >       struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> > > -     struct rtrs_srv_sess *sess = stats->sess;
> > >
> > > -     return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
> > > -                      (s64)atomic64_read(&r->dir[READ].cnt),
> > > -                      (s64)atomic64_read(&r->dir[READ].size_total),
> > > -                      (s64)atomic64_read(&r->dir[WRITE].cnt),
> > > -                      (s64)atomic64_read(&r->dir[WRITE].size_total),
> > > -                      atomic_read(&sess->ids_inflight));
> > > +     return sysfs_emit(page, "%lld %lld %lld %lldn\n",
> > > +                       (s64)atomic64_read(&r->dir[READ].cnt),
> > > +                       (s64)atomic64_read(&r->dir[READ].size_total),
> > > +                       (s64)atomic64_read(&r->dir[WRITE].cnt),
> > > +                       (s64)atomic64_read(&r->dir[WRITE].size_total));
> > >  }
> >
> > This seems like an unrelated hunk
>
> Previous to the commit, ids_inflight was an atomic variable, and hence
> was read and printed along with other rtrs_srv stats. With this
> commit, since ids_inflight is changed to percpu_ref, we removed it
> being printed in rtrs_srv stats. Its related to the commit.
I think Jason meant the sysfs_emit conversion should be in a separate patch.
>
>
> >
> > Jason

Re: [PATCHv2 for-next 03/19] RDMA/rtrs-srv: Add error messages for cases when failing RDMA connection

[Jason Gunthorpe]

On Wed, May 26, 2021 at 11:28:41AM +0200, Haris Iqbal wrote:
> On Tue, May 25, 2021 at 10:18 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Mon, May 17, 2021 at 11:18:27AM +0200, Gioh Kim wrote:
> > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > >
> > > It was difficult to find out why it failed to establish RDMA
> > > connection. This patch adds some messages to show which function
> > > has failed why.
> > >
> > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > index 3d09d01e34b4..df17dd4c1e28 100644
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
> > >        * If this request is not the first connection request from the
> > >        * client for this session then fail and return error.
> > >        */
> > > -     if (!first_conn)
> > > +     if (!first_conn) {
> > > +             pr_err("Error: Not the first connection request for this session\n");
> > >               return ERR_PTR(-ENXIO);
> >
> > You really shouldn't be printing based on attacker controlled data..
> 
> I want to make sure I understand correctly. Did you mean that an
> attacker can bombard a server with such connection request, which can
> lead to uncontrolled prints, and possibly DOS?

Yes

> If so, would a ratelimited print be better?

Probably

Jason

Re: [PATCHv2 for-next 13/19] RDMA/rtrs-srv: Replace atomic_t with percpu_ref for ids_inflight

[Haris Iqbal]

On Wed, May 26, 2021 at 11:38 AM Jinpu Wang <jinpu.wang@ionos.com> wrote:
>
> On Wed, May 26, 2021 at 11:17 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
> >
> > On Tue, May 25, 2021 at 10:15 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Mon, May 17, 2021 at 11:18:37AM +0200, Gioh Kim wrote:
> > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > > >
> > > > ids_inflight is used to track the inflight IOs. But the use of atomic_t
> > > > variable can cause performance drops and can also become a performance
> > > > bottleneck.
> > > >
> > > > This commit replaces the use of atomic_t with a percpu_ref structure. The
> > > > advantage it offers is, it doesn't check if the reference has fallen to 0,
> > > > until the user explicitly signals it to; and that is done by the
> > > > percpu_ref_kill() function call. After that, the percpu_ref structure
> > > > behaves like an atomic_t and for every put call, checks whether the
> > > > reference has fallen to 0 or not.
> > > >
> > > > rtrs_srv_stats_rdma_to_str shows the count of ids_inflight as 0
> > > > for user-mode tools not to be confused.
> > > >
> > > > Fixes: 9cb837480424e ("RDMA/rtrs: server: main functionality")
> > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > ---
> > > >  drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c | 12 +++---
> > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c       | 43 +++++++++++++-------
> > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  4 +-
> > > >  3 files changed, 35 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > > index e102b1368d0c..df1d7d6b1884 100644
> > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
> > > > @@ -27,12 +27,10 @@ ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
> > > >                                   char *page, size_t len)
> > > >  {
> > > >       struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> > > > -     struct rtrs_srv_sess *sess = stats->sess;
> > > >
> > > > -     return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
> > > > -                      (s64)atomic64_read(&r->dir[READ].cnt),
> > > > -                      (s64)atomic64_read(&r->dir[READ].size_total),
> > > > -                      (s64)atomic64_read(&r->dir[WRITE].cnt),
> > > > -                      (s64)atomic64_read(&r->dir[WRITE].size_total),
> > > > -                      atomic_read(&sess->ids_inflight));
> > > > +     return sysfs_emit(page, "%lld %lld %lld %lldn\n",
> > > > +                       (s64)atomic64_read(&r->dir[READ].cnt),
> > > > +                       (s64)atomic64_read(&r->dir[READ].size_total),
> > > > +                       (s64)atomic64_read(&r->dir[WRITE].cnt),
> > > > +                       (s64)atomic64_read(&r->dir[WRITE].size_total));
> > > >  }
> > >
> > > This seems like an unrelated hunk
> >
> > Previous to the commit, ids_inflight was an atomic variable, and hence
> > was read and printed along with other rtrs_srv stats. With this
> > commit, since ids_inflight is changed to percpu_ref, we removed it
> > being printed in rtrs_srv stats. Its related to the commit.
> I think Jason meant the sysfs_emit conversion should be in a separate patch.

Okay. I missed that.

Thanks; will send the updated patches.

> >
> >
> > >
> > > Jason

Re: [PATCHv2 for-next 03/19] RDMA/rtrs-srv: Add error messages for cases when failing RDMA connection

[Haris Iqbal]

On Wed, May 26, 2021 at 6:07 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, May 26, 2021 at 11:28:41AM +0200, Haris Iqbal wrote:
> > On Tue, May 25, 2021 at 10:18 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Mon, May 17, 2021 at 11:18:27AM +0200, Gioh Kim wrote:
> > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > > >
> > > > It was difficult to find out why it failed to establish RDMA
> > > > connection. This patch adds some messages to show which function
> > > > has failed why.
> > > >
> > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > index 3d09d01e34b4..df17dd4c1e28 100644
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
> > > >        * If this request is not the first connection request from the
> > > >        * client for this session then fail and return error.
> > > >        */
> > > > -     if (!first_conn)
> > > > +     if (!first_conn) {
> > > > +             pr_err("Error: Not the first connection request for this session\n");
> > > >               return ERR_PTR(-ENXIO);
> > >
> > > You really shouldn't be printing based on attacker controlled data..
> >
> > I want to make sure I understand correctly. Did you mean that an
> > attacker can bombard a server with such connection request, which can
> > lead to uncontrolled prints, and possibly DOS?
>
> Yes
>
> > If so, would a ratelimited print be better?
>
> Probably

Alright.. Will change this print to a ratelimited one, and send the
updates patches.

Thanks.

>
> Jason