Skip to contentSkip to conversation

tipc: fix oops when creating server socket fails

PATCH Superseded Paul Gortmaker 2 participants 4 messages Jul 30, 2013 – Aug 1, 2013

Subsystems (3)
Networking Network drivers TIPC network layer
lore
Share & tools
lore

Paul Gortmaker

author
#0 lore raw

PATCH 1/1 tipc: fix oops when creating server socket fails

From: Ying Xue <[email protected]>

When creation of TIPC internal server socket fails, we get an oops with the following dump:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
IP: [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
PGD 13719067 PUD 12008067 PMD 0
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: tipc(+)
CPU: 4 PID: 4340 Comm: insmod Not tainted 3.10.0+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
task: ffff880014360000 ti: ffff88001374c000 task.ti: ffff88001374c000
RIP: 0010:[<ffffffffa0011f49>]  [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
RSP: 0018:ffff88001374dc98  EFLAGS: 00010292
RAX: 0000000000000000 RBX: ffff880012ac09d8 RCX: 0000000000000000
RDX: 0000000000000046 RSI: 0000000000000001 RDI: ffff880014360000
RBP: ffff88001374dcb8 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa0016fa0
R13: ffffffffa0017010 R14: ffffffffa0017010 R15: ffff880012ac09d8
FS:  0000000000000000(0000) GS:ffff880016600000(0063) knlGS:00000000f76668d0
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000020 CR3: 0000000012227000 CR4: 00000000000006e0
Stack:
ffff88001374dcb8 ffffffffa0016fa0 0000000000000000 0000000000000001
ffff88001374dcf8 ffffffffa0012922 ffff88001374dce8 00000000ffffffea
ffffffffa0017100 0000000000000000 ffff8800134241a8 ffffffffa0017150
Call Trace:
[<ffffffffa0012922>] tipc_server_stop+0xa2/0x1b0 [tipc]
[<ffffffffa0009995>] tipc_subscr_stop+0x15/0x20 [tipc]
[<ffffffffa00130f5>] tipc_core_stop+0x1d/0x33 [tipc]
[<ffffffffa001f0d4>] tipc_init+0xd4/0xf8 [tipc]
[<ffffffffa001f000>] ? 0xffffffffa001efff
[<ffffffff8100023f>] do_one_initcall+0x3f/0x150
[<ffffffff81082f4d>] ? __blocking_notifier_call_chain+0x7d/0xd0
[<ffffffff810cc58a>] load_module+0x11aa/0x19c0
[<ffffffff810c8d60>] ? show_initstate+0x50/0x50
[<ffffffff8190311c>] ? retint_restore_args+0xe/0xe
[<ffffffff810cce79>] SyS_init_module+0xd9/0x110
[<ffffffff8190dc65>] sysenter_dispatch+0x7/0x1f
Code: 6c 24 70 4c 89 ef e8 b7 04 8f e1 8b 73 04 4c 89 e7 e8 7c 9e 32 e1 41 83 ac 24

b8 00 00 00 01 4c 89 ef e8 eb 0a 8f e1 48 8b 43 08 <4c> 8b 68 20 4d 8d a5 48 03 00 00 4c 89 e7 e8 04 05 8f e1 4c 89
RIP [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
RSP <ffff88001374dc98>
CR2: 0000000000000020

---[ end trace b02321f40e4269a3 ]---

We have the following call chain:

tipc_core_start()

    ret = tipc_subscr_start()
        ret = tipc_server_start(){
                  server->enabled = 1;
                  ret = tipc_open_listening_sock()
              }

I.e., the server->enabled flag is unconditionally set to 1, whatever the return value of tipc_open_listening_sock().

This causes a crash when tipc_core_start() tries to clean up resources after a failed initialization:

    if (ret == failed)
        tipc_subscr_stop()
            tipc_server_stop(){
                if (server->enabled)
                    tipc_close_conn(){
                        NULL reference of con->sock-sk
                        OOPS!
                }
            }

To avoid this, tipc_server_start() should only set server->enabled to 1 in case of a succesful socket creation. In case of failure, it should release all allocated resources before returning.

Problem introduced as of v3.11-rc1~16^2~73^2~12 -- note that it won't be seen often; it takes a module load under memory constrained conditions in order to trigger the failure condition.

Signed-off-by Ying Xue <[email protected]>
Signed-off-by Jon Maloy <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>
---
 net/tipc/server.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
net/tipc/server.c +12 -3
index 19da5ab..fd3fa57 100644
@@ -355,8 +355,12 @@ static int tipc_open_listening_sock(struct tipc_server *s)
 		return PTR_ERR(con);
 
 	sock = tipc_create_listen_sock(con);
-	if (!sock)
+	if (!sock) {
+		idr_remove(&s->conn_idr, con->conid);
+		s->idr_in_use--;
+		kfree(con);
 		return -EINVAL;
+	}
 
 	tipc_register_callbacks(sock, con);
 	return 0;

@@ -563,9 +567,14 @@ int tipc_server_start(struct tipc_server *s)
 		kmem_cache_destroy(s->rcvbuf_cache);
 		return ret;
 	}
+	ret = tipc_open_listening_sock(s);
+	if (ret < 0) {
+		tipc_work_stop(s);
+		kmem_cache_destroy(s->rcvbuf_cache);
+		return ret;
+	}
 	s->enabled = 1;
-
-	return tipc_open_listening_sock(s);
+	return ret;
 }
 
 void tipc_server_stop(struct tipc_server *s)
Signature · 1 line
1.8.1.2
maintainer
#1 lore raw

From: Paul Gortmaker <[email protected]>
Date: Tue, 30 Jul 2013 19:46:35 -0400

Quoted 1 line

Problem introduced as of v3.11-rc1~16^2~73^2~12

Please do not refer to commits this way.

Use the format:

SHA1_ID ("Commit message header line")

thank you.

author
#2 lore raw

PATCH v2 1/1 tipc: fix oops when creating server socket fails

From: Ying Xue <[email protected]>

When creation of TIPC internal server socket fails, we get an oops with the following dump:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
IP: [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
PGD 13719067 PUD 12008067 PMD 0
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: tipc(+)
CPU: 4 PID: 4340 Comm: insmod Not tainted 3.10.0+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
task: ffff880014360000 ti: ffff88001374c000 task.ti: ffff88001374c000
RIP: 0010:[<ffffffffa0011f49>]  [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
RSP: 0018:ffff88001374dc98  EFLAGS: 00010292
RAX: 0000000000000000 RBX: ffff880012ac09d8 RCX: 0000000000000000
RDX: 0000000000000046 RSI: 0000000000000001 RDI: ffff880014360000
RBP: ffff88001374dcb8 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffffa0016fa0
R13: ffffffffa0017010 R14: ffffffffa0017010 R15: ffff880012ac09d8
FS:  0000000000000000(0000) GS:ffff880016600000(0063) knlGS:00000000f76668d0
CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
CR2: 0000000000000020 CR3: 0000000012227000 CR4: 00000000000006e0
Stack:
ffff88001374dcb8 ffffffffa0016fa0 0000000000000000 0000000000000001
ffff88001374dcf8 ffffffffa0012922 ffff88001374dce8 00000000ffffffea
ffffffffa0017100 0000000000000000 ffff8800134241a8 ffffffffa0017150
Call Trace:
[<ffffffffa0012922>] tipc_server_stop+0xa2/0x1b0 [tipc]
[<ffffffffa0009995>] tipc_subscr_stop+0x15/0x20 [tipc]
[<ffffffffa00130f5>] tipc_core_stop+0x1d/0x33 [tipc]
[<ffffffffa001f0d4>] tipc_init+0xd4/0xf8 [tipc]
[<ffffffffa001f000>] ? 0xffffffffa001efff
[<ffffffff8100023f>] do_one_initcall+0x3f/0x150
[<ffffffff81082f4d>] ? __blocking_notifier_call_chain+0x7d/0xd0
[<ffffffff810cc58a>] load_module+0x11aa/0x19c0
[<ffffffff810c8d60>] ? show_initstate+0x50/0x50
[<ffffffff8190311c>] ? retint_restore_args+0xe/0xe
[<ffffffff810cce79>] SyS_init_module+0xd9/0x110
[<ffffffff8190dc65>] sysenter_dispatch+0x7/0x1f
Code: 6c 24 70 4c 89 ef e8 b7 04 8f e1 8b 73 04 4c 89 e7 e8 7c 9e 32 e1 41 83 ac 24

b8 00 00 00 01 4c 89 ef e8 eb 0a 8f e1 48 8b 43 08 <4c> 8b 68 20 4d 8d a5 48 03 00 00 4c 89 e7 e8 04 05 8f e1 4c 89
RIP [<ffffffffa0011f49>] tipc_close_conn+0x59/0xb0 [tipc]
RSP <ffff88001374dc98>
CR2: 0000000000000020

---[ end trace b02321f40e4269a3 ]---

We have the following call chain:

tipc_core_start()

    ret = tipc_subscr_start()
        ret = tipc_server_start(){
                  server->enabled = 1;
                  ret = tipc_open_listening_sock()
              }

I.e., the server->enabled flag is unconditionally set to 1, whatever the return value of tipc_open_listening_sock().

This causes a crash when tipc_core_start() tries to clean up resources after a failed initialization:

    if (ret == failed)
        tipc_subscr_stop()
            tipc_server_stop(){
                if (server->enabled)
                    tipc_close_conn(){
                        NULL reference of con->sock-sk
                        OOPS!
                }
            }

To avoid this, tipc_server_start() should only set server->enabled to 1 in case of a succesful socket creation. In case of failure, it should release all allocated resources before returning.

Problem introduced in commit c5fa7b3cf3cb22e4ac60485fc2dc187fe012910f ("tipc: introduce new TIPC server infrastructure") in v3.11-rc1. Note that it won't be seen often; it takes a module load under memory constrained conditions in order to trigger the failure condition.

Signed-off-by Ying Xue <[email protected]>
Signed-off-by Jon Maloy <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>

---

[v2: switch back to conventional ID ("shortlog") reference in log]

 net/tipc/server.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
net/tipc/server.c +12 -3
index 19da5ab..fd3fa57 100644
@@ -355,8 +355,12 @@ static int tipc_open_listening_sock(struct tipc_server *s)
 		return PTR_ERR(con);
 
 	sock = tipc_create_listen_sock(con);
-	if (!sock)
+	if (!sock) {
+		idr_remove(&s->conn_idr, con->conid);
+		s->idr_in_use--;
+		kfree(con);
 		return -EINVAL;
+	}
 
 	tipc_register_callbacks(sock, con);
 	return 0;

@@ -563,9 +567,14 @@ int tipc_server_start(struct tipc_server *s)
 		kmem_cache_destroy(s->rcvbuf_cache);
 		return ret;
 	}
+	ret = tipc_open_listening_sock(s);
+	if (ret < 0) {
+		tipc_work_stop(s);
+		kmem_cache_destroy(s->rcvbuf_cache);
+		return ret;
+	}
 	s->enabled = 1;
-
-	return tipc_open_listening_sock(s);
+	return ret;
 }
 
 void tipc_server_stop(struct tipc_server *s)
Signature · 1 line
1.8.1.2