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

[Santosh Shilimkar]

On 3/2/2018 6:42 AM, David Miller wrote:

From: "santosh.shilim...@oracle.com" 
Date: Thu, 1 Mar 2018 22:22:07 -0800


Versioning comment typically goes below "---" and not part of
commit message.


I like them to be in the commit message most of the time.

Especially for patch series header postings.


Series header I have seen it followed but not in patch commit since
it goes into kernel git history.


Later if someone reviews the patch and says "why didn't they do X" and
if it says in the version history text "don't do X based upon feedback
from developer Y" that helps a lot.


I agree the versioning info helps, but didn't know that you like that
to be in commit message of patches as well. Will keep that in mind for
future for netdev list.

Regards,
Santosh

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

[David Miller]

From: "santosh.shilim...@oracle.com" 
Date: Thu, 1 Mar 2018 22:22:07 -0800

> Versioning comment typically goes below "---" and not part of
> commit message.

I like them to be in the commit message most of the time.

Especially for patch series header postings.

Later if someone reviews the patch and says "why didn't they do X" and
if it says in the version history text "don't do X based upon feedback
from developer Y" that helps a lot.

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

[David Miller]

From: Ka-Cheong Poon 
Date: Thu,  1 Mar 2018 21:07:18 -0800

> 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.
> 
> v2: Update comments
> 
> Signed-off-by: Ka-Cheong Poon 

Applied and queued up for -stable.

Please provide a proper, appropriate, "Fixes: " tag next time.
I added it for you in this case.

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

[Sowmini Varadhan]

On (03/01/18 21:07), Ka-Cheong Poon 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.

Acked-by: Sowmini Varadhan 

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

[santosh.shilim...@oracle.com]

On 3/1/18 9:07 PM, Ka-Cheong Poon 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.

v2: Update comments


Versioning comment typically goes below "---" and not part of
commit message.


Signed-off-by: Ka-Cheong Poon 
---

Patch looks fine.
Acked-by: Santosh Shilimkar 

[PATCH v2 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.

v2: Update comments

Signed-off-by: Ka-Cheong Poon 
---
 net/rds/tcp_listen.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index c061d6e..2257118 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,20 @@ 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;
 
+   /* sock_create_lite() does not get a hold on the owner module so we
+* need to do it here.  Note that sock_release() uses sock->ops to
+* determine if it needs to decrement the reference count.  So set
+* sock->ops after calling accept() in case that fails.  And there's
+* no need to do try_module_get() as the listener should have a hold
+* already.
+*/
+   new_sock->ops = sock->ops;
+   __module_get(new_sock->ops->owner);
+
ret = rds_tcp_keepalive(new_sock);
if (ret < 0)
goto out;
-- 
1.8.3.1