[PATCH 0/2] Fix ADDR_CHANGE event handling for NFSD

[PATCH 0/2] Fix ADDR_CHANGE event handling for NFSD

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

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

[PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

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 <chuck.lever@oracle.com>
---
 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

[PATCH 2/2] svcrdma: Handle ADDR_CHANGE CM event properly

[cel]

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;
+
 	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

Re: [PATCH 2/2] svcrdma: Handle ADDR_CHANGE CM event properly

[Zhu Yanjun]

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;
>   	}

Re: [PATCH 2/2] svcrdma: Handle ADDR_CHANGE CM event properly

[Chuck Lever]

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;
> >   	}
> 

-- 
Chuck Lever

Re: [PATCH 0/2] Fix ADDR_CHANGE event handling for NFSD

[Sagi Grimberg]

On 31/05/2024 16:15, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
>
> 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 <sagi@grimberg.me>

Re: [PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID

[Guoqing Jiang]

On 5/31/24 21:15, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
>
> 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 <chuck.lever@oracle.com>
> ---
>   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

Re: [PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID

[Chuck Lever]

On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
> 
> 
> On 5/31/24 21:15, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > 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 <chuck.lever@oracle.com>
> > ---
> >   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

Re: [PATCH 0/2] Fix ADDR_CHANGE event handling for NFSD

[Chuck Lever III]

> On Jun 2, 2024, at 3:50 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
> 
> 
> 
> On 31/05/2024 16:15, cel@kernel.org wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> 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 <sagi@grimberg.me>

Thank you! Applied to nfsd-next (for 6.11).


--
Chuck Lever

Re: [PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID

[Guoqing Jiang]

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, cel@kernel.org wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>
>>> 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 <chuck.lever@oracle.com>
>>> ---
>>>    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