diff mbox series

[net] netlink: do not reset transport header in netlink_recvmsg()

Message ID 20220505161946.2867638-1-eric.dumazet@gmail.com (mailing list archive)
State Accepted
Commit d5076fe4049cadef1f040eda4aaa001bb5424225
Delegated to: Netdev Maintainers
Headers show
Series [net] netlink: do not reset transport header in netlink_recvmsg() | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 33 this patch: 33
netdev/cc_maintainers warning 1 maintainers not CCed: fw@strlen.de
netdev/build_clang success Errors and warnings before: 12 this patch: 12
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 31 this patch: 31
netdev/checkpatch warning WARNING: Possible repeated word: 'Google'
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Dumazet May 5, 2022, 4:19 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

netlink_recvmsg() does not need to change transport header.

If transport header was needed, it should have been reset
by the producer (netlink_dump()), not the consumer(s).

The following trace probably happened when multiple threads
were using MSG_PEEK.

BUG: KCSAN: data-race in netlink_recvmsg / netlink_recvmsg

write to 0xffff88811e9f15b2 of 2 bytes by task 32012 on cpu 1:
 skb_reset_transport_header include/linux/skbuff.h:2760 [inline]
 netlink_recvmsg+0x1de/0x790 net/netlink/af_netlink.c:1978
 sock_recvmsg_nosec net/socket.c:948 [inline]
 sock_recvmsg net/socket.c:966 [inline]
 __sys_recvfrom+0x204/0x2c0 net/socket.c:2097
 __do_sys_recvfrom net/socket.c:2115 [inline]
 __se_sys_recvfrom net/socket.c:2111 [inline]
 __x64_sys_recvfrom+0x74/0x90 net/socket.c:2111
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

write to 0xffff88811e9f15b2 of 2 bytes by task 32005 on cpu 0:
 skb_reset_transport_header include/linux/skbuff.h:2760 [inline]
 netlink_recvmsg+0x1de/0x790 net/netlink/af_netlink.c:1978
 ____sys_recvmsg+0x162/0x2f0
 ___sys_recvmsg net/socket.c:2674 [inline]
 __sys_recvmsg+0x209/0x3f0 net/socket.c:2704
 __do_sys_recvmsg net/socket.c:2714 [inline]
 __se_sys_recvmsg net/socket.c:2711 [inline]
 __x64_sys_recvmsg+0x42/0x50 net/socket.c:2711
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

value changed: 0xffff -> 0x0000

Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 32005 Comm: syz-executor.4 Not tainted 5.18.0-rc1-syzkaller-00328-ge1f700ebd6be-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/netlink/af_netlink.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Jakub Kicinski May 5, 2022, 4:57 p.m. UTC | #1 
On Thu,  5 May 2022 09:19:46 -0700 Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> netlink_recvmsg() does not need to change transport header.
> 
> If transport header was needed, it should have been reset
> by the producer (netlink_dump()), not the consumer(s).

Should I insert a reference to commit 99c07327ae11 ("netlink: reset
network and mac headers in netlink_dump()") when applying to give 
backporters an extra hint?
Eric Dumazet May 5, 2022, 5:05 p.m. UTC | #2 
On Thu, May 5, 2022 at 9:57 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu,  5 May 2022 09:19:46 -0700 Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > netlink_recvmsg() does not need to change transport header.
> >
> > If transport header was needed, it should have been reset
> > by the producer (netlink_dump()), not the consumer(s).
>
> Should I insert a reference to commit 99c07327ae11 ("netlink: reset
> network and mac headers in netlink_dump()") when applying to give
> backporters an extra hint?

I thought about that, but CBPF has no business with transport header.

I felt this would confuse things.
patchwork-bot+netdevbpf@kernel.org May 6, 2022, 10:50 p.m. UTC | #3 
Hello:

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

On Thu,  5 May 2022 09:19:46 -0700 you wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> netlink_recvmsg() does not need to change transport header.
> 
> If transport header was needed, it should have been reset
> by the producer (netlink_dump()), not the consumer(s).
> 
> [...]

Here is the summary with links:
  - [net] netlink: do not reset transport header in netlink_recvmsg()
    https://git.kernel.org/netdev/net/c/d5076fe4049c

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 05a3795eac8e9a7c8343460d9a41e0755a64c36e..73e9c0a9c187674cced15dbec079734489c3329f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1975,7 +1975,6 @@  static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 		copied = len;
 	}
 
-	skb_reset_transport_header(data_skb);
 	err = skb_copy_datagram_msg(data_skb, 0, msg, copied);
 
 	if (msg->msg_name) {