Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
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:c90001727d18 EFLAGS: 00010286 [ 457.018383] RAX: 0001 RBX: 880012413000 RCX: 0001 [ 457.018384] RDX: 018a RSI: fe01 RDI: 8156fae8 [ 457.018384] RBP: c90001727d38 R08: R09: 4305 [ 457.018385] R10: 0001 R11: 4304 R12: 880035ae7a00 [ 457.018386] R13: 88001282af10 R14: 880034e4e200 R15: [ 457.018387] FS: () GS:88003fc0() knlGS: [ 457.018388] CS: 0010 DS: ES: CR0: 80050033 [ 457.018389] CR2: 7fdec22f9000 CR3: 02b5a000 CR4: 06f0 [ 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 Steve. Signed-off-by: Guoqing Jiang --- 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;
Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
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:c90001727d18 EFLAGS: 00010286 [ 457.018383] RAX: 0001 RBX: 880012413000 RCX: 0001 [ 457.018384] RDX: 018a RSI: fe01 RDI: 8156fae8 [ 457.018384] RBP: c90001727d38 R08: R09: 4305 [ 457.018385] R10: 0001 R11: 4304 R12: 880035ae7a00 [ 457.018386] R13: 88001282af10 R14: 880034e4e200 R15: [ 457.018387] FS: () GS:88003fc0() knlGS: [ 457.018388] CS: 0010 DS: ES: CR0: 80050033 [ 457.018389] CR2: 7fdec22f9000 CR3: 02b5a000 CR4: 06f0 [ 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 Reported-by: Zhilong Liu --- 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 v06 16/36] uapi linux/dlm_netlink.h: include linux/dlmconstants.h
Fixes userspace compilation error: error: ‘DLM_RESNAME_MAXLEN’ undeclared here (not in a function) char resource_name[DLM_RESNAME_MAXLEN]; Signed-off-by: Mikko Rapeli Cc: Christine Caulfield Cc: David Teigland Cc: cluster-devel@redhat.com --- include/uapi/linux/dlm_netlink.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/uapi/linux/dlm_netlink.h b/include/uapi/linux/dlm_netlink.h index 647c8ef27227..ef1e2e08769a 100644 --- a/include/uapi/linux/dlm_netlink.h +++ b/include/uapi/linux/dlm_netlink.h @@ -10,6 +10,7 @@ #define _DLM_NETLINK_H #include +#include enum { DLM_STATUS_WAITING = 1, -- 2.13.3
Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
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
Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
- 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:c90001727d18 EFLAGS: 00010286 | [ 457.018383] RAX: 0001 RBX: 880012413000 RCX: | 0001 | [ 457.018384] RDX: 018a RSI: fe01 RDI: | 8156fae8 | [ 457.018384] RBP: c90001727d38 R08: R09: | 4305 | [ 457.018385] R10: 0001 R11: 4304 R12: | 880035ae7a00 | [ 457.018386] R13: 88001282af10 R14: 880034e4e200 R15: | | [ 457.018387] FS: () GS:88003fc0() | knlGS: | [ 457.018388] CS: 0010 DS: ES: CR0: 80050033 | [ 457.018389] CR2: 7fdec22f9000 CR3: 02b5a000 CR4: | 06f0 | [ 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 | --- | 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
Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
| | Signed-off-by: Guoqing Jiang | | --- | | 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
Re: [Cluster-devel] [PATCH] dlm: use sock_create_lite inside tcp_accept_from_sock
On 08/08/2017 03:10 AM, Bob Peterson wrote: | | Signed-off-by: Guoqing Jiang | | --- | | 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