* [PATCH net] rds: Incorrect reference counting in TCP socket creation
@ 2018-03-01 4:44 Ka-Cheong Poon
2018-03-01 11:54 ` Sowmini Varadhan
0 siblings, 1 reply; 4+ messages in thread
From: Ka-Cheong Poon @ 2018-03-01 4:44 UTC (permalink / raw)
To: netdev; +Cc: santosh.shilimkar, davem, rds-devel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation
2018-03-01 4:44 [PATCH net] rds: Incorrect reference counting in TCP socket creation Ka-Cheong Poon
@ 2018-03-01 11:54 ` Sowmini Varadhan
2018-03-01 12:19 ` Ka-Cheong Poon
0 siblings, 1 reply; 4+ messages in thread
From: Sowmini Varadhan @ 2018-03-01 11:54 UTC (permalink / raw)
To: Ka-Cheong Poon
Cc: netdev, santosh.shilimkar, David Miller, rds-devel, 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation
2018-03-01 11:54 ` Sowmini Varadhan
@ 2018-03-01 12:19 ` Ka-Cheong Poon
2018-03-01 12:31 ` Sowmini Varadhan
0 siblings, 1 reply; 4+ messages in thread
From: Ka-Cheong Poon @ 2018-03-01 12:19 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: netdev, santosh.shilimkar, David Miller, rds-devel, Sowmini Varadhan
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation
2018-03-01 12:19 ` Ka-Cheong Poon
@ 2018-03-01 12:31 ` Sowmini Varadhan
0 siblings, 0 replies; 4+ messages in thread
From: Sowmini Varadhan @ 2018-03-01 12:31 UTC (permalink / raw)
To: Ka-Cheong Poon
Cc: Sowmini Varadhan, netdev, santosh.shilimkar, David Miller, rds-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-03-01 12:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 4:44 [PATCH net] rds: Incorrect reference counting in TCP socket creation Ka-Cheong Poon
2018-03-01 11:54 ` Sowmini Varadhan
2018-03-01 12:19 ` Ka-Cheong Poon
2018-03-01 12:31 ` Sowmini Varadhan
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.