[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Guoqing Jiang]

With commit 0ffdaf5b41cf ("net/sock: add WARN_ON(parent->sk)
in sock_graft()"), a calltrace happened as follows:

[  457.018340] WARNING: CPU: 0 PID: 15623 at ./include/net/sock.h:1703 inet_accept+0x135/0x140
...
[  457.018381] RIP: 0010:inet_accept+0x135/0x140
[  457.018381] RSP: 0018:ffffc90001727d18 EFLAGS: 00010286
[  457.018383] RAX: 0000000000000001 RBX: ffff880012413000 RCX: 0000000000000001
[  457.018384] RDX: 000000000000018a RSI: 00000000fffffe01 RDI: ffffffff8156fae8
[  457.018384] RBP: ffffc90001727d38 R08: 0000000000000000 R09: 0000000000004305
[  457.018385] R10: 0000000000000001 R11: 0000000000004304 R12: ffff880035ae7a00
[  457.018386] R13: ffff88001282af10 R14: ffff880034e4e200 R15: 0000000000000000
[  457.018387] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
[  457.018388] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  457.018389] CR2: 00007fdec22f9000 CR3: 0000000002b5a000 CR4: 00000000000006f0
[  457.018395] Call Trace:
[  457.018402]  tcp_accept_from_sock.part.8+0x12d/0x449 [dlm]
[  457.018405]  ? vprintk_emit+0x248/0x2d0
[  457.018409]  tcp_accept_from_sock+0x3f/0x50 [dlm]
[  457.018413]  process_recv_sockets+0x3b/0x50 [dlm]
[  457.018415]  process_one_work+0x138/0x370
[  457.018417]  worker_thread+0x4d/0x3b0
[  457.018419]  kthread+0x109/0x140
[  457.018421]  ? rescuer_thread+0x320/0x320
[  457.018422]  ? kthread_park+0x60/0x60
[  457.018424]  ret_from_fork+0x25/0x30

Since newsocket created by sock_create_kern sets it's
sock by the path:

	sock_create_kern -> __sock_creat
			 ->pf->create => inet_create
			 -> sock_init_data

Then WARN_ON is triggered by "con->sock->ops->accept =>
inet_accept -> sock_graft", it also means newsock->sk
is leaked since sock_graft will replace it with a new
sk.

To resolve the issue, we need to use sock_create_lite
instead of sock_create_kern, like commit 0933a578cd55
("rds: tcp: use sock_create_lite() to create the accept
socket") did.

Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
 fs/dlm/lowcomms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 9382db9..4813d0e 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
 	mutex_unlock(&connections_lock);
 
 	memset(&peeraddr, 0, sizeof(peeraddr));
-	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
+	result = sock_create_lite(dlm_local_addr[0]->ss_family,
 				  SOCK_STREAM, IPPROTO_TCP, &newsock);
 	if (result < 0)
 		return -ENOMEM;
-- 
2.10.0

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Steven Whitehouse]

Hi,


On 07/08/17 07:31, Guoqing Jiang wrote:
> With commit 0ffdaf5b41cf ("net/sock: add WARN_ON(parent->sk)
> in sock_graft()"), a calltrace happened as follows:
>
> [  457.018340] WARNING: CPU: 0 PID: 15623 at ./include/net/sock.h:1703 inet_accept+0x135/0x140
> ...
> [  457.018381] RIP: 0010:inet_accept+0x135/0x140
> [  457.018381] RSP: 0018:ffffc90001727d18 EFLAGS: 00010286
> [  457.018383] RAX: 0000000000000001 RBX: ffff880012413000 RCX: 0000000000000001
> [  457.018384] RDX: 000000000000018a RSI: 00000000fffffe01 RDI: ffffffff8156fae8
> [  457.018384] RBP: ffffc90001727d38 R08: 0000000000000000 R09: 0000000000004305
> [  457.018385] R10: 0000000000000001 R11: 0000000000004304 R12: ffff880035ae7a00
> [  457.018386] R13: ffff88001282af10 R14: ffff880034e4e200 R15: 0000000000000000
> [  457.018387] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
> [  457.018388] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  457.018389] CR2: 00007fdec22f9000 CR3: 0000000002b5a000 CR4: 00000000000006f0
> [  457.018395] Call Trace:
> [  457.018402]  tcp_accept_from_sock.part.8+0x12d/0x449 [dlm]
> [  457.018405]  ? vprintk_emit+0x248/0x2d0
> [  457.018409]  tcp_accept_from_sock+0x3f/0x50 [dlm]
> [  457.018413]  process_recv_sockets+0x3b/0x50 [dlm]
> [  457.018415]  process_one_work+0x138/0x370
> [  457.018417]  worker_thread+0x4d/0x3b0
> [  457.018419]  kthread+0x109/0x140
> [  457.018421]  ? rescuer_thread+0x320/0x320
> [  457.018422]  ? kthread_park+0x60/0x60
> [  457.018424]  ret_from_fork+0x25/0x30
>
> Since newsocket created by sock_create_kern sets it's
> sock by the path:
>
> 	sock_create_kern -> __sock_creat
> 			 ->pf->create => inet_create
> 			 -> sock_init_data
>
> Then WARN_ON is triggered by "con->sock->ops->accept =>
> inet_accept -> sock_graft", it also means newsock->sk
> is leaked since sock_graft will replace it with a new
> sk.
>
> To resolve the issue, we need to use sock_create_lite
> instead of sock_create_kern, like commit 0933a578cd55
> ("rds: tcp: use sock_create_lite() to create the accept
> socket") did.
Good spotting!

Bob, is this the reason that you had so much trouble figuring out what 
was going on with the sk callbacks? You will need to review your patches 
for that I think, in case this makes a difference to them.

Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.

> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
>   fs/dlm/lowcomms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 9382db9..4813d0e 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
>   	mutex_unlock(&connections_lock);
>   
>   	memset(&peeraddr, 0, sizeof(peeraddr));
> -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
>   				  SOCK_STREAM, IPPROTO_TCP, &newsock);
>   	if (result < 0)
>   		return -ENOMEM;

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Zhilong Liu]

On 08/07/2017 02:31 PM, Guoqing Jiang wrote:
> With commit 0ffdaf5b41cf ("net/sock: add WARN_ON(parent->sk)
> in sock_graft()"), a calltrace happened as follows:
>
> [  457.018340] WARNING: CPU: 0 PID: 15623 at ./include/net/sock.h:1703 inet_accept+0x135/0x140
> ...
> [  457.018381] RIP: 0010:inet_accept+0x135/0x140
> [  457.018381] RSP: 0018:ffffc90001727d18 EFLAGS: 00010286
> [  457.018383] RAX: 0000000000000001 RBX: ffff880012413000 RCX: 0000000000000001
> [  457.018384] RDX: 000000000000018a RSI: 00000000fffffe01 RDI: ffffffff8156fae8
> [  457.018384] RBP: ffffc90001727d38 R08: 0000000000000000 R09: 0000000000004305
> [  457.018385] R10: 0000000000000001 R11: 0000000000004304 R12: ffff880035ae7a00
> [  457.018386] R13: ffff88001282af10 R14: ffff880034e4e200 R15: 0000000000000000
> [  457.018387] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:0000000000000000
> [  457.018388] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  457.018389] CR2: 00007fdec22f9000 CR3: 0000000002b5a000 CR4: 00000000000006f0
> [  457.018395] Call Trace:
> [  457.018402]  tcp_accept_from_sock.part.8+0x12d/0x449 [dlm]
> [  457.018405]  ? vprintk_emit+0x248/0x2d0
> [  457.018409]  tcp_accept_from_sock+0x3f/0x50 [dlm]
> [  457.018413]  process_recv_sockets+0x3b/0x50 [dlm]
> [  457.018415]  process_one_work+0x138/0x370
> [  457.018417]  worker_thread+0x4d/0x3b0
> [  457.018419]  kthread+0x109/0x140
> [  457.018421]  ? rescuer_thread+0x320/0x320
> [  457.018422]  ? kthread_park+0x60/0x60
> [  457.018424]  ret_from_fork+0x25/0x30
>
> Since newsocket created by sock_create_kern sets it's
> sock by the path:
>
> 	sock_create_kern -> __sock_creat
> 			 ->pf->create => inet_create
> 			 -> sock_init_data
>
> Then WARN_ON is triggered by "con->sock->ops->accept =>
> inet_accept -> sock_graft", it also means newsock->sk
> is leaked since sock_graft will replace it with a new
> sk.
>
> To resolve the issue, we need to use sock_create_lite
> instead of sock_create_kern, like commit 0933a578cd55
> ("rds: tcp: use sock_create_lite() to create the accept
> socket") did.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>

Reported-by: Zhilong Liu <zlliu@suse.com>

> ---
>   fs/dlm/lowcomms.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 9382db9..4813d0e 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
>   	mutex_unlock(&connections_lock);
>   
>   	memset(&peeraddr, 0, sizeof(peeraddr));
> -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
>   				  SOCK_STREAM, IPPROTO_TCP, &newsock);
>   	if (result < 0)
>   		return -ENOMEM;

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[David Teigland]

On Mon, Aug 07, 2017 at 02:31:20PM +0800, Guoqing Jiang wrote:
> To resolve the issue, we need to use sock_create_lite
> instead of sock_create_kern, like commit 0933a578cd55
> ("rds: tcp: use sock_create_lite() to create the accept
> socket") did.

Thanks, this is now in linux-dlm next.
Dave

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Bob Peterson]

----- Original Message -----
| With commit 0ffdaf5b41cf ("net/sock: add WARN_ON(parent->sk)
| in sock_graft()"), a calltrace happened as follows:
| 
| [  457.018340] WARNING: CPU: 0 PID: 15623 at ./include/net/sock.h:1703
| inet_accept+0x135/0x140
| ...
| [  457.018381] RIP: 0010:inet_accept+0x135/0x140
| [  457.018381] RSP: 0018:ffffc90001727d18 EFLAGS: 00010286
| [  457.018383] RAX: 0000000000000001 RBX: ffff880012413000 RCX:
| 0000000000000001
| [  457.018384] RDX: 000000000000018a RSI: 00000000fffffe01 RDI:
| ffffffff8156fae8
| [  457.018384] RBP: ffffc90001727d38 R08: 0000000000000000 R09:
| 0000000000004305
| [  457.018385] R10: 0000000000000001 R11: 0000000000004304 R12:
| ffff880035ae7a00
| [  457.018386] R13: ffff88001282af10 R14: ffff880034e4e200 R15:
| 0000000000000000
| [  457.018387] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000)
| knlGS:0000000000000000
| [  457.018388] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
| [  457.018389] CR2: 00007fdec22f9000 CR3: 0000000002b5a000 CR4:
| 00000000000006f0
| [  457.018395] Call Trace:
| [  457.018402]  tcp_accept_from_sock.part.8+0x12d/0x449 [dlm]
| [  457.018405]  ? vprintk_emit+0x248/0x2d0
| [  457.018409]  tcp_accept_from_sock+0x3f/0x50 [dlm]
| [  457.018413]  process_recv_sockets+0x3b/0x50 [dlm]
| [  457.018415]  process_one_work+0x138/0x370
| [  457.018417]  worker_thread+0x4d/0x3b0
| [  457.018419]  kthread+0x109/0x140
| [  457.018421]  ? rescuer_thread+0x320/0x320
| [  457.018422]  ? kthread_park+0x60/0x60
| [  457.018424]  ret_from_fork+0x25/0x30
| 
| Since newsocket created by sock_create_kern sets it's
| sock by the path:
| 
| 	sock_create_kern -> __sock_creat
| 			 ->pf->create => inet_create
| 			 -> sock_init_data
| 
| Then WARN_ON is triggered by "con->sock->ops->accept =>
| inet_accept -> sock_graft", it also means newsock->sk
| is leaked since sock_graft will replace it with a new
| sk.
| 
| To resolve the issue, we need to use sock_create_lite
| instead of sock_create_kern, like commit 0933a578cd55
| ("rds: tcp: use sock_create_lite() to create the accept
| socket") did.
| 
| Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
| ---
|  fs/dlm/lowcomms.c | 2 +-
|  1 file changed, 1 insertion(+), 1 deletion(-)
| 
| diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
| index 9382db9..4813d0e 100644
| --- a/fs/dlm/lowcomms.c
| +++ b/fs/dlm/lowcomms.c
| @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
|  	mutex_unlock(&connections_lock);
|  
|  	memset(&peeraddr, 0, sizeof(peeraddr));
| -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
| +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
|  				  SOCK_STREAM, IPPROTO_TCP, &newsock);
|  	if (result < 0)
|  		return -ENOMEM;
| --
| 2.10.0
| 
| 

Isn't this also a problem for the sctp equivalent, sctp_connect_to_sock?

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Bob Peterson]

| | Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
| | ---
| |  fs/dlm/lowcomms.c | 2 +-
| |  1 file changed, 1 insertion(+), 1 deletion(-)
| | 
| | diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
| | index 9382db9..4813d0e 100644
| | --- a/fs/dlm/lowcomms.c
| | +++ b/fs/dlm/lowcomms.c
| | @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
| |  	mutex_unlock(&connections_lock);
| |  
| |  	memset(&peeraddr, 0, sizeof(peeraddr));
| | -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
| | +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
| |  				  SOCK_STREAM, IPPROTO_TCP, &newsock);
| |  	if (result < 0)
| |  		return -ENOMEM;
| 
| Isn't this also a problem for the sctp equivalent, sctp_connect_to_sock?
| 
| Regards,
| 
| Bob Peterson
| Red Hat File Systems
| 

In fact, I see 5 different calls to sock_create_kern in DLM.
Shouldn't it be done to all of them?

One could also argue that sock_create_kern should itself be fixed,
not its callers.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Guoqing Jiang]

On 08/08/2017 03:10 AM, Bob Peterson wrote:
> | | Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> | | ---
> | |  fs/dlm/lowcomms.c | 2 +-
> | |  1 file changed, 1 insertion(+), 1 deletion(-)
> | |
> | | diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> | | index 9382db9..4813d0e 100644
> | | --- a/fs/dlm/lowcomms.c
> | | +++ b/fs/dlm/lowcomms.c
> | | @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
> | |  	mutex_unlock(&connections_lock);
> | |
> | |  	memset(&peeraddr, 0, sizeof(peeraddr));
> | | -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> | | +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
> | |  				  SOCK_STREAM, IPPROTO_TCP, &newsock);
> | |  	if (result < 0)
> | |  		return -ENOMEM;
> |
> | Isn't this also a problem for the sctp equivalent, sctp_connect_to_sock?
> |
> | Regards,
> |
> | Bob Peterson
> | Red Hat File Systems
> |
>
> In fact, I see 5 different calls to sock_create_kern in DLM.
> Shouldn't it be done to all of them?

Only this one called accept immediately after the socket is created, so 
others
probably are safe. Plus, the sock is used after sock_create_kern, so I 
am not
sure it can be replaced with sock_create_lite.

         result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
                                   SOCK_STREAM, IPPROTO_SCTP, &sock);
         ...
         sock->sk->sk_user_data = con;

> One could also argue that sock_create_kern should itself be fixed,
> not its callers.

Pls see https://patchwork.ozlabs.org/patch/780356/ for more infos.

Thanks,
GUoqing

[Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock

[Steven Whitehouse]

Hi,


On 07/08/17 20:10, Bob Peterson wrote:
> | | Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> | | ---
> | |  fs/dlm/lowcomms.c | 2 +-
> | |  1 file changed, 1 insertion(+), 1 deletion(-)
> | |
> | | diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> | | index 9382db9..4813d0e 100644
> | | --- a/fs/dlm/lowcomms.c
> | | +++ b/fs/dlm/lowcomms.c
> | | @@ -729,7 +729,7 @@ static int tcp_accept_from_sock(struct connection *con)
> | |  	mutex_unlock(&connections_lock);
> | |
> | |  	memset(&peeraddr, 0, sizeof(peeraddr));
> | | -	result = sock_create_kern(&init_net, dlm_local_addr[0]->ss_family,
> | | +	result = sock_create_lite(dlm_local_addr[0]->ss_family,
> | |  				  SOCK_STREAM, IPPROTO_TCP, &newsock);
> | |  	if (result < 0)
> | |  		return -ENOMEM;
> |
> | Isn't this also a problem for the sctp equivalent, sctp_connect_to_sock?
> |
> | Regards,
> |
> | Bob Peterson
> | Red Hat File Systems
> |
>
> In fact, I see 5 different calls to sock_create_kern in DLM.
> Shouldn't it be done to all of them?
>
> One could also argue that sock_create_kern should itself be fixed,
> not its callers.
The two functions do different things. Normally you want to create both 
a struct socket and a struct sock, which is what sock_create_kern does. 
In accept though, you need to pass in a struct socket, and the struct 
sock is created during accept and grafted on to it. That is why you were 
having such trouble with the callbacks, because it was leaking the 
struct sock that was originally created by sock_create_kern. Since DLM 
usually doesn't make many connections, that would not have shown up in 
any greatly increased memory consumption at any stage.

If you look at sctp, it calls kernel_accept() which uses 
sock_create_lite anyway, so that is already correct. We should probably 
be using that helper for the tcp case too, which would be perhaps a 
better way to resolve that problem, since it reduces code duplication,

Steve.

> Regards,
>
> Bob Peterson
> Red Hat File Systems
>