All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL
@ 2022-05-21  9:51 Kinglong Mee
  2022-05-21 16:33 ` Chuck Lever III
  0 siblings, 1 reply; 8+ messages in thread
From: Kinglong Mee @ 2022-05-21  9:51 UTC (permalink / raw)
  To: Chuck Lever, Anna Schumaker, linux-nfs

When rdma server returns a fault reply, rpcrdma may treats it as a bcall.
As using NFSv3, a bc server is never exist.
rpcrdma_bc_receive_call will meets NULL pointer as,

[  226.057890] BUG: unable to handle kernel NULL pointer dereference at 
00000000000000c8
...
[  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
...
[  226.059732] Call Trace:
[  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
[  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
[  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
[  226.060257]  process_one_work+0x1a7/0x360
[  226.060367]  ? create_worker+0x1a0/0x1a0
[  226.060440]  worker_thread+0x30/0x390
[  226.060500]  ? create_worker+0x1a0/0x1a0
[  226.060574]  kthread+0x116/0x130
[  226.060661]  ? kthread_flush_work_fn+0x10/0x10
[  226.060724]  ret_from_fork+0x35/0x40
...

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
  net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
index 281ddb87ac8d..9486bb98eb2f 100644
--- a/net/sunrpc/xprtrdma/rpc_rdma.c
+++ b/net/sunrpc/xprtrdma/rpc_rdma.c
@@ -1121,9 +1121,14 @@ static bool
  rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
  #if defined(CONFIG_SUNRPC_BACKCHANNEL)
  {
+	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
  	struct xdr_stream *xdr = &rep->rr_stream;
  	__be32 *p;

+	/* no bc service, not a bcall. */
+	if (xprt->bc_serv == NULL)
+		return false;
+
  	if (rep->rr_proc != rdma_msg)
  		return false;

-- 
2.27.0


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

* Re: [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL
  2022-05-21  9:51 [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL Kinglong Mee
@ 2022-05-21 16:33 ` Chuck Lever III
  2022-05-22  1:24   ` Kinglong Mee
  0 siblings, 1 reply; 8+ messages in thread
From: Chuck Lever III @ 2022-05-21 16:33 UTC (permalink / raw)
  To: Kinglong Mee; +Cc: Anna Schumaker, Linux NFS Mailing List



> On May 21, 2022, at 5:51 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
> When rdma server returns a fault reply, rpcrdma may treats it as a bcall.
> As using NFSv3, a bc server is never exist.
> rpcrdma_bc_receive_call will meets NULL pointer as,
> 
> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
> ...
> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
> ...
> [  226.059732] Call Trace:
> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
> [  226.060257]  process_one_work+0x1a7/0x360
> [  226.060367]  ? create_worker+0x1a0/0x1a0
> [  226.060440]  worker_thread+0x30/0x390
> [  226.060500]  ? create_worker+0x1a0/0x1a0
> [  226.060574]  kthread+0x116/0x130
> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
> [  226.060724]  ret_from_fork+0x35/0x40
> ...
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
> index 281ddb87ac8d..9486bb98eb2f 100644
> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
> @@ -1121,9 +1121,14 @@ static bool
> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> {
> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
> 	struct xdr_stream *xdr = &rep->rr_stream;
> 	__be32 *p;
> 
> +	/* no bc service, not a bcall. */
> +	if (xprt->bc_serv == NULL)
> +		return false;
> +
> 	if (rep->rr_proc != rdma_msg)
> 		return false;

I'm not sure what you mean above by "fault reply".

The check here for whether the RPC/RDMA procedure is an RDMA_MSG
is supposed to be enough to avoid any further processing of an
RDMA_ERR type procedure.

What kind of fault has occurred? Can you share with us the
actual RPC/RDMA transport header that triggers the BUG?

--
Chuck Lever




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

* Re: [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL
  2022-05-21 16:33 ` Chuck Lever III
@ 2022-05-22  1:24   ` Kinglong Mee
  2022-05-22  1:41     ` Chuck Lever III
  0 siblings, 1 reply; 8+ messages in thread
From: Kinglong Mee @ 2022-05-22  1:24 UTC (permalink / raw)
  To: Chuck Lever III; +Cc: Anna Schumaker, Linux NFS Mailing List

Hi Chuck,

On 2022/5/22 12:33 AM, Chuck Lever III wrote:
> 
> 
>> On May 21, 2022, at 5:51 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>
>> When rdma server returns a fault reply, rpcrdma may treats it as a bcall.
>> As using NFSv3, a bc server is never exist.
>> rpcrdma_bc_receive_call will meets NULL pointer as,
>>
>> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
>> ...
>> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
>> ...
>> [  226.059732] Call Trace:
>> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
>> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
>> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
>> [  226.060257]  process_one_work+0x1a7/0x360
>> [  226.060367]  ? create_worker+0x1a0/0x1a0
>> [  226.060440]  worker_thread+0x30/0x390
>> [  226.060500]  ? create_worker+0x1a0/0x1a0
>> [  226.060574]  kthread+0x116/0x130
>> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
>> [  226.060724]  ret_from_fork+0x35/0x40
>> ...
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 281ddb87ac8d..9486bb98eb2f 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -1121,9 +1121,14 @@ static bool
>> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
>> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
>> {
>> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
>> 	struct xdr_stream *xdr = &rep->rr_stream;
>> 	__be32 *p;
>>
>> +	/* no bc service, not a bcall. */
>> +	if (xprt->bc_serv == NULL)
>> +		return false;
>> +
>> 	if (rep->rr_proc != rdma_msg)
>> 		return false;
> 
> I'm not sure what you mean above by "fault reply".
> 
> The check here for whether the RPC/RDMA procedure is an RDMA_MSG
> is supposed to be enough to avoid any further processing of an
> RDMA_ERR type procedure.
> 
> What kind of fault has occurred? Can you share with us the
> actual RPC/RDMA transport header that triggers the BUG?

I have a nfs rdma server, but it handles drc reply wrongly sometimes,
it returns a bad format reply to nfs client.
Nfs rdma client treats the bad format reply as a bcall, and

Thanks,
Kinglong Mee

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

* Re: [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL
  2022-05-22  1:24   ` Kinglong Mee
@ 2022-05-22  1:41     ` Chuck Lever III
  2022-05-22 12:11       ` Kinglong Mee
  2022-05-22 12:36       ` [PATCH v2] xprtrdma: treat all calls not a " Kinglong Mee
  0 siblings, 2 replies; 8+ messages in thread
From: Chuck Lever III @ 2022-05-22  1:41 UTC (permalink / raw)
  To: Kinglong Mee; +Cc: Anna Schumaker, Linux NFS Mailing List



> On May 21, 2022, at 9:24 PM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
> Hi Chuck,
> 
> On 2022/5/22 12:33 AM, Chuck Lever III wrote:
>>> On May 21, 2022, at 5:51 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>> 
>>> When rdma server returns a fault reply, rpcrdma may treats it as a bcall.
>>> As using NFSv3, a bc server is never exist.
>>> rpcrdma_bc_receive_call will meets NULL pointer as,
>>> 
>>> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
>>> ...
>>> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
>>> ...
>>> [  226.059732] Call Trace:
>>> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
>>> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
>>> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
>>> [  226.060257]  process_one_work+0x1a7/0x360
>>> [  226.060367]  ? create_worker+0x1a0/0x1a0
>>> [  226.060440]  worker_thread+0x30/0x390
>>> [  226.060500]  ? create_worker+0x1a0/0x1a0
>>> [  226.060574]  kthread+0x116/0x130
>>> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
>>> [  226.060724]  ret_from_fork+0x35/0x40
>>> ...
>>> 
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>> 
>>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> index 281ddb87ac8d..9486bb98eb2f 100644
>>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> @@ -1121,9 +1121,14 @@ static bool
>>> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
>>> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
>>> {
>>> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
>>> 	struct xdr_stream *xdr = &rep->rr_stream;
>>> 	__be32 *p;
>>> 
>>> +	/* no bc service, not a bcall. */
>>> +	if (xprt->bc_serv == NULL)
>>> +		return false;
>>> +
>>> 	if (rep->rr_proc != rdma_msg)
>>> 		return false;
>> I'm not sure what you mean above by "fault reply".
>> The check here for whether the RPC/RDMA procedure is an RDMA_MSG
>> is supposed to be enough to avoid any further processing of an
>> RDMA_ERR type procedure.
>> What kind of fault has occurred? Can you share with us the
>> actual RPC/RDMA transport header that triggers the BUG?
> 
> I have a nfs rdma server, but it handles drc reply wrongly sometimes,
> it returns a bad format reply to nfs client.
> Nfs rdma client treats the bad format reply as a bcall, and

Doesn't this test return false:

1144         if (*p != cpu_to_be32(RPC_CALL))
1145                 return false;

But OK: a malicious NFSv3 server can trigger a client crash.
That's a bug.

This is an additional conditional in a hot path. Would it make
sense to move the new test to just after 1145?

Even then, it could be a bcall, the client simply isn't
prepared to process it. So "/* no bc service */" by itself
would be a more accurate comment.

--
Chuck Lever




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

* Re: [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL
  2022-05-22  1:41     ` Chuck Lever III
@ 2022-05-22 12:11       ` Kinglong Mee
  2022-05-22 12:36       ` [PATCH v2] xprtrdma: treat all calls not a " Kinglong Mee
  1 sibling, 0 replies; 8+ messages in thread
From: Kinglong Mee @ 2022-05-22 12:11 UTC (permalink / raw)
  To: Chuck Lever III; +Cc: Anna Schumaker, Linux NFS Mailing List



On 2022/5/22 9:41 AM, Chuck Lever III wrote:
> 
> 
>> On May 21, 2022, at 9:24 PM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>
>> Hi Chuck,
>>
>> On 2022/5/22 12:33 AM, Chuck Lever III wrote:
>>>> On May 21, 2022, at 5:51 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>>>
>>>> When rdma server returns a fault reply, rpcrdma may treats it as a bcall.
>>>> As using NFSv3, a bc server is never exist.
>>>> rpcrdma_bc_receive_call will meets NULL pointer as,
>>>>
>>>> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
>>>> ...
>>>> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
>>>> ...
>>>> [  226.059732] Call Trace:
>>>> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
>>>> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
>>>> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
>>>> [  226.060257]  process_one_work+0x1a7/0x360
>>>> [  226.060367]  ? create_worker+0x1a0/0x1a0
>>>> [  226.060440]  worker_thread+0x30/0x390
>>>> [  226.060500]  ? create_worker+0x1a0/0x1a0
>>>> [  226.060574]  kthread+0x116/0x130
>>>> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
>>>> [  226.060724]  ret_from_fork+0x35/0x40
>>>> ...
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>> ---
>>>> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>>>> index 281ddb87ac8d..9486bb98eb2f 100644
>>>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>>>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>>>> @@ -1121,9 +1121,14 @@ static bool
>>>> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
>>>> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
>>>> {
>>>> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
>>>> 	struct xdr_stream *xdr = &rep->rr_stream;
>>>> 	__be32 *p;
>>>>
>>>> +	/* no bc service, not a bcall. */
>>>> +	if (xprt->bc_serv == NULL)
>>>> +		return false;
>>>> +
>>>> 	if (rep->rr_proc != rdma_msg)
>>>> 		return false;
>>> I'm not sure what you mean above by "fault reply".
>>> The check here for whether the RPC/RDMA procedure is an RDMA_MSG
>>> is supposed to be enough to avoid any further processing of an
>>> RDMA_ERR type procedure.
>>> What kind of fault has occurred? Can you share with us the
>>> actual RPC/RDMA transport header that triggers the BUG?
>>
>> I have a nfs rdma server, but it handles drc reply wrongly sometimes,
>> it returns a bad format reply to nfs client.
>> Nfs rdma client treats the bad format reply as a bcall, and
> 
> Doesn't this test return false:
> 
> 1144         if (*p != cpu_to_be32(RPC_CALL))
> 1145                 return false;

No, it doesn't return false here.

The debug message at rpcrdma_bc_receive_call are,

[56579.837169] RPC:       rpcrdma_bc_receive_call: callback XID 
00000001, length=20
[56579.837174] RPC:       rpcrdma_bc_receive_call: 00 00 00 01 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 04

> 
> But OK: a malicious NFSv3 server can trigger a client crash.
> That's a bug.
> 
> This is an additional conditional in a hot path. Would it make
> sense to move the new test to just after 1145? >
> Even then, it could be a bcall, the client simply isn't
> prepared to process it. So "/* no bc service */" by itself
> would be a more accurate comment.

Okay, I will update it and send a v2 patch after moving the checking
after 1145.

Thanks,
Kinglong Mee

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

* [PATCH v2] xprtrdma: treat all calls not a bcall when bc_serv is NULL
  2022-05-22  1:41     ` Chuck Lever III
  2022-05-22 12:11       ` Kinglong Mee
@ 2022-05-22 12:36       ` Kinglong Mee
  2022-05-22 15:52         ` Chuck Lever III
  1 sibling, 1 reply; 8+ messages in thread
From: Kinglong Mee @ 2022-05-22 12:36 UTC (permalink / raw)
  To: Chuck Lever III, Anna Schumaker, Linux NFS Mailing List

When a rdma server returns a fault format reply, nfs v3 client may
treats it as a bcall when bc service is not exist.

The debug message at rpcrdma_bc_receive_call are,

[56579.837169] RPC:       rpcrdma_bc_receive_call: callback XID 
00000001, length=20
[56579.837174] RPC:       rpcrdma_bc_receive_call: 00 00 00 01 00 00 00 
00 00 00 00 00 00 00 00 00 00 00 00 04

After that, rpcrdma_bc_receive_call will meets NULL pointer as,

[  226.057890] BUG: unable to handle kernel NULL pointer dereference at 
00000000000000c8
...
[  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
...
[  226.059732] Call Trace:
[  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
[  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
[  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
[  226.060257]  process_one_work+0x1a7/0x360
[  226.060367]  ? create_worker+0x1a0/0x1a0
[  226.060440]  worker_thread+0x30/0x390
[  226.060500]  ? create_worker+0x1a0/0x1a0
[  226.060574]  kthread+0x116/0x130
[  226.060661]  ? kthread_flush_work_fn+0x10/0x10
[  226.060724]  ret_from_fork+0x35/0x40
...

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
  net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
index 281ddb87ac8d..190a4de239c8 100644
--- a/net/sunrpc/xprtrdma/rpc_rdma.c
+++ b/net/sunrpc/xprtrdma/rpc_rdma.c
@@ -1121,6 +1121,7 @@ static bool
  rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
  #if defined(CONFIG_SUNRPC_BACKCHANNEL)
  {
+	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
  	struct xdr_stream *xdr = &rep->rr_stream;
  	__be32 *p;

@@ -1144,6 +1145,10 @@ rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, 
struct rpcrdma_rep *rep)
  	if (*p != cpu_to_be32(RPC_CALL))
  		return false;

+	/* No bc service. */
+	if (xprt->bc_serv == NULL)
+		return false;
+
  	/* Now that we are sure this is a backchannel call,
  	 * advance to the RPC header.
  	 */
-- 
2.27.0


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

* Re: [PATCH v2] xprtrdma: treat all calls not a bcall when bc_serv is NULL
  2022-05-22 12:36       ` [PATCH v2] xprtrdma: treat all calls not a " Kinglong Mee
@ 2022-05-22 15:52         ` Chuck Lever III
  2022-05-23  1:39           ` Kinglong Mee
  0 siblings, 1 reply; 8+ messages in thread
From: Chuck Lever III @ 2022-05-22 15:52 UTC (permalink / raw)
  To: Kinglong Mee; +Cc: Anna Schumaker, Linux NFS Mailing List



> On May 22, 2022, at 8:36 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
> When a rdma server returns a fault format reply, nfs v3 client may
> treats it as a bcall when bc service is not exist.
> 
> The debug message at rpcrdma_bc_receive_call are,
> 
> [56579.837169] RPC:       rpcrdma_bc_receive_call: callback XID 00000001, length=20
> [56579.837174] RPC:       rpcrdma_bc_receive_call: 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04
> 
> After that, rpcrdma_bc_receive_call will meets NULL pointer as,
> 
> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
> ...
> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
> ...
> [  226.059732] Call Trace:
> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
> [  226.060257]  process_one_work+0x1a7/0x360
> [  226.060367]  ? create_worker+0x1a0/0x1a0
> [  226.060440]  worker_thread+0x30/0x390
> [  226.060500]  ? create_worker+0x1a0/0x1a0
> [  226.060574]  kthread+0x116/0x130
> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
> [  226.060724]  ret_from_fork+0x35/0x40
> ...
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>

Patch is good, Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

Commentary (no changes to v2 needed):

The description still suggests we are adapting the client to
work around a broken server. I don't feel this is the right
reason to accept this change. Instead: we really don't want
the client to be vulnerable to bad input for any reason.
After this fix is applied, bad RPC messages are eventually
dropped rather than processed, which is the desired behavior.

Anna, I recommend adding Cc: stable for this fix.

Kinglong, please ensure that client Receive resources are not
leaked in this case. If they are, send along an additional
patch; if not, let us know and we can close this issue. Thank
you!


> ---
> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
> index 281ddb87ac8d..190a4de239c8 100644
> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
> @@ -1121,6 +1121,7 @@ static bool
> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
> {
> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
> 	struct xdr_stream *xdr = &rep->rr_stream;
> 	__be32 *p;
> 
> @@ -1144,6 +1145,10 @@ rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
> 	if (*p != cpu_to_be32(RPC_CALL))
> 		return false;
> 
> +	/* No bc service. */
> +	if (xprt->bc_serv == NULL)
> +		return false;
> +
> 	/* Now that we are sure this is a backchannel call,
> 	 * advance to the RPC header.
> 	 */
> -- 
> 2.27.0
> 

--
Chuck Lever




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

* Re: [PATCH v2] xprtrdma: treat all calls not a bcall when bc_serv is NULL
  2022-05-22 15:52         ` Chuck Lever III
@ 2022-05-23  1:39           ` Kinglong Mee
  0 siblings, 0 replies; 8+ messages in thread
From: Kinglong Mee @ 2022-05-23  1:39 UTC (permalink / raw)
  To: Chuck Lever III; +Cc: Anna Schumaker, Linux NFS Mailing List



On 2022/5/22 11:52 PM, Chuck Lever III wrote:
> 
> 
>> On May 22, 2022, at 8:36 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>
>> When a rdma server returns a fault format reply, nfs v3 client may
>> treats it as a bcall when bc service is not exist.
>>
>> The debug message at rpcrdma_bc_receive_call are,
>>
>> [56579.837169] RPC:       rpcrdma_bc_receive_call: callback XID 00000001, length=20
>> [56579.837174] RPC:       rpcrdma_bc_receive_call: 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04
>>
>> After that, rpcrdma_bc_receive_call will meets NULL pointer as,
>>
>> [  226.057890] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c8
>> ...
>> [  226.058704] RIP: 0010:_raw_spin_lock+0xc/0x20
>> ...
>> [  226.059732] Call Trace:
>> [  226.059878]  rpcrdma_bc_receive_call+0x138/0x327 [rpcrdma]
>> [  226.060011]  __ib_process_cq+0x89/0x170 [ib_core]
>> [  226.060092]  ib_cq_poll_work+0x26/0x80 [ib_core]
>> [  226.060257]  process_one_work+0x1a7/0x360
>> [  226.060367]  ? create_worker+0x1a0/0x1a0
>> [  226.060440]  worker_thread+0x30/0x390
>> [  226.060500]  ? create_worker+0x1a0/0x1a0
>> [  226.060574]  kthread+0x116/0x130
>> [  226.060661]  ? kthread_flush_work_fn+0x10/0x10
>> [  226.060724]  ret_from_fork+0x35/0x40
>> ...
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> 
> Patch is good, Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> 
> Commentary (no changes to v2 needed):
> 
> The description still suggests we are adapting the client to
> work around a broken server. I don't feel this is the right
> reason to accept this change. Instead: we really don't want
> the client to be vulnerable to bad input for any reason.
> After this fix is applied, bad RPC messages are eventually
> dropped rather than processed, which is the desired behavior.
> 
> Anna, I recommend adding Cc: stable for this fix.
> 
> Kinglong, please ensure that client Receive resources are not
> leaked in this case. If they are, send along an additional
> patch; if not, let us know and we can close this issue. 

I have double check of the code.
I think client Receive resources are not leaked here.
We can close it.

Thanks,
Kinglong Mee

> 
> 
>> ---
>> net/sunrpc/xprtrdma/rpc_rdma.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>> index 281ddb87ac8d..190a4de239c8 100644
>> --- a/net/sunrpc/xprtrdma/rpc_rdma.c
>> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c
>> @@ -1121,6 +1121,7 @@ static bool
>> rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
>> #if defined(CONFIG_SUNRPC_BACKCHANNEL)
>> {
>> +	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
>> 	struct xdr_stream *xdr = &rep->rr_stream;
>> 	__be32 *p;
>>
>> @@ -1144,6 +1145,10 @@ rpcrdma_is_bcall(struct rpcrdma_xprt *r_xprt, struct rpcrdma_rep *rep)
>> 	if (*p != cpu_to_be32(RPC_CALL))
>> 		return false;
>>
>> +	/* No bc service. */
>> +	if (xprt->bc_serv == NULL)
>> +		return false;
>> +
>> 	/* Now that we are sure this is a backchannel call,
>> 	 * advance to the RPC header.
>> 	 */
>> -- 
>> 2.27.0
>>
> 
> --
> Chuck Lever
> 
> 
> 

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

end of thread, other threads:[~2022-05-23  1:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-21  9:51 [PATCH] xprtrdam: Don't treat a call as bcall when bc_serv is NULL Kinglong Mee
2022-05-21 16:33 ` Chuck Lever III
2022-05-22  1:24   ` Kinglong Mee
2022-05-22  1:41     ` Chuck Lever III
2022-05-22 12:11       ` Kinglong Mee
2022-05-22 12:36       ` [PATCH v2] xprtrdma: treat all calls not a " Kinglong Mee
2022-05-22 15:52         ` Chuck Lever III
2022-05-23  1:39           ` Kinglong Mee

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.