From: Chuck Lever <[email protected]>
Sagi recently pointed out that the CM ADDR_CHANGE event handler in
svcrdma has a bug similar to one he fixed in the NVMe target. This
series attempts to address that issue.
Chuck Lever (2):
svcrdma: Refactor the creation of listener CMA ID
svcrdma: Handle ADDR_CHANGE CM event properly
net/sunrpc/xprtrdma/svc_rdma_transport.c | 83 ++++++++++++++++--------
1 file changed, 55 insertions(+), 28 deletions(-)
--
2.45.1
From: Chuck Lever <[email protected]>
In a moment, I will add a second consumer of CMA ID creation in
svcrdma. Refactor so this code can be reused.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
1 file changed, 40 insertions(+), 27 deletions(-)
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 2b1c16b9547d..fa50b7494a0a 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -65,6 +65,8 @@
static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
struct net *net, int node);
+static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
+ struct rdma_cm_event *event);
static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
struct net *net,
struct sockaddr *sa, int salen,
@@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
}
}
+static struct rdma_cm_id *
+svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
+ void *context)
+{
+ struct rdma_cm_id *listen_id;
+ int ret;
+
+ listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
+ RDMA_PS_TCP, IB_QPT_RC);
+ if (IS_ERR(listen_id))
+ return listen_id;
+
+ /* Allow both IPv4 and IPv6 sockets to bind a single port
+ * at the same time.
+ */
+#if IS_ENABLED(CONFIG_IPV6)
+ ret = rdma_set_afonly(listen_id, 1);
+ if (ret)
+ goto out_destroy;
+#endif
+ ret = rdma_bind_addr(listen_id, sap);
+ if (ret)
+ goto out_destroy;
+
+ ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
+ if (ret)
+ goto out_destroy;
+
+ return listen_id;
+
+out_destroy:
+ rdma_destroy_id(listen_id);
+ return ERR_PTR(ret);
+}
+
static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
struct net *net, int node)
{
@@ -307,7 +344,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
{
struct rdma_cm_id *listen_id;
struct svcxprt_rdma *cma_xprt;
- int ret;
if (sa->sa_family != AF_INET && sa->sa_family != AF_INET6)
return ERR_PTR(-EAFNOSUPPORT);
@@ -317,30 +353,13 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
set_bit(XPT_LISTENER, &cma_xprt->sc_xprt.xpt_flags);
strcpy(cma_xprt->sc_xprt.xpt_remotebuf, "listener");
- listen_id = rdma_create_id(net, svc_rdma_listen_handler, cma_xprt,
- RDMA_PS_TCP, IB_QPT_RC);
+ listen_id = svc_rdma_create_listen_id(net, sa, cma_xprt);
if (IS_ERR(listen_id)) {
- ret = PTR_ERR(listen_id);
- goto err0;
+ kfree(cma_xprt);
+ return (struct svc_xprt *)listen_id;
}
-
- /* Allow both IPv4 and IPv6 sockets to bind a single port
- * at the same time.
- */
-#if IS_ENABLED(CONFIG_IPV6)
- ret = rdma_set_afonly(listen_id, 1);
- if (ret)
- goto err1;
-#endif
- ret = rdma_bind_addr(listen_id, sa);
- if (ret)
- goto err1;
cma_xprt->sc_cm_id = listen_id;
- ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
- if (ret)
- goto err1;
-
/*
* We need to use the address from the cm_id in case the
* caller specified 0 for the port number.
@@ -349,12 +368,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
svc_xprt_set_local(&cma_xprt->sc_xprt, sa, salen);
return &cma_xprt->sc_xprt;
-
- err1:
- rdma_destroy_id(listen_id);
- err0:
- kfree(cma_xprt);
- return ERR_PTR(ret);
}
/*
--
2.45.1
From: Chuck Lever <[email protected]>
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 <[email protected]>
Signed-off-by: Chuck Lever <[email protected]>
---
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;
+
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;
}
--
2.45.1
On 31.05.24 15:15, [email protected] wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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 31/05/2024 16:15, [email protected] wrote:
> From: Chuck Lever <[email protected]>
>
> Sagi recently pointed out that the CM ADDR_CHANGE event handler in
> svcrdma has a bug similar to one he fixed in the NVMe target. This
> series attempts to address that issue.
>
> Chuck Lever (2):
> svcrdma: Refactor the creation of listener CMA ID
> svcrdma: Handle ADDR_CHANGE CM event properly
>
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 83 ++++++++++++++++--------
> 1 file changed, 55 insertions(+), 28 deletions(-)
>
Looks good, For the series:
Reviewed-by: Sagi Grimberg <[email protected]>
On 5/31/24 21:15, [email protected] wrote:
> From: Chuck Lever <[email protected]>
>
> In a moment, I will add a second consumer of CMA ID creation in
> svcrdma. Refactor so this code can be reused.
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
> 1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 2b1c16b9547d..fa50b7494a0a 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -65,6 +65,8 @@
>
> static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
> struct net *net, int node);
> +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> + struct rdma_cm_event *event);
> static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> struct net *net,
> struct sockaddr *sa, int salen,
> @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
> }
> }
>
> +static struct rdma_cm_id *
> +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
> + void *context)
> +{
> + struct rdma_cm_id *listen_id;
> + int ret;
> +
> + listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
> + RDMA_PS_TCP, IB_QPT_RC);
> + if (IS_ERR(listen_id))
> + return listen_id;
I am wondering if above need to return PTR_ERR(listen_id), and I find
some callers (in net/rds/, nvme etc)
return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return
ERR_PTR(ret) with ret is set to PTR_ERR(id).
Thanks,
Guoqing
On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
>
>
> On 5/31/24 21:15, [email protected] wrote:
> > From: Chuck Lever <[email protected]>
> >
> > In a moment, I will add a second consumer of CMA ID creation in
> > svcrdma. Refactor so this code can be reused.
> >
> > Signed-off-by: Chuck Lever <[email protected]>
> > ---
> > net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
> > 1 file changed, 40 insertions(+), 27 deletions(-)
> >
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > index 2b1c16b9547d..fa50b7494a0a 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > @@ -65,6 +65,8 @@
> > static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
> > struct net *net, int node);
> > +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> > + struct rdma_cm_event *event);
> > static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> > struct net *net,
> > struct sockaddr *sa, int salen,
> > @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
> > }
> > }
> > +static struct rdma_cm_id *
> > +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
> > + void *context)
> > +{
> > + struct rdma_cm_id *listen_id;
> > + int ret;
> > +
> > + listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
> > + RDMA_PS_TCP, IB_QPT_RC);
> > + if (IS_ERR(listen_id))
> > + return listen_id;
>
> I am wondering if above need to return PTR_ERR(listen_id),
PTR_ERR would convert the listen_id error to an integer, but
svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
using PTR_ERR() would be wrong in this case.
> and I find some
> callers (in net/rds/, nvme etc)
> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
> with ret is set to PTR_ERR(id).
These functions use PTR_ERR only when the calling function returns
an int.
--
Chuck Lever
> On Jun 2, 2024, at 3:50 AM, Sagi Grimberg <[email protected]> wrote:
>
>
>
> On 31/05/2024 16:15, [email protected] wrote:
>> From: Chuck Lever <[email protected]>
>>
>> Sagi recently pointed out that the CM ADDR_CHANGE event handler in
>> svcrdma has a bug similar to one he fixed in the NVMe target. This
>> series attempts to address that issue.
>>
>> Chuck Lever (2):
>> svcrdma: Refactor the creation of listener CMA ID
>> svcrdma: Handle ADDR_CHANGE CM event properly
>>
>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 83 ++++++++++++++++--------
>> 1 file changed, 55 insertions(+), 28 deletions(-)
>>
>
> Looks good, For the series:
>
> Reviewed-by: Sagi Grimberg <[email protected]>
Thank you! Applied to nfsd-next (for 6.11).
--
Chuck Lever
On 6/3/24 21:23, Chuck Lever wrote:
> On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
>>
>> On 5/31/24 21:15, [email protected] wrote:
>>> From: Chuck Lever <[email protected]>
>>>
>>> In a moment, I will add a second consumer of CMA ID creation in
>>> svcrdma. Refactor so this code can be reused.
>>>
>>> Signed-off-by: Chuck Lever <[email protected]>
>>> ---
>>> net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
>>> 1 file changed, 40 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> index 2b1c16b9547d..fa50b7494a0a 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -65,6 +65,8 @@
>>> static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
>>> struct net *net, int node);
>>> +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
>>> + struct rdma_cm_event *event);
>>> static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>>> struct net *net,
>>> struct sockaddr *sa, int salen,
>>> @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
>>> }
>>> }
>>> +static struct rdma_cm_id *
>>> +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
>>> + void *context)
>>> +{
>>> + struct rdma_cm_id *listen_id;
>>> + int ret;
>>> +
>>> + listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
>>> + RDMA_PS_TCP, IB_QPT_RC);
>>> + if (IS_ERR(listen_id))
>>> + return listen_id;
>> I am wondering if above need to return PTR_ERR(listen_id),
> PTR_ERR would convert the listen_id error to an integer, but
> svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
> using PTR_ERR() would be wrong in this case.
>
>
>> and I find some
>> callers (in net/rds/, nvme etc)
>> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
>> with ret is set to PTR_ERR(id).
> These functions use PTR_ERR only when the calling function returns
> an int.
Thanks for the explanation!
Guoqing