diff mbox series

net: avoid double iput when sock_alloc_file fails

Message ID 20230307173707.468744-1-cascardo@canonical.com (mailing list archive)
State Accepted
Commit 649c15c7691e9b13cbe9bf6c65c365350e056067
Delegated to: Netdev Maintainers
Headers show
Series net: avoid double iput when sock_alloc_file fails | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Thadeu Lima de Souza Cascardo March 7, 2023, 5:37 p.m. UTC 
When sock_alloc_file fails to allocate a file, it will call sock_release.
__sys_socket_file should then not call sock_release again, otherwise there
will be a double free.

[   89.319884] ------------[ cut here ]------------
[   89.320286] kernel BUG at fs/inode.c:1764!
[   89.320656] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[   89.321051] CPU: 7 PID: 125 Comm: iou-sqp-124 Not tainted 6.2.0+ #361
[   89.321535] RIP: 0010:iput+0x1ff/0x240
[   89.321808] Code: d1 83 e1 03 48 83 f9 02 75 09 48 81 fa 00 10 00 00 77 05 83 e2 01 75 1f 4c 89 ef e8 fb d2 ba 00 e9 80 fe ff ff c3 cc cc cc cc <0f> 0b 0f 0b e9 d0 fe ff ff 0f 0b eb 8d 49 8d b4 24 08 01 00 00 48
[   89.322760] RSP: 0018:ffffbdd60068bd50 EFLAGS: 00010202
[   89.323036] RAX: 0000000000000000 RBX: ffff9d7ad3cacac0 RCX: 0000000000001107
[   89.323412] RDX: 000000000003af00 RSI: 0000000000000000 RDI: ffff9d7ad3cacb40
[   89.323785] RBP: ffffbdd60068bd68 R08: ffffffffffffffff R09: ffffffffab606438
[   89.324157] R10: ffffffffacb3dfa0 R11: 6465686361657256 R12: ffff9d7ad3cacb40
[   89.324529] R13: 0000000080000001 R14: 0000000080000001 R15: 0000000000000002
[   89.324904] FS:  00007f7b28516740(0000) GS:ffff9d7aeb1c0000(0000) knlGS:0000000000000000
[   89.325328] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   89.325629] CR2: 00007f0af52e96c0 CR3: 0000000002a02006 CR4: 0000000000770ee0
[   89.326004] PKRU: 55555554
[   89.326161] Call Trace:
[   89.326298]  <TASK>
[   89.326419]  __sock_release+0xb5/0xc0
[   89.326632]  __sys_socket_file+0xb2/0xd0
[   89.326844]  io_socket+0x88/0x100
[   89.327039]  ? io_issue_sqe+0x6a/0x430
[   89.327258]  io_issue_sqe+0x67/0x430
[   89.327450]  io_submit_sqes+0x1fe/0x670
[   89.327661]  io_sq_thread+0x2e6/0x530
[   89.327859]  ? __pfx_autoremove_wake_function+0x10/0x10
[   89.328145]  ? __pfx_io_sq_thread+0x10/0x10
[   89.328367]  ret_from_fork+0x29/0x50
[   89.328576] RIP: 0033:0x0
[   89.328732] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[   89.329073] RSP: 002b:0000000000000000 EFLAGS: 00000202 ORIG_RAX: 00000000000001a9
[   89.329477] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f7b28637a3d
[   89.329845] RDX: 00007fff4e4318a8 RSI: 00007fff4e4318b0 RDI: 0000000000000400
[   89.330216] RBP: 00007fff4e431830 R08: 00007fff4e431711 R09: 00007fff4e4318b0
[   89.330584] R10: 0000000000000000 R11: 0000000000000202 R12: 00007fff4e441b38
[   89.330950] R13: 0000563835e3e725 R14: 0000563835e40d10 R15: 00007f7b28784040
[   89.331318]  </TASK>
[   89.331441] Modules linked in:
[   89.331617] ---[ end trace 0000000000000000 ]---

Fixes: da214a475f8b ("net: add __sys_socket_file()")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 net/socket.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Kuniyuki Iwashima March 7, 2023, 11:18 p.m. UTC | #1 
From:   Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Date:   Tue,  7 Mar 2023 14:37:07 -0300
> When sock_alloc_file fails to allocate a file, it will call sock_release.
> __sys_socket_file should then not call sock_release again, otherwise there
> will be a double free.
> 
> [   89.319884] ------------[ cut here ]------------
> [   89.320286] kernel BUG at fs/inode.c:1764!
> [   89.320656] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [   89.321051] CPU: 7 PID: 125 Comm: iou-sqp-124 Not tainted 6.2.0+ #361
> [   89.321535] RIP: 0010:iput+0x1ff/0x240
> [   89.321808] Code: d1 83 e1 03 48 83 f9 02 75 09 48 81 fa 00 10 00 00 77 05 83 e2 01 75 1f 4c 89 ef e8 fb d2 ba 00 e9 80 fe ff ff c3 cc cc cc cc <0f> 0b 0f 0b e9 d0 fe ff ff 0f 0b eb 8d 49 8d b4 24 08 01 00 00 48
> [   89.322760] RSP: 0018:ffffbdd60068bd50 EFLAGS: 00010202
> [   89.323036] RAX: 0000000000000000 RBX: ffff9d7ad3cacac0 RCX: 0000000000001107
> [   89.323412] RDX: 000000000003af00 RSI: 0000000000000000 RDI: ffff9d7ad3cacb40
> [   89.323785] RBP: ffffbdd60068bd68 R08: ffffffffffffffff R09: ffffffffab606438
> [   89.324157] R10: ffffffffacb3dfa0 R11: 6465686361657256 R12: ffff9d7ad3cacb40
> [   89.324529] R13: 0000000080000001 R14: 0000000080000001 R15: 0000000000000002
> [   89.324904] FS:  00007f7b28516740(0000) GS:ffff9d7aeb1c0000(0000) knlGS:0000000000000000
> [   89.325328] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   89.325629] CR2: 00007f0af52e96c0 CR3: 0000000002a02006 CR4: 0000000000770ee0
> [   89.326004] PKRU: 55555554
> [   89.326161] Call Trace:
> [   89.326298]  <TASK>
> [   89.326419]  __sock_release+0xb5/0xc0
> [   89.326632]  __sys_socket_file+0xb2/0xd0
> [   89.326844]  io_socket+0x88/0x100
> [   89.327039]  ? io_issue_sqe+0x6a/0x430
> [   89.327258]  io_issue_sqe+0x67/0x430
> [   89.327450]  io_submit_sqes+0x1fe/0x670
> [   89.327661]  io_sq_thread+0x2e6/0x530
> [   89.327859]  ? __pfx_autoremove_wake_function+0x10/0x10
> [   89.328145]  ? __pfx_io_sq_thread+0x10/0x10
> [   89.328367]  ret_from_fork+0x29/0x50
> [   89.328576] RIP: 0033:0x0
> [   89.328732] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
> [   89.329073] RSP: 002b:0000000000000000 EFLAGS: 00000202 ORIG_RAX: 00000000000001a9
> [   89.329477] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f7b28637a3d
> [   89.329845] RDX: 00007fff4e4318a8 RSI: 00007fff4e4318b0 RDI: 0000000000000400
> [   89.330216] RBP: 00007fff4e431830 R08: 00007fff4e431711 R09: 00007fff4e4318b0
> [   89.330584] R10: 0000000000000000 R11: 0000000000000202 R12: 00007fff4e441b38
> [   89.330950] R13: 0000563835e3e725 R14: 0000563835e40d10 R15: 00007f7b28784040
> [   89.331318]  </TASK>
> [   89.331441] Modules linked in:
> [   89.331617] ---[ end trace 0000000000000000 ]---
> 
> Fixes: da214a475f8b ("net: add __sys_socket_file()")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!


> ---
>  net/socket.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 6bae8ce7059e..9c92c0e6c4da 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -450,7 +450,9 @@ static struct file_system_type sock_fs_type = {
>   *
>   *	Returns the &file bound with @sock, implicitly storing it
>   *	in sock->file. If dname is %NULL, sets to "".
> - *	On failure the return is a ERR pointer (see linux/err.h).
> + *
> + *	On failure @sock is released, and an ERR pointer is returned.
> + *
>   *	This function uses GFP_KERNEL internally.
>   */
>  
> @@ -1638,7 +1640,6 @@ static struct socket *__sys_socket_create(int family, int type, int protocol)
>  struct file *__sys_socket_file(int family, int type, int protocol)
>  {
>  	struct socket *sock;
> -	struct file *file;
>  	int flags;
>  
>  	sock = __sys_socket_create(family, type, protocol);
> @@ -1649,11 +1650,7 @@ struct file *__sys_socket_file(int family, int type, int protocol)
>  	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
>  		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
>  
> -	file = sock_alloc_file(sock, flags, NULL);
> -	if (IS_ERR(file))
> -		sock_release(sock);
> -
> -	return file;
> +	return sock_alloc_file(sock, flags, NULL);
>  }
>  
>  int __sys_socket(int family, int type, int protocol)
> -- 
> 2.34.1
patchwork-bot+netdevbpf@kernel.org March 9, 2023, 7:30 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  7 Mar 2023 14:37:07 -0300 you wrote:
> When sock_alloc_file fails to allocate a file, it will call sock_release.
> __sys_socket_file should then not call sock_release again, otherwise there
> will be a double free.
> 
> [   89.319884] ------------[ cut here ]------------
> [   89.320286] kernel BUG at fs/inode.c:1764!
> [   89.320656] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [   89.321051] CPU: 7 PID: 125 Comm: iou-sqp-124 Not tainted 6.2.0+ #361
> [   89.321535] RIP: 0010:iput+0x1ff/0x240
> [   89.321808] Code: d1 83 e1 03 48 83 f9 02 75 09 48 81 fa 00 10 00 00 77 05 83 e2 01 75 1f 4c 89 ef e8 fb d2 ba 00 e9 80 fe ff ff c3 cc cc cc cc <0f> 0b 0f 0b e9 d0 fe ff ff 0f 0b eb 8d 49 8d b4 24 08 01 00 00 48
> [   89.322760] RSP: 0018:ffffbdd60068bd50 EFLAGS: 00010202
> [   89.323036] RAX: 0000000000000000 RBX: ffff9d7ad3cacac0 RCX: 0000000000001107
> [   89.323412] RDX: 000000000003af00 RSI: 0000000000000000 RDI: ffff9d7ad3cacb40
> [   89.323785] RBP: ffffbdd60068bd68 R08: ffffffffffffffff R09: ffffffffab606438
> [   89.324157] R10: ffffffffacb3dfa0 R11: 6465686361657256 R12: ffff9d7ad3cacb40
> [   89.324529] R13: 0000000080000001 R14: 0000000080000001 R15: 0000000000000002
> [   89.324904] FS:  00007f7b28516740(0000) GS:ffff9d7aeb1c0000(0000) knlGS:0000000000000000
> [   89.325328] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   89.325629] CR2: 00007f0af52e96c0 CR3: 0000000002a02006 CR4: 0000000000770ee0
> [   89.326004] PKRU: 55555554
> [   89.326161] Call Trace:
> [   89.326298]  <TASK>
> [   89.326419]  __sock_release+0xb5/0xc0
> [   89.326632]  __sys_socket_file+0xb2/0xd0
> [   89.326844]  io_socket+0x88/0x100
> [   89.327039]  ? io_issue_sqe+0x6a/0x430
> [   89.327258]  io_issue_sqe+0x67/0x430
> [   89.327450]  io_submit_sqes+0x1fe/0x670
> [   89.327661]  io_sq_thread+0x2e6/0x530
> [   89.327859]  ? __pfx_autoremove_wake_function+0x10/0x10
> [   89.328145]  ? __pfx_io_sq_thread+0x10/0x10
> [   89.328367]  ret_from_fork+0x29/0x50
> [   89.328576] RIP: 0033:0x0
> [   89.328732] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
> [   89.329073] RSP: 002b:0000000000000000 EFLAGS: 00000202 ORIG_RAX: 00000000000001a9
> [   89.329477] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f7b28637a3d
> [   89.329845] RDX: 00007fff4e4318a8 RSI: 00007fff4e4318b0 RDI: 0000000000000400
> [   89.330216] RBP: 00007fff4e431830 R08: 00007fff4e431711 R09: 00007fff4e4318b0
> [   89.330584] R10: 0000000000000000 R11: 0000000000000202 R12: 00007fff4e441b38
> [   89.330950] R13: 0000563835e3e725 R14: 0000563835e40d10 R15: 00007f7b28784040
> [   89.331318]  </TASK>
> [   89.331441] Modules linked in:
> [   89.331617] ---[ end trace 0000000000000000 ]---
> 
> [...]

Here is the summary with links:
  - net: avoid double iput when sock_alloc_file fails
    https://git.kernel.org/netdev/net/c/649c15c7691e

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/socket.c b/net/socket.c
index 6bae8ce7059e..9c92c0e6c4da 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -450,7 +450,9 @@  static struct file_system_type sock_fs_type = {
  *
  *	Returns the &file bound with @sock, implicitly storing it
  *	in sock->file. If dname is %NULL, sets to "".
- *	On failure the return is a ERR pointer (see linux/err.h).
+ *
+ *	On failure @sock is released, and an ERR pointer is returned.
+ *
  *	This function uses GFP_KERNEL internally.
  */
 
@@ -1638,7 +1640,6 @@  static struct socket *__sys_socket_create(int family, int type, int protocol)
 struct file *__sys_socket_file(int family, int type, int protocol)
 {
 	struct socket *sock;
-	struct file *file;
 	int flags;
 
 	sock = __sys_socket_create(family, type, protocol);
@@ -1649,11 +1650,7 @@  struct file *__sys_socket_file(int family, int type, int protocol)
 	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
 		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
 
-	file = sock_alloc_file(sock, flags, NULL);
-	if (IS_ERR(file))
-		sock_release(sock);
-
-	return file;
+	return sock_alloc_file(sock, flags, NULL);
 }
 
 int __sys_socket(int family, int type, int protocol)