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

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

[Eric Dumazet]

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(-)

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) {
-- 
2.36.0.512.ge40c2bad7a-goog

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

[Jakub Kicinski]

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?

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

[Eric Dumazet]

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.

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html