[PATCH for-next 0/7] Misc update for RTRS

[PATCH for-next 0/7] Misc update for RTRS

[Md Haris Iqbal]

Hi Jason, hi Doug,

Please consider to include following changes to the next merge window.

The patchset is orgnized as:
- patch1 sysfs_emit conversion.
- patch2 remove len parameter.
- patch3 Fix warning with poll mode.
- patch4 Replace duplicate check.
- patch5 Introduce helper function
- patch6 Disallow special characters.
- patch7 One entry one value for sysfs

Jack Wang (2):
  RDMA/rtrs: Fix warning when use poll mode on client side.
  RDMA/rtrs: Replace duplicate check with is_pollqueue helper

Md Haris Iqbal (5):
  RDMA/rtrs: Use sysfs_emit instead of s*printf function for sysfs show
  RDMA/rtrs: Remove len parameter from helper print functions of sysfs
  RDMA/rtrs: Introduce destroy_cq helper
  RDMA/rtrs: Do not allow sessname to contain special symbols / and .
  RDMA/rtrs-clt: Follow "one entry one value" rule for IO migration
    stats

 drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c | 49 +++++++++++---------
 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c | 11 +++--
 drivers/infiniband/ulp/rtrs/rtrs-clt.c       |  6 +++
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       | 13 +++---
 drivers/infiniband/ulp/rtrs/rtrs-pri.h       |  2 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c |  3 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  2 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv.c       |  6 +++
 drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  3 +-
 drivers/infiniband/ulp/rtrs/rtrs.c           | 31 ++++++++++---
 10 files changed, 80 insertions(+), 46 deletions(-)

-- 
2.25.1

[PATCH for-next 1/7] RDMA/rtrs: Use sysfs_emit instead of s*printf function for sysfs show

[Md Haris Iqbal]

sysfs_emit function was added to be aware of the PAGE_SIZE maximum of
the temporary buffer used for outputting sysfs content, so there is no
possible overruns. So replace the uses of any s*printf functions for
the sysfs show functions with sysfs_emit.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c | 24 +++++++++-----------
 drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c |  2 +-
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
index 5e780bdd763d..9c27f21ec040 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
@@ -45,24 +45,23 @@ int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats,
 	size_t used;
 	int cpu;
 
-	used = scnprintf(buf, len, "    ");
+	used = sysfs_emit(buf, "    ");
 	for_each_possible_cpu(cpu)
-		used += scnprintf(buf + used, len - used, " CPU%u", cpu);
+		used += sysfs_emit_at(buf, used, " CPU%u", cpu);
 
-	used += scnprintf(buf + used, len - used, "\nfrom:");
+	used += sysfs_emit_at(buf, used, "\nfrom:");
 	for_each_possible_cpu(cpu) {
 		s = per_cpu_ptr(stats->pcpu_stats, cpu);
-		used += scnprintf(buf + used, len - used, " %d",
+		used += sysfs_emit_at(buf, used, " %d",
 				  atomic_read(&s->cpu_migr.from));
 	}
 
-	used += scnprintf(buf + used, len - used, "\nto  :");
+	used += sysfs_emit_at(buf, used, "\nto  :");
 	for_each_possible_cpu(cpu) {
 		s = per_cpu_ptr(stats->pcpu_stats, cpu);
-		used += scnprintf(buf + used, len - used, " %d",
-				  s->cpu_migr.to);
+		used += sysfs_emit_at(buf, used, " %d", s->cpu_migr.to);
 	}
-	used += scnprintf(buf + used, len - used, "\n");
+	used += sysfs_emit_at(buf, used, "\n");
 
 	return used;
 }
@@ -70,9 +69,8 @@ int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats,
 int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf,
 				      size_t len)
 {
-	return scnprintf(buf, len, "%d %d\n",
-			 stats->reconnects.successful_cnt,
-			 stats->reconnects.fail_cnt);
+	return sysfs_emit(buf, "%d %d\n", stats->reconnects.successful_cnt,
+			  stats->reconnects.fail_cnt);
 }
 
 ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
@@ -94,7 +92,7 @@ ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
 		sum.failover_cnt	  += r->failover_cnt;
 	}
 
-	return scnprintf(page, len, "%llu %llu %llu %llu %u %llu\n",
+	return sysfs_emit(page, "%llu %llu %llu %llu %u %llu\n",
 			 sum.dir[READ].cnt, sum.dir[READ].size_total,
 			 sum.dir[WRITE].cnt, sum.dir[WRITE].size_total,
 			 atomic_read(&stats->inflight), sum.failover_cnt);
@@ -103,7 +101,7 @@ ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
 ssize_t rtrs_clt_reset_all_help(struct rtrs_clt_stats *s,
 				 char *page, size_t len)
 {
-	return scnprintf(page, len, "echo 1 to reset all statistics\n");
+	return sysfs_emit(page, "echo 1 to reset all statistics\n");
 }
 
 int rtrs_clt_reset_rdma_stats(struct rtrs_clt_stats *stats, bool enable)
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
index 20efd44297fb..9c43ce5ba1c1 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-sysfs.c
@@ -102,7 +102,7 @@ static ssize_t rtrs_srv_src_addr_show(struct kobject *kobj,
 	sess = container_of(kobj, struct rtrs_srv_sess, kobj);
 	cnt = sockaddr_to_str((struct sockaddr *)&sess->s.dst_addr,
 			      page, PAGE_SIZE);
-	return cnt + scnprintf(page + cnt, PAGE_SIZE - cnt, "\n");
+	return cnt + sysfs_emit_at(page, cnt, "\n");
 }
 
 static struct kobj_attribute rtrs_srv_src_addr_attr =
-- 
2.25.1

[PATCH for-next 2/7] RDMA/rtrs: Remove len parameter from helper print functions of sysfs

[Md Haris Iqbal]

Since we have changed all sysfs show functions to use sysfs_emit, we do
not require the len (PAGE_SIZE) in our helper print functions. So remove
it from the function parameter.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c | 12 ++++--------
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       | 12 +++++-------
 drivers/infiniband/ulp/rtrs/rtrs-pri.h       |  2 +-
 drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c |  3 +--
 drivers/infiniband/ulp/rtrs/rtrs-srv.h       |  3 +--
 5 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
index 9c27f21ec040..61d5e0018392 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
@@ -37,8 +37,7 @@ void rtrs_clt_inc_failover_cnt(struct rtrs_clt_stats *stats)
 	s->rdma.failover_cnt++;
 }
 
-int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats,
-					 char *buf, size_t len)
+int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats, char *buf)
 {
 	struct rtrs_clt_stats_pcpu *s;
 
@@ -66,15 +65,13 @@ int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats,
 	return used;
 }
 
-int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf,
-				      size_t len)
+int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf)
 {
 	return sysfs_emit(buf, "%d %d\n", stats->reconnects.successful_cnt,
 			  stats->reconnects.fail_cnt);
 }
 
-ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
-				    char *page, size_t len)
+ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats, char *page)
 {
 	struct rtrs_clt_stats_rdma sum;
 	struct rtrs_clt_stats_rdma *r;
@@ -98,8 +95,7 @@ ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
 			 atomic_read(&stats->inflight), sum.failover_cnt);
 }
 
-ssize_t rtrs_clt_reset_all_help(struct rtrs_clt_stats *s,
-				 char *page, size_t len)
+ssize_t rtrs_clt_reset_all_help(struct rtrs_clt_stats *s, char *page)
 {
 	return sysfs_emit(page, "echo 1 to reset all statistics\n");
 }
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
index 9dc819885ec7..6d81aae53df4 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
@@ -224,19 +224,17 @@ void rtrs_clt_update_all_stats(struct rtrs_clt_io_req *req, int dir);
 int rtrs_clt_reset_rdma_lat_distr_stats(struct rtrs_clt_stats *stats,
 					 bool enable);
 ssize_t rtrs_clt_stats_rdma_lat_distr_to_str(struct rtrs_clt_stats *stats,
-					      char *page, size_t len);
+					      char *page);
 int rtrs_clt_reset_cpu_migr_stats(struct rtrs_clt_stats *stats, bool enable);
-int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats, char *buf,
-					 size_t len);
+int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats, char *buf);
 int rtrs_clt_reset_reconnects_stat(struct rtrs_clt_stats *stats, bool enable);
-int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf,
-				     size_t len);
+int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf);
 int rtrs_clt_reset_rdma_stats(struct rtrs_clt_stats *stats, bool enable);
 ssize_t rtrs_clt_stats_rdma_to_str(struct rtrs_clt_stats *stats,
-				    char *page, size_t len);
+				    char *page);
 int rtrs_clt_reset_all_stats(struct rtrs_clt_stats *stats, bool enable);
 ssize_t rtrs_clt_reset_all_help(struct rtrs_clt_stats *stats,
-				 char *page, size_t len);
+				 char *page);
 
 /* rtrs-clt-sysfs.c */
 
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
index d12ddfa50747..78eac9a4f703 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
@@ -398,7 +398,7 @@ static ssize_t get_value##_show(struct kobject *kobj,			\
 {									\
 	type *stats = container_of(kobj, type, kobj_stats);		\
 									\
-	return print(stats, page, PAGE_SIZE);			\
+	return print(stats, page);			\
 }
 
 #define STAT_ATTR(type, stat, print, reset)				\
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
index 12c374b5eb6e..44b1c1652131 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv-stats.c
@@ -23,8 +23,7 @@ int rtrs_srv_reset_rdma_stats(struct rtrs_srv_stats *stats, bool enable)
 	return -EINVAL;
 }
 
-ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
-				    char *page, size_t len)
+ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats, char *page)
 {
 	struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
 
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
index 9d8d2a91a235..7d403c12faf3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
@@ -136,8 +136,7 @@ static inline void rtrs_srv_update_rdma_stats(struct rtrs_srv_stats *s,
 
 /* functions which are implemented in rtrs-srv-stats.c */
 int rtrs_srv_reset_rdma_stats(struct rtrs_srv_stats *stats, bool enable);
-ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
-				    char *page, size_t len);
+ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats, char *page);
 int rtrs_srv_reset_all_stats(struct rtrs_srv_stats *stats, bool enable);
 ssize_t rtrs_srv_reset_all_help(struct rtrs_srv_stats *stats,
 				 char *page, size_t len);
-- 
2.25.1

[PATCH for-next 3/7] RDMA/rtrs: Fix warning when use poll mode on client side.

[Md Haris Iqbal]

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

when test with poll mode, it will fail and lead to warning below on
client side:
echo "sessname=bla path=gid:fe80::2:c903:4e:d0b3@gid:fe80::2:c903:8:ca17
device_path=/dev/nullb2 nr_poll_queues=-1" |
sudo tee /sys/devices/virtual/rnbd-client/ctl/map_device

rnbd_client L597: Mapping device /dev/nullb2 on session bla,
(access_mode: rw, nr_poll_queues: 8)
WARNING: CPU: 3 PID: 9886 at drivers/infiniband/core/cq.c:447 ib_cq_pool_get+0x26f/0x2a0 [ib_core]

The problem is in case of poll queue, we need to still call
ib_alloc_cq/ib_free_cq, we can't use cq_poll api for poll queue.

As both client and server use shared function from rtrs, set irq_con_num
to con_num on server side, which is number of total connection of the
session, this way we can differ if the rtrs_con requires pollqueue.

Following up patches will replace the duplicate code with helpers.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c |  1 +
 drivers/infiniband/ulp/rtrs/rtrs.c     | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 716ef7b23558..078a1cbac90c 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1766,6 +1766,7 @@ static struct rtrs_srv_sess *__alloc_sess(struct rtrs_srv *srv,
 	strscpy(sess->s.sessname, str, sizeof(sess->s.sessname));
 
 	sess->s.con_num = con_num;
+	sess->s.irq_con_num = con_num;
 	sess->s.recon_cnt = recon_cnt;
 	uuid_copy(&sess->s.uuid, uuid);
 	spin_lock_init(&sess->state_lock);
diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index ca542e477d38..9bc323490ce3 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -228,7 +228,12 @@ static int create_cq(struct rtrs_con *con, int cq_vector, int nr_cqe,
 	struct rdma_cm_id *cm_id = con->cm_id;
 	struct ib_cq *cq;
 
-	cq = ib_cq_pool_get(cm_id->device, nr_cqe, cq_vector, poll_ctx);
+	if (con->cid >= con->sess->irq_con_num)
+		cq = ib_alloc_cq(cm_id->device, con, nr_cqe, cq_vector,
+				 poll_ctx);
+	else
+		cq = ib_cq_pool_get(cm_id->device, nr_cqe, cq_vector, poll_ctx);
+
 	if (IS_ERR(cq)) {
 		rtrs_err(con->sess, "Creating completion queue failed, errno: %ld\n",
 			  PTR_ERR(cq));
@@ -283,7 +288,10 @@ int rtrs_cq_qp_create(struct rtrs_sess *sess, struct rtrs_con *con,
 	err = create_qp(con, sess->dev->ib_pd, max_send_wr, max_recv_wr,
 			max_send_sge);
 	if (err) {
-		ib_cq_pool_put(con->cq, con->nr_cqe);
+		if (con->cid >= con->sess->irq_con_num)
+			ib_free_cq(con->cq);
+		else
+			ib_cq_pool_put(con->cq, con->nr_cqe);
 		con->cq = NULL;
 		return err;
 	}
@@ -300,7 +308,10 @@ void rtrs_cq_qp_destroy(struct rtrs_con *con)
 		con->qp = NULL;
 	}
 	if (con->cq) {
-		ib_cq_pool_put(con->cq, con->nr_cqe);
+		if (con->cid >= con->sess->irq_con_num)
+			ib_free_cq(con->cq);
+		else
+			ib_cq_pool_put(con->cq, con->nr_cqe);
 		con->cq = NULL;
 	}
 }
-- 
2.25.1

[PATCH for-next 4/7] RDMA/rtrs: Replace duplicate check with is_pollqueue helper

[Md Haris Iqbal]

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

if (con->cid >= con->sess->irq_con_num) check can be replaced with
a is_pollqueue helper.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index 9bc323490ce3..ac83cd97f838 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -222,13 +222,18 @@ static void qp_event_handler(struct ib_event *ev, void *ctx)
 	}
 }
 
+static bool is_pollqueue(struct rtrs_con *con)
+{
+	return con->cid >= con->sess->irq_con_num;
+}
+
 static int create_cq(struct rtrs_con *con, int cq_vector, int nr_cqe,
 		     enum ib_poll_context poll_ctx)
 {
 	struct rdma_cm_id *cm_id = con->cm_id;
 	struct ib_cq *cq;
 
-	if (con->cid >= con->sess->irq_con_num)
+	if (is_pollqueue(con))
 		cq = ib_alloc_cq(cm_id->device, con, nr_cqe, cq_vector,
 				 poll_ctx);
 	else
@@ -288,7 +293,7 @@ int rtrs_cq_qp_create(struct rtrs_sess *sess, struct rtrs_con *con,
 	err = create_qp(con, sess->dev->ib_pd, max_send_wr, max_recv_wr,
 			max_send_sge);
 	if (err) {
-		if (con->cid >= con->sess->irq_con_num)
+		if (is_pollqueue(con))
 			ib_free_cq(con->cq);
 		else
 			ib_cq_pool_put(con->cq, con->nr_cqe);
@@ -308,7 +313,7 @@ void rtrs_cq_qp_destroy(struct rtrs_con *con)
 		con->qp = NULL;
 	}
 	if (con->cq) {
-		if (con->cid >= con->sess->irq_con_num)
+		if (is_pollqueue(con))
 			ib_free_cq(con->cq);
 		else
 			ib_cq_pool_put(con->cq, con->nr_cqe);
-- 
2.25.1

[PATCH for-next 5/7] RDMA/rtrs: Introduce destroy_cq helper

[Md Haris Iqbal]

The same code snip used twice, to avoid duplicate, replace
it with a destroy_cq helper.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c b/drivers/infiniband/ulp/rtrs/rtrs.c
index ac83cd97f838..37952c8e768c 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs.c
@@ -279,6 +279,17 @@ static int create_qp(struct rtrs_con *con, struct ib_pd *pd,
 	return ret;
 }
 
+static void destroy_cq(struct rtrs_con *con)
+{
+	if (con->cq) {
+		if (is_pollqueue(con))
+			ib_free_cq(con->cq);
+		else
+			ib_cq_pool_put(con->cq, con->nr_cqe);
+	}
+	con->cq = NULL;
+}
+
 int rtrs_cq_qp_create(struct rtrs_sess *sess, struct rtrs_con *con,
 		       u32 max_send_sge, int cq_vector, int nr_cqe,
 		       u32 max_send_wr, u32 max_recv_wr,
@@ -293,11 +304,7 @@ int rtrs_cq_qp_create(struct rtrs_sess *sess, struct rtrs_con *con,
 	err = create_qp(con, sess->dev->ib_pd, max_send_wr, max_recv_wr,
 			max_send_sge);
 	if (err) {
-		if (is_pollqueue(con))
-			ib_free_cq(con->cq);
-		else
-			ib_cq_pool_put(con->cq, con->nr_cqe);
-		con->cq = NULL;
+		destroy_cq(con);
 		return err;
 	}
 	con->sess = sess;
@@ -312,13 +319,7 @@ void rtrs_cq_qp_destroy(struct rtrs_con *con)
 		rdma_destroy_qp(con->cm_id);
 		con->qp = NULL;
 	}
-	if (con->cq) {
-		if (is_pollqueue(con))
-			ib_free_cq(con->cq);
-		else
-			ib_cq_pool_put(con->cq, con->nr_cqe);
-		con->cq = NULL;
-	}
+	destroy_cq(con);
 }
 EXPORT_SYMBOL_GPL(rtrs_cq_qp_destroy);
 
-- 
2.25.1

[PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Md Haris Iqbal]

Allowing these characters in sessname can lead to unexpected results,
particularly because / is used as a separator between files in a path,
and . points to the current directory.

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
index bc8824b4ee0d..15c0077dd27e 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
@@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
 	struct rtrs_clt *clt;
 	int err, i;
 
+	if (strchr(sessname, '/') || strchr(sessname, '.')) {
+		pr_err("sessname cannot contain / and .\n");
+		err = -EINVAL;
+		goto out;
+	}
+
 	clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
 			ops->link_ev,
 			reconnect_delay_sec,
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 078a1cbac90c..7df71f8cf149 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
 		return err;
 	}
 
+	if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
+		rtrs_err(s, "sessname cannot contain / and .\n");
+		return -EINVAL;
+	}
+
 	if (exist_sessname(sess->srv->ctx,
 			   msg->sessname, &sess->srv->paths_uuid)) {
 		rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
-- 
2.25.1

[PATCH for-next 7/7] RDMA/rtrs-clt: Follow "one entry one value" rule for IO migration stats

[Md Haris Iqbal]

This commit divides the sysfs entry cpu_migration into 2 different entries
One for "from cpus" and the other for "to cpus".

Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c | 27 +++++++++++++-------
 drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c | 11 +++++---
 drivers/infiniband/ulp/rtrs/rtrs-clt.h       |  3 ++-
 3 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
index 61d5e0018392..f7e459fe68be 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-stats.c
@@ -37,29 +37,38 @@ void rtrs_clt_inc_failover_cnt(struct rtrs_clt_stats *stats)
 	s->rdma.failover_cnt++;
 }
 
-int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats, char *buf)
+int rtrs_clt_stats_migration_from_cnt_to_str(struct rtrs_clt_stats *stats, char *buf)
 {
 	struct rtrs_clt_stats_pcpu *s;
 
 	size_t used;
 	int cpu;
 
-	used = sysfs_emit(buf, "    ");
-	for_each_possible_cpu(cpu)
-		used += sysfs_emit_at(buf, used, " CPU%u", cpu);
-
-	used += sysfs_emit_at(buf, used, "\nfrom:");
+	used = 0;
 	for_each_possible_cpu(cpu) {
 		s = per_cpu_ptr(stats->pcpu_stats, cpu);
-		used += sysfs_emit_at(buf, used, " %d",
+		used += sysfs_emit_at(buf, used, "%d ",
 				  atomic_read(&s->cpu_migr.from));
 	}
 
-	used += sysfs_emit_at(buf, used, "\nto  :");
+	used += sysfs_emit_at(buf, used, "\n");
+
+	return used;
+}
+
+int rtrs_clt_stats_migration_to_cnt_to_str(struct rtrs_clt_stats *stats, char *buf)
+{
+	struct rtrs_clt_stats_pcpu *s;
+
+	size_t used;
+	int cpu;
+
+	used = 0;
 	for_each_possible_cpu(cpu) {
 		s = per_cpu_ptr(stats->pcpu_stats, cpu);
-		used += sysfs_emit_at(buf, used, " %d", s->cpu_migr.to);
+		used += sysfs_emit_at(buf, used, "%d ", s->cpu_migr.to);
 	}
+
 	used += sysfs_emit_at(buf, used, "\n");
 
 	return used;
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
index 4ee592ccf979..0e69180c3771 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt-sysfs.c
@@ -296,8 +296,12 @@ static struct kobj_attribute rtrs_clt_remove_path_attr =
 	__ATTR(remove_path, 0644, rtrs_clt_remove_path_show,
 	       rtrs_clt_remove_path_store);
 
-STAT_ATTR(struct rtrs_clt_stats, cpu_migration,
-	  rtrs_clt_stats_migration_cnt_to_str,
+STAT_ATTR(struct rtrs_clt_stats, cpu_migration_from,
+	  rtrs_clt_stats_migration_from_cnt_to_str,
+	  rtrs_clt_reset_cpu_migr_stats);
+
+STAT_ATTR(struct rtrs_clt_stats, cpu_migration_to,
+	  rtrs_clt_stats_migration_to_cnt_to_str,
 	  rtrs_clt_reset_cpu_migr_stats);
 
 STAT_ATTR(struct rtrs_clt_stats, reconnects,
@@ -313,7 +317,8 @@ STAT_ATTR(struct rtrs_clt_stats, reset_all,
 	  rtrs_clt_reset_all_stats);
 
 static struct attribute *rtrs_clt_stats_attrs[] = {
-	&cpu_migration_attr.attr,
+	&cpu_migration_from_attr.attr,
+	&cpu_migration_to_attr.attr,
 	&reconnects_attr.attr,
 	&rdma_attr.attr,
 	&reset_all_attr.attr,
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.h b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
index 6d81aae53df4..9afffccff973 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-clt.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.h
@@ -226,7 +226,8 @@ int rtrs_clt_reset_rdma_lat_distr_stats(struct rtrs_clt_stats *stats,
 ssize_t rtrs_clt_stats_rdma_lat_distr_to_str(struct rtrs_clt_stats *stats,
 					      char *page);
 int rtrs_clt_reset_cpu_migr_stats(struct rtrs_clt_stats *stats, bool enable);
-int rtrs_clt_stats_migration_cnt_to_str(struct rtrs_clt_stats *stats, char *buf);
+int rtrs_clt_stats_migration_from_cnt_to_str(struct rtrs_clt_stats *stats, char *buf);
+int rtrs_clt_stats_migration_to_cnt_to_str(struct rtrs_clt_stats *stats, char *buf);
 int rtrs_clt_reset_reconnects_stat(struct rtrs_clt_stats *stats, bool enable);
 int rtrs_clt_stats_reconnects_to_str(struct rtrs_clt_stats *stats, char *buf);
 int rtrs_clt_reset_rdma_stats(struct rtrs_clt_stats *stats, bool enable);
-- 
2.25.1

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> Allowing these characters in sessname can lead to unexpected results,
> particularly because / is used as a separator between files in a path,
> and . points to the current directory.
> 
> Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
>  2 files changed, 11 insertions(+)

It will be safer if you check for only allowed symbols and disallow
everything else. Check for: a-Z, 0-9 and "-".

Thanks

> 
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> index bc8824b4ee0d..15c0077dd27e 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
>  	struct rtrs_clt *clt;
>  	int err, i;
>  
> +	if (strchr(sessname, '/') || strchr(sessname, '.')) {
> +		pr_err("sessname cannot contain / and .\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
>  			ops->link_ev,
>  			reconnect_delay_sec,
> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> index 078a1cbac90c..7df71f8cf149 100644
> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
>  		return err;
>  	}
>  
> +	if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> +		rtrs_err(s, "sessname cannot contain / and .\n");
> +		return -EINVAL;
> +	}
> +
>  	if (exist_sessname(sess->srv->ctx,
>  			   msg->sessname, &sess->srv->paths_uuid)) {
>  		rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> -- 
> 2.25.1
> 

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jack Wang]

Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
>
> On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > Allowing these characters in sessname can lead to unexpected results,
> > particularly because / is used as a separator between files in a path,
> > and . points to the current directory.
> >
> > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > ---
> >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> >  2 files changed, 11 insertions(+)
>
> It will be safer if you check for only allowed symbols and disallow
> everything else. Check for: a-Z, 0-9 and "-".
>
Hi Leon,

Thanks for your suggestions.
The reasons we choose to do disallow only '/' and '.':
1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
2 matching for 2 characters is faster than checking all the allowed
symbols during session establishment.
3 we do use hostnameA@hostnameB for production usage right now, we
don't want to break the user space.

I hope this makes sense to you.

Regards!

>
> >
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > index bc8824b4ee0d..15c0077dd27e 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
> >       struct rtrs_clt *clt;
> >       int err, i;
> >
> > +     if (strchr(sessname, '/') || strchr(sessname, '.')) {
> > +             pr_err("sessname cannot contain / and .\n");
> > +             err = -EINVAL;
> > +             goto out;
> > +     }
> > +
> >       clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
> >                       ops->link_ev,
> >                       reconnect_delay_sec,
> > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > index 078a1cbac90c..7df71f8cf149 100644
> > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
> >               return err;
> >       }
> >
> > +     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> > +             rtrs_err(s, "sessname cannot contain / and .\n");
> > +             return -EINVAL;
> > +     }
> > +
> >       if (exist_sessname(sess->srv->ctx,
> >                          msg->sessname, &sess->srv->paths_uuid)) {
> >               rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> > --
> > 2.25.1
> >

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> >
> > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > Allowing these characters in sessname can lead to unexpected results,
> > > particularly because / is used as a separator between files in a path,
> > > and . points to the current directory.
> > >
> > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > ---
> > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > >  2 files changed, 11 insertions(+)
> >
> > It will be safer if you check for only allowed symbols and disallow
> > everything else. Check for: a-Z, 0-9 and "-".
> >
> Hi Leon,
> 
> Thanks for your suggestions.
> The reasons we choose to do disallow only '/' and '.':
> 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.

So you need to add all possible protections and checks that VFS has to allow "random" name.

> 2 matching for 2 characters is faster than checking all the allowed
> symbols during session establishment.

Extra CPU cycles won't make any difference here.

> 3 we do use hostnameA@hostnameB for production usage right now, we
> don't want to break the user space.

You can add "@" into the list of accepted symbols.

> 
> I hope this makes sense to you.
> 
> Regards!
> 
> >
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > index bc8824b4ee0d..15c0077dd27e 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
> > >       struct rtrs_clt *clt;
> > >       int err, i;
> > >
> > > +     if (strchr(sessname, '/') || strchr(sessname, '.')) {
> > > +             pr_err("sessname cannot contain / and .\n");
> > > +             err = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > >       clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
> > >                       ops->link_ev,
> > >                       reconnect_delay_sec,
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > index 078a1cbac90c..7df71f8cf149 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
> > >               return err;
> > >       }
> > >
> > > +     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> > > +             rtrs_err(s, "sessname cannot contain / and .\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > >       if (exist_sessname(sess->srv->ctx,
> > >                          msg->sessname, &sess->srv->paths_uuid)) {
> > >               rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> > > --
> > > 2.25.1
> > >

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jack Wang]

Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
>
> On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > >
> > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > Allowing these characters in sessname can lead to unexpected results,
> > > > particularly because / is used as a separator between files in a path,
> > > > and . points to the current directory.
> > > >
> > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > ---
> > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > >  2 files changed, 11 insertions(+)
> > >
> > > It will be safer if you check for only allowed symbols and disallow
> > > everything else. Check for: a-Z, 0-9 and "-".
> > >
> > Hi Leon,
> >
> > Thanks for your suggestions.
> > The reasons we choose to do disallow only '/' and '.':
> > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
>
> So you need to add all possible protections and checks that VFS has to allow "random" name.
It's only about sysfs here, as we use sessname to create dir in sysfs,
and I checked the code, it allows any 8-bit set, and convert '/' to
'!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
>
> > 2 matching for 2 characters is faster than checking all the allowed
> > symbols during session establishment.
>
> Extra CPU cycles won't make any difference here.
As we can have hundreds of sessions, in the end, it matters.

Thanks
>
> > 3 we do use hostnameA@hostnameB for production usage right now, we
> > don't want to break the user space.
>
> You can add "@" into the list of accepted symbols.
>
> >
> > I hope this makes sense to you.
> >
> > Regards!
> >
> > >
> > > >
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > index bc8824b4ee0d..15c0077dd27e 100644
> > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
> > > >       struct rtrs_clt *clt;
> > > >       int err, i;
> > > >
> > > > +     if (strchr(sessname, '/') || strchr(sessname, '.')) {
> > > > +             pr_err("sessname cannot contain / and .\n");
> > > > +             err = -EINVAL;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > >       clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
> > > >                       ops->link_ev,
> > > >                       reconnect_delay_sec,
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > index 078a1cbac90c..7df71f8cf149 100644
> > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
> > > >               return err;
> > > >       }
> > > >
> > > > +     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> > > > +             rtrs_err(s, "sessname cannot contain / and .\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > >       if (exist_sessname(sess->srv->ctx,
> > > >                          msg->sessname, &sess->srv->paths_uuid)) {
> > > >               rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> > > > --
> > > > 2.25.1
> > > >

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> >
> > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > >
> > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > particularly because / is used as a separator between files in a path,
> > > > > and . points to the current directory.
> > > > >
> > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > ---
> > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > >  2 files changed, 11 insertions(+)
> > > >
> > > > It will be safer if you check for only allowed symbols and disallow
> > > > everything else. Check for: a-Z, 0-9 and "-".
> > > >
> > > Hi Leon,
> > >
> > > Thanks for your suggestions.
> > > The reasons we choose to do disallow only '/' and '.':
> > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> >
> > So you need to add all possible protections and checks that VFS has to allow "random" name.
> It's only about sysfs here, as we use sessname to create dir in sysfs,
> and I checked the code, it allows any 8-bit set, and convert '/' to
> '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> >
> > > 2 matching for 2 characters is faster than checking all the allowed
> > > symbols during session establishment.
> >
> > Extra CPU cycles won't make any difference here.
> As we can have hundreds of sessions, in the end, it matters.

Your rtrs_clt_open() function is far from being optimized for
performance. It allocates memory, iterates over all paths, creates
sysfs and kobject.

So no, it doesn't matter here.

Thanks

> 
> Thanks
> >
> > > 3 we do use hostnameA@hostnameB for production usage right now, we
> > > don't want to break the user space.
> >
> > You can add "@" into the list of accepted symbols.
> >
> > >
> > > I hope this makes sense to you.
> > >
> > > Regards!
> > >
> > > >
> > > > >
> > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > index bc8824b4ee0d..15c0077dd27e 100644
> > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
> > > > >       struct rtrs_clt *clt;
> > > > >       int err, i;
> > > > >
> > > > > +     if (strchr(sessname, '/') || strchr(sessname, '.')) {
> > > > > +             pr_err("sessname cannot contain / and .\n");
> > > > > +             err = -EINVAL;
> > > > > +             goto out;
> > > > > +     }
> > > > > +
> > > > >       clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
> > > > >                       ops->link_ev,
> > > > >                       reconnect_delay_sec,
> > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > index 078a1cbac90c..7df71f8cf149 100644
> > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
> > > > >               return err;
> > > > >       }
> > > > >
> > > > > +     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> > > > > +             rtrs_err(s, "sessname cannot contain / and .\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > >       if (exist_sessname(sess->srv->ctx,
> > > > >                          msg->sessname, &sess->srv->paths_uuid)) {
> > > > >               rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> > > > > --
> > > > > 2.25.1
> > > > >

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jinpu Wang]

On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > >
> > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > >
> > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > particularly because / is used as a separator between files in a path,
> > > > > > and . points to the current directory.
> > > > > >
> > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > ---
> > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > >  2 files changed, 11 insertions(+)
> > > > >
> > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > >
> > > > Hi Leon,
> > > >
> > > > Thanks for your suggestions.
> > > > The reasons we choose to do disallow only '/' and '.':
> > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > >
> > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > and I checked the code, it allows any 8-bit set, and convert '/' to
> > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > >
> > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > symbols during session establishment.
> > >
> > > Extra CPU cycles won't make any difference here.
> > As we can have hundreds of sessions, in the end, it matters.
>
> Your rtrs_clt_open() function is far from being optimized for
> performance. It allocates memory, iterates over all paths, creates
> sysfs and kobject.
>
> So no, it doesn't matter here.
>
Let me reiterate, why do we want to further slow it down, what do you
anticipate if we only do the disallow approach
as we do it now?

Thanks!
>
> Thanks
>
> >
> > Thanks
> > >
> > > > 3 we do use hostnameA@hostnameB for production usage right now, we
> > > > don't want to break the user space.
> > >
> > > You can add "@" into the list of accepted symbols.
> > >
> > > >
> > > > I hope this makes sense to you.
> > > >
> > > > Regards!
> > > >
> > > > >
> > > > > >
> > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > > index bc8824b4ee0d..15c0077dd27e 100644
> > > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > > > > @@ -2788,6 +2788,12 @@ struct rtrs_clt *rtrs_clt_open(struct rtrs_clt_ops *ops,
> > > > > >       struct rtrs_clt *clt;
> > > > > >       int err, i;
> > > > > >
> > > > > > +     if (strchr(sessname, '/') || strchr(sessname, '.')) {
> > > > > > +             pr_err("sessname cannot contain / and .\n");
> > > > > > +             err = -EINVAL;
> > > > > > +             goto out;
> > > > > > +     }
> > > > > > +
> > > > > >       clt = alloc_clt(sessname, paths_num, port, pdu_sz, ops->priv,
> > > > > >                       ops->link_ev,
> > > > > >                       reconnect_delay_sec,
> > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > index 078a1cbac90c..7df71f8cf149 100644
> > > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > > > @@ -803,6 +803,11 @@ static int process_info_req(struct rtrs_srv_con *con,
> > > > > >               return err;
> > > > > >       }
> > > > > >
> > > > > > +     if (strchr(msg->sessname, '/') || strchr(msg->sessname, '.')) {
> > > > > > +             rtrs_err(s, "sessname cannot contain / and .\n");
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > > +
> > > > > >       if (exist_sessname(sess->srv->ctx,
> > > > > >                          msg->sessname, &sess->srv->paths_uuid)) {
> > > > > >               rtrs_err(s, "sessname is duplicated: %s\n", msg->sessname);
> > > > > > --
> > > > > > 2.25.1
> > > > > >

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > >
> > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > >
> > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > and . points to the current directory.
> > > > > > >
> > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > ---
> > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > >  2 files changed, 11 insertions(+)
> > > > > >
> > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > >
> > > > > Hi Leon,
> > > > >
> > > > > Thanks for your suggestions.
> > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > >
> > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > >
> > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > symbols during session establishment.
> > > >
> > > > Extra CPU cycles won't make any difference here.
> > > As we can have hundreds of sessions, in the end, it matters.
> >
> > Your rtrs_clt_open() function is far from being optimized for
> > performance. It allocates memory, iterates over all paths, creates
> > sysfs and kobject.
> >
> > So no, it doesn't matter here.
> >
> Let me reiterate, why do we want to further slow it down, what do you
> anticipate if we only do the disallow approach
> as we do it now?

It is common practice to sanitize user input and explicitly allow known
good input, instead of relying on deny of bad input. We don't know the
future and can't be sure that "deny" is actually closed all holes.

Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jinpu Wang]

On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > > >
> > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > > >
> > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > > and . points to the current directory.
> > > > > > > >
> > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > > ---
> > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > >
> > > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > > >
> > > > > > Hi Leon,
> > > > > >
> > > > > > Thanks for your suggestions.
> > > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > > >
> > > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > > >
> > > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > > symbols during session establishment.
> > > > >
> > > > > Extra CPU cycles won't make any difference here.
> > > > As we can have hundreds of sessions, in the end, it matters.
> > >
> > > Your rtrs_clt_open() function is far from being optimized for
> > > performance. It allocates memory, iterates over all paths, creates
> > > sysfs and kobject.
> > >
> > > So no, it doesn't matter here.
> > >
> > Let me reiterate, why do we want to further slow it down, what do you
> > anticipate if we only do the disallow approach
> > as we do it now?
>
> It is common practice to sanitize user input and explicitly allow known
> good input, instead of relying on deny of bad input. We don't know the
> future and can't be sure that "deny" is actually closed all holes.

Thanks for the clarification, but still what kind of holes do you have
in mind, the input string length is already checked and it's
not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC.

Thanks!

> Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote:
> On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > > > >
> > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > > > >
> > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > > > and . points to the current directory.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > >
> > > > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > > > >
> > > > > > > Hi Leon,
> > > > > > >
> > > > > > > Thanks for your suggestions.
> > > > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > > > >
> > > > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > > > >
> > > > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > > > symbols during session establishment.
> > > > > >
> > > > > > Extra CPU cycles won't make any difference here.
> > > > > As we can have hundreds of sessions, in the end, it matters.
> > > >
> > > > Your rtrs_clt_open() function is far from being optimized for
> > > > performance. It allocates memory, iterates over all paths, creates
> > > > sysfs and kobject.
> > > >
> > > > So no, it doesn't matter here.
> > > >
> > > Let me reiterate, why do we want to further slow it down, what do you
> > > anticipate if we only do the disallow approach
> > > as we do it now?
> >
> > It is common practice to sanitize user input and explicitly allow known
> > good input, instead of relying on deny of bad input. We don't know the
> > future and can't be sure that "deny" is actually closed all holes.
> 
> Thanks for the clarification, but still what kind of holes do you have
> in mind, the input string length is already checked and it's
> not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC.

As an example, symbols like "/" and "\".

> 
> Thanks!
> 
> > Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jinpu Wang]

On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote:
> > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > > > > >
> > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > > > > >
> > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > > > > and . points to the current directory.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > > >
> > > > > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > > > > >
> > > > > > > > Hi Leon,
> > > > > > > >
> > > > > > > > Thanks for your suggestions.
> > > > > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > > > > >
> > > > > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > > > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > > > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > > > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > > > > >
> > > > > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > > > > symbols during session establishment.
> > > > > > >
> > > > > > > Extra CPU cycles won't make any difference here.
> > > > > > As we can have hundreds of sessions, in the end, it matters.
> > > > >
> > > > > Your rtrs_clt_open() function is far from being optimized for
> > > > > performance. It allocates memory, iterates over all paths, creates
> > > > > sysfs and kobject.
> > > > >
> > > > > So no, it doesn't matter here.
> > > > >
> > > > Let me reiterate, why do we want to further slow it down, what do you
> > > > anticipate if we only do the disallow approach
> > > > as we do it now?
> > >
> > > It is common practice to sanitize user input and explicitly allow known
> > > good input, instead of relying on deny of bad input. We don't know the
> > > future and can't be sure that "deny" is actually closed all holes.
> >
> > Thanks for the clarification, but still what kind of holes do you have
> > in mind, the input string length is already checked and it's
> > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC.
>
> As an example, symbols like "/" and "\".
"/" aside, we already disable it.
I did a test, there is no problem to use "\" as sysfs name.

Thanks!
>
> >
> > Thanks!
> >
> > > Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Leon Romanovsky]

On Fri, Oct 01, 2021 at 02:40:24PM +0200, Jinpu Wang wrote:
> On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote:
> > > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> > > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > > > > > >
> > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > > > > > >
> > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > > > > > and . points to the current directory.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > > > >
> > > > > > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > > > > > >
> > > > > > > > > Hi Leon,
> > > > > > > > >
> > > > > > > > > Thanks for your suggestions.
> > > > > > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > > > > > >
> > > > > > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > > > > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > > > > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > > > > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > > > > > >
> > > > > > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > > > > > symbols during session establishment.
> > > > > > > >
> > > > > > > > Extra CPU cycles won't make any difference here.
> > > > > > > As we can have hundreds of sessions, in the end, it matters.
> > > > > >
> > > > > > Your rtrs_clt_open() function is far from being optimized for
> > > > > > performance. It allocates memory, iterates over all paths, creates
> > > > > > sysfs and kobject.
> > > > > >
> > > > > > So no, it doesn't matter here.
> > > > > >
> > > > > Let me reiterate, why do we want to further slow it down, what do you
> > > > > anticipate if we only do the disallow approach
> > > > > as we do it now?
> > > >
> > > > It is common practice to sanitize user input and explicitly allow known
> > > > good input, instead of relying on deny of bad input. We don't know the
> > > > future and can't be sure that "deny" is actually closed all holes.
> > >
> > > Thanks for the clarification, but still what kind of holes do you have
> > > in mind, the input string length is already checked and it's
> > > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC.
> >
> > As an example, symbols like "/" and "\".
> "/" aside, we already disable it.
> I did a test, there is no problem to use "\" as sysfs name.

It say nothing about future.

Please change your implementation to accept only valid
symbols and improve connection performance in other places.

Thanks

> 
> Thanks!
> >
> > >
> > > Thanks!
> > >
> > > > Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jinpu Wang]

On Fri, Oct 1, 2021 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Oct 01, 2021 at 02:40:24PM +0200, Jinpu Wang wrote:
> > On Thu, Sep 30, 2021 at 9:53 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 09:10:33AM +0200, Jinpu Wang wrote:
> > > > On Thu, Sep 30, 2021 at 9:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Thu, Sep 30, 2021 at 08:03:40AM +0200, Jinpu Wang wrote:
> > > > > > On Wed, Sep 29, 2021 at 2:04 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, Sep 29, 2021 at 09:00:56AM +0200, Jack Wang wrote:
> > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月28日周二 上午9:28写道：
> > > > > > > > >
> > > > > > > > > On Tue, Sep 28, 2021 at 09:08:26AM +0200, Jack Wang wrote:
> > > > > > > > > > Leon Romanovsky <leon@kernel.org> 于2021年9月27日周一 下午2:23写道：
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Sep 22, 2021 at 02:53:32PM +0200, Md Haris Iqbal wrote:
> > > > > > > > > > > > Allowing these characters in sessname can lead to unexpected results,
> > > > > > > > > > > > particularly because / is used as a separator between files in a path,
> > > > > > > > > > > > and . points to the current directory.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > > > > > > > > > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > > > > > > > > > > Reviewed-by: Aleksei Marov <aleksei.marov@ionos.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-clt.c | 6 ++++++
> > > > > > > > > > > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 5 +++++
> > > > > > > > > > > >  2 files changed, 11 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > It will be safer if you check for only allowed symbols and disallow
> > > > > > > > > > > everything else. Check for: a-Z, 0-9 and "-".
> > > > > > > > > > >
> > > > > > > > > > Hi Leon,
> > > > > > > > > >
> > > > > > > > > > Thanks for your suggestions.
> > > > > > > > > > The reasons we choose to do disallow only '/' and '.':
> > > > > > > > > > 1 more flexible, most UNIX filenames allow any 8-bit set, except '/' and null.
> > > > > > > > >
> > > > > > > > > So you need to add all possible protections and checks that VFS has to allow "random" name.
> > > > > > > > It's only about sysfs here, as we use sessname to create dir in sysfs,
> > > > > > > > and I checked the code, it allows any 8-bit set, and convert '/' to
> > > > > > > > '!', see https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L299
> > > > > > > > >
> > > > > > > > > > 2 matching for 2 characters is faster than checking all the allowed
> > > > > > > > > > symbols during session establishment.
> > > > > > > > >
> > > > > > > > > Extra CPU cycles won't make any difference here.
> > > > > > > > As we can have hundreds of sessions, in the end, it matters.
> > > > > > >
> > > > > > > Your rtrs_clt_open() function is far from being optimized for
> > > > > > > performance. It allocates memory, iterates over all paths, creates
> > > > > > > sysfs and kobject.
> > > > > > >
> > > > > > > So no, it doesn't matter here.
> > > > > > >
> > > > > > Let me reiterate, why do we want to further slow it down, what do you
> > > > > > anticipate if we only do the disallow approach
> > > > > > as we do it now?
> > > > >
> > > > > It is common practice to sanitize user input and explicitly allow known
> > > > > good input, instead of relying on deny of bad input. We don't know the
> > > > > future and can't be sure that "deny" is actually closed all holes.
> > > >
> > > > Thanks for the clarification, but still what kind of holes do you have
> > > > in mind, the input string length is already checked and it's
> > > > not duplicated with other sessname. and sysfs does allow all 8 bit set IIUC.
> > >
> > > As an example, symbols like "/" and "\".
> > "/" aside, we already disable it.
> > I did a test, there is no problem to use "\" as sysfs name.
>
> It say nothing about future.
>
> Please change your implementation to accept only valid
> symbols and improve connection performance in other places.
>
> Thanks
Sorry, I'm really not convinced, why should we only do allowing, not
disabling in this case?

Jason, what's your suggestion?

Thanks
>
> >
> > Thanks!
> > >
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jason Gunthorpe]

On Mon, Oct 04, 2021 at 07:41:08AM +0200, Jinpu Wang wrote:
> Sorry, I'm really not convinced, why should we only do allowing, not
> disabling in this case?
> 
> Jason, what's your suggestion?

At least from a correctness perspective only the characters / and \0
are forbidden, as well as any existing filenames like . and ..

Generally it is not a great idea to restrict things too much because
it can block the full use of UTF-8 for non-English languages.

However, I also think the sysfs in this driver is far too elaborate,
the time to complain was before this was all merged
unfortunately. Given that it is a simple enough bug fix it may as well
go ahead as-is.

Jason

Re: [PATCH for-next 0/7] Misc update for RTRS

[Jason Gunthorpe]

On Wed, Sep 22, 2021 at 02:53:26PM +0200, Md Haris Iqbal wrote:
> Hi Jason, hi Doug,
> 
> Please consider to include following changes to the next merge window.
> 
> The patchset is orgnized as:
> - patch1 sysfs_emit conversion.
> - patch2 remove len parameter.
> - patch3 Fix warning with poll mode.
> - patch4 Replace duplicate check.
> - patch5 Introduce helper function
> - patch6 Disallow special characters.
> - patch7 One entry one value for sysfs
> 
> Jack Wang (2):
>   RDMA/rtrs: Fix warning when use poll mode on client side.
>   RDMA/rtrs: Replace duplicate check with is_pollqueue helper
> 
> Md Haris Iqbal (5):
>   RDMA/rtrs: Use sysfs_emit instead of s*printf function for sysfs show
>   RDMA/rtrs: Remove len parameter from helper print functions of sysfs
>   RDMA/rtrs: Introduce destroy_cq helper
>   RDMA/rtrs: Do not allow sessname to contain special symbols / and .
>   RDMA/rtrs-clt: Follow "one entry one value" rule for IO migration
>     stats

Applied to for-next, thanks

Jason

Re: [PATCH for-next 6/7] RDMA/rtrs: Do not allow sessname to contain special symbols / and .

[Jinpu Wang]

On Mon, Oct 4, 2021 at 8:47 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Oct 04, 2021 at 07:41:08AM +0200, Jinpu Wang wrote:
> > Sorry, I'm really not convinced, why should we only do allowing, not
> > disabling in this case?
> >
> > Jason, what's your suggestion?
>
> At least from a correctness perspective only the characters / and \0
> are forbidden, as well as any existing filenames like . and ..
>
> Generally it is not a great idea to restrict things too much because
> it can block the full use of UTF-8 for non-English languages.
>
> However, I also think the sysfs in this driver is far too elaborate,
> the time to complain was before this was all merged
> unfortunately. Given that it is a simple enough bug fix it may as well
> go ahead as-is.
>
> Jason

Thanks Jason and Leon.