| Message ID | 20240531131550.64044-6-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ADDR_CHANGE event handling for NFSD | expand |
On 31.05.24 15:15, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > Sagi tells me that when a bonded device reports an address change, > the consumer must destroy its listener IDs and create new ones. > > See commit a032e4f6d60d ("nvmet-rdma: fix bonding failover possible > NULL deref"). > > Suggested-by: Sagi Grimberg <sagi@grimberg.me> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c > index fa50b7494a0a..a003b62fb7d5 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c > @@ -284,17 +284,31 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, > * > * Return values: > * %0: Do not destroy @cma_id > - * %1: Destroy @cma_id (never returned here) > + * %1: Destroy @cma_id > * > * NB: There is never a DEVICE_REMOVAL event for INADDR_ANY listeners. > */ > static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id, > struct rdma_cm_event *event) > { > + struct svcxprt_rdma *cma_xprt = cma_id->context; > + struct svc_xprt *cma_rdma = &cma_xprt->sc_xprt; > + struct sockaddr *sap = (struct sockaddr *)&cma_id->route.addr.src_addr; > + struct rdma_cm_id *listen_id; I am not sure whether I need to suggest "Reverse Christmas Tree" or not. This is a trivial problem. ^_^ You can ignore it. And it is not mandatory. But if we follow this rule, the source code will become more readable. Zhu Yanjun > + > switch (event->event) { > case RDMA_CM_EVENT_CONNECT_REQUEST: > handle_connect_req(cma_id, &event->param.conn); > break; > + case RDMA_CM_EVENT_ADDR_CHANGE: > + listen_id = svc_rdma_create_listen_id(cma_rdma->xpt_net, > + sap, cma_xprt); > + if (IS_ERR(listen_id)) { > + pr_err("Listener dead, address change failed for device %s\n", > + cma_id->device->name); > + } else > + cma_xprt->sc_cm_id = listen_id; > + return 1; > default: > break; > }
On Sat, Jun 01, 2024 at 12:48:40PM +0200, Zhu Yanjun wrote: > On 31.05.24 15:15, cel@kernel.org wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > > > Sagi tells me that when a bonded device reports an address change, > > the consumer must destroy its listener IDs and create new ones. > > > > See commit a032e4f6d60d ("nvmet-rdma: fix bonding failover possible > > NULL deref"). > > > > Suggested-by: Sagi Grimberg <sagi@grimberg.me> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > --- > > net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c > > index fa50b7494a0a..a003b62fb7d5 100644 > > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c > > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c > > @@ -284,17 +284,31 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, > > * > > * Return values: > > * %0: Do not destroy @cma_id > > - * %1: Destroy @cma_id (never returned here) > > + * %1: Destroy @cma_id > > * > > * NB: There is never a DEVICE_REMOVAL event for INADDR_ANY listeners. > > */ > > static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id, > > struct rdma_cm_event *event) > > { > > + struct svcxprt_rdma *cma_xprt = cma_id->context; > > + struct svc_xprt *cma_rdma = &cma_xprt->sc_xprt; > > + struct sockaddr *sap = (struct sockaddr *)&cma_id->route.addr.src_addr; > > + struct rdma_cm_id *listen_id; > > I am not sure whether I need to suggest "Reverse Christmas Tree" or not. > This is a trivial problem. ^_^ > You can ignore it. And it is not mandatory. Oops, I missed that @sap can be declared first. It must have gotten lost while I was trying out different solutions for this issue. Fixed. > But if we follow this rule, the source code will become more readable. > > Zhu Yanjun > > > + > > switch (event->event) { > > case RDMA_CM_EVENT_CONNECT_REQUEST: > > handle_connect_req(cma_id, &event->param.conn); > > break; > > + case RDMA_CM_EVENT_ADDR_CHANGE: > > + listen_id = svc_rdma_create_listen_id(cma_rdma->xpt_net, > > + sap, cma_xprt); > > + if (IS_ERR(listen_id)) { > > + pr_err("Listener dead, address change failed for device %s\n", > > + cma_id->device->name); > > + } else > > + cma_xprt->sc_cm_id = listen_id; > > + return 1; > > default: > > break; > > } >
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c index fa50b7494a0a..a003b62fb7d5 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c @@ -284,17 +284,31 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, * * Return values: * %0: Do not destroy @cma_id - * %1: Destroy @cma_id (never returned here) + * %1: Destroy @cma_id * * NB: There is never a DEVICE_REMOVAL event for INADDR_ANY listeners. */ static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event) { + struct svcxprt_rdma *cma_xprt = cma_id->context; + struct svc_xprt *cma_rdma = &cma_xprt->sc_xprt; + struct sockaddr *sap = (struct sockaddr *)&cma_id->route.addr.src_addr; + struct rdma_cm_id *listen_id; + switch (event->event) { case RDMA_CM_EVENT_CONNECT_REQUEST: handle_connect_req(cma_id, &event->param.conn); break; + case RDMA_CM_EVENT_ADDR_CHANGE: + listen_id = svc_rdma_create_listen_id(cma_rdma->xpt_net, + sap, cma_xprt); + if (IS_ERR(listen_id)) { + pr_err("Listener dead, address change failed for device %s\n", + cma_id->device->name); + } else + cma_xprt->sc_cm_id = listen_id; + return 1; default: break; }