linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init
@ 2020-08-10 11:50 Md Haris Iqbal
  2020-08-11  7:01 ` Danil Kipnis
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Md Haris Iqbal @ 2020-08-10 11:50 UTC (permalink / raw)
  To: danil.kipnis, jinpu.wang, linux-rdma, dledford, jgg, leon, linux-block
  Cc: Md Haris Iqbal, kernel test robot

The rnbd_server module's communication manager (cm) initialization depends
on the registration of the "network namespace subsystem" of the RDMA CM
agent module. As such, when the kernel is configured to load the
rnbd_server and the RDMA cma module during initialization; and if the
rnbd_server module is initialized before RDMA cma module, a null ptr
dereference occurs during the RDMA bind operation.

Call trace below,

[    1.904782] Call Trace:
[    1.904782]  ? xas_load+0xd/0x80
[    1.904782]  xa_load+0x47/0x80
[    1.904782]  cma_ps_find+0x44/0x70
[    1.904782]  rdma_bind_addr+0x782/0x8b0
[    1.904782]  ? get_random_bytes+0x35/0x40
[    1.904782]  rtrs_srv_cm_init+0x50/0x80
[    1.904782]  rtrs_srv_open+0x102/0x180
[    1.904782]  ? rnbd_client_init+0x6e/0x6e
[    1.904782]  rnbd_srv_init_module+0x34/0x84
[    1.904782]  ? rnbd_client_init+0x6e/0x6e
[    1.904782]  do_one_initcall+0x4a/0x200
[    1.904782]  kernel_init_freeable+0x1f1/0x26e
[    1.904782]  ? rest_init+0xb0/0xb0
[    1.904782]  kernel_init+0xe/0x100
[    1.904782]  ret_from_fork+0x22/0x30
[    1.904782] Modules linked in:
[    1.904782] CR2: 0000000000000015
[    1.904782] ---[ end trace c42df88d6c7b0a48 ]---

All this happens cause the cm init is in the call chain of the module init,
which is not a preferred practice.

So remove the call to rdma_create_id() from the module init call chain.
Instead register rtrs-srv as an ib client, which makes sure that the
rdma_create_id() is called only when an ib device is added.

Fixes: 9cb837480424 ("RDMA/rtrs: server: main functionality")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
---
Change in v2:
        Use only single variable to track number of IB devices and failure
        Change according to kernel coding style

 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 79 +++++++++++++++++++++++++-
 drivers/infiniband/ulp/rtrs/rtrs-srv.h |  6 ++
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index 0d9241f5d9e6..69a37ce73b0c 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -16,6 +16,7 @@
 #include "rtrs-srv.h"
 #include "rtrs-log.h"
 #include <rdma/ib_cm.h>
+#include <rdma/ib_verbs.h>
 
 MODULE_DESCRIPTION("RDMA Transport Server");
 MODULE_LICENSE("GPL");
@@ -31,6 +32,7 @@ MODULE_LICENSE("GPL");
 static struct rtrs_rdma_dev_pd dev_pd;
 static mempool_t *chunk_pool;
 struct class *rtrs_dev_class;
+static struct rtrs_srv_ib_ctx ib_ctx;
 
 static int __read_mostly max_chunk_size = DEFAULT_MAX_CHUNK_SIZE;
 static int __read_mostly sess_queue_depth = DEFAULT_SESS_QUEUE_DEPTH;
@@ -2033,6 +2035,64 @@ static void free_srv_ctx(struct rtrs_srv_ctx *ctx)
 	kfree(ctx);
 }
 
+static int rtrs_srv_add_one(struct ib_device *device)
+{
+	struct rtrs_srv_ctx *ctx;
+	int ret;
+
+	if (ib_ctx.ib_dev_count)
+		goto out;
+
+	/*
+	 * Since our CM IDs are NOT bound to any ib device we will create them
+	 * only once
+	 */
+	ctx = ib_ctx.srv_ctx;
+	ret = rtrs_srv_rdma_init(ctx, ib_ctx.port);
+	if (ret) {
+		/*
+		 * We errored out here.
+		 * According to the ib code, if we encounter an error here then the
+		 * error code is ignored, and no more calls to our ops are made.
+		 */
+		pr_err("Failed to initialize RDMA connection");
+		ib_ctx.ib_dev_count = -1;
+		return ret;
+	}
+
+out:
+	/*
+	 * Keep a track on the number of ib devices added
+	 */
+	ib_ctx.ib_dev_count++;
+
+	return 0;
+}
+
+static void rtrs_srv_remove_one(struct ib_device *device, void *client_data)
+{
+	struct rtrs_srv_ctx *ctx;
+
+	ib_ctx.ib_dev_count--;
+
+	if (ib_ctx.ib_dev_count)
+		return;
+
+	/*
+	 * Since our CM IDs are NOT bound to any ib device we will remove them
+	 * only once, when the last device is removed
+	 */
+	ctx = ib_ctx.srv_ctx;
+	rdma_destroy_id(ctx->cm_id_ip);
+	rdma_destroy_id(ctx->cm_id_ib);
+}
+
+static struct ib_client rtrs_srv_client = {
+	.name	= "rtrs_server",
+	.add	= rtrs_srv_add_one,
+	.remove	= rtrs_srv_remove_one
+};
+
 /**
  * rtrs_srv_open() - open RTRS server context
  * @ops:		callback functions
@@ -2051,12 +2111,26 @@ struct rtrs_srv_ctx *rtrs_srv_open(struct rtrs_srv_ops *ops, u16 port)
 	if (!ctx)
 		return ERR_PTR(-ENOMEM);
 
-	err = rtrs_srv_rdma_init(ctx, port);
+	ib_ctx = (struct rtrs_srv_ib_ctx) {
+		.srv_ctx	= ctx,
+		.port		= port,
+	};
+
+	err = ib_register_client(&rtrs_srv_client);
 	if (err) {
 		free_srv_ctx(ctx);
 		return ERR_PTR(err);
 	}
 
+	/*
+	 * Since ib_register_client does not propagate the device add error
+	 * we check if .add was called and the RDMA connection init failed
+	 */
+	if (ib_ctx.ib_dev_count < 0) {
+		free_srv_ctx(ctx);
+		return ERR_PTR(-ENODEV);
+	}
+
 	return ctx;
 }
 EXPORT_SYMBOL(rtrs_srv_open);
@@ -2090,8 +2164,7 @@ static void close_ctx(struct rtrs_srv_ctx *ctx)
  */
 void rtrs_srv_close(struct rtrs_srv_ctx *ctx)
 {
-	rdma_destroy_id(ctx->cm_id_ip);
-	rdma_destroy_id(ctx->cm_id_ib);
+	ib_unregister_client(&rtrs_srv_client);
 	close_ctx(ctx);
 	free_srv_ctx(ctx);
 }
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
index dc95b0932f0d..e8f7e99a9a6e 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h
@@ -118,6 +118,12 @@ struct rtrs_srv_ctx {
 	struct list_head srv_list;
 };
 
+struct rtrs_srv_ib_ctx {
+	struct rtrs_srv_ctx	*srv_ctx;
+	u16			port;
+	int			ib_dev_count;
+};
+
 extern struct class *rtrs_dev_class;
 
 void close_sess(struct rtrs_srv_sess *sess);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 29+ messages in thread
* Re: [PATCH] Delay the initialization of rnbd_server module to late_initcall level
@ 2020-06-23 17:23 Jason Gunthorpe
  2020-08-04 13:37 ` [PATCH v2] RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init Md Haris Iqbal
  0 siblings, 1 reply; 29+ messages in thread
From: Jason Gunthorpe @ 2020-06-23 17:23 UTC (permalink / raw)
  To: Haris Iqbal
  Cc: Leon Romanovsky, linux-block, linux-rdma, Danil Kipnis,
	Jinpu Wang, dledford, kernel test robot

On Tue, Jun 23, 2020 at 05:05:51PM +0530, Haris Iqbal wrote:
> On Tue, Jun 23, 2020 at 7:54 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Jun 23, 2020 at 07:15:03PM +0530, Haris Iqbal wrote:
> > > On Tue, Jun 23, 2020 at 5:47 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > >
> > > > On Tue, Jun 23, 2020 at 03:20:27PM +0530, Haris Iqbal wrote:
> > > > > Hi Jason and Leon,
> > > > >
> > > > > Did you get a chance to look into my previous email?
> > > >
> > > > Was there a question?
> > >
> > > Multiple actually :)
> > >
> > > >
> > > > Jason
> > >
> > > In response to your emails,
> > >
> > > > Somehow nvme-rdma works:
> > >
> > > I think that's because the callchain during the nvme_rdma_init_module
> > > initialization stops at "nvmf_register_transport()". Here only the
> > > "struct nvmf_transport_ops nvme_rdma_transport" is registered, which
> > > contains the function "nvme_rdma_create_ctrl()". I tested this in my
> > > local setup and during kernel boot, that's the extent of the
> > > callchain.
> > > The ".create_ctrl"; which now points to "nvme_rdma_create_ctrl()" is
> > > called later from "nvmf_dev_write()". I am not sure when this is
> > > called, probably when the "discover" happens from the client side or
> > > during the server config.
> > >
> > > It seems that the "rdma_bind_addr()" is called by the nvme rdma
> > > module; but during the following events
> > > 1) When a discover happens from the client side. Call trace for that looks like,
> > > [ 1098.409398] nvmf_dev_write
> > > [ 1098.409403] nvmf_create_ctrl
> > > [ 1098.414568] nvme_rdma_create_ctrl
> > > [ 1098.415009] nvme_rdma_setup_ctrl
> > > [ 1098.415010] nvme_rdma_configure_admin_queue
> > > [ 1098.415010] nvme_rdma_alloc_queue
> > > [ 1098.415032] rdma_resolve_addr
> > > [ 1098.415032] cma_bind_addr
> > > [ 1098.415033] rdma_bind_addr
> > >
> > > 2) When a connect happens from the client side. Call trace is the same
> > > as above, plus "nvme_rdma_alloc_queue()" is called n number of times;
> > > n being the number of IO queues being created.
> > >
> > > On the server side, when an nvmf port is enabled, that also triggers a
> > > call to "rdma_bind_addr()", but that is not from the nvme rdma module.
> > > may be nvme target rdma? (not sure).
> > >
> > > Does this make sense or am I missing something here?
> >
> > It make sense, delaying creating and CM ID's until user space starts
> > will solve this init time problme
> 
> Right, and the patch is trying to achieve the delay by changing the
> init level to "late_initcall()"

It should not be done with initcall levels

> > Right rdma_create_id() must precede anything that has problems, and it
> > should not be done from module_init.
> 
> I understand this, but I am not sure why that is; as in why it should
> not be done from module_init?

Because that is how our module ordering scheme works

> > It is not OK to create RDMA CM IDs outside
> > a client - CM IDs are supposed to be cleaned up when the client is
> > removed.
> >
> > Similarly they are supposed to be created from the client attachment.
> 
> This again is a little confusing to me, since what I've observed in
> nvmt is, when a server port is created, the "rdma_bind_addr()"
> function is called.
> And this goes well with the server/target and client/initiator model,
> where the server has to get ready and start listening before a client
> can initiate a connection.
> What am I missing here?

client means a struct ib_client

Jason

> 
> >
> > Jason
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2020-08-19  7:57 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10 11:50 [PATCH v2] RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init Md Haris Iqbal
2020-08-11  7:01 ` Danil Kipnis
2020-08-11  8:31 ` Jinpu Wang
2020-08-11  8:45 ` Leon Romanovsky
2020-08-11  8:57   ` Haris Iqbal
2020-08-11 10:47     ` Leon Romanovsky
2020-08-11 10:53       ` Haris Iqbal
2020-08-11 11:05         ` Leon Romanovsky
2020-08-11 11:13         ` Jinpu Wang
2020-08-11 11:44           ` Danil Kipnis
2020-08-11 12:07             ` Leon Romanovsky
2020-08-11 12:32               ` Danil Kipnis
2020-08-12  5:48                 ` Leon Romanovsky
2020-08-17 23:04                   ` Haris Iqbal
2020-08-18  7:51                     ` Leon Romanovsky
2020-08-19  7:56                       ` Haris Iqbal
  -- strict thread matches above, loose matches on Subject: below --
2020-06-23 17:23 [PATCH] Delay the initialization of rnbd_server module to late_initcall level Jason Gunthorpe
2020-08-04 13:37 ` [PATCH v2] RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init Md Haris Iqbal
2020-08-05  5:57   ` Leon Romanovsky
2020-08-05  7:50     ` Haris Iqbal
2020-08-05  9:04       ` Leon Romanovsky
2020-08-05 11:09         ` Haris Iqbal
2020-08-05 13:12           ` Leon Romanovsky
2020-08-05 13:53             ` Haris Iqbal
2020-08-05 14:55               ` Leon Romanovsky
2020-08-05 15:27                 ` Haris Iqbal
2020-08-05  9:09     ` Danil Kipnis
2020-08-05  9:16       ` Leon Romanovsky
2020-08-05 11:18         ` Danil Kipnis
2020-08-05 13:09           ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).