diff mbox series

xprtrdam: Don't treat a call as bcall when bc_serv is NULL

Message ID 3d65c9a2-6c3e-7224-5701-c3e0060b6df6@gmail.com (mailing list archive)
State New
Headers show
Series xprtrdam: Don't treat a call as bcall when bc_serv is NULL | expand

Commit Message

Kinglong Mee May 21, 2022, 9:51 a.m. UTC
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(+)

Comments

Chuck Lever May 21, 2022, 4:33 p.m. UTC | #1
> 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
Kinglong Mee May 22, 2022, 1:24 a.m. UTC | #2
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
Chuck Lever May 22, 2022, 1:41 a.m. UTC | #3
> 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
Kinglong Mee May 22, 2022, 12:11 p.m. UTC | #4
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
diff mbox series

Patch

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;