[PATCH net] rds: Incorrect reference counting in TCP socket creation

[PATCH net] rds: Incorrect reference counting in TCP socket creation

[Ka-Cheong Poon]

Commit 0933a578cd55 ("rds: tcp: use sock_create_lite() to create the
accept socket") has a reference counting issue in TCP socket creation
when accepting a new connection.  The code uses sock_create_lite() to
create a kernel socket.  But it does not do __module_get() on the
socket owner.  When the connection is shutdown and sock_release() is
called to free the socket, the owner's reference count is decremented
and becomes incorrect.  Note that this bug only shows up when the socket
owner is configured as a kernel module.

Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com>
---
 net/rds/tcp_listen.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index c061d6e..d62c986 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006 Oracle.  All rights reserved.
+ * Copyright (c) 2006, 2018 Oracle.  All rights reserved.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -142,12 +142,17 @@ int rds_tcp_accept_one(struct socket *sock)
 	if (ret)
 		goto out;
 
-	new_sock->type = sock->type;
-	new_sock->ops = sock->ops;
 	ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true);
 	if (ret < 0)
 		goto out;
 
+	new_sock->ops = sock->ops;
+	/* sock_create_lite() does not get a hold on the owner module so we
+	 * need to do it here.  No need to do try_module_get() as the listener
+	 * should have a hold already.
+	 */
+	__module_get(new_sock->ops->owner);
+
 	ret = rds_tcp_keepalive(new_sock);
 	if (ret < 0)
 		goto out;
-- 
1.8.3.1

Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation

[Sowmini Varadhan]

On Wed, Feb 28, 2018 at 11:44 PM, Ka-Cheong Poon
<ka-cheong.poon@oracle.com> wrote:
> Commit 0933a578cd55 ("rds: tcp: use sock_create_lite() to create the
> accept socket") has a reference counting issue in TCP socket creation
> when accepting a new connection.  The code uses sock_create_lite() to
> create a kernel socket.  But it does not do __module_get() on the
> socket owner.  When the connection is shutdown and sock_release() is
> called to free the socket, the owner's reference count is decremented
> and becomes incorrect.  Note that this bug only shows up when the socket
> owner is configured as a kernel module.

>
> -       new_sock->type = sock->type;
> -       new_sock->ops = sock->ops;
>         ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true);
>         if (ret < 0)
>                 goto out;
>
> +       new_sock->ops = sock->ops;

How is this delta relevant to the commit comment? Seems unrelated?

--Sowmini

Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation

[Ka-Cheong Poon]

On 03/01/2018 07:54 PM, Sowmini Varadhan wrote:
> On Wed, Feb 28, 2018 at 11:44 PM, Ka-Cheong Poon
> <ka-cheong.poon@oracle.com> wrote:
>> Commit 0933a578cd55 ("rds: tcp: use sock_create_lite() to create the
>> accept socket") has a reference counting issue in TCP socket creation
>> when accepting a new connection.  The code uses sock_create_lite() to
>> create a kernel socket.  But it does not do __module_get() on the
>> socket owner.  When the connection is shutdown and sock_release() is
>> called to free the socket, the owner's reference count is decremented
>> and becomes incorrect.  Note that this bug only shows up when the socket
>> owner is configured as a kernel module.
> 
>>
>> -       new_sock->type = sock->type;
>> -       new_sock->ops = sock->ops;
>>          ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true);
>>          if (ret < 0)
>>                  goto out;
>>
>> +       new_sock->ops = sock->ops;
> 
> How is this delta relevant to the commit comment? Seems unrelated?


Note that sock_release() checks if sock->ops is set before
decrementing the refcnt.  By moving the ops assignment after
the ops->accept() call, we save increasing the refcnt in
case the ops->accept() fails.  Otherwise, the __module_get()
needs to be moved before ops->accept() to handle this failure
case.


-- 
K. Poon
ka-cheong.poon@oracle.com

Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation

[Sowmini Varadhan]

On (03/01/18 20:19), Ka-Cheong Poon wrote:
> >>
> >>-       new_sock->type = sock->type;
> >>-       new_sock->ops = sock->ops;
> >>         ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true);
> >>         if (ret < 0)
> >>                 goto out;
> >>
> >>+       new_sock->ops = sock->ops;
> >
> >How is this delta relevant to the commit comment? Seems unrelated?
> 
> 
> Note that sock_release() checks if sock->ops is set before
> decrementing the refcnt.  By moving the ops assignment after
> the ops->accept() call, we save increasing the refcnt in
> case the ops->accept() fails.  Otherwise, the __module_get()
> needs to be moved before ops->accept() to handle this failure
> case.

I see, thanks for clarification.

It may be helpful to have some comment in there, in case some other
module trips on something similar in the future.

--Sowmini